Bring back JSON support to codegen by johanandren · Pull Request #269 · lightbend/kalix-jvm-sdk

johanandren · 2021-09-02T15:43:10Z

First stab at a possible new JSON support solution #263

More explicit and less magic (and dangerous), no new protobuf annotations needed, user code instead says exactly type to try to deserialize to.

If we agree on something like this I think what is missing is support for decoding heterogenous json types based on the "jsonType" in the type_url (since it is a single handler for all possible json subtypes). Perhaps something like .deserializeJsonAndHandle(subType, ..., decoded -> return effect for that type).

I think we should also drop the @Jsonable annotation if we go with this because all places we expect to serialize/deserialize is explicit so the annotation becomes superfluous.

patriknw · 2021-09-03T09:52:23Z

+   * @param <T> The type of the deserialized message
+   * @return The deserialized message
+   */
+  <T> T deserializeJson(Class<T> jsonClass, Any any);


since this is more like a utility function we don't have to provide it via the context, right? Place it in some helper class? JsonConverter? JsonSupport? JsonDecoder?

It currently needs access to the AnySupport instance that comes from the service to do the actual json deserialization. But perhaps it could be extracted from there, I'll look into that.

ah, I missed that part. I think AnySupport is per component so makes sense to add it to the context (or the Action base class)

I actually think the any support state is not needed, I'll make it a utility helper instead.

Done, now in JsonSupport.encode/decode

patriknw · 2021-09-03T09:56:25Z

+            LocalPersistenceEventing.Response.newBuilder()
+                .setId(actionContext().eventSubject().orElse(""))
+                .setMessage(jsonMessage.message)
+                .build());


did we have support for encoding JSON also? Like publishing a JSON message to a topic?

Would that be similar with Any as the response type and then explicit conversion?

We already have emitJson-effects in the value and event sourced entities, so I guess we can align with that. Not entirely sure I understand how publishing to a topic works and if that fits though, will look into that.

publishing to a topic is the returned message when proto defined with eventing.out

Sorry, I was wrong about that, emitJson was TCK action methods. After some more investigation any parameter or return value where you instead used a non proto Java class annotated with @Jsonable would be automagically serialized/deserialized into a proto any with the akka serverless json type_url.

patriknw · 2021-09-03T09:58:04Z

-        .setMessage(jsonMessage.message)
-        .build();
+  public Action.Effect<LocalPersistenceEventing.Response> processAnyEvent(Any any) {
+    JsonMessage jsonMessage = actionContext().deserializeJson(JsonMessage.class, any);


I like this explicit approach but I don't know how it was intended to work before and if there was some strong opinions about that it should be automagic and not be part of user's business logic code.

The alternative as I see it is with reflection out of the picture is to introduce protobuf annotations describing the possible Java classes that the json could be deserialized to and then generate methods for each of those types as well as logic to try to deserialize the right type in the handler. That would add more complexity here, require bigger changes all the way back to the protocol/annotations adding another case of stringly typed references to the Java codebase in the protobuf.

Unless I miss something we should focus on the json support for pub-sub. Since that is done via actions and the main responsibility of such actions is to translate between internal/external protocols I think it's a good fit to handle json there as well. Explicitly.

Ok, I may be missing the whole json support story here, but it isn't clear to me how that will look like for a generated action. The problem with this example from the TCK is that it's fully hand-coded.

For illustration, let's take the following protobuf fragment

rpc SayHello(Person) returns (Hello) {}

this will generate a method as

public Action.Effect<Hello> sayHello(Person person)

How would we know that when generating this Action, we will need to pass a String or Any and that the user will have to manually deserialize it to the right type?

So, the only way out of this to me is to say, if you want to pass json to an action, you have to encode it by hand yourself, but that means that user will have to generate a Provider and a Handler. And that bring us to the next question, how we will tell Akka Serverless how such an Action needs to be exposed? What are the endpoints?

It seems to me that whatever we do, a proto service definition is always needed and it's in the proto file that we will need to tell if the types will be ultimately serialized to json.

So if a user is building an action that subscribes to a topic, they will need to know what serialization format the messages are, and type their action input parameter as google.protobuf.Any (just like how it is now), then do an explicit deserialization to their expected internal type. We provide Jackson parsing json support out of the box to deserialize to pojos since that might be common enough.

If a user is publishing to a topic and do not want to use protobuf they will instead use google.protobuf.Any as return value, and have explicit logic to turn their object into the right serialized format. Again we provide explicit support to turn a pojo into json and then that into a google.protobuf.Any with the akka serverless json type-url.

If a user wants to produce or consume some other event encoding with an arbitrary third party library for serialization/deserialization they have access to the raw bytes from the google.protobuf.Any and can do that (not sure about how/if the proxy deals with that currently though).

The explicit deserialisation looks good to me too. And it feels like it will be clearer that the protobuf rpc method and the action implementation method both take Any.

johanandren · 2021-09-03T11:57:21Z

Digging a bit deeper:

The JSON support currently actually documented are the following two aspects as far as I can see:

Subscribing to a topic, defining the input type to google.protobuf.Any. This is needed since the producer could be non-akka-serverless and want to produce json.

https://developer.lightbend.com/docs/akka-serverless/java/topic-eventing.html#_receiving_json_messages

This is pretty much what I address in this PR, and since the format of the published data is in the control of the producer I'd expect that it could also be some other format than json, which means that an action/entity will need access to the raw bytes to be able to use a third party library to deserialize to the internal data format of the app.

Arbitrary use of JSON in place of probuf, no real actual use cases described in docs or samples. The TCK uses this to emit json events from an event sourced entity and a value entity to be consumed by in a topic subscriber and verify that the subscriber part works (but not actually replaying/applying json events.

Akka Serverless uses Jackson to serialize JSON. Any classes that are annotated with @Jsonable will be serialized to and from JSON using Jackson.

https://developer.lightbend.com/docs/akka-serverless/java/serialization.html#_json

I guess this part is more about thinking that Java devs want to avoid having to deal with the protobuf builder apis etc and work with pojos, but I'm not convinced it is a great idea to actually support - it essentially means sugar for an escape hatch using proto Any everywhere to store non-schema data/whatever.

Maybe I'm missing some strong motivation for this but I'd wish we take a step back from that idea and reconsider if it is worth it. Or for someone telling me how useful it is so that I change my mind.

patriknw · 2021-09-03T12:20:48Z

The TCK uses this to emit json events from an event sourced entity and a value entity to be consumed by in a topic subscriber

We don't support eventing.out (publish to topic) directly from an EventSourcedEntity or ValueEntity. It must go via a projection to an action. Therefore I don't see the need for emitting/storing events in json.

johanandren · 2021-09-03T12:38:59Z

Ok, I didn't find any docs on publishing to a topic so that's why it is a bit unclear to me. (Section "Publishing and subscribing to topics on a broker" only covers subscribing)

octonato · 2021-09-03T12:43:53Z

Subscribing to a topic, defining the input type to google.protobuf.Any. This is needed since the producer could be non-akka-serverless and want to produce json.

@johanandren, regarding this and what I commented here. It seems that the path here is to create a proto file that accepts google.protobuf.Any and subscribe to some topic. As an implementer of such an Action, I will need to know what will be encoded inside the Any and deserialize it myself. Correct?

johanandren · 2021-09-03T12:55:21Z

@octonato yes, that is my suggestion at least #269 (comment)

patriknw · 2021-09-03T13:18:50Z

Section "Publishing and subscribing to topics on a broker" only covers subscribing

That is issue #122

johanandren · 2021-09-03T13:26:47Z

To sum up what I'd like to do, after discussing this in this PR:

In addition to the json/grpc-web transcoding in front of the proxy already present for regular HTTP/json calls from clients
we will only support json to and from pub/sub-topics since that is potential communication with the outside world where a different serialized encoding than Protobuf is needed (and potentially out of our control if it is about consuming a topic).

It is likely useful that users can also support an arbitrary format here (say XML, YAML, Avro, CSV, CBOR) with some third party library, which means access to the raw bytes is likely good.

Because of that we drop all other automagic encoding to json in favour of explicit methods for dealing with transformation from and to google.protobuf.Any instances with the json string and the type_url json.akkaserverless.com/....

When deserializing consumed messages it is not likely that the outside knows/should know about the internal Java classes of the service, so trying to parse the specific class out of the type_url suffix is not useful, however, it will be useful to be able to use that suffix to discern heterogenous types (when it is not possible to have a common Java super type that Jackson understands for all different types of event in one topic).

When serializing we can use the class name for this suffix, but since knowing the fully qualified name of a class in the service introduces tight coupling, it would make more sense if this was something symbolic/defined by the user where serialization happens.

Since we know that a handler on an action is for consuming/publishing to a topic and is specified to use com.protobuf.Any we can generate some hints about the explicit transformation api in the generated code for such action handlers to guide users to get it right.

Since the TCK relies on projections to test JSON support, I think we'll need to change the TCK to instead just call a few topic-subscriber methods directly with json to verify that it works.

@pvlugter, @retgits would be good with your feedback here if I'm missing something important around the previous json support.

johanandren · 2021-09-06T15:14:19Z

Ready for final review.

patriknw

LGTM, something minor...

patriknw · 2021-09-06T15:30:06Z

+          // note: the somewhat funky indenting is on purpose to lf+indent only if comment present
+          if (cmd.inFromTopic && cmd.inputType.fullQualifiedName == "com.google.protobuf.Any")
+            """|// JSON input from a topic can be decoded using JsonSupport.decodeJson(MyClass.class, any)
+               |  """.stripMargin


patriknw · 2021-09-06T15:32:45Z

+
+  public static final String AKKA_SERVERLESS_JSON = "json.akkaserverless.com/";
+
+  private static final ObjectMapper objectMapper = new ObjectMapper();


oh, we should probably have a better configured ObjectMapper by default. Maybe even use some subset of Akka's configuration mechanism for Jackson. I'll create an issue.

patriknw · 2021-09-06T15:36:51Z

          .withEnv("TCK_SERVICE_HOST", "host.testcontainers.internal")
-          .withCommand(
-              "-Dakkaserverless.tck.ignore-tests.0=eventing") // FIXME disabled because of lack of
-          // JSON support


patriknw reviewed Sep 3, 2021

View reviewed changes

johanandren force-pushed the wip-json-support-codegen branch from a55f806 to a93c050 Compare September 6, 2021 11:40

johanandren added 3 commits September 6, 2021 13:41

First stab at a possible new JSON support solution #263

b56ded9

AnySupport test changes for non auto json

4c3450c

Only explicit JSON encode/decode, tck passing.

6850b16

johanandren force-pushed the wip-json-support-codegen branch from a93c050 to 6850b16 Compare September 6, 2021 11:42

johanandren added 2 commits September 6, 2021 14:40

Detect and add comment hint about JSON encoding/decoding

bd66192

No more jsonable in tck

d6583f3

johanandren mentioned this pull request Sep 6, 2021

java-eventing-shopping-cart #277

Closed

More codegen tests that I missed

8a17158

johanandren marked this pull request as ready for review September 6, 2021 14:27

johanandren mentioned this pull request Sep 6, 2021

java-eventing-shopping-cart with codegen #282

Merged

patriknw approved these changes Sep 6, 2021

View reviewed changes

patriknw mentioned this pull request Sep 6, 2021

Jackson ObjectMapper settings #283

Closed

johanandren added 2 commits September 6, 2021 17:50

It's in the name of the class

412e2b3

Formatting

4900a59

johanandren requested a review from octonato September 7, 2021 09:23

patriknw merged commit d35860f into sdk-codegen-dev Sep 7, 2021

patriknw deleted the wip-json-support-codegen branch September 7, 2021 09:33


		public static final String AKKA_SERVERLESS_JSON = "json.akkaserverless.com/";

		private static final ObjectMapper objectMapper = new ObjectMapper();

Conversation

johanandren commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanandren commented Sep 3, 2021

Uh oh!

patriknw commented Sep 3, 2021

Uh oh!

johanandren commented Sep 3, 2021

Uh oh!

octonato commented Sep 3, 2021

Uh oh!

johanandren commented Sep 3, 2021

Uh oh!

patriknw commented Sep 3, 2021

Uh oh!

johanandren commented Sep 3, 2021

Uh oh!

johanandren commented Sep 6, 2021

Uh oh!

patriknw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johanandren commented Sep 2, 2021 •

edited

Loading