feat: Allow for providers with equal name in the multiprovider - #1848
feat: Allow for providers with equal name in the multiprovider#1848chrfwow wants to merge 2 commits into
Conversation
Summary of ChangesHello @chrfwow, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the MultiProvider to allow providers with identical names by switching from a Map to a List for storing providers. This is a well-documented breaking change for the experimental Strategy interface. The modifications are consistently applied throughout the affected classes and their corresponding tests. The test suite has been updated effectively to accommodate these changes. I've identified one minor area for improvement in a test case to enhance its robustness. Overall, the change is well-implemented.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1848 +/- ##
============================================
+ Coverage 92.52% 93.05% +0.53%
+ Complexity 728 727 -1
============================================
Files 60 60
Lines 1739 1728 -11
Branches 202 200 -2
============================================
- Hits 1609 1608 -1
+ Misses 80 72 -8
+ Partials 50 48 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
chrfwow
changed the title
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
c2cb2b9 to
84e9870
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe multi-provider implementation now stores ordered ChangesMulti-provider list migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
🧹 Nitpick comments (4)
src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderMetadata.java (2)
11-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument ordering and duplicate preservation.
State that
originalMetadatafollows provider-list order and retains duplicate entries; consumers otherwise cannot safely associate metadata with providers sharing a name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderMetadata.java` around lines 11 - 12, Update the documentation for MultiProviderMetadata to specify that originalMetadata is ordered according to the underlying provider list and preserves duplicate entries, allowing consumers to associate metadata with providers that share a name.
19-19: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDefensively copy
originalMetadataat construction.
@Valuemakes the reference final, not theListcontents. A caller using the builder can mutate the list after construction, changing metadata returned bygetOriginalMetadata(). Copy the collection at the value-object boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderMetadata.java` at line 19, Defensively copy originalMetadata in the MultiProviderMetadata construction path so the stored list cannot be mutated through the builder’s input after construction. Preserve the existing metadata values and getter behavior while ensuring the value object owns an independent collection.src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java (1)
57-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the complete metadata list.
containsdoes not verify ordering or cardinality; incorrect aggregation, reordering, or dropped entries could still pass. Compare against the expected list directly and add a duplicate-name case.Proposed test adjustment
- List<Metadata> list = metadata.getOriginalMetadata(); - assertTrue(list.contains(mockMetaData1)); - assertTrue(list.contains(mockMetaData2)); + assertEquals(List.of(mockMetaData1, mockMetaData2), metadata.getOriginalMetadata()); - List<Metadata> list = metadata.getOriginalMetadata(); - assertTrue(list.contains(mockMetaData1)); + assertEquals(List.of(mockMetaData1), metadata.getOriginalMetadata());Also applies to: 169-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java` around lines 57 - 59, Update the metadata assertions in MultiProviderTest to compare getOriginalMetadata() directly with the expected ordered list, validating both contents and cardinality rather than using individual contains checks. Extend the test with duplicate-name metadata entries to verify aggregation retains all entries and preserves their expected order.src/main/java/dev/openfeature/sdk/multiprovider/FirstSuccessfulStrategy.java (1)
27-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover duplicate names in strategy evaluation.
This migration is intended to preserve multiple providers with the same metadata name, but strategy tests currently use unique names. Add equivalent coverage for both strategies with duplicate names and assert that both providers are evaluated and retained in error order.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/openfeature/sdk/multiprovider/FirstSuccessfulStrategy.java` around lines 27 - 35, Add strategy tests covering providers with duplicate metadata names for both evaluation strategies, ensuring each provider is invoked and both resulting errors are retained in their evaluation order. Use the existing provider-function and error-collection assertions, changing only provider names and expectations needed to verify duplicates are not collapsed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java`:
- Around line 58-60: Update the MultiProvider constructor to store an
unmodifiable defensive copy of the supplied providers list rather than retaining
the caller’s mutable list. Preserve the list’s original order and duplicate
entries, and keep strategy initialization, lifecycle operations, and evaluations
using this stable snapshot.
In `@src/main/java/dev/openfeature/sdk/multiprovider/Strategy.java`:
- Line 37: Update the `@param` providers documentation for the affected method in
Strategy to describe the List<FeatureProvider> accurately: state that provider
order is significant and duplicate provider names are preserved, removing the
outdated “ordered map” wording.
---
Nitpick comments:
In
`@src/main/java/dev/openfeature/sdk/multiprovider/FirstSuccessfulStrategy.java`:
- Around line 27-35: Add strategy tests covering providers with duplicate
metadata names for both evaluation strategies, ensuring each provider is invoked
and both resulting errors are retained in their evaluation order. Use the
existing provider-function and error-collection assertions, changing only
provider names and expectations needed to verify duplicates are not collapsed.
In `@src/main/java/dev/openfeature/sdk/multiprovider/MultiProviderMetadata.java`:
- Around line 11-12: Update the documentation for MultiProviderMetadata to
specify that originalMetadata is ordered according to the underlying provider
list and preserves duplicate entries, allowing consumers to associate metadata
with providers that share a name.
- Line 19: Defensively copy originalMetadata in the MultiProviderMetadata
construction path so the stored list cannot be mutated through the builder’s
input after construction. Preserve the existing metadata values and getter
behavior while ensuring the value object owns an independent collection.
In `@src/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java`:
- Around line 57-59: Update the metadata assertions in MultiProviderTest to
compare getOriginalMetadata() directly with the expected ordered list,
validating both contents and cardinality rather than using individual contains
checks. Extend the test with duplicate-name metadata entries to verify
aggregation retains all entries and preserves their expected order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f56b200-c35c-4311-8636-6d7c94743fee
📒 Files selected for processing (8)
src/main/java/dev/openfeature/sdk/multiprovider/FirstMatchStrategy.javasrc/main/java/dev/openfeature/sdk/multiprovider/FirstSuccessfulStrategy.javasrc/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.javasrc/main/java/dev/openfeature/sdk/multiprovider/MultiProviderMetadata.javasrc/main/java/dev/openfeature/sdk/multiprovider/Strategy.javasrc/test/java/dev/openfeature/sdk/multiprovider/BaseStrategyTest.javasrc/test/java/dev/openfeature/sdk/multiprovider/FirstSuccessfulStrategyTest.javasrc/test/java/dev/openfeature/sdk/multiprovider/MultiProviderTest.java
| this.providers = providers; | ||
| this.strategy = Objects.requireNonNull(strategy, "strategy must not be null"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'MultiProvider\.java|.*Provider.*\.java' . | sed 's#^\./##' | head -50
echo "== MultiProvider outline and relevant source =="
if [ -f src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java ]; then
wc -l src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java
ast-grep outline src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java || true
cat -n src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java
else
echo "MultiProvider.java not found at expected path"
fi
echo "== usages / references =="
rg -n "MultiProvider|providers|strategy" src/main/java/dev/openfeature/sdk -S || true
echo "== behavioral probe: direct retention and mutation impact =="
python3 - <<'PY'
import re, pathlib
p = pathlib.Path('src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java')
s = p.read_text()
checks = {
"constructor_stores_providers_directly": "this.providers = providers" in s,
"initializes_unmodifiable_metadata": "unmodifiableList" in s and "providerMetadata" in s,
"passes_providers_to_strategy": bool(re.search(r'strategy\s*=\s*.*provider.*\)', s, re.S)) or 'providers' in s and 'strategy' in s,
}
print(checks)
# Minimal Java-ish simulation of retaining a caller's mutable list and later mutation.
providers = ["A", "B"]
metadata = [f"{p}/meta" for p in providers]
providers.append("C")
print(metadata)
try:
for p in providers:
print(p)
providers.remove("B")
except RuntimeError as e:
print("mutation_while_iterating_error_type=", type(e).__name__)
PYRepository: open-feature/java-sdk
Length of output: 22557
Snapshot the provider list in the constructor.
MultiProvider retains the caller-supplied mutable list and passes it to strategies during evaluations. If callers mutate the list later, initialization results can diverge from lifecycle operations or evaluation behavior, and concurrent iteration can fail. Preserve duplicates and order while storing an unmodifiable defensive copy.
Proposed fix
- this.providers = providers;
+ this.providers = Collections.unmodifiableList(
+ new ArrayList<>(Objects.requireNonNull(providers, "providers must not be null")));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.providers = providers; | |
| this.strategy = Objects.requireNonNull(strategy, "strategy must not be null"); | |
| } | |
| this.providers = Collections.unmodifiableList( | |
| new ArrayList<>(Objects.requireNonNull(providers, "providers must not be null"))); | |
| this.strategy = Objects.requireNonNull(strategy, "strategy must not be null"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/dev/openfeature/sdk/multiprovider/MultiProvider.java` around
lines 58 - 60, Update the MultiProvider constructor to store an unmodifiable
defensive copy of the supplied providers list rather than retaining the caller’s
mutable list. Preserve the list’s original order and duplicate entries, and keep
strategy initialization, lifecycle operations, and evaluations using this stable
snapshot.
| */ | ||
| <T> ProviderEvaluation<T> evaluate( | ||
| Map<String, FeatureProvider> providers, | ||
| List<FeatureProvider> providers, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the provider parameter documentation.
The @param providers text at Line 28 still says “ordered map,” although this method now receives a List<FeatureProvider>. Document that list order is significant and duplicate provider names are preserved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/dev/openfeature/sdk/multiprovider/Strategy.java` at line 37,
Update the `@param` providers documentation for the affected method in Strategy to
describe the List<FeatureProvider> accurately: state that provider order is
significant and duplicate provider names are preserved, removing the outdated
“ordered map” wording.
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
|
toddbaert
left a comment
toddbaert
left a comment
There was a problem hiding this comment.
This is breaking - approved in spirit, blocking for now but will release with manually edited notes.
2 participants
This PR
Adds support for different providers with the same name in the multi provider. Since the interface
Strategyhas changed, this would be a breaking change if the multiprovider was not experimental.The downside is, that the providers are not passed to the
Strategyin a map, which makes the retrieval of a provider per name more difficult, and, depending on the number of providers, slower. However, since there will only ever be a handfull of providers, this should make no difference in real scenarios. Furthermore, the current map based version may result in unexpected behavior when two or more providers have the same name.Related Issues
Fixes #1792