bufbuild / protoc-gen-validate

Protocol Buffer Validation - Being replaced by github.com/bufbuild/protovalidate
https://github.com/bufbuild/protovalidate
Apache License 2.0
3.77k stars 582 forks source link

Support for gogoproto #52

Closed kyessenov closed 2 years ago

kyessenov commented 6 years ago

It seems that this tool only supports the standard goproto code generation output. I have noticed the following deviations for gogo:

What needs to be done to extend PGV with gogo support?

rodaine commented 6 years ago

disregard of go_package annotation that allows multiple proto files to be combined into one package, to avoid circular dependencies between packages

This is probably an oversight in the template, not using the correct package value. I can look into this.

no support for gogoproto code generation optimization annotations:

I haven't worked with gogoproto, so I don't have specifics here, but I'd say we should treat this as a different language target since it deviates in some fundamental ways from the official protoc-gen-go output. A lot could be reused between the two, though.

freman commented 6 years ago

Tis a good start to be sure to be sure, but I have run into an issue with (gogoproto.customname)

./dhcp.pb.validate.go:135:11: m.GetTtl undefined (type *InterfaceConfiguration has no field or method GetTtl, but does have GetTTL)
kyessenov commented 6 years ago

customname is not supported last time I checked. It should not be hard to add extra handling for customname. Seems like you might need to override accessor here https://github.com/lyft/protoc-gen-validate/blob/master/templates/goshared/register.go#L88.

rvolosatovs commented 5 years ago

I added GoGo support to protoc-gen-star directly: https://github.com/lyft/protoc-gen-star/pull/51 Using that functionality I added full support for customname/customtype/casttype/embeded: https://github.com/TheThingsIndustries/protoc-gen-validate/commit/0065acaf05f10558b4d4173f1a109485b2106316 And validation of non-nullable messages: https://github.com/TheThingsIndustries/protoc-gen-validate/commit/228ba7a816cd0d31ce9642ef0e6d6e78788c2d0b

If approach in https://github.com/lyft/protoc-gen-star/pull/51 seems viable and it gets merged, I can open PR to this repo for improving the GoGo support.

FTR, in our fork (https://github.com/TheThingsIndustries/protoc-gen-validate) we added support for fielpaths in validators, so the caller can specify a subset of fields to validate. If no paths are specified - all fields are validated, same as the original plugin.

coolyrat commented 5 years ago

Is there any plan supporting gogoproto.embed? Embed reduce a lot of duplicate codes. Right now I have to skip the embed message.

message AdPrimaryKey {
    string media = 1;
    string name = 2;
}

message Ad {
    AdPrimaryKey pk = 1 [(gogoproto.embed) = true, (validate.rules).message.skip = true];
    AdType type = 2 [(validate.rules).enum.not_in = 0];

    enum AdType {
        NONE = 0;
        OTHERS = 1;
        VIDEO = 2;
    }
}

message UpdateAdRequest {
    AdPrimaryKey pk = 1 [(gogoproto.embed) = true, (validate.rules).message.skip = true];
    Ad ad = 2;
}
monotone commented 2 years ago

care about this too.

rvolosatovs commented 2 years ago

care about this too.

Given that the project is unmaintained for a while https://github.com/gogo/protobuf/issues/691 I would not expect this to happen