Skip to content
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

Fix digital twins client not deserializing date times correctly #16975

Merged
merged 5 commits into from
Oct 29, 2020
Merged

Fix digital twins client not deserializing date times correctly #16975

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d8e5a6
Fix digital twins client not deserializing date times correctly
timtay-microsoft Oct 29, 2020
88df582
fixup
timtay-microsoft Oct 29, 2020
8f21ef7
fixup
timtay-microsoft Oct 29, 2020
a4973e8
Revert "fixup"
timtay-microsoft Oct 29, 2020
fdfdb83
fixup
timtay-microsoft Oct 29, 2020
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
4 changes: 3 additions & 1 deletion ...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 @@ -3,6 +3,7 @@

package com.azure.digitaltwins.core;

import com.azure.core.annotation.Fluent;
import com.azure.digitaltwins.core.models.DigitalTwinsJsonPropertyNames;
import com.fasterxml.jackson.annotation.JsonProperty;

Expand All @@ -11,7 +12,8 @@
/**
* Contains metadata about changes on properties on a digital twin or component.
*/
public class DigitalTwinPropertyMetadata {
@Fluent
public final class DigitalTwinPropertyMetadata {
@JsonProperty(value = DigitalTwinsJsonPropertyNames.METADATA_PROPERTY_LAST_UPDATE_TIME, required = true)
private OffsetDateTime lastUpdatedOn;

Expand Down
6 changes: 4 additions & 2 deletions ...-digitaltwins-core/src/main/java/com/azure/digitaltwins/core/DigitalTwinsAsyncClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.module.SimpleModule;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import reactor.core.publisher.Mono;

import java.util.ArrayList;
Expand Down Expand Up @@ -53,17 +54,18 @@
@ServiceClient(builder = DigitalTwinsClientBuilder.class, isAsync = true)
public final class DigitalTwinsAsyncClient {
private static final ClientLogger logger = new ClientLogger(DigitalTwinsAsyncClient.class);
private static final ObjectMapper mapper = new ObjectMapper();
private static ObjectMapper mapper;
private final DigitalTwinsServiceVersion serviceVersion;
private final AzureDigitalTwinsAPIImpl protocolLayer;
private static final Boolean includeModelDefinitionOnGet = true;
private final JsonSerializer serializer;

DigitalTwinsAsyncClient(String serviceEndpoint, HttpPipeline pipeline, DigitalTwinsServiceVersion serviceVersion, JsonSerializer jsonSerializer) {
final SimpleModule stringModule = new SimpleModule("String Serializer");
stringModule.addSerializer(new DigitalTwinsStringSerializer(String.class, mapper));

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));
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Got it, so now we are back to the original state where we support <T> as String as input for these APIs.

jacksonAdapter.serializer().registerModule(stringModule);

this.serviceVersion = serviceVersion;
Expand Down
12 changes: 11 additions & 1 deletion ...-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 @@ -13,10 +13,15 @@
import com.azure.identity.ClientSecretCredentialBuilder;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;

import javax.net.ssl.HttpsURLConnection;
import java.io.IOException;
import java.util.*;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Random;
import java.util.function.Function;

public class ComponentSyncSamples {
Expand Down Expand Up @@ -46,6 +51,10 @@ public static void main(String[] args) throws IOException {
.setLogLevel(parsedArguments.getHttpLogDetailLevel()))
.buildClient();

// 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());
Copy link
Member Author

Choose a reason for hiding this comment

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

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


runComponentSample();
}

Expand Down Expand Up @@ -145,6 +154,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).getLastUpdatedOn());
}

// Clean up
Expand Down