fix(cli): preserve explicit null examples so Go wire tests match generated models - #17427
fix(cli): preserve explicit null examples so Go wire tests match generated models#17427devin-ai-integration[bot] wants to merge 2 commits into
Conversation
A null example routed through an unknown schema (e.g. a declared property on an object with additionalProperties: true) was replaced with the {"key": "value"} placeholder, so generated Go wire tests mocked an object where the model expects a nullable scalar. Adds a go-sdk seed fixture covering nullable scalars with wire tests enabled.
Co-Authored-By: bot_apk <apk@cognition.ai>
A configured importPath or module.path that already ends in /vN is left alone, so releasing v34.0.0 with importPath github.com/acme/acme-go/v46 no longer produces github.com/acme/acme-go/v46/v34. Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 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
Two targeted fixes: preserve explicit null for unknown schemas in the OpenAPI example factory, and stop doubling /vN module suffixes in both Go generators. The logic changes look correct; my concerns are the newly-permissive suffix regex (matches v0/v01, and silently discards a genuine major-version bump) and a weak test assertion that would pass even if the property were dropped entirely.
- 🟡 2 warning(s)
- 🔵 1 suggestion(s)
| // majorVersionSuffixPattern matches a Go major version suffix, e.g. "v2". | ||
| var majorVersionSuffixPattern = regexp.MustCompile(`^v[0-9]+$`) | ||
|
|
||
| // maybeAppendVersionSuffix appends the given version suffix to the importPath, | ||
| // unless the importPath already ends in a major version suffix. The configured | ||
| // suffix wins, even if it doesn't match the version being released. | ||
| func maybeAppendVersionSuffix(importPath string, version string) string { | ||
| if path.Base(importPath) == version { | ||
| if majorVersionSuffixPattern.MatchString(path.Base(importPath)) { |
There was a problem hiding this comment.
🟡 warning
Two nits on the widened match:
^v[0-9]+$also matchesv0,v01, etc., which are not valid Go major-version suffixes. If someone configuresgithub.com/acme/acme-go/v0and releasesv2.0.0, we now silently emit an unbuildable module path instead of appending/v2. Tighten to^v[1-9][0-9]*$.- When the existing suffix disagrees with the release major (the
/v46+v34case), we silently prefer the configured value. That's the right default, but a warn-level log would save the next person a debugging session.
| // majorVersionSuffixPattern matches a Go major version suffix, e.g. "v2". | |
| var majorVersionSuffixPattern = regexp.MustCompile(`^v[0-9]+$`) | |
| // maybeAppendVersionSuffix appends the given version suffix to the importPath, | |
| // unless the importPath already ends in a major version suffix. The configured | |
| // suffix wins, even if it doesn't match the version being released. | |
| func maybeAppendVersionSuffix(importPath string, version string) string { | |
| if path.Base(importPath) == version { | |
| if majorVersionSuffixPattern.MatchString(path.Base(importPath)) { | |
| // majorVersionSuffixPattern matches a Go major version suffix, e.g. "v2". | |
| var majorVersionSuffixPattern = regexp.MustCompile(`^v[1-9][0-9]*$`) | |
| // maybeAppendVersionSuffix appends the given version suffix to the importPath, | |
| // unless the importPath already ends in a major version suffix. The configured | |
| // suffix wins, even if it doesn't match the version being released. | |
| func maybeAppendVersionSuffix(importPath string, version string) string { | |
| if majorVersionSuffixPattern.MatchString(path.Base(importPath)) { |
| const DEFAULT_MODULE_PATH = "sdk"; | ||
|
|
||
| // Matches a Go major version suffix, e.g. "v2". | ||
| const MAJOR_VERSION_SUFFIX_PATTERN = /^v\d+$/; |
There was a problem hiding this comment.
🟡 warning
Same as the Go side: ^v\d+$ matches v0 and v01, neither of which is a legal Go major-version suffix. A configured .../v0 would now suppress a legitimate /v2 append. Suggest ^v[1-9]\d*$ to keep both implementations aligned.
| const MAJOR_VERSION_SUFFIX_PATTERN = /^v\d+$/; | |
| const MAJOR_VERSION_SUFFIX_PATTERN = /^v[1-9]\d*$/; |
| expect(result?.type).toBe("object"); | ||
| if (result?.type === "object") { | ||
| expect(result.properties.logo?.type).not.toBe("map"); | ||
| expect(result.properties.logo?.type).not.toBe("object"); |
There was a problem hiding this comment.
🔵 suggestion
These assertions pass vacuously if logo is dropped from the example entirely (undefined?.type is undefined, which is neither "map" nor "object"). Assert the positive case so the test actually guards the fix.
| expect(result?.type).toBe("object"); | |
| if (result?.type === "object") { | |
| expect(result.properties.logo?.type).not.toBe("map"); | |
| expect(result.properties.logo?.type).not.toBe("object"); | |
| expect(result?.type).toBe("object"); | |
| if (result?.type === "object") { | |
| expect(result.properties.logo?.type).toBe("null"); | |
| } |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Docs Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on
Docs generation runs |
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
Description
Two independent fixes, one per commit.
1.
fix(cli): explicitnullexamples were replaced with a placeholder object.The reporter's framing (nullable scalars break Go wire tests) was close but not the mechanism — nullability alone is not the trigger, and WireMock is not inventing anything.
WireMock.convertToWireMockcopies the IR example'sjsonExampleverbatim, so the bad body already exists in the IR. It comes fromExampleTypeFactory: for an object withadditionalProperties: true, properties not produced by the declared-property pass are re-built throughSchemaWithExample.unknown, and theunknownbranch ignored an explicitnulland fell through to the placeholder:So
logo: nullin the spec example became"logo": {"key": "value"}inwiremock-mappings.json, while the generated model stayed correctly*string— hencejson: cannot unmarshal object into Go struct field ... of type string. The model side was right; the example side was wrong, so only the example side changed.This is not a Go-specific defect:
go-v2,python-v2,ruby-v2,php, andrustall generate wire mocks from the same@fern-api/mock-utils+ IR examples, so all of them would mock the same object. Go is just the first generator with wire tests on by default over a spec that hits theadditionalPropertiespath. The fix is in the shared OpenAPI importer, so all of them get it.2.
fix(go): doubled/vNmodule suffix. Confirmed genuine. Both the native generator (maybeAppendVersionSuffix) and go-v2 (maybeAppendMajorVersionSuffix) only skipped appending when the existing suffix equalled the release major, soimportPath: github.com/plaid/plaid-go/v46released asv34.0.0producedmodule github.com/plaid/plaid-go/v46/v34. Now any existing/vNsuffix wins.Changes Made
ExampleTypeFactory: preserve an explicitnullforunknownschemas instead of emitting{"key": "value"}.go-nullable-wire-tests(nullable string / nullable int / nullable nested object, explicitnullresponse example,additionalProperties: true) — the only go-sdk fixture withenableWireTests: truebesides the existing ones; the global default is untouched./vN.packages/cli/cli/changes/unreleased/andgenerators/go/sdk/changes/unreleased/.Testing
ExampleTypeFactory.test.ts(explicit-null cases),resolveRootImportPath.test.ts(go-v2 suffix),cmd_test.go(go v1 suffix, incl./v46+ releasev34→/v46).wiremock/docker-compose.test.yml:json: cannot unmarshal object into Go struct field ... of type string"logo": null, "rank": nullandgo test ./...passes.pnpm seed test --generator go-sdk --fixture go-nullable-wire-tests --skip-scripts --local→ 1/1 passed.pnpm testgreen, and no committed IR/JSON-schema snapshot changed across the 58 OpenAPI-based test definitions — i.e. the example fix is inert for every existing fixture.pnpm check(biome),gofmt,go build ./...,go vet.Link to Devin session: https://app.devin.ai/sessions/ee05b7f0ee07445c97a64a00db1d4d5a