fix: valid specs generate unusable params, bodies, and enum constants - #49
Merged
Conversation
Closes #15, #16, #17, all reported against the Mealie OpenAPI 3.1 spec. An enum of comparison operators sanitized every value to the same identifier, so the constants came out numbered (RelationalOperator2..7) with nothing to say which was which. Punctuation-only values are now spelled out: "<>" becomes RelationalOperatorNotEqual. A parameter typed `anyOf: [{type: string}, {type: "null"}]` -- how OpenAPI 3.1 spells "nullable string" -- generated *any, which callers had no way to construct a value for. Such a union collapses to the variant's own type, as does one whose members are all refinements of a single type (a uuid-formatted string or a plain string). Members that are inline schemas now keep their Go type instead of degrading to any, and a `type: null` member no longer becomes a variant of its own. A multipart/form-data body was JSON-marshaled and sent as application/json. The declared media type now selects the encoding: multipart is written part by part, url-encoded bodies become form pairs, and a body in any other media type is sent as the bytes or text it already is. Binary properties of a multipart body generate a FormFile carrying a filename and content type, which is what makes a server treat the part as an upload at all. Fixed along the way, each of which produced a client that did not compile or silently lost data: - Mutually recursive schemas emitted `type A = B; type B = A`. Go expands aliases eagerly, so no indirection saves them -- `[]` included. - A body offered as both JSON and multipart typed its binary properties as FormFile while emitting no such type. - A spec-controlled media type was interpolated into Go source unquoted. - additionalProperties never reached the wire on a non-JSON body. - A CRLF in a filename or content type could inject MIME headers. Generated clients are exercised, not just compiled: the e2e tests run the client against a server that parses what it sent.
The templates always declare Client, APIError, RetryConfig and friends at package scope. A schema of the same name was emitted alongside them, and Go has one package scope, so the client did not compile. Those names are now reserved before any schema is converted, so such a schema is renamed the same way any other collision is. FormFile, added for multipart bodies, joined that set and is reserved with them. Reserving surfaced a second bug in the same area: a reference to a schema converted later fell back to its exported spelling rather than the name the schema was actually assigned. Two schemas differing only in punctuation share one spelling, so `foo-bar` and `foo_bar` both became FooBar and a forward reference to the second silently pointed at the first -- it compiled, and decoded into the wrong type. Every component's Go name is now settled up front and referenced from there.
Five bugs, each of which produced a client that did not compile or lost
data the payload carried.
A required property referencing its own type generated `type Node struct
{ Child Node }`. Go allows a type to contain itself only through an
indirection, so such a field is now a pointer -- the tree, threaded
comment, and category-hierarchy shapes every spec of any size has. A
slice or map between the two ends already breaks the recursion and is
left alone.
A request body the spec declares without a schema generated a method
with an untyped parameter, which is a syntax error. It now takes the
bytes or text its media type implies, or any under JSON; a body with no
content at all is no body, not an empty one.
Discriminated unions stopped being comparable when the preserved payload
of an unrecognized variant was stored as json.RawMessage. A union is an
ordinary field of the structs that hold it, so that made every one of
those structs uncomparable too -- consumers could no longer compare them
or use them as map keys. The payload is kept as a string, which also
stops Raw() from handing out an alias of the union's own buffer.
Composed schemas dropped additionalProperties entirely: only plain
objects collected them. They are collected for allOf now, and the wire
names an embedded schema contributes are recognized as declared, so
inherited properties are not also re-collected and emitted twice.
A typed additionalProperties failed the whole decode when one undeclared
property did not match the declared value type, losing the response
along with it. Such a property is skipped instead, matching how an
unrecognized discriminator is already handled.
Composed schemas whose embedded schema also collects undeclared
properties keep the previous behavior, which is not yet correct: the
embedded marshalers are promoted onto the outer struct and swallow its
declared fields. Fixing that means replacing the shadow-type trick with
field-by-field marshaling; tracked separately.
…ncode The identifiers reserved against schema names covered only the ones the templates always declare. The templates also build names at render time -- <Op>Params for an operation's parameters and <ErrorType>Response for an error body -- which a static list cannot hold. Those are reserved as the spec is walked, before schema names are assigned, so a schema called ListUsersParams no longer redeclares the struct of that name. The ten Err* sentinels errors.go always declares were missing from the list outright. The test could not have caught either, because it iterated the list it was checking; it now parses the generated files and asserts every exported package-level name is reserved, which fails when a template gains a declaration nothing reserved. A form or multipart body whose schema is not an object generated a method taking that scalar, which every call then failed to encode, since the encoders build parts out of an object's properties. Such a body takes a map instead. A trace operation was silently skipped: the walk that is meant to be the single source of truth for a path item omitted it. A byte slice in a multipart body is now sent as the base64 text OpenAPI's format: byte calls for, rather than as a file part. Files arrive as FormFile, and the schemas a multipart body composes with allOf are marked alongside it, so a binary property inherited through composition is still generated as one. Breaking an alias cycle keeps the shape the alias carried: a self referencing array is `= []any` rather than `= any`.
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
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.
Closes #15, closes #16, closes #17, all filed by @antifuchs against the Mealie spec.
#15: an enum of comparison operators sanitized every value down to the same identifier, so the constants came out as
RelationalOperator2through7. Punctuation-only values are spelled out now, so"<>"becomesRelationalOperatorNotEqual.#16:
anyOf: [{type: string}, {type: "null"}]is just how 3.1 spells "nullable string", so it collapses to the variant's own type instead of*any. Same for unions whose members are all refinements of one type, which covered the rest of the fields in the reported struct.#17: multipart bodies were JSON-marshaled and sent as
application/json. The declared media type picks the encoding now, and binary properties in a multipart body become aFormFileyou can give a filename and content type. Servers that key onContent-Dispositionwon't treat a nameless part as an upload.Checked by generating the Mealie client (245 types, 247 operations) and running the three examples against a test server, not just compiling it.
Also in here
Verifying the above kept turning up adjacent bugs that made clients fail to compile or silently lose data. Most predate this branch:
type Node struct { Child Node }) and recursive aliases, neither of which Go allowsClient,APIError,ErrNotFound,ListUsersParams)allOfdroppingadditionalPropertiesentirely, and one off-type extra property failing a whole decode==for every struct holding oneTRACEoperations skipped outrightKnown gap
#50: when a schema's
allOfparent hasadditionalProperties, Go promotes the parent's marshalers onto the child and they swallow the child's declared fields. Fixing it means replacing the shadow-type trick with field-by-field marshaling, which is its own change.Checks
golangci-lint run ./...clean,go test -race -shuffle=on ./...green. Everytestdata/spec plus Mealie and nine edge-case specs regenerate, compile, and vet.