Conversation
📝 WalkthroughWalkthroughAdds nested validation for the Filter.min_should field across API and segment layers. In lib/api/build.rs, two validation targets are registered for Filter.min_should and MinShould.conditions. In lib/api/src/grpc/qdrant.rs, a new public MinShould struct is introduced with Validate derive; Filter.min_should and MinShould.conditions receive #[validate(nested)]. In lib/segment/src/types.rs, MinShould.conditions also gains #[validate(nested)]. No existing validations are removed, and no public signatures (other than the new type) are modified. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/segment/src/types.rs (1)
3250-3256: Optional: validate min_count against conditions and deny unknown fieldsAdd a schema validator to ensure min_count does not exceed conditions.len() (and optionally forbid unknown fields for parity with Filter).
Apply:
#[derive(Debug, Deserialize, Serialize, JsonSchema, Validate, Clone, PartialEq, Default)] -#[serde(rename_all = "snake_case")] +#[serde(rename_all = "snake_case")] +#[schemars(deny_unknown_fields)] +#[validate(schema(function = "validate_min_should"))] pub struct MinShould { #[validate(nested)] pub conditions: Vec<Condition>, pub min_count: usize, }And add (outside the hunk):
fn validate_min_should(ms: &MinShould) -> Result<(), ValidationError> { if ms.min_count > ms.conditions.len() { return Err(ValidationError::new("min_count_must_be_lte_conditions_len")); } Ok(()) }If zero should be disallowed, extend the check to enforce ms.min_count >= 1.
lib/api/build.rs (1)
271-273: Consider constraining MinShould.min_count at proto level (if 0 is invalid)Add a range constraint so invalid requests are rejected before hitting runtime.
Apply:
("Filter.min_should", ""), ("MinShould.conditions", ""), + ("MinShould.min_count", "range(min = 1)"),Please confirm whether min_count = 0 is a supported no-op; if yes, skip this.
lib/api/src/grpc/qdrant.rs (1)
6596-6605: Validate each MinShould.conditions element — LGTM; consider guarding min_countThe nested validation on
conditionsis correct. Optionally, add a simple bound onmin_countto prevent trivially unsatisfiable configs (e.g., min_count > conditions.len()) and/or zero:Example minimal bound via attribute (if zero is not meaningful in your API):
pub struct MinShould { #[prost(message, repeated, tag = "1")] #[validate(nested)] pub conditions: ::prost::alloc::vec::Vec<Condition>, #[prost(uint64, tag = "2")] + #[validate(range(min = 1))] pub min_count: u64, }If you need
min_count <= conditions.len()at runtime, add a custom validator in build.rs for this field or an impl in the non-generated layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
lib/api/build.rs(1 hunks)lib/api/src/grpc/qdrant.rs(1 hunks)lib/segment/src/types.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/build.rslib/segment/src/types.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/segment/src/types.rslib/api/src/grpc/qdrant.rs
🧠 Learnings (2)
📚 Learning: 2025-08-10T18:30:02.986Z
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.
Applied to files:
lib/api/build.rs
📚 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.rs
🧬 Code graph analysis (2)
lib/segment/src/types.rs (2)
lib/collection/src/operations/types.rs (4)
validate(641-647)validate(1794-1799)validate(1926-1928)validate(1959-1961)lib/api/src/grpc/validate.rs (5)
validate(14-14)validate(19-21)validate(29-31)validate(39-41)validate(49-54)
lib/api/src/grpc/qdrant.rs (2)
lib/segment/src/types.rs (5)
validate(784-790)validate(1580-1590)validate(1910-1921)validate(1952-1959)validate(3015-3026)lib/api/src/grpc/validate.rs (7)
validate(14-14)validate(19-21)validate(29-31)validate(39-41)validate(49-54)validate(58-64)validate(68-74)
⏰ 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: e2e-tests
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: integration-tests-consensus
- GitHub Check: test-consistency
- GitHub Check: test-consensus-compose
- GitHub Check: integration-tests
- GitHub Check: lint
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: storage-compat-test
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests (windows-latest)
🔇 Additional comments (3)
lib/segment/src/types.rs (1)
3253-3255: LGTM: nested validation for MinShould.conditions is correctly addedThis enables recursive validation of each Condition as intended.
lib/api/build.rs (1)
271-273: LGTM: proto-level nested validation wired for Filter.min_should and MinShould.conditionsThis will auto-derive Validate for Filter and MinShould and apply #[validate(nested)] on both fields.
lib/api/src/grpc/qdrant.rs (1)
6592-6594: Nested validation on Filter.min_should — good catchBrings
min_shouldin line withshould/must/must_not, ensuring inner conditions are validated. LGTM.
The
Conditionswhen using aMinShouldare currently not validated (REST & gRPC).Backported from #7216