protoc-gen-validate icon indicating copy to clipboard operation
protoc-gen-validate copied to clipboard

`ensure the imports are used` is broken

Open mashail opened this issue 2 years ago • 13 comments

We have a proto with this message

.....
import "packages/proto/types/language.proto";
import "packages/api/protos/common/v1/common.proto";

.....
......

message GetBeneficiaryAccountsByAliasRequest {
  string customer_id = 1 [(validate.rules).string.uuid = true];
  Alias alias = 2;

  cigar.types.Language language = 4 [(validate.rules).enum = {not_in: [0], defined_only: true}];
  cigar.api.common.v1.BankEnum.Bank bank = 3 [(validate.rules).enum = {defined_only: true}];
}

....
....

After upgrading protoc-gen-validate we are getting this error

beneficiaries.pb.validate.go:41:6: undefined: typespb.BankEnum_Bank

When I checked the generated code

var (
	_ = bytes.MinRead
	_ = errors.New("")
	_ = fmt.Print
	_ = utf8.UTFMax
	_ = (*regexp.Regexp)(nil)
	_ = (*strings.Reader)(nil)
	_ = net.IPv4len
	_ = time.Duration(0)
	_ = (*url.URL)(nil)
	_ = (*mail.Address)(nil)
	_ = anypb.Any{}
	_ = sort.Sort

	_ = types.BankEnum_Bank(0)

	_ = v1.BankEnum_Bank(0)
)

The issue types.BankEnum_Bank(0) should be types.Language(0)

mashail avatar Apr 05 '22 07:04 mashail

Help please

mashail avatar Apr 07 '22 22:04 mashail

We're running into this issue as well. Are there any updates on this? Thanks!

dbaneman avatar Jun 02 '22 12:06 dbaneman

me too image

fifsky avatar Jun 17 '22 08:06 fifsky

Same here. I get:

var (
    // ...

    _ = ccsds.DataType(0)

    _ = manoeuvre.DataType(0)
)

However, only ccsds.DataType() exists. It seems the problem occurs for enum types defined within another package.

fsj avatar Jul 07 '22 14:07 fsj

image meet this too

WakesyChen avatar Jul 08 '22 02:07 WakesyChen

Hey @rodaine not sure if you are the right person to look at this but this bug is pretty severe. The generated go code is unusable. I think the culprit is the change in the template (go/file.go) from {{ range $pkg, $path := enumPackages (externalEnums .) }} {{ $pkg }} "{{ $path }}" {{ end }} to {{ range $pkg, $path := enumPackages (externalEnums .) }} _ = {{ $pkg }}.{{ enumName (index (externalEnums $) 0) }}(0) {{ end }}

it would seem to me that the index on the second lookup is fixed and that is why the combo of pkg/path is broken and you always get the same enum name regardless...

I honestly don't understand why the change was made in the first place (like I don't get what was the case) but as of now this is broken.

I can try a couple of combinations and see if I can come up with a fix but if you can see the fix and can do it faster than I can it would be great to have this in.

eleduardo avatar Aug 29 '22 15:08 eleduardo

I would also be interested in an update on this. It makes the validator generation unusable for some of our proto files.

carsonoid avatar Aug 29 '22 17:08 carsonoid

@rodaine @keith @rmichela sorry for just randomly pinging you on this issue!! It honestly is pretty bad since the code that gets generated is unusable... I sent a PR last night but then I just realized there are a couple others trying to address it!! This seems to be the list:

https://github.com/envoyproxy/protoc-gen-validate/pull/586 https://github.com/envoyproxy/protoc-gen-validate/pull/624 https://github.com/envoyproxy/protoc-gen-validate/pull/604 and of course mine https://github.com/envoyproxy/protoc-gen-validate/pull/626

Is there any way to get any of these in and close the issue for good?

eleduardo avatar Aug 30 '22 16:08 eleduardo

I hate to be the bearer of bad news, but per #616 PGV is effectively unmaintained.

rmichela avatar Aug 30 '22 16:08 rmichela

Hey @ElliotMJackson thank you so much for starting work on this one!! I see that the PRs are getting closed, do you happen to have an ETA on when this will be fixed?

eleduardo avatar Sep 15 '22 18:09 eleduardo

yeah working on it now, shouldn't be too long at all - the fix has existed on various forks in the wild for a long time so it's looking fairly straight forward. In saying that, I'm still finding my feet maintaining the project so thanks in advance for you continued patience.

elliotmjackson avatar Sep 15 '22 18:09 elliotmjackson

@ElliotMJackson thank you!!! Happy to help if I can!

eleduardo avatar Sep 15 '22 19:09 eleduardo

Just to let you know that we included another fix to solve an issue with enums that belong to the same proto package name but have a different go packages defined on them. This happens in those packages like google.type.

References:

  • https://github.com/googleapis/googleapis/blob/master/google/type/dayofweek.proto
  • https://github.com/googleapis/googleapis/blob/master/google/type/date.proto

Fix: https://github.com/saltosystems/protoc-gen-validate/pull/10/files

glerchundi avatar Sep 15 '22 19:09 glerchundi