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

Different behavior from Google protoc generated message for accessing proto extensions #1583

Closed
GreyPlane opened this issue Sep 27, 2023 · 15 comments

Comments

@GreyPlane
Copy link

GreyPlane commented Sep 27, 2023

Problem

I'm currently trying to implement gRPC HTTP transcoding feature for akka-grpc, but I found there is a weird issue for accessing proto extension.
for proto like this

service ShelfService {
  rpc GetShelf(GetShelfRequest) returns (Shelf) {
    option (google.api.http) = {get: "/v1/shelves/{shelf}"};
  }
}

if the proto class generated by the protocol buffer compiler, MethodDescriptor be like
Screenshot 2023-09-27 at 15 33 27

and if it is generated by the Scala Plugin for the Protocol Buffer Compiler, MethodDescriptor be like
Screenshot 2023-09-27 at 16 04 37

which I highlight the difference, seems pbc itself generated descriptor puts http option into extension instead of unknownFields.

currently I'm using AnnotationsProto.http.get(methDescriptor) from ScalaBP generated class for com.google.api.annotations.proto, which has a lens that access extension from unknownFields, and doesn't work for javadsl(bcs it's using pbc generated java class).

IMO, *Descriptor should be some kind of metadata that related only to the proto itself not the implementation, it's really weird to see that even javaDescriptor which return a class from com.google.protobuf works differently from "official" implementation, and even in ScalaPB itself actually uses API from com.google.protobuf, only bcs the way to use it causes the difference.
So I think it's better to put extension into extensions field instead of unknownSets at least for javaDescriptor, it's more convenient to use under situation also looks more reasonable.

Suggested Fixes

After deeper digging, I believe this is bcs when pbc generated class initialize it's descriptor there is some extra codes:

   com.google.protobuf.ExtensionRegistry registry =
        com.google.protobuf.ExtensionRegistry.newInstance();
    registry.add(com.google.api.AnnotationsProto.http);
    com.google.protobuf.Descriptors.FileDescriptor
        .internalUpdateFileDescriptor(descriptor, registry);
    com.google.api.AnnotationsProto.getDescriptor();

and inside internalUpdateFileDescriptor is just a simple method:

    public static void internalUpdateFileDescriptor(
        final FileDescriptor descriptor, final ExtensionRegistry registry) {
      ByteString bytes = descriptor.proto.toByteString();
      FileDescriptorProto proto;
      try {
        proto = FileDescriptorProto.parseFrom(bytes, registry);
      } catch (InvalidProtocolBufferException e) {
        throw new IllegalArgumentException(
            "Failed to parse protocol buffer descriptor for generated code.", e);
      }
      descriptor.setProto(proto);
    }

both are missing in Scalapb's plugin generated class, so in which I believe the fix should be construct registry for extensions and using another overloading for com.google.protobuf.DescriptorProtos.FileDescriptorProto.parseFrom, also changing lens accordingly.

  lazy val javaDescriptor: com.google.protobuf.Descriptors.FileDescriptor = {
    val javaProto = com.google.protobuf.DescriptorProtos.FileDescriptorProto.parseFrom(ProtoBytes)
    com.google.protobuf.Descriptors.FileDescriptor.buildFrom(javaProto, _root_.scala.Array(
      com.google.api.AnnotationsProto.javaDescriptor
    ))
  }

Alternative

It causes me really headache to think use difference code for accessing extension for different languages :(

Versions

Scala: 2.12
ScalaPB: 0.11.13
Protobuf: 3.21.9

@GreyPlane GreyPlane changed the title Difference behavior from Google protoc generated message for accessing proto extensions Different behavior from Google protoc generated message for accessing proto extensions Sep 27, 2023
@thesamet
Copy link
Contributor

thesamet commented Sep 28, 2023

Trying to understand the issue.

  1. You seem to be generating the descriptor in two ways: "by the protocol buffer compiler", and through "Scala plugin". Can you specify exactly how the descriptors are being generated in each of the two options?
  2. currently I'm using AnnotationsProto.http.get(methDescriptor) from ScalaBP generated class for com.google.api.annotations.proto, which has a lens that access extension from unknownFields, and doesn't work for javadsl(bcs it's using pbc generated java class).

Like you indiciate, the ScalaPB extensions API works on ScalaPB descriptors/extensions. Is the expectation that it would work with Java descriptors?

Can you somehow make the problem statement more clear?

@GreyPlane
Copy link
Author

GreyPlane commented Sep 29, 2023

Trying to understand the issue.

  1. You seem to be generating the descriptor in two ways: "by the protocol buffer compiler", and through "Scala plugin". Can you specify exactly how the descriptors are being generated in each of the two options?
  2. currently I'm using AnnotationsProto.http.get(methDescriptor) from ScalaBP generated class for com.google.api.annotations.proto, which has a lens that access extension from unknownFields, and doesn't work for javadsl(bcs it's using pbc generated java class).

Like you indiciate, the ScalaPB extensions API works on ScalaPB descriptors/extensions. Is the expectation that it would work with Java descriptors?

Can you somehow make the problem statement more clear?

sry for being confusing.

for 1, to be precisely, "by the protocol buffer compiler" in which I mean sbtprotoc.ProtocPlugin.gens.java that from sbt plugin protocbridge, and "Scala plugin" in which I mean scalapb.gen.SandboxedGenerator that using ScalaPbCodeGenerator.

for 2, yes, I was expect ScalaPB generated descriptors' lens work with Java descriptors, it feels weird to me that ScalaPB generated descriptors' implementation for proto extensions differ from the Java one's.

@thesamet
Copy link
Contributor

I am sorry I still don't follow. Can you provide a minimal project that demonstrate the problem and the expectation that is not met by ScalaPB? In any case, the implementation details do not have to match between ScalaPB and Java - the respective APIs for each should give the same results.

@GreyPlane
Copy link
Author

GreyPlane commented Sep 29, 2023

I guess I can, but would you like to elaborate about which point confused you? I mean if it is like why setup a project like this anyway or what specific code structure I was made to make this situation?

Also you actually answered my problem, and I would like to argue a little bit more about it.

To be fair, I'm far from protobuf expert by any means, but from a user perspective, I see Descriptor as a data that represent the programmable structure of a proto, also the API javaDescriptor returns a class that lays in com.google.protobuf which makes it looks like the "common part" of all different protobuf implementations), it just feels wrong to have the same data class/type but works actually different.

And from implementation perspective, it looks reasonable to put extension into extensions field instead of unknownFields, also relatively easy to do it I guess?

@GreyPlane
Copy link
Author

GreyPlane commented Sep 29, 2023

sry I was not make my point very clear, I've update the Problem section.

@thesamet
Copy link
Contributor

I am still confused. The two screenshots you share show the Java protobufs, none of them is a ScalaPB object, but then you mention you try to access them through the ScalaPB extensions API which isn't supposed to work with the Java objects.

@GreyPlane
Copy link
Author

GreyPlane commented Sep 30, 2023

I am still confused. The two screenshots you share show the Java protobufs, none of them is a ScalaPB object, but then you mention you try to access them through the ScalaPB extensions API which isn't supposed to work with the Java objects.

uh, I knew what I was missed now, I mean I did manually transform Java protobufs(com.google.protobuf.MethodOptions) to SPB ones(com.google.protobuf.descriptor.MethodOptions) by copying each fields, and then access the extension through generated lens, sry for cause confusion.

so just for sure, which I was misunderstand and you pointed is, ScalaPB extensions API(generated lens) isn't supposed to work with neither pbc generated Java classes or ScalaPB's javaDescriptor. thanks for clearing that.

if it's true, may I ask is that expected to have Java protobuf generated API(Annotations.http.getExtension) to work with ScalaPB's javaDescriptor, if it is, as I pointed, I don't think it works bcs one just simply differ from other.

@GreyPlane
Copy link
Author

And for clarify what I want to achieve, akka-grpc support both javadsl and scaladsl, and for some reason, javadsl and scaladsl use different code generator, when you try to implement functionality like HTTP transcoding which requires you to parse HttpRule from Descriptors, naturally you would to choose the common part of both generated classes and use Scala API for accessing, and it's javaDescriptor from ScalaPB and descriptor from Java ones.

Even they're the same type, but the implementation is actually different as I pointed, so after the same transformation procedure(to ScalaPB class), one works and one doesn't. that's why I think it is an issue.

@GreyPlane
Copy link
Author

Also to be fair, I do heard that unknownSets is the widely used approach, it maybe protobuf-java suddenly changes it's implementation at some point.

@thesamet
Copy link
Contributor

thesamet commented Sep 30, 2023

if it's true, may I ask is that expected to have Java protobuf generated API(Annotations.http.getExtension) to work with ScalaPB's javaDescriptor, if it is, as I pointed, I don't think it works bcs one just simply differ from other.

The Java API's getExtension should work with javaDescriptor.

...one works and one doesn't. that's why I think it is an issue.

Can you provide a minimal project that demonstrates what doesn't work?

@GreyPlane
Copy link
Author

GreyPlane commented Sep 30, 2023

if it's true, may I ask is that expected to have Java protobuf generated API(Annotations.http.getExtension) to work with ScalaPB's javaDescriptor, if it is, as I pointed, I don't think it works bcs one just simply differ from other.

The Java API's getExtension should work with javaDescriptor.

Bcs some weird issues of akka-grpc I couldn't get the Java API works(google common proto library's Java classes missing from the target), so I haven't test it yet, still working on that.

And if that's the case, sry for misunderstanding the API, I will try it.

@GreyPlane
Copy link
Author

GreyPlane commented Sep 30, 2023

...one works and one doesn't. that's why I think it is an issue.

Can you provide a minimal project that demonstrates what doesn't work?

minimal reproduce

Also I just realize that since I'm only copy unknownSets and there is no place for extensions in ScalaPB's class, so use ScalaPB's lens is not gonna work anyway.

@GreyPlane
Copy link
Author

if it's true, may I ask is that expected to have Java protobuf generated API(Annotations.http.getExtension) to work with ScalaPB's javaDescriptor, if it is, as I pointed, I don't think it works bcs one just simply differ from other.

The Java API's getExtension should work with javaDescriptor.

...one works and one doesn't. that's why I think it is an issue.

Can you provide a minimal project that demonstrates what doesn't work?

I just update the reproduce project to show it seems getExtension doesn't work with javaDescriptor

@thesamet
Copy link
Contributor

Thanks for providing the reproduction. It helps understanding the issue you are seeing better.

When ScalaPB generates the code for javaDescriptor it has a limitation: it can't assume that there are Java classes for anything that gets imported by the proto file. In this case, the generator has no way of knowing that AnnotatationsProto exists, so it can't reliably emit code like registry.add(com.google.api.AnnotationsProto.http);

There are two solutions you can have:

  1. Enable javaConversions=true generator option. This makes javaDescriptor to be a reference to the actual java descriptors.
  2. You could use javaDescriptor.toProto.toByteString and re-parse it using an extension registry you manually build in your code.

The code in Options.scala seems unnecessarily complex. What is it actually trying to do? Why doesn't it use the ScalaPB extensions API directly?

@GreyPlane
Copy link
Author

Thanks for your patience and comprehensive answer.

I can see the limitation here now, since akka-grpc doesn't include java classes for scaladsl and vice verse, I guess reconstruct descriptor is the only option.

The code in Options.scala seems unnecessarily complex. What is it actually trying to do? Why doesn't it use the ScalaPB extensions API directly?

I was thinking that use javaDescriptor would work, also wanna using Scala API for convenience, and parse from bytes seems expensive, so i've decide why not just manually copy every fields, after all it's just field mapping...
yeah, I guess it is a bad idea.

Anyway, I have no further question, pls feel free to close this issue.

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