`Json` produces invalid JSON output for integer keys

[LinqLover]

Dictionary new
    at: 1 put: 2;
    asJsonString --> '{1:2}'

What would be the expected behavior?

• '{"1":2}' ⭐
• raise an error: invalid key for json

What in this case?

Dictionary new
    at: 1 put: 2;
    at: '1' put: '2';
    asJsonString

• '{"1":2}'
• '{"1":"2"}' ⭐
• raise an error: duplicate keys

My tendency would be the variants with a star ...

[timrowledge]

Looking at https://www.json.org/json-en.html & https://jsonlint.com/ I think the expectation is that the key must be a string and the value must be the json-ised value, so

Dictionary new
    at: 1 put: 2;
    asJsonString 

should result in an error.

Consider the first EBNF diagram on https://www.json.org/json-en.html - it can be simplified for this case to { string : jsonised-value}.

Since we're exceptionally clever we could consider doing "make the key be a string of whatever we have" except that trying to reliably convert non-string keys might open up entire pallet-loads of worms. What if somebody is making dictionaries where the keys are complex objects?

Might also want to check on STON as well (https://wiki.squeak.org/squeak/6504). If we're attempting to produce sometihng that meets a standard we should probably actually meet the standard...

[codefrau]

The second should raise an error. Our dictionaries do not preserve insertion order so we don't even know which one is the duplicate.

For the first case where it's unambiguous I'd find it more convenient to allow this and automatically convert the key to a string, just like Association>> jsonWriteOn: does (there is an argument for removing this method though, because unlike all other methods it does not by itself produce valid JSON)

[LinqLover]

I'm personally rather with Tim on that. Let's not bring the joy of [object Object] to Squeak. :D I'm personally more a fan of EAFP than LBYL, but silent invalidation of data just makes debugging harder IMO. I even wonder whether JsonObject should reject any non-string keys during construction and manipulation (e.g., add:) in the first place. However, the latter idea might impact compatibility (but seriously, the name JsonObject conveys a pretty clear idea what types of keys might be allowed there.)

just like Association>> jsonWriteOn: does (there is an argument for removing this method though, because unlike all other methods it does not by itself produce valid JSON)

Oh yes indeed. I placed an #isThisEverCalled in this method in my image and if we do not identify any surprising usages, I support removing this method (or deprecating it at least).

[tonyg]

IMO JsonObject was a mistake :-) but rejecting non-string keys there would make sense. For Dictionary, the best we can do is signal an error when trying to write a non-string key as JSON. But we should definitely do that rather than silently emit non-JSON. I do not like the idea of automagic conversion to strings: this seems guaranteed to produce unpleasant surprises downstream, and also might result in (technically legal JSON!!) monstrosities like {"1": 2, "1": 2} for the case where a Dictionary has a key of 1 and another of '1'.

To be specific wrt the test cases from the top post here: the first case should signal an error during asJsonString, and the second should signal an error when trying to render the key 1. In both cases this would be some kind of "invalid JSON dictionary key type" error.

[LinqLover]

There are other kinds of invalid keys, such as String cr, that we should detect and treat equivantly.

[tonyg]

There are other kinds of invalid keys, such as String cr, that we should detect and treat equivantly.

That's not an invalid key: it should serialize to

{ "\r": 123 }

for example. So long as it's a valid JSON string, it should be a valid JSON key.

[LinqLover]

There are other kinds of invalid keys, such as String cr, that we should detect and treat equivantly.

That's not an invalid key: it should serialize to

{ "\r": 123 }

for example. So long as it's a valid JSON string, it should be a valid JSON key.

My bad. I tested it in JavaScript and Python but forgot to escape the backslash ...