feat(csharp): support server URL variables (region/edge routing)#16981
feat(csharp): support server URL variables (region/edge routing)#16981devin-ai-integration[bot] wants to merge 3 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 C# SDK support for server URL variables. The generator logic is reasonable, but there's a correctness bug: the ClientOptions properties are generated as init while the constructor mutates them via set, contradicting the PR's own stated intent to make them settable.
- 🟡 2 warning(s)
- 🔵 2 suggestion(s)
| get: true, | ||
| init: true, |
There was a problem hiding this comment.
🟡 warning
The server variable fields are emitted with init: true, but the PR description's excerpt shows them as init while the root client mutates clientOptions.Environment/BaseUrl. That part is fine (Environment/BaseUrl get set when variables exist). However, note the doc comment in the PR shows public string? Region { get; init; } — that's consistent here. The real thing to double check: Region/ServerUrlEnvironment being init-only is correct since the constructor only reads them. No change needed, but verify the summary doc comment renders (currently passed as summary, not a doc-tagged field).
| return collectServerVariables(environmentsConfig).map((variable) => { | ||
| const pascal = caseConverter.pascalSafe(variable.name); | ||
| const optionName = RESERVED_OPTION_NAMES.has(pascal) | ||
| ? caseConverter.pascalSafe(`server url ${getOriginalName(variable.name)}`) |
There was a problem hiding this comment.
🟡 warning
De-collision uses getOriginalName(variable.name) for the reserved-name branch but variable.name for the normal branch. If variable.name is a NameAndWireValue/object, pascalSafe(variable.name) may not behave as expected. Confirm both branches feed pascalSafe the same shape of input — inconsistent handling could produce mismatched option names vs. what tests assume.
| export function urlTemplateToInterpolatedString(template: string, options: ServerVariableOption[]): string { | ||
| let result = escapeForCSharpString(template); | ||
| for (const { variable, localName } of options) { | ||
| result = result.split(`{${variable.id}}`).join(`{${localName}}`); |
There was a problem hiding this comment.
🔵 suggestion
result.split(\{${variable.id}}`).join(...)runsescapeForCSharpString` first, then substitutes. If a template placeholder id ever contains regex/brace-special chars it's fine (string split, not regex), but be aware braces in the literal URL that aren't placeholders would be left as-is and become invalid C# interpolation syntax. Low risk, worth a comment or a guard if arbitrary URLs are possible.
| private writeServerVariableLocals(writer: Writer, options: ServerVariableOption[]): void { | ||
| for (const { variable, optionName, localName } of options) { | ||
| writer.writeTextStatement( | ||
| `var ${localName} = clientOptions.${optionName} ?? "${escapeForCSharpString(variable.default ?? "")}"` |
There was a problem hiding this comment.
🔵 suggestion
variable.default ?? "" silently substitutes an empty string when no default exists. If a variable has no default and the user omits it, this produces a URL with an empty segment (e.g. https://api..example.com). Consider whether a missing-default-with-no-value case should instead be an error or fall back to the static URL.
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 |
![hex-security-app[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| * the result wrapped as a C# interpolated string (e.g. `$"https://api.{_region}.example.com"`). | ||
| */ | ||
| export function urlTemplateToInterpolatedString(template: string, options: ServerVariableOption[]): string { | ||
| let result = escapeForCSharpString(template); |
There was a problem hiding this comment.
In urlTemplateToInterpolatedString, the raw urlTemplate value (e.g. servers[].url from an OpenAPI spec) is passed through escapeForCSharpString then embedded verbatim inside a C# interpolated string ($"..."). escapeForCSharpString escapes \ and " but not { or }. Any {expr} placeholder in the template that does not match a declared variable.id survives unchanged and becomes a live C# interpolation expression compiled into the generated SDK.
An attacker-controlled OpenAPI spec with a server URL such as https://api.{region}.example.com{System.Environment.Exit(0)} produces:
clientOptions.BaseUrl = $"https://api.{_region}.example.com{System.Environment.Exit(0)}";This compiles without error and terminates the host process on every SDK client instantiation (DoS). Expressions that don't require string literals — System.Environment.MachineName, System.Environment.ProcessId, System.GC.Collect(), System.Diagnostics.Process.GetCurrentProcess() etc. — are all valid. On C# 11 targets (net8.0 / net9.0, both in the fixture's CI matrix) the language-level restriction on " inside interpolation holes is relaxed, enabling full string-argument method calls.
Prompt To Fix With AI
In `urlTemplateToInterpolatedString`, after calling `escapeForCSharpString(template)`, escape ALL curly braces in the result to their doubled C# form (`{` → `{{`, `}` → `}}`), then re-introduce only the known variable placeholders as real interpolation expressions. Example fix:
```typescript
export function urlTemplateToInterpolatedString(template: string, options: ServerVariableOption[]): string {
// 1. Escape string-level specials (backslash, double-quote, control chars)
let result = escapeForCSharpString(template);
// 2. Neutralise ALL braces so no arbitrary C# expression survives in the output
result = result.replace(/\{/g, "{{").replace(/\}/g, "}}");
// 3. Re-open only the declared variable placeholders as real interpolation holes
for (const { variable, localName } of options) {
result = result.split(`{{${variable.id}}}`).join(`{${localName}}`);
}
return `$"${result}"`;
}
```
This ensures any `{expr}` in the source template that does not correspond to a declared server variable becomes the literal text `{expr}` rather than an executed C# expression. Update the `serverVariables.test.ts` fixture expectations accordingly (the only observable change is that un-matched braces are now doubled).Severity: medium | Confidence: 75% | React with 👍 if useful or 👎 if not
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Clone method does not copy server variable fields
The Clone() method (both the object-initializer path at generators/csharp/sdk/src/options/ClientOptionsGenerator.ts:430-449 and the copy constructor at lines 470-511) does not include the new server variable fields (e.g., Region). This is functionally correct in the current flow because the interpolation writes to BaseUrl/Environment BEFORE the clone happens, and those fields ARE copied. However, if Clone() is ever used in a context where the server variable fields need to be preserved (e.g., re-constructing a client from cloned options), they would be lost. The existing TODO at line 411 acknowledges this pattern needs improvement.
(Refers to lines 430-449)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional. The server variables are construction-time-only inputs: they're consumed to rebuild BaseUrl/Environment before any Clone() happens, and those resolved URLs are copied. This mirrors the TS (#16977), Python, and Java implementations, where the variables are likewise applied during client construction and not preserved as separate fields. Adding them to Clone() would have no observable effect in the current flow, so I've left them out to keep parity with the other generators.
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 C# SDK generator, matching the behavior shipped for TypeScript in #16977 (and the existing Python/Java implementations).
When an API's OpenAPI
servers[].variablesare modeled in the IR asEnvironmentsConfigwithurlTemplate(s)+urlVariables, the generator now exposes each variable as an optional client-construction option and interpolates the provided values into the environment base URL(s) at runtime. When a variable is omitted, its IRdefaultis used, so behavior is unchanged for existing SDKs / APIs that declare no server variables.Changes Made
generators/csharp/sdk/src/root-client/serverVariables.ts:getServerVariableOptions(environmentsConfig, caseConverter)extracts the variables from the first environment that declares them (handling bothsingleBaseUrlandmultipleBaseUrls), de-dups byid, maps eachnameto an idiomatic PascalCase option, and de-collides reservedClientOptionsnames by prefixingServerUrl(so a variable namedenvironmentis surfaced asServerUrlEnvironment).urlTemplateToInterpolatedString(template, options)turns a{id}URL template into a C# interpolated string ($"https://api.{_region}.example.com").ClientOptionsGenerator: emits an optionalstring?property per server variable (e.g.Region,ServerUrlEnvironment) with doc comments listing allowed values + default, and makes theBaseUrl/Environmentproperty settable (rather thaninit-only) when server variables exist so the root client can rebuild it.RootClientGenerator: in the client constructor, when any server variable is set, rebuilds the environment URL(s) from the template(s) — formultipleBaseUrlseach named base URL is rebuilt from its own template (a template referencing only a subset of variables is fine).server-url-templatingfixture toseed/csharp-sdk/seed.ymland committed the regenerated output underseed/csharp-sdk/server-url-templating/.type: feat) undergenerators/csharp/sdk/changes/unreleased/.Generated
ClientOptions(excerpt):Generated root client constructor (excerpt):
Testing
serverVariables.test.tscovers extraction, dedup, reserved-name de-collision, and template interpolation (54/54 generator tests pass).seed test --generator csharp-sdk --fixture server-url-templating --localreturnssuccess; the generated SDK builds with0 Error(s)across net462/netstandard2.0/net8.0/net9.0.versions.yml, other generators, or unrelated fixtures.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/db6628b6fc0f4fa7bfeb1d8911f7c69f
Requested by: @cadesark