chore: Cleanup JsonSupport #105
Conversation
patriknw
commented
Dec 18, 2024
patriknw
commented
- deprecate json in pb Any
- add encode/decode of json bytes
* deprecate json in pb Any * add encode/decode of json bytes
| @@ -96,7 +98,10 @@ private JsonSupport() { | |||
| * for the JSON type instead. | |||
| * | |||
| * @see {{encodeJson(T, String}} | |||
| * | |||
| * @deprecated Protobuf Any with JSON is not supported | |||
There was a problem hiding this comment.
Does it still make sense for message broker messages?
I'd say that then it's better to use a more explicit protobuf message that carry the raw bytes and type hint.
When we add "real" protobuf serialization support I think we should revisit this. Would be better to have the serialization support as something that is injected rather than static methods.
There was a problem hiding this comment.
if we should add protobuf as internal serialization format at all, we can trigger it by the fact that there is a Java protobuf message base types, that way we wont need explicit user serialization like this.
BTW I think we can drop these completely, as there is no way to use them that makes sense in the new SDK, they exist because of basing off of old SDK but they could never really be used here.
The only use case where this class can be used for anything useful right now, afaics, is to do streaming JSON encoding for results in a query in a HTTP endpoint.
There was a problem hiding this comment.
I think protobuf can make sense when we add gRCP endpoints. Since that will happen later I think we can leave these as deprecated now and introduce proper serialization support at that point (and then remove these here).
| @@ -117,7 +117,7 @@ private[akka] final case class RequestBuilderImpl[R]( | |||
| override def withRequestBody(`object`: AnyRef): RequestBuilder[R] = { | |||
| if (`object` eq null) throw new IllegalArgumentException("object must not be null") | |||
| try { | |||
| val body = JsonSupport.encodeToBytes(`object`).toByteArray | |||
| val body = JsonSupport.encodeToAkkaByteString(`object`) | |||
There was a problem hiding this comment.
wasn't this even wrong? encodeToBytes expected it to be in protobuf encoding?
| public static <T> akka.util.ByteString encodeToAkkaByteString(T value) { | ||
| try { | ||
| return akka.util.ByteString.fromArrayUnsafe(objectMapper.writerFor(value.getClass()).writeValueAsBytes(value)); | ||
| } catch (JsonProcessingException ex) { |
There was a problem hiding this comment.
wrapping the JsonProcessingException is not fully compatible change, but very inconvenient to have that as checked exception, and it's not like that in other places
| @@ -15,9 +14,9 @@ | |||
| class CustomerEventSerializationTest { | |||
| @@ -96,7 +98,10 @@ private JsonSupport() { | |||
| * for the JSON type instead. | |||
| * | |||
| * @see {{encodeJson(T, String}} | |||
| * | |||
| * @deprecated Protobuf Any with JSON is not supported | |||
There was a problem hiding this comment.
if we should add protobuf as internal serialization format at all, we can trigger it by the fact that there is a Java protobuf message base types, that way we wont need explicit user serialization like this.
BTW I think we can drop these completely, as there is no way to use them that makes sense in the new SDK, they exist because of basing off of old SDK but they could never really be used here.
The only use case where this class can be used for anything useful right now, afaics, is to do streaming JSON encoding for results in a query in a HTTP endpoint.
| ArrayNode dynamicJson = objectNode.putArray(key); | ||
| values.forEach(v -> dynamicJson.add(v.toString())); | ||
| return akka.util.ByteString.fromArrayUnsafe(objectMapper.writeValueAsBytes(objectNode)); | ||
| public static akka.util.ByteString encodeDynamicCollectionToAkkaByteString(String key, Collection<?> values) { |
There was a problem hiding this comment.
Quite strange that this ever was public API I think it was mostly methods we needed ourselves internally that accidentally became public API. We should reconsider that and limit the surface to intentionally public API.
There was a problem hiding this comment.
deprecated those, and changed to internal JsonSerializer: c9f7b3b
| @@ -17,7 +15,7 @@ class CustomerEventSerializationTest { | |||
| @Test | |||
| public void shouldDeserializeWithMandatoryField() { | |||
| //given | |||
| Any serialized = JsonSupport.encodeJson(new CustomerEvent.NameChanged("andre")); | |||
| ByteString serialized = JsonSupport.encodeToAkkaByteString(new CustomerEvent.NameChanged("andre")); | |||
There was a problem hiding this comment.
This doesn't work, because I think we are loosing some migration. Somewhat strange for this test case, but understand that it will be a problem for the other migration cases.
Not sure what we shall do? Using pb Any in our docs doesn't make sense. BytesPayload could be a replacement but that is not intended to be user facing. What do you think @aludwiko ?
