Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enums defined in proto2 throws an exception #8

Open
jrhee17 opened this issue Jan 12, 2022 · 2 comments
Open

Enums defined in proto2 throws an exception #8

jrhee17 opened this issue Jan 12, 2022 · 2 comments

Comments

@jrhee17
Copy link

jrhee17 commented Jan 12, 2022

When one tries to deserialize json into proto2 with enums defined, an exception seems to be thrown.
This behavior seems to differ from upstream, since only proto2 specific features aren't supported.
I haven't gone through the code so I'm unsure how trivial the fix will be, but wanted to file an issue to record my findings.


If one defines an enum in proto2

syntax = "proto2";

...

enum Foo {
  A = 1;
  B = 2;
}

message Proto2Response {
  required string message = 1;
  optional Foo foo = 2;
}

And tries to parse json into proto

JsonParser parser = new ObjectMapper().getFactory().createParser("{\"message\": \"hello\", \"foo\": 1}");
MessageMarshaller marshaller =
    MessageMarshaller.builder().register(Proto2Test.Proto2Response.getDefaultInstance()).build();
marshaller.mergeValue(parser, Proto2Test.Proto2Response.newBuilder());
assertThat(parser.isClosed()).isFalse();

the following exception is thrown

Could not find setter.
java.lang.IllegalStateException: Could not find setter.
	at org.curioswitch.common.protobuf.json.ProtoFieldInfo.setValueMethod(ProtoFieldInfo.java:213)
	at org.curioswitch.common.protobuf.json.DoParse.setFieldValue(DoParse.java:433)
	at org.curioswitch.common.protobuf.json.DoParse.apply(DoParse.java:391)
	at net.bytebuddy.dynamic.scaffold.TypeWriter$MethodPool$Record$ForDefinedMethod$WithBody.applyCode(TypeWriter.java:719)
	at net.bytebuddy.dynamic.scaffold.TypeWriter$MethodPool$Record$ForDefinedMethod$WithBody.applyBody(TypeWriter.java:704)
	at net.bytebuddy.dynamic.scaffold.TypeWriter$MethodPool$Record$ForDefinedMethod.apply(TypeWriter.java:611)
	at net.bytebuddy.dynamic.scaffold.TypeWriter$Default$ForCreation.create(TypeWriter.java:5959)
	at net.bytebuddy.dynamic.scaffold.TypeWriter$Default.make(TypeWriter.java:2213)
	at net.bytebuddy.dynamic.scaffold.subclass.SubclassDynamicTypeBuilder.make(SubclassDynamicTypeBuilder.java:232)
	at net.bytebuddy.dynamic.scaffold.subclass.SubclassDynamicTypeBuilder.make(SubclassDynamicTypeBuilder.java:204)
	at net.bytebuddy.dynamic.DynamicType$Builder$AbstractBase.make(DynamicType.java:3661)
	at net.bytebuddy.dynamic.DynamicType$Builder$AbstractBase$Delegator.make(DynamicType.java:3899)
	at org.curioswitch.common.protobuf.json.TypeSpecificMarshaller.buildOrFindMarshaller(TypeSpecificMarshaller.java:257)
	at org.curioswitch.common.protobuf.json.TypeSpecificMarshaller.buildAndAdd(TypeSpecificMarshaller.java:140)
	at org.curioswitch.common.protobuf.json.MessageMarshaller$Builder.build(MessageMarshaller.java:407)
	at org.curioswitch.common.protobuf.json.MessageMarshallerTest.proto2Enum(MessageMarshallerTest.java:1021)
	...

It seems like proto2 generates stubs differently for enums

...
public Builder setFoo(org.curioswitch.common.protobuf.json.test.Proto2Test.Foo value) {
        if (value == null) {
...
@jrhee17
Copy link
Author

jrhee17 commented Jan 12, 2022

@chokoswitch
Copy link
Contributor

Hi @jrhee17 - upstream defined JSON serialization for protobuf only in proto3, not proto2

https://developers.google.com/protocol-buffers/docs/proto3#json

So this project only targets proto3 primarily and proto2 support is best effort. It was definitely quite intentional to use only numeric value setting for enums from the byte-buddy generated code because having to reference the actual Java class for the enum makes things much trickier.

trustin pushed a commit to line/armeria that referenced this issue Jan 25, 2022
…ion failure (#4033)

Motivation:

Currently, armeria server fails to start up while trying to create a `MessageMarshaller` for `proto2` files containing `Enum` definitions.

It seems like [protobuf-jackson](curioswitch/protobuf-jackson#8) doesn't support `proto2`.
However, it seems like upstream at least partially supports `proto2`.

https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/util/JsonFormat

> The JSON format follows Proto3 JSON specification and only proto3 features are supported. Proto2 only features (e.g., extensions and unknown fields) will be discarded in the conversion. That is, when converting proto2 messages to JSON format, extensions and unknown fields will be treated as if they do not exist. This applies to proto2 messages embedded in proto3 messages as well.

Modifications:

- Introduce a `GsonGrpcJsonMarshaller` which performs JSON marshalling using Gson.
  - Also introduce a corresponding builder class `GrpcJsonMarshallerBuilder`
- Add `GrpcJsonMarshaller#ofJackson`, `GrpcJsonMarshaller#builderForJackson`, `GrpcJsonMarshaller#ofGson`, `GrpcJsonMarshaller#builderForGson`

Result:

- Closes #4020
- Armeria now provides an easy way to fall back to the upstream `gson` implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants