Enhance IP validation #274
Conversation
| return celtypes.BoolType(_is_ip_prefix(val, version, strict=strict)) | ||
|
|
||
|
|
||
| def _is_ip_prefix(string: str, version: int, *, strict=False) -> bool: |
There was a problem hiding this comment.
Not sure of the best way to handle this. Other validation methods call this internally and if we stick with just is_ip_prefix then they have to pass cel values and handle cel types.
We have a similar scenario with Java (and C++). We could go back to the is_email and validate_email pattern that was in use before. FWIW, Java has that pattern now also. 🤷
There was a problem hiding this comment.
Yes, looks like a good reason for a separate function to me.
is_ vs. validate_ is not self-explanatory. What if we prefix the type converting function with cel_?
def cel_is_ip(val: celtypes.Value, ver: typing.Optional[celtypes.Value] = None) -> celpy.Result:
...
return celtypes.BoolType(is_ip(...))
def is_ip(val: str, ver: int) -> bool:
...There was a problem hiding this comment.
Updated. I also updated the other functions that don't require an internal implementation just to keep things consistent.
| # The below tests are failing due to a bug in the conformance runner. These | ||
| # proto messages are marked as IGNORE_ALWAYS which means they should always | ||
| # pass, which this implementation does correctly. However, the runner is | ||
| # expecting them to fail. | ||
| # See: | ||
| # https://github.com/bufbuild/protovalidate/blob/main/proto/protovalidate-testing/buf/validate/conformance/cases/required_field_proto2.proto#L24 | ||
| # https://github.com/bufbuild/protovalidate/blob/main/proto/protovalidate-testing/buf/validate/conformance/cases/required_field_proto2.proto#L31 |
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.