Open fenollp opened 2 weeks ago
Hi! Thank you so much for tackling this :)
First thing is I'm not able to compile the plugin binary (within Docker, that's a hard requirement for us). See this reproducer:
docker buildx build . -o=. -f- <<'DOCKERFILE'
FROM rust:1.80-slim AS crate__protoc-gen-prost-validate
WORKDIR /app
ARG PROTOBUF_RELEASE_TAG=3.19.6
ADD --chmod=0664 --checksum=sha256:f51152d0926ccf18d89e2b4e4f31c4bf16ee5f94499d379029f80ddb8304bdd0 \
https://github.com/protocolbuffers/protobuf/releases/download/v$PROTOBUF_RELEASE_TAG/protoc-$PROTOBUF_RELEASE_TAG-linux-x86_64.zip protoc.zip
ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update && \
apt-get install --no-install-recommends -y \
unzip
RUN unzip protoc.zip -d protoc && \
mv ./protoc/bin/protoc /usr/bin/protoc && \
mv ./protoc/include/google /usr/local/include/google
#RUN ls -lha /usr/local/include/google /usr/local/include/google/protobuf && exit 42
RUN set -eux && cargo install --locked --git https://github.com/linka-cloud/protoc-gen-prost.git --branch protoc-gen-prost-validate protoc-gen-prost-validate
FROM scratch
COPY --from=crate__protoc-gen-prost-validate /usr/local/cargo/bin/protoc-gen-prost-validate /
DOCKERFILE
Errors with:
13.91 Compiling prost-validate-types v0.2.1
14.72 error: failed to run custom build command for `prost-validate-types v0.2.1`
14.72
14.72 Caused by:
14.72 process didn't exit successfully: `/tmp/cargo-install6B9abf/release/build/prost-validate-types-241d98e0a9eb0081/build-script-build` (exit status: 1)
14.72 --- stdout
14.72 cargo:rerun-if-changed=proto/validate/validate.proto
14.72
14.72 --- stderr
14.72 Error: Custom { kind: Other, error: "protoc failed: google/protobuf/descriptor.proto: File not found.\ngoogle/protobuf/duration.proto: File not found.\ngoogle/protobuf/timestamp.proto: File not found.\nvalidate.proto:7:1: Import \"google/protobuf/descriptor.proto\" was not found or had errors.\nvalidate.proto:8:1: Import \"google/protobuf/duration.proto\" was not found or had errors.\nvalidate.proto:9:1: Import \"google/protobuf/timestamp.proto\" was not found or had errors.\nvalidate.proto:799:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:803:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:807:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:811:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:815:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:819:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:823:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:833:14: \"google.protobuf.Timestamp\" is not defined.\nvalidate.proto:837:14: \"google.protobuf.Timestamp\" is not defined.\nvalidate.proto:841:14: \"google.protobuf.Timestamp\" is not defined.\nvalidate.proto:845:14: \"google.protobuf.Timestamp\" is not defined.\nvalidate.proto:849:14: \"google.protobuf.Timestamp\" is not defined.\nvalidate.proto:862:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:12:8: \"google.protobuf.MessageOptions\" is not defined.\nvalidate.proto: \"google.protobuf.MessageOptions\" is not defined.\nvalidate.proto:21:8: \"google.protobuf.OneofOptions\" is not defined.\nvalidate.proto:28:8: \"google.protobuf.FieldOptions\" is not defined.\n" }
14.72 warning: build failed, waiting for other jobs to finish...
23.26 error: failed to compile `protoc-gen-prost-validate v0.0.1 (https://github.com/linka-cloud/protoc-gen-prost.git?branch=protoc-gen-prost-validate#51aa0aa3)`, intermediate artifacts can be found at `/tmp/cargo-install6B9abf`.
23.26 To reuse those artifacts with a future compilation, set the environment variable `CARGO_TARGET_DIR` to that path.
Note that this works when outside docker (protoc 3.21.12
):
cargo install --locked --git https://github.com/linka-cloud/protoc-gen-prost.git --rev 51aa0aa3254126c439e78df93818d262b876417b protoc-gen-prost-validate
I'm thinking https://github.com/linka-cloud/prost-validate/blob/30fcad4a122ad916823f7e15461ec0a2466943a7/prost-validate-types/build.rs isn't looking for globally-installed include files?
Anyway, a safe fix might be to include google's proto files along with validate.proto
if that can be alright with you?
I'll continue on my side, trying to add more include paths to https://github.com/linka-cloud/prost-validate/blob/30fcad4a122ad916823f7e15461ec0a2466943a7/prost-validate-types/build.rs#L18
Thanks @fenollp for your feedback.
I cannot reproduce using:
cat << EOF > Dockerfile
ARG TAG=1.74
FROM rust:\${TAG} AS builder
WORKDIR /src/protoc-gen-prost
RUN apt update && DEBIAN_FRONTEND=noninteractive apt install -y protobuf-compiler
RUN git clone https://github.com/linka-cloud/protoc-gen-prost -b protoc-gen-prost-validate .
RUN cargo build --release
FROM scratch
COPY --from=builder /src/protoc-gen-prost/target/release/protoc-gen-prost-validate /usr/local/bin/
EOF
docker buildx build .
# ...
docker buildx build --build-arg TAG=latest .
# ...
But I get the issue with alpine based images:
cat << EOF > Dockerfile
ARG TAG=1.74-alpine
FROM rust:${TAG} AS builder
WORKDIR /src/protoc-gen-prost
RUN apk add --no-cache protobuf git build-base
RUN git clone https://github.com/linka-cloud/protoc-gen-prost -b protoc-gen-prost-validate .
RUN cargo build --release
FROM scratch
COPY --from=builder /src/protoc-gen-prost/target/release/protoc-gen-prost-validate /usr/local/bin/
EOF
docker buildx build .
# Error: Custom { kind: Other, error: "protoc failed: google/protobuf/descriptor.proto: File not found.\ngoogle/protobuf/duration.proto: File not found.\ngoogle/protobuf/timestamp.proto: File not found.\nvalidate.proto:7:1: Import \"google/protobuf/descriptor.proto\" was not found or had errors.\nvalidate.proto:8:1: Import \"google/protobuf/duration.proto\" was not found or had errors.\nvalidate.proto:9:1: Import \"google/protobuf/timestamp.proto\" was not found or had errors.\nvalidate.proto:799:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:803:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:807:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:811:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:815:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:819:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:823:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:833:14: \"google.protobuf.Timestamp\" is not defined.\nvalidate.proto:837:14: \"google.protobuf.Timestamp\" is not defined.\nvalidate.proto:841:14: \"google.protobuf.Timestamp\" is not defined.\nvalidate.proto:845:14: \"google.protobuf.Timestamp\" is not defined.\nvalidate.proto:849:14: \"google.protobuf.Timestamp\" is not defined.\nvalidate.proto:862:14: \"google.protobuf.Duration\" is not defined.\nvalidate.proto:12:8: \"google.protobuf.MessageOptions\" is not defined.\nvalidate.proto: \"google.protobuf.MessageOptions\" is not defined.\nvalidate.proto:21:8: \"google.protobuf.OneofOptions\" is not defined.\nvalidate.proto:28:8: \"google.protobuf.FieldOptions\" is not defined.\n" }
But this works:
cat << EOF > Dockerfile
ARG TAG=1.74-alpine
FROM rust:${TAG} AS builder
WORKDIR /src/protoc-gen-prost
RUN apk add --no-cache protobuf protobuf-dev git build-base
RUN git clone https://github.com/linka-cloud/protoc-gen-prost -b protoc-gen-prost-validate .
RUN cargo build --release
FROM scratch
COPY --from=builder /src/protoc-gen-prost/target/release/protoc-gen-prost-validate /usr/local/bin/
EOF
docker buildx build .
# ...
docker buildx build --build-tab TAG=alpine .
Thanks! Alright I got generation to work.
However generated code fails to compile due to many of these:
error[E0277]: the trait bound `i32: prost_validate::Validator` is not satisfied
--> src/grpc/REDACTED.validate.rs:157:55
|
157 | ::prost_validate::Validator::validate(value)
| ------------------------------------- ^^^^^ the trait `prost_validate::Validator` is not implemented for `i32`
| |
| required by a bound introduced by this call
also on WKT and also on my types that just don't have validate.rules
annotations:
error[E0277]: the trait bound
grpc::my_package::MyType: prost_validate::Validator
is not satisfied
the trait bound `i32: prost_validate::Validator` is not satisfied
This is surprising. It seems that the field is evaluated as a message instead of a scalar.
wkt is only supported with prost-types
.
As you redacted the message name, I imagine you cannot share the proto definition.
Do you have a minimal reproduction case and can you share the script / buf options you used for the code generation?
Ah I'm noticing that these trait issues on scalars are actually on WKT. Maybe I'm not setting the right options? Here's my buf config:
version: v1
plugins:
- name: prost-validate
out: generated/rust/src/grpc
strategy: all
opt:
- compile_well_known_types
- enable_type_names
Here's a tiny reproducer for the other issue. I believe this simply has to do with nested messages (maybe in conjunction with google's types?). Note that there aren't any validation annotations in use in there
syntax = "proto3";
import "google/api/annotations.proto";
import "google/protobuf/timestamp.proto";
service Api {
rpc Call(Req) returns (Rep) {}
}
message Req {
string id = 1;
}
message Rep {
message Nested {
string a = 1;
string b = 2;
string c = 3;
}
string f1 = 1;
string f2 = 2;
string f3 = 3;
string f4 = 4;
google.protobuf.Timestamp f5 = 5;
google.protobuf.Timestamp f6 = 6;
Nested f7 = 7;
string f8 = 8;
string f9 = 9;
}
Thank you for the reproducer.
Remove the compile_well_known_types
option on prost-validate
(and on prost
), wkt is only supported with prost-types
for the moment.
Alright I'm disabling it. What do you mean "only supported with prost-types
"? I use the crate prost-types
with my generated code. Are you referencing prost-types
in another way than as a crate? e.g. as a buf
plugin option?
Here's another reproducer with nested messages and oneof:
syntax = "proto3";
package company.product.v1;
import "google/api/annotations.proto";
service Api {
rpc Call(Req) returns (Rep) {}
}
message Req {
message NestedMsg {
string f1 = 1;
string f2 = 2;
}
oneof lookup_by {
NestedMsg o1 = 1;
string o2 = 2;
}
}
message Rep {
string f1 = 1;
}
error[E0277]: the trait bound `grpc::company::product::v1::req::LookupBy: prost_validate::Validator` is not satisfied
--> src/grpc/company.product.v1.validate.rs:5:51
|
5 | ::prost_validate::Validator::validate(lookup_by)?;
| ------------------------------------- ^^^^^^^^^ the trait `prost_validate::Validator` is not implemented for `grpc::company::product::v1::req::LookupBy`
| |
| required by a bound introduced by this call
|
= help: the following other types implement trait `prost_validate::Validator`:
grpc::company::product::v1::Rep
grpc::company::product::v1::Req
What do you mean "only supported with prost-types"? I use the crate prost-types with my generated code. Are you referencing prost-types in another way than as a crate? e.g. as a buf plugin option?
I mean that it does not work with pbjson-types
and compile_well_known_types
options. By default prost-types
are used by prost
.
There will indeed be validation issues with external messages because of the way in which protoc-gen-prost(-validate)
and prost-validate
work differently.
prost-validate
, when used in build.rs
compiles all proto definitions, which is not the case with protoc-gen-prost(-validate)
.
I don't yet know how to solve this problem in Rust (if it's even possible). In go this would be:
if v, ok := any(m.Message).(interface{ Validate() error }); ok {
if err := v.Validate(); err != nil {
return fmt.Errorf("....: %w", err)
}
}
I suppose you need to mark the field as explicitly skipped, e.g:
message Message {
External msg = 1 [(validate.field).message.skip = true];
}
I'll take a look at your reproducer as soon as possible.
Ok then, I'll continue skipping these options. I wasn't using the pbjson-types
option anyway.
I see. Maybe this can be used as the Golang interface-casting test: https://github.com/nvzqz/impls#how-it-works
This should work for types defined in the same crate. For proto types generated in multiple crates, I don't see an easy solution.
For us this would be fine as we generate all our pb types in the same crate. (+ we have so many that marking them skip = true
would very painful).
Maybe the trick could be to impl the validation fn
on all generated types, just sometimes the fn
would not do anything. This way we're sure it always exists and hope the compiler optimizes it away.
What's needed to impl handling of WKT?
Maybe this can be used as the Golang interface-casting test: https://github.com/nvzqz/impls#how-it-works
Already tried 😉, but it is unusable because it creates the same compilation issue. According to the research I've done, this isn't possible in Rust.
Maybe the trick could be to impl the validation fn on all generated types, just sometimes the fn would not do anything. This way we're sure it always exists and hope the compiler optimizes it away.
This is exactly what protoc-gen-prost-validate
and prost-validate-build
do.
What's needed to impl handling of WKT
The Well-Known types are natively supported by prost
using prost-types
. The same applies to prost-validate
.
Wrappers are generated in the same way as proto3 optionals are (it's transparent to the user: Option<native-type>
).
It really is a better user experience.
But pbjson-types
doesn't handle wrappers in this way, (the API uses a Message with the value
field, as would compile_well_known_types
) so the code generation had to be adapted.
Support has been implemented.
I've finally found a way to call the validate method only if it's implemented 😀.
Here's another reproducer, this time for missing oneof
validation code:
syntax = "proto3";
package company.product.v1;
import "validate/validate.proto";
message Msg {
oneof scope {
string a = 1 [(validate.rules).string.uuid = true];
string b = 2 [(validate.rules).string.uuid = true];
}
repeated string cs = 3 [(validate.rules).repeated = {
unique: true
items: { string: {uuid: true} }
}];
}
// @generated by protoc-gen-prost-validate
impl ::prost_validate::Validator for Msg {
fn validate(&self) -> prost_validate::Result<()> {
let cs = &self.cs;
if ::prost_validate::VecExt::unique(cs).len() != cs.len() {
return Err(
::prost_validate::Error::new(
"company.product.v1.Msg.cs",
::prost_validate::errors::list::Error::Unique,
),
);
}
for (i, item) in cs.iter().enumerate() {
{
if let Err(_) = ::prost_validate::ValidateStringExt::validate_uuid(
&item,
) {
return Err(
::prost_validate::Error::new(
"company.product.v1.Msg.cs",
::prost_validate::errors::string::Error::Uuid,
),
);
}
Ok(())
}
.map_err(|e| ::prost_validate::Error::new(
format!("{}[{}]", "company.product.v1.Msg.cs", i),
::prost_validate::errors::list::Error::Item(Box::new(e)),
))?;
}
if let Some(ref scope) = self.scope {
::prost_validate::Validator::validate(scope)?;
}
Ok(())
}
}
error[E0277]: the trait bound `grpc::company::product::v1::msg::Scope: prost_validate::Validator` is not satisfied
--> src/grpc/company.product.v1.validate.rs:33:51
|
33 | ::prost_validate::Validator::validate(scope)?;
| ------------------------------------- ^^^^^ the trait `prost_validate::Validator` is not implemented for `grpc::company::product::v1::msg::Scope`
| |
| required by a bound introduced by this call
|
= help: the trait `prost_validate::Validator` is implemented for `grpc::company::product::v1::Msg`
Thanks.
I've got the problem: it's because sub-modules (created by oneof, enum and sub-messages) aren't handled in protoc-gen-prost-validate
.
I'll fix it as soon as possible.
~I'm afraid this is a very, very big problem actually.~
Thanks for your great work and reactivity! I have what I believe to be the last issue here. Reproducer:
syntax = "proto3";
import "validate/validate.proto";
message A {
message B {
enum X {
e1 = 0;
e2 = 1;
e3 = 2;
}
X x = 4 [(validate.rules).enum.defined_only = true];
}
}
thread 'main' panicked at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/prost-validate-derive-core-0.2.5/src/enum.rs:31:96:
called `Option::unwrap()` on a `None` value
Note: it doesn't reproduce if enum isn't nested.
You're welcome ! 😀
Can you give me a link or paste the contents of the generated validation code and your input message, please?
I was a bit too quick to push a fix on enum's sub-modules, so I'm surprised it even compiles in this case...
Well this crash comes from the execution of your plugin (version https://github.com/neoeinstein/protoc-gen-prost/pull/107/commits/e26dc79c74fb02e1788b396e6cec68ef145ba600 ) through buf (version 1.45.0
).
There's no generated code :)
prost-validate-derive-core-0.2.5/src/enum.rs:31:96
Sorry, I read a bit too quickly.
I'll check tomorrow.
😂 yes, that's exactly the problem I was thinking.
Done.
I've added a test in protoc-gen-prost-validate
to validate the generation of nested enums | messages | oneofs.
It all compiles! Thanks! Now I'll take time in the coming days to actually test the generated code.
Tests adapted from the official test suite are implemented here for prost-types
, here for pbjson-types
and here for protoc-gen-prost-validate
😉
Hi! I saw your comment today on the protoc
This issue asks to integrate with https://github.com/neoeinstein/protoc-gen-prost/tree/main/protoc-gen-prost-validate
I believe first some work is required to support prost generated files instead of straight up file descriptor sets.
Note that ideally this protobuf plugin should be usable through
protoc
orbuf
and generate files inOUT_DIR
, as is the case with the other protoc plugins in @neoeinstein's repo.I'd like to help! :)