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.

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
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,19 @@ <T> Mono<T> load(String actorType, ActorId actorId, String stateName, TypeRef<T>
return Mono.empty();
}

if (!this.isStateSerializerDefault) {
if (s.length == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed only in the deserialize path?

Copy link
Contributor Author

@abogdanov37 abogdanov37 Jul 15, 2021

Choose a reason for hiding this comment

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

In another way, we'll get an exception as I remember correcly

return Mono.empty();
}
s = OBJECT_MAPPER.readValue(s, byte[].class);
if (type.getType().equals(String.class)) {
byte[] out = new byte[s.length + 2];
out[0] = '"';
Copy link
Member

Choose a reason for hiding this comment

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

This does not cover the scenarios where the string contains a quote already since it would not be escaped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quotes will be added in the serialization process. Give me please an example I'll add it in tests

Copy link
Member

Choose a reason for hiding this comment

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

my "string" with quotes

Copy link
Contributor Author

@abogdanov37 abogdanov37 Aug 12, 2021

Choose a reason for hiding this comment

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

I add two test cases for quoted strings. As I saw in debugging, additional quotes had been escaped with the serializer.

Copy link
Member

Choose a reason for hiding this comment

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

I am still concerned about this logic. This should not be needed. I will need more time to try this PR on my desktop first.

System.arraycopy(s, 0, out, 1, s.length);
out[out.length - 1] = '"';
s = out;
}
}
T response = this.stateSerializer.deserialize(s, type);
if (this.isStateSerializerDefault && (response instanceof byte[])) {
if (s.length == 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@

package io.dapr.actors.runtime;

import com.fasterxml.jackson.databind.ObjectMapper;
import io.dapr.actors.ActorId;
import io.dapr.client.ObjectSerializer;
import io.dapr.serializer.DaprObjectSerializer;
import io.dapr.serializer.DefaultObjectSerializer;
import io.dapr.utils.TypeRef;
import org.junit.Assert;
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,25 @@ public int hashCode() {

}

class CustomJsonSerializer extends ObjectSerializer implements DaprObjectSerializer{


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

@Override
public <T> T deserialize(byte[] data, TypeRef<T> type) throws IOException {
return super.deserialize(data, type);
}

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

@Test
public void happyCaseApply() {
DaprClient daprClient = mock(DaprClient.class);
Expand Down Expand Up @@ -197,6 +215,65 @@ public void happyCaseLoad() throws Exception {
provider.load("MyActor", new ActorId("123"), "emptyBytes", TypeRef.get(byte[].class)).block());
}

@Test
public void happyCaseLoadForCustomSerializer() throws Exception {
DaprClient daprClient = mock(DaprClient.class);
when(daprClient
.getState(any(), any(), eq("name")))
.thenReturn(Mono.just("\"Sm9uIERvZQ==\"".getBytes()));
when(daprClient
.getState(any(), any(), eq("zipcode")))
.thenReturn(Mono.just("\"OTgwMjE=\"".getBytes()));
when(daprClient
.getState(any(), any(), eq("goals")))
.thenReturn(Mono.just("\"OTg=\"".getBytes()));
when(daprClient
.getState(any(), any(), eq("balance")))
.thenReturn(Mono.just("\"NDYuNTU=\"".getBytes()));
when(daprClient
.getState(any(), any(), eq("active")))
.thenReturn(Mono.just("\"dHJ1ZQ==\"".getBytes()));
when(daprClient
.getState(any(), any(), eq("customer")))
.thenReturn(Mono.just("\"eyAiaWQiOiAxMDAwLCAibmFtZSI6ICJSb3hhbmUifQ==\"".getBytes()));
when(daprClient
.getState(any(), any(), eq("anotherCustomer")))
.thenReturn(Mono.just("\"eyAiaWQiOiAyMDAwLCAibmFtZSI6ICJNYXgifQ==\"".getBytes()));
when(daprClient
.getState(any(), any(), eq("nullCustomer")))
.thenReturn(Mono.empty());
when(daprClient
.getState(any(), any(), eq("bytes")))
.thenReturn(Mono.just("\"QQ==\"".getBytes()));
when(daprClient
.getState(any(), any(), eq("emptyBytes")))
.thenReturn(Mono.just(new byte[0]));

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(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
public void happyCaseContains() {
DaprClient daprClient = mock(DaprClient.class);
Expand Down