Skip to content

Conversation

@danielhochman
Copy link
Contributor

Have you read the Contributing Guidelines?

Yes

Brief description of what is fixed or changed

Starting in v2, if any base imports collide with imported packages in the proto, the base import will be aliased and pb.gw.go will not compile. Compilation fails because the rest of the template does not account for the import alias.

Proto file import (whose go_package is google.golang.org/genproto/googleapis/rpc/status):

...
import "google/rpc/status.proto";

message BatchResponse {
  repeated google.rpc.Status statuses = 1;
}

Generated batch.pb.gw.go:

import (
	...
	status_0 "google.golang.org/grpc/status"
	...
)

Build fails.

$ go build
api/v1/batch.pb.gw.go:22:2: imported and not used: "google.golang.org/grpc/status" as status_0
api/v1/batch.pb.gw.go:29:7: undefined: status

The regression occurred as part of the move to v2 because the files are loaded into the registry in v2 before the generator set up base imports in the registry.

Other comments

I'm sure there are many possible fixes here. However, I went ahead and just had the registry returned by the generator New instead of being an input parameter to enforce the call order previously used in v1. Happy to try another approach if this is building on a brittle foundation.

(note: working on getting corp CLA signed)

@google-cla
Copy link

google-cla bot commented Oct 14, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Oct 14, 2020
@codecov-io
Copy link

Codecov Report

Merging #1757 into master will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1757      +/-   ##
==========================================
- Coverage   58.74%   58.73%   -0.02%     
==========================================
  Files          33       33              
  Lines        3624     3625       +1     
==========================================
  Hits         2129     2129              
- Misses       1237     1238       +1     
  Partials      258      258              
Impacted Files Coverage Δ
...-gen-grpc-gateway/internal/gengateway/generator.go 40.17% <0.00%> (-0.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1aab03c...5fb70cb. Read the comment docs.

retain parsing order

comment
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

This fix looks fine, and I think it will change in the long term with the work @adambabik is doing in #1756 (:heart:), but I was wondering if we could add a test for this, or a test file that when generated without this fix exhibits the failure?

Thanks again for your contributions!

@johanbrandhorst
Copy link
Collaborator

Looks like we'll need to rebase these changes on master since #1752 was merged.

@johanbrandhorst
Copy link
Collaborator

Hi Daniel, I just merged #1756 so this will need a rebase, sorry for the inconvenience.

@johanbrandhorst johanbrandhorst merged commit 80a7a7b into grpc-ecosystem:master Oct 16, 2020
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants