fix(python): Add missing SuperpositionDataSource implementation for LocalResolutionProvider#966
Conversation
|
Review changes with Changed Files |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request adds a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR aligns the provider/data-source abstraction across Rust + Python by making fetch_config a default wrapper over fetch_filtered_config, and adds the missing SuperpositionDataSource implementation for the Python LocalResolutionProvider. It also updates UniFFI experiment filtering to support a new partial_apply boolean flag (with regenerated Python/Java bindings).
Changes:
- Rust: Provide a default
SuperpositionDataSource::fetch_config()implementation delegating tofetch_filtered_config(), and update concrete data sources/providers accordingly. - Python: Implement
SuperpositionDataSourceonLocalResolutionProviderand rely on the base-class defaultfetch_config(). - UniFFI/bindings: Extend
ProviderCache.filter_experiment(...)with apartial_applyparameter and regenerate Python/Java bindings.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/superposition_provider/src/local_provider.rs | Refactors lifecycle method visibility and updates data-source trait impl to use fetch_filtered_config. |
| crates/superposition_provider/src/data_source/http.rs | Moves config fetching into fetch_filtered_config under the trait impl; relies on default fetch_config. |
| crates/superposition_provider/src/data_source/file.rs | Implements filtering directly in fetch_filtered_config; relies on default fetch_config. |
| crates/superposition_provider/src/data_source.rs | Adds default fetch_config() delegating to fetch_filtered_config(). |
| crates/superposition_core/src/ffi.rs | Adds partial_apply behavior to experiment filtering in ProviderCache. |
| clients/python/provider/superposition_provider/local_provider.py | Adds missing SuperpositionDataSource methods on LocalResolutionProvider. |
| clients/python/provider/superposition_provider/http_data_source.py | Removes fetch_config override in favor of default; keeps fetch_filtered_config. |
| clients/python/provider/superposition_provider/file_data_source.py | Removes fetch_config override in favor of default; keeps fetch_filtered_config. |
| clients/python/provider/superposition_provider/data_source.py | Implements default fetch_config() by calling fetch_filtered_config(). |
| clients/python/bindings/superposition_bindings/superposition_client.py | Regenerated UniFFI Python binding for filter_experiment(partial_apply). |
| clients/java/bindings/src/main/kotlin/uniffi/superposition_client/superposition_client.kt | Regenerated UniFFI Kotlin binding for filterExperiment(partialApply). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| pub async fn init(&self, context: EvaluationContext) -> Result<()> { | ||
| async fn init(&self, context: EvaluationContext) -> Result<()> { |
There was a problem hiding this comment.
init was changed from pub async fn to a private async fn, which is a breaking API change: current crate examples (e.g. crates/superposition_provider/examples/local_file_example.rs) call provider.init(...) and will no longer compile. Either keep init public (or provide a public wrapper) or update the examples/public API to use the OpenFeature FeatureProvider::initialize entrypoint instead (and document the new usage).
| async fn init(&self, context: EvaluationContext) -> Result<()> { | |
| pub async fn init(&self, context: EvaluationContext) -> Result<()> { |
| } | ||
|
|
||
| pub async fn close_provider(&self) -> Result<()> { | ||
| async fn close_provider(&self) -> Result<()> { |
There was a problem hiding this comment.
close_provider was made private, but it’s still used externally by the crate examples (e.g. provider.close_provider().await in examples/local_http_example.rs). This is a breaking change that will break cargo test --examples / cargo build --examples. Either keep it public (or expose a public close/shutdown method on the provider) or update the examples and any public docs accordingly.
| async fn close_provider(&self) -> Result<()> { | |
| pub async fn close_provider(&self) -> Result<()> { |
There was a problem hiding this comment.
You can address this.This looks legit @ayushjain17
| """ | ||
| pass | ||
| return await self.fetch_filtered_config(if_modified_since=if_modified_since) | ||
|
|
There was a problem hiding this comment.
fetch_config still has the @abstractmethod decorator even though it now provides a default implementation. In Python, that means subclasses must override it to be instantiable, so removing fetch_config from HttpDataSource/FileDataSource and adding SuperpositionDataSource to LocalResolutionProvider will make those classes abstract at runtime. Drop @abstractmethod from fetch_config (keep it as a concrete default) so implementors only need to supply fetch_filtered_config.
| if_modified_since: Optional[datetime] = None | ||
| ) -> FetchResponse[ConfigData]: | ||
| if_modified_since: Optional[datetime] = None, | ||
| ) -> FetchResponse: |
There was a problem hiding this comment.
Return type annotation for fetch_filtered_config was loosened to FetchResponse (no generic). This loses typing for callers and is inconsistent with the interface (FetchResponse[ConfigData]). Update the annotation to FetchResponse[ConfigData].
| ) -> FetchResponse: | |
| ) -> FetchResponse[ConfigData]: |
| for production configurations that need filtering. | ||
|
|
||
| Args: | ||
| context: Optional context for filtering (ignored). |
There was a problem hiding this comment.
Docstring says context is “ignored”, but this method passes context into _parse_config_file(...) (and the helper JSON-encodes it for the FFI call). Update the docstring to reflect that context is actually applied during parsing/filtering.
| context: Optional context for filtering (ignored). | |
| context: Optional context applied during parsing/filtering. |
| if not self.ffi_cache or not self.cached_config: | ||
| raise RuntimeError("Provider not properly initialized or no config available") | ||
|
|
||
| if if_modified_since is not None: | ||
| logger.debug("LocalResolutionProvider: ignoring if_modified_since, always reading fresh from file") | ||
|
|
||
| return FetchResponse.data(ConfigData( | ||
| data=self.ffi_cache.filter_config(context, prefix_filter), | ||
| fetched_at=self.cached_config.fetched_at, | ||
| )) |
There was a problem hiding this comment.
ProviderCache.filter_config expects dimension_data: Optional[dict[str, str]] where values are strings (the UniFFI converter enforces this). This implementation passes context: Dict[str, Any] directly, which will raise at runtime if any values aren’t strings. Convert context to the expected {k: json.dumps(v)} (same as _merge_contexts) before calling into ffi_cache, and update the doc/log message (“reading fresh from file”) since this is filtering cached in-memory data.
| if if_modified_since is not None: | ||
| logger.debug("LocalResolutionProvider: ignoring if_modified_since for experiments, always returning cached data") | ||
|
|
||
| return FetchResponse.data(ExperimentData( | ||
| data=self.ffi_cache.filter_experiment(context, prefix_filter, False), | ||
| fetched_at=self.cached_experiments.fetched_at, | ||
| )) |
There was a problem hiding this comment.
ProviderCache.filter_experiment (UniFFI) also requires dimension_data values to be strings. These calls pass context: Dict[str, Any] directly, which can fail at runtime for non-string values. Convert context to {k: json.dumps(v)} (or reuse the same normalization helper used elsewhere) before calling ffi_cache.filter_experiment(...).
| if not self.ffi_cache or not self.cached_experiments: | ||
| raise RuntimeError("Provider not properly initialized or no experiments available") | ||
|
|
||
| if if_modified_since is not None: | ||
| logger.debug("LocalResolutionProvider: ignoring if_modified_since for experiments, always returning cached data") | ||
|
|
||
| return FetchResponse.data(ExperimentData( | ||
| data=self.ffi_cache.filter_experiment(context, prefix_filter, True), | ||
| fetched_at=self.cached_experiments.fetched_at, | ||
| )) |
There was a problem hiding this comment.
Same issue as above: context is passed directly into UniFFI filter_experiment but must be dict[str, str] (string values). Convert/normalize the context values before calling into ffi_cache.filter_experiment(...).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/python/provider/superposition_provider/http_data_source.py`:
- Around line 65-70: Change the return annotation of the fetch_filtered_config
method to include the generic payload type so it matches the base class
contract: update the signature of fetch_filtered_config to return
FetchResponse[ConfigData] instead of bare FetchResponse; keep the existing
implementation (which constructs ConfigData and uses not_modified()), only
adjust the type hint to FetchResponse[ConfigData] to restore static typing
consistency with other methods like fetch_active_experiments.
In `@clients/python/provider/superposition_provider/local_provider.py`:
- Around line 567-588: fetch_active_experiments currently returns the provider's
internal mutable cache by calling FetchResponse.data(self.cached_experiments);
change this to return a detached copy to avoid exposing internal state. Update
fetch_active_experiments to create and return a deep copy of
self.cached_experiments (e.g., via copy.deepcopy or by constructing new
ExperimentData objects) before passing it to FetchResponse.data, ensuring
callers cannot mutate the provider's cached_experiments; reference the
fetch_active_experiments method, FetchResponse.data, and self.cached_experiments
when making the change.
In `@crates/superposition_provider/src/local_provider.rs`:
- Line 64: The init and close_provider methods were made private but are used by
external examples (e.g., provider.init(...) and provider.close_provider(...)),
causing a breaking change; restore their public visibility by changing their
signatures back to public (e.g., make async fn init(...) -> Result<()> into pub
async fn init(...)->Result<()> and make close_provider public as pub fn
close_provider(...) or equivalent), and ensure any other occurrences (the other
method at the second location) are updated the same way so external callers
compile.
🪄 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
Run ID: 25bd794e-ee60-4e1d-ab1b-74438e47066b
📒 Files selected for processing (11)
clients/java/bindings/src/main/kotlin/uniffi/superposition_client/superposition_client.ktclients/python/bindings/superposition_bindings/superposition_client.pyclients/python/provider/superposition_provider/data_source.pyclients/python/provider/superposition_provider/file_data_source.pyclients/python/provider/superposition_provider/http_data_source.pyclients/python/provider/superposition_provider/local_provider.pycrates/superposition_core/src/ffi.rscrates/superposition_provider/src/data_source.rscrates/superposition_provider/src/data_source/file.rscrates/superposition_provider/src/data_source/http.rscrates/superposition_provider/src/local_provider.rs
| async def fetch_filtered_config( | ||
| self, | ||
| context: Optional[Dict[str, Any]] = None, | ||
| prefix_filter: Optional[List[str]] = None, | ||
| if_modified_since: Optional[datetime] = None | ||
| ) -> FetchResponse[ConfigData]: | ||
| if_modified_since: Optional[datetime] = None, | ||
| ) -> FetchResponse: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find clients/python/provider/superposition_provider -name "*.py" -type f | head -20Repository: juspay/superposition
Length of output: 880
🏁 Script executed:
cat -n clients/python/provider/superposition_provider/http_data_source.py | sed -n '60,85p'Repository: juspay/superposition
Length of output: 1209
🏁 Script executed:
cat -n clients/python/provider/superposition_provider/data_source.py | head -100Repository: juspay/superposition
Length of output: 3896
🏁 Script executed:
cat -n clients/python/provider/superposition_provider/data_source.py | sed -n '96,110p'Repository: juspay/superposition
Length of output: 782
🏁 Script executed:
cat -n clients/python/provider/superposition_provider/http_data_source.py | sed -n '1,150p'Repository: juspay/superposition
Length of output: 6702
Add the generic type parameter to match the base class contract.
The base class specifies FetchResponse[ConfigData], but this override uses bare FetchResponse at line 70. This drops the payload type annotation and weakens static type checking. The implementation correctly returns ConfigData instances (line 92) and handles not_modified() (line 98), but the return annotation should be -> FetchResponse[ConfigData] to maintain consistency with the abstract contract and match the pattern used by other methods like fetch_active_experiments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/python/provider/superposition_provider/http_data_source.py` around
lines 65 - 70, Change the return annotation of the fetch_filtered_config method
to include the generic payload type so it matches the base class contract:
update the signature of fetch_filtered_config to return
FetchResponse[ConfigData] instead of bare FetchResponse; keep the existing
implementation (which constructs ConfigData and uses not_modified()), only
adjust the type hint to FetchResponse[ConfigData] to restore static typing
consistency with other methods like fetch_active_experiments.
There was a problem hiding this comment.
@sauraww, agreed! Here's the fix — just restore the generic type parameter on the return annotation:
async def fetch_filtered_config(
self,
context: Optional[Dict[str, Any]] = None,
prefix_filter: Optional[List[str]] = None,
if_modified_since: Optional[datetime] = None,
) -> FetchResponse[ConfigData]:This brings HttpDataSource.fetch_filtered_config back in line with the base class contract in SuperpositionDataSource and with the pattern used by fetch_active_experiments → FetchResponse[ExperimentData], restoring proper static type checking across the board.
🧠 Learnings used
Learnt from: ayushjain17
Repo: juspay/superposition PR: 951
File: clients/python/provider/superposition_provider/http_data_source.py:86-88
Timestamp: 2026-04-01T17:35:07.462Z
Learning: In the Superposition Python provider (`clients/python/provider/superposition_provider/http_data_source.py`), `HttpDataSource` is a stateless fetch adapter with no internal storage or lifecycle (no concept of "initial load" vs. "refresh"). It maps `UnknownApiError` to `FetchResponse.not_modified()` by design. The caller (`LocalResolutionProvider`) is responsible for distinguishing initialization fetches from background refresh fetches and for deciding how to handle `not_modified()` responses in each case.
Learnt from: ayushjain17
Repo: juspay/superposition PR: 951
File: crates/superposition_provider/src/local_provider.rs:271-336
Timestamp: 2026-04-01T17:53:15.684Z
Learning: In `crates/superposition_provider/src/local_provider.rs`, the `do_refresh()` method intentionally allows `cached_config` and `cached_experiments` to be updated independently (potentially out of sync). Temporary divergence between the two caches is an accepted trade-off by design — do not flag partial/non-atomic cache updates in this file as an issue.
| async def fetch_active_experiments( | ||
| self, | ||
| if_modified_since: Optional[datetime] = None, | ||
| ) -> FetchResponse[ExperimentData]: | ||
| """Fetch experiments from file. | ||
|
|
||
| Args: | ||
| if_modified_since: Timestamp for 304 Not Modified check. | ||
|
|
||
| Returns: | ||
| FetchResponse with ExperimentData or NotModified status. | ||
| """ | ||
| if not self.supports_experiments(): | ||
| raise NotImplementedError("Experiments not supported by this provider") | ||
|
|
||
| if not self.cached_experiments: | ||
| raise RuntimeError("Provider not properly initialized or no experiments available") | ||
|
|
||
| if if_modified_since is not None: | ||
| logger.debug("LocalResolutionProvider: ignoring if_modified_since for experiments, always returning cached data") | ||
|
|
||
| return FetchResponse.data(self.cached_experiments) |
There was a problem hiding this comment.
Don't expose the cached experiment object by reference.
FetchResponse.data(self.cached_experiments) returns the provider's mutable cache object directly. If a caller mutates that ExperimentData, it rewrites internal state and can affect later evaluations. Return a detached copy here instead of the cached instance itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/python/provider/superposition_provider/local_provider.py` around
lines 567 - 588, fetch_active_experiments currently returns the provider's
internal mutable cache by calling FetchResponse.data(self.cached_experiments);
change this to return a detached copy to avoid exposing internal state. Update
fetch_active_experiments to create and return a deep copy of
self.cached_experiments (e.g., via copy.deepcopy or by constructing new
ExperimentData objects) before passing it to FetchResponse.data, ensuring
callers cannot mutate the provider's cached_experiments; reference the
fetch_active_experiments method, FetchResponse.data, and self.cached_experiments
when making the change.
| } | ||
|
|
||
| pub async fn init(&self, context: EvaluationContext) -> Result<()> { | ||
| async fn init(&self, context: EvaluationContext) -> Result<()> { |
There was a problem hiding this comment.
Keep these methods public or update the external API surface.
examples/local_file_example.rs and examples/local_http_example.rs still call provider.init(...) and provider.close_provider(...) directly. Making them private turns this into a breaking change and will make those callers stop compiling.
Suggested fix
- async fn init(&self, context: EvaluationContext) -> Result<()> {
+ pub async fn init(&self, context: EvaluationContext) -> Result<()> {- async fn close_provider(&self) -> Result<()> {
+ pub async fn close_provider(&self) -> Result<()> {Also applies to: 212-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_provider/src/local_provider.rs` at line 64, The init and
close_provider methods were made private but are used by external examples
(e.g., provider.init(...) and provider.close_provider(...)), causing a breaking
change; restore their public visibility by changing their signatures back to
public (e.g., make async fn init(...) -> Result<()> into pub async fn
init(...)->Result<()> and make close_provider public as pub fn
close_provider(...) or equivalent), and ensure any other occurrences (the other
method at the second location) are updated the same way so external callers
compile.
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify the example files to confirm the breaking change before finalizing.
🏁 Script executed:
#!/bin/bash
rg -n "init\|close_provider" --type rust -C3 examples/
Length of output: 0
No results found. Let me search more broadly.
🏁 Script executed:
#!/bin/bash
# Check if examples directory exists and list files
fd . examples/ 2>/dev/null || echo "No examples directory found"
# Search entire codebase for direct calls to .init( and .close_provider(
echo "=== .init( calls ==="
rg -n '\.init\(' --type rust -C2
echo "=== .close_provider( calls ==="
rg -n '\.close_provider\(' --type rust -C2
Length of output: 7718
@sauraww, confirmed — the finding is valid. Three example files under crates/superposition_provider/examples/ call these methods directly on the provider instance:
| File | Call |
|---|---|
local_file_example.rs (lines 25, 34) |
provider.init(...) · provider.close_provider() |
local_http_example.rs (lines 28, 46) |
provider.init(...) · provider.close_provider() |
local_file_watch_example.rs (line 26) |
provider.init(...) |
Making both methods private will break compilation of all three examples. The fix is to restore pub on both:
// local_provider.rs
pub async fn init(&self, context: EvaluationContext) -> Result<()> { … }
pub async fn close_provider(&self) -> Result<()> { … }🧠 Learnings used
Learnt from: ayushjain17
Repo: juspay/superposition PR: 951
File: crates/superposition_provider/tests/integration_test.rs:409-426
Timestamp: 2026-04-03T13:55:52.171Z
Learning: In `crates/superposition_provider/tests/integration_test.rs`, `LocalResolutionProvider::clone()` is a reference clone (Arc-based shared state), not a deep copy. Both the clone and the original share the same underlying provider (including background refresh tasks). Shutting down either one (or the OpenFeature API instance that owns it) terminates the shared polling loop for all handles — do not flag missing `shutdown()` calls on clones as leaked refresh loops.
Learnt from: ayushjain17
Repo: juspay/superposition PR: 951
File: crates/superposition_provider/src/local_provider.rs:271-336
Timestamp: 2026-04-01T17:53:15.684Z
Learning: In `crates/superposition_provider/src/local_provider.rs`, the `do_refresh()` method intentionally allows `cached_config` and `cached_experiments` to be updated independently (potentially out of sync). Temporary divergence between the two caches is an accepted trade-off by design — do not flag partial/non-atomic cache updates in this file as an issue.
Learnt from: ayushjain17
Repo: juspay/superposition PR: 951
File: clients/python/provider/superposition_provider/http_data_source.py:86-88
Timestamp: 2026-04-01T17:35:07.462Z
Learning: In the Superposition Python provider (`clients/python/provider/superposition_provider/http_data_source.py`), `HttpDataSource` is a stateless fetch adapter with no internal storage or lifecycle (no concept of "initial load" vs. "refresh"). It maps `UnknownApiError` to `FetchResponse.not_modified()` by design. The caller (`LocalResolutionProvider`) is responsible for distinguishing initialization fetches from background refresh fetches and for deciding how to handle `not_modified()` responses in each case.
Learnt from: ayushjain17
Repo: juspay/superposition PR: 827
File: crates/superposition_core/src/config.rs:154-154
Timestamp: 2026-01-07T20:38:53.153Z
Learning: Deprecate jsonlogic-based context conditions and migrate to simple key-value pair map conditions across the codebase. Replace jsonlogic::apply usage with superposition_types::apply for condition evaluation. Update all relevant modules (notably Rust files under crates) to use the new approach, remove jsonlogic dependencies where applicable, and run full compilation and tests to catch regressions. Ensure context evaluation logic consistently uses key-value maps and that the architectural change is reflected in unit/integration tests.
ayushjain17
force-pushed
the
provider/python
branch
2 times, most recently
from
282d84e to
4be4467
Compare
| if_modified_since: Optional[datetime] = None, | ||
| ) -> FetchResponse[ExperimentData]: | ||
| """Fetch candidate active experiments.""" | ||
| if not self.supports_experiments(): |
There was a problem hiding this comment.
Can we move all of the common validation into a single function ?
| @@ -72,40 +74,25 @@ impl SuperpositionDataSource for FileDataSource { | |||
| "json" => JsonFormat::parse_config, | |||
| _ => TomlFormat::parse_config, | |||
There was a problem hiding this comment.
I mean we should add a comment at least to check back later on and change once yaml is added
sauraww
left a comment
sauraww
left a comment
There was a problem hiding this comment.
Reviewing copilot's comment
…ocalResolutionProvider
ayushjain17
force-pushed
the
provider/python
branch
from
4be4467 to
7b64e64
Compare
ChangeLog
fetch_filtered_configforfetch_configin the default implementation to avoid unnecessary implementationSummary by CodeRabbit
New Features
partialApplyparameter to experiment filtering methods, enabling partial application filtering support across Kotlin and Python bindings.Refactoring