Skip to content

add new validation type not_contains#253

Merged
rodaine merged 9 commits intobufbuild:masterfrom
asraa:notcontain
Oct 10, 2019
Merged

add new validation type not_contains#253
rodaine merged 9 commits intobufbuild:masterfrom
asraa:notcontain

Conversation

@asraa
Copy link
Copy Markdown
Contributor

@asraa asraa commented Aug 1, 2019 •
edited
Loading

This commit adds a not_contains validation type for string fields. The goal is to validate that field doesn't contain a blacklisted character, like nul characters.

Signed-off-by: Asra Ali asraa@google.com

@asraa
initial not_contains validator
8f61832 
Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Copy Markdown
Contributor Author

asraa commented Aug 1, 2019

Hey, this is my initial commit for a not_contains type. Had a quick question:
I used protoc-gen-go to generate validate.pb.go, but the formatting looks very different than the old version. Was there a better way to do this?
I also updated vettools, but I'm having doubts it was necessary since the "-shadow" removal was a while back (?) see https://go-review.googlesource.com/c/go/+/154584/3/doc/go1.12.html
/review @akonradi

@asraa asraa marked this pull request as ready for review August 1, 2019 14:43
akonradi
akonradi suggested changes Aug 1, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@akonradi akonradi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's up with validate.proto.go but the rest LGTM modulo the one doc comment. I'd suggest getting an additional reviewer since this is an API change, though.

@asraa
add README comment and fix validate.pb.go
fdd462a 
Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Copy Markdown
Contributor Author

asraa commented Aug 1, 2019

/review @rodaine

rodaine
rodaine requested changes Aug 2, 2019
View reviewed changes
tests/harness/executor/cases.go Outdated
{"string - not contains - valid", &cases.StringNotContains{Val: "candy bars"}, false},
{"string - not contains - valid (only)", &cases.StringNotContains{Val: "bar"}, false},
{"string - not contains - invalid", &cases.StringNotContains{Val: "candy bazs"}, true},
{"string - not contains - invalid (case-sensitive)", &cases.StringNotContains{Val: "Candy Bars"}, true},
Copy link
Copy Markdown
Member

@rodaine rodaine Aug 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're identifying the first two tests as "valid" but then setting their validity to false, and vice-versa for the last two. Either the labels are wrong, or the implementation is backwards.

Copy link
Copy Markdown
Contributor Author

@asraa asraa Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thank you so much for catching that. I didn't catch the mislabeling.

validate/validate.pb.go Outdated
// A compilation error at this line likely means your copy of the
// proto package needs to be updated.
const _ = proto.ProtoPackageIsVersion2 // please upgrade the proto package
const _ = proto.ProtoPackageIsVersion3 // please upgrade the proto package
Copy link
Copy Markdown
Member

@rodaine rodaine Aug 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as stated elsewhere, please set the file permissions back to 0644; this file has no business being executable 😅

Also, let's generate this file with a version of protoc-gen-go before the addition of this new constant. For folks not using this package via Bazel, there is no significant changes that warrant the runtime upgrade (and this will cause compilation errors for them).

Copy link
Copy Markdown
Contributor Author

@asraa asraa Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it enough to manually change the proto version of the file? (I saw some instructions about installing protoc-gen-go with a custom tag, but this seemed like enough from a previous PR).

Copy link
Copy Markdown
Member

@rodaine rodaine Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, unfortunately. protoc-gen-go emits those constants to control the minimum runtime version of the protobuf package used. The emitted code emitted with "3" has optimizations that don't exist from when the value used to be "2". That is to say, it will still be broken.

Copy link
Copy Markdown
Contributor Author

@asraa asraa Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I installed v 1.2.0 of protoc-gen-go and used that to generate the pb.go file.

@rmichela rmichela added the Enhancement Extend or improve functionality label Aug 7, 2019
@asraa
Copy link
Copy Markdown
Contributor Author

asraa commented Aug 8, 2019

Just added python support as well.

I had to change sys.stdin.read() to sys.stdin.buffer.read() in harness.py. @kothariaditya, did you encounter errors like:

Traceback (most recent call last):
  File "/usr/local/google/home/asraa/.cache/bazel/_bazel_asraa/846d06093714e38d686b02e69f690c56/execroot/com_envoyproxy_protoc_gen_validate/bazel-out/k8-fastbuild/bin/tests/harness/executor/linux_amd64_stripped/executor.runfiles/com_envoyproxy_protoc_gen_validate/tests/harness/python/harness.py", line 37, in <module>
    message = sys.stdin.read()
  File "/usr/lib/python3.6/codecs.py", line 321, in decode
    (result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode bytes in position 61-62: unexpected end of data

when you ran the python tests?

@kothariaditya
Copy link
Copy Markdown
Contributor

kothariaditya commented Aug 8, 2019

Hey @asraa I don't think I encountered that before. Did you get the error before adding the not_contains testcases as well?

@asraa
Copy link
Copy Markdown
Contributor Author

asraa commented Aug 8, 2019

Yep, I hit that for any Python testcase including floats, etc, as a matter of just reading in the file.

@kothariaditya
Copy link
Copy Markdown
Contributor

kothariaditya commented Aug 8, 2019

Wow that's really weird. Never had that issue while running locally or in the CI. Thanks for updating it to sys.stdin.buffer.read(). Sorry for the trouble!

@asraa
Merge remote-tracking branch 'upstream/master' into notcontain
506e98e 
Signed-off-by: Asra Ali <asraa@google.com>
@asraa
update
b9eed3f 
Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa requested a review from rodaine September 20, 2019 14:42
@asraa asraa mentioned this pull request Sep 26, 2019
Merged
@asraa
Copy link
Copy Markdown
Contributor Author

asraa commented Sep 26, 2019

@rodaine Hey Chris, when you have a chance, would you mind taking a look at this PR again? Thank you!

@asraa asraa requested a review from akonradi October 9, 2019 18:29
@rodaine
Copy link
Copy Markdown
Member

rodaine commented Oct 10, 2019

Thanks for the patch @asraa!

@rodaine rodaine merged commit 4f00761 into bufbuild:master Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rodaine rodaine rodaine approved these changes

+1 more reviewer

@akonradi akonradi akonradi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Enhancement Extend or improve functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@asraa @kothariaditya @rodaine @akonradi @rmichela