fix(openfeature): Python provider improvements#1095
Open
ayushjain17 wants to merge 1 commit into
Open
Conversation
Contributor
|
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:
WalkthroughThe Python provider SDK adds typed errors, millisecond-based refresh configuration, explicit HTTP 304 handling, stricter flag type resolution, and updated local/remote provider lifecycle behavior with new contract tests. ChangesProvider SDK behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LocalResolutionProvider
participant HttpDataSource
participant FetchResponse
participant ProviderStatus
LocalResolutionProvider->>HttpDataSource: fetch filtered config
HttpDataSource->>FetchResponse: return data or NotModified
LocalResolutionProvider->>LocalResolutionProvider: refresh within timeout
LocalResolutionProvider->>ProviderStatus: emit READY or STALE state
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 updates the Python Superposition OpenFeature provider to better match the OpenFeature spec and align behavior with other Superposition client implementations, focusing on stricter type handling, clearer error signaling, and correct HTTP cache semantics.
Changes:
- Tightens flag type extraction (notably boolean coercion) and improves per-flag error reporting (FLAG_NOT_FOUND vs TYPE_MISMATCH).
- Reworks refresh strategy durations to be millisecond-based (with deprecated seconds fields) and adds refresh timeout/staleness signaling behavior.
- Fixes HTTP 304 “Not Modified” handling so it’s not conflated with real failures; adds standalone contract tests for type handling and HTTP behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| clients/python/provider/test_type_contract.py | Adds standalone contract tests to pin cross-language flag type behavior (no coercion except int→float widening). |
| clients/python/provider/test_http_data_source.py | Adds standalone tests ensuring HTTP 304 is distinguishable from auth/server failures. |
| clients/python/provider/superposition_provider/types.py | Introduces millisecond-based refresh strategy fields with deprecated seconds fields and validation helpers. |
| clients/python/provider/superposition_provider/remote_provider.py | Adjusts initialization failure status and modifies applicable-variants identifier handling. |
| clients/python/provider/superposition_provider/provider.py | Removes evaluation cache / default_toss plumbing from provider initialization. |
| clients/python/provider/superposition_provider/local_provider.py | Improves error typing, JSON decoding diagnostics, refresh timeout handling, and stale-status/event emission. |
| clients/python/provider/superposition_provider/interfaces.py | Improves typed resolution error details and removes permissive boolean coercion. |
| clients/python/provider/superposition_provider/http_data_source.py | Adds interceptor-based 304 detection and wraps non-304 failures as typed network errors. |
| clients/python/provider/superposition_provider/file_data_source.py | Switches to typed provider errors for unsupported formats and read failures. |
| clients/python/provider/superposition_provider/exp_config.py | Updates polling/on-demand strategy handling to use millisecond helpers. |
| clients/python/provider/superposition_provider/errors.py | Adds a typed SuperpositionError with a provider-specific ErrorCode enum. |
| clients/python/provider/superposition_provider/data_source.py | Makes FetchResponse a true sum type (NotModified vs Data) rather than “data is None”. |
| clients/python/provider/superposition_provider/cac_config.py | Updates polling/on-demand strategy handling to use millisecond helpers. |
| clients/python/provider/superposition_provider/init.py | Updates exports (adds typed errors; removes deprecated option types). |
| clients/python/provider-sdk-tests/main.py | Updates SDK test harness to use millisecond refresh strategy fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+24
to
+28
| # Refresh Strategy Types | ||
| # | ||
| # Durations are MILLISECONDS, matching the other Superposition clients. The seconds-based | ||
| # `interval` / `ttl` / `timeout` fields are DEPRECATED: set the `_milliseconds` field instead. | ||
| # When both are given the `_milliseconds` field wins. |
Comment on lines
11
to
+13
| from openfeature.evaluation_context import EvaluationContext | ||
| from openfeature.exception import ErrorCode | ||
| from openfeature.flag_evaluation import FlagResolutionDetails | ||
| from openfeature.flag_evaluation import FlagResolutionDetails, Reason |
Comment on lines
147
to
155
| targeting_key, merged_context = self._get_merged_context(context) | ||
|
|
||
| if not targeting_key: | ||
| logger.warning("Missing targeting key for variant resolution") | ||
| return [] | ||
|
|
||
| try: | ||
| response = await self.client.applicable_variants( | ||
| input=ApplicableVariantsInput( | ||
| workspace_id=self.options.workspace_id, | ||
| org_id=self.options.org_id, | ||
| identifier=targeting_key, | ||
| identifier=targeting_key or "", | ||
| context=merged_context, |
Collaborator
Author
There was a problem hiding this comment.
let the server handle the behaviour
Comment on lines
+26
to
+28
| # Durations are MILLISECONDS, matching the other Superposition clients. The seconds-based | ||
| # `interval` / `ttl` / `timeout` fields are DEPRECATED: set the `_milliseconds` field instead. | ||
| # When both are given the `_milliseconds` field wins. |
Comment on lines
+74
to
+76
| finally: | ||
| server.shutdown() | ||
|
|
Comment on lines
+91
to
+94
| finally: | ||
| STATUS, BODY = 200, {} | ||
| server.shutdown() | ||
|
|
Comment on lines
+106
to
+109
| finally: | ||
| STATUS, BODY = 200, {} | ||
| server.shutdown() | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
clients/python/provider/superposition_provider/local_provider.py (2)
470-499: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMisleading "fallback" messaging during refresh (init=False) — see consolidated comment below for the fix.
Also applies to: 515-543
🤖 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 `@clients/python/provider/superposition_provider/local_provider.py` around lines 470 - 499, Update _fetch_and_cache_config so the init=False error path does not claim that fallback fetching was attempted when it only logs the primary refresh failure. Keep the init=True fallback attempt and its messaging unchanged, and revise the corresponding refresh-path messaging in the related handling around _handle_fetch_config_from_fallback to accurately describe what occurred.
545-573: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard
self.cached_configbefore computing refresh state
_ensure_fresh_data()dereferencesself.cached_config.fetched_atbefore the provider’s not-initialized checks can run. On the on-demand path, that raisesAttributeErrorwhen config hasn’t been cached yet, so the caller never reaches the intendedprovider_error(...)path.Fix
- should_refresh_config = self.cached_config.fetched_at is None or is_elapsed(self.cached_config.fetched_at) + should_refresh_config = self.cached_config is None or self.cached_config.fetched_at is None or is_elapsed(self.cached_config.fetched_at)🤖 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 `@clients/python/provider/superposition_provider/local_provider.py` around lines 545 - 573, Update _ensure_fresh_data to handle an uninitialized self.cached_config before accessing fetched_at or computing refresh state, allowing the existing provider_error(...) path to run as intended. Preserve the current refresh behavior for initialized configuration and experiment caches.
🧹 Nitpick comments (6)
clients/python/provider/superposition_provider/interfaces.py (2)
293-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the comment to reflect the async context.
This comment appears to be copy-pasted from the sync
resolve_typedmethod and incorrectly refers to "SuperpositionAPIProvider is async-only" and "sync call".💡 Proposed wording adjustment
except NotImplementedError: - # A provider that cannot resolve this way at all (SuperpositionAPIProvider is - # async-only). Reporting it as a per-flag GENERAL error made every sync call quietly + # A provider that cannot resolve this way at all (e.g., a sync-only provider). + # Reporting it as a per-flag GENERAL error made every async call quietly # return the default forever, with nothing to distinguish it from a real evaluation. raise🤖 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 `@clients/python/provider/superposition_provider/interfaces.py` around lines 293 - 296, Update the comment immediately preceding the bare raise in the async resolution method to describe the async context, removing references to sync calls and the provider being async-only while preserving the explanation that unresolved exceptions must not be converted into per-flag GENERAL errors.
196-202: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExplicitly cast widened integers to
float.Both the sync and async
resolve_floatextractors successfully allow anintto pass through, but they return the originalintobject as-is. While Python treats integers and floats interoperably in most math operations, returning anintwhen the method signature guaranteesFlagResolutionDetails[float]may cause subtle downstream bugs if user code expects strictly float-specific behavior (e.g., using the.is_integer()method).
clients/python/provider/superposition_provider/interfaces.py#L196-L202: explicitly castvto float in the lambda:lambda v: float(v) if isinstance(v, (int, float)) and not isinstance(v, bool) else None.clients/python/provider/superposition_provider/interfaces.py#L358-L364: explicitly castvto float in the lambda:lambda v: float(v) if isinstance(v, (int, float)) and not isinstance(v, bool) else None.🤖 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 `@clients/python/provider/superposition_provider/interfaces.py` around lines 196 - 202, Update the sync resolve_float extractor in clients/python/provider/superposition_provider/interfaces.py:196-202 and the async resolve_float extractor in clients/python/provider/superposition_provider/interfaces.py:358-364 to convert accepted int or float values with float(v), while continuing to reject bool and unsupported types.clients/python/provider/test_type_contract.py (1)
60-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the strict type of the widened float.
If the float extractors in the provider are updated to explicitly cast integers to
float, you can strengthen this test by explicitly asserting the type of the returned value. Since10000 == 10000.0is true in Python regardless of the type, verifying the object instance guarantees the widening worked precisely as expected.💡 Proposed test enhancement
def test_an_integer_widens_to_a_float(): # Lossless, and the one conversion all three clients allow. - assert provider.resolve_float("price", 0.0, ctx).value == 10000.0 + result = provider.resolve_float("price", 0.0, ctx).value + assert result == 10000.0 + assert isinstance(result, float)🤖 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 `@clients/python/provider/test_type_contract.py` around lines 60 - 62, Update test_an_integer_widens_to_a_float to assert that provider.resolve_float(...).value is a float, in addition to preserving the existing 10000.0 value assertion.clients/python/provider/superposition_provider/data_source.py (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding type hints to
map_data.Adding type hints for the
mappercallable and the return type will improve static analysis and developer experience, ensuring consistency with the genericFetchResponse[T]design.♻️ Proposed refactor
First, ensure
CallableandTypeVarare imported:+from typing import Callable, TypeVar + +U = TypeVar("U")Then, update the
map_datamethod:- def map_data(self, mapper): + def map_data(self, mapper: Callable[[T], U]) -> "FetchResponse[U]": """Transform the data if present, preserving a NotModified response.""" if self._not_modified: return FetchResponse.not_modified() return FetchResponse.data(mapper(self._data))🤖 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 `@clients/python/provider/superposition_provider/data_source.py` around lines 52 - 56, Update FetchResponse.map_data with type hints for the mapper callable and its generic FetchResponse return value, introducing or reusing the appropriate TypeVar and Callable imports. Preserve the existing NotModified handling and mapped-data behavior while aligning the annotations with the FetchResponse[T] design.clients/python/provider/test_http_data_source.py (1)
52-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
HttpDataSourcecreated in each test is never closed.
source.fetch_config(None)is awaited butsource.close()is never called, leaking the underlying aiohttp session/connector on every test run (each of the 3 tests creates a fresh one). This typically surfaces as "Unclosed client session" warnings and can leave sockets open across test runs.🧹 Suggested fix
async def run(): source = HttpDataSource(SuperpositionOptions( endpoint=f"http://127.0.0.1:{server.server_address[1]}", token="test-token", org_id="test-org", workspace_id="test-workspace", )) - return await source.fetch_config(None) + try: + return await source.fetch_config(None) + finally: + await source.close()🤖 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 `@clients/python/provider/test_http_data_source.py` around lines 52 - 63, Update the _fetch_config helper to close the HttpDataSource after fetch_config(None) completes, ensuring cleanup occurs even when fetching raises; preserve the existing return value and asyncio.run flow.clients/python/provider/superposition_provider/types.py (1)
74-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
interval_ms()/ttl_ms()raise rawValueError, not the new typedSuperpositionError.Given this PR's goal of typed provider errors (
SuperpositionError.provider_error/config_erroretc. pererrors.py), a misconfigured strategy (e.g.PollingStrategy()with neitherintervalnorinterval_millisecondsset) surfaces as a bareValueErrorfrom deep inside_start_refresh_strategy/_ensure_fresh_data/create_config(), uncaught and unconverted anywhere in the files reviewed. Callers trying to catchSuperpositionErroruniformly won't catch this.♻️ Suggested direction
def interval_ms(self) -> int: """The refresh interval in milliseconds.""" resolved = _resolve_ms(self.interval_milliseconds, self.interval, "interval") if resolved is None: - raise ValueError("PollingStrategy needs interval_milliseconds") + raise SuperpositionError.provider_error("PollingStrategy needs interval_milliseconds") return resolvedAlso applies to: 104-109
🤖 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 `@clients/python/provider/superposition_provider/types.py` around lines 74 - 79, Update interval_ms() and ttl_ms() to raise the typed SuperpositionError configuration error instead of raw ValueError when no interval or TTL is configured. Use the existing SuperpositionError.provider_error/config_error construction pattern from errors.py so misconfigured strategies are consistently catchable by callers.
🤖 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 `@clients/python/provider/superposition_provider/cac_config.py`:
- Around line 142-154: Update the refresh scheduling in the configuration flows
containing poll_config and the OnDemandStrategy branch: ensure poll_config
applies the provided timeout when waiting or retrying, and convert the value
returned by ttl_ms() from milliseconds to the seconds unit expected by
timedelta. Apply the same corrections in exp_config.py while preserving the
existing strategy behavior.
In `@clients/python/provider/superposition_provider/exp_config.py`:
- Around line 94-106: Update the OnDemandStrategy refresh flow in
on_demand_config to pass ttl from ttl_ms() as milliseconds when constructing the
timedelta, preserving the intended TTL duration. Also either use the computed
timeout in the refresh logic or remove the unused timeout retrieval and logging
from this path.
---
Outside diff comments:
In `@clients/python/provider/superposition_provider/local_provider.py`:
- Around line 470-499: Update _fetch_and_cache_config so the init=False error
path does not claim that fallback fetching was attempted when it only logs the
primary refresh failure. Keep the init=True fallback attempt and its messaging
unchanged, and revise the corresponding refresh-path messaging in the related
handling around _handle_fetch_config_from_fallback to accurately describe what
occurred.
- Around line 545-573: Update _ensure_fresh_data to handle an uninitialized
self.cached_config before accessing fetched_at or computing refresh state,
allowing the existing provider_error(...) path to run as intended. Preserve the
current refresh behavior for initialized configuration and experiment caches.
---
Nitpick comments:
In `@clients/python/provider/superposition_provider/data_source.py`:
- Around line 52-56: Update FetchResponse.map_data with type hints for the
mapper callable and its generic FetchResponse return value, introducing or
reusing the appropriate TypeVar and Callable imports. Preserve the existing
NotModified handling and mapped-data behavior while aligning the annotations
with the FetchResponse[T] design.
In `@clients/python/provider/superposition_provider/interfaces.py`:
- Around line 293-296: Update the comment immediately preceding the bare raise
in the async resolution method to describe the async context, removing
references to sync calls and the provider being async-only while preserving the
explanation that unresolved exceptions must not be converted into per-flag
GENERAL errors.
- Around line 196-202: Update the sync resolve_float extractor in
clients/python/provider/superposition_provider/interfaces.py:196-202 and the
async resolve_float extractor in
clients/python/provider/superposition_provider/interfaces.py:358-364 to convert
accepted int or float values with float(v), while continuing to reject bool and
unsupported types.
In `@clients/python/provider/superposition_provider/types.py`:
- Around line 74-79: Update interval_ms() and ttl_ms() to raise the typed
SuperpositionError configuration error instead of raw ValueError when no
interval or TTL is configured. Use the existing
SuperpositionError.provider_error/config_error construction pattern from
errors.py so misconfigured strategies are consistently catchable by callers.
In `@clients/python/provider/test_http_data_source.py`:
- Around line 52-63: Update the _fetch_config helper to close the HttpDataSource
after fetch_config(None) completes, ensuring cleanup occurs even when fetching
raises; preserve the existing return value and asyncio.run flow.
In `@clients/python/provider/test_type_contract.py`:
- Around line 60-62: Update test_an_integer_widens_to_a_float to assert that
provider.resolve_float(...).value is a float, in addition to preserving the
existing 10000.0 value assertion.
🪄 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: 924050e3-868e-43de-b8a3-add27a873035
📒 Files selected for processing (15)
clients/python/provider-sdk-tests/main.pyclients/python/provider/superposition_provider/__init__.pyclients/python/provider/superposition_provider/cac_config.pyclients/python/provider/superposition_provider/data_source.pyclients/python/provider/superposition_provider/errors.pyclients/python/provider/superposition_provider/exp_config.pyclients/python/provider/superposition_provider/file_data_source.pyclients/python/provider/superposition_provider/http_data_source.pyclients/python/provider/superposition_provider/interfaces.pyclients/python/provider/superposition_provider/local_provider.pyclients/python/provider/superposition_provider/provider.pyclients/python/provider/superposition_provider/remote_provider.pyclients/python/provider/superposition_provider/types.pyclients/python/provider/test_http_data_source.pyclients/python/provider/test_type_contract.py
💤 Files with no reviewable changes (1)
- clients/python/provider/superposition_provider/provider.py
ayushjain17
force-pushed
the
python/openfeature
branch
3 times, most recently
from
July 15, 2026 03:25
f19acd9 to
762aec8
Compare
ayushjain17
force-pushed
the
python/openfeature
branch
from
July 15, 2026 08:34
762aec8 to
fc35432
Compare
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
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.
Change log
Improvements and fixes in python provider to make it more consistent with openfeature spec and other languages in parallel
Summary by CodeRabbit