fix(params): handle deepObject style with anyOf/oneOf schemas#7
Merged
fix(params): handle deepObject style with anyOf/oneOf schemas#7
Conversation
Previously, query params with style=deepObject and a top-level anyOf or
oneOf schema (no top-level type:object) were always rejected, even when
optional and absent. The deserialize_param object branch only triggers
when stype == 'object', so anyOf schemas fell through to coerce_value on
the placeholder string and failed the anyOf.
This affects common real-world specs — e.g. Stripe's range_query_specs
(created, transacted_at, arrival_date, ...) accept either a Unix
timestamp (integer) or a {gt, gte, lt, lte} object via deepObject style.
50 Stripe operations were affected.
Fix: when style=deepObject and the schema has anyOf/oneOf, try
parse_deep_object against each object branch first; if no param[...] keys
are present, try a scalar branch against the bare param value.
Adds three regression tests covering: missing optional, object-form
deepObject, and integer-form scalar.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtend deepObject handling in parameter deserialization when the parameter schema uses anyOf/oneOf: prefer object-typed branches and run parse_deep_object on them; if none parse, fall back to reading and coercing the raw query value against the composite schema. Unit tests added for these cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
t/unit/test_params.lua (1)
165-220: Add a matchingoneOfregression case.The new tests cover
anyOfwell, but the runtime path also handlesoneOf. Adding onedeepObject + oneOfcase would lock this fix against regressions in both branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@t/unit/test_params.lua` around lines 165 - 220, Add a regression test mirroring the existing deepObject anyOf cases but using oneOf: create a new T.describe block that builds a route with make_route where the "created" query param uses style="deepObject", explode=true and schema = { oneOf = { { type="object", properties = { gt={type="integer"}, lte={type="integer"} } }, { type="integer" } } }; then call params_mod.validate with both the object-form params (e.g. ["created[gt]"]="1700000000", ["created[lte]"]="1800000000") and a scalar-form params (e.g. ["created"]="1700000000") in separate tests, asserting ok is true and errs is nil/empty just like the anyOf tests to ensure oneOf runtime path remains covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/resty/openapi_validator/params.lua`:
- Around line 276-281: The loop over branches uses coerce_value(scalar_raw,
branch) and accepts any non-nil return (v ~= nil), but coerce_value can return
non-nil even when coercion failed, causing the first scalar branch to win;
change coerce_value to return two values (coerced_value, ok) or otherwise expose
a success flag, then in the branches loop check the success flag (e.g., local v,
ok = coerce_value(scalar_raw, branch); if ok then return v end) so only truly
successful coercions for a branch are accepted.
---
Nitpick comments:
In `@t/unit/test_params.lua`:
- Around line 165-220: Add a regression test mirroring the existing deepObject
anyOf cases but using oneOf: create a new T.describe block that builds a route
with make_route where the "created" query param uses style="deepObject",
explode=true and schema = { oneOf = { { type="object", properties = {
gt={type="integer"}, lte={type="integer"} } }, { type="integer" } } }; then call
params_mod.validate with both the object-form params (e.g.
["created[gt]"]="1700000000", ["created[lte]"]="1800000000") and a scalar-form
params (e.g. ["created"]="1700000000") in separate tests, asserting ok is true
and errs is nil/empty just like the anyOf tests to ensure oneOf runtime path
remains covered.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5dcdebe9-3e67-464a-85e3-6dc5c8638457
📒 Files selected for processing (2)
lib/resty/openapi_validator/params.luat/unit/test_params.lua
…ssion tests - Replace the per-branch coerce_value loop with a single coerce_value call against the full anyOf/oneOf schema. coerce_value returns the input as-is on failed coercion, so picking the first 'non-nil' result was meaningless; the downstream jsonschema validator already evaluates every branch. - Add deepObject + oneOf object/integer regression tests (mirror anyOf). - Add negative regression: deepObject anyOf with an unmatched scalar value must be rejected.
Contributor
Author
|
Also addressed the oneOf nitpick — added two oneOf regression tests (object branch + integer branch) mirroring the anyOf cases. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/resty/openapi_validator/params.lua`:
- Around line 265-279: The deepObject branch check only matches branch.type ==
"object" and thus misses array-valued type declarations like {"object","null"};
update the loop over branches to treat branch.type as either a string or an
array and detect whether it "contains" "object" (e.g., if branch.type ==
"object" or if type(branch.type) == "table" and any element == "object"), then
call parse_deep_object(param.name, query_args or {}, branch) and return the
parsed object if non-nil; keep the existing scalar handling (scalar_raw ->
coerce_value(scalar_raw, schema)) unchanged. Ensure you still iterate with
ipairs(branches) so behavior remains deterministic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46cfae49-b8e8-4d67-a70b-16beda3ce6c8
📒 Files selected for processing (2)
lib/resty/openapi_validator/params.luat/unit/test_params.lua
🚧 Files skipped from review as they are similar to previous changes (1)
- t/unit/test_params.lua
…ct,null})
CodeRabbit pointed out that 'branch.type == "object"' misses union type
arrays produced by nullable normalisation (type = {"object", "null"}).
Added a small helper that treats branch.type as either a string or a list.
Added a regression test covering the nullable object branch.
(Did not adopt the proposal to per-branch validate and pick the matching
object branch: parse_deep_object is schema-agnostic about which keys it
collects, so all object branches see the same parsed object, and the
downstream jsonschema validator on the full anyOf/oneOf already evaluates
every branch. Branch order only affects per-property type coercion via
coerce_object_values, and in practice deepObject filters use uniform
property types — adding per-branch jsonschema validation here would
duplicate work without changing acceptance behaviour.)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/resty/openapi_validator/params.lua`:
- Around line 265-276: The branch detection currently only checks branch.type
and misses object schemas expressed via composition; update the
branch_has_object function to recursively inspect a branch for object types by
traversing composition keys (allOf, anyOf, oneOf), nested schemas, and common
object indicators (properties, additionalProperties) in addition to branch.type;
ensure branch_has_object(branch) returns true if any nested schema or composed
entry has type "object" (including when type is an array), so the loop over
branches correctly detects deepObject-capable object branches.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45dee1a6-564b-481f-b000-ef79102d8b03
📒 Files selected for processing (2)
lib/resty/openapi_validator/params.luat/unit/test_params.lua
✅ Files skipped from review due to trivial changes (1)
- t/unit/test_params.lua
CodeRabbit pointed out that the local branch_has_object helper would miss
object branches expressed via composition (e.g. anyOf:[{allOf:[{type:object,...}]}, ...]).
Replaced the ad-hoc check with collect_types(b)['object'], which already
recursively walks anyOf/oneOf/allOf and handles union type arrays.
Added a regression test using an allOf-wrapped object branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fix a bug where query parameters with
style: deepObjectand a top-levelanyOf/oneOfschema (no top-leveltype: object) were always rejected, even when the parameter was optional and absent.Why
deserialize_param's object branch only triggers whenstype == "object". For ananyOfschema there's no top-leveltype, so the request fell through tocoerce_value("deepObject_placeholder", schema)and failed theanyOf(the placeholder string matches neither the object nor any scalar branch).Common in real-world specs — e.g. the Stripe API spec uses this pattern for every range filter (
created,transacted_at,arrival_date,canceled_at,current_period_end,effective_at,amount):A QA pass against the full Stripe OpenAPI spec found this bug affects ≥50 operations. Reproducer:
How
In
deserialize_param, whenstyle == "deepObject"and the schema hasanyOf/oneOf:parse_deep_object. If it returns a non-nil result (i.e. the user suppliedparam[key]=valuekeys), use it.?param=value, try coercing it against each non-object branch (e.g. integer/number).validate_param_groupreturnsnilinstead of an error).Tests
Added 3 regression tests in
t/unit/test_params.lua:deepObject anyOf optional missing— no param supplied, optional → accepteddeepObject anyOf object branch—?created[gt]=1700000000&created[lte]=1800000000→ accepteddeepObject anyOf integer branch—?created=1700000000→ acceptedmake testpasses (27 unit tests + all conformance).Impact on Stripe corpus
Re-running the full Stripe per-operation corpus (587 operations × 1 valid request each) before/after this PR:
The remaining 43 failures are all generator-side artefacts (Stripe's deeply-nested
additionalProperties: falsebodies and array-of-objectdeepObjectarrays not synthesized correctly), not validator bugs.Summary by CodeRabbit
Bug Fixes
Tests