fix(api): API multiple filters#4322
Conversation
📝 WalkthroughWalkthroughThis PR refactors field filter schemas from a ChangesField Filter Schema Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/spec/packages/aip/src/common/parameters.tsp (1)
38-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading "provide exactly one" doc text — update across all filter types.
The descriptions for
NumericFieldFilter,StringFieldFilter,StringFieldFilterExact,ULIDFieldFilter, andDateTimeFieldFilterall say:"All properties are optional; provide exactly one to specify the comparison."
But the entire point of this PR is to allow multiple operators simultaneously (e.g.
gte+ltefor a range). The text now actively contradicts the intended behavior, which is going to confuse API consumers reading the generated docs.✏️ Suggested doc update (shown for NumericFieldFilter, same pattern for the others)
/** * Filter by a numeric value. - * All properties are optional; provide exactly one to specify the comparison. + * All properties are optional; operators can be combined (e.g. gte + lte for a range). */And the same one-line fix in
StringFieldFilter,StringFieldFilterExact,ULIDFieldFilter, andDateTimeFieldFilter. The generatedopenapi.test.yamldescriptions will then update automatically on the next codegen run.Also applies to: 76-77, 124-125, 150-151, 175-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/spec/packages/aip/src/common/parameters.tsp` around lines 38 - 39, Update the misleading docstrings for NumericFieldFilter, StringFieldFilter, StringFieldFilterExact, ULIDFieldFilter, and DateTimeFieldFilter: replace the line "All properties are optional; provide exactly one to specify the comparison." with a sentence that states multiple operators may be provided (e.g. "All properties are optional; one or more operators may be provided to combine comparisons (for example, gte and lte to express a range).") so the docs reflect that filters can accept multiple operators simultaneously.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@api/spec/packages/aip/src/common/parameters.tsp`:
- Around line 38-39: Update the misleading docstrings for NumericFieldFilter,
StringFieldFilter, StringFieldFilterExact, ULIDFieldFilter, and
DateTimeFieldFilter: replace the line "All properties are optional; provide
exactly one to specify the comparison." with a sentence that states multiple
operators may be provided (e.g. "All properties are optional; one or more
operators may be provided to combine comparisons (for example, gte and lte to
express a range).") so the docs reflect that filters can accept multiple
operators simultaneously.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6aafb01b-9401-42da-9479-f4149ec66fde
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (5)
api/spec/packages/aip/src/common/parameters.tspapi/v3/api.gen.goapi/v3/filters/parse_test.goapi/v3/test/filters_test.goapi/v3/test/openapi.test.yaml
Summary by CodeRabbit
Release Notes
New Features
Tests