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

add descriptor loader #87

Merged
merged 10 commits into from
Jun 20, 2017
Merged

add descriptor loader #87

merged 10 commits into from
Jun 20, 2017

Conversation

knoguchi
Copy link
Contributor

@knoguchi knoguchi commented May 8, 2017

This is a preliminary work for the #78 "create ProtoSchema from descriptor file".

A "descriptor file" is a set of descriptors. A descriptor is a binary representation of the .proto file. Typically one (or more) of the descriptors is the main, and the others are dependencies.

This reader creates a NativeProtobufSchema object from the main descriptor. Dependencies are "copied" into the main descriptor before generating the object. I chose copying over implementing the import mechanism per #86 discussion. The import statement is a potential security risk, and it would certainly add the complexity.

This descriptor reader should work for simple message types that do not have references to other nested message types. #73 needs to be fixed.

Note: descriptor.proto was copied from Google Protobuf. The reserved fields were commented out because ProtoParser could not parse. See #84.

{
InputStream fin;
fin = this.getClass().getClassLoader().getResourceAsStream(descriptorFilePath);
if (fin == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the resource can not be read, why would accessing it as URL work differently (and should it)? Maybe at least add a comment explaining what is the motivation here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to make it dual purpose -- load the descriptor from resources directory as well as external URL. I will remove the resources. I guess the descriptors in the resources directory is only useful for unit tests.

NativeProtobufSchema nativeSchema = NativeProtobufSchema.construct(protoFileMap.get("main.proto"));

ProtobufSchema schema = nativeSchema.forType("main1");
System.out.println(schema.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed.

@cowtowncoder
Copy link
Member

Ok, I think this could be a good starting point. There are some things I'd probably change, such as making ProtobufMapper initiate work instead of reading things in constructor (which is typically not something to avoid), but in general I think it makes sense. This would also remove the dependency from reader to constructing mapper which seems wrong way around.
I think it also would make sense to take different kinds of input sources: assuming a String defines a resource path seems limiting; it may be one option but I assume there are other cases such as reading from an external file.

But one question first: what is intended usage pattern for ProtoFiles? I'd prefer not exposing 3rd party types as-is through API, if they can be encapsulated somehow. Can they be converted to protoc definition? Test seems to just verify that something was read, but I assume these are useful for some actual usage.

@knoguchi
Copy link
Contributor Author

knoguchi commented May 10, 2017

Thanks for the comments. The reason why I exposed ProtoFiles map is to let the driver to pick the main proto file, then call NativeProtobufSchema.construct(ProtoFile) API.
I will modify it to be something like below to hide the 3rd party API.

// in the DescriptorReader class
private Map<String, ProtoFile> protoFileMap;

NativeProtobufSchema forProto(String protoName) {
    return NativeProtobufSchema.construct(protoFileMap.get(protoName));
}

ProtoFile.toSchema() method can generate the .proto string but I did not intend the usage.

The test case barely shows that the two .proto files (main.proto and other.proto) are merged. I will add tests that actually check the correctness of the output.

Other items that you pointed out:

  1. I will move the ProtobufMapper for the descriptor schema out of the reader class.
  2. add input sources:
    URL
    InputStream
    byte[]

- rename DescriptorReader to DescriptorLoader
 - return ProtobufSchema instead of NativeProtobufSchema
 - find the main .proto file by message type.
 - remove "main .proto file" concept.
- introduce FileDescriptorSet POJO
@knoguchi knoguchi changed the title add descriptor reader add descriptor loader May 14, 2017
@cowtowncoder
Copy link
Member

Unfortunately haven't had time to spend on this, but wanted to add a note to say it's still on my radar.

try {
in.close();
}
catch (IOException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why catch? Shouldn't it just be propagated as-is?

@cowtowncoder cowtowncoder merged commit e1a976f into FasterXML:master Jun 20, 2017
@cowtowncoder
Copy link
Member

Actually I think I can make some changes here from API perspective, but retain functionality.
Nothing big, just bit of touch up.

cowtowncoder added a commit that referenced this pull request Jun 20, 2017
@cowtowncoder
Copy link
Member

Ok so did refactoring, which is sizable by lines of code. But didn't much change functionality: mostly just changes access point, which is now via ProtobufMapper.

@knoguchi knoguchi deleted the descriptor branch December 5, 2018 05:13
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

Successfully merging this pull request may close these issues.

2 participants