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

GH-585 Fix incomplete references to enums defined in separate packages #633

Closed
wants to merge 4 commits into from

Conversation

elliotmjackson
Copy link
Contributor

@elliotmjackson elliotmjackson commented Sep 15, 2022

Generated Go code with references to enums from imported packages is broken. To fix this we recursively walk up the parents of the enum until it reaches the root file, prepending Message_ for each message it sees on the way. This builds up the generated enum type name.

resolves #586
resolves #624
resolves #604
resolves #626

Seems to be caused by an assumption in the go file template that assumes enums are defined under a message type.

Following is an _ variable that tries to make sure the import is used somewhere.

	{{ range $pkg, $path := enumPackages (externalEnums .) }}
		_ = {{ $pkg }}.{{ (index (externalEnums $) 0).Parent.Name }}_{{ (index (externalEnums $) 0).Name }}(0)
	{{ end }}

The Parent.Name_Name pattern is only valid in one case, when the enum is defined under a message. Any other place an enum can be defined will break, producing different kinds of failures.

Error case 1: Enum on its own

If the enum is on its own, the Parent is a file and you get something like

// pkg1/foo.proto
enum Enum { }

// pkg2/foo.proto
import "pkg1/foo.proto"

// pkg2/foo.pb.validate.go
_ = pkg1.pkg1/pkg1.proto_Enum(0)

This is not valid go code, and the GoFmt post-processor cannot format it correctly, hence the error.

EDIT: on closer thought, it looks valid (valid division), but it won't compile. I think it becomes invalid when a keyword like type appears as one of the path elements, since go will try and interpret it as a divisor.

Error case 2: enum is under a nested message (two messages deep)

OTOH, if the enum is defined under a nested message (message.message.enum), The Parent is just M2, however the enum type includes both M1 and M2 (type M1_M2_Enum). The result is you get a reference to something that does not exist.

// pkg1/foo.proto
message M1 { message M2 { enum Enum { } } }

// pkg2/foo.proto
import "pkg1/foo.proto"

// pkg2/foo.pb.validate.go
_ = pkg1.M2_Enum(0) // This does not exist, instead 'M1_M2_Enum(0)' is the correct value to use.

This causes code that generates successfully but will not compile.

Signed-off-by: Elliot Jackson <elliot@buf.build>
Elliot Jackson added 2 commits September 15, 2022 14:46
Signed-off-by: Elliot Jackson <elliot@buf.build>
@elliotmjackson elliotmjackson marked this pull request as ready for review September 16, 2022 15:15
@elliotmjackson elliotmjackson marked this pull request as draft September 19, 2022 18:57
@elliotmjackson elliotmjackson marked this pull request as ready for review September 19, 2022 20:16
@joshcarp
Copy link

end of day right now, will look at tomorrow

@danielbairstow97
Copy link

Is there any update on this fix? This issue has been plaguing me for the last couple weeks

@elliotmjackson elliotmjackson deleted the bugfix/GH-585-walk-enum-parents branch September 28, 2022 19:14
@elliotmjackson elliotmjackson restored the bugfix/GH-585-walk-enum-parents branch October 14, 2022 14:20
@leefernandes
Copy link

huh, this is still borked

@elliotmjackson
Copy link
Contributor Author

@leefernandes It's my understanding this is fixed BUT it has been "fixed" a few times now. Could you help me with the edge case you've found by example in a new issue?

@leefernandes
Copy link

leefernandes commented Dec 16, 2022

@elliotmjackson Unfortunately I don't have the time to re-create a shareable representation of the proto files to share right now, but it occurred after migrating an enum to be an import. Prior to making the enum an import reference, the validation rule below worked.

	my.package.proto.v1.MyEnum my_enum = 8 [
		(google.api.field_behavior) = REQUIRED
		 // enum rule causes protoc validation error when referencing imported enum.
		 (validate.rules).enum = {
			defined_only: true,
			not_in: [0]
		}
	];

The error

pb.validate.go:154:14: undefined: v1

Here is our version of the lib.

    go_repository(
        name = "com_github_envoyproxy_protoc_gen_validate",
        build_file_proto_mode = "disable",
        importpath = "github.com/envoyproxy/protoc-gen-validate",
        sum = "h1:PS7VIOgmSVhWUEeZwTe7z7zouA22Cr590PzXKbZHOVY=",
        version = "v0.9.1",
    )

@elliotmjackson elliotmjackson deleted the bugfix/GH-585-walk-enum-parents branch December 28, 2022 20:41
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.

None yet

4 participants