Please update the Dynamic Data Objects library for your tests

[SeanSolberg]

You have a really nice comparison tool and we have used it to help us with improving our code. So thanks.

Please update the DynamicDataObjects library for your performance tests as the performance for duplicate slots is greatly improved to work within test cases that have a high number of fields in an object.

But the performance test you have is a bit janky. It really isn't a performance test on serializing or deserializing, but rather is a performance test that is centered around objects with a large number of fields in one object. Which is an extremely rare use-case for JSON. The problem with the test is it only tests string values so numerical value validation is never exercised, and the majority of the test is stressing performance around an object having many fields. The problem is that you are explicitly turning OFF the duplicate field searching and testing to make some of the libraries look faster, but then turning on the duplicate field searching when performing the serialization error testing to make them look testing complete. So when you compare some libraries that do properly perform duplicate field testing against other libraries that have that check turned off, you are getting great performance results for some libraries that are actually not performing the duplicate field checking. This is an issue for both generating and loading. The test should be a big file test, but not where you have thousands of fields with slightly different names because that is a non-realistic test case.

At least have all the libraries properly perform the duplicate field checking when doing the performance test. For the Generating and for the Loading tests, duplicate field checks should always be enabled for all libraries so they can properly be compared with each other. For example, in the McJson performance test, you have the following call:

procedure TMcJsonItem.fSetAsJSON(aValue: string);
begin
if (Self = nil) then Error(SItemNil, 'set as JSON');
// speed up = no key duplication verification (default)
Self.parse(aValue, True);
end;

Passing in "True" for the .Parse() call means that it is skipping duplicate field checking. Of course it will be faster than others that do properly do the check. Knowledge is freedom.

[hydrobyte]

Hi Sean,

Thank you for your contact and suggestions.

I will update the version of Dynamic Data Objects and its tests into the next release. Please wait a moment, as I need to resolve the issue of the expired Delphi 12 CE license.

But the performance test you have is a bit janky.

Well, these tests were initially created for my needs.

The problem is that you are explicitly turning OFF the duplicate field searching and testing to make some of the libraries look faster, but then turning on the duplicate field searching when performing the serialization error testing to make them look testing complete

Your criticisms are valid only for the "Generate" and "Load" sub-tests, but the reason for not checking for duplicates (as default in McJSON) lies in the origin of my requirements, where the data came from an SQL database for records that did not have duplicates. So, I respectfully don't accept your statement of "to make some of the libraries look faster". Please remember that the purpose of my McJSON unit is primarily focused on backward compatibility with very old versions of Delphi, C++Builder, and Lazarus. This is especially true because it only depends on TList internally.

In fact, the Disclaimer makes this clear right from the start:

Also, it is important to mention that the size of JSON objects used in tests here doesn't represent the most common cases of data manipulation in JSON.

On the other hand, I believe the "Validation" and "Open File" tests are more relevant and aligned with your point of view, especially the latter, which loads a 1 MByte JSON file heavily based on vector data.

I believe that a skilled developer will pay more attention to these tests than to the "Speed ​​Run".

In fact, it surprised me that the "Validation" test didn't generate as many comments among those who contacted us requesting improvements.

Knowledge (still) is freedom.

[SeanSolberg]

Awesome. Thanks for the comments. I super appreciate your efforts. By the way, love the validation tests. I would suggest adding one that contains duplicate field names.

[hydrobyte]

Sean,

Duplicated field names do not fit validation tests.

Check {"a":1, "a":2} with jsonlint.com for example. Its is valid and the duplication is shown as a warning:

Duplicate key found: "a". The last value will be used.

But I´ll include this as a matter of checking how Libs deal with duplication.

[SeanSolberg]

Agreed. Some libraries will accept the second value and some will produce an error or warning and not allow the second value. Both of these seem like an appropriate handling. However, for the libraries that skip the checking during a parsing operation, they internally simply add another second item to their internal object value container (such as TList like you mentioned) and then within the code that uses that parsed JSON structure, the receiver is none to the wiser that they probably cannot access the second value or even that it exists. Then, if it is re-serialized out to JSON, the duplicate field is probably re-written. It's pretty important for all libraries to properly check for the potential duplicates, both to explicitly handle parsing them and to prevent producing them.

IETF RFC 8259 (The Internet Standard)
The RFC is explicit about duplicate keys. It states that keys should be unique, and warns that duplicate keys cause unpredictable behavior across different programming languages.
From Section 4 (Objects): "The names within an object SHOULD be unique."
"An object whose names are not unique is causing interoperability problems in that software receiving such an object may perform differently or unpredictably..."

[hydrobyte]

Sean,

The RFC is explicit about duplicate keys.

I think you've pointed out the correct angle from which to discuss this issue of duplication: the standard.

In defense of my work with McJSON, it's worth highlighting that my library identifies duplicates as errors. Therefore, the optimization flag of not checking for duplicates is in accordance with your reasoning and also with IETF RFC 8259. For more details, search for "duplicate" in the McJSON's test project.

I also believe we're unnecessarily focusing on testing for duplicates in this situation, in reference to the "Speed ​​Run" test. Since your goal is to improve your library, especially considering the aspects of the tests I created, let's focus more on the "Validation" and "Open File" tests. Is that alright with you?

I'm updating your library and will run new tests to update the TestJSON project.

After updating your library and its respective tests, I will add a duplication sub-test to the "Validation" and re-evaluate all libraries in this regard. I should add that, since McJSON identifies these cases as errors, I believe it's on the user's side, which should actually be alerted to this type of problem.

[hydrobyte]

Sean,

Please check the latest commit made today. Your library has seen considerable improvements, but there are still some important points:

• The "Validation" test still needs attention.
• The "Open File" test still fails when attempting to display the JSON after reading the 1 MB file.

[SeanSolberg]

Wow, your banging on code over the weekend. Dedication.

One of the things we improved 6 months ago or so was the concept of being able to choose some of the parsing rules to either be holding tight to the JSON serializing spec or to allow some freedom to parse and handle some cases that are not perfectly compliant but are still reasonable to parse. These serialization settings are in the TJsonStreamer object. The defaults are set up to allow flexibility such as allowing single quotes, parsing symbols, parsing extended types, etc.

cDefaultAllowParsingSymbols = true;
cDefaultAllowSingleQuoteStrings = true;
cDefaultAllowParsingExtendedTypes = false;
cDefaultAllowOverwritingExistingSlots = true;
cDefaultAllowLeadingZero = true;
cDefaultSlotnameIsCaseSensitive = true;    // strict JSON is case sensitive.

If the tester should be as tight as possible on the rules, then we should modify the .Check call to turn those flexibilities off.

function TLibDynamicDataObjects.Check(const aCode: string): Boolean;
var
  lStreamer: TJsonStreamer;
begin
  Result := False;
  lStreamer := TJsonStreamer.create;
  try
    lStreamer.AllowParsingSymbols := false;
    lStreamer.AllowParsingExtendedTypes := false;
    lStreamer.AllowSingleQuoteStrings := false;
    lStreamer.AllowOverwritingExistingSlots := false;
    lStreamer.SlotnameIsCaseSensitive := true;
    lStreamer.AllowParsingLeadingZeros := false;
    lStreamer.JSON := aCode;
    lStreamer.Decode(fJson);
    Result := True;
  finally
    lStreamer.free;
  end;
end;

After turning off those serialization settings, the only rule failure should be #18. But we have to support stacking multiple sequential JSON serializations in series within a tstream and a string, so this is one rule we will probably never make strictly enforced.

[     0/   0]: Starting test
[     0/   0]:   Test   : Validation
[     0/   0]:   Library: DynamicDataObjects
[     0/   0]: 
[    16/  16]: FAIL = fail01.json = \x is not a valid escape character = [Exception] = Invalid character after encountering an escape "\" character. at 29
[    16/   0]: FAIL = fail02.json = Objects require colon between name/value = [Exception] = Invalid token parsing JSON at 17
[    31/  15]: FAIL = fail03.json = Objects do not have comma separators = [Exception] = Invalid token parsing JSON at 25
[    31/   0]: FAIL = fail04.json = Arrays don't have colon separators = [Exception] = Error parsing an array.  Expected ] at 25
[    31/   0]: FAIL = fail05.json = Truth is not a valid boolean value = [Exception] = Invalid token parsing JSON at 14
[    31/   0]: FAIL = fail06.json = Strings need double quotes, not single quotes = [Exception] = Single quotes are not allowed to start a JSON string. at 1
[    47/  16]: FAIL = fail07.json = Line break in a string value is not valid = [Exception] = Error Parsing a string. Control Characters not allowed in JSON string. at 6
[    47/   0]: FAIL = fail08.json = Escaped line break char is still not valid = [Exception] = Invalid character after encountering an escape "\" character. at 7
[    47/   0]: FAIL = fail09.json = Unclosed array = [Exception] = Error parsing an array.  Expected ] at 18
[    63/  16]: FAIL = fail10.json = Numbers require exponent if 'e' is there = [Exception] = Unable to convert text to floating point number at 1
[    63/   0]: FAIL = fail11.json = Only 1 sign char can precede the value = [Exception] = Unable to convert text to floating point number at 1
[    63/   0]: FAIL = fail12.json = Commas cannot close objects = [Exception] = Invalid token parsing JSON at 41
[    78/  15]: FAIL = fail13.json = Brackets must be matching = [Exception] = Error parsing an array.  Expected ] at 11
[    78/   0]: FAIL = fail14.json = Double quotes must be escaped = [Exception] = Error parsing an array.  Expected ] at 17
[    78/   0]: FAIL = fail15.json = Key string must be quoted = [Exception] = Invalid token parsing JSON at 1
[    94/  16]: FAIL = fail16.json = Arrays must not have comma after last value = [Exception] = Error parsing an array.  Expected ] at 15
[    94/   0]: FAIL = fail17.json = Arrays must have values between commas = [Exception] = Invalid token parsing JSON at 4
[    94/   0]: PASS = fail18.json = Nothing but whitespace can follow the root value
[   110/  16]: FAIL = fail19.json = Each opening bracket must be closed = [Exception] = Error parsing an array.  Expected ] at 17
[   110/   0]: FAIL = fail20.json = Extra comma after object = [Exception] = Error Parsing JSON. Missing closing } at 21
[   125/  15]: FAIL = fail21.json = Numbers cannot have leading 0s = [Exception] = Invalid JSON.  Not allowed to have leading zeros on numbers. at 40
[   125/   0]: FAIL = fail22.json = Numbers can't be hex encoded = [Exception] = Error Parsing JSON. Missing closing } at 27
[   141/  16]: FAIL = fail23.json = Decimal numbers need a digit before the dot = [Exception] = Invalid token parsing JSON at 0
[   141/   0]: FAIL = fail24.json = Object with repeated fieldname = [Exception] = NewSlot called with slotname="firstname" but this slot already existed in the DataFrame.
[   141/   0]: PASS = pass01.json = General large array testing valid values
[   156/  15]: PASS = pass02.json = Heavily nested array
[   156/   0]: PASS = pass03.json = Nested object
[   156/   0]: PASS = pass04.json = Simple string value
[   172/  16]: PASS = pass05.json = Unicode character string
[   172/   0]: PASS = pass06.json = From https://json.org/example.html
[   172/   0]: PASS = pass07.json = Test a number with a big E
[   188/  16]: Test finished

The "Open file" test relies on "toString" which is intentionally not implemented. This library is not specific to JSON serializing because it supports many different serializers. So the concept of "ToString" could be JSON formatted or formatted to other serializations. There is, however, a "PrintToString" which is NOT JSON, but rather a json-ish format that is really for debug printing, which we probably should map to the default TObject.ToString method so something is implemented. The core TDataObject supports data types that are not natively possible in JSON such as binary data, etc, which are published in this PrintToString method. It would be pretty easy to add this test support with the following code. Notice the three options, two of which are JSON formatted:

function TLibDynamicDataObjects.ToString: string;
begin
// result := fJson.PrintToString; // Use this for the documentation-orientated PrintToString
// result := fJson.JSONFormatted(2); // Use this for "human-readable" formatted JSON
result := fJson.JSON; // Use this for actual Tight JSON
end;

[SeanSolberg]

One other little thing since we are banging on details. For the Fail19 test, it seems that the test JSON doesn't match the description of the test case. The "Each opening bracket must be closed" is a good test, but the actual test JSON is that there is an additional character after the close of the root element. The final "]" should be moved to be a leading "[" to match the description of the actual test situation.

Each opening bracket must be closed
["Extra close"]]

should be...

Each opening bracket must be closed
[["Missing a close"]

[hydrobyte]

Sean,

Great, I think we can clear up these issues now.

Please just wait for the next commit where I will add the "Duplicated keys" tests. They will appear in the "Validation" section, but I created an extra category because the existing SHOULD in the Standard does not allow for a definitive answer about what is or is not wrong.

Next, I'll take your comments from here and apply them to TLibDynamicDataObjects.