[#405] Convert object to/from JSON #411
Conversation
f0690a2 to
a034385
Compare
|
Directly wrapping objects with the WrappedMessage leads to: // new WrappedMessage(new WrappedMessage(UUID.randomUUID()))
{
"_type":"org.infinispan.protostream.WrappedMessage",
"_value":{
"_type":"org.infinispan.protostream.WrappedMessage",
"_value":{
"_type":"org.infinispan.protostream.commons.UUID",
"mostSigBitsFixed":15461222357418722979,
"leastSigBitsFixed":13070220576097665502
}
}
}Using with collections: // List.of(new WrappedMessage(UUID.randomUUID()), new WrappedMessage(UUID.randomUUID()))
{
"_type":"collections.ListOfAdapter",
"elements":[
{
"_type":"org.infinispan.protostream.WrappedMessage",
"_value":{
"_type":"org.infinispan.protostream.commons.UUID",
"mostSigBitsFixed":1624897274251135499,
"leastSigBitsFixed":11937877021784957987
}
},
{
"_type":"org.infinispan.protostream.WrappedMessage",
"_value":{
"_type":"org.infinispan.protostream.commons.UUID",
"mostSigBitsFixed":12908692626181016473,
"leastSigBitsFixed":9445129632214002810
}
}
]
}I've also removed the use of // new WrappedMessage(1.23f)
{
"_type":"org.infinispan.protostream.WrappedMessage",
"wrappedFloat":1.23
} |
|
I'll revert this to a draft. There are some corner cases with more nested objects, collections, and adapters that don't create exact same byte array for the conversion of JSON -> Proto. And by looking at the test failures, the |
|
Some containers were completely ignored by the JSON serialization (ArrayList, HashSet). I'll work to add those as well. |
a034385 to
66533a4
Compare
|
Some time later, more than I would like. I've refactored the old code. Instead of doing (parse, write, prettify, etc.) spread all around, I've created specialized classes for each type we would expect in the byte stream: primitives, nested objects, repeated fields, maps, and containers. This uses the specialized classes with internal delegates, so each class can focus on doing its part and passing the request along. For example, a list of UUIDs: there is the root parser, which passes the request to an object writer for the adapter, which delegates to an array writer, which delegates each UUID to an object writer. The writers will pass the context along so we can track where we are in the JSON. Leading to the other change. Instead of passing a StringBuilder and writing, we tokenize the byte stream by pushing tokens from the JSON specification into a list. After reading the stream, we write the tokens in order and create the final JSON. The parsing of JSON to byte array remains more or less the same as before, plus fixes to handle more types. This needed changes to go over again from the start when parsing a document, and we need to pass along which field number it belongs. The downside here is that we go to the bottom of the JSON and build up writing in a nested buffer. This has some extra allocations in some places when we need to wrap a byte array with WRAPPED_MESSAGE and the field. The changes here are pretty much backwards compatible with what we had before. A JSON written in a previous version should be decoded just fine. Observe that we only changed the expected JSON in a single test: a test that uses an ArrayList container. Everything else is the same. |
|
@jabolina would this allow to push json from the client and convert to / from protostream ? in hotrod I mean |
|
@karesti, correct. A sample application doing that with the Hot Rod client: public class EncodingRemoteCache {
private static final String CACHE_NAME = "test-cache";
private static void registerMagazineSchemaInTheServer(RemoteCacheManager cacheManager, GeneratedSchema schema) throws IOException {
// Retrieve metadata cache
RemoteCache<String, String> metadataCache =
cacheManager.getCache(PROTOBUF_METADATA_CACHE_NAME);
// Define the new schema on the server too
metadataCache.put(schema.getProtoFileName(), schema.getProtoFile());
}
public static ConfigurationBuilder connectionConfig() {
ConfigurationBuilder builder = new ConfigurationBuilder();
builder.addServer()
.host("127.0.0.1").port(11222)
.security()
.authentication()
.username("admin")
.password("password");
String configuration = """
localCache:
name: "test-cache"
statistics: "true"
encoding:
mediaType: "application/x-protostream"
""";
builder.remoteCache(CACHE_NAME).configuration(configuration);
return builder;
}
public static void main(String[] args) throws IOException {
GeneratedSchema userSchema = new UserModelSerializationContextInitializerImpl();
ProtoStreamMarshaller marshaller = new ProtoStreamMarshaller();
marshaller.register(userSchema);
ConfigurationBuilder cb = connectionConfig();
cb.marshaller(marshaller);
RemoteCacheManager rcm = new RemoteCacheManager(cb.build());
registerMagazineSchemaInTheServer(rcm, userSchema);
RemoteCache<String, UserModel> userCache = rcm.getCache(CACHE_NAME);
RemoteCache<String, String> jsonCache = rcm.getCache(CACHE_NAME)
.withDataFormat(DataFormat.builder()
.valueType(MediaType.APPLICATION_JSON)
.valueMarshaller(new UTF8StringMarshaller())
.build());
UserModel user = new UserModel("jabolina", "jose", 28, Instant.EPOCH);
userCache.put("jose", user);
System.out.println("GET PROTO: " + userCache.get("jose"));
System.out.println("GET JSON: " + jsonCache.get("jose"));
rcm.stop();
}
@Proto
public record UserModel(String username, String name, int age, Instant creation) {
@ProtoSchema(
syntax = ProtoSyntax.PROTO3,
dependsOn = {
CommonContainerTypes.class,
},
includeClasses = UserModel.class,
schemaFileName = "user.proto",
schemaFilePath = "proto/",
schemaPackageName = "user"
)
public interface UserModelSerializationContextInitializer extends GeneratedSchema {}
}
}The output here for the system out is: I believe the marshaller here is only necessary for the put operation. The server is the one doing the work of transforming the model to JSON. In theory, the Hot Rod client doesn't need to know about the Proto descriptor. Also, the changes in the PR already exist in previous versions of ProtoStream. The current PR is only refactoring that part. |
|
@jabolina I believed it was something else, like really posting json JSONObject for example, and getting it converted! |
| @@ -711,6 +719,20 @@ private static <E> E readContainerElementWrapped(ImmutableSerializationContext c | |||
| return readMessage(ctx, elementReader, true); | |||
| } | |||
|
|
|||
| public static boolean knownWrappedDescriptor(ImmutableSerializationContext ctx, GenericDescriptor descriptor) { | |||
There was a problem hiding this comment.
Use a stream?
public static boolean knownWrappedDescriptor(ImmutableSerializationContext ctx, GenericDescriptor descriptor) {
return WRAPPED_KNOWN_TYPES.stream()
.filter(ctx::canMarshall)
.map(ctx::getMarshaller)
.filter(Objects::nonNull)
.anyMatch(m -> ctx.getDescriptorByName(m.getTypeName()) == descriptor);
}We could even remove WRAPPED_KNOWN_TYPES and just declare inline: Stream.of(Instant.class, Date.class)...
There was a problem hiding this comment.
Updated. I've kept the WRAPPED_KNOWN_TYPES so we don't create the stream every invocation.
| * | ||
| * @author anistor@redhat.com | ||
| * @author José Bolina | ||
| * @since 4.4 |
There was a problem hiding this comment.
I think this is sufficiently different to the old code that we can change this to 6.0
| if (ast.isEmpty()) return; | ||
|
|
||
| int last = ast.size() - 1; | ||
| JsonTokenWriter jtw = Objects.requireNonNull(ast.get(last)); |
There was a problem hiding this comment.
It's a shame we don't baseline on 21 so we could use getLast() 😢 .
| * | ||
| * @param out The output to write the token. | ||
| */ | ||
| void write(StringBuilder out); |
There was a problem hiding this comment.
Is it worth making this a higher level interface, e.g. Appendable?
I can't think of a particular use-case, I'm just thinking it might provide more flexibility in the future.
There was a problem hiding this comment.
I can make that work. I'll create the Appendable outside and extend the interface here.
| } | ||
|
|
||
| @Test | ||
| public void testNestedWrappedMessage() throws IOException { |
There was a problem hiding this comment.
The current JSON produced by this is:
{"_type":"org.infinispan.protostream.WrappedMessage","wrappedTypeId":1005,"wrappedMessage":"GTBDX5fqKajeIaHZX/rYYGi9"}Whereas I would expect the UUID type to be represented in a readable format.
I like the generic marshallAndUnmarshallTest but I think we need to make it more robust by also passing in the expected JSON output so that we can verify it's correct. I'm not sure how we do this in a way that satisfies all of the MarshallingMethodType values though 🤔
There was a problem hiding this comment.
It was a matter of distinguishing when the field was coming from a wrapped message of from a primitive. This now outputs something like:
{
"_type":"org.infinispan.protostream.WrappedMessage",
"wrappedMessage":{
"_type":"org.infinispan.protostream.commons.UUID",
"mostSigBitsFixed":16476212963255011726,
"leastSigBitsFixed":11281785963689059814
}
}The tests in ProtobufUtilTest do something similar, validating the expected JSON, but it does not have as many tests as here. I'll create a new test dedicated to the JSON conversion and compare it against the contents of a file.
There was a problem hiding this comment.
I've created a new test that validates the JSON format. I am unsure if we can approach this in some other way to avoid the many new files.
There was a problem hiding this comment.
I think the individual files are good for readability. AFAICT we're still missing test cases for prettyPrint? I don't think we need a test for all cases, but it would be good to have at least one just to be sure.
There was a problem hiding this comment.
Sounds like a good idea. I'll put together some nested objects.
There was a problem hiding this comment.
@ryanemerson, great suggestion! There was a problem where a raw list ("list": ["a", "b"]) didn't have the elements with proper indentation. This is fixed now.
66533a4 to
bb297bd
Compare
* Instant and Date objects not handled during JSON conversion. * Additionally, these objects try to convert as primitive types. This loses all the type context for the conversion back from JSON. * Require adapters from the types module to perform the conversion to JSON. * Updates JSON serialization to support nested types in lists and wrapped messages.
bb297bd to
757b605
Compare
|
Thanks @jabolina |
The field IDs for Instant and Date were ignored during the conversion so the generated JSON was an empty string. The fix was a bit more convoluted. I'll explain all I have and maybe there's a better solution.
Instantis converted by theWrappedMessagetobyte[]by writing oneint64field for seconds andint32for nanos.message-wrapping.protofile describes the seconds part as aoneOfwhich writes the key as"_value". We still need to write to fields, so we end with{"_value": 0, "wrappedInstantNanos": 0}without any typing.WrappedMessage, but the conversion from JSON toInstantwould fail as it creates theInstantinside theWrappedMessageobject. So we need an explicit type for the conversion to create the correct object.Since we need this conversion from JSON to
Instantto work, I've created an adapter in thetypesmodule. During the Proto to JSON conversion, we require this adapter to be registered so we can write the field names and the type correctly.We are still left with converting JSON to Proto
byte[]. We need to verify in theWrappedMessageclass whether this object corresponds to a known type. Otherwise, the generatedbyte[]will be entirely different from the one produced byInstantto Protobyte[]serialization.The
Dateobject suffers from the same thing. The difference is that:Date#getTimemethod.{"_type": "int64", "_value": 12345}. Which loses any type parameter that this value is aDateobject.Datewill fail as it returns a primitivelongnumber instead of theDateobject.Closes #405.