SimpleJson fails with DateTime used as Dictionary key #5141
Conversation
I haven't checked json.net to the details. But at least using objects as a key will result in just calling ToString as well. I could see that we add some special handling about some common types but not really sure how many serializers properly handle custom types with comparison overloads for dictionary key columns? |
|
@timbussmann I checked this with Json.NET and the test I've added here passes. However, to handle an arbitrary type as the key, Json.NET requires a TypeConverter or JsonConverter to be written to handle that. So, that aspect can considered out of scope, but I think handling other primitive types as keys is a reasonable change to make. |
bording
force-pushed
the
simplejson-datetime-dictionary
branch
from
f347ae2 to
142a0c1
Compare
|
I've added a few additional tests and some potential solutions. I'll write some more information as line comments. |
| SerializeString(key.ToString(), builder); | ||
| { | ||
| // alternative approach: | ||
| // jsonSerializerStrategy.TrySerializeNonPrimitiveObject(key, out var keyValue); |
There was a problem hiding this comment.
this approach works too and can probably even handle Uri as keys (not tested though). It is quite a bit more expensive in case of "regular" key types like int etc, as it will go through the whole TrySerializeNonPrimiteObject path which also includes some reflection. This approach handles additional types besides DateTimes which are:
- Guids: they work already as proven by the test
- Enums: they don't work as the deserialization can't handle that
- Uris: current behavior not tested
There was a problem hiding this comment.
So what additional types do we gain from this?
There was a problem hiding this comment.
probably Uris :D or any other type which might be added to this logic in the future (unlikely though).
| // jsonSerializerStrategy.TrySerializeNonPrimitiveObject(key, out var keyValue); | ||
| // SerializeString((keyValue as string) ?? key.ToString(), builder); | ||
|
|
||
| switch (key) |
There was a problem hiding this comment.
downside of this approach is they duplicate DateTime serialization logic and that it only handles DateTimes, nothing else (wouldn't profit from types additionally supported by TrySerializeNonPrimiteObject.
|
@Particular/nservicebus-maintainers thoughts? |
|
If we do gain additional types with the first approach, then I say we go with that one. We're primarily using this in the learning transport, so perf isn't of the utmost importance. Handling more data types seems like the way to go. |
timbussmann
changed the title
|
I think we should go with the current approach. I left the comment about the alternative approach in there in case we have to revisit that code. Thoughts? |
Even after the changes @timbussmann made in #5131, it there are some other cases that aren't handled that I think we might want to support.
Currently, if a dictionary key isn't a string,
ToStringis called on it, and then it's serialized as a string:NServiceBus/src/NServiceBus.Core/Serializers/SimpleJson/SimpleJson.g.cs
Line 1070 in f347ae2
This breaks things like
DateTimesince it's not serialized to the correct format. I would think this almost always the wrong thing to do, since most types are not going to produce a valid string from calling `ToString' on it.