Fixed issue in digital twins sample in deserializing into offsetDateTime #16938
Conversation
OffsetDateTime wasn't be deserialized into correctly because the mapper saw the property as a string. It isn't ideal, but defining the type in our DigitalTwinPropertyMetadata as a String instead of an OffsetDateTime at least lets our sample run as expected. Also make the BasicDigitalTwinMetadata's property metadata map contain DigitalTwinPropertyMetadata values instead of object values
timtay-microsoft
requested review from
abhipsaMisra,
azabbasi,
barustum,
bikamani,
drwill-ms and
vinagesh
as code owners
ghost
added
the
| */ | ||
| public class DigitalTwinPropertyMetadata { | ||
| @JsonProperty(value = DigitalTwinsJsonPropertyNames.METADATA_PROPERTY_LAST_UPDATE_TIME, required = true) | ||
| private OffsetDateTime lastUpdatedOn; |
There was a problem hiding this comment.
The default mapper we use in our SDK couldn't deserialize into this type after it has already been deserialized into a string, from what I can tell. We do use this datatype in our DigitalTwinsModelData class and it deserializes fine there, but I can't get the same behavior here. Since this is basically sample code, making this a string should be fine
There was a problem hiding this comment.
when you say this is sample code, do you mean this class is only used in samples?
There was a problem hiding this comment.
where is it deserialized into a string?
I second Basel's question, this is not just sample code, it's helper class that we expose to users.
Can we involve the java folks in the SDK team to help us figure it out?
There was a problem hiding this comment.
@srnagar any thoughts here? In our convenience layer, we try to deserialize a map<string, object> and a few layers down there is a date time. In the map, it presents as a string, but we know it is an OffsetDateTime.
If this class has OffsetDateTime instead of String, when we convert into this DigitalTwinPropertyMetadata class we get
Exception in thread "main" java.lang.IllegalArgumentException: Cannot construct instance of java.time.OffsetDateTime (no Creators, like default constructor, exist): no String-argument constructor/factory method to deserialize from String value ('2020-10-28T23:06:28.8057025Z')
Is something lost if the date time was already deserialized once from bytes into this map<String, Object> ?
There was a problem hiding this comment.
when you say this is sample code, do you mean this class is only used in samples?
I mean to say that while we expose this as a helper class, we expect users to write their own deserialization classes. We aren't explicitly telling people to use these classes in their production code
| * Gets the date and time the property was last updated. | ||
| * @return The date and time the property was last updated. | ||
| */ | ||
| public OffsetDateTime getLastUpdatedOnOffsetDateTime() { |
There was a problem hiding this comment.
To compensate for not deserializing the type as a string directly, I added this method so users can still get this value as a datetime offset
| ConsoleLogger.print("Retrieved component for digital twin " + basicDigitalTwinId + " :"); | ||
| for (String key : getComponentResponse.getContents().keySet()) { | ||
| ConsoleLogger.print("\t" + key + " : " + getComponentResponse.getContents().get(key)); | ||
| ConsoleLogger.print("\t\tLast updated on: " + getComponentResponse.getMetadata().get(key).getLastUpdatedOnOffsetDateTime()); |
There was a problem hiding this comment.
I added this to the component sample to demonstrate getting this property metadata field
There was a problem hiding this comment.
I came across the same issue when I was running the samples to verify JsonPatchDocument
| */ | ||
| public OffsetDateTime getLastUpdatedOn() { | ||
| public String getLastUpdatedOn() { | ||
| return lastUpdatedOn; |
There was a problem hiding this comment.
Can't this method still return OffsetDateTime, and in the method body do:
return OffsetDateTime.parse(lastUpdatedOn)
There was a problem hiding this comment.
I tried that, and it caused some complications during deserialization of this field. Essentially, jackson requres a setter and getter of the same type as the property. And since the property could only be read as a String, I had to make the setter and getter a String as well
drwill-ms
left a comment
drwill-ms
left a comment
There was a problem hiding this comment.
I know we're close to shipping and pressure is high. You know I feel that.
That said, this feels like a bad example. Even if it was a code sample I'd feel it should be fixed (at least soon after shipping). Given this is in the client library, we should get help from Azure SDK Java experts to try to help us fix this.
|
Closing this PR as I found out why the mapper in our client couldn't parse date times from strings |
|
#16975 is the correct solution |
OffsetDateTime wasn't be deserialized into correctly because the mapper saw the property as a string. It isn't ideal, but defining the type in our DigitalTwinPropertyMetadata as a String instead of an OffsetDateTime at least lets our sample run as expected.
Also make the BasicDigitalTwinMetadata's property metadata map contain DigitalTwinPropertyMetadata values instead of object values