fix(cli-generator): unwrap anyOf/oneOf when detecting multipart file fields - #17304
fix(cli-generator): unwrap anyOf/oneOf when detecting multipart file fields#17304devin-ai-integration[bot] wants to merge 1 commit into
Conversation
…fields Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Targeted fix that unwraps anyOf/oneOf when classifying multipart file properties, plus the extracted is_binary_schema predicate and regenerated seed fixtures. Logic is correct for the reported case and behavior for the existing paths is preserved. A few resolution gaps remain ($ref inside items, allOf wrappers, nested compositions) that are common enough in real specs to be worth a follow-up.
- 🟡 1 warning(s)
- 🔵 2 suggestion(s)
| if ty == Some("array") { | ||
| if let Some(items) = &resolved.items { | ||
| if (items.schema_type() == Some("string") | ||
| if let Some(items) = &schema.items { | ||
| return (items.schema_type() == Some("string") | ||
| && items.format.as_deref() == Some("binary")) | ||
| || items.schema_type() == Some("file") | ||
| { | ||
| return (true, Some("application/octet-stream".to_string())); | ||
| } | ||
| || items.schema_type() == Some("file"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 warning
is_binary_schema doesn't resolve $ref in items, so the very common type: array, items: { $ref: '#/components/schemas/Upload' } (where Upload is string/binary) still falls through to a text part — the same class of bug this PR fixes, one level down. Consider threading component_schemas into the predicate and resolving items refs before checking type/format.
| // Optional / nullable uploads wrap the binary schema in a composition: | ||
| // `anyOf: [{type: string, format: binary}, {type: "null"}]`. Unwrap the | ||
| // non-null branches (resolving `$ref`) and classify on those. | ||
| for branch in resolved.one_of.iter().chain(resolved.any_of.iter()) { | ||
| let effective = branch | ||
| .schema_ref | ||
| .as_ref() | ||
| .and_then(|r| component_schemas.get(&strip_ref_prefix(r))) | ||
| .unwrap_or(branch); | ||
| if is_null_sentinel(effective) { | ||
| continue; | ||
| } | ||
| if is_binary_schema(effective) { | ||
| return (true, Some("application/octet-stream".to_string())); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔵 suggestion
Only one level of composition is unwrapped: an allOf: [{$ref: Upload}] wrapper (used to attach a description to a binary field) or a nested anyOf inside a branch will still be missed. A small bounded-depth recursive helper would cover allOf and nested unions uniformly.
| if is_binary_schema(resolved) { | ||
| return (true, Some("application/octet-stream".to_string())); | ||
| } | ||
|
|
||
| // `type: string, format: binary` or legacy `type: file` | ||
| if (ty == Some("string") && fmt == Some("binary")) || ty == Some("file") { | ||
| let ct = Some("application/octet-stream".to_string()); | ||
| return (true, ct); | ||
| // Optional / nullable uploads wrap the binary schema in a composition: | ||
| // `anyOf: [{type: string, format: binary}, {type: "null"}]`. Unwrap the | ||
| // non-null branches (resolving `$ref`) and classify on those. | ||
| for branch in resolved.one_of.iter().chain(resolved.any_of.iter()) { | ||
| let effective = branch | ||
| .schema_ref | ||
| .as_ref() | ||
| .and_then(|r| component_schemas.get(&strip_ref_prefix(r))) | ||
| .unwrap_or(branch); | ||
| if is_null_sentinel(effective) { | ||
| continue; | ||
| } | ||
| if is_binary_schema(effective) { | ||
| return (true, Some("application/octet-stream".to_string())); |
There was a problem hiding this comment.
🔵 suggestion
Some("application/octet-stream".to_string()) is now built in two places. Hoist it into a const OCTET_STREAM: &str (or a tiny fn file_part() helper) so the two return paths can't drift.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Description
Optional file uploads in generated CLIs sent the filename string as a plain text part instead of the file contents, so every optional upload was rejected by the API:
Root cause:
classify_multipart_property(generators/cli/sdk/src/openapi/parser.rs) only matched a baretype: string, format: binary/type: file(or an array of those). An optional file is emitted by most specs as a composition:which fell through to
(is_file = false), socollect_multipart_partssent the flag value as a text part.@pathdidn't help — the escape is handled inside the file branch that was never taken.The check now also walks
oneOf/anyOfbranches (resolving$ref, skipping thenullsentinel) and classifies on those, mirroring the existing nullable-union handling elsewhere in the parser:is_binary_schemais the extracted predicate (direct binary, legacytype: file, and arrays of either), so nullable arrays of files are covered too.Changes Made
generators/cli/sdk/src/openapi/parser.rs: unwrapanyOf/oneOf(incl.$refbranches) in multipart file-field classification; extractis_binary_schema.seed/cli/*fixtures (verbatim copy, no other output changes).generators/cli/changes/unreleased/.Testing
test_multipart_nullable_anyof_binary_is_file_part(inlineanyOf [binary, null]) andtest_multipart_nullable_oneof_ref_binary_is_file_part($refbranch + nullable array of binaries). Both fail before the change, pass after;cargo test --lib multipart→ 18 passed.Link to Devin session: https://app.devin.ai/sessions/a4bcc0e77e634201a1259d693a9dfe45