fix(python): Add missing SuperpositionDataSource implementation for LocalResolutionProvider#966
fix(python): Add missing SuperpositionDataSource implementation for LocalResolutionProvider#966ayushjain17 wants to merge 1 commit intomainfrom
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<()> { |
| @abstractmethod | ||
| async def fetch_config( | ||
| self, | ||
| if_modified_since: Optional[datetime] = None, | ||
| ) -> FetchResponse[ConfigData]: | ||
| """Fetch the full resolved configuration. | ||
|
|
||
| Args: | ||
| if_modified_since: Optional timestamp for 304 Not Modified check. | ||
|
|
||
| Returns: | ||
| FetchResponse with ConfigData or NotModified status. | ||
| """ | ||
| 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.
| 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.
…ocalResolutionProvider
ayushjain17
force-pushed
the
provider/python
branch
from
71b1d89 to
282d84e
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