VBA-tools/VBA-JSON

Poor data types

Deedolith opened this issue · 8 comments

As it stand, the library is barely usable as it is extremly easy to mess up.

  • Almost no data types are handled other than Object or Variant.
  • Possibility to insert any non JSON related data,

Consider the following code (within Excel):

Public Sub test()
    Dim Data As Scripting.Dictionary
    Set Data = JsonConverter.ParseJson("{""value"" : ""a string""}")    '// Ok
    
    Data.Add "range_1", ThisWorkbook.Worksheets(1).Range("A1")    '// No sens, a Range is not a valid JSON data
    
    ThisWorkbook.Worksheets(1).Range("A2") = "some value"
    Data.Add "range_2", ThisWorkbook.Worksheets(1).Range("A2")    '// No sens, a Range is not a valid JSON data
    
    Dim TestData As Scripting.FileSystemObject
    Set TestData = New Scripting.FileSystemObject 
    Data.Add "TestData", TestData        '// No sens, FileSystemObject is not a valid JSON data
    
    Dim Output As String
    Output = JsonConverter.ConvertToJson(Data)
    Debug.Print Output
End Sub

WORST:
It does not fail.

Personally, I have used this project for more than 5 years in both Excel and Access and have not had any major issues with using the module, but I also have been programming in VBA for well over 10 years.

  • Did you read the project readme?
  • Do you understand the JSON data format?
  • Do you understand how to program in VBA and the difference between objects, collections and dictionaries?
  • Did you include the reference for Scripting.Dictionary in your VBA project?

Those might be places to start your learning experience, rather than venting your frustrations.

A Scripting.FileSystemObject is not a valid JSON data type. Valid JSON data types are numbers, strings, arrays, objects, true, false and null. So adding it to the Scripting.Dictionary as a value causes a reference to the object to be added, which cannot be converted into a valid JSON data type.

Skipping over that error in the debugger, the output printed to my immediate window is:

{"value":"a string","range_1":22,"range_2":"some value"}

I had the number value 22 in cell $A1, which looks correct to me given your code added range_1 and range_2.

I do agree that when calling ConvertToJson that it should generate the null value for any value in a VBA.collection or Scripting.Dictionary when that value is a reference to an object other than a VBA.collection or a Scripting.Dictionary, rather than producing invalid JSON. In this case a property with any value. However, if you placed a proper JSON data type into the Scripting.Dictionary you would not have had that issue. Not an excuse, just saying...

FYI, when using Scripting.Dictionary beware of the caveat using the item property either implicitly or explicitly:

If key is not found when changing an item, a new key is created with the specified newitem. If key is not found when attempting to return an existing item, a new key is created and its corresponding item is left empty.

BTW, you can also edit ConvertToJson, correct the issue in your VBA project, then push the change to the project.

I am highlighting the fact that the library allow to do illegal things.
And from experience, if you allow code users to do illegal things, sooner or later they will do it.

The only "fix" I can see is to redesign everything, with strong data types.

The library already uses strong types that are found in VBA, e.g., string, integer, long, boolean, collection, dictionary. The issue you had is an oversite that has to do with handling references to unknown objects, which IMHO, should be converted to JSON null or to throw an error. So the library doesn't need to be completely redesigned, just used properly, with a minor fix to correct the oversight of handling unknown object references and in the interim can be avoid by proper programming until a fix can be implemented. If you feel the library really needs to be completely redesigned, then have at it, and create a new open source project that showcases your ideas.

Afraid you missed the point:

The library do not throw any error on invalid assignations when it should.
In consequences, it can generate invalid JSON output.
This behavior is due to its inhability to discriminate valid data types from invalid ones. Object, Variant, Collection and Dictionary data types are simply not strong enough (ehence poor data types).

Create a new open source project that showcases your ideas

Actually, I forgot to mention your assertion about invalid assignments in your original code comments. In my original post, I opened up Excel, went to the developer tab, opened Visual Basic, created a new model in the default spreadsheet and pasted your code directly from your original post, added a reference for Scripting.Dictionary, then stepped through it in the debugger and tested variables in the immediate window. These lines did not produce errors as you indicated:

    Data.Add "range_1", ThisWorkbook.Worksheets(1).Range("A1")    '// No sens, a Range is not a valid JSON data
    Data.Add "range_2", ThisWorkbook.Worksheets(1).Range("A2")    '// No sens, a Range is not a valid JSON data
  1. The best I can tell from your code is that you are using ThisWorkbook.Worksheets(1).Range("A1") which uses the default accessor on the Range object. Using the default accessor on a COM object is not normally an issue and the Range object should assign the value of the cell at A1. However, I have experienced in some older versions of Excel, you didn't mention which one you were using, that using the default accessor sometimes causes an error for no apparent reason and by explicitly stating which property on the object you want eliminates the issue. For this reason, I never use the default accessors in my VBA code and always explicitly state the property I want.

  2. You have a misunderstanding of what is being controlled by VBA objects and the VBA-JSON library. When you write the code:

    Dim Data As Scripting.Dictionary
    Set Data = JsonConverter.ParseJson("{""value"" : ""a string""}")    '// Ok

    the Data variable is a VBA Scripting.Dictionary object, it has nothing to do with the VBA-JSON library. The VBA-JSON library is just returning a standard VBA object that it used to convert the parsed JSON. When you do your assignments, which you indicate they fail for some odd reason, that would be a problem with the VBA Scripting.Dictionary object and not the VBA-JSON library. The VBA-JSON library does not return its own specialty objects for each JSON datatype. It could, but instead the author decided to return standard VBA objects. That is the way the library was designed and works.

    It sounds like you would have implemented your own specialty objects for JSON datatypes. There are trade offs in doing so, mostly in increased memory consumption and decreased performance since they would be implemented in VBA. Microsoft has optimized the VBA.Collection and Scripting.Dictionary COM objects to be performant. You could implement your own specialty objects for JSON datatypes as COM objects, but I personally wouldn't load them because COM objects are a security risk since you don't know whether they implement malware, viruses or are mining bitcoin on your computer. Having the VBA-JSON library use standard VBA objects and provide the source module that basically maps JSON datatypes to VBA datatypes and back again gives me comfort from an auditing and security standpoint. Lastly, there are many different ways to solve any problem, none of them are either right or wrong, they all make tradeoffs in areas like, space, time, finance, security, etc. Your way isn't the only way it could or should be done.

  3. You have a misunderstanding of how the VBA-JSON library works. The VBA-JSON library maps all JSON datatypes to standard VBA datatypes:

    JSON VBA
    string String, Variant where it contains a string or can be coerced to a string
    number Double, Single, Integer, Long, Variant where VBA.isNumeric(value) is true
    object Scripting.Dictionary
    array VBA.Collection
    true True,Boolean
    false False,Boolean
    null Nothing
  4. Yes, you stumbled upon a minor issue with the VBA-JSON library, that requires a fix. The workaround is to not assign object references to the values in the Scripting.Dictionary hash unless they are references to VBA.Collection or Scripting.Dictionary. I don't want to minimize the point that there is a minor issue with the library, but it doesn't require a rewrite of the entire library into your way of thinking how the library should work, as you indicated in your initial post. Again, as I and @Steve-Sexton indicated, feel free to create a new open source project that showcases your ideas. Maybe your implementation will be better in some way that causes us all to switch, but right now the VBA-JSON library is sufficient for working with JSON in VBA and is performant for most needs within the limitations of being an in-memory implementation.

    Generally, most people using the VBA-JSON library understand the above mapping and wouldn't assign an object reference other than VBA.Collection or Scripting.Dictionary since it would not be a valid JSON datatype. I suggested initially that the VBA-JSON library should replace these unknown references with null or throw an error, but on further thought it could also interrogate the COM object for its exposed properties and treat it as a reference to a Scripting.Dictionary. Would have to think some more about that, e.g., what do you do with nested or recursive objects and obviously COM methods cannot be converted, but then they wouldn't be in NodeJS either using JSON.stringify. I would be fine with these unknown references just being converted to null for now.

From now on, my work in progress is within my repositories.

I won't comment everything since you tend to focus on a particular use case: the sample code I posted, wich is just that: A sample (don't confuse it with an issue I am experiencing).

Generally speaking, any decent library must fail on illegal operation, either:

  • Raise an error or exception.
  • Return a False indicator notifying the failure of the operation.
    Choose your favorit behavior.

As a user, I am performing black-box tests, and I am looking at the services the VBA-JSON library offer, to name a few:

  • Parse a JSON file and return an object hierarchy representing the data held within the file.
  • Build or update an object hierarchy representing JSON data.
  • Obtain a string representation of the object hierarchy.

On the 2nd point, there are issues, such as allowing illegal operations (to refer back to the sample I posted: an illegal assignment. And Excel.Range or Scripting.FileSystemObject data types are particular use cases, the same behavior can be observed with any "VBA Object" data type). The concequences are invalid results (upating the result to make it valid isn't a fix, it will only hide the issue).

As for the general "fix" you suggest, which looks a lot like "Good practices and discipline", it's a wishful thinking, alas our world do not run around this concept.