feat: JSON schema evolution support #1760

aludwiko · 2023-08-08T13:02:58Z

reference #1332

aludwiko · 2023-08-08T13:04:54Z

build.sbt

@@ -126,7 +126,7 @@ lazy val javaSdkProtobufTestKit = project

 lazy val javaSdkSpring = project
  .in(file("sdk/java-sdk-spring"))
-  .dependsOn(javaSdkProtobuf)
+  .dependsOn(javaSdkProtobuf % "compile->compile;test->test")


I'm reusing some test classes.

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JacksonMigration.java

aludwiko · 2023-08-08T13:09:37Z

sdk/java-sdk-spring/src/main/scala/kalix/javasdk/impl/JsonMessageCodec.scala

    //TODO verify if this could be replaced by sth smarter/safer
-    reversedCache.compute(
+    addToReversedCache(clz, typeName)


While updating typeHints map, I'm also updating another map reversedTypeHints, this feels wrong, but I haven't figured out a better option for that.

If I recall correctly, we register the types when we register the components. This is supposed to be done by a single thread running over all components and finding out the types. In that sense, it's safe to update the main map and then the reversed one.

For good measure, we could synchronize this method to make sure we are safe. Just in case something changes one day.

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JacksonMigration.java

aludwiko · 2023-08-09T09:02:38Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JsonSupport.java

+   * @return The decoded object
+   * @throws IllegalArgumentException if the given value cannot be decoded to a T
+   */
+  public static <T> T decodeJson(Class<T> valueClass, Any any, Optional<? extends JacksonMigration> jacksonMigration) {


A jacksonMigration additional param to pass the migration from the upper level, otherwise I would need to move the @ Migration annotation here.

Make the signature with Optional param internal and just have one overload with migration and one without, for Java?

I thought about this, but then all the invocation would look like:

extractMigration(typeClass) .map(JsonSupport.decodeJson(typeClass, any, _)) .getOrElse(JsonSupport.decodeJson(typeClass, any))

which is also smelly.

I have some serious doubts about this signature. I want to show serialization test examples for the user. From the test perspective, I need to pass the migration explicitly:

JsonSupport.decodeJson(AddressChanged.class, serialized, Optional.of(new AddressChangedMigration()))

the test can be green, but the in the actual implementation the linking @Migration(AddressChangedMigration.class) annotation might be missing.

The main question is if we want to support migration for JSON in Proto SDK, like here: https://docs.kalix.io/java-protobuf/actions-publishing-subscribing.html#_json If that's the case then I would move not only @ Migration annotation but also @ TypeName, which is even more realistic use case.

I thought user API for proto would skip the reflection-migration-lookup and be that the user supplies migrations manually, for that it just seems messy that they'd need to deal with optional, but for the reflective may-have-migration use I understand why it needs to be optional.

Another thought: if we support annotation-detection of migration also for proto, can we just hide it inside the JsonSupport instead and the user doesn't have to pass anything?

"I thought user API for proto would skip the reflection-migration-lookup" - in some sense, it will, because the user will just use the public static <T> T decodeJson(Class<T> valueClass, Any any) { method. Although nothing will prevent him to use the signature with migration.

"can we just hide it inside the" - exactly that's my point one method without any extra params. The "only" drawback is that some code-first annotations will be from the java-sdk-spring and some (@Migration, @TypeName ?) from the java-sdk-protobuf.

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JsonSupport.java

aludwiko · 2023-08-11T10:05:44Z

PR is ready for the first review round.

johanandren

Did an initial skim-review, looking good

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JacksonMigration.java

johanandren · 2023-08-15T13:22:21Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JsonSupport.java

+   * @return The decoded object
+   * @throws IllegalArgumentException if the given value cannot be decoded to a T
+   */
+  public static <T> T decodeJson(Class<T> valueClass, Any any, Optional<? extends JacksonMigration> jacksonMigration) {


Make the signature with Optional param internal and just have one overload with migration and one without, for Java?

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JsonSupport.java

patriknw

looking good, recognising much from Akka (which is a good thing)

patriknw · 2023-08-16T07:49:21Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JsonMigration.java

@@ -0,0 +1,68 @@
+/*
+ * Copyright 2021 Lightbend Inc.


chore: bumping the year

it's auto-generated by the plugin, I could bump the configuration and update all files, but in a separate PR

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JsonSupport.java

patriknw · 2023-08-16T08:12:16Z

sdk/java-sdk-protobuf/src/test/java/kalix/javasdk/DummyClassRenamed.java

+  public Optional<String> optionalStringValue;
+
+  @JsonCreator
+  public DummyClassRenamed(@JsonProperty("stringValue") String stringValue, @JsonProperty("intValue") int intValue, @JsonProperty("optionalStringValue") Optional<String> optionalStringValue) {


are these JsonProperty needed if you have javacOptions += "-parameters"?

nope, removed

aludwiko · 2023-08-18T13:23:25Z

I will create a separate PR with the documentation.

johanandren

LGTM, I think the protobuf side usage can be revisited in the future and is good enough for now.

octonato

Late for the party, but I think there are a few things that we should change before releasing it. See my comments.

octonato · 2023-08-28T14:22:15Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JsonSupport.java

+   * @return The decoded object
+   * @throws IllegalArgumentException if the given value cannot be decoded to a T
+   */
+  public static <T> T decodeJson(Class<T> valueClass, Any any, Optional<? extends JsonMigration> jacksonMigration) {


Arriving late for the party, but I think we should make this one private. No need to augment the API surface with a method receiving Optional<JsonMigration>.

Each time we call this method, we are calling MigrationExtractor.extractMigration and passing the same valueClass we are receiving as the first parameter. So we can simply remove this method and to this call internally.

It is a problematic situation. Initially, this method was private, but to make it private I would need to move the @Migration to java-sdk-protobuf.

I think it's better to have the Migration annotation in the java-sdk-protobuf package, no?

Or do we plan to use this method in other contexts?

The decodeJson method is also used when consuming json from a topic, but in such case, I doubt that it will be useful to pass a migration class as the type URL won't have the version number attached to it.

octonato · 2023-08-28T14:55:43Z

sdk/java-sdk-spring/src/it/java/com/example/wiring/SpringSdkIntegrationTest.java

+        .params(777));
+
+    Thread.sleep(60000);


Is that a leftover? Better to wrap it with await and assert the return. Or get the state and assert it is 777

Yeah, just ignore it, I removed it in some other PRs.

octonato · 2023-08-28T14:59:22Z

sdk/java-sdk-spring/src/it/java/com/example/wiring/SpringSdkIntegrationTest.java

    String entityId = "hello1";
    execute(componentClient.forEventSourcedEntity(entityId)
-      .call(CounterEntity::increase)
-      .params(42));
+        .call(CounterEntity::increase)
+        .params(42));


Strange that this is working previous method increased hello1 to 777 and here we are using the same entity id. What am I missing?

probably the order of tests is different than the order in the class

But that has the potential to introduce flakiness. Better to use a different entityId.

Also, I don't see how that could even work. If the ordering is different, it should also fail. One of the tests must fail, not clear why both are passing. 🤷

Because one of them is not checking anything, I was testing sth and I forgot to delete it. It is fixed on the main branch.

octonato · 2023-08-28T15:15:49Z

sdk/java-sdk-spring/src/main/scala/kalix/javasdk/impl/JsonMessageCodec.scala

    //TODO verify if this could be replaced by sth smarter/safer
-    reversedCache.compute(
+    addToReversedCache(clz, typeName)


If I recall correctly, we register the types when we register the components. This is supposed to be done by a single thread running over all components and finding out the types. In that sense, it's safe to update the main map and then the reversed one.

For good measure, we could synchronize this method to make sure we are safe. Just in case something changes one day.

octonato · 2023-08-28T15:21:12Z

sdk/java-sdk-spring/src/test/scala/kalix/javasdk/impl/JsonMessageCodecSpec.scala

+    "decode with new schema version" in {
+      val encoded = messageCodec.encodeJava(SimpleClass("abc", 10))
+      val decoded =
+        JsonSupport.decodeJson(classOf[SimpleClassUpdated], encoded, Optional.of(new SimpleClassUpdatedMigration))


I guess we are adding this extra method receiving Optional<JsonMigration> because we are using classes that belong to the Protobuf package and we don't have access to MigrationExtractor.extractMigration nor the Migration annotation.

IMO, this is another reason to remove this method. We should not increase the public API just because we want to reuse some classes.

octonato · 2023-08-28T15:25:52Z

One more thing...

I see comments about Protobuf migration. If we plan to do it, we should build another abstraction.

The class name is JsonMigration and it depends on Jackson classes (JsonNode). Not something that we want or can reuse for Protobuf payloads.

feat: JSON schema evolution support

8623fc1

aludwiko linked an issue Aug 8, 2023 that may be closed by this pull request

Java code-first SDK: support for Akka's Jackson migration #1332

Closed

github-actions bot added java-sdk java-sdk-protobuf kalix-runtime Runtime and SDKs sub-team labels Aug 8, 2023

aludwiko commented Aug 8, 2023

View reviewed changes

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JacksonMigration.java Outdated Show resolved Hide resolved

aludwiko commented Aug 8, 2023

View reviewed changes

fmt

5a58508

github-actions bot added the Documentation Improvements or additions to the documentation label Aug 9, 2023

documentation + moving @migration to the rest of annotations

634a3b3

aludwiko force-pushed the 1332-java-code-first-sdk-support-for-akkas-jackson-migration branch from f99a007 to 634a3b3 Compare August 9, 2023 09:00

github-actions bot removed the Documentation Improvements or additions to the documentation label Aug 9, 2023

aludwiko commented Aug 9, 2023

View reviewed changes

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JacksonMigration.java Outdated Show resolved Hide resolved

aludwiko commented Aug 9, 2023

View reviewed changes

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JsonSupport.java Outdated Show resolved Hide resolved

aludwiko added 4 commits August 9, 2023 11:30

fixing event handlers with version

9a80230

handling migration for ES subscriptions

e9b1968

fixing tests

b4e2a5f

minor improvements

2fcd76a

aludwiko marked this pull request as ready for review August 11, 2023 10:05

fixing test

a88d5a1

johanandren reviewed Aug 15, 2023

View reviewed changes

aludwiko added 2 commits August 15, 2023 17:28

ranaming to JsonMigration

207c8c0

removing unused classes

5529ca7

patriknw reviewed Aug 16, 2023

View reviewed changes

aludwiko added 3 commits August 17, 2023 16:07

replacing split to substring

f3f5d70

JsonProperties removed

df96c1d

workflow serialization integration test

9d32311

fmt

8845087

johanandren approved these changes Aug 21, 2023

View reviewed changes

aludwiko merged commit 00297b0 into main Aug 21, 2023
65 checks passed

aludwiko deleted the 1332-java-code-first-sdk-support-for-akkas-jackson-migration branch August 21, 2023 12:04

octonato reviewed Aug 28, 2023

View reviewed changes

feat: JSON schema evolution support #1760

feat: JSON schema evolution support #1760

Conversation

aludwiko commented Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aludwiko Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aludwiko Aug 15, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aludwiko commented Aug 11, 2023

johanandren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw 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

aludwiko commented Aug 18, 2023

johanandren left a comment

Choose a reason for hiding this comment

octonato 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

Choose a reason for hiding this comment

octonato commented Aug 28, 2023

aludwiko commented Aug 8, 2023 •

edited

Loading

aludwiko Aug 9, 2023 •

edited

Loading

aludwiko Aug 15, 2023 •

edited

Loading