feat(api): update aip field filter types#4301
Conversation
tothandras
changed the title
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRedesigns field-filter schemas into explicit oneOf variants, adds a TypeSpec test and generated OpenAPI test spec (embedded), updates Go filter parsing to handle label pointer variants and merged label shapes, adds extensive validation/parsing tests, adds a YAML post-processor (seal-object-schemas.mjs) and Makefile wiring, and removes or adds some TypeSpec extension metadata/constants. ChangesField Filter Type Redesign & Test Flow
Spec Generation & Post-processing
API Extension Metadata
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant Validator as Validator
participant Parser as Parser
participant Handler as Handler
Client->>Server: GET /field-filters?filter=...
Server->>Validator: Validate request against openapi.test.yaml
alt validation OK
Validator-->>Server: OK
Server->>Parser: Parse query into typed filters
alt parsed successfully
Parser-->>Server: typed filters
Server->>Handler: Invoke handler with typed filters
Handler-->>Server: 204 / response
Server-->>Client: 204
else parse error
Parser-->>Server: parse error
Server-->>Client: 400 (parse error)
end
else validation error
Validator-->>Server: validation errors
Server-->>Client: 400 (validation error payload)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
api/v3/test/openapi.Test.yaml (1)
129-148: ⚖️ Poor tradeoffNote:
itemsontype: stringis technically non-standard OpenAPI 3.0 (informational only)Fields like
ocontainsandoeqemit:type: string items: type: string format: ArrayEncoding.commaDelimited
itemsis only valid fortype: arrayper the OpenAPI 3.0 spec. This is the TypeSpec OpenAPI3 emitter's way of encoding@encode(ArrayEncoding.commaDelimited)arrays as query params. Sincex-go-typeextensions drive Go codegen, this doesn't break anything today, but external validators or SDK generators consuming this spec may trip on it.No action needed in this file since it's generated — just good to have on the radar in case client SDK generation is added later.
🤖 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/v3/test/openapi.Test.yaml` around lines 129 - 148, The OpenAPI emitter is outputting items on a string schema for the query fields ocontains and oeq; update the emitter so that for parameters annotated with ArrayEncoding.commaDelimited (the ocontains and oeq parameter schemas) the schema emits type: array with items: { type: string } and the existing format: ArrayEncoding.commaDelimited (instead of type: string plus items), ensuring external validators/SDK generators accept the spec.api/v3/test/filters_test.go (2)
237-240: 💤 Low valueTiny nit: defer the response body close.
If a
require.*later fails betweenhttp.Getandresp.Body.Close(), the body won't be closed. Not a real leak here (the test ends andhttptestserver gets cleaned up), butdefer resp.Body.Close()right after theNoErrorcheck is the idiomatic shape and saves you from forgetting it on future edits. Same comment applies to lines 570–573.♻️ Suggested tweak
resp, err := http.Get(url) require.NoError(t, err) + defer resp.Body.Close() body, _ := io.ReadAll(resp.Body) - resp.Body.Close()🤖 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/v3/test/filters_test.go` around lines 237 - 240, After the require.NoError(t, err) check replace the explicit resp.Body.Close() call with a deferred close (defer resp.Body.Close()) immediately after NoError to ensure the response body is always closed even if subsequent assertions fail; update the same pattern for the other occurrence referenced (the block using http.Get, resp, io.ReadAll and resp.Body.Close around lines 570–573) so both use defer resp.Body.Close() right after require.NoError.
555-560: 💤 Low valueHeads up: the
unknownsubstring assertion is a bit loose.
wantBodySubstr: "unknown"will match any error mentioning "unknown" anywhere in the body, which makes this test a bit too forgiving if the parser's error wording drifts. If the parser returns something likeunknown filter field: unknown, consider tightening to that phrase so a regression in error categorization actually fails the test.🤖 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/v3/test/filters_test.go` around lines 555 - 560, The test case named "unknown filter field rejected" uses a loose substring assertion (wantBodySubstr: "unknown"); update the test case's wantBodySubstr in the same test table entry to the tighter, explicit error phrase expected from the parser (for example "unknown filter field: unknown" or the exact message your parser emits) so the assertion references the precise error text; locate the table entry by the name field "unknown filter field rejected" and change the wantBodySubstr value accordingly.api/spec/packages/aip/src/common/parameters.tsp (1)
313-318: 💤 Low valueNaming nit:
LabelsFieldEqualsFilterbreaks the*EQFilterpattern.Every other "equals" variant in this file is named
<X>FieldEQFilter(e.g.BooleanFieldEQFilter,NumericFieldEQFilter,StringFieldEQFilter,ULIDFieldEQFilter,DateTimeFieldEQFilter), but the labels union calls itLabelsFieldEqualsFilter. Worth aligning for consistency in the generated schemas/types.♻️ Suggested rename
`@summary`("Equals") - LabelsFieldEqualsFilter: { + LabelsFieldEQFilter: { /** Value strictly equals the given string value. */ eq: string, },🤖 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 313 - 318, Rename the type LabelsFieldEqualsFilter to LabelsFieldEQFilter to match the existing <X>FieldEQFilter naming convention; update its declaration (currently labeled "Equals") and all references/usages/unions/exports that mention LabelsFieldEqualsFilter so generated schemas/types stay consistent, and keep the same fields (eq: string) and summary text but adjust the summary if you want to match the "EQ" naming convention.
🤖 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.
Inline comments:
In `@api/v3/test/filters_test.go`:
- Line 32: The test fixture's fieldFiltersTarget struct uses the wrong type for
Labels (currently *filters.FilterString); update the Labels field to use
filters.FilterLabels (a map[string]FilterLabel) so it matches the OpenAPI
x-go-type and the production handler shape—locate the fieldFiltersTarget
definition in api/v3/test/filters_test.go and replace the Labels field type with
filters.FilterLabels, then run the tests to ensure compatibility with
TestParse_FilterLabels behavior.
In `@api/v3/test/openapi.Test.yaml`:
- Around line 1-5: The file name in the repo is incorrectly cased: rename
openapi.Test.yaml to lowercase openapi.test.yaml so it matches the //go:embed
pattern in api/v3/test/embed.go (the embed directive "//go:embed
openapi.test.yaml"); ensure the rename is committed so case-sensitive CI/Linux
builds find the embedded file.
---
Nitpick comments:
In `@api/spec/packages/aip/src/common/parameters.tsp`:
- Around line 313-318: Rename the type LabelsFieldEqualsFilter to
LabelsFieldEQFilter to match the existing <X>FieldEQFilter naming convention;
update its declaration (currently labeled "Equals") and all
references/usages/unions/exports that mention LabelsFieldEqualsFilter so
generated schemas/types stay consistent, and keep the same fields (eq: string)
and summary text but adjust the summary if you want to match the "EQ" naming
convention.
In `@api/v3/test/filters_test.go`:
- Around line 237-240: After the require.NoError(t, err) check replace the
explicit resp.Body.Close() call with a deferred close (defer resp.Body.Close())
immediately after NoError to ensure the response body is always closed even if
subsequent assertions fail; update the same pattern for the other occurrence
referenced (the block using http.Get, resp, io.ReadAll and resp.Body.Close
around lines 570–573) so both use defer resp.Body.Close() right after
require.NoError.
- Around line 555-560: The test case named "unknown filter field rejected" uses
a loose substring assertion (wantBodySubstr: "unknown"); update the test case's
wantBodySubstr in the same test table entry to the tighter, explicit error
phrase expected from the parser (for example "unknown filter field: unknown" or
the exact message your parser emits) so the assertion references the precise
error text; locate the table entry by the name field "unknown filter field
rejected" and change the wantBodySubstr value accordingly.
In `@api/v3/test/openapi.Test.yaml`:
- Around line 129-148: The OpenAPI emitter is outputting items on a string
schema for the query fields ocontains and oeq; update the emitter so that for
parameters annotated with ArrayEncoding.commaDelimited (the ocontains and oeq
parameter schemas) the schema emits type: array with items: { type: string } and
the existing format: ArrayEncoding.commaDelimited (instead of type: string plus
items), ensuring external validators/SDK generators accept the spec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a008ac04-898f-49b5-957e-b95580117028
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (15)
api/spec/Makefileapi/spec/packages/aip/src/common/parameters.tspapi/spec/packages/aip/src/currencies/cost-bases/operations.tspapi/spec/packages/aip/src/currencies/operations.tspapi/spec/packages/aip/src/customers/charges/operations.tspapi/spec/packages/aip/src/customers/credits/operations.tspapi/spec/packages/aip/src/defaults/operations.tspapi/spec/packages/aip/src/llmcost/operations.tspapi/spec/packages/aip/src/main.tspapi/spec/packages/aip/src/shared/consts.tspapi/spec/packages/aip/src/test.tspapi/v3/api.gen.goapi/v3/test/embed.goapi/v3/test/filters_test.goapi/v3/test/openapi.Test.yaml
💤 Files with no reviewable changes (6)
- api/spec/packages/aip/src/customers/charges/operations.tsp
- api/spec/packages/aip/src/defaults/operations.tsp
- api/spec/packages/aip/src/llmcost/operations.tsp
- api/spec/packages/aip/src/currencies/cost-bases/operations.tsp
- api/spec/packages/aip/src/currencies/operations.tsp
- api/spec/packages/aip/src/customers/credits/operations.tsp
tothandras
force-pushed
the
feat/aip-field-filter-types
branch
from
3f3dc50 to
90f7a48
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/v3/test/filters_test.go (1)
32-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLabels field type still doesn't match the OpenAPI
x-go-type.
Labelsis typed as*filters.FilterStringin this fixture, but the OpenAPI spec mapsLabelsFieldFiltertofilters.FilterLabels(which is amap[string]FilterLabel). Your own comment at lines 479-480 calls this out, and there's a separateTestParse_FilterLabelscovering the map shape — but as it stands this suite never exercises the production-wiredFilterLabelsshape end-to-end through the OAS validator, so a divergence between schema and Go-side parsing could slip through unnoticed.If switching the fixture to
filters.FilterLabelsis too invasive for this PR (since the parser API differs), consider adding at least one test that uses the realFilterLabelstarget so the validator + parser combo is exercised against the declared schema.🤖 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/v3/test/filters_test.go` at line 32, The Labels field in the test fixture is using the wrong type: it's declared as *filters.FilterString but the OpenAPI x-go-type maps LabelsFieldFilter to filters.FilterLabels (map[string]FilterLabel); update the fixture to use filters.FilterLabels for the Labels field (or add an additional test case) so the validator + parser exercise the real map shape end-to-end—specifically change the Labels declaration in the affected test struct to filters.FilterLabels (or create a new test that constructs a filters.FilterLabels instance and call the OAS validator + parser, e.g., mirror the coverage of TestParse_FilterLabels) to ensure parity between the OAS mapping and Go parsing.api/v3/test/openapi.test.yaml (1)
129-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
ocontains/oeqschemas across this file look malformed (root cause is in TypeSpec).These (and the matching ones in
NumericFieldFilterlines 181-191,StringFieldFilterlines 255-274,StringFieldFilterExactlines 327-336,ULIDFieldFilterlines 368-377) emit shapes like:ocontains: type: string items: type: string format: ArrayEncoding.commaDelimitedTwo problems here:
itemsis only valid whentype: array— ontype: stringit's silently ignored by every validator.format: ArrayEncoding.commaDelimitedisn't a recognized OpenAPI format; it looks like the TypeSpec@encode(ArrayEncoding.commaDelimited)enum identifier leaked verbatim into the schema. In OpenAPI 3, comma-delimited serialization is expressed at the parameter level viastyle/explode, not as a schemaformat.Net effect: the spec ends up describing each
oeq/ocontainsoperator as "any string", so the OAS validator accepts e.g.oeq=anything-at-allwithout enforcing the underlying item type (ULID pattern, number, etc.). That's also why the validator-side tests for these operators are pretty loose — they're actually only checking that a string was sent.Since this YAML is generated, no change needed here directly — I've left the substantive comment on
parameters.tspwhere this originates. (Also nice work on the lowercase filename — that resolves the previous embed mismatch.)🤖 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/v3/test/openapi.test.yaml` around lines 129 - 148, The generated OpenAPI schemas for the ocontains/oeq operators (and their counterparts in NumericFieldFilter, StringFieldFilter, StringFieldFilterExact, ULIDFieldFilter) are malformed because TypeSpec emitted `type: string` with `items:` and a leaked `format: ArrayEncoding.commaDelimited`; fix the TypeSpec generator in parameters.tsp so these operators are emitted as arrays (type: array + items: { ...item schema... }) when they represent multi-value filters, and remove the enum identifier from schema.format; instead surface comma-delimited serialization at the parameter level using OpenAPI's style/explode (or map `@encode`(ArrayEncoding.commaDelimited) to appropriate parameter serialization) so the schema validates item types (e.g., ULID pattern, number) rather than allowing any string.
🧹 Nitpick comments (3)
api/spec/packages/aip/src/common/parameters.tsp (1)
314-318: 💤 Low valueNaming nit:
LabelsFieldEqualsFilteris the odd one out.Every other variant in this file uses the
*Field<OP>Filtershorthand (StringFieldEQFilter,NumericFieldEQFilter,ULIDFieldEQFilter,DateTimeFieldEQFilter), but this one spells outEquals. Renaming toLabelsFieldEQFilterkeeps the family consistent and makes generated schema names predictable for downstream consumers.♻️ Suggested rename
`@summary`("Equals") - LabelsFieldEqualsFilter: { + LabelsFieldEQFilter: { /** Value strictly equals the given string value. */ eq: string, },🤖 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 314 - 318, The type name LabelsFieldEqualsFilter is inconsistent with the other field filter types; rename the declaration and all references from LabelsFieldEqualsFilter to LabelsFieldEQFilter (update the type definition and any imports/usage sites such as validators, tests, or schema generation code that reference LabelsFieldEqualsFilter) so the family matches StringFieldEQFilter, NumericFieldEQFilter, ULIDFieldEQFilter, and DateTimeFieldEQFilter and downstream generated schema names remain predictable.api/spec/packages/aip/src/test.tsp (1)
18-33: 💤 Low valueHeads up: property names that shadow built-in scalar names.
Properties like
boolean,numeric,string,ulid,datetimereuse the same identifiers as built-in scalars / Common types. TypeSpec itself is fine with this, but since multiple codegen toolchains consume these specs (and each has its quirks), it can occasionally trip up generators or schema renderers. Since this is a test fixture this is mostly fine, but worth keeping an eye on if you ever see a generator complain — quoting the keys (e.g."string"?: ...) or prefixing them is an easy out.🤖 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/test.tsp` around lines 18 - 33, The FieldFilters model uses property names that shadow built-in scalar identifiers (e.g., boolean, numeric, string, string_exact, ulid, datetime, labels) which can trip some codegen/schema tools; update the FieldFilters model to avoid shadowing by either quoting the problematic keys (e.g., "string"?: ...) or renaming/prefixing them (e.g., string_field?: Common.StringFieldFilter) consistently across the model and any consumers/tests that reference FieldFilters to ensure generators and renderers handle the keys reliably.api/v3/test/filters_test.go (1)
237-241: 💤 Low valueTiny nit:
defer resp.Body.Close()reads cleaner.Just a style nudge — using
defer resp.Body.Close()right after therequire.NoError(t, err)keeps the cleanup co-located with the resource and survives any future early returns added between the read and the close. Totally optional for these short subtests.♻️ Sketch
resp, err := http.Get(url) require.NoError(t, err) + defer resp.Body.Close() body, _ := io.ReadAll(resp.Body) - resp.Body.Close()Also applies to: 570-574
🤖 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/v3/test/filters_test.go` around lines 237 - 241, The test currently calls resp.Body.Close() explicitly after reading the body; change this to defer resp.Body.Close() immediately after require.NoError(t, err) so the response body is closed even if future early returns are added—i.e., after the http.Get(url) and require.NoError(t, err) keep resp variable and replace the later explicit resp.Body.Close() with the single defer; apply the same change to the other occurrence around the http.Get(...) / require.NoError(...) block referenced (lines ~570-574).
🤖 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.
Inline comments:
In `@api/spec/packages/aip/src/common/parameters.tsp`:
- Around line 69-74: The `@encode`(ArrayEncoding.commaDelimited) annotations
inside the union variants (e.g., NumericFieldOEQFilter.oeq and analogous
ocontains fields in other filters) are producing invalid OpenAPI (string with
items and a TypeSpec identifier as format); remove these `@encode` annotations
from the nested variants so the schema emits a proper type: array with items: {
type: ... } and let comma-delimited serialization be handled at the
parameter/operation level (style/explode) or by the request parser
(filters.Parse) instead; update all affected variants (NumericFieldOEQFilter.oeq
and similar ocontains/oeq fields referenced in the review) to use plain array
types and, if necessary, move comma-delimited serialization metadata to the
top-level parameter definitions rather than the nested object schemas.
---
Duplicate comments:
In `@api/v3/test/filters_test.go`:
- Line 32: The Labels field in the test fixture is using the wrong type: it's
declared as *filters.FilterString but the OpenAPI x-go-type maps
LabelsFieldFilter to filters.FilterLabels (map[string]FilterLabel); update the
fixture to use filters.FilterLabels for the Labels field (or add an additional
test case) so the validator + parser exercise the real map shape
end-to-end—specifically change the Labels declaration in the affected test
struct to filters.FilterLabels (or create a new test that constructs a
filters.FilterLabels instance and call the OAS validator + parser, e.g., mirror
the coverage of TestParse_FilterLabels) to ensure parity between the OAS mapping
and Go parsing.
In `@api/v3/test/openapi.test.yaml`:
- Around line 129-148: The generated OpenAPI schemas for the ocontains/oeq
operators (and their counterparts in NumericFieldFilter, StringFieldFilter,
StringFieldFilterExact, ULIDFieldFilter) are malformed because TypeSpec emitted
`type: string` with `items:` and a leaked `format:
ArrayEncoding.commaDelimited`; fix the TypeSpec generator in parameters.tsp so
these operators are emitted as arrays (type: array + items: { ...item schema...
}) when they represent multi-value filters, and remove the enum identifier from
schema.format; instead surface comma-delimited serialization at the parameter
level using OpenAPI's style/explode (or map
`@encode`(ArrayEncoding.commaDelimited) to appropriate parameter serialization) so
the schema validates item types (e.g., ULID pattern, number) rather than
allowing any string.
---
Nitpick comments:
In `@api/spec/packages/aip/src/common/parameters.tsp`:
- Around line 314-318: The type name LabelsFieldEqualsFilter is inconsistent
with the other field filter types; rename the declaration and all references
from LabelsFieldEqualsFilter to LabelsFieldEQFilter (update the type definition
and any imports/usage sites such as validators, tests, or schema generation code
that reference LabelsFieldEqualsFilter) so the family matches
StringFieldEQFilter, NumericFieldEQFilter, ULIDFieldEQFilter, and
DateTimeFieldEQFilter and downstream generated schema names remain predictable.
In `@api/spec/packages/aip/src/test.tsp`:
- Around line 18-33: The FieldFilters model uses property names that shadow
built-in scalar identifiers (e.g., boolean, numeric, string, string_exact, ulid,
datetime, labels) which can trip some codegen/schema tools; update the
FieldFilters model to avoid shadowing by either quoting the problematic keys
(e.g., "string"?: ...) or renaming/prefixing them (e.g., string_field?:
Common.StringFieldFilter) consistently across the model and any consumers/tests
that reference FieldFilters to ensure generators and renderers handle the keys
reliably.
In `@api/v3/test/filters_test.go`:
- Around line 237-241: The test currently calls resp.Body.Close() explicitly
after reading the body; change this to defer resp.Body.Close() immediately after
require.NoError(t, err) so the response body is closed even if future early
returns are added—i.e., after the http.Get(url) and require.NoError(t, err) keep
resp variable and replace the later explicit resp.Body.Close() with the single
defer; apply the same change to the other occurrence around the http.Get(...) /
require.NoError(...) block referenced (lines ~570-574).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25815b62-3711-4c97-ad0e-2c250b9e2b0b
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (8)
api/spec/Makefileapi/spec/packages/aip/src/common/parameters.tspapi/spec/packages/aip/src/main.tspapi/spec/packages/aip/src/test.tspapi/v3/api.gen.goapi/v3/test/embed.goapi/v3/test/filters_test.goapi/v3/test/openapi.test.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/v3/test/filters_test.go (1)
254-264: ⚡ Quick winMake invalid-parameter assertions order-independent.
Nice coverage here. One small reliability tweak: with
MultiError: true(Line 71), relying oninvalid_parameters[0]can get flaky if ordering changes. It’s safer to match any entry that fits the expected field/rule/reason.Proposed change
- ip := got.InvalidParameters[0] - assert.Equal(t, "query", ip.Source) - if tc.wantField != "" { - assert.Equal(t, tc.wantField, ip.Field) - } - if tc.wantRule != "" { - assert.Equal(t, tc.wantRule, ip.Rule) - } - if tc.wantReasonSubstr != "" { - assert.Contains(t, ip.Reason, tc.wantReasonSubstr) - } + var matched *validatorInvalidParameter + for i := range got.InvalidParameters { + ip := &got.InvalidParameters[i] + if ip.Source != "query" { + continue + } + if tc.wantField != "" && ip.Field != tc.wantField { + continue + } + if tc.wantRule != "" && ip.Rule != tc.wantRule { + continue + } + matched = ip + break + } + + require.NotNil(t, matched, "no matching invalid parameter in %+v", got.InvalidParameters) + if tc.wantReasonSubstr != "" { + assert.Contains(t, matched.Reason, tc.wantReasonSubstr) + }🤖 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/v3/test/filters_test.go` around lines 254 - 264, The test currently assumes order by using got.InvalidParameters[0], which can fail when MultiError: true produces different ordering; instead, scan got.InvalidParameters for any entry matching the expectations (compare ip.Field to tc.wantField if non-empty, ip.Rule to tc.wantRule if non-empty, and ip.Reason contains tc.wantReasonSubstr if non-empty), set that as the matched ip, and assert that a matching entry was found before asserting its fields—use a simple loop or helper to find the matching entry rather than a fixed index.
🤖 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.
Inline comments:
In `@api/v3/test/filters_test.go`:
- Around line 479-480: The comment describing LabelsFieldFilter is out of date:
it says the labels field is handled as *FilterString but the fixture and cases
now use *filters.FilterLabels (see LabelsFieldFilter and filters.FilterLabels in
filters_test.go); update the inline comment to accurately state that
LabelsFieldFilter is handled as *filters.FilterLabels (or remove the remark if
redundant) so the comment matches the test fixture and the cases below.
---
Nitpick comments:
In `@api/v3/test/filters_test.go`:
- Around line 254-264: The test currently assumes order by using
got.InvalidParameters[0], which can fail when MultiError: true produces
different ordering; instead, scan got.InvalidParameters for any entry matching
the expectations (compare ip.Field to tc.wantField if non-empty, ip.Rule to
tc.wantRule if non-empty, and ip.Reason contains tc.wantReasonSubstr if
non-empty), set that as the matched ip, and assert that a matching entry was
found before asserting its fields—use a simple loop or helper to find the
matching entry rather than a fixed index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e83628e-197d-40e4-8320-3ec6df9115c6
📒 Files selected for processing (1)
api/v3/test/filters_test.go
tothandras
force-pushed
the
feat/aip-field-filter-types
branch
from
99b576e to
9716186
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Summary by CodeRabbit
New Features
Enhancements
Tests / Chores