Golang: importing enums into package names with same ending component generates compiler errors

[gdotgordon]

If I import enums from one or more proto files, each in a go_pacakge ending in "v1" to another proto file whose go_pacakge name also ends in "v1", there are multiple compile errors in the generated pb.validate.go file. There was a prior fix for this, #387, that was never merged to main, in fact it was recently marked as closed. I've built a version of the plugin with that fix, and it fixes most, but not all, of the compiler errors. So I believe that plus an additional fix is needed. There are other related fixes that are recently merged into the latest release, but the plugin still generates invalid Go code in this scenario.

I have included a zip file, valtest.zip with a simple Go project demonstrating the issues. To run it, simply invoke sh ./run.sh or something similar:
valtest.zip

Note here I am using google.golang.org/protobuf/cmd/protoc-gen-go, whereas our project still uses the deprecated github.com/golang/protobuf tools, but the results are the same. The paths=source_relative:. option for protoc-gen-validate seems to not honor the relative paths as specified in the new plugin, and creates a whole hierarchy rooted at github.com in the current project, perhaps the validator code generator should be able to handle both versions of protoc-gen-go. This could be user error or maybe another bug.

Input:

source 1:

option go_package = "github.com/valtest/from/v1";
package fv1;
enum ReportStatus { ...

source 2:

option go_package = "github.com/valtest/from/yetanother/v1";
package yav1;
enum OtherReportStatus { ...

destination:

option go_package = "github.com/valtest/to/v1";
package tv1;
...
import "from/v1/types.proto";
import "from/yetanother/v1/types.proto";
import "validate/validate.proto";
...
message UserInfo {
fv1.ReportStatus s1 = 1 [(validate.rules).enum.defined_only = true];
yav1.OtherReportStatus s2 = 2 [(validate.rules).enum.defined_only = true];
}

Expected Output in user.pb.validate.go:

import (
...
v1 "github.com/valtest/from/v1"
v11 "github.com/valtest/from/yetanother/v1"
...
)
...
_ = v1.ReportStatus(0)
_ = v11.OtherReportStatus(0)
...
if _, ok := v1.ReportStatus_name[int32(m.GetS1())]; !ok {
...
if _, ok := v11.OtherReportStatus_name[int32(m.GetS3())]; !ok {
...

Observed Output (with current released version):

// missing imports and check that imports are used for github.com/valtest/from/v1 and "github.com/valtest/from/yetanother/v1

// error, v1 undefined
if _, ok := v1.ReportStatus_name[int32(m.GetS1())]; !ok {

// error, v1 undefined plus should be a unique prefix like v11
if _, ok := v1.OtherReportStatus_name[int32(m.GetS3())]; !ok {

compiler error:
to/v1/user.pb.validate.go:60:14: undefined: v1
to/v1/user.pb.validate.go:71:14: undefined: v1

Observed Output Applying change from Issue 387 - correct except last 'if' has wrong prefix, 'v1' instead of 'v11' :

import (
...
v1 "github.com/valtest/from/v1"
v11 "github.com/valtest/from/yetanother/v1"
...
)
...
_ = v1.ReportStatus(0)
_ = v11.OtherReportStatus(0)
...
if _, ok := v1.ReportStatus_name[int32(m.GetS1())]; !ok {
...
if _, ok := v1.OtherReportStatus_name[int32(m.GetS3())]; !ok {
...

compiler error:
to/v1/user.pb.validate.go:79:17: undefined: v1.OtherReportStatus_name

Analysis (with change from Issue 387 applied):
Most of the action happens in templates/goshared/register.go. When there are trailing package name conflicts, a set of unique pacakge aliases is generated in func (fns goSharedFuncs) enumPackages, which is then applied to the template header in templates/go/file.go to generate the includes and code to ensure everything is used. Later, the template function named "typ" is applied to generate the prefixes in actual enum validation code in templates/goshared/enum.go. However, the function bound to "typ" is fns.Type, which is defined in the protoc-gen-star pacakge, and has no knowledge of the package aliases defined above. In fact, I put in some debugging and found:

***in Type() got enum type: 's1', fully qualified name: '.fv1.ReportStatus', context type: 'v1.ReportStatus'
***in Type() got enum type: 's2', fully qualified name: '.yav1.OtherReportStatus', context type: 'v1.OtherReportStatus'

So perhaps we need an override of Type() and possibly we need a way to persist the alias map created for the header part of the file.

[elliotmjackson]

thanks for raising this issue @gdotgordon, I was confident this had been fixed... can you test again with our latest version to confirm?

[gdotgordon]

@elliotmjackson yes, I am using the latest release, [email protected], as per the go.mod in the test project I had submitted with the issue report. I just ran my test project again and still get the compile errors in user.pb.validate.go. It really simple to run it, just unzip it and run "sh ./run.sh".

[elliotmjackson]

Sorry about that, I looked straight passed it. I'll get right onto this bug next.

[elliotmjackson]

Ok, I've had a moment to play around with this bug... i agree with you that this should be fixed and we will get around to it soon. Right now, I have made a version of your example using buf (buf_fix.zip) which might unblock you. If it doesn't, we will work on resolving your issue soon.

Looking at the directory structure you provided...

├── from
│   ├── v1
│   │   └── types.proto
│   └── yetanother
│       └── v1
│           └── types.proto
└── to
    └── v1
        └── user.proto

with the following packages:

• from/v1: fv1
• from/yetanother/v1: yav1
• to/v1: tv1

which, as a result of the build script, then results in:

├── from
│   ├── v1
│   │   ├── types.pb.go
│   │   ├── types.pb.validate.go
│   │   └── types.proto
│   └── yetanother
│       └── v1
│           ├── types.pb.go
│           ├── types.pb.validate.go
│           └── types.proto
└── to
    └── v1
        ├── user.pb.go
        ├── user.pb.validate.go
        └── user.proto

modifying the structure of your project a touch. The package structure is well-defined and buf managed mode is enabled for consistent go_package values.

├── buf.gen.yaml
├── buf.work.yaml
└── proto
    ├── buf.yaml
    ├── bar
    │   └── v1
    │       └── types.proto
    ├── foo
    │   └── v1
    │       └── types.proto
    └── user
        └── v1
            └── user.proto

and the user.proto file can now import like this:

syntax = "proto3";

package user.v1;

import "foo/v1/types.proto";
import "bar/v1/types.proto";
import "validate/validate.proto";

message UserInfo {
    foo.v1.ReportStatus s1 = 1 [(validate.rules).enum.defined_only = true];
    bar.v1.OtherReportStatus s2 = 2 [(validate.rules).enum.defined_only = true];
}

After telling buf to output to a /gen directory, the result is now:

└── gen
    ├── bar
    │   └── v1
    │       ├── types.pb.go
    │       └── types.pb.validate.go
    ├── foo
    │   └── v1
    │       ├── types.pb.go
    │       └── types.pb.validate.go
    └── user
        └── v1
            ├── user.pb.go
            └── user.pb.validate.go

and our user.pb.validate is correctly formed:

package userv1

import (
	...
	barv1 "github.com/valtest/gen/bar/v1"

	foov1 "github.com/valtest/gen/foo/v1"
	...
)
var (
	_ = barv1.OtherReportStatus(0)

	_ = foov1.ReportStatus(0)
)
func (m *UserInfo) validate(all bool) error {

	if _, ok := foov1.ReportStatus_name[int32(m.GetS1())]; !ok {
                ...
	}

	if _, ok := barv1.OtherReportStatus_name[int32(m.GetS2())]; !ok {
		...
	}
}

[gdotgordon]

@elliotmjackson thanks for the modified project. We are not currently using the buf tooling, so my first question is whether there is a way to preserve the exact code hierarchy as it existed in my original submission and without using buf? Moving proto files around and/or renaming the proto package in the proto files might be a possibility, but not for the go packages.

The problem with modifying our project structure is that the issue depicted in my sample occurs many times throughout the code, it is not a "one-off" occurrence. Second, it is an older pre-Go modules project (which explains all the package names ending in "v1") that we are simply trying to refresh to make it work with modern versions of Go, while minimizing code changes or introducing new tooling.

So if you are aware of a way to tweak the proto files as explained above, without requiring buf, I can give that a try. If not, due to the limitations on what can be changed in the project, then I'd say we'll need a bug fix.

[elliotmjackson]

Thats ok, I don't expect you to retrofit an established codebase to work around a bug.

1 further question that will help - did this work on a previous version of PGV or is this the first time you're using?

[gdotgordon]

@elliotmjackson yes, this used to work, the validation code generated without compile errors in the older versions. I looked in the go.mod file from then and we were using [email protected]. This was around 2019.

[elliotmjackson]

this really helps, thank you.

[gdotgordon]

Is there an ETA for a fix on this? Thanks.

[jdpedrie]

Having this issue as well! Any ETA on a fix?

[elliotmjackson]

Looking ahead, I want to let you know that we're shifting our focus towards protovalidate, which naturally eliminates the need for this bugfix. Your understanding and continued support mean a lot to us. Feel free to reach out if you have any questions or feedback as we make this transition.