Fix digital twins client not deserializing date times correctly #16975
Conversation
Also fix BasicDigitalTwinMetadata to use the DigitalTwinPropertyMetadata class
timtay-microsoft
requested review from
abhipsaMisra,
azabbasi,
barustum,
bikamani,
drwill-ms and
vinagesh
as code owners
ghost
added
the
|
|
||
| JacksonAdapter jacksonAdapter = new JacksonAdapter(); | ||
| mapper = jacksonAdapter.serializer(); // Use the same mapper in this layer that the generated layer will use | ||
| stringModule.addSerializer(new DigitalTwinsStringSerializer(String.class, mapper)); |
There was a problem hiding this comment.
The issue was that we used a different mapper in our convenience layer than we did in our generated layer. The generated layer one (from jacksonAdapter.serializer()) has a lot of useful modules registered already, including a module for parsing of date times. By simply using the generated layer's adapter in this layer as well, this issue is solved
There was a problem hiding this comment.
Note that this mapper is not to be confused with the custom serializer that users can provide. If the user provides a custom serializer, then this mapper won't be used in the convenience layer
|
|
||
| // This mapper gets used to deserialize a digital twin that has a date time within a property metadata, so it | ||
| // needs to have this module in order to correctly deserialize that date time | ||
| mapper.registerModule(new JavaTimeModule()); |
There was a problem hiding this comment.
This is the module that was missing from our default mapper in the digital twins client. We get this module for free when we construct the JacksonAdapter and call .serializer() on that jacksonAdapter. We need to explicitly use it here, though
|
|
||
| JacksonAdapter jacksonAdapter = new JacksonAdapter(); | ||
| mapper = jacksonAdapter.serializer(); // Use the same mapper in this layer that the generated layer will use | ||
| stringModule.addSerializer(new DigitalTwinsStringSerializer(String.class, mapper)); |
There was a problem hiding this comment.
Do we still need this string serializer? Are there any APIs that still operate on Strings? I thought we got rid of all those, and everything was objects now.
There was a problem hiding this comment.
That was the plan back when we were going to force APIs to take in a <T extends IDigitalTwin>, but those plans fell through, so we're back to supporting strings again. It would be more work to remove them at this point then I think it would be worth
There was a problem hiding this comment.
Got it, so now we are back to the original state where we support <T> as String as input for these APIs.
Also fix BasicDigitalTwinMetadata to use the DigitalTwinPropertyMetadata class
Also mark DigitalTwinPropertyMetadata as fluent and final as per Azure SDK feedback
For some context:
#16938