fix(cosmos) guard CosmosClientOptions.ApplicationName against empty/null#59
Merged
Merged
Conversation
Real bug, caught on the first deployed-Cosmos invocation: the Cosmos SDK rejects empty/null User-Agent additions with System.ArgumentException: Application name '' is invalid. System.FormatException: The format of value '<null>' is invalid. Root cause: AddCosmosPersistence set clientOptions.ApplicationName = options.ApplicationName unconditionally. CosmosOptions.ApplicationName is nullable by design (its docstring explicitly notes 'Optional override... helpful in Cosmos diagnostics for distinguishing the scraper job from the API'), so the first real config that didn't set it crashed at DI resolution. The existing tests (CosmosRepositoryTests, MachineRepositoryTests, etc.) mocked at the Container level rather than constructing a real CosmosClient, so the bug never surfaced under test. Two new tests in AddCosmosPersistenceTests resolve the CosmosClient through the DI container — the WithoutApplicationName test pins the regression (pre-fix throws), the WithApplicationName test pins the happy path. Fix: only assign clientOptions.ApplicationName when the option is populated (string.IsNullOrWhiteSpace gate). Load-bearing comment on the gate documents the SDK behavior so the next reader doesn't try to 'simplify' it back to the unconditional form. Tests: 501 -> 503. Build clean. Pre-push self-audit: 7-item mechanical (all pass). /local-review skipped — single-line guard fix with regression test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Real bug, caught the first time the CLI tried to talk to deployed Cosmos:
The Cosmos SDK appends
ApplicationNameto its User-Agent header. The HTTP-headers parser rejects empty/null values.AddCosmosPersistencewas settingclientOptions.ApplicationName = options.ApplicationNameunconditionally — butCosmosOptions.ApplicationNameis nullable by design (its docstring: "Optional override... helpful in Cosmos diagnostics for distinguishing the scraper job from the API").The existing tests (
CosmosRepositoryTests,MachineRepositoryTests, etc.) all mocked at theContainerlevel rather than constructing a realCosmosClient, so the crash never surfaced under test. The first real-config DI resolution after PR #57's deploy-validation flag was wired tripped it.Fix: guard with
string.IsNullOrWhiteSpace. Load-bearing comment on the gate documents the SDK behavior so a future reader doesn't try to "simplify" it back to the unconditional form.Test Plan
dotnet test PinballWizard.slnx-> 503 / 503 passing (was 501 — +2 new tests inAddCosmosPersistenceTests)CosmosClientthrough the actual DI container:AddCosmosPersistence_WithoutApplicationName_ResolvesCosmosClientWithoutThrowing— pins the regression. Pre-fix, this throwsArgumentException 'Application name "" is invalid'.AddCosmosPersistence_WithApplicationName_AppliesItToClient— pins the happy path. The CosmosClient setter runs without throwing for any non-empty value.dotnet run --project src/PinballWizard.Cli -- --ensure-cosmos-containersagainst deployed Cosmos with the existingCosmos__AccountEndpointenv var — expected output:Out of Scope
ApplicationNameto"PinballWizard"inCosmosOptions. Considered; rejected. The option is genuinely optional (per docstring), and a non-null default would force every future consumer (Phase 2 services) to override or accept the same name. The conditional-assign at the consumer is more flexible.Checklist
TODO/FIXME/ commented-out code committedPre-push self-audit
Step 0 —
/local-review(qualitative)Step 1 — Mechanical checklist
*Optionsproperty has at least one real getter call insrc/— N/A (no new options; existingApplicationNamegetter still in use)catch { }— N/A (no catch added)ISourceScraper? — N/ACosmosClientthrough DI; theWithoutApplicationNametest would throw pre-fixgit log -1 --format='%an <%ae>'shows personal noreply, not work email