Skip to content

Conversation

@dani-garcia
Copy link
Member

🎟️ Tracking

📔 Objective

The server bindings template iterates over all auth methods of all the servers and generates code for each of them. This means that if the schema has two servers defined (like the US and EU servers) it will generate code like this:

        if let Some(ref token) = config.oauth_access_token {
            request = request.bearer_auth(token.to_owned());
        };
        // This one is just duplicate
        if let Some(ref token) = config.oauth_access_token {
            request = request.bearer_auth(token.to_owned());
        };

This won't break anything and is safe to do, but it's kind of wasteful. Instead, we can just use the first auth method only.

The change doesn't have any effect on the bindings for the moment as we're only ever defining one server and one auth method, but bitwarden/server#6653 will add the US and EU servers to the server API schema, which will result in the above shown duplication.

🚨 Breaking Changes

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@dani-garcia dani-garcia requested a review from a team as a code owner November 27, 2025 14:52
@claude
Copy link

claude bot commented Nov 27, 2025

Claude finished @dani-garcia's task —— View job


Review Complete

Summary

This PR modifies the OpenAPI mustache template to prevent duplicate auth method code generation when multiple servers are defined in the schema. The change uses {{#-first}}...{{/-first}} tags to ensure only the first auth method is generated, avoiding redundant code when schemas define multiple servers (e.g., US and EU).

Findings

Finding 1: Template change is correct and effective for the stated goal.

The addition of {{#-first}} and {{/-first}} wrapper tags around the auth methods section (lines 356 and 395 in support/openapi-template/reqwest-trait/api.mustache:356-395) successfully limits code generation to the first auth method, preventing duplicate bearer_auth calls.

Finding 2: 💭 Consider edge cases with multiple different auth methods.

The current implementation assumes all servers share the same auth method. If a future schema defines servers with different auth methods (e.g., Server 1 uses OAuth, Server 2 uses API key), this change would silently ignore all but the first. While this scenario seems unlikely given the PR description, it might be worth documenting this assumption.

Rationale and potential alternatives

The template iterates through {{#authMethods}}, and by wrapping the entire block with {{#-first}}{{/-first}}, only the first iteration executes. This is functionally equivalent to:

// Before: With US and EU servers both using oauth_access_token
if let Some(ref token) = config.oauth_access_token {
    request = request.bearer_auth(token.to_owned());
};
if let Some(ref token) = config.oauth_access_token {
    request = request.bearer_auth(token.to_owned());
};

// After: Only one auth block
if let Some(ref token) = config.oauth_access_token {
    request = request.bearer_auth(token.to_owned());
};

If schemas ever have different auth per server, you'd need to deduplicate by auth type rather than position, but that's significantly more complex and currently unnecessary.

Finding 3: 🎨 PR description could include example of generated code impact.

The PR description shows the before/after for generated Rust code, which is excellent. Consider also showing the template change itself to help reviewers understand the mustache syntax if they're unfamiliar with it.

Good Practices Observed

  • Clear PR objective explaining the problem and solution
  • Minimal, targeted change that doesn't affect functionality
  • No breaking changes to existing generated code (only removes redundant operations)
  • Aligns with CLAUDE.md guidance that auto-generated code changes are discouraged, but this modifies the template itself, which is appropriate

Verification

The change correctly:

  • Wraps all auth method implementations (supportTokenSource, isApiKey, isBasic, isOAuth) with first-iteration check
  • Preserves the {{#hasAuthMethods}} outer guard
  • Maintains all existing logic within the first iteration block
  • Will reduce code size when server#6653 adds US/EU server definitions

@github-actions
Copy link
Contributor

github-actions bot commented Nov 27, 2025

🔍 SDK Breaking Change Detection Results

SDK Version: ps/bindings-only-one-auth (e27e4b0)
Completed: 2025-11-27 15:05:11 UTC
Total Time: 218s

Client Status Details
typescript ✅ No breaking changes detected TypeScript compilation passed with new SDK version - View Details

Breaking change detection completed. View SDK workflow

@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Details6d1317b0-8956-4e14-abce-dea46181d113

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Nov 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.46%. Comparing base (4b0d128) to head (e27e4b0).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #589   +/-   ##
=======================================
  Coverage   79.46%   79.46%           
=======================================
  Files         302      302           
  Lines       32331    32331           
=======================================
  Hits        25691    25691           
  Misses       6640     6640           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dani-garcia dani-garcia merged commit 2cca3d4 into main Nov 27, 2025
58 checks passed
@dani-garcia dani-garcia deleted the ps/bindings-only-one-auth branch November 27, 2025 15:24
bw-ghapp bot pushed a commit to bitwarden/sdk-swift that referenced this pull request Nov 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants