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 issue where custom JSON serializer generates Base64 encoded data in state store #539

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
+111 −4
Open

Fix issue where custom JSON serializer generates Base64 encoded data in state store #539

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
536ba0d
Fix breaking chsnges for deserialize states saved earlier.
May 3, 2021
4f87869
Merge branch 'master' into feature_fix-statestore-serialization
May 3, 2021
301587d
Add tests for custorm serializer
abogdanov37 May 5, 2021
199673a
Merge branch 'master' into feature_fix-statestore-serialization
artursouza Jun 3, 2021
5155e49
Merge branch 'master' into feature_fix-statestore-serialization
artursouza Jun 7, 2021
0886ff2
Merge branch 'master' of github.com:dapr/java-sdk into feature_fix-st…
abogdanov37 Jun 17, 2021
db1f822
Fix backward compatibility
abogdanov37 Jun 19, 2021
d73e06b
Fix deserialization by addin step to process Base64.Fix tests.
abogdanov37 Jun 21, 2021
8c08b82
Merge branch 'feature_fix-statestore-serialization' of github.com:abo…
abogdanov37 Jun 21, 2021
7153b41
Fix imports
abogdanov37 Jun 21, 2021
0e71cf5
Fix tests
abogdanov37 Jun 21, 2021
278c7ff
Merge branch 'master' into feature_fix-statestore-serialization
abogdanov37 Jun 21, 2021
bea49e8
Merge branch 'master' into feature_fix-statestore-serialization
abogdanov37 Jul 12, 2021
c735f66
Fix for quoted strings
Aug 12, 2021
9dc08d4
Fix name
Aug 12, 2021
2d2e162
Merge branch 'master' into feature_fix-statestore-serialization
abogdanov37 Aug 12, 2021
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
4 changes: 2 additions & 2 deletions sdk-actors/src/main/java/io/dapr/actors/ActorId.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
/**
* The ActorId represents the identity of an actor within an actor service.
*/
public class ActorId extends Object implements Comparable<ActorId> {
public class ActorId implements Comparable<ActorId> {

/**
* The ID of the actor as a String.
Expand All @@ -20,7 +20,7 @@ public class ActorId extends Object implements Comparable<ActorId> {
/**
* An error message for an invalid constructor arg.
*/
private final String errorMsg = "actor needs to be initialized with an id!";
private static final String errorMsg = "actor needs to be initialized with an id!";
Copy link
Contributor

Choose a reason for hiding this comment

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

Static final variables should be capitalized: ERROR_MSG


/**
* Initializes a new instance of the ActorId class with the id passed in.
Expand Down
14 changes: 9 additions & 5 deletions sdk-actors/src/main/java/io/dapr/actors/runtime/DaprStateAsyncProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import io.dapr.actors.ActorId;
import io.dapr.config.Properties;
import io.dapr.serializer.DaprObjectSerializer;
import io.dapr.serializer.DefaultObjectSerializer;
import io.dapr.utils.TypeRef;
import reactor.core.publisher.Mono;

Expand All @@ -27,6 +26,11 @@ class DaprStateAsyncProvider {
*/
private static final Charset CHARSET = Properties.STRING_CHARSET.get();

/**
* Marker to identify Json serializers.
*/
public static final String JSON_CONTENT_TYPE = "application/json";

/**
* Handles special serialization cases.
*/
Expand All @@ -45,7 +49,7 @@ class DaprStateAsyncProvider {
/**
* Flag determining if state serializer is the default serializer instead of user provided.
*/
private final boolean isStateSerializerDefault;
private final boolean isStateSerializerJson;

/**
* Instantiates a new Actor's state provider.
Expand All @@ -56,7 +60,7 @@ class DaprStateAsyncProvider {
DaprStateAsyncProvider(DaprClient daprClient, DaprObjectSerializer stateSerializer) {
this.daprClient = daprClient;
this.stateSerializer = stateSerializer;
this.isStateSerializerDefault = stateSerializer.getClass() == DefaultObjectSerializer.class;
this.isStateSerializerJson = JSON_CONTENT_TYPE.equals(stateSerializer.getContentType());
}

<T> Mono<T> load(String actorType, ActorId actorId, String stateName, TypeRef<T> type) {
Expand All @@ -69,7 +73,7 @@ <T> Mono<T> load(String actorType, ActorId actorId, String stateName, TypeRef<T>
}

T response = this.stateSerializer.deserialize(s, type);
if (this.isStateSerializerDefault && (response instanceof byte[])) {
if (this.isStateSerializerJson && (response instanceof byte[])) {
Copy link
Member

Choose a reason for hiding this comment

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

Changing this logic will make this not backwards compatible - it means that values saving with the current version of the code will not be able to be read from a newer version.

After thinking about this same problem in another PR, I think this should be done by a configuration. When registering the ActorType, a config can be set to determine is the data is written as Text or binary (Base64). The default behavior is Base64. This way, existing code does not break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! I understood your point. But we have a unit test that checks that binary processing is OK. See https://github.com/dapr/java-sdk/blob/master/sdk-actors/src/test/java/io/dapr/actors/runtime/DaprStateAsyncProviderTest.java#L166
I add this test for a custom serializer and it works well.
I can add a method to the serializer interface with the name useMimeType or checkMimeType and change behavior use this method

Copy link
Member

Choose a reason for hiding this comment

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

@abogdanov37 Adding a new method to the interface would also be a breaking change since apps would need to implement that new interface method after upgrading.

I thought about bringing this in as a configuration per Actor Type, which means that it would arrive in this class as a new argument in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@artursouza since Java 8 we have a default method. Because of that, it won't be breaking changes. I still can't imagine a case in which current changes will be breaking. Did you find this case with integration tests? May be beta tester give this information to you?
I have a real system with a custom serializer and many states stored in binary format. I try changed SDK for binary format. All works as expected.

Copy link
Member

Choose a reason for hiding this comment

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

@abogdanov37 There are 2 things here:

  1. Adding to the serializer interface might be done without breaking by using default - OK, but I would still be concerned over adding more to that interface. I would like to keep it as simple as possible. So, I would be OK if this setting was outside the serializer.

  2. This is a breaking change because data saved with the old code cannot be read with the new code - we don't have test for that yet, I am inferring this by looking at the code. If you can prove that data saved with the old code is read with the new, that would close this concern.

If not, I plan to address this in the next milestone. I am not 100% satisfied with our current serializer approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! Thanks for example. Sorry for my bad testing. I check only for objects but not for primitives (((

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found my place in my code where I convert Base64. I can reproduce this behavior in tests. Take a time to update code to avoid breaking changes. Thank you @artursouza!

Copy link
Member

Choose a reason for hiding this comment

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

@abogdanov37 You might want to enable the new behavior via a System property, this way, apps can opt in when they know it will not break them (new app, for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

        T response;
        try {
          response = this.stateSerializer.deserialize(s, type);
        } catch (IOException ex) {
          byte[] data = Arrays.copyOfRange(s, 1, s.length - 1);
          response = this.stateSerializer.deserialize(Base64.getDecoder().decode(data), type);
        }

this solution works for all types exclude String. Strings pass through as is (base64 encoded)
Feature flag is a great idea!
By System property you mean JVM property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@artursouza I push another solution based on "fair" conversion.
If we look there

java-sdk/sdk-actors/src/main/java/io/dapr/actors/runtime/DaprHttpClient.java

Line 98 in 0e5e311

generator.writeBinaryField("value", (byte[]) value);
we can see that we have a transformation to Base64
I add code to transform Base64 from store to origin byte array which can be processed.

if (s.length == 0) {
return Mono.empty();
}
Expand Down Expand Up @@ -138,7 +142,7 @@ Mono<Void> apply(String actorType, ActorId actorId, ActorStateChange... stateCha
try {
byte[] data = this.stateSerializer.serialize(stateChange.getValue());
if (data != null) {
if (this.isStateSerializerDefault && !(stateChange.getValue() instanceof byte[])) {
if (this.isStateSerializerJson && !(stateChange.getValue() instanceof byte[])) {
// DefaultObjectSerializer is a JSON serializer, so we just pass it on.
value = new String(data, CHARSET);
} else {
Expand Down
51 changes: 49 additions & 2 deletions sdk-actors/src/test/java/io/dapr/actors/runtime/DaprStateAsyncProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.junit.Test;
import reactor.core.publisher.Mono;

import java.io.IOException;
import java.util.Arrays;
import java.util.Objects;

Expand All @@ -27,8 +28,6 @@ public class DaprStateAsyncProviderTest {

private static final DaprObjectSerializer SERIALIZER = new DefaultObjectSerializer();

private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();

private static final double EPSILON = 1e-10;

/**
Expand Down Expand Up @@ -74,6 +73,28 @@ public int hashCode() {

}

class CustomJsonSerializer implements DaprObjectSerializer{
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a space before {

private final ObjectMapper OBJECT_MAPPER = new ObjectMapper();

@Override
public byte[] serialize(Object o) throws IOException {
return OBJECT_MAPPER.writeValueAsBytes(o);
}

@Override
public <T> T deserialize(byte[] data, TypeRef<T> type) throws IOException {
if (data.length == 0) {
return null;
}
return OBJECT_MAPPER.readValue(data, OBJECT_MAPPER.constructType(type.getType()));
}

@Override
public String getContentType() {
return "application/json";
}
}

@Test
public void happyCaseApply() {
DaprClient daprClient = mock(DaprClient.class);
Expand Down Expand Up @@ -195,6 +216,32 @@ public void happyCaseLoad() throws Exception {
provider.load("MyActor", new ActorId("123"), "bytes", TypeRef.get(byte[].class)).block());
Assert.assertNull(
provider.load("MyActor", new ActorId("123"), "emptyBytes", TypeRef.get(byte[].class)).block());

DaprStateAsyncProvider providerWithCustomJsonSerializer = new DaprStateAsyncProvider(daprClient, new CustomJsonSerializer());

Assert.assertEquals("Jon Doe",
providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "name", TypeRef.STRING).block());
Assert.assertEquals(98021,
(int) providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "zipcode", TypeRef.INT).block());
Assert.assertEquals(98,
(int) providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "goals", TypeRef.INT).block());
Assert.assertEquals(98,
(int) providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "goals", TypeRef.INT).block());
Assert.assertEquals(46.55,
(double) providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "balance", TypeRef.DOUBLE).block(),
EPSILON);
Assert.assertEquals(true,
(boolean) providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "active", TypeRef.BOOLEAN).block());
Assert.assertEquals(new Customer().setId(1000).setName("Roxane"),
providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "customer", TypeRef.get(Customer.class)).block());
Assert.assertNotEquals(new Customer().setId(1000).setName("Roxane"),
providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "anotherCustomer", TypeRef.get(Customer.class)).block());
Assert.assertNull(
providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "nullCustomer", TypeRef.get(Customer.class)).block());
Assert.assertArrayEquals("A".getBytes(),
providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "bytes", TypeRef.get(byte[].class)).block());
Assert.assertNull(
providerWithCustomJsonSerializer.load("MyActor", new ActorId("123"), "emptyBytes", TypeRef.get(byte[].class)).block());
}

@Test
Expand Down