feat(go): support server URL variables (region/edge routing)#16983
feat(go): support server URL variables (region/edge routing)#16983devin-ai-integration[bot] wants to merge 4 commits into
Conversation
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
Adds server URL variable routing to the Go v1 and v2 SDK generators. The implementation is well-structured and mirrors the other generators. A few issues: the v2 getBaseUrlTemplates has a non-exhaustive switch that can fall through without returning, and the interpolation only fires when a variable is non-empty which drops the default-substitution behavior when the user leaves a variable unset but the template still has a placeholder.
- 🟡 2 warning(s)
- 🔵 1 suggestion(s)
| switch (environments.type) { | ||
| case "singleBaseUrl": | ||
| for (const environment of environments.environments) { | ||
| if (environment.urlTemplate != null) { | ||
| return [{ fieldName: "", template: environment.urlTemplate }]; | ||
| } | ||
| } | ||
| return []; | ||
| case "multipleBaseUrls": { | ||
| const baseUrlNamesById = new Map<string, string>(); | ||
| for (const baseUrl of environments.baseUrls) { | ||
| baseUrlNamesById.set(baseUrl.id, this.context.caseConverter.pascalUnsafe(baseUrl.name)); | ||
| } | ||
| for (const environment of environments.environments) { | ||
| if (environment.urlTemplates != null && Object.keys(environment.urlTemplates).length > 0) { | ||
| const result: BaseUrlTemplate[] = []; | ||
| for (const baseUrlId of Object.keys(environment.urlTemplates).sort()) { | ||
| const template = environment.urlTemplates[baseUrlId]; | ||
| if (template == null) { | ||
| continue; | ||
| } | ||
| result.push({ | ||
| fieldName: baseUrlNamesById.get(baseUrlId) ?? baseUrlId, | ||
| template | ||
| }); | ||
| } | ||
| return result; | ||
| } | ||
| } | ||
| return []; | ||
| } | ||
| default: | ||
| assertNever(environments); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 warning
getBaseUrlTemplates returns BaseUrlTemplate[] but the default: assertNever(...) branch is unreachable at type level yet the function has no return after the switch on non-default paths that fall through. Specifically, both case arms return inside their loops but if no environment declares templates the singleBaseUrl/multipleBaseUrls arms do return [] — good. However TypeScript's control-flow may still complain because assertNever returns never but the compiler needs the switch to be exhaustive to infer the function always returns. This compiles only because assertNever is never. Confirm the tsc build is clean; if not, add an explicit return [] after the switch.
| writer.write("if "); | ||
| writer.write(options.map((option) => `options.${option.fieldName} != ""`).join(" || ")); |
There was a problem hiding this comment.
🟡 warning
The interpolation only runs when at least one variable option is non-empty (options.X != "" || ...). If the user sets none, the base URL keeps its original static default — but that default may itself still contain unresolved {id} placeholders in the template path (the static Environment/BaseURL default is presumably already resolved, so this is likely fine). Worth confirming the generated static default URL is fully resolved so a user who leaves everything unset doesn't get a broken URL. If the static default is already interpolated with variable defaults, ignore this.
| if _, ok := reservedRequestOptionNames[optionName]; ok { | ||
| optionName = "ServerURL" + variable.Name.PascalCase.UnsafeName | ||
| paramName = "serverURL" + variable.Name.PascalCase.UnsafeName |
There was a problem hiding this comment.
🔵 suggestion
The collision-prefixing derives paramName from PascalCase.UnsafeName (serverURL + Pascal), which produces e.g. serverURLEnvironment — fine here. But note the non-collision path uses CamelCase.SafeName while the collision path uses the raw Pascal name; if a variable name would need SafeName escaping (e.g. a Go keyword), the collision branch could emit an unsafe param identifier. Low risk given the ServerURL prefix, but consider "serverURL" + variable.Name.PascalCase.UnsafeName is guaranteed non-keyword — it is, so this is minor.
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const format = template.replace(/\{([^}]+)\}/g, (match, id: string) => { | ||
| const option = optionsByVariableId.get(id); | ||
| if (option == null) { | ||
| return match; | ||
| } | ||
| args.push(option.localName); | ||
| return "%s"; | ||
| }); |
There was a problem hiding this comment.
🟡 Literal percent signs in URL templates are misinterpreted during string formatting
URL template strings are used directly as Go format strings without escaping existing % characters (template.replace(...) at generators/go-v2/sdk/src/client/ClientGenerator.ts:464), so any literal % in a template (e.g. URL-encoded segments) would be misinterpreted by fmt.Sprintf at runtime, producing garbled URLs or a panic.
Impact: Generated SDK clients would crash or produce incorrect URLs when the API's server URL template contains percent-encoded characters.
Mechanism: unescaped % in fmt.Sprintf format string
In writeSprintfForTemplate (ClientGenerator.ts:454-478), the method builds a fmt.Sprintf call by replacing {id} placeholders with %s in the template string. However, it does not escape pre-existing % characters to %% before the replacement. For example, a template like https://api.example.com/path%20name/{region} would produce the format string "https://api.example.com/path%20name/%s", where %20 would be interpreted by fmt.Sprintf as a format verb (%2 followed by 0), causing incorrect output or a runtime error.
The fix is to escape % to %% before performing the {id} → %s replacement:
const escaped = template.replace(/%/g, "%%");
const format = escaped.replace(/\{([^}]+)\}/g, ...);| const format = template.replace(/\{([^}]+)\}/g, (match, id: string) => { | |
| const option = optionsByVariableId.get(id); | |
| if (option == null) { | |
| return match; | |
| } | |
| args.push(option.localName); | |
| return "%s"; | |
| }); | |
| const args: string[] = []; | |
| const escaped = template.replace(/%/g, "%%"); | |
| const format = escaped.replace(/\{([^}]+)\}/g, (match, id: string) => { | |
| const option = optionsByVariableId.get(id); | |
| if (option == null) { | |
| return match; | |
| } | |
| args.push(option.localName); | |
| return "%s"; | |
| }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — fixed. writeSprintfForTemplate now escapes literal % → %% before introducing the %s verbs, so percent-encoded segments in a template no longer break fmt.Sprintf. Seed output for the fixture is unchanged (it has no literal %).
| private collectServerVariables(): FernIr.ServerVariable[] { | ||
| const config = this.context.ir.environments; | ||
| if (config == null) { | ||
| return []; | ||
| } | ||
| const seen = new Set<string>(); | ||
| const result: FernIr.ServerVariable[] = []; | ||
| const add = (variables: FernIr.ServerVariable[]): void => { | ||
| for (const variable of variables) { | ||
| if (!seen.has(variable.id)) { | ||
| seen.add(variable.id); | ||
| result.push(variable); | ||
| } | ||
| } | ||
| }; | ||
| const environments = config.environments; | ||
| switch (environments.type) { | ||
| case "singleBaseUrl": | ||
| for (const environment of environments.environments) { | ||
| if (environment.urlVariables != null && environment.urlVariables.length > 0) { | ||
| add(environment.urlVariables); | ||
| break; | ||
| } | ||
| } | ||
| break; | ||
| case "multipleBaseUrls": | ||
| for (const environment of environments.environments) { | ||
| if (environment.urlVariables != null && Object.keys(environment.urlVariables).length > 0) { | ||
| for (const baseUrlId of Object.keys(environment.urlVariables).sort()) { | ||
| add(environment.urlVariables[baseUrlId] ?? []); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| break; | ||
| default: | ||
| assertNever(environments); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the URL templates (one per base URL for multipleBaseUrls, or a single | ||
| * entry for singleBaseUrl) that should be re-interpolated when a server URL | ||
| * variable is provided. For singleBaseUrl the fieldName is empty (the result is | ||
| * assigned to options.BaseURL rather than an Environment struct field). | ||
| */ | ||
| private getBaseUrlTemplates(): BaseUrlTemplate[] { | ||
| const config = this.context.ir.environments; | ||
| if (config == null) { | ||
| return []; | ||
| } | ||
| const environments = config.environments; | ||
| switch (environments.type) { | ||
| case "singleBaseUrl": | ||
| for (const environment of environments.environments) { | ||
| if (environment.urlTemplate != null) { | ||
| return [{ fieldName: "", template: environment.urlTemplate }]; | ||
| } | ||
| } | ||
| return []; | ||
| case "multipleBaseUrls": { | ||
| const baseUrlNamesById = new Map<string, string>(); | ||
| for (const baseUrl of environments.baseUrls) { | ||
| baseUrlNamesById.set(baseUrl.id, this.context.caseConverter.pascalUnsafe(baseUrl.name)); | ||
| } | ||
| for (const environment of environments.environments) { | ||
| if (environment.urlTemplates != null && Object.keys(environment.urlTemplates).length > 0) { | ||
| const result: BaseUrlTemplate[] = []; | ||
| for (const baseUrlId of Object.keys(environment.urlTemplates).sort()) { | ||
| const template = environment.urlTemplates[baseUrlId]; | ||
| if (template == null) { | ||
| continue; | ||
| } | ||
| result.push({ | ||
| fieldName: baseUrlNamesById.get(baseUrlId) ?? baseUrlId, | ||
| template | ||
| }); | ||
| } | ||
| return result; | ||
| } | ||
| } | ||
| return []; | ||
| } | ||
| default: | ||
| assertNever(environments); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Template and variable lookups iterate environments independently, risking a mismatch
In collectServerVariables (ClientGenerator.ts:310-316), the code picks variables from the first singleBaseUrl environment that has urlVariables. In getBaseUrlTemplates (ClientGenerator.ts:347-353), it picks the template from the first environment that has urlTemplate. If the IR has multiple environments where one has variables but no template (or vice versa), these two methods would select data from different environments. The template's {id} placeholders might not match the collected variable IDs, leaving unreplaced placeholders in the generated URL. The same pattern exists in the multipleBaseUrls branch. In practice, the IR should always pair variables with their template on the same environment, but this implicit coupling is worth noting.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged. This mirrors the existing Python/Java/TS implementations, which also read variables and templates without cross-checking they came from the same environment — the IR always pairs urlVariables with the urlTemplate(s) on the same environment (both are populated together from servers[].variables). Left as-is to stay consistent with the other generators and keep the change minimal.
| writer.write("if "); | ||
| writer.write(options.map((option) => `options.${option.fieldName} != ""`).join(" || ")); | ||
| writer.writeLine(" {"); | ||
| writer.indent(); | ||
|
|
||
| // Declare a local for each variable, defaulting to its IR default when unset. | ||
| for (const option of options) { | ||
| writer.writeLine(`${option.localName} := options.${option.fieldName}`); | ||
| if (option.variable.default != null) { | ||
| writer.writeLine(`if ${option.localName} == "" {`); | ||
| writer.indent(); | ||
| writer.writeLine(`${option.localName} = ${JSON.stringify(option.variable.default)}`); | ||
| writer.dedent(); | ||
| writer.writeLine("}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Variables without defaults produce empty string substitutions when only some variables are set
In writeServerVariableInterpolation (ClientGenerator.ts:403-447), the outer if guard fires when ANY variable option is non-empty. Inside, each variable's local is initialized from its option value, and only falls back to a default if one exists (ClientGenerator.ts:411-417). If a user sets variable A but not variable B, and B has no IR default, B's local will be "" and fmt.Sprintf will substitute an empty string into the URL (e.g., https://api..example.com). This is arguably correct behavior (the user should set all variables or rely on defaults), but it could produce surprising URLs. Consider whether the generated code should validate that all variables are non-empty before interpolation.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is intentional and matches the other generators (Python/Java/TS): a variable is only substituted with its IR default when unset, and if it has no default it interpolates as empty. Per the spec, variables are expected to either declare a default or be set by the user. Adding a "require all variables" validation would diverge from the cross-language UX, so I've kept the fallback-to-default behavior. In this fixture both variables have defaults (us-east-1 / prod), so no empty substitution occurs.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Docs Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on
Docs generation runs |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Description
Adds server URL variable (region/edge) routing support to the Go SDK generator (v1 + v2 tandem), matching the TypeScript (PR #16977), Python, and Java implementations.
OpenAPI
servers[].variablesare modeled in the Fern IR as environments carryingurlTemplate/urlTemplates(URL strings with{id}placeholders) plusurlVariables(ServerVariable { id, name, default, values? }). Previously the Go generator ignoredurlVariablesand only emitted the static default URL(s). Now each variable is surfaced as an optional functional client option and interpolated into the base URL(s) at construction time.Changes Made
Go v1 (
generators/go/internal/generator/sdk.go) — extract server URL variables fromEnvironmentsConfig(bothsingleBaseUrlandmultipleBaseUrls, taking the first environment that declares them, de-duped byid). Emit:core.RequestOptionsstruct (core/request_option.go),*Optionstruct implementingapplyRequestOptionsper variable,With<Name>functional option (option/request_option.go).Name de-collision mirrors the other generators: a variable's
namemaps to a PascalCase option name, and if it collides with a reserved option (e.g.Environment) it is prefixed withServerURL→WithServerURLEnvironment.Go v1 IR structs (
generators/go/internal/fern/ir/common/common.go) — added the hand-maintainedServerVariablestruct and theurlTemplate/urlTemplates/urlVariablesfields (+ getters) toSingleBaseUrlEnvironment/MultipleBaseUrlsEnvironmentso the native v1 side can read them from IR v66. No IR schema /versions.ymlchanges.Go v2 (
generators/go-v2/sdk/src/client/ClientGenerator.ts) — interpolate at construction time. When any server-variable option is set, rebuild the base URL(s) from the environment's template(s), substituting each{id}(falling back to the variable's IRdefault). FormultipleBaseUrlseach named base URL is rebuilt from its own template and assigned tooptions.Environment; forsingleBaseUrlthe result is assigned tooptions.BaseURL.Generated constructor for the fixture:
Seed fixture — added
server-url-templating(outputFolder: no-custom-config,customConfig: null) toseed/go-sdk/seed.ymland committed the regenerated output underseed/go-sdk/server-url-templating/no-custom-config/(the stale root-level output was removed so the layout matches other fixtures).Changelog —
generators/go/sdk/changes/unreleased/add-server-url-variable-support.yml(type: feat).Testing
seed test --generator go-sdk --fixture server-url-templating --local --skip-scripts→success(1/1 passed); generated SDK builds (go build ./...).WithRegionandWithServerURLEnvironment(de-collided) and interpolates both the base and auth URLs, with defaultsus-east-1/prod.go test ./internal/generator/...); v2 compiles;biome checkclean.Server URL variable validation
Server URL variable validation: Region/environment values are interpolated into the URL template verbatim. Values are NOT validated against the declared allowed values (
enum) — passing an unknown value (e.g.region="mars") simply produceshttps://api.mars.<...>rather than raising. This matches the existing Python and Java generator behavior for server URL variables.Link to Devin session: https://app.devin.ai/sessions/21f08d51342e45c4b9cf38635a23595f
Requested by: @cadesark