Validate OpenAPI spec before codegen#2435
Conversation
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge; the validator is additive and only rejects strings that would already produce uncompilable generated output. The change is a well-scoped validation gate that runs after the spec has been filtered and pruned, so it only rejects values that were already causing downstream compiler errors. The implementation correctly handles the subtle ref-sibling extension case, uses the same parse helpers the generator uses, and is backed by a fixture per error class. The only finding is a cosmetic inconsistency in iteration order (raw map range instead of SortedMapKeys in one walk method), which has no correctness impact. pkg/codegen/validate_spec.go — the walkPathItem non-deterministic iteration is the only item worth a second look.
|
| Filename | Overview |
|---|---|
| pkg/codegen/validate_spec.go | New validator that walks the entire spec and rejects values with embedded quotes, backticks, or control characters; well-structured with clear check helpers. One minor inconsistency: walkPathItem uses a raw map range over item.Operations() while every other collection in the validator is iterated via SortedMapKeys, making per-operation error order non-deterministic. |
| pkg/codegen/codegen.go | Adds an unconditional call to ValidateSpec after filtering/pruning — correct placement; the new error path is straightforward. |
| internal/test/spec_validation/spec_validation_test.go | Comprehensive test matrix covering all major injection vectors (paths, parameter names, property names, content types, enum values, discriminators, headers, extra-tags, x-go-name, x-go-type, ref-sibling x-go-type, and security scopes), plus a positive test for a legitimately complex spec. |
| internal/test/spec_validation/testdata/ref_sibling_x_go_type.yaml | Correctly exercises the ref-node extension validation path that was the subject of prior review discussion; the YAML is well-structured. |
| internal/test/spec_validation/testdata/valid.yaml | Covers the key positive case (content-type with quoted parameters and multi-line description); good for preventing false positives on standard specs. |
Reviews (2): Last reviewed commit: "Validate OpenAPI spec before codegen" | Re-trigger Greptile
| "path_newline.yaml": "path", | ||
| "parameter_name_quote.yaml": "parameter name", | ||
| "property_name_backtick.yaml": "property name", | ||
| "content_type_newline.yaml": "content type", | ||
| "enum_value_newline.yaml": "enum value", | ||
| "discriminator_quote.yaml": "discriminator", | ||
| "header_name_quote.yaml": "header", | ||
| "extra_tags_backtick.yaml": "x-oapi-codegen-extra-tags", | ||
| "x_go_name_invalid.yaml": "x-go-name", | ||
| "x_go_type_semicolon.yaml": "x-go-type", | ||
| } | ||
|
|
||
| for file, want := range cases { | ||
| t.Run(file, func(t *testing.T) { |
There was a problem hiding this comment.
x-go-type-name validation
ValidateSpec validates x-go-type-name via checkIdentifier in checkExtensions, but TestValidateSpecRejects has no entry for it. The test matrix covers x-go-name and x-go-type but leaves x-go-type-name (the extGoTypeName extension) untested. A test spec with an invalid x-go-type-name value (e.g. x-go-type-name: "9lives") should be added alongside the other x_go_* fixtures.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/test/spec_validation/spec_validation_test.go
Line: 22-35
Comment:
**No test case for `x-go-type-name` validation**
`ValidateSpec` validates `x-go-type-name` via `checkIdentifier` in `checkExtensions`, but `TestValidateSpecRejects` has no entry for it. The test matrix covers `x-go-name` and `x-go-type` but leaves `x-go-type-name` (the `extGoTypeName` extension) untested. A test spec with an invalid `x-go-type-name` value (e.g. `x-go-type-name: "9lives"`) should be added alongside the other `x_go_*` fixtures.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Add ValidateSpec in pkg/codegen, called from Generate after filtering and pruning. It walks the loaded document and rejects values that would produce invalid Go source: names, paths, media types, discriminator values, security scopes and struct-tag content may not contain quotes, backticks, or control characters; x-go-name/x-go-type-name must be valid Go identifiers; x-go-type must be a type expression. Values that legitimately contain quotes (media types, enum values) are checked only for control characters, since they are already emitted through strconv.Quote. The target of a $ref is not followed (it is validated at its own definition, and external documents are never copied into the output) but extensions on the ref node itself are checked, since an x-go-type sibling next to a $ref is honoured by code generation. Errors are friendly and name the offending field, so they are useful for ordinary malformed specs too. Adds internal/test/spec_validation with one spec per error type plus a legitimate spec that must pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
31f83d5 to
a4e5927
Compare
Add ValidateSpec in pkg/codegen, called from Generate after filtering and pruning. It walks the loaded document and rejects values that would produce invalid Go source: names, paths, media types, discriminator values and struct-tag content may not contain quotes, backticks, or control characters; x-go-name/x-go-type-name must be valid Go identifiers; x-go-type must be a type expression. Values that legitimately contain quotes (media types, enum values) are checked only for control characters, since they are already emitted through strconv.Quote.
Adds internal/test/spec_validation with one spec per error type plus a legitimate spec that must pass.