chore: add updateConfig method into config service #289
Conversation
WalkthroughAdded a private helper to centralize Zod validation error handling and a new public method Changes
Sequence DiagramsequenceDiagram
participant Client
participant ConfigService
participant Schema as AppConfigSchema
participant Logger
Client->>ConfigService: updateConfig(values)
ConfigService->>Schema: parse/validate(values)
alt Validation Success
Schema-->>ConfigService: validated config
ConfigService->>ConfigService: this.config = validated
ConfigService->>ConfigService: this.serverKeys = {}
ConfigService->>Logger: debug("updated serverName ...")
ConfigService-->>Client: return / resolve
else Zod Validation Failure
Schema-->>ConfigService: ZodError
ConfigService->>ConfigService: handleValidationError(err, "Configuration validation failed - check your federation settings")
ConfigService->>Logger: error(formatted details)
ConfigService-->>Client: throw Invalid configuration error
else Other Exception
Schema-->>ConfigService: exception
ConfigService-->>Client: rethrow
end
note over ConfigService: Constructor validation now also calls handleValidationError on ZodError
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
==========================================
- Coverage 60.49% 60.37% -0.12%
==========================================
Files 67 67
Lines 6675 6691 +16
==========================================
+ Hits 4038 4040 +2
- Misses 2637 2651 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/federation-sdk/src/services/config.service.ts (1)
19-19: Critical: Type mismatch between interface and schema forsigningKeyPath.The
AppConfiginterface declaressigningKeyPathas optional (line 19), butAppConfigSchemarequires it (line 52 has no.optional()). This inconsistency will cause validation failures whensigningKeyPathis undefined, breaking both the constructor and the newupdateConfigmethod.Apply this diff to align the schema with the interface:
- signingKeyPath: z.string(), + signingKeyPath: z.string().optional(),Also applies to: 52-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/federation-sdk/src/services/config.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/config.service.ts (1)
packages/federation-sdk/src/index.ts (1)
AppConfig(58-58)
🔇 Additional comments (3)
packages/federation-sdk/src/services/config.service.ts (3)
160-168: Verify test coverage for the new public method.The Codecov report indicates only 4.76% patch coverage for this PR, with 20 lines missing coverage. The new
updateConfigmethod is a public API that handles critical validation and state updates, and should have comprehensive test coverage including success cases, validation failures, and edge cases.Please ensure tests cover:
- Successful config updates with valid values
- Validation failures with various invalid inputs
- Behavior when
serverKeyscache is reset- State consistency after failed updates
160-182: Good addition for runtime reconfiguration.The
updateConfigmethod provides a clean way to update configuration at runtime with proper validation and state management. The approach of resettingserverKeysafter config changes ensures fresh key generation on the next access.
160-182: Concurrent access concern is theoretically valid but updateConfig is unused internally—verify external usage requirements.
updateConfigis exported publicly but has zero callers in the codebase and no test coverage. While a race condition is theoretically possible (asyncgetSigningKey()yields control at its await, allowingupdateConfig()to resetserverKeysmid-operation), this only matters if the method is actually called externally.Verify:
- Is
updateConfigrequired for external SDK consumers?- If yes, add synchronization or atomic state updates to prevent race conditions.
- If no, consider removing it or deferring until there's an actual use case.
|
closed in favor of #292 |
Jira Issue: FDR-156.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability