Skip to content

Upgrade validation logic #275

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

Merged
merged 15 commits into from
Apr 22, 2025
Merged

Upgrade validation logic #275

merged 15 commits into from
Apr 22, 2025

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Mar 28, 2025

This includes upgrades to validation logic for:

  • URIs, URI references
  • IP addresses and prefixes (v4 and v6)
  • Hostname, Port
  • Email Address

The validation here adheres to the defined RFC standards for each entity and passes conformance for all new conformance tests defined in:

smaye81 and others added 6 commits March 24, 2025 12:01
This enhances the current `isEmail` validation by using a consistent
regex that will be used across protovalidate implementations.
This ports the validation logic from `protovalidate-go` for validating
URIs and URI references. As a result, all conformance tests for 0.10.3
related to these validations are now passing.

This uses a single underscore for protected class members and a
double-underscore for private class methods to use name-mangling. There
are varying recommendations on how to structure these (for example,
Effective Python recommends public members, some recommendations suggest
a single-underscore for class methods, etc.).

The current pattern in protovalidate-python seems to use a single
underscore for module-level functions, which we could certainly adopt
for classes. I'm open to whatever is the established pattern that we
should adopt.
Here is a proposal for how to structure docstrings in
protovalidate-python going forward (at least when porting the rest of
the validation implementations). I think this strikes the right balance
of adhering to best practices while not being onerous.

* Functions being added to the library should get the bulk of the
comment description in a docstring. No need to define `Args:`,
`Raises:`, etc. though.
* Any other public function, class, etc. should preferably have
docstrings, though not necessary.
* Be as succinct as possible while also keeping comments across
implementations consistently worded.

For validation following an RFC / spec:
* Always include any relevant ABNF grammar and keep aligned.
* Indent grammar inside docstring.

One suggestion outside the scope of this PR might be to always have full
docstring (`Args`, `Raises`, etc.) on any public method the user
interacts with (i.e. stuff in `validator.py`)
This beefs up the validation for the following:

IP address (v4 and v6)
IP prefix (v4 and v6)
Hostname / Port

The validation here adheres to the defined RFC standards for each entity
and passes conformance for all new conformance tests defined in
bufbuild/protovalidate#320.
- Avoid ambiguity in "string contains"
- Don't quote identifiers with backticks
- KISS with "Parses the rule"

Context:
bufbuild/protovalidate-java#257 (comment)
Also see: bufbuild/protovalidate-go#208
timostamm and others added 2 commits April 9, 2025 18:05
This ports the ipv6 fix from
bufbuild/protovalidate-go#215.

For context see the description on the above PR. The summary is that
this fixes the validation for some corner cases of IPv6 address
validation. Namely:

* Adds a check that an IPv6 address can't begin or end on a single
colon.
* Adds a check to fail-fast on invalid hextets.
@rodaine rodaine requested a review from pkwarren April 14, 2025 19:08
Copy link
Member

@pkwarren pkwarren left a comment

Choose a reason for hiding this comment

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

I see a few style things but overall looks great - nice to see all the newly passing tests.

Co-authored-by: Philip K. Warren <pkwarren@users.noreply.github.com>
Update generated code to match runtime requirement (6.30.1). Make some
small fixes to custom CEL functions.
@pkwarren pkwarren requested a review from timostamm April 15, 2025 14:42
@timostamm
Copy link
Member

Looks good to me, but please give me a bit of time to do some fuzzing on this branch before we merge it.

@nicksnyder nicksnyder merged commit 44885c1 into main Apr 22, 2025
12 checks passed
@nicksnyder nicksnyder deleted the sayers/validation_upgrades branch April 22, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants