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

[protobuf] . can't resolve inner types #73

Open
knoguchi opened this issue Apr 4, 2017 · 7 comments
Open

[protobuf] . can't resolve inner types #73

knoguchi opened this issue Apr 4, 2017 · 7 comments
Labels

Comments

@knoguchi
Copy link
Contributor

knoguchi commented Apr 4, 2017

When messages are defined as below, the parser failed to resolve t1.i1 type.

package mypackage;

message t1 {
        message i1 {
                optional uint32 x = 1;
                optional uint32 y = 2;
        }
}

message t2 {
        optional t1.i1 z = 1;
}

Error

Exception in thread "main" java.lang.IllegalArgumentException: Unknown protobuf field type 't1.i1' for field 'z' of MessageType 't2' (known enum types: ; known message types: t1, t2)
	at com.fasterxml.jackson.dataformat.protobuf.schema.TypeResolver._resolve(TypeResolver.java:141)
	at com.fasterxml.jackson.dataformat.protobuf.schema.TypeResolver.resolve(TypeResolver.java:93)
	at com.fasterxml.jackson.dataformat.protobuf.schema.NativeProtobufSchema.forType(NativeProtobufSchema.java:67)
	at com.fasterxml.jackson.dataformat.protobuf.schema.ProtobufSchemaLoader.load(ProtobufSchemaLoader.java:51)
cowtowncoder added a commit that referenced this issue Apr 4, 2017
@cowtowncoder
Copy link
Member

Unfortunately I can not reproduce this: if I load schema you specify, there is no error.
This could either be due to version you are using (if it's old, could be fixed), or your code does something different that what I have. My test has:

 ProtobufSchema schema = MAPPER.schemaLoader()
                .load(new StringReader(SCHEMA_STR));

with schema s above.

@knoguchi
Copy link
Contributor Author

knoguchi commented Apr 5, 2017

Thanks for the quick response. I simplified my test code. The only difference is that I specified the rootTypeName "t2". I'm using the HEAD of the 2.8 branch pulled today.

ProtobufSchema schema = MAPPER.schemaLoader()
                .load(new StringReader(SCHEMA_STR), "t2");

And then the stacktrace

Exception in thread "main" java.lang.IllegalArgumentException: Unknown protobuf field type 't1.i1' for field 'z' of MessageType 't2' (known enum types: ; known message types: t1, t2)
	at com.fasterxml.jackson.dataformat.protobuf.schema.TypeResolver._resolve(TypeResolver.java:141)
	at com.fasterxml.jackson.dataformat.protobuf.schema.TypeResolver.resolve(TypeResolver.java:94)
	at com.fasterxml.jackson.dataformat.protobuf.schema.NativeProtobufSchema.forType(NativeProtobufSchema.java:67)
	at com.fasterxml.jackson.dataformat.protobuf.schema.ProtobufSchemaLoader.load(ProtobufSchemaLoader.java:97)

@cowtowncoder
Copy link
Member

Thanks! Yes, that does trigger the issue for me too, for 2.8 branch and probably master too.

@cowtowncoder
Copy link
Member

(accidental fix by github, due to comment)

@knoguchi
Copy link
Contributor Author

knoguchi commented May 15, 2017

I am trying to understand the code base, and I saw the type resolver tries to find the name recursively but it's a little hard for me to fix. I wonder if it's easy for you to fix.

My idea is to calculate fully qualified name beginning with the package name at the time of parsing so that the type resolver does not have to traverse the type tree. Do you think it's a sane approach?

@knoguchi
Copy link
Contributor Author

knoguchi commented May 16, 2017

I just noticed FieldElement or DataType.NamedType class does not have methods that returns qualified name or type unlike TypeElement class that has .qualifiedName(). So we still need type resolution for the fields.

Looks like the Wire, the successor of the ProtoParser, was created for the very reason.

I hope someday ProtoParser will be replaced for Wire in this project. For now, I decided to use other solutions. Thanks.

@cowtowncoder
Copy link
Member

@knoguchi I was hoping to fix this along with #140, as I now understand the issue finally (it has been a while but I got back to here). Scoped resolution works with latest fixes but only in certain orders of traversal; I'll have to rewrite code a bit. Should get done for 2.10.0 however, need to get 2.9.8 out first.

@cowtowncoder cowtowncoder added 2.12 and removed 2.10 labels Apr 14, 2020
@cowtowncoder cowtowncoder added 2.13 and removed 2.12 labels Nov 28, 2020
@cowtowncoder cowtowncoder removed the 2.13 label Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants