Conversation
There was a problem hiding this comment.
Pull request overview
Updates the TypeScript OpenFeature provider to call the new Octopus “evaluations” endpoint and aligns the client-side evaluation model with the new response shape (removing name, adding rollout-related fields, and making segments optional).
Changes:
- Switch provider/test server calls from
/api/featuretoggles/v3/to/api/toggles/evaluations/v3/. - Update
V1FeatureToggleEvaluationto match the new endpoint (removename, add optionalevaluationKey/clientRolloutPercentage, makesegmentsoptional). - Add client-side validation for required evaluation fields and adjust segment evaluation logic; disable specification fixture tests for now.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/specificationTests/server.ts | Updates mocked server route to the new evaluations endpoint. |
| src/specificationTests/fixtureEvaluationTests.test.ts | Skips specification fixture tests pending rollout hashing support. |
| src/octopusFeatureProvider.test.ts | Updates test evaluation objects to include new fields. |
| src/octopusFeatureContext.ts | Updates evaluation interface/logic and adds required-field validation for enabled toggles. |
| src/octopusFeatureContext.test.ts | Updates/extends tests for new evaluation shape and missing-field behavior. |
| src/octopusFeatureClient.ts | Switches client request URL to the new evaluations endpoint. |
| src/octopusFeatureClient.test.ts | Updates URL assertion to match the new endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (missingRequiredPropertiesForClientSideEvaluation(evaluation)) { | ||
| return { | ||
| value: defaultValue, | ||
| errorCode: ErrorCode.PARSE_ERROR, | ||
| errorMessage: `Feature toggle ${slug} is missing necessary information for client-side evaluation.`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The new missingRequiredFieldsForClientEvaluation PARSE_ERROR path will break fallback-to-cache behavior for users upgrading from the previous response shape: cached evaluations (schemaVersion "v1") won’t have evaluationKey/clientRolloutPercentage, so enabled flags will now resolve to defaultValue+PARSE_ERROR whenever the network fetch fails. Consider bumping the cache schema/localStorage key to invalidate older cached manifests, or relaxing this check until client-side rollout logic actually depends on these fields (e.g., only require them when performing rollout hashing).
There was a problem hiding this comment.
Huh. The use of local storage could indeed be the source of bugs in the future... I'll find a fix and make a note to discuss this further.
| // Skipped: rollout percentage evaluation (hashing) not yet implemented. | ||
| // Re-enable when the follow-up PR adds that logic. | ||
| test.skip.each(testCases)("$testCase.description", async ({ testResponse, testCase }) => { |
There was a problem hiding this comment.
test.skip.each(...) still executes the module-level loadTestCases() (and fs.readdirSync) when Jest loads this file, so skipping won’t prevent failures if the specification/Fixtures submodule/directory isn’t present. If the intent is to disable these spec tests temporarily, consider guarding fixture loading with an existence check and conditionally skipping the suite, or making fixture loading lazy so it doesn’t run when skipped.
There was a problem hiding this comment.
This is fine. It's only until the next PR.
| possibleV1CacheEntry.contents !== undefined && | ||
| possibleV1CacheEntry.contents.evaluations !== undefined && | ||
| possibleV1CacheEntry.contents.contentHash !== undefined | ||
| possibleV2CacheEntry.schemaVersion === "v2" && |
There was a problem hiding this comment.
This will ensure that previously cached entries won't be used with the new logic.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/octopusFeatureClient.ts:91
axiosrejects non-2xx responses by default, so a 404 from this endpoint will throw and never reach theif (response.status == 404)branch. That preventsgetEvaluationContext()from falling back to cached/empty toggles as intended. Consider addingvalidateStatusto allow 404 responses through, or wrapping the request in a try/catch and returningundefinedwhenerror.response?.status === 404(and optionally for other expected failure modes).
const response = await this.axiosInstance.request<V2FeatureToggleEvaluation[]>(config);
if (response.status == 404) {
this.logger.warn(`Failed to retrieve feature toggles for client identifier ${this.clientIdentifier} from ${this.serverUri}`);
return undefined;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| config.headers!["X-Release-Version"] = this.releaseVersionOverride; | ||
| } | ||
|
|
||
| const response = await this.axiosInstance.request<V1FeatureToggleEvaluation[]>(config); | ||
| const response = await this.axiosInstance.request<V2FeatureToggleEvaluation[]>(config); | ||
|
|
||
| if (response.status == 404) { | ||
| this.logger.warn(`Failed to retrieve feature toggles for client identifier ${this.clientIdentifier} from ${this.serverUri}`); |
There was a problem hiding this comment.
Within getFeatureManifest(), header parsing currently relies on response.headers.get("ContentHash") (with a // @ts-ignore). That isn’t safe across Axios adapters/versions because response.headers can be a plain object (no .get) and header names are often normalized to lowercase. Consider reading the header in a way that supports both AxiosHeaders and plain objects (case-insensitive), and remove the ts-ignore once types align.
There was a problem hiding this comment.
I've made a note of this for discussion/ticketing.
| if (!evaluation.isEnabled) { | ||
| return false; | ||
| } | ||
| return evaluation.evaluationKey == null || evaluation.segments == null || evaluation.clientRolloutPercentage == null; |
There was a problem hiding this comment.
missingRequiredPropertiesForClientSideEvaluation requires segments/evaluationKey/clientRolloutPercentage for enabled toggles, but V2FeatureToggleEvaluation marks all three as optional and the current segment evaluation logic can already treat missing segments as “no segments”. This can cause enabled flags to return PARSE_ERROR + defaultValue for responses where the service omits these fields (e.g., enabled toggle with no segments). Consider either (a) modeling enabled vs disabled evaluations as a discriminated union (enabled => fields required), or (b) relaxing the check to only require fields when they’re actually needed (e.g. when rollout hashing is implemented) and treating segments undefined as an empty list.
| return evaluation.evaluationKey == null || evaluation.segments == null || evaluation.clientRolloutPercentage == null; | |
| // The current client-side evaluation only uses `isEnabled` and, optionally, | |
| // `segments`. Missing `segments` is already treated as "no segments" by | |
| // `evaluateSegments`, and rollout metadata is not yet used here. | |
| return false; |
There was a problem hiding this comment.
I already considered a discriminated union, but that still wouldn't protect against the API returning unexpected values (though we could think about implementing a schema check like zod).
I think Copilot is probably picking up the half-way nature of this PR here. We can tidy in the next PR.
| export interface V2FeatureToggles { | ||
| evaluations: V2FeatureToggleEvaluation[]; | ||
| contentHash: string; | ||
| } |
There was a problem hiding this comment.
Maybe a silly question but what's the reason for renaming everything to V2?
There was a problem hiding this comment.
When I realised I had to update the cache entry, I updated these as well. I have gone with the assumption that the version number related to the shape of the objects rather than the API.
There was a problem hiding this comment.
Makes sense
There was a problem hiding this comment.
Existing users of the consuming application may use evaluations without the new fields leading to incorrect client-side evaluations.
To be honest, I'm actually not sure how much value the cache is providing and I have a note to discuss it when Dylan gets back.
caitlynstocker
left a comment
There was a problem hiding this comment.
Looks in line with the .NET and Java versions 😊
Background
The client percentage rollout feature requires
ClientRolloutPercentageandEvaluationKeyto be included in evaluation responses.We were unable to add these fields to the existing
GET /featuretoggles/v3evaluations endpoint because the Java OpenFeature provider was previously using Jackson with strict deserialisation. See OctopusDeploy/openfeature-provider-java#5.This PR updates this provider to use the new endpoint, which has the following changes compared to the old endpoint:
namefield was removedevaluationKeyandclientRolloutPercentagewere added, though are only returned ifisEnabledevaluated totruein the feature toggles servicesegmentsare also only returned ifisEnabledevaluated totrueResolves DEVEX-137.
Changes
V1FeatureToggleEvaluationinterfacenamepropertyevaluationKeyandclientRolloutPercentagepropertiessegmentsto be optionalOctopusFeatureContextclassV1FeatureToggleEvaluationmissingRequiredPropertiesForClientSideEvaluationcheckNotes for review
Testing
OctopusDeploy/openfeature-provider-specificationthat uses the new endpoint response shape that doesn't also expect the client percentage rollout feature to be implemented. This will be implemented and fully tested in the next PR, prior to the next release of this library