fix(cli): inherit path-item-level OpenAPI parameters - #7606
Conversation
OpenAPI 3.x lets a Path Item Object declare `parameters` shared by every
operation under that path, with operation-level entries overriding them by
(name, in). The codegen read only `op.get("parameters")`, so inherited
parameters were dropped silently — the generated command kept the URL
template but had nothing registered to fill it.
Real-world impact: the Xero Accounting API declares its required
`xero-tenant-id` header once per path item across all 138 paths. Before this
change `build_command_spec` produced 78 commands and none carried the
header, so every generated command 403s. Nothing warned.
Adds `operation_parameters()`, which merges the two levels and dereferences
both, and routes the pre-computed spec path (`build_command_spec`) and the
interactive path (`build_command_index` -> `build_parser_from_operation`)
through it. Unresolvable refs, non-dict entries, and nameless parameters are
skipped as before. Specs without path-item parameters are unaffected.
|
|
|
Some additional data on how common this pattern is, in case it helps with prioritizing. I ran a path-item-parameter check across the APIs.guru directory — 2,510 published specs fetched successfully. 673 of them (27%) declare parameters at the path-item level, so every command generated from those specs currently drops them:
Restricted to the finance/banking/payments subset, it's 25 of 160 specs (16%). In Xero's and Codat's case the inherited parameter is a required auth header, so the generated commands can't succeed at all — which is how I ran into it. While I had the corpus loaded I also checked three adjacent conformance gaps, and none of them look worth writing code for:
The |
|
Hey @DerMayer1, thanks for the PR. Did you try adding the If these do not work as-expected, then that is a bug. |
|
You're right about the header, and my example was a bad one. A constant The general problem isn't headers though. Path-item
108 of the 673 specs declare Others in that set: Microsoft Graph (12,001 paths), Keycloak (191), Apple App Store Connect (133), Bitbucket (117), Jira (115). On scope, if you'd rather this only inherit non-header parameters and leave headers to |
|
Can you try reframing the problem with clear scope? There are certainly API endpoints which will never universally translate into Python Interface functions, such as webhooks, OIDC, websockets, etc. The codegen detects SecuritySchema and wraps it into the Path and query parameters are already handled across all interfaces, but your example won't track across all the surfaces because a Python function does not have "headers" by its very nature. Only external HTTP requests to an API server carry headers - i.e, If the goal is to handle an The config system (
|
| Field | Type | Default |
|---|---|---|
docstring_sections |
list[str] |
["description", "parameters", "returns", "examples"] |
docstring_max_length |
PositiveInt | None |
None |
http |
dict | None |
{} |
uvicorn |
dict | None |
{} |
docstring_sections controls which sections are included in autogenerated docstrings.
docstring_max_length caps the length of those docstrings. None means no cap.
http is consumed by openbb_core.provider.utils.helpers.{make_request, amake_request, amake_requests, get_requests_session, get_async_requests_session}. Keys:
| Key | Applies to | Purpose |
|---|---|---|
cafile |
both | Path to a CA certificate file. |
certfile |
both | Path to a client certificate file. |
keyfile |
both | Path to a client key file. |
password |
aiohttp | Password for the client key file. |
verify_ssl |
both | Verify SSL certificates. |
fingerprint |
aiohttp | SSL fingerprint. |
proxy |
both | Proxy URL. |
proxy_auth |
aiohttp | Proxy authentication. |
proxy_headers |
aiohttp | Proxy headers. |
timeout |
both | Request timeout. |
auth |
both | Basic authentication. |
headers |
both | Request headers. |
cookies |
both | Session cookies. |
Additional keys are ignored unless explicitly implemented by custom code.
Per review: headers do not translate into a Python interface function, and outbound headers are transport configuration (SystemSettings.PythonSettings.http.headers, the CLI's -H) or a security-scheme concern — not a per-command argument. Path-item `in: header` and `in: cookie` entries are now dropped rather than inherited. Operation-level parameters are untouched, so an operation that declares its own header keeps today's behavior. Scope is now only what cannot be supplied any other way: `in: path` and `in: query` parameters declared on the Path Item Object.
|
That reframing is right, and I've pushed a commit that narrows the PR to match it. Scope, explicitly: Out. Headers and cookies. You're correct that In. That's the whole remaining claim. Path and query parameters are handled across all interfaces when they're declared on the operation — what isn't handled is the same parameter declared one level up, which the codegen never read. End to end on Codat's accounting spec: The argument can't be passed, and the unsubstituted placeholder goes out on the wire. Narrowing shrinks the blast radius: of the 673 specs in the APIs.guru corpus that use path-item parameters, 396 declare path or query there (108 path, 313 query) and are affected; the remaining 277 are header/cookie-only and this PR now correctly ignores them. Xero — my original example — is in that ignored group, which I think is the right outcome given Changed: inherited parameters are filtered to |
|
Thanks for the details, @DerMayer1. This resolves the params side, and the security schema builds correctly as a provider credential that gets inserted into the request headers; however, the response schema is not getting dug up from the $ref. If you can apply the same treatment for those, with consideration that some may contain nested references, that will round it out nicely! 
|
Response-schema $refs were already expanded by deref_schema, but a schema
whose top level is an allOf composition was left unflattened. The properties
live inside the members, so a consumer reading them off the composition finds
none. fetcher_gen._data_schema looks for `properties.results`, gets nothing,
and falls through to _unwrap_schema_envelopes, which handles oneOf and anyOf
but not allOf. The generated Data model ends up with no fields.
Codat's listBills response is `allOf: [{properties: {results}}, {properties:
{_links, pageNumber, pageSize, totalResults}}]`, which produced an empty model.
merge_allof collapses a composition of object subschemas into one object
schema: properties unioned with the first declaration winning on conflict,
required unioned, and sibling keywords on the allOf node outranking members.
A composition that is not a plain intersection of object subschemas (a member
carrying its own oneOf/anyOf, or a non-object type) is returned unchanged
rather than flattened into something the spec does not describe. Nested
compositions are merged too, so a $ref inside a merged member still resolves.
Applied to extract_response_schema and extract_response_schemas.
Measured across five specs that use the pattern, counting operations whose
response schema exposes readable properties: 244 of 588 before, 394 after
(Codat Accounting 36 to 88, Codat Banking 0 to 8, Codat Commerce 1 to 11,
Rebilly 62 to 104, Box 145 to 183).
|
Pushed. I went in expecting the refs and spent a while there before realising they were already fine. Nothing merges those, so the properties are one level below where anything looks for them.
I left compositions alone when they aren't a plain intersection of objects, so a member with its own
Across the specs I had loaded, counting operations whose response schema actually exposes properties:
One thing I didn't touch: 7 new tests, 157 in that file. |
deeleeramone
added
bug
Seems like this would be a good idea to handle it here, eh? |
Same gap as the response side. request_body_parameters needs `type: object` or `properties` to flatten a body into parameters, and an allOf composition carries neither, so a POST or PUT generated from one gets no body parameters at all. extract_request_body_schema now runs merge_allof after deref_schema, matching extract_response_schema. Across the same five specs, counting request bodies that produce parameters: 138 of 206 before, 177 after (Codat Accounting 1 to 26 of its 30 bodies, Rebilly 50 to 63, Box 87 to 88).
f4a3ed9 to
5886c6b
Compare
|
Done, pushed. Same treatment on Request bodies that produce parameters, same specs as before:
Codat is the one that really moves, since nearly every prototype schema there is a composition. 158 tests in that file now, |
|
Hey @DerMayer1, I'm still not seeing the response model being captured. It also seems to have duplicated some parameters, which generates invalid syntax. Potentially fixed one thing but broke another? 
I'm not familiar with this service, so are these "options" endpoints valid or are they being interpreted incorrectly? 
|
_mergeable_allof_members required every member's keys to come from a small whitelist, so any unrecognized keyword disqualified the whole composition. Codat's PagingInfo carries `definitions` next to its properties, which meant the live Codat-Accounting.yaml never merged and its response models stayed empty. The APIs.guru copy of the same API omits that key, which is why the earlier measurements looked clean. The check is now a blocklist. A member blocks the merge when it carries its own combinator (oneOf/anyOf/allOf/not), an unresolved $ref left by a cycle, a scalar or array constraint (enum/const/items), or a type other than object. Annotation keywords that do not affect the intersection (title, description, examples, definitions, additionalProperties, vendor x- keys) no longer block it, and are dropped from the merged result along with the members. On the live Codat spec, operations whose response schema exposes readable properties go from 36 of 127 to 114. Across the five specs measured earlier: 244 of 588 before, 488 after (was 394 under the whitelist). Corpus check over 2,816 response and request-body extractions in 6 specs: 567 changed, every one attributable to an allOf in the pre-merge output, 0 unexplained. Twilio declares no allOf compositions and is unchanged.
…perty
_signature_params appended request-body properties and then operation
parameters without deduplicating, so a name declared in both places produced
two function parameters with the same name and the generated module raised
SyntaxError.
Codat's push endpoints hit this: /push/bankAccounts/{accountId}/bankTransactions
declares accountId as a path parameter and repeats it in the request body, so
connections.push.bankAccounts.bankTransactions generated `accountId: str` and
`accountId: str = None` in one signature.
The operation parameter wins, since it carries the path placeholder and its own
required flag. The request payload is unchanged: _render_body_block derives body
fields from request_body_schema rather than from this list, so accountId is
still sent in the body.
This predates the path-item and allOf work in this PR. On origin/v5 the same
spec produces the same duplicate; on this branch the Codat spec generates 91
commands with 0 duplicated signatures, down from 1.
_render_body_block already writes `{'from': from_}`, keying the payload by the
wire name and reading a safe identifier, but _signature_params declared the raw
name. A body property named after a Python keyword therefore emitted
`from: str = None` and the generated module raised SyntaxError.
This did not fire before the allOf work in this PR, because a composed request
body produced no body parameters at all. Codat's Transfer body is such a
composition and has a `from` property, so connections.push.transfers started
generating invalid syntax once the body flattened.
_signature_params now emits safe_field_name(name)[0], the same helper
fetcher_gen and pydantic_gen already use. The body/query split in
_render_body_block compares against the safe names too: it previously matched
signature names against wire names, so `from_` was not recognized as a body
field and leaked into the query string as a `from_` parameter.
End to end on Codat-Accounting.yaml, generating the full extension and byte
compiling every emitted module: 136 files, 0 failures. origin/v5 emits 1 file
that does not compile (duplicate argument 'accountId').
Known gap left alone: an operation parameter named after a keyword would still
break, since the query and path lines use the raw name as a local variable.
None of the five specs checked declares one.
…ntifiers
Two defects found by generating extensions from real specs and byte compiling
every emitted module.
merge_allof and deref_schema walked every value in a schema dict, including
keywords that hold instance data rather than subschemas. A `default` of
`{"allOf": [{"a": 1}, {"b": 2}]}` was rewritten to `{"type": "object"}`, and a
`$ref` key inside an example was resolved as if it were a reference. Both now
copy `default`, `example`, `examples`, `enum`, `const` and vendor `x-` values
through untouched.
_signature_params emitted operation parameter names verbatim, so a wire name
that is not a Python identifier produced invalid syntax. Rebilly declares an
`Organization-Id` header and a `REB-APIKEY` security header, which emitted
`REB-APIKEY: str = None`. It now uses safe_field_name, matching fetcher_gen,
and _render_body_block keys the query dict by the wire name while reading the
safe identifier so the request is unchanged.
This surfaced through the allOf work rather than being caused by it: a composed
request body used to yield no properties, so `has_body` was false in
package_gen and the command was generated as a GET fetcher, where identifiers
were already sanitized. With bodies flattening, 14 of Codat's 24 POST endpoints
correctly move to the POST path, which had never been exercised by these specs.
Generating the full extension and compiling every module:
Codat-Accounting.yaml v5: 1 failure branch: 0 (136 files)
Rebilly 2.1 v5: 14 failures branch: 1 (97 files)
The one remaining Rebilly failure is `async def 3dsecure(` in the router, which
fails identically on v5. Sanitizing that is a naming decision about the command
surface, so it is left alone here.
Known gap: a non-credential header parameter is still placed in the query
string on the POST path. Pre-existing, and unchanged by this PR.
The previous dedupe compared wire names, but the failure it prevents depends on the sanitized identifier. Two wire names can collapse onto one (`from` and `from_`, `Organization-Id` and `Organization_Id`), and operation_parameters keeps a name declared twice under different `in` locations by design, so both cases could still emit a duplicate argument. _signature_params now tracks the identifiers it has emitted and skips anything that would repeat one. Emission order is unchanged, so existing signatures are byte-identical; only genuinely colliding names are dropped, and operation parameters still win over body properties. Neither collision occurs in Codat-Accounting.yaml or Rebilly 2.1. This closes the class rather than the instance, since the instances are what this PR has been finding. Generation unchanged: Codat 136 files 0 failures, Rebilly 97 files 1 failure (`async def 3dsecure(`, identical on v5).
|
Both real. The duplicate isn't from this PR, the empty response model was my fault for a reason I didn't expect, and chasing them turned up a third thing. Taking the duplicate first. That path item declares no The operation parameter wins now, since it carries the path placeholder. The payload is unchanged because The response model is on me, though not for the reason you'd guess. I validated the merge against the APIs.guru copy of Codat 2.1.0 and you generate from It's a blocklist now: a member blocks the merge only if it has its own combinator, an unresolved The third thing is the one I'd look at first. That reclassification then exposed two things I did break, both found by generating the extension and importing every module rather than reading the diff. Codat's Transfer body has a property named Generating full extensions and importing every module:
The 5 on Codat is one Also ran the wider check again after loosening the member rule: 2,816 response and request-body extractions across 6 specs, 567 changed, every change traceable to an On the options endpoints, those are genuine. Codat's create/update model endpoints sit at Three things I deliberately left alone. Non-credential header parameters end up in the query string, but that's true on the fetcher path too and you've already said 10 tests added across the two files. |
Mutation checking the new tests (revert each fix, confirm its test fails) found two problems with the previous commits. The guard that stops merge_allof and deref_schema from walking into `default`, `example`, `enum`, `const` and vendor values shipped with no test at all. It is now covered from both directions: a default holding an `allOf` key keeps its value, and a `$ref` inside a default stays a literal while a real reference beside it still resolves. The body-property loop also carried a redundant check. It tested the raw wire name against the operation parameter names before testing the sanitized one, but `operation_param_names` holds both forms and `safe_field_name` is idempotent on identifiers, so the first test could never fire alone. Reverting it left every test passing, which is what surfaced it. Removed, and the sanitized name is now computed once. All nine mutations are caught by the suite: both data guards, the allOf blocklist in each direction, the response merge wiring, body/parameter precedence, the keyword identifier, the body/query split and the emitted-identifier dedupe. cli/tests at 1,954 passed against the same 80 environment failures as the base. Generation unchanged: Codat 136 files 0 failures, Rebilly 97 files 1 failure.
|
Correction to my last comment: I said 10 tests added, it is 31 across the PR and 6 since that comment. Overstating my own verification is the wrong direction to be wrong in, so flagging it. Went back and mutation checked the tests, reverting each fix and confirming its test fails. Two things came out of that. The guard that stops The body-property loop also had a branch that could never fire. It tested the raw wire name against the operation parameter names before testing the sanitized one, but that set holds both forms and All nine mutations are caught now: both data guards, the
|
3 participants
What
OpenAPI 3.x lets a Path Item Object declare
parametersthat apply to every operation under that path, with operation-level entries overriding them by(name, in). The V5 codegen only readop.get("parameters"), so path-item parameters were dropped — silently. The generated command kept the URL template but had nothing registered to satisfy it.This is the same silent-success failure surface as #7585, from a different root cause: the generator reports success, and the breakage only shows up when the user calls the command.
Real-world repro
The Xero Accounting API declares its required
xero-tenant-idheader once per path item, across all 138 paths:Running
build_command_specagainst the live spec:xero-tenant-idXero returns 403 without that header, so before this change every generated command was dead on arrival with no warning.
How
Adds
operation_parameters(spec, path_item, op)toopenapi_schema.py. It merges the two levels, dereferences$refon both, and applies the spec's override rule keyed on(name, in)— so a same-named parameter in a different location is correctly kept rather than swallowed.Both consumers route through it:
build_command_spec→_build_operation_entry— the pre-computed--generate-specpathbuild_command_index→build_parser_from_operation— the interactive--serverpathbuild_router_map,build_reference, anddetect_api_prefixonly read path keys and descriptions, so they're unaffected.Unresolvable
$refs, non-dict entries, and nameless parameters are skipped exactly as before.build_parser_from_operationkeeps its previous operation-only behavior when called without aspec, and specs with no path-item parameters produce byte-identical output.Tests
14 new tests covering: inheritance,
$refresolution on both levels, operation-over-path-item override,(name, in)keying, inheritance across multiple operations under one path item, and the defensive skip paths.(2005 before this change, plus the 14 new.)