Skip to content

feat(csharp-sdk): remove omitted basic auth fields from SDK API, use empty string internally#14409

Open
Swimburger wants to merge 10 commits intomainfrom
devin/1774997734-basic-auth-optional-csharp-sdk
Open

feat(csharp-sdk): remove omitted basic auth fields from SDK API, use empty string internally#14409
Swimburger wants to merge 10 commits intomainfrom
devin/1774997734-basic-auth-optional-csharp-sdk

Conversation

@Swimburger
Copy link
Copy Markdown
Member

@Swimburger Swimburger commented Mar 31, 2026

Description

Refs #14378

When usernameOmit or passwordOmit flags are set in the IR's BasicAuthScheme, the generated C# SDK now completely removes the flagged field from the constructor and client options — it does not appear in the end-user API at all. Internally, the omitted field is treated as an empty string when encoding the Authorization: Basic header (e.g. password: omit: true → header encodes username:). Default behavior (both required) is preserved when no omit flags are set.

Split from #14378 (one PR per generator).

Changes Made

  • RootClientGenerator.ts:
    • Reads basicScheme.usernameOmit / basicScheme.passwordOmit as independent booleans
    • Constructor parameter removal: omitted fields are excluded entirely from the params array (not just made optional). Only non-omitted fields appear in the generated constructor.
    • Auth header encoding: omitted fields use "" directly in the Base64 interpolation (e.g. $"{""}:{password}" when username is omitted), instead of a null-coalescing fallback on a parameter that no longer exists.
    • Per-field condition logic: only non-omitted fields contribute to the null-check guard (e.g. when only passwordOmit is set, condition is username != null)
  • versions.yml: new 2.55.4 entry
  • New basic-auth-optional test fixture with password: omit: true, plus seed output at seed/csharp-sdk/basic-auth-optional/
  • IR-to-JSON-schema snapshot for the new fixture

Updates since last revision

  • Breaking semantic change: omitted fields are now completely removed from the end-user SDK API (constructor, client options) instead of being made nullable/optional. This matches the IR docs: "the field will be omitted from the SDK."
  • Internally, omitted fields are hard-coded as "" in the auth header encoding
  • Multiple CI retriggers needed due to flaky seed-test-results (csharp-sdk) cancellations (infrastructure issue, not code-related)

Testing

  • Seed snapshot generated for basic-auth-optional fixture
  • Existing seed fixtures unchanged (no regressions)
  • Lint passes (biome check)

⚠️ Human Review Checklist

  1. Stale seed output: The committed SeedBasicAuthOptionalClient.cs still shows password as an optional constructor parameter with password ?? "" null-coalescing. This appears to have been generated before the field-removal changes were applied to the generator. CI should regenerate this — verify after CI passes that the generated constructor no longer includes the omitted password field and uses a bare "" instead.
  2. Verify field removal in constructor generation (lines ~814–850 of RootClientGenerator.ts): Confirm that if (!usernameOmitted) / if (!passwordOmitted) correctly excludes the field from params entirely, and that the returned array only contains non-omitted fields.
  3. Verify auth header encoding (lines ~466–490): When a field is omitted, the expression should be "" (literal empty string), not a reference to a now-nonexistent parameter. Check that usernameExpr / passwordExpr are correct.
  4. Condition when both fields omitted: The condition becomes "true" (always set the header with "":"" encoding Og==). Confirm this is the desired behavior.

Link to Devin session: https://app.devin.ai/sessions/0786b963284f4799acb409d5373cde0a
Requested by: @Swimburger


Open with Devin

…en configured in IR

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

Open in Devin Review

…y instead of coarse eitherOmitted flag

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Comment on lines +469 to +481
const usernameOmitted = basicScheme.usernameOmit === true;
const passwordOmitted = basicScheme.passwordOmit === true;
// Per-field condition: required fields must be present, omittable fields are always satisfied
let condition: string;
if (!usernameOmitted && !passwordOmitted) {
condition = `${usernameAccess} != null && ${passwordAccess} != null`;
} else if (usernameOmitted && passwordOmitted) {
condition = `${usernameAccess} != null || ${passwordAccess} != null`;
} else if (usernameOmitted) {
condition = `${passwordAccess} != null`;
} else {
condition = `${usernameAccess} != null`;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic bug: When both username and password are optional (usernameOmit and passwordOmit are both true), but auth itself is required (isAuthOptional = false) and there's only one basic scheme, no condition check is generated. This causes the Authorization header to always be set with "" (empty credentials) even when both username and password are null, contradicting the changelog which states the header should be omitted when neither credential is provided.

Fix: The condition logic needs to account for the case where fields are optional even when isAuthOptional is false. When either field is omitted, a condition check should still be generated:

const eitherOmitted = usernameOmitted || passwordOmitted;
if (isAuthOptional || basicSchemes.length > 1 || eitherOmitted) {
    const controlFlowKeyword = i === 0 ? "if" : "else if";
    innerWriter.controlFlow(controlFlowKeyword, this.csharp.codeblock(condition));
}

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

devin-ai-integration[bot]

This comment was marked as resolved.

Swimburger and others added 2 commits April 1, 2026 16:15
… per-field omit fix

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>
devin-ai-integration[bot]

This comment was marked as resolved.

Swimburger and others added 2 commits April 1, 2026 18:36
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ms, use empty string internally

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration devin-ai-integration bot changed the title feat(csharp-sdk): support optional username/password in basic auth when configured in IR feat(csharp-sdk): remove omitted basic auth fields from SDK API, use empty string internally Apr 2, 2026
Swimburger and others added 4 commits April 2, 2026 01:16
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>
…(attempt 3)

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…(attempt 4)

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant