fix(codegen): explain unhandled Schema type errors - #2531
Open
bendrucker wants to merge 3 commits into
Open
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
The primitive-type dispatch reported `unhandled Schema type: &[int]` -- a Go pointer formatting of kin-openapi's type slice, with no reason attached. Issue oapi-codegen#1976 hit it with a misspelled `type: int` and had to run vacuum to find out what was wrong. The dispatch handles every JSON Schema type, so falling off the end of it means the `type` is not one the document may carry. That narrows to two causes, and both are cheap to tell apart: a name that is not a JSON Schema type, or the 3.1-only list form in an earlier document. The message now names the declared value verbatim (quoted, so a typo is visible) and which of the two it is, listing the valid type names for the first and the document's declared version for the second. The `type` reported is the declared one, not the "null"-stripped value the dispatch runs on, so it matches what the reader has in their spec. Three wrappers above it lacked the context to say which schema failed. Request and response bodies now name the operation and content type, and the response body one no longer calls itself a request body; response headers name the header.
…nestly Review of the error-message change turned up three gaps in it. The response body and header wrappers built an "operation.status" label, but components.responses generate with no operation, so `-generate strict-server` without `models` reported `for .NotFound` with a stray leading dot. The label now falls back to the component name alone. The version hint read the declared version off the spec while the branch gating it read globalState.is31. SetGlobalStateSpec sets one without the other, so a 3.1 document could be told "is OpenAPI 3.1 syntax, but this document declares OpenAPI 3.1.0". The version is now a suffix that is omitted whenever it would not explain the failure. generateUnion computed a branch index for naming and dropped it on the error path, so an inline oneOf/anyOf branch with a bad type reported only the parent schema. It now names the branch. Also collapses the three near-identical unknown-name branches into one, and adds the Precondition note the file's other globalState-dependent functions carry.
Copy link
Copy Markdown
Contributor
Greptile SummaryThe PR improves invalid-schema diagnostics without accepting previously invalid schemas.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported branch-numbering issue is invalid because zero-based numbering matches generated branch names and existing JSON Pointer-style indexing. Important Files Changed
Reviews (2): Last reviewed commit: "fix(codegen): name the failing response,..." | Re-trigger Greptile |
Sorry, something went wrong.
Comment thread
pkg/codegen/schema.go
Copy link
Copy Markdown
Contributor
Author
|
@greptileai please redo the review. The one comment from the last pass was the zero-based oneOf/anyOf branch label, and you agreed on the thread that zero-based is right here. Nothing changed on the branch since, so the 4/5 is scored against a finding that no longer stands. |
Sorry, something went wrong.
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
Reviewers
At least 1 approving review is required to merge this pull request.
1 participant
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.
The primitive-type dispatch printed a Go pointer and left the user to guess. Closes #1977.
The first is #1976 exactly. examples/client/api.yaml still carries type: int, and diagnosing it took an external linter run.
After #2522 the dispatch handles every JSON Schema type, so reaching the end of it leaves two causes: a name that is no JSON Schema type, or the 3.1-only list form in an earlier document. Both stay errors, and nothing is widened to any. The message reports the type as declared, so it matches what the reader has in their spec. The version suffix drops whenever it wouldn't explain the failure, so a caller that never ran Generate() still gets the requirement without a bogus version.
Schema Position
Component schemas already carried a path. Four positions that reach this error did not:
The response wrapper called itself a request body. generateUnion computed a branch index for naming and dropped it on the error path. Branch numbering stays zero-based to match the generated type names (ParamOneOf0, ParamOneOf1) and the /oneOf/1 pointer linters report.
Reusable components.responses generate with no operation, so an operationID.statusCode label rendered as .NotFound. That is reachable with -generate strict-server and no models. It now falls back to the component name.
allOf members still report only their parent. MergeSchemas composes them into one synthetic schema before the failure, so naming a member needs provenance through the merge.
Added tests covering each message shape and each new label.