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

ensure the imports are used is broken #585

Closed
mashail opened this issue Apr 5, 2022 · 13 comments
Closed

ensure the imports are used is broken #585

mashail opened this issue Apr 5, 2022 · 13 comments
Labels
Bug Reports and/or fixes a bug

Comments

@mashail
Copy link

mashail commented Apr 5, 2022

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 mashail changed the title ensure the imports are used is broken ensure the imports are used is broken Apr 5, 2022
@mashail
Copy link
Author

mashail commented Apr 7, 2022

Help please

@dbaneman
Copy link

dbaneman commented Jun 2, 2022

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

@fifsky
Copy link

fifsky commented Jun 17, 2022

me too
image

@fsj
Copy link

fsj commented Jul 7, 2022

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.

@WakesyChen
Copy link

image

meet this too

@eleduardo
Copy link
Contributor

eleduardo commented Aug 29, 2022

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.

@carsonoid
Copy link

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

eleduardo added a commit to eleduardo/protoc-gen-validate that referenced this issue Aug 29, 2022
eleduardo added a commit to eleduardo/protoc-gen-validate that referenced this issue Aug 29, 2022
Signed-off-by: Eduardo Solis <[email protected]>
@eleduardo
Copy link
Contributor

eleduardo commented Aug 30, 2022

@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:

#586
#624
#604
and of course mine
#626

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

@rmichela
Copy link
Contributor

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

@eleduardo
Copy link
Contributor

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?

@elliotmjackson
Copy link
Contributor

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.

@eleduardo
Copy link
Contributor

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

@glerchundi
Copy link
Contributor

glerchundi commented Sep 15, 2022

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:

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

@elliotmjackson elliotmjackson added the Bug Reports and/or fixes a bug label Sep 20, 2022
elliotmjackson pushed a commit that referenced this issue Sep 21, 2022
Signed-off-by: Eduardo Solis <[email protected]>

Signed-off-by: Eduardo Solis <[email protected]>
Co-authored-by: Elliot Jackson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Reports and/or fixes a bug
Projects
None yet
10 participants