Conversation
| decoder.dateDecodingStrategy = .iso8601 | ||
|
|
||
| do { | ||
| let configuration = try decoder.decode(PrecomputedConfiguration.self, from: data) |
There was a problem hiding this comment.
In local testing, I saw that this causes issues
Looks like fetchedAt is mainly generated for the wire format used for offline initialization: https://github.com/Eppo-exp/js-sdk-common/blob/16e822d77cfe66d450b075152ea0016333432f3f/src/configuration-wire/configuration-wire-types.ts#L217
The Android and JS SDKs aren't otherwise tracking fetchedAt at all, e.g. in logs, so I took it out to match
| request.setValue("application/json", forHTTPHeaderField: "Content-Type") | ||
|
|
||
| let encoder = JSONEncoder() | ||
| encoder.keyEncodingStrategy = .convertToSnakeCase |
There was a problem hiding this comment.
Confirmed with local testing: removing this preserves the camelCase
And then we can override the camelCase-auto-encoding with CodingKeys to set snake_case where expected
There was a problem hiding this comment.
Sorry that API is messy; I don't like the mix of camel and snake case. Could I augment it to accept both to resolve this?
There was a problem hiding this comment.
That would be nice if they could all be snake_case, though I'd say it's not immediately necessary. I caught one more issue in local testing with the fetchedAt so I'll still need to release a patch
| XCTAssertTrue(json.contains("\"numericAttributes\""), "JSON was: \(json)") | ||
| XCTAssertTrue(json.contains("\"categoricalAttributes\""), "JSON was: \(json)") |
There was a problem hiding this comment.
The other SDKs set these to camelCase
And matches the network request in browser
sameerank
force-pushed
the
sameerank/FFL-1808-Override-convertToSnakeCase-encoding-for-context-attributes
branch
4 times, most recently
from
b7440ff to
b8960e9
Compare
sameerank
force-pushed
the
sameerank/FFL-1808-Override-convertToSnakeCase-encoding-for-context-attributes
branch
from
b8960e9 to
67ca519
Compare
There was a problem hiding this comment.
Pull request overview
Updates precomputed flag request/response encoding/decoding to match the server’s expected key casing and removes fetchedAt from the SDK configuration model.
Changes:
- Encode request payload with snake_case top-level keys while keeping nested
ContextAttributeskeys in camelCase. - Decode server responses into a dedicated
PrecomputedServerResponseand buildPrecomputedConfigurationlocally (not expectingsubject/fetchedAtfrom the server). - Remove
fetchedAtfromPrecomputedConfigurationand update affected tests/fixtures; bump SDK version.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/eppo/precomputed/PrecomputedRequestorTests.swift | Adds coverage for top-level snake_case + nested camelCase payload encoding. |
| Tests/eppo/precomputed/PrecomputedConfigurationTests.swift | Removes fetchedAt usage from initialization/encoding/fixture expectations. |
| Tests/eppo/precomputed/PrecomputedConfigurationStoreTests.swift | Updates store tests to align with PrecomputedConfiguration without fetchedAt. |
| Tests/eppo/precomputed/EppoPrecomputedClientTests.swift | Updates client tests for new configuration shape (no fetchedAt). |
| Tests/eppo/precomputed/EppoPrecomputedClientPollingTests.swift | Removes fetchedAt from polling test configurations. |
| Tests/eppo/precomputed/EppoPrecomputedClientPerformanceTests.swift | Updates performance tests for config model without fetchedAt. |
| Tests/eppo/precomputed/EppoPrecomputedClientInitializationTests.swift | Updates initialization tests to remove fetchedAt. |
| Tests/eppo/precomputed/EppoPrecomputedClientErrorTests.swift | Adjusts error/concurrency test setup to match new server response decoding path. |
| Tests/eppo/precomputed/EppoPrecomputedClientAssignmentTests.swift | Removes fetchedAt from assignment test configuration setup. |
| Sources/eppo/precomputed/PrecomputedRequestor.swift | Implements custom payload coding keys and decodes PrecomputedServerResponse to construct PrecomputedConfiguration. |
| Sources/eppo/precomputed/PrecomputedConfiguration.swift | Removes fetchedAt from the model and wire-format decoding/encoding. |
| Sources/eppo/precomputed/EppoPrecomputedClient.swift | Simplifies client to store the configuration returned by the requestor (now includes subject). |
| Sources/eppo/Constants.swift | Bumps sdkVersion to 5.3.3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
leoromanovsky
left a comment
leoromanovsky
left a comment
There was a problem hiding this comment.
sorry that the json serialization gave you so much trouble in swift; kudos for jettisoning the fetchedAt instead of chasing after completeness there.
sameerank
deleted the
sameerank/FFL-1808-Override-convertToSnakeCase-encoding-for-context-attributes
branch
🎟️ Fixes issue
📜 Design Doc: link if applicable
Motivation and Context
Description
Two fixes:
categorical_attributes→categoricalAttributes,numeric_attributes→numericAttributesfetchedAt. This field is generated in the configuration wire for offline initialization, but not currently used by any of the SDKs, so it shouldn't be "expected" by the SDK's config data modelHow has this been documented?
How has this been tested?