Skip to content

Fixed issue in digital twins sample in deserializing into offsetDateTime #16938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
timtay-microsoft wants to merge 1 commit into from
Closed

Fixed issue in digital twins sample in deserializing into offsetDateTime #16938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ...digitaltwins-core/src/main/java/com/azure/digitaltwins/core/BasicDigitalTwinMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public final class BasicDigitalTwinMetadata {
private String modelId;

@JsonIgnore
private final Map<String, Object> propertyMetadata = new HashMap<>();
private final Map<String, DigitalTwinPropertyMetadata> propertyMetadata = new HashMap<>();

/**
* Creates an instance of digital twin metadata.
Expand Down Expand Up @@ -56,7 +56,7 @@ public BasicDigitalTwinMetadata setModelId(String modelId) {
* @return The metadata about changes on properties on a component.
*/
@JsonAnyGetter
public Map<String, Object> getPropertyMetadata() {
public Map<String, DigitalTwinPropertyMetadata> getPropertyMetadata() {
return propertyMetadata;
}

Expand All @@ -68,7 +68,7 @@ public Map<String, Object> getPropertyMetadata() {
* @return The BasicDigitalTwin object itself.
*/
@JsonAnySetter
public BasicDigitalTwinMetadata addPropertyMetadata(String key, Object value) {
public BasicDigitalTwinMetadata addPropertyMetadata(String key, DigitalTwinPropertyMetadata value) {
this.propertyMetadata.put(key, value);
return this;
}
Expand Down
14 changes: 11 additions & 3 deletions ...italtwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinPropertyMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@
*/
public class DigitalTwinPropertyMetadata {
@JsonProperty(value = DigitalTwinsJsonPropertyNames.METADATA_PROPERTY_LAST_UPDATE_TIME, required = true)
private OffsetDateTime lastUpdatedOn;
Copy link
Member Author

@timtay-microsoft timtay-microsoft Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@barustum barustum Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you say this is sample code, do you mean this class is only used in samples?

Copy link
Contributor

@azabbasi azabbasi Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@timtay-microsoft timtay-microsoft Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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> ?

Copy link
Member Author

@timtay-microsoft timtay-microsoft Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private String lastUpdatedOn;

/**
* Gets the date and time the property was last updated.
* @return The date and time the property was last updated.
*/
public OffsetDateTime getLastUpdatedOn() {
public String getLastUpdatedOn() {
return lastUpdatedOn;
Copy link

@barustum barustum Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this method still return OffsetDateTime, and in the method body do:
return OffsetDateTime.parse(lastUpdatedOn)

Copy link
Member Author

@timtay-microsoft timtay-microsoft Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

Expand All @@ -28,8 +28,16 @@ public OffsetDateTime getLastUpdatedOn() {
* @param lastUpdatedOn The date and time the property was last updated.
* @return The DigitalTwinPropertyMetadata object itself.
*/
public DigitalTwinPropertyMetadata setLastUpdatedOn(OffsetDateTime lastUpdatedOn) {
public DigitalTwinPropertyMetadata setLastUpdatedOn(String lastUpdatedOn) {
this.lastUpdatedOn = lastUpdatedOn;
return this;
}

/**
* Gets the date and time the property was last updated.
* @return The date and time the property was last updated.
*/
public OffsetDateTime getLastUpdatedOnOffsetDateTime() {
Copy link
Member Author

@timtay-microsoft timtay-microsoft Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return OffsetDateTime.parse(lastUpdatedOn);
}
}
1 change: 1 addition & 0 deletions ...-digitaltwins-core/src/samples/java/com/azure/digitaltwins/core/ComponentSyncSamples.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ public static void runComponentSample() throws JsonProcessingException {
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());
Copy link
Member Author

@timtay-microsoft timtay-microsoft Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to the component sample to demonstrate getting this property metadata field

Copy link
Contributor

@azabbasi azabbasi Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch !

Copy link
Contributor

@azabbasi azabbasi Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came across the same issue when I was running the samples to verify JsonPatchDocument

}

// Clean up
Expand Down