Fix stream config adjustments on update #995
Merged
Conversation
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
Includes tests to verify domain is properly converted to `external.api` in stream configurations during create, update, and createOrUpdate operations. Adjusted validation in `NatsJSContext` to handle domain and external conflicts.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors domain-to-external-API conversion logic for JetStream stream operations and adjusts validation to allow Domain and External.Api to coexist.
- Extracted duplicate domain conversion logic into a shared
AdjustStreamConfigForDomainmethod - Modified validation to only throw when both Domain and External.Deliver are set (not just External)
- Added comprehensive tests verifying domain conversion for all stream operation methods
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/NATS.Client.JetStream/NatsJSContext.Streams.cs | Refactored domain conversion logic from three methods into a single shared method |
| src/NATS.Client.JetStream/NatsJSContext.cs | Updated validation logic to allow Domain with External.Api (only blocks External.Deliver) |
| tests/NATS.Client.JetStream.Tests/StreamDomainAdjustmentTest.cs | Added test coverage for domain-to-external-API conversion in all stream operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/NATS.Client.JetStream.Tests/StreamDomainAdjustmentTest.cs
Outdated
Show resolved
Hide resolved
tests/NATS.Client.JetStream.Tests/StreamDomainAdjustmentTest.cs
Outdated
Show resolved
Hide resolved
tests/NATS.Client.JetStream.Tests/StreamDomainAdjustmentTest.cs
Outdated
Show resolved
Hide resolved
tests/NATS.Client.JetStream.Tests/StreamDomainAdjustmentTest.cs
Outdated
Show resolved
Hide resolved
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.
This PR is a fix for the KV Mirror related features implemented in PR Added Domain support for stream mirroring and sourcing and KV full support for the same. #631
This pull request refactors how domain-to-external API conversion is handled for JetStream stream configurations and adds comprehensive tests to ensure correct behavior. The main change is the introduction of a shared method to adjust stream configurations for domain settings, ensuring consistent and correct conversion of
Domainfields to theExternal.Apiproperty for both mirrors and sources across all stream-related operations. Additionally, new tests verify that this conversion is performed as expected.Core refactoring and consistency improvements:
AdjustStreamConfigForDomaininNatsJSContext.Streams.csto handle conversion ofDomainfields toExternal.Apifor both mirrors and sources, replacing duplicated logic inCreateStreamAsync,UpdateStreamAsync, andCreateOrUpdateStreamAsync. All three methods now call this helper for consistent behavior. [1] [2] [3] [4]ConvertDomainmethod to throw an exception if bothDomainand an existingExternal.Deliverare set, ensuring correct configuration and preventing invalid states.Testing improvements:
Added a new test class
StreamDomainAdjustmentTestthat verifies domain-to-external API conversion for stream creation, update, and create-or-update operations, both with and without domains set on sources. These tests check both the outgoing payload and the returned configuration, ensuring the conversion is performed and that theDomainproperty is not serialized in the payload.fixes Updating a stream with external sources doesn't set the external JS API string as expected #994