Dictionary>>#asJson does not produce valid JSON when using integer keys

[koendehondt]

Seaside version: 3.5.1

This is fine:

(Dictionary new at: 'test' put: 7; yourself) asJson
=> '{"test": 7}'

This is not ok, because object property names in JSON should be strings:

(Dictionary new at: 1 put: 7; yourself) asJson
=> '{1: 7}'

In older versions of Seaside, it was correct:

(Dictionary new at: 1 put: 7; yourself) asJson
=> '{"1":7}'

Back then, we had this code:

Object>>#asJson
	^ String streamContents: [ :stream | self jsonOn: stream ]

Dictionary>>#jsonOn: aStream
	JSJsonStream encodeDictionary: self on: aStream

JSJsonStream>>#encodeDictionary: aDictionary on: aStream 
	"Dictionary or hash-maps common structures in JSON to aStream."
	
	| first |
	first := true.
	aStream nextPut: ${.
	aDictionary keysAndValuesDo: [ :key :value |
		first
			ifTrue: [ first := false ]
			ifFalse: [ aStream nextPut: $, ].
		aStream 
			json: key greaseString;
			nextPut: $:;
			json: value ].
	aStream nextPut: $}

As you can see in the last method, key greaseString ensured that the object property key was a string.

In version 3.5.1, we have this code:

Object>>#asJson
	^ WAJsonCanvas builder render: [ :json |
		self jsonOn: json ]

Dictionary>>#jsonOn: aRenderer
	aRenderer object: [
		self keysAndValuesDo: [ :key :value |
			aRenderer key: key value: value ] ]

WAJsonCanvas>>#key: aKeyString value: aValueObject
	^ (self brush: (WAJsonKeyValueBrush key: aKeyString)) with: aValueObject

WAJsonKeyValueBrush>>#openBrush
	super openBrush.
	key jsonOn: canvas.
	self document stream nextPutAll: ': '

In the last method, key jsonOn: canvas generates JSON for the key. If the key is an integer, it will generate the integer, not a string as in older versions of Seaside. So this is a regression.

It gets worse when the key is an array.

(Dictionary new at: #(1 2 3) put: 7; yourself) asJson

results in an infinite loop. I would agree if one argues that this example is not a real use case, but if so, the code should signal an exception rather than looping infinitely. I did not try other types of object as dictionary keys. I found the above example by playing around after seeing the regression.

[jbrichau]

Hey @koendehondt ,

Instead of erroring, I opted to convert the key (1 2) to a string: '#(1 2)'.
Maybe this is better than erroring, since we also convert a numeric value to a string automatically again.
What do you think?

[koendehondt]

I think it is strange 😄, but it is a way to make it work for every object, even if it may not be very useful. That is why I suggested to signal an error in case the Smalltalk dictionary has keys that are not supported.

The result is that the array key will be transformed into a string that does not hold a valid JSON array, because #(1 2) is not valid JSON. [1, 2] is.

[jbrichau]

On second thought I am not happy with it either. The serialisation to a string is different on each platform so that also makes things dodgy. But mind that the key is always a string, and its contents should not be any valid JSON serialisation.

Throwing an error is probably the best course of action. But in that case a number should also error, otherwise we are not consistent. The keys in dictionaries you want to convert must then always be strings.

This is probably not something we can ship in a bugfix release since it changes the semantics. I'll have to see...

[koendehondt]

Throwing an error is probably the best course of action. But in that case a number should also error, otherwise we are not consistent.

I agree.

This is probably not something we can ship in a bugfix release since it changes the semantics.

Indeed.

[jbrichau]

I updated #1368
It introduces a flag to make the JSON writer error when the keys of a dictionary are not strings.

[koendehondt]

OK. Thanks @jbrichau.