fix: RemoteProvider cleanup#932
Conversation
|
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 PR introduces a new typed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 updates the Superposition API contract and SDKs to (1) move ApplicableVariants’s identifier into query params (and add an optional prefix filter), and (2) replace config: Document with a typed ConfigData shape for config version metadata responses. It also cleans up the Rust remote provider to call the identifier-based resolved-config endpoint and to persist the OpenFeature initialization context.
Changes:
ApplicableVariants:identifiermoved from request body to required query param; added optional repeatedprefixquery param.GetVersion/ListVersions:configchanged from untypedDocumentto structuredConfigDataacross models + generated SDKs/docs.- Rust provider updates: remote provider uses
get_resolved_config_with_identifierand threadsprefixthrough filtering APIs; stores global context on initialize.
Reviewed changes
Copilot reviewed 61 out of 61 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| smithy/models/experiments.smithy | Moves ApplicableVariantsInput.identifier to @httpQuery and adds prefix query list. |
| smithy/models/config.smithy | Introduces ConfigData; updates resource properties and version metadata to use structured config. |
| docs/docs/api/list-versions.api.mdx | Regenerates API doc blob for ListVersions after schema changes. |
| docs/docs/api/list-versions.StatusCodes.json | Updates response schema to show config as ConfigData. |
| docs/docs/api/get-version.api.mdx | Regenerates API doc blob for GetVersion after schema changes. |
| docs/docs/api/get-version.StatusCodes.json | Updates response schema to show config as ConfigData. |
| docs/docs/api/applicable-variants.api.mdx | Regenerates API doc blob reflecting query-based identifier and new prefix. |
| docs/docs/api/applicable-variants.RequestSchema.json | Removes identifier from request body schema (now query param). |
| docs/docs/api/applicable-variants.ParamsDetails.json | Adds identifier (required) and prefix (optional) query params. |
| docs/docs/api/Superposition.openapi.json | Updates OpenAPI: query params for ApplicableVariants, adds ConfigData schema, and swaps version metadata config to ConfigData. |
| crates/superposition_types/src/api/experiments.rs | Updates Rust API types: query now supports prefix; request body no longer includes identifier. |
| crates/superposition_sdk/src/types/builders.rs | Exposes ConfigDataBuilder in the Rust SDK builders module. |
| crates/superposition_sdk/src/types/_list_versions_member.rs | Switches ListVersionsMember.config from Document to ConfigData. |
| crates/superposition_sdk/src/types/_config_data.rs | Adds generated Rust SDK type ConfigData. |
| crates/superposition_sdk/src/types.rs | Re-exports ConfigData and wires in the new module. |
| crates/superposition_sdk/src/serde_util.rs | Adds error-correction defaults for ConfigData and updates version shapes to use it. |
| crates/superposition_sdk/src/protocol_serde/shape_list_versions_member.rs | Deserializes config as ConfigData instead of Document. |
| crates/superposition_sdk/src/protocol_serde/shape_get_version.rs | Deserializes config as ConfigData instead of Document. |
| crates/superposition_sdk/src/protocol_serde/shape_config_data.rs | Adds generated deserializer for ConfigData. |
| crates/superposition_sdk/src/protocol_serde/shape_applicable_variants_input.rs | Stops serializing identifier in the request body. |
| crates/superposition_sdk/src/protocol_serde.rs | Registers shape_config_data module for protocol serde. |
| crates/superposition_sdk/src/operation/get_version/_get_version_output.rs | Switches GetVersionOutput.config from Document to ConfigData. |
| crates/superposition_sdk/src/operation/applicable_variants/builders.rs | Adds fluent builder methods for prefix. |
| crates/superposition_sdk/src/operation/applicable_variants/_applicable_variants_input.rs | Adds prefix to operation input type and builder. |
| crates/superposition_sdk/src/operation/applicable_variants.rs | Adds query-string serialization for identifier (required) and prefix. |
| crates/superposition_sdk/src/client/get_version.rs | Updates client docs to reference ConfigData in the output. |
| crates/superposition_sdk/src/client/applicable_variants.rs | Updates client docs to include optional prefix input. |
| crates/superposition_provider/src/traits.rs | Extends provider traits to accept an owned prefix_filter vector and adds it to applicable-variants meta API. |
| crates/superposition_provider/src/remote_provider.rs | Removes response caching; switches to identifier-based resolved-config endpoint; threads prefix and stores global context on initialize. |
| crates/superposition_provider/src/local_provider.rs | Propagates the new prefix_filter type/signature through local provider evaluation and variant selection. |
| crates/superposition_provider/src/conversions.rs | Adds EvaluationContextFieldValue -> Document conversion and a query_document helper for SDK calls. |
| crates/superposition_provider/examples/local_http_example.rs | Updates example to pass the new prefix_filter argument. |
| crates/superposition/src/resolve/handlers.rs | Plumbs prefix filter through to experiment variant resolution helper. |
| crates/experimentation_platform/src/api/experiments/handlers.rs | Adds server-side prefix filtering for applicable variants and switches handler to query-based identifier. |
| clients/python/sdk/superposition_sdk/serialize.py | Adds query param serialization for identifier and prefix on ApplicableVariants. |
| clients/python/sdk/superposition_sdk/models.py | Introduces ConfigData model; updates version outputs to use it; updates applicable variants input to include prefix and omit identifier from body. |
| clients/python/sdk/superposition_sdk/_private/schemas.py | Updates Smithy Python schemas for query-based identifier/prefix and adds ConfigData shape. |
| clients/javascript/sdk/src/protocols/Aws_restJson1.ts | Moves ApplicableVariants.identifier to query, adds prefix query, and deserializes version configs as ConfigData. |
| clients/javascript/sdk/src/models/models_0.ts | Adds prefix to applicable variants input; introduces ConfigData; updates version response members. |
| clients/javascript/sdk/src/commands/ListVersionsCommand.ts | Updates JSDoc example to show structured ConfigData in config. |
| clients/javascript/sdk/src/commands/GetVersionCommand.ts | Updates JSDoc example to show structured ConfigData in config. |
| clients/javascript/sdk/src/commands/ApplicableVariantsCommand.ts | Updates JSDoc example to include prefix. |
| clients/java/sdk/src/main/java/io/juspay/superposition/model/SharedSerde.java | Reorders/adjusts serde helpers after new/relocated shapes (incl. context map + dependency graph). |
| clients/java/sdk/src/main/java/io/juspay/superposition/model/SharedSchemas.java | Reorders schema declarations and wires in CONTEXT_MAP placement changes. |
| clients/java/sdk/src/main/java/io/juspay/superposition/model/ListVersionsMember.java | Switches config from Document to ConfigData and updates serde. |
| clients/java/sdk/src/main/java/io/juspay/superposition/model/GetVersionOutput.java | Switches config from Document to ConfigData and updates serde. |
| clients/java/sdk/src/main/java/io/juspay/superposition/model/GetResolvedConfigWithIdentifierOutput.java | Trait order adjustment for payload/required on config. |
| clients/java/sdk/src/main/java/io/juspay/superposition/model/GetResolvedConfigOutput.java | Trait order adjustment for payload/required on config. |
| clients/java/sdk/src/main/java/io/juspay/superposition/model/ConfigVersion.java | Updates config property schema to ConfigData. |
| clients/java/sdk/src/main/java/io/juspay/superposition/model/ConfigData.java | Adds generated Java model ConfigData. |
| clients/java/sdk/src/main/java/io/juspay/superposition/model/Config.java | Updates resource properties to include structured config fields instead of a single config document. |
| clients/java/sdk/src/main/java/io/juspay/superposition/model/ApplicableVariantsInput.java | Adds prefix and marks identifier as HttpQuery; updates serialization accordingly. |
| clients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionClientImpl.java | Reorders type registry entries (no semantic change). |
| clients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionClient.java | Reorders auth scheme declarations (no semantic change). |
| clients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionAsyncClientImpl.java | Reorders type registry entries (no semantic change). |
| clients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionAsyncClient.java | Reorders auth scheme declarations (no semantic change). |
| clients/haskell/sdk/SuperpositionSDK.cabal | Exposes the new ConfigData module and adjusts export ordering. |
| clients/haskell/sdk/Io/Superposition/Model/ListVersionsMember.hs | Switches config from Aeson.Value to ConfigData. |
| clients/haskell/sdk/Io/Superposition/Model/GetVersionOutput.hs | Switches config from Aeson.Value to ConfigData. |
| clients/haskell/sdk/Io/Superposition/Model/ConfigData.hs | Adds generated Haskell model ConfigData. |
| clients/haskell/sdk/Io/Superposition/Model/ApplicableVariantsInput.hs | Adds prefix and serializes identifier/prefix as query params rather than body fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let context = if req.method() == actix_web::http::Method::GET { | ||
| dimension_params.into_inner() | ||
| } else { | ||
| req_body.map_or_else(QueryMap::default, |body| body.into_inner().context.into()) | ||
| }; |
There was a problem hiding this comment.
For POST requests, ApplicableVariantsInput’s context is marked @required in the Smithy model, but this handler currently allows req_body to be None and silently uses an empty default context. Consider returning a bad argument error when req.method() != GET and req_body is missing (or make the extractor non-optional) to keep the API contract consistent.
| let (query_data, targeting_key) = | ||
| conversions::evaluation_context_to_query_document(context); | ||
|
|
||
| // 4. Build and send the get_resolved_config request | ||
| // Always fetch WITHOUT prefix filter so we can cache the full result | ||
| let mut builder = client | ||
| .get_resolved_config() | ||
| let response = self | ||
| .client | ||
| .get_resolved_config_with_identifier() | ||
| .workspace_id(&self.options.workspace_id) | ||
| .org_id(&self.options.org_id); | ||
|
|
||
| // Set context dimensions from evaluation context | ||
| let sdk_context: HashMap<String, Document> = query_data | ||
| .into_iter() | ||
| .map(|(k, v)| (k, conversions::value_to_document(v))) | ||
| .collect(); | ||
| builder = builder.set_context(Some(sdk_context)); | ||
|
|
||
| // NOTE: We intentionally do NOT set prefix filter on the SDK request | ||
| // so we always get the full config and can cache it. Prefix filtering | ||
| // is applied locally after caching. | ||
|
|
||
| let response = builder.send().await.map_err(|e| { | ||
| SuperpositionError::NetworkError(format!( | ||
| "Failed to get resolved config: {}", | ||
| e | ||
| )) | ||
| })?; | ||
| .org_id(&self.options.org_id) | ||
| .set_context(Some(query_data)) | ||
| .set_identifier(targeting_key) | ||
| .set_prefix(prefix_filter) | ||
| .send() |
There was a problem hiding this comment.
get_resolved_config_with_identifier requires a non-empty identifier, but this code forwards targeting_key: Option<String> directly into set_identifier(...). If the incoming/global EvaluationContext lacks a targeting key, the request will fail at build time and be surfaced as a NetworkError. Consider validating targeting_key (Some + non-empty) up front and returning a more appropriate error (or deriving a fallback identifier) before calling .send().
There was a problem hiding this comment.
please re-check
| let (query_data, targeting_key) = | ||
| conversions::evaluation_context_to_query_document(context); | ||
|
|
||
| let applicable_variants = self | ||
| .client | ||
| .applicable_variants() | ||
| .workspace_id(&self.options.workspace_id) | ||
| .org_id(&self.options.org_id) | ||
| .set_context(Some(query_data)) | ||
| .set_identifier(targeting_key) | ||
| .set_prefix(prefix_filter) | ||
| .send() |
There was a problem hiding this comment.
Same issue here: .set_identifier(targeting_key) forwards an Option<String> into an operation where identifier is required/non-empty. If targeting_key is missing, this becomes a build-time failure but is mapped into a NetworkError. Validate targeting_key earlier and return a clearer error (or require callers to provide it) before sending the request.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ayushjain17
force-pushed
the
fix/remote-provider
branch
from
f7d27ba to
abca9bc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
clients/java/sdk/src/main/java/io/juspay/superposition/model/GetVersionOutput.java (1)
283-285:⚠️ Potential issue | 🟠 Major
errorCorrection()marksconfigpresent without creating it.Line 211 makes
confignon-nullable, but Line 284 only flips the presence tracker. AftererrorCorrection().build(),GetVersionOutputcan still carryconfig == null, and Line 128 then quietly omits a required field during serialization. Populate a correctedConfigDatainstance here instead of only setting the tracker.🛠 Suggested fix
if (!tracker.checkMember($SCHEMA_CONFIG)) { - tracker.setMember($SCHEMA_CONFIG); + config(ConfigData.builder().errorCorrection().build()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/java/sdk/src/main/java/io/juspay/superposition/model/GetVersionOutput.java` around lines 283 - 285, In errorCorrection() for GetVersionOutput, the code currently flips the presence tracker via tracker.setMember($SCHEMA_CONFIG) without actually populating the config field, leaving config nullable despite being marked non-nullable; update errorCorrection() to create and assign a default ConfigData instance (e.g., new ConfigData(...) or ConfigData.builder().build()) to the config member before calling tracker.setMember($SCHEMA_CONFIG) so that after errorCorrection().build() the config field is non-null and will be serialized correctly.docs/docs/api/Superposition.openapi.json (2)
2604-2613:⚠️ Potential issue | 🟠 MajorMake the POST body optional in the spec.
Line 2612 still marks
requestBodyas required, but the handler acceptsOption<Json<ApplicableVariantsRequest>>and falls back to an empty context when no body is sent. After movingidentifierto the query string, that makes generated clients stricter than the server for valid query-only calls.💡 Suggested change
"requestBody": { "content": { "application/json": { "schema": { "$ref": "#/components/schemas/ApplicableVariantsRequestContent" } } - }, - "required": true + } },Also applies to: 6118-6127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/api/Superposition.openapi.json` around lines 2604 - 2613, The OpenAPI spec marks the POST operation's requestBody as required but the server handler accepts Option<Json<ApplicableVariantsRequest>> and treats an absent body as an empty context; update the operation(s) (the requestBody for the ApplicableVariantsRequestContent schema) to set "required": false (or remove the required flag) so generated clients may omit the body when using query-only calls; search for occurrences of ApplicableVariantsRequestContent and the requestBody block and make the change consistently (also apply the same fix to the other occurrence around the ApplicableVariantsRequestContent reference at the later location).
2600-2677:⚠️ Potential issue | 🟡 MinorDocument the
GETform of/experiments/applicable-variants.Line 2601 only exposes
post, whilecrates/experimentation_platform/src/api/experiments/handlers.rs:970-1000still registers bothGETandPOSTfor this path. If the GET route is public, it should be described here too; otherwise the published docs and generated clients will continue to hide a supported entry point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/api/Superposition.openapi.json` around lines 2600 - 2677, The OpenAPI spec currently documents only the POST for "/experiments/applicable-variants" (operationId "ApplicableVariants") while the Rust handlers (crates/experimentation_platform/src/api/experiments/handlers.rs) register both GET and POST; add a GET operation entry for "/experiments/applicable-variants" that mirrors the POST (same parameters: query "identifier", query "prefix" array, headers "x-org-id" and "x-workspace"; same request/response schemas or if GET is truly query-only, move the request body into query schema accordingly) and include the same 200 and 500 responses so the public docs and generated clients expose the GET endpoint used by the ApplicableVariants handler.
🧹 Nitpick comments (4)
crates/superposition_provider/src/conversions.rs (1)
119-145: Keep remote context normalization aligned with the local path.
evaluation_context_to_valuealready sanitizesNaN/±Inf, but this newDocumentpath forwards them unchanged. The sameEvaluationContextcan now be normalized differently by local vs remote providers.♻️ Suggested simplification
pub fn evaluation_context_to_document(value: EvaluationContextFieldValue) -> Document { - match value { - EvaluationContextFieldValue::Bool(b) => Document::Bool(b), - EvaluationContextFieldValue::Int(i) => { - Document::Number(aws_smithy_types::Number::NegInt(i)) - } - EvaluationContextFieldValue::Float(f) => { - Document::Number(aws_smithy_types::Number::Float(f)) - } - EvaluationContextFieldValue::String(s) => Document::String(s), - EvaluationContextFieldValue::DateTime(dt) => Document::String(dt.to_string()), - EvaluationContextFieldValue::Struct(s) => { - let struct_map = s - .downcast_ref::<HashMap<String, EvaluationContextFieldValue>>() - .map(|m| { - m.iter() - .map(|(k, v)| (k.clone(), evaluation_context_to_document(v.clone()))) - .collect() - }) - .unwrap_or_else(|| { - log::warn!("Failed to downcast struct value to expected HashMap format, got {:?}. Returning empty object.", s.type_id()); - HashMap::new() - }); - Document::Object(struct_map) - } - } + value_to_document(evaluation_context_to_value(value)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_provider/src/conversions.rs` around lines 119 - 145, The Document path (evaluation_context_to_document) is not normalizing Float values the way evaluation_context_to_value does, causing inconsistent normalization for NaN/±Inf; update evaluation_context_to_document so floats are sanitized the same way as evaluation_context_to_value (either call/evaluate via evaluation_context_to_value then convert that Value to a Document, or replicate the exact NaN/±Inf normalization logic used in evaluation_context_to_value) for EvaluationContextFieldValue::Float (and any numeric Document::Number conversion) so remote and local providers produce identical normalized representations.docs/docs/api/Superposition.openapi.json (1)
6350-6375: ReuseConfigDataforGetConfigResponseContenttoo.
GetConfigResponseContentstill duplicates this schema field-for-field. Pointing it atConfigDatawould remove a second copy of the same contract and reduce the chance that live-config and versioned-config payloads drift apart later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/api/Superposition.openapi.json` around lines 6350 - 6375, Get rid of the duplicated schema by updating the OpenAPI component that defines GetConfigResponseContent to reference the existing ConfigData schema instead of repeating its fields; locate the GetConfigResponseContent component and replace its inline object definition with a $ref to "#/components/schemas/ConfigData" so both live-config and versioned-config use the single ConfigData contract (symbols to edit: ConfigData, GetConfigResponseContent).clients/python/sdk/superposition_sdk/models.py (2)
2553-2569: Duplicate implementation of_serialize_object/_deserialize_object.
_serialize_context_mapand_deserialize_context_mapare identical to_serialize_objectand_deserialize_objectdefined at lines 2455-2471. Consider reusing the existing functions to avoid duplication.♻️ Proposed fix to remove duplication
-def _serialize_context_map(serializer: ShapeSerializer, schema: Schema, value: dict[str, Document]) -> None: - with serializer.begin_map(schema, len(value)) as m: - value_schema = schema.members["value"] - for k, v in value.items(): - m.entry(k, lambda vs: vs.write_document(value_schema, v)) - -def _deserialize_context_map(deserializer: ShapeDeserializer, schema: Schema) -> dict[str, Document]: - result: dict[str, Document] = {} - value_schema = schema.members["value"] - def _read_value(k: str, d: ShapeDeserializer): - if d.is_null(): - d.read_null() - - else: - result[k] = d.read_document(value_schema) - deserializer.read_map(schema, _read_value) - return resultThen replace usages of
_serialize_context_map/_deserialize_context_mapwith_serialize_object/_deserialize_object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/sdk/superposition_sdk/models.py` around lines 2553 - 2569, The two functions _serialize_context_map and _deserialize_context_map duplicate logic already implemented in _serialize_object and _deserialize_object; remove the duplicate functions, update any references to call _serialize_object and _deserialize_object instead (ensure the same parameter ordering and types are preserved when replacing calls), and run tests to confirm behavior; if any schema/member names differ, adapt the call by passing the same schema.members["value"] and value dict to _serialize_object/_deserialize_object so behavior remains identical.
2455-2471: Consider using default argument to capture loop variable in lambda.The lambda
lambda vs: vs.write_document(value_schema, v)capturesvby reference. While this typically works becausem.entrycalls the lambda immediately, the idiomatic approach to avoid late-binding closure issues is to capture by value using a default argument.♻️ Proposed defensive fix
for k, v in value.items(): - m.entry(k, lambda vs: vs.write_document(value_schema, v)) + m.entry(k, lambda vs, v=v: vs.write_document(value_schema, v))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/sdk/superposition_sdk/models.py` around lines 2455 - 2471, In _serialize_object, the inline lambda passed to m.entry currently closes over the loop variable v which can cause late-binding issues; change the lambda in function _serialize_object (used with ShapeSerializer.m.entry) to capture v by value via a default argument (e.g., lambda vs, v=v: ...) so each entry writes the correct document value immediately when invoked.
🤖 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/java/sdk/src/main/java/io/juspay/superposition/model/ConfigData.java`:
- Around line 46-50: The constructor in ConfigData currently stores unmodifiable
views of the Builder's collections but does not defensively copy them, so
subsequent mutations of builder.contexts, builder.defaultConfigs,
builder.dimensions, or the maps inside builder.overrides can mutate the supposed
immutable model and break equals()/hashCode(). Fix by making defensive copies
before wrapping: e.g., copy builder.contexts into a new List and wrap with
Collections.unmodifiableList, copy builder.defaultConfigs and builder.dimensions
into new Maps and wrap with Collections.unmodifiableMap, and for overrides
create a new Map whose values are copies of each inner Map (and make those inner
maps unmodifiable) before wrapping the outer map as unmodifiable; update the
ConfigData(Builder builder) assignments for contexts, overrides, defaultConfigs,
and dimensions accordingly (referencing ConfigData constructor, fields
contexts/overrides/defaultConfigs/dimensions and
builder.contexts/builder.overrides).
In `@clients/javascript/sdk/src/protocols/Aws_restJson1.ts`:
- Around line 5512-5525: de_ConfigData currently calls deserializers (de_Object,
de_OverridesMap, de_ContextList, de_DimensionData) that drop explicit null map
entries, causing stored nulls (e.g. default_configs.foo = null) to disappear;
update de_ConfigData to use null-preserving deserializers instead: replace the
de_Object call for 'default_configs' with a null-preserving document map
deserializer (e.g. de_ObjectPreserveNull), replace the 'overrides' call with a
null-preserving overrides map deserializer (e.g. de_OverridesMapPreserveNull),
and ensure de_ContextList and de_DimensionData use/compose null-preserving
variants for contexts[*].condition and dimensions[*].schema (or implement
helpers de_ContextListPreserveNull and de_DimensionDataPreserveNull if needed)
so explicit null entries are retained through GetVersion/ListVersions responses.
In `@crates/experimentation_platform/src/api/experiments/handlers.rs`:
- Around line 995-996: The handler is passing query_data.prefix directly into
get_applicable_variants_helper which allows unbounded user-controlled lists to
be copied and turned into a HashSet; instead, validate and bind a sanitized
prefix before passing it: inspect query_data.prefix, enforce a maximum number of
entries and maximum element byte/char length (or reject oversized requests),
truncate elements if desired, and build a new Vec/String collection (e.g., a
sanitized_prefix variable) to pass instead of prefix.map(|p| p.0); ensure this
validation occurs in the same scope that constructs the arguments for
get_applicable_variants_helper and use the sanitized identifier/prefix variables
when calling the helper.
- Around line 983-987: The GET branch builds context from
DimensionQuery<QueryMap> which currently folds control params like "identifier",
"toss" and "prefix" into the evaluation context and alters bucketing; change the
GET branch so after calling dimension_params.into_inner() you explicitly remove
those control query keys (e.g. filter out "identifier", "toss", "prefix" from
the returned QueryMap) before assigning to context, keeping the non-GET req_body
handling (req_body.map_or_else(...)) unchanged; reference DimensionQuery,
QueryMap, the context variable, and the req.method() check to locate where to
perform the filtering.
In `@crates/superposition_provider/src/remote_provider.rs`:
- Around line 119-130: Short-circuit when the targeting_key returned by
conversions::evaluation_context_to_query_document(context) is None instead of
calling .set_identifier(None) on the request: check the targeting_key Option
before constructing the self.client.applicable_variants() request, and if it's
None return the appropriate local outcome (e.g. an early Ok result for no
applicable variants or a clear domain-level error) so you never send a request
with set_identifier(None) that would produce a remote 400/NetworkError.
In `@smithy/models/config.smithy`:
- Around line 79-91: The schema incorrectly marks ConfigData members as required
(contexts, overrides, default_configs, dimensions) while the Rust SDK still
auto-fills missing fields via config_data_correct_errors in serde_util.rs before
ConfigDataBuilder::build(); remove the `@required` annotations from those four
members in the ConfigData structure so the model no longer treats them as
mandatory and avoids masking missing server fields, or alternatively ensure the
SDK stops auto-filling before build (adjust config_data_correct_errors) so
missing fields are detected.
In `@smithy/models/experiments.smithy`:
- Around line 224-231: The change binds identifier to `@httpQuery` which breaks
the POST /experiments/applicable-variants contract; either revert the
`@httpQuery`("identifier") annotation on the identifier shape so it remains a body
field, or implement dual-support on the server: keep the identifier field in the
request body shape (remove the `@httpQuery`) and update the Rust handler
(experiments handlers.rs) and the deserializer
(shape_applicable_variants_input.rs) to accept identifier from query params
first and fall back to the request body if absent (add tests and a deprecation
log when query-supplied is used) so existing clients remain compatible during
rollout.
---
Outside diff comments:
In
`@clients/java/sdk/src/main/java/io/juspay/superposition/model/GetVersionOutput.java`:
- Around line 283-285: In errorCorrection() for GetVersionOutput, the code
currently flips the presence tracker via tracker.setMember($SCHEMA_CONFIG)
without actually populating the config field, leaving config nullable despite
being marked non-nullable; update errorCorrection() to create and assign a
default ConfigData instance (e.g., new ConfigData(...) or
ConfigData.builder().build()) to the config member before calling
tracker.setMember($SCHEMA_CONFIG) so that after errorCorrection().build() the
config field is non-null and will be serialized correctly.
In `@docs/docs/api/Superposition.openapi.json`:
- Around line 2604-2613: The OpenAPI spec marks the POST operation's requestBody
as required but the server handler accepts
Option<Json<ApplicableVariantsRequest>> and treats an absent body as an empty
context; update the operation(s) (the requestBody for the
ApplicableVariantsRequestContent schema) to set "required": false (or remove the
required flag) so generated clients may omit the body when using query-only
calls; search for occurrences of ApplicableVariantsRequestContent and the
requestBody block and make the change consistently (also apply the same fix to
the other occurrence around the ApplicableVariantsRequestContent reference at
the later location).
- Around line 2600-2677: The OpenAPI spec currently documents only the POST for
"/experiments/applicable-variants" (operationId "ApplicableVariants") while the
Rust handlers (crates/experimentation_platform/src/api/experiments/handlers.rs)
register both GET and POST; add a GET operation entry for
"/experiments/applicable-variants" that mirrors the POST (same parameters: query
"identifier", query "prefix" array, headers "x-org-id" and "x-workspace"; same
request/response schemas or if GET is truly query-only, move the request body
into query schema accordingly) and include the same 200 and 500 responses so the
public docs and generated clients expose the GET endpoint used by the
ApplicableVariants handler.
---
Nitpick comments:
In `@clients/python/sdk/superposition_sdk/models.py`:
- Around line 2553-2569: The two functions _serialize_context_map and
_deserialize_context_map duplicate logic already implemented in
_serialize_object and _deserialize_object; remove the duplicate functions,
update any references to call _serialize_object and _deserialize_object instead
(ensure the same parameter ordering and types are preserved when replacing
calls), and run tests to confirm behavior; if any schema/member names differ,
adapt the call by passing the same schema.members["value"] and value dict to
_serialize_object/_deserialize_object so behavior remains identical.
- Around line 2455-2471: In _serialize_object, the inline lambda passed to
m.entry currently closes over the loop variable v which can cause late-binding
issues; change the lambda in function _serialize_object (used with
ShapeSerializer.m.entry) to capture v by value via a default argument (e.g.,
lambda vs, v=v: ...) so each entry writes the correct document value immediately
when invoked.
In `@crates/superposition_provider/src/conversions.rs`:
- Around line 119-145: The Document path (evaluation_context_to_document) is not
normalizing Float values the way evaluation_context_to_value does, causing
inconsistent normalization for NaN/±Inf; update evaluation_context_to_document
so floats are sanitized the same way as evaluation_context_to_value (either
call/evaluate via evaluation_context_to_value then convert that Value to a
Document, or replicate the exact NaN/±Inf normalization logic used in
evaluation_context_to_value) for EvaluationContextFieldValue::Float (and any
numeric Document::Number conversion) so remote and local providers produce
identical normalized representations.
In `@docs/docs/api/Superposition.openapi.json`:
- Around line 6350-6375: Get rid of the duplicated schema by updating the
OpenAPI component that defines GetConfigResponseContent to reference the
existing ConfigData schema instead of repeating its fields; locate the
GetConfigResponseContent component and replace its inline object definition with
a $ref to "#/components/schemas/ConfigData" so both live-config and
versioned-config use the single ConfigData contract (symbols to edit:
ConfigData, GetConfigResponseContent).
🪄 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: 5b27bc07-444c-48f7-acf1-81777febe411
📒 Files selected for processing (61)
clients/haskell/sdk/Io/Superposition/Model/ApplicableVariantsInput.hsclients/haskell/sdk/Io/Superposition/Model/ConfigData.hsclients/haskell/sdk/Io/Superposition/Model/GetVersionOutput.hsclients/haskell/sdk/Io/Superposition/Model/ListVersionsMember.hsclients/haskell/sdk/SuperpositionSDK.cabalclients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionAsyncClient.javaclients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionAsyncClientImpl.javaclients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionClient.javaclients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionClientImpl.javaclients/java/sdk/src/main/java/io/juspay/superposition/model/ApplicableVariantsInput.javaclients/java/sdk/src/main/java/io/juspay/superposition/model/Config.javaclients/java/sdk/src/main/java/io/juspay/superposition/model/ConfigData.javaclients/java/sdk/src/main/java/io/juspay/superposition/model/ConfigVersion.javaclients/java/sdk/src/main/java/io/juspay/superposition/model/GetResolvedConfigOutput.javaclients/java/sdk/src/main/java/io/juspay/superposition/model/GetResolvedConfigWithIdentifierOutput.javaclients/java/sdk/src/main/java/io/juspay/superposition/model/GetVersionOutput.javaclients/java/sdk/src/main/java/io/juspay/superposition/model/ListVersionsMember.javaclients/java/sdk/src/main/java/io/juspay/superposition/model/SharedSchemas.javaclients/java/sdk/src/main/java/io/juspay/superposition/model/SharedSerde.javaclients/javascript/sdk/src/commands/ApplicableVariantsCommand.tsclients/javascript/sdk/src/commands/GetVersionCommand.tsclients/javascript/sdk/src/commands/ListVersionsCommand.tsclients/javascript/sdk/src/models/models_0.tsclients/javascript/sdk/src/protocols/Aws_restJson1.tsclients/python/sdk/superposition_sdk/_private/schemas.pyclients/python/sdk/superposition_sdk/models.pyclients/python/sdk/superposition_sdk/serialize.pycrates/experimentation_platform/src/api/experiments/handlers.rscrates/superposition/src/resolve/handlers.rscrates/superposition_provider/examples/local_http_example.rscrates/superposition_provider/src/conversions.rscrates/superposition_provider/src/local_provider.rscrates/superposition_provider/src/remote_provider.rscrates/superposition_provider/src/traits.rscrates/superposition_sdk/src/client/applicable_variants.rscrates/superposition_sdk/src/client/get_version.rscrates/superposition_sdk/src/operation/applicable_variants.rscrates/superposition_sdk/src/operation/applicable_variants/_applicable_variants_input.rscrates/superposition_sdk/src/operation/applicable_variants/builders.rscrates/superposition_sdk/src/operation/get_version/_get_version_output.rscrates/superposition_sdk/src/protocol_serde.rscrates/superposition_sdk/src/protocol_serde/shape_applicable_variants_input.rscrates/superposition_sdk/src/protocol_serde/shape_config_data.rscrates/superposition_sdk/src/protocol_serde/shape_get_version.rscrates/superposition_sdk/src/protocol_serde/shape_list_versions_member.rscrates/superposition_sdk/src/serde_util.rscrates/superposition_sdk/src/types.rscrates/superposition_sdk/src/types/_config_data.rscrates/superposition_sdk/src/types/_list_versions_member.rscrates/superposition_sdk/src/types/builders.rscrates/superposition_types/src/api/experiments.rsdocs/docs/api/Superposition.openapi.jsondocs/docs/api/applicable-variants.ParamsDetails.jsondocs/docs/api/applicable-variants.RequestSchema.jsondocs/docs/api/applicable-variants.api.mdxdocs/docs/api/get-version.StatusCodes.jsondocs/docs/api/get-version.api.mdxdocs/docs/api/list-versions.StatusCodes.jsondocs/docs/api/list-versions.api.mdxsmithy/models/config.smithysmithy/models/experiments.smithy
💤 Files with no reviewable changes (1)
- crates/superposition_sdk/src/protocol_serde/shape_applicable_variants_input.rs
| private ConfigData(Builder builder) { | ||
| this.contexts = Collections.unmodifiableList(builder.contexts); | ||
| this.overrides = Collections.unmodifiableMap(builder.overrides); | ||
| this.defaultConfigs = Collections.unmodifiableMap(builder.defaultConfigs); | ||
| this.dimensions = Collections.unmodifiableMap(builder.dimensions); |
There was a problem hiding this comment.
Copy builder-owned collections before storing them.
Collections.unmodifiableList/Map only protects the returned view. Because the builder keeps caller-owned references, mutating the original collections after build() mutates this supposedly immutable SDK model and can invalidate equals/hashCode; overrides also needs its inner maps copied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/java/sdk/src/main/java/io/juspay/superposition/model/ConfigData.java`
around lines 46 - 50, The constructor in ConfigData currently stores
unmodifiable views of the Builder's collections but does not defensively copy
them, so subsequent mutations of builder.contexts, builder.defaultConfigs,
builder.dimensions, or the maps inside builder.overrides can mutate the supposed
immutable model and break equals()/hashCode(). Fix by making defensive copies
before wrapping: e.g., copy builder.contexts into a new List and wrap with
Collections.unmodifiableList, copy builder.defaultConfigs and builder.dimensions
into new Maps and wrap with Collections.unmodifiableMap, and for overrides
create a new Map whose values are copies of each inner Map (and make those inner
maps unmodifiable) before wrapping the outer map as unmodifiable; update the
ConfigData(Builder builder) assignments for contexts, overrides, defaultConfigs,
and dimensions accordingly (referencing ConfigData constructor, fields
contexts/overrides/defaultConfigs/dimensions and
builder.contexts/builder.overrides).
| /** | ||
| * deserializeAws_restJson1ConfigData | ||
| */ | ||
| const de_ConfigData = ( | ||
| output: any, | ||
| context: __SerdeContext | ||
| ): ConfigData => { | ||
| return take(output, { | ||
| 'contexts': (_: any) => de_ContextList(_, context), | ||
| 'default_configs': (_: any) => de_Object(_, context), | ||
| 'dimensions': (_: any) => de_DimensionData(_, context), | ||
| 'overrides': (_: any) => de_OverridesMap(_, context), | ||
| }) as any; | ||
| } |
There was a problem hiding this comment.
Explicit null config entries are lost during deserialization.
Once version payloads are funneled through de_ConfigData, default_configs, overrides, contexts[*].condition, and dimensions[*].schema all inherit helpers that drop null entries (see Line 5504, Line 5904, and Line 5956). A stored value like default_configs.foo = null will come back as if foo were absent, which changes the payload shape for GetVersion and ListVersions. Use null-preserving deserializers for these document-valued maps before routing version responses through de_ConfigData.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/javascript/sdk/src/protocols/Aws_restJson1.ts` around lines 5512 -
5525, de_ConfigData currently calls deserializers (de_Object, de_OverridesMap,
de_ContextList, de_DimensionData) that drop explicit null map entries, causing
stored nulls (e.g. default_configs.foo = null) to disappear; update
de_ConfigData to use null-preserving deserializers instead: replace the
de_Object call for 'default_configs' with a null-preserving document map
deserializer (e.g. de_ObjectPreserveNull), replace the 'overrides' call with a
null-preserving overrides map deserializer (e.g. de_OverridesMapPreserveNull),
and ensure de_ContextList and de_DimensionData use/compose null-preserving
variants for contexts[*].condition and dimensions[*].schema (or implement
helpers de_ContextListPreserveNull and de_DimensionDataPreserveNull if needed)
so explicit null entries are retained through GetVersion/ListVersions responses.
| let context = if req.method() == actix_web::http::Method::GET { | ||
| dimension_params.into_inner() | ||
| } else { | ||
| req_body.map_or_else(QueryMap::default, |body| body.into_inner().context.into()) | ||
| }; |
There was a problem hiding this comment.
Strip control query params out of the GET evaluation context.
DimensionQuery<QueryMap> reads the full query string, so identifier/toss and prefix now get folded into context on GET requests. That changes cohort evaluation and bucketing whenever the new query parameters are present.
Suggested fix
- let context = if req.method() == actix_web::http::Method::GET {
- dimension_params.into_inner()
+ let context = if req.method() == actix_web::http::Method::GET {
+ let mut context = dimension_params.into_inner();
+ context.remove("identifier");
+ context.remove("toss");
+ context.remove("prefix");
+ context
} else {
req_body.map_or_else(QueryMap::default, |body| body.into_inner().context.into())
};📝 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.
| let context = if req.method() == actix_web::http::Method::GET { | |
| dimension_params.into_inner() | |
| } else { | |
| req_body.map_or_else(QueryMap::default, |body| body.into_inner().context.into()) | |
| }; | |
| let context = if req.method() == actix_web::http::Method::GET { | |
| let mut context = dimension_params.into_inner(); | |
| context.remove("identifier"); | |
| context.remove("toss"); | |
| context.remove("prefix"); | |
| context | |
| } else { | |
| req_body.map_or_else(QueryMap::default, |body| body.into_inner().context.into()) | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/experimentation_platform/src/api/experiments/handlers.rs` around lines
983 - 987, The GET branch builds context from DimensionQuery<QueryMap> which
currently folds control params like "identifier", "toss" and "prefix" into the
evaluation context and alters bucketing; change the GET branch so after calling
dimension_params.into_inner() you explicitly remove those control query keys
(e.g. filter out "identifier", "toss", "prefix" from the returned QueryMap)
before assigning to context, keeping the non-GET req_body handling
(req_body.map_or_else(...)) unchanged; reference DimensionQuery, QueryMap, the
context variable, and the req.method() check to locate where to perform the
filtering.
| query_data.identifier, | ||
| query_data.prefix.map(|p| p.0), |
There was a problem hiding this comment.
Bound prefix before passing it through.
This list is fully user-controlled and is copied again into a HashSet in get_applicable_variants_helper. Without a length and element-size cap, one request can force large allocations and expensive prefix filtering work.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 996-996: Uncontrolled allocation size
This allocation size is derived from a user-provided value and could allocate arbitrary amounts of memory.
This allocation size is derived from a user-provided value and could allocate arbitrary amounts of memory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/experimentation_platform/src/api/experiments/handlers.rs` around lines
995 - 996, The handler is passing query_data.prefix directly into
get_applicable_variants_helper which allows unbounded user-controlled lists to
be copied and turned into a HashSet; instead, validate and bind a sanitized
prefix before passing it: inspect query_data.prefix, enforce a maximum number of
entries and maximum element byte/char length (or reject oversized requests),
truncate elements if desired, and build a new Vec/String collection (e.g., a
sanitized_prefix variable) to pass instead of prefix.map(|p| p.0); ensure this
validation occurs in the same scope that constructs the arguments for
get_applicable_variants_helper and use the sanitized identifier/prefix variables
when calling the helper.
| let (query_data, targeting_key) = | ||
| conversions::evaluation_context_to_query_document(context); | ||
|
|
||
| let applicable_variants = self | ||
| .client | ||
| .applicable_variants() | ||
| .workspace_id(&self.options.workspace_id) | ||
| .org_id(&self.options.org_id) | ||
| .set_context(Some(query_data)) | ||
| .set_identifier(targeting_key) | ||
| .set_prefix(prefix_filter) | ||
| .send() |
There was a problem hiding this comment.
Handle a missing targeting key locally.
The server-side ApplicableVariantsQuery.identifier is required, so forwarding targeting_key through set_identifier(...) when it is None can only fail after a network hop. Short-circuit that case with a clear local outcome instead of surfacing a remote 400 as NetworkError.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition_provider/src/remote_provider.rs` around lines 119 - 130,
Short-circuit when the targeting_key returned by
conversions::evaluation_context_to_query_document(context) is None instead of
calling .set_identifier(None) on the request: check the targeting_key Option
before constructing the self.client.applicable_variants() request, and if it's
None return the appropriate local outcome (e.g. an early Ok result for no
applicable variants or a clear domain-level error) so you never send a request
with set_identifier(None) that would produce a remote 400/NetworkError.
| structure ConfigData { | ||
| @required | ||
| contexts: ContextList | ||
|
|
||
| @required | ||
| overrides: OverridesMap | ||
|
|
||
| @required | ||
| default_configs: Object | ||
|
|
||
| @required | ||
| dimensions: DimensionData | ||
| } |
There was a problem hiding this comment.
Don’t mark these members required if one SDK still auto-fills them.
The new contract says all four ConfigData fields are mandatory, but the Rust SDK currently routes this shape through config_data_correct_errors (crates/superposition_sdk/src/serde_util.rs:970-976), which replaces any missing field with an empty collection before ConfigDataBuilder::build() runs. That turns malformed /version payloads into apparently valid configs and can silently drop server data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@smithy/models/config.smithy` around lines 79 - 91, The schema incorrectly
marks ConfigData members as required (contexts, overrides, default_configs,
dimensions) while the Rust SDK still auto-fills missing fields via
config_data_correct_errors in serde_util.rs before ConfigDataBuilder::build();
remove the `@required` annotations from those four members in the ConfigData
structure so the model no longer treats them as mandatory and avoids masking
missing server fields, or alternatively ensure the SDK stops auto-filling before
build (adjust config_data_correct_errors) so missing fields are detected.
There was a problem hiding this comment.
what does this mean ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look at the config_data_correct_errors function in serde_util.rs
rg -n "config_data_correct_errors" --type rust -A 15 -B 2
Length of output: 8243
@ayushjain17, here's a plain-language breakdown of what the comment is flagging.
Two things are in conflict
1. The Smithy model says these fields are mandatory
In smithy/models/config.smithy, the new ConfigData structure declares all four fields as @required:
structure ConfigData {
`@required`
contexts: ContextList
`@required`
overrides: OverridesMap
`@required`
default_configs: Object
`@required`
dimensions: DimensionData
}@required means: "the server MUST always send this field — if it's missing, that's an error."
2. The Rust SDK silently fakes missing fields with empty defaults
In crates/superposition_sdk/src/serde_util.rs (lines 970–976), there's a generated helper called config_data_correct_errors:
pub(crate) fn config_data_correct_errors(mut builder: ConfigDataBuilder) -> ConfigDataBuilder {
if builder.contexts.is_none() { builder.contexts = Some(Default::default()) }
if builder.overrides.is_none() { builder.overrides = Some(Default::default()) }
if builder.default_configs.is_none() { builder.default_configs = Some(Default::default()) }
if builder.dimensions.is_none() { builder.dimensions = Some(Default::default()) }
builder
}And this is called during deserialization (shape_config_data.rs:40) before build() runs:
Ok(Some(crate::serde_util::config_data_correct_errors(builder).build()...))Why this is a problem
Suppose the server returns a /version response where contexts is accidentally missing (a server bug, a schema mismatch, etc.). Instead of failing with a deserialization error (which would alert you to the problem), the SDK silently substitutes an empty list for contexts. Your application then sees ConfigData { contexts: [], ... } and has no idea data was missing.
In short: the model promises strict validation (@required), but the SDK implementation bypasses it — turning potentially serious data loss into a silent no-op.
What the fix options are
| Option | What to change |
|---|---|
Option A – Remove @required from the 4 fields |
Makes the model honest: these fields are optional in practice, and the SDK's defaulting behaviour is intentional |
Option B – Remove config_data_correct_errors auto-filling |
Makes the SDK enforce the contract: a missing required field on the wire becomes a hard error |
The right choice depends on whether the server is guaranteed to always send all four fields. If yes, Option B is safer (fail loudly on bad responses). If the fields can legitimately be absent, Option A (remove @required) is the honest model.
Does that clarify it? Happy to help implement whichever option you choose.
🧠 Learnings used
Learnt from: Datron
Repo: juspay/superposition PR: 773
File: smithy/models/secret.smithy:26-28
Timestamp: 2026-01-14T07:52:22.442Z
Learning: In Smithy resource modeling, model operations that affect all instances of a resource type at a workspace/organizational level (e.g., key rotation for all secrets) as workspace-scoped or standalone workspace-level operations, not bound to individual entity resources like Secret. Only instance-specific operations (create, read, update, delete) should be bound to entity resources. Apply this pattern broadly to Smithy models in the repository, ensuring workspace-scoped actions are defined on Workspace resources or as separate workspace-level operations, while entity resources retain only per-instance operations.
| @required | ||
| @notProperty | ||
| @httpQuery("identifier") | ||
| identifier: String | ||
|
|
||
| @notProperty | ||
| @httpQuery("prefix") | ||
| prefix: StringList |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && pwd && git ls-files | grep -E "\.smithy$" | head -20Repository: juspay/superposition
Length of output: 119
🏁 Script executed:
cat -n smithy/models/experiments.smithy | sed -n '220,235p'Repository: juspay/superposition
Length of output: 608
🏁 Script executed:
# Check the broader context of ApplicableVariants operation
rg "ApplicableVariants" -B 5 -A 10 smithy/models/experiments.smithyRepository: juspay/superposition
Length of output: 1541
🏁 Script executed:
# Search for any backward-compatibility or migration handling in the codebase
rg -i "identifier.*httpQuery|backward.*compat|dual.*support" --type smithyRepository: juspay/superposition
Length of output: 94
🏁 Script executed:
git log --oneline -n 20 -- smithy/models/experiments.smithyRepository: juspay/superposition
Length of output: 100
🏁 Script executed:
# Get the previous version of the Smithy file to compare
git show HEAD~1:smithy/models/experiments.smithy | grep -A 20 "structure ApplicableVariantsInput"Repository: juspay/superposition
Length of output: 100
🏁 Script executed:
# Search for the server implementation of ApplicableVariants
fd -e java -e go -e ts -e js | xargs grep -l "ApplicableVariants" | head -10Repository: juspay/superposition
Length of output: 6795
🏁 Script executed:
cat -n clients/java/sdk/src/main/java/io/juspay/superposition/model/ApplicableVariantsInput.javaRepository: juspay/superposition
Length of output: 13680
🏁 Script executed:
# Find the server-side handler for ApplicableVariants
fd -type f \( -name "*.java" -o -name "*.go" -o -name "*.ts" -o -name "*.kt" \) | xargs grep -l "applicable-variants" | grep -v client | head -10Repository: juspay/superposition
Length of output: 234
🏁 Script executed:
# Search for server code in common directories
find . -path ./clients -prune -o -type f \( -name "*.java" -o -name "*.kt" \) -print | xargs grep -l "ApplicableVariants" | grep -v clientRepository: juspay/superposition
Length of output: 46
🏁 Script executed:
find . -type f -name "*.java" -o -name "*.kt" | grep -v clients | head -30Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look for server code directories
find . -type d -name "src" -o -name "server" -o -name "backend" | head -10Repository: juspay/superposition
Length of output: 391
🏁 Script executed:
# Search for any handler or controller code
find . -type f \( -name "*Handler*.java" -o -name "*Controller*.java" -o -name "*Service*.java" \) | grep -v clients | head -20Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check for README or documentation that might explain the API contract
find . -type f \( -name "README*" -o -name "CHANGELOG*" -o -name "MIGRATION*" \) | head -10Repository: juspay/superposition
Length of output: 440
🏁 Script executed:
# Look for any test files that might show the expected request format
find . -type f -name "*.test.*" -o -name "*Test*" | grep -i applicable | head -10Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check git log more broadly to understand repo state
git log --all --oneline | head -20Repository: juspay/superposition
Length of output: 1241
🏁 Script executed:
# Check what changed in the ApplicableVariants structure in recent commits
git log --oneline -- smithy/models/experiments.smithy | head -20Repository: juspay/superposition
Length of output: 100
🏁 Script executed:
# Show the diff for the most recent changes to the Smithy file
git show 330279e7:smithy/models/experiments.smithy | grep -A 20 "structure ApplicableVariantsInput"Repository: juspay/superposition
Length of output: 916
🏁 Script executed:
# Check the commit that says "Spec for list exp and get config endpoints"
git show 330279e7 -- smithy/models/experiments.smithy | head -100Repository: juspay/superposition
Length of output: 2558
🏁 Script executed:
# Get the full diff for ApplicableVariants in that commit
git show 330279e7 -- smithy/models/experiments.smithy | grep -A 30 "ApplicableVariantsInput"Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check the current state and confirm the changes
git show HEAD:smithy/models/experiments.smithy | grep -A 15 "structure ApplicableVariantsInput"Repository: juspay/superposition
Length of output: 496
🏁 Script executed:
# Check if there's any migration handling or dual-support in the codebase
rg -i "identifier.*body|backward|compat|migration" --type-list | head -20Repository: juspay/superposition
Length of output: 453
🏁 Script executed:
# Get the precise diff for ApplicableVariantsInput
git show 330279e7 -- smithy/models/experiments.smithy | grep -B 5 -A 20 "@httpQuery(\"identifier\")"Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Search for any body parameter support or migration logic
rg "identifier" smithy/models/experiments.smithy -B 3 -A 3Repository: juspay/superposition
Length of output: 334
🏁 Script executed:
# Check if there are any server/backend implementations that might handle both formats
find . -type f \( -name "*.rs" -o -name "*.go" \) | head -20Repository: juspay/superposition
Length of output: 881
🏁 Script executed:
# Search for ApplicableVariants in the Rust server code
find ./crates -type f -name "*.rs" | xargs grep -l "applicable.*variants" -iRepository: juspay/superposition
Length of output: 1514
🏁 Script executed:
# Check the experiments API handler
find ./crates -type f -name "*.rs" | xargs grep -l "Experiments\|experiments" | grep -E "handler|api" | head -10Repository: juspay/superposition
Length of output: 633
🏁 Script executed:
# Look at the experiments service/handler structure
ls -la ./crates/*/src/api/Repository: juspay/superposition
Length of output: 3075
🏁 Script executed:
# Check the experiments handler for ApplicableVariants
cat -n ./crates/experimentation_platform/src/api/experiments/handlers.rsRepository: juspay/superposition
Length of output: 50376
🏁 Script executed:
# Check the applicable_variants operation input parsing
cat -n ./crates/superposition_sdk/src/operation/applicable_variants/_applicable_variants_input.rsRepository: juspay/superposition
Length of output: 9397
🏁 Script executed:
# Check the protocol serde for applicable variants
cat -n ./crates/superposition_sdk/src/protocol_serde/shape_applicable_variants_input.rsRepository: juspay/superposition
Length of output: 934
🏁 Script executed:
# Confirm the exact change made in commit 330279e7
git show 330279e7:smithy/models/experiments.smithy | grep -A 10 "structure ApplicableVariantsInput"Repository: juspay/superposition
Length of output: 401
🏁 Script executed:
# Check if there's any request body parameter that still accepts identifier
rg "identifier.*body|body.*identifier" crates/Repository: juspay/superposition
Length of output: 3579
Binding identifier to @httpQuery creates a breaking wire change for the POST /experiments/applicable-variants operation.
Moving identifier from the request body to a query parameter changes the request contract for all existing callers. Without server-side dual-support during rollout, older SDKs sending identifier in the body will fail immediately upon deployment when the server stops reading from that location.
Evidence from codebase
Commit 330279e introduced the change: identifier was previously a plain body field, now has @httpQuery("identifier") annotation. The Rust server handler (line 995 of crates/experimentation_platform/src/api/experiments/handlers.rs) extracts identifier from query parameters only, and the serialization layer (shape_applicable_variants_input.rs) does not include identifier in the request body. No fallback parsing logic exists to accept identifier from both locations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@smithy/models/experiments.smithy` around lines 224 - 231, The change binds
identifier to `@httpQuery` which breaks the POST /experiments/applicable-variants
contract; either revert the `@httpQuery`("identifier") annotation on the
identifier shape so it remains a body field, or implement dual-support on the
server: keep the identifier field in the request body shape (remove the
`@httpQuery`) and update the Rust handler (experiments handlers.rs) and the
deserializer (shape_applicable_variants_input.rs) to accept identifier from
query params first and fall back to the request body if absent (add tests and a
deprecation log when query-supplied is used) so existing clients remain
compatible during rollout.
There was a problem hiding this comment.
server updated, please check
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the handler for ApplicableVariants - how identifier is extracted
grep -n "identifier\|query_data\|ApplicableVariantsQuery\|ApplicableVariantsRequest\|get_applicable_variants_helper\|get_applicable_variants_handler" crates/experimentation_platform/src/api/experiments/handlers.rs | head -60
echo "---"
# Check the ApplicableVariantsQuery and ApplicableVariantsRequest structs
cat crates/superposition_types/src/api/experiments.rs | grep -A 20 "ApplicableVariants"
echo "---"
# Check the Rust SDK operation input
grep -n "identifier\|prefix\|uri_query\|httpQuery" crates/superposition_sdk/src/operation/applicable_variants/_applicable_variants_input.rs
echo "---"
# Check the serialization layer
cat crates/superposition_sdk/src/protocol_serde/shape_applicable_variants_input.rs
echo "---"
# Check the uri_query in applicable_variants operation
grep -n "identifier\|prefix\|uri_query" crates/superposition_sdk/src/operation/applicable_variants.rs 2>/dev/null || \
grep -rn "identifier\|prefix\|uri_query" crates/superposition_sdk/src/operation/applicable_variants/ | grep -v "_input\|builders\|_output"
Length of output: 5277
@ayushjain17, thanks for the update! I've reviewed the current server-side code. Here's the assessment:
✅ What's fixed:
- The handler (
handlers.rslines 983–988) now correctly readsidentifierfrom query parameters (query_data.identifier) for both GET and POST methods. - The Rust SDK's
uri_queryproperly serializes bothidentifierandprefixas query params. shape_applicable_variants_input.rsno longer emitsidentifierin the request body — onlycontextis sent in the body.
ApplicableVariantsRequest still has identifier in the body with a TODO comment:
// TODO: remove this once services start using newer sdk
#[serde(alias = "toss", deserialize_with = "deserialize_identifier")]
pub identifier: String,However, the handler never reads req_body.identifier — it only uses query_data.identifier. Since ApplicableVariantsQuery.identifier is a required String (not Option<String>), Actix's Query<> extractor will return a 400 error before the handler even runs if an old client omits identifier from the query string. The body field is dead code.
If coordinated migration is the intent (i.e., all callers will be updated simultaneously), the current server state is fine. But if true backward compatibility is needed during rollout, the fix would be to make query_data.identifier optional and fall back to req_body.identifier:
// In ApplicableVariantsQuery
pub identifier: Option<String>,
// In handler
let identifier = query_data.identifier
.or_else(|| req_body.as_ref().and_then(|b| b.identifier.clone()))
.ok_or_else(|| bad_argument!("identifier is required"))?;Is this an intentional coordinated cutover, or does backward compatibility need to be maintained?
🧠 Learnings used
Learnt from: Datron
Repo: juspay/superposition PR: 773
File: smithy/models/secret.smithy:26-28
Timestamp: 2026-01-14T07:52:22.442Z
Learning: In Smithy resource modeling, model operations that affect all instances of a resource type at a workspace/organizational level (e.g., key rotation for all secrets) as workspace-scoped or standalone workspace-level operations, not bound to individual entity resources like Secret. Only instance-specific operations (create, read, update, delete) should be bound to entity resources. Apply this pattern broadly to Smithy models in the repository, ensuring workspace-scoped actions are defined on Workspace resources or as separate workspace-level operations, while entity resources retain only per-instance operations.
abca9bc to
c00cfad
Compare
| mut context: EvaluationContext, | ||
| prefix_filter: Option<&[String]>, | ||
| prefix_filter: Option<Vec<String>>, | ||
| ) -> Result<Map<String, Value>> { |
There was a problem hiding this comment.
Can we add a readiness check here :
async fn ensure_ready(&self) -> Result<()> {
let status = self.status.read().await;
match *status {
ProviderStatus::Ready => Ok(()),
ProviderStatus::NotReady => Err(SuperpositionError::ProviderError(
"Provider not initialized. Call initialize() before evaluating flags.".into(),
)),
ProviderStatus::Error => Err(SuperpositionError::ProviderError(
"Provider is in error state.".into(),
)),
ProviderStatus::STALE => {
log::warn!("SuperpositionAPIProvider: provider data is stale, proceeding with request");
Ok(())
}
}
}
We can then do : ensure_ready().await? here
We are doing something similar in localProvider , we don't need fresh_data since caching isn't there yet but
adding readiness check would be useful here
ayushjain17
force-pushed
the
fix/remote-provider
branch
from
c00cfad to
2f25f83
Compare
| let global_context = self.global_context.read().await; | ||
| context.merge_missing(&global_context); | ||
|
|
||
| // 3. Extract context and targeting_key | ||
| let (query_data, _targeting_key) = | ||
| conversions::evaluation_context_to_query(context); | ||
| let (query_data, targeting_key) = | ||
| conversions::evaluation_context_to_query_document(context); |
There was a problem hiding this comment.
Same steps being repeated in get_applicable_variants
ayushjain17
force-pushed
the
fix/remote-provider
branch
2 times, most recently
from
7819098 to
307e1b8
Compare
ayushjain17
force-pushed
the
fix/remote-provider
branch
from
307e1b8 to
57cea65
Compare
Problem
Describe the problem you are trying to solve here
Solution
Provide a brief summary of your solution so that reviewers can understand your code
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change
Summary by CodeRabbit
New Features
prefixquery parameter for filtering applicable variants by key prefix.Changes
identifierandprefixinstead of request body.ConfigDataobjects instead of generic JSON documents.