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

abogdanov37 · 2021-05-04T08:14:36Z

Description

Refactor ObjectSerializer class to add ability set custom object mapper.
Refactor DaprStateAsyncprovider class to use custom serializers based on ObjectSerializer class as Json serializer.
Add unit tests.

Issue reference

#503

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

halspang

Code seems fine in general, just some formatting callouts. My concern here is how this fits into the larger picture. Especially in regards to the conversation that happened on the issue and how it relates to #509.

We also have an IT failure which should be looked into.

@artursouza What are your thoughts?

halspang · 2021-05-05T17:30:47Z

sdk-actors/src/main/java/io/dapr/actors/ActorId.java

@@ -20,7 +20,7 @@
  /**
   * 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!";


Static final variables should be capitalized: ERROR_MSG

halspang · 2021-05-05T17:31:20Z

sdk-actors/src/test/java/io/dapr/actors/runtime/DaprStateAsyncProviderTest.java

@@ -74,6 +73,28 @@ public int hashCode() {

  }

+  class CustomJsonSerializer implements DaprObjectSerializer{


Missing a space before {

artursouza

@abogdanov37 Thanks for your contribution.

My main concern is backwards compatibility. This time, I think I found a way out, please, see my comment.

@halspang Yes, we need to be mindful about the vision to move default serializer out of this package. This improvement (IMO) does not invalidate that.

artursouza · 2021-05-05T18:31:21Z

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

@@ -69,7 +73,7 @@
        }

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


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.

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

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

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

@abogdanov37 There are 2 things here:

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.

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.

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

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!

@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).

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?

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

…atestore-serialization

…gdanov37/java-sdk into feature_fix-statestore-serialization

artursouza · 2021-07-15T00:04:52Z

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

@@ -68,6 +68,19 @@
          return Mono.empty();
        }

+        if (!this.isStateSerializerDefault) {
+          if (s.length == 0) {


Why is this needed only in the deserialize path?

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

artursouza · 2021-07-15T00:05:34Z

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

+          s = OBJECT_MAPPER.readValue(s, byte[].class);
+          if (type.getType().equals(String.class)) {
+            byte[] out = new byte[s.length + 2];
+            out[0] = '"';


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

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

my "string" with quotes

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

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.

codecov · 2021-08-12T22:05:54Z

Codecov Report

Merging #539 (2d2e162) into master (df54a5d) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #539      +/-   ##
============================================
- Coverage     81.03%   80.98%   -0.05%     
- Complexity      991      995       +4     
============================================
  Files            88       88              
  Lines          3042     3051       +9     
  Branches        332      335       +3     
============================================
+ Hits           2465     2471       +6     
- Misses          409      412       +3     
  Partials        168      168

Impacted Files	Coverage Δ
...k-actors/src/main/java/io/dapr/actors/ActorId.java	`100.00% <ø> (ø)`
...io/dapr/actors/runtime/DaprStateAsyncProvider.java	`69.64% <100.00%> (+8.77%)`	⬆️
sdk/src/main/java/io/dapr/utils/Retry.java	`50.00% <0.00%> (-25.00%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df54a5d...2d2e162. Read the comment docs.

cicoyle · 2023-12-21T15:16:41Z

gentle ping - @abogdanov37 mind resolving the conflicts and addressing the ^ comments?

cicoyle · 2024-02-19T21:52:07Z

gentle ping - @abogdanov37

Andrey Bogdanov and others added 3 commits May 3, 2021 23:59

Fix breaking chsnges for deserialize states saved earlier.

536ba0d

Merge branch 'master' into feature_fix-statestore-serialization

4f87869

Add tests for custorm serializer

301587d

halspang reviewed May 5, 2021

View reviewed changes

artursouza requested changes May 5, 2021

View reviewed changes

Merge branch 'master' into feature_fix-statestore-serialization

199673a

artursouza self-assigned this Jun 3, 2021

Merge branch 'master' into feature_fix-statestore-serialization

5155e49

artursouza added breaking-change do-not-merge labels Jun 14, 2021

abogdanov37 added 8 commits June 17, 2021 18:26

Merge branch 'master' of github.com:dapr/java-sdk into feature_fix-st…

0886ff2

…atestore-serialization

Fix backward compatibility

db1f822

Fix deserialization by addin step to process Base64.Fix tests.

d73e06b

Merge branch 'feature_fix-statestore-serialization' of github.com:abo…

8c08b82

…gdanov37/java-sdk into feature_fix-statestore-serialization

Fix imports

7153b41

Fix tests

0e71cf5

Merge branch 'master' into feature_fix-statestore-serialization

278c7ff

Merge branch 'master' into feature_fix-statestore-serialization

bea49e8

abogdanov37 requested review from a team as code owners July 12, 2021 11:47

artursouza requested changes Jul 15, 2021

View reviewed changes

Andrey Bogdanov and others added 3 commits August 13, 2021 00:58

Fix for quoted strings

c735f66

Fix name

9dc08d4

Merge branch 'master' into feature_fix-statestore-serialization

2d2e162

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

abogdanov37 commented May 4, 2021

halspang left a comment

halspang May 5, 2021

halspang May 5, 2021

artursouza left a comment

artursouza May 5, 2021

abogdanov37 May 6, 2021

artursouza May 6, 2021

abogdanov37 May 6, 2021

artursouza May 7, 2021

abogdanov37 Jun 17, 2021

abogdanov37 Jun 17, 2021

artursouza Jun 18, 2021

abogdanov37 Jun 19, 2021

abogdanov37 Jun 21, 2021

artursouza Jul 15, 2021

abogdanov37 Jul 15, 2021 •

edited

Loading

artursouza Jul 15, 2021

abogdanov37 Jul 15, 2021

artursouza Jul 28, 2021

abogdanov37 Aug 12, 2021 •

edited

Loading

artursouza Aug 14, 2021

codecov bot commented Aug 12, 2021

cicoyle commented Dec 21, 2023

cicoyle commented Feb 19, 2024

		@@ -74,6 +73,28 @@ public int hashCode() {

		}

		class CustomJsonSerializer implements DaprObjectSerializer{

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

Are you sure you want to change the base?

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

Conversation

abogdanov37 commented May 4, 2021

Description

Issue reference

Checklist

halspang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artursouza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abogdanov37 Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abogdanov37 Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 12, 2021

Codecov Report

cicoyle commented Dec 21, 2023

cicoyle commented Feb 19, 2024

abogdanov37 Jul 15, 2021 •

edited

Loading

abogdanov37 Aug 12, 2021 •

edited

Loading