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

grpc-gateway_out does not entirely obey the -M parameter #229

Open
johanbrandhorst opened this Issue Oct 6, 2016 · 8 comments

Comments

Projects
None yet
6 participants
@johanbrandhorst
Collaborator

johanbrandhorst commented Oct 6, 2016

Hi,

I'm using grpc-gateway_out with the github.com/gogo/protobuf project and I came across a case where the grpc-gateway generated code is invalid when using the -M parameter to the compiler.

When using the
google/protobuf/empty.proto
and want to replace the generated file with one from
github.com/gogo/protobuf/types/
via
grpc-gateway_out=Mgoogle/protobuf/empty.proto=github.com/gogo/protobuf/types
the generated code will import
github.com/gogo/protobuf/types
as expected, but the code still uses the
empty.Empty
to reference the Empty struct.

This results in both an unused import and an undefined reference compile error. The correct action should be to use types.Empty in the generated functions in this case.

@johanbrandhorst

This comment has been minimized.

Collaborator

johanbrandhorst commented Oct 6, 2016

Pinging @awalterschulze, the author of gogo/protobuf

@glenfordwilliams

This comment has been minimized.

glenfordwilliams commented Oct 22, 2017

any updates on this issue? currently running into this. doing

protoc \
-I ./proto/ \
-I ./vendor/ \
-I ./vendor/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
--grpc-gateway_out=grpc,Mgoogle/protobuf/timestamp.proto=github.com/gogo/protobuf/types,Mgoogle/protobuf/empty.proto=github.com/gogo/protobuf/types:. \
 proto/restaurant.proto

results in getting the proper import "github.com/gogo/protobuf/types" but empty.Empty is being used in the generated code var protoReq empty.Empty

@johanbrandhorst

This comment has been minimized.

Collaborator

johanbrandhorst commented Feb 12, 2018

At work I wrote a post-generate workaround for this in python:

def correct_grpc_gateway_imports(package):
    '''
    Workaround for https://github.com/grpc-ecosystem/grpc-gateway/issues/229.
    Replace instances of 'empty.Empty' with 'types.Empty' in any files.
    '''
    for root, dirs, filenames in os.walk(package):
        for filename in filenames:
            with open(os.path.join(root, filename), 'r') as f:
                file_contents = f.read()

            file_contents = file_contents.replace('empty.Empty', 'types.Empty')

            with open(os.path.join(root, filename), 'w') as f:
                f.write(file_contents)
@awalterschulze

This comment has been minimized.

awalterschulze commented Feb 23, 2018

Is there any plan to properly fix this?

@achew22

This comment has been minimized.

Collaborator

achew22 commented Feb 23, 2018

@awalterschulze, at this point I don't use gogoproto and so it isn't at the top of my priority queue. It is definitely a correctness issue, but I won't have the cycles to fix it. I don't think it would be extremely difficult to fix in the generator.

  1. Enhance the flag parsing to handle the additional M.*=.* args.
  2. Enhance the registry to register these mappings.
  3. Use those new mappings when rendering (there are a bunch of ways to do that and I'd leave it up to the implementer to decide which one to use)
@awalterschulze

This comment has been minimized.

awalterschulze commented Feb 26, 2018

How is the Empty message and other well known messages currently working with golang/protobuf without using the -M parameter?
Are you simply hardcoding the paths?

@Mistobaan

This comment has been minimized.

Mistobaan commented May 9, 2018

@achew22 why you don't use gogoproto? any particular reason?

@achew22

This comment has been minimized.

Collaborator

achew22 commented May 9, 2018

Mistobaan added a commit to Mistobaan/grpc-gateway that referenced this issue May 9, 2018

@Mistobaan Mistobaan referenced a pull request that will close this issue May 9, 2018

Open

fix #229 make Message use the name of the mapped import #647

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment