Fix missing gRPC api validations points selection#7221
Conversation
📝 WalkthroughWalkthroughAdds nested validation for Points-related gRPC types. build.rs registers additional fields with empty constraints to trigger nested validation. qdrant.rs: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Why no CI? 🤔 Edit: Fixed with a rebase 🤷 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/api/src/grpc/qdrant.rs (1)
4399-4404: Add nested validation to DeletePoints.points
- In lib/api/src/grpc/qdrant.rs, above the
pub points: ::core::option::Option<PointsSelector>field in the top-levelDeletePointsstruct (tag = "3"), insert#[validate(nested)].- In lib/api/build.rs, add the corresponding attribute injection for
DeletePoints.pointsso that codegen preserves the nested validation./// Affected points #[prost(message, optional, tag = "3")] - pub points: ::core::option::Option<PointsSelector>, + #[validate(nested)] + pub points: ::core::option::Option<PointsSelector>,
🧹 Nitpick comments (2)
lib/api/src/grpc/validate.rs (1)
481-488: Add a focused test for invalid nested filter via PointsSelector.A quick case (e.g., SetPayloadPoints with PointsSelector::Filter { FieldCondition with all fields None }) would assert ValidationErrors bubble through the nested path.
I can draft the test scaffolding if helpful.
lib/api/src/grpc/qdrant.rs (1)
6049-6189: Optional: extend validation to batch update operations using PointsSelectorIn points_update_operation::{SetPayload, OverwritePayload, DeletePayload, DeleteVectors, DeletePoints, ClearPayload}, points_selector appears but these structs don’t derive Validate and aren’t nested-validated through UpdateBatchPoints. If you want parity for batch API, consider deriving Validate on these messages and adding #[validate(nested)] to their points_selector fields, then wiring nested validation from UpdateBatchPoints.operations.
Also applies to: 6117-6141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/api/build.rs(2 hunks)lib/api/src/grpc/qdrant.rs(5 hunks)lib/api/src/grpc/validate.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust code
Do not use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice in new code; use bytemuck or zerocopy instead
Files:
lib/api/src/grpc/validate.rslib/api/build.rslib/api/src/grpc/qdrant.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/src/**/*.rs: Prefer exhaustive match arms over a catch-all _ arm to avoid missing new enum variants (except in tests/benchmarks or when provably safe)
Prefer explicit field ignoring with : _ over .. in struct patterns (except in tests/benchmarks or when provably safe)
Files:
lib/api/src/grpc/validate.rslib/api/src/grpc/qdrant.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: coszio
PR: qdrant/qdrant#7065
File: lib/collection/src/operations/universal_query/shard_query.rs:0-0
Timestamp: 2025-08-15T15:56:36.821Z
Learning: Internal gRPC APIs in Qdrant don't require additional runtime validation when proto-level validation is already configured via build.rs range constraints.
Learnt from: generall
PR: qdrant/qdrant#7006
File: lib/collection/src/operations/verification/update.rs:158-174
Timestamp: 2025-08-10T18:30:02.986Z
Learning: In Qdrant's strict mode verification code (lib/collection/src/operations/verification/update.rs), exhaustive pattern matching without `..` is intentionally used for structs like PointsBatch and PointsList. This design pattern ensures compilation fails when new fields are added, forcing developers to explicitly consider how new fields should be handled in the indexed_filter_write method. This provides visibility and compile-time safety for struct evolution.
Learnt from: generall
PR: qdrant/qdrant#7006
File: lib/collection/src/operations/point_ops.rs:501-528
Timestamp: 2025-08-10T18:31:56.855Z
Learning: In Qdrant, batch operations validate that `ids`, `vectors`, and `payloads` (if present) have matching lengths at the REST API level in `lib/api/src/rest/validate.rs` through the `Validate` trait implementation for `Batch`. This validation happens before data is converted to internal structures like `PointInsertOperationsInternal`, so methods operating on these internal structures can safely assume the lengths match.
📚 Learning: 2025-08-15T15:56:36.821Z
Learnt from: coszio
PR: qdrant/qdrant#7065
File: lib/collection/src/operations/universal_query/shard_query.rs:0-0
Timestamp: 2025-08-15T15:56:36.821Z
Learning: Internal gRPC APIs in Qdrant don't require additional runtime validation when proto-level validation is already configured via build.rs range constraints.
Applied to files:
lib/api/build.rslib/api/src/grpc/qdrant.rs
🧬 Code graph analysis (1)
lib/api/src/grpc/qdrant.rs (1)
lib/api/src/grpc/validate.rs (8)
validate(14-14)validate(19-21)validate(29-31)validate(39-41)validate(49-54)validate(58-64)validate(68-74)validate(78-85)
🔇 Additional comments (8)
lib/api/build.rs (2)
194-194: Good addition: nested validation wired for PointsSelector usage.Adding nested validation on PointsSelector itself and on points_selector/points fields across requests is correct and aligns codegen with runtime expectations.
Also applies to: 205-205, 209-209, 211-211, 213-213
194-194: Add missing enum‐level validation for PointsSelector
lib/api/build.rs is missing the("PointsSelector.points_selector_one_of", "")entry—add this line in the validation block (e.g. around line 194) to cover the enum‐level nested case.⛔ Skipped due to learnings
Learnt from: generall PR: qdrant/qdrant#7006 File: lib/collection/src/operations/verification/update.rs:158-174 Timestamp: 2025-08-10T18:30:02.986Z Learning: In Qdrant's strict mode verification code (lib/collection/src/operations/verification/update.rs), exhaustive pattern matching without `..` is intentionally used for structs like PointsBatch and PointsList. This design pattern ensures compilation fails when new fields are added, forcing developers to explicitly consider how new fields should be handled in the indexed_filter_write method. This provides visibility and compile-time safety for struct evolution.Learnt from: CR PR: qdrant/qdrant#0 File: .github/review-rules.md:0-0 Timestamp: 2025-08-29T08:16:28.521Z Learning: Applies to **/src/**/*.rs : Prefer exhaustive match arms over a catch-all _ arm to avoid missing new enum variants (except in tests/benchmarks or when provably safe)lib/api/src/grpc/validate.rs (1)
481-488: LGTM: enum-level validation for PointsSelectorOneOf is correct and exhaustive.Filter delegates to its own Validate; Points path is a no-op. Matches the desired behavior.
lib/api/src/grpc/qdrant.rs (5)
4498-4501: Good: nested validation for DeletePointVectors.points_selectorThis ensures Filter inside PointsSelector is validated when provided.
4530-4533: Good: nested validation for SetPayloadPoints.points_selectorCovers payload updates using filter-based selection.
4562-4565: Good: nested validation for DeletePayloadPoints.points_selectorPrevents invalid filters from slipping through delete-payload ops.
4588-4591: Good: nested validation for ClearPayloadPoints.pointsAligns clear-payload with other operations using PointsSelector.
6869-6878: Confirm Validate impl for PointsSelectorOneOf
No matchingimpl validator::Validate for points_selector::PointsSelectorOneOfwas found; please verify thatlib/api/src/grpc/validate.rsdefines this impl and delegates toFilter::validatewhen the Filter variant is used.
27aea0a to
5b8d9ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/api/build.rs(2 hunks)lib/api/src/grpc/qdrant.rs(5 hunks)lib/api/src/grpc/validate.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/api/build.rs
- lib/api/src/grpc/qdrant.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust code
Do not use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice in new code; use bytemuck or zerocopy instead
Files:
lib/api/src/grpc/validate.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/src/**/*.rs: Prefer exhaustive match arms over a catch-all _ arm to avoid missing new enum variants (except in tests/benchmarks or when provably safe)
Prefer explicit field ignoring with : _ over .. in struct patterns (except in tests/benchmarks or when provably safe)
Files:
lib/api/src/grpc/validate.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: coszio
PR: qdrant/qdrant#7065
File: lib/collection/src/operations/universal_query/shard_query.rs:0-0
Timestamp: 2025-08-15T15:56:36.821Z
Learning: Internal gRPC APIs in Qdrant don't require additional runtime validation when proto-level validation is already configured via build.rs range constraints.
Learnt from: generall
PR: qdrant/qdrant#7006
File: lib/collection/src/operations/point_ops.rs:501-528
Timestamp: 2025-08-10T18:31:56.855Z
Learning: In Qdrant, batch operations validate that `ids`, `vectors`, and `payloads` (if present) have matching lengths at the REST API level in `lib/api/src/rest/validate.rs` through the `Validate` trait implementation for `Batch`. This validation happens before data is converted to internal structures like `PointInsertOperationsInternal`, so methods operating on these internal structures can safely assume the lengths match.
🧬 Code graph analysis (1)
lib/api/src/grpc/validate.rs (2)
lib/api/src/rest/schema.rs (3)
validate(52-61)validate(134-143)validate(1428-1433)lib/segment/src/types.rs (6)
validate(784-790)validate(1580-1590)validate(1910-1921)validate(1952-1959)validate(3015-3026)filter(2919-2921)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: integration-tests-consensus
- GitHub Check: test-consensus-compose
- GitHub Check: test-consistency
- GitHub Check: storage-compat-test
- GitHub Check: integration-tests
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: lint
- GitHub Check: e2e-tests
| impl Validate for super::qdrant::points_selector::PointsSelectorOneOf { | ||
| fn validate(&self) -> Result<(), ValidationErrors> { | ||
| match self { | ||
| grpc::points_selector::PointsSelectorOneOf::Points(_) => Ok(()), | ||
| grpc::points_selector::PointsSelectorOneOf::Filter(filter) => filter.validate(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Nested validation for PointsSelectorOneOf is correct and exhaustive.
Filter delegates to filter.validate(), Points is pass-through; matches existing style (e.g., ConditionOneOf) and avoids a _ arm. Looks good.
Run to verify the generator attributes landed in qdrant.rs and build.rs as intended:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Validate impl present (this file):"
rg -nP 'impl\s+Validate\s+for\s+super::qdrant::points_selector::PointsSelectorOneOf' lib/api/src/grpc/validate.rs
echo "PointsSelector derives Validate:"
rg -nP 'derive\([^)]*Validate[^)]*\)\s*pub\s+struct\s+PointsSelector\b' lib/api/src/grpc/qdrant.rs -C2 || true
echo "points_selector_one_of is nested-validated:"
rg -nP '#\[validate\(nested\)\]\s*pub\s+\w+\s*:\s*::core::option::Option<[^>]*PointsSelectorOneOf' lib/api/src/grpc/qdrant.rs -C2 || true
echo "Endpoints reference nested validation on points_selector / points:"
rg -nP '#\[validate\(nested\)\].*points_selector' lib/api/src/grpc/qdrant.rs -C2 || true
rg -nP '#\[validate\(nested\)\].*ClearPayloadPoints' lib/api/src/grpc/qdrant.rs -C3 || true
echo "build.rs wires attributes for nested validation:"
rg -nP 'PointsSelector\.points_selector_one_of|DeletePointVectors\.points_selector|SetPayloadPoints\.points_selector|DeletePayloadPoints\.points_selector|ClearPayloadPoints\.points' lib/api/build.rs -n -C2 || trueLength of output: 2062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate the file defining PointsSelector:"
rg -lP 'pub\s+struct\s+PointsSelector\b' -g 'lib/api/src/grpc/**/*.rs'
FILE=$(rg -lP 'pub\s+struct\s+PointsSelector\b' -g 'lib/api/src/grpc/**/*.rs' | head -n1)
echo "Checking in $FILE"
echo "1) Check for derive Validate on PointsSelector:"
rg -nP 'derive\s*\([^)]*Validate[^)]*\)\s*pub\s+struct\s+PointsSelector' "$FILE" -C2 || true
echo "2) Check for nested validation attribute on points_selector_one_of field:"
rg -nP '#\[validate\(nested\)\]\s*pub\s+points_selector_one_of' "$FILE" -C2 || trueLength of output: 228
Add missing Validate derive and nested validation attribute
In lib/api/src/grpc/qdrant.rs, the PointsSelector struct isn’t annotated with #[derive(Validate)], and its points_selector_one_of field lacks #[validate(nested)]. As a result, nested validation won’t run despite entries wired in build.rs.
🤖 Prompt for AI Agents
In lib/api/src/grpc/validate.rs around lines 481 to 488: the generated Validate
impl for PointsSelectorOneOf is fine but the PointsSelector struct in
lib/api/src/grpc/qdrant.rs is missing the #[derive(Validate)] and its
points_selector_one_of field is missing #[validate(nested)], so nested
validation never runs; to fix, add #[derive(Validate)] to the PointsSelector
struct declaration, annotate the points_selector_one_of field with
#[validate(nested)] (and ensure validator::Validate is in scope or imported),
then rebuild to regenerate/validate nested fields.
The filter within the
PointsSelectorin gRPC should be validated.Backported from #7216