Fix hybrid configuration for basic authentication credentials #68
Conversation
Fixes #67 ## Problem When using hybrid configuration (binding from appsettings.json combined with programmatic overrides), basic authentication credentials set programmatically were not being applied to requests. Users expected to be able to override appsettings.json values with programmatic configuration, particularly for sensitive credentials like BasicAuthUsername and BasicAuthPassword. ## Root Cause The service registration API didn't properly support the .NET options pattern for applying configuration overrides. Options need to be configured using PostConfigure to ensure overrides run after all other configuration sources. ## Solution Simplified the API to use standard .NET options configuration patterns: 1. **Removed custom options helper** - Users now use standard `AddOptions<T>()` 2. **Added non-generic overloads** - `AddGotenbergSharpClient()` defaults to `GotenbergSharpClientOptions` for simpler API 3. **Use `GetRequiredService`** - Changed from nullable `GetService` to fail fast if options aren't registered 4. **Updated documentation** - All examples now show proper use of `PostConfigure` for overrides ## Changes - **TypedClientServiceCollectionExtensions.cs**: Simplified API, added non-generic overloads, updated XML docs - **BasicAuthTests.cs**: Updated tests to use standard options patterns, added hybrid config tests - **README.md**: Fixed all configuration examples to show correct patterns - **DIExample/Program.cs**: Updated to use simplified non-generic API ## Usage ```csharp // Hybrid configuration (appsettings + programmatic overrides) services.AddOptions<GotenbergSharpClientOptions>() .Bind(Configuration.GetSection("GotenbergSharpClient")) .PostConfigure(options => { // These run AFTER binding, so they override appsettings values options.BasicAuthUsername = Environment.GetEnvironmentVariable("GOTENBERG_USER"); options.BasicAuthPassword = Environment.GetEnvironmentVariable("GOTENBERG_PASS"); }); services.AddGotenbergSharpClient(); ``` ## Testing All 9 BasicAuthTests pass, including new tests for: - Hybrid configuration with PostConfigure overrides - Programmatic-only configuration - Incomplete auth validation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors client registration to use the Options pattern and DI-resolved options (IOptions) with new AddGotenbergSharpClient overloads (parameterless, generic, and configureClient variants). README examples and tests updated to show binding + PostConfigure hybrid configuration and to validate credential/override behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Startup as Startup
participant DI as DI Container
participant Options as IOptions<TOptions>
participant Builder as IHttpClientBuilder
participant Handler as HttpMessageHandler
participant Client as GotenbergSharpClient
Startup->>DI: AddOptions<TOptions>().Configure(config)
Note right of DI #DFF3E0: appsettings binding
Startup->>DI: AddOptions<TOptions>().PostConfigure(override)
Note right of DI #FFF4D6: programmatic overrides applied
Startup->>Builder: AddGotenbergSharpClient() / AddGotenbergSharpClient<TOptions>()
Builder->>DI: Resolve IOptions<TOptions>
DI-->>Builder: return combined options
Builder->>Handler: create/configure handler (BasicAuth, Timeout, Retry)
Handler->>Client: attach handler
Client-->>Startup: Typed client registered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
README.md (1)
133-145: Guard env-var overrides to avoid incomplete auth exceptionsIf either GOTENBERG_USER or GOTENBERG_PASS is missing, the runtime validation will throw InvalidOperationException. Suggest guarding to only set both when present.
- .PostConfigure(options => - { - // Override or add settings programmatically (runs after binding) - options.TimeOut = TimeSpan.FromMinutes(10); // Override timeout - options.BasicAuthUsername = Environment.GetEnvironmentVariable("GOTENBERG_USER"); - options.BasicAuthPassword = Environment.GetEnvironmentVariable("GOTENBERG_PASS"); - }); + .PostConfigure(options => + { + // Override or add settings programmatically (runs after binding) + options.TimeOut = TimeSpan.FromMinutes(10); + var user = Environment.GetEnvironmentVariable("GOTENBERG_USER"); + var pass = Environment.GetEnvironmentVariable("GOTENBERG_PASS"); + if (!string.IsNullOrWhiteSpace(user) && !string.IsNullOrWhiteSpace(pass)) + { + options.BasicAuthUsername = user; + options.BasicAuthPassword = pass; + } + });test/GotenbergSharpClient.Tests/BasicAuthTests.cs (1)
169-207: Gate integration test to avoid CI flakinessThis requires a running Gotenberg with basic auth. Consider marking with [Explicit], a category (e.g., [Category("Integration")]), or a conditional assume/ignore to skip when the service isn’t available.
src/Gotenberg.Sharp.Api.Client/Extensions/TypedClientServiceCollectionExtensions.cs (3)
93-110: Type reference may need namespace importThis file references GotenbergSharpClient (nameof and AddTypedClient) without a using for Gotenberg.Sharp.API.Client. If no global using exists, add:
+using Gotenberg.Sharp.API.Client;Alternatively, fully qualify the type in nameof and AddTypedClient.
150-156: Add null check for services for consistencyOther overloads guard services; mirror that here.
public static IHttpClientBuilder AddGotenbergSharpClient( - this IServiceCollection services, + this IServiceCollection services, Action<IServiceProvider, HttpClient> configureClient) { - return services.AddGotenbergSharpClient<GotenbergSharpClientOptions>(configureClient); + if (services == null) throw new ArgumentNullException(nameof(services)); + return services.AddGotenbergSharpClient<GotenbergSharpClientOptions>(configureClient); }
197-244: Consider moving auth validation to options validationCurrent validation occurs in the handler factory. You could also add options validation so failures surface earlier and consistently:
services.AddOptions<TOptions>() .Validate(o => string.IsNullOrWhiteSpace(o.BasicAuthUsername) == string.IsNullOrWhiteSpace(o.BasicAuthPassword), "BasicAuth configuration is incomplete. Both BasicAuthUsername and BasicAuthPassword must be set, or neither should be set.");Handler-level validation can remain as defense-in-depth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)src/Gotenberg.Sharp.Api.Client/Extensions/TypedClientServiceCollectionExtensions.cs(3 hunks)test/GotenbergSharpClient.Tests/BasicAuthTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/GotenbergSharpClient.Tests/BasicAuthTests.cs (1)
src/Gotenberg.Sharp.Api.Client/Domain/Settings/GotenbergSharpClientOptions.cs (1)
GotenbergSharpClientOptions(25-64)
src/Gotenberg.Sharp.Api.Client/Extensions/TypedClientServiceCollectionExtensions.cs (1)
src/Gotenberg.Sharp.Api.Client/Domain/Settings/GotenbergSharpClientOptions.cs (1)
GotenbergSharpClientOptions(25-64)
🪛 markdownlint-cli2 (0.18.1)
README.md
133-133: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (5)
README.md (1)
105-123: Programmatic options sample looks correctGood illustration of options pattern and RetryOptions usage. No issues.
test/GotenbergSharpClient.Tests/BasicAuthTests.cs (3)
131-167: Hybrid PostConfigure test is solidValidates override precedence correctly via IOptions. LGTM.
209-239: Good negative test for incomplete authAsserts the new validation path. Message match is specific; keep it synced with the thrown text.
Ensure the exception message remains unchanged if refactoring; otherwise loosen to Contains("BasicAuth configuration is incomplete").
241-269: Programmatic-only configuration test looks goodCovers non-hybrid path; assertions align with expected values.
src/Gotenberg.Sharp.Api.Client/Extensions/TypedClientServiceCollectionExtensions.cs (1)
56-66: Non-generic overload delegating to default options is cleanReads options from DI and fails fast if missing. LGTM.
README.md
Outdated
| ..... | ||
| services.AddOptions<GotenbergSharpClientOptions>() | ||
| .Bind(Configuration.GetSection(nameof(GotenbergSharpClient))); | ||
| .Bind(Configuration.GetSection(nameof(GotenbergSharpClient))) |
There was a problem hiding this comment.
Fix markdownlint MD010: replace hard tabs with spaces
Line 133 uses a hard tab; replace with spaces to satisfy markdownlint.
Based on static analysis hints
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
133-133: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🤖 Prompt for AI Agents
README.md around line 133: the line contains a hard tab character which violates
markdownlint MD010; replace the tab with spaces (preferably 2 or 4 spaces
consistent with the file) so the indentation uses spaces instead of tabs and
re-run linting to confirm the MD010 issue is resolved.
Fixes #67
Problem
When using hybrid configuration (binding from appsettings.json combined with programmatic overrides), basic authentication credentials set programmatically were not being applied to requests. Users expected to be able to override appsettings.json values with programmatic configuration, particularly for sensitive credentials like
BasicAuthUsernameandBasicAuthPassword.Root Cause
The service registration API didn't properly support the .NET options pattern for applying configuration overrides. Options need to be configured using
PostConfigureto ensure overrides run after all other configuration sources (like.Bind()).Solution
Simplified the API to use standard .NET options configuration patterns:
AddOptions<T>()AddGotenbergSharpClient()defaults toGotenbergSharpClientOptionsfor simpler APIGetRequiredService- Changed from nullableGetServiceto fail fast if options aren't registeredPostConfigurefor overridesChanges
Usage Example
Testing
Breaking Changes
None - this is a bug fix that makes the existing hybrid configuration pattern work correctly.
Checklist
Summary by CodeRabbit
Documentation
New Features
Tests