Skip to content

Simplify gRPC validation#5650

Merged
timvisee merged 1 commit intodevfrom
simplify-grpc-validation
Dec 16, 2024
Merged

Simplify gRPC validation#5650
timvisee merged 1 commit intodevfrom
simplify-grpc-validation

Conversation

@timvisee
Copy link
Copy Markdown
Member

In gRPC we had a lot of custom validation rules because earlier validator versions had limited support for numbers wrapped in options.

Since #4894 we can use the built-in range and length validators on numbers wrapped in options.

I've removed the custom validation functions we now don't need anymore to greatly simplify validation in gRPC.

All Submissions:

  • Contributions should target the dev branch. Did you create your branch from dev?
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Comment thread lib/api/build.rs
("DeletePoints.collection_name", "length(min = 1, max = 255)"),
("UpdatePointVectors.collection_name", "length(min = 1, max = 255)"),
("UpdatePointVectors.vectors", "custom(function = \"crate::grpc::validate::validate_named_vectors_not_empty\", message = \"must specify vectors to update\")"),
("UpdatePointVectors.points", ""),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What?! The build script does not fail on non existing field?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sadly not. It's a limitation in the code generator of tonic/prost.

Copy link
Copy Markdown
Member

@agourlay agourlay left a comment

Choose a reason for hiding this comment

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

Very neat change 👏

@timvisee timvisee merged commit 7d52c95 into dev Dec 16, 2024
@timvisee timvisee deleted the simplify-grpc-validation branch December 16, 2024 11:46
timvisee added a commit that referenced this pull request Jan 8, 2025
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.

2 participants