Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,30 @@
import com.fasterxml.jackson.annotation.JsonProperty;

import java.util.Collections;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

class FeatureToggleEvaluation {
private final String name;
private final String slug;
private final boolean isEnabled;
private final String evaluationKey;
private final List<Segment> segments;
private final Integer clientRolloutPercentage;

@JsonCreator(mode = JsonCreator.Mode.PROPERTIES)
FeatureToggleEvaluation(
@JsonProperty("name") String name,
@JsonProperty("slug") String slug,
@JsonProperty("isEnabled") boolean isEnabled,
@JsonProperty("segments") List<Segment> segments
@JsonProperty(value = "slug", required = true) String slug,
@JsonProperty(value = "isEnabled", required = true) boolean isEnabled,
@JsonProperty("evaluationKey") String evaluationKey,
@JsonProperty("segments") List<Segment> segments,
@JsonProperty("clientRolloutPercentage") Integer clientRolloutPercentage
) {
this.name = name;
this.slug = slug;
this.isEnabled = isEnabled;

this.segments = new ArrayList<>();
if (segments != null) {
this.segments.addAll(segments);
}
}

public String getName() {
return name;
this.evaluationKey = evaluationKey;
this.segments = segments;
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

segments is assigned directly from the constructor parameter and later wrapped with Collections.unmodifiableList(...) in the getter. This still allows external mutation if the original list reference is retained (e.g., callers constructing FeatureToggleEvaluation directly), which can lead to hard-to-trace behavior changes. Consider making a defensive copy in the constructor (e.g., List.copyOf(segments) when non-null) so the instance is truly immutable.

Suggested change
this.segments = segments;
this.segments = segments == null ? null : List.copyOf(segments);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low risk, but seems good to me.

this.clientRolloutPercentage = clientRolloutPercentage;
}

public String getSlug() {
Expand All @@ -42,7 +38,19 @@ public boolean isEnabled() {
return isEnabled;
}

public List<Segment> getSegments() {
return Collections.unmodifiableList(segments);
public Optional<String> getEvaluationKey() {
return Optional.ofNullable(evaluationKey);
}

public Optional<List<Segment>> getSegments() {
return segments == null ? Optional.empty() : Optional.of(Collections.unmodifiableList(segments));
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my first time seeing this Optional class, but it sounds right 👍


public boolean hasSegments() {
return segments != null && !segments.isEmpty();
}

public Optional<Integer> getClientRolloutPercentage() {
return Optional.ofNullable(clientRolloutPercentage);
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.octopus.openfeature.provider;

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.net.MalformedURLException;
import java.net.URI;
import java.net.URISyntaxException;
Expand Down Expand Up @@ -65,7 +64,7 @@ FeatureToggles getFeatureToggleEvaluationManifest()
logger.log(System.Logger.Level.WARNING,String.format("Feature toggle response from %s did not contain expected ContentHash header", manifestURI.toString()));
return null;
}
List<FeatureToggleEvaluation> evaluations = OctopusObjectMapper.INSTANCE.readValue(httpResponse.body(), new TypeReference<List<FeatureToggleEvaluation>>(){});
List<FeatureToggleEvaluation> evaluations = OctopusObjectMapper.INSTANCE.readValue(httpResponse.body(), new TypeReference<>(){});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not seen this before but Claude says you're just telling the deserializer to put the response into a list of FeatureToggleEvaluations. Sounds good 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is just a bit of syntax clean up. Because the type of the variable List<FeatureToggleEvaluation> is specified (no var in Java 8), it isn't needed to be specified in the TypeReference<>.

return new FeatureToggles(evaluations, Base64.getDecoder().decode(contentHashHeader.get()));
} catch (Exception e) {
logger.log(System.Logger.Level.WARNING, "Unable to query Octopus Feature Toggle service", e);
Expand All @@ -83,16 +82,12 @@ private URI getCheckURI() {

private URI getManifestURI() {
try {
return new URL(config.getServerUri().toURL(), "/api/featuretoggles/v3/").toURI();
return new URL(config.getServerUri().toURL(), "/api/toggles/evaluations/v3/").toURI();
} catch (MalformedURLException | URISyntaxException ignored) // we know this URL is well-formed
{ }
return null;
}

private Boolean isSuccessStatusCode(int statusCode) {
return statusCode >= 200 && statusCode < 300;
}

// This class needs to be static to allow deserialization
private static class FeatureToggleCheckResponse {
public byte[] contentHash;
Expand Down
38 changes: 27 additions & 11 deletions src/main/java/com/octopus/openfeature/provider/OctopusContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

import dev.openfeature.sdk.*;
import dev.openfeature.sdk.exceptions.FlagNotFoundError;
import dev.openfeature.sdk.exceptions.ParseError;

import java.util.List;
import java.util.Map;

import static java.util.stream.Collectors.groupingBy;

Expand All @@ -20,35 +20,51 @@ class OctopusContext {
static OctopusContext empty() {
return new OctopusContext(new FeatureToggles(List.of(), new byte[0]));
}

byte[] getContentHash() { return featureToggles.getContentHash(); }

byte[] getContentHash() {
return featureToggles.getContentHash();
}

ProviderEvaluation<Boolean> evaluate(String slug, Boolean defaultValue, EvaluationContext evaluationContext) {
// find the feature toggle matching the slug
var toggleValue = featureToggles.getEvaluations().stream().filter(f -> f.getSlug().equalsIgnoreCase(slug)).findFirst().orElse(null);

// this exception will be handled by OpenFeature, and the default value will be used
if (toggleValue == null) {
throw new FlagNotFoundError();
throw new FlagNotFoundError();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Java implementation throws rather than returning an error response?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specification test seems to confirm this for the parse error case.

Copy link
Copy Markdown
Contributor Author

@liamhughes liamhughes Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of note: these exception types also exist in the .NET SDK: https://github.com/open-feature/dotnet-sdk/tree/main/src/OpenFeature/Error

We may want to align the clients in one direction or the other at some point.

}

if (missingRequiredPropertiesForClientSideEvaluation(toggleValue)) {
throw new ParseError("Feature toggle " + toggleValue.getSlug() + " is missing necessary information for client-side evaluation.");
}
Comment on lines +37 to 39
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ParseError message is generic and doesn’t indicate which required fields are missing. Including the missing property names (e.g., evaluationKey/segments/clientRolloutPercentage) would make diagnosing malformed responses much easier, especially since the exception causes OpenFeature to fall back to the default value.

Copilot uses AI. Check for mistakes.
// if the toggle is disabled, or if it has no segments, then we don't need to evaluate dynamically
if (!toggleValue.isEnabled() || toggleValue.getSegments().isEmpty()) {

// if the toggle is disabled, or if it has no segments, then we don't need to evaluate dynamically
if (!toggleValue.isEnabled() || !toggleValue.hasSegments()) {
return ProviderEvaluation.<Boolean>builder()
.value(toggleValue.isEnabled())
.reason(Reason.DEFAULT.toString())
.build();
}
// If the toggle is enabled and has segments configured, then we need to evaluate dynamically,

// If the toggle is enabled and has segments configured, then we need to evaluate dynamically,
// checking the context matches the segments
return ProviderEvaluation.<Boolean>builder()
.value(MatchesSegment(evaluationContext, toggleValue.getSegments()))
.value(matchesSegment(evaluationContext, toggleValue.getSegments().orElseThrow())) // checked in hasSegments
.reason(Reason.TARGETING_MATCH.toString())
.build();
}

private Boolean MatchesSegment(EvaluationContext evaluationContext, List<Segment> segments) {
private boolean missingRequiredPropertiesForClientSideEvaluation(FeatureToggleEvaluation evaluation) {
if (!evaluation.isEnabled()) {
return false;
}

return evaluation.getClientRolloutPercentage().isEmpty()
|| evaluation.getEvaluationKey().isEmpty()
|| evaluation.getSegments().isEmpty();
Comment on lines +58 to +64
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missingRequiredPropertiesForClientSideEvaluation causes evaluate(...) to throw ParseError for any enabled toggle where segments is missing, even though evaluate can safely treat missing segments as "no targeting" (and you already have a fixture covering a missing segments field). This changes behavior from returning the server-provided isEnabled value to falling back to the caller’s default. Consider treating a missing segments field as an empty list (or only requiring segments when client-side targeting/rollout evaluation is actually performed).

Suggested change
if (!evaluation.isEnabled()) {
return false;
}
return evaluation.getClientRolloutPercentage().isEmpty()
|| evaluation.getEvaluationKey().isEmpty()
|| evaluation.getSegments().isEmpty();
// Client-side evaluation is only applicable when the toggle is enabled and has segments
if (!evaluation.isEnabled() || !evaluation.hasSegments()) {
return false;
}
return evaluation.getClientRolloutPercentage().isEmpty()
|| evaluation.getEvaluationKey().isEmpty();

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be correct. Will give it more thought tomorrow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches what the .NET client is doing and passes the missing-client-validation-fields specification tests.

}

private Boolean matchesSegment(EvaluationContext evaluationContext, List<Segment> segments) {
if (evaluationContext == null) {
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import java.io.InputStream;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import java.util.Optional is present but not referenced anywhere in this test class. Removing it will keep the file clean and avoid warnings in stricter build setups.

Suggested change
import java.util.Optional;

Copilot uses AI. Check for mistakes.

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand All @@ -20,15 +20,14 @@ private InputStream resource(String name) {
return getClass().getResourceAsStream(name);
}

private void assertSegmentsContain(List<Segment> segments, Segment... expected) {
assertThat(segments).usingRecursiveFieldByFieldElementComparator().contains(expected);
private void assertSegmentsContain(Optional<List<Segment>> segments, Segment expected) {
assertThat(segments.orElseThrow()).usingRecursiveFieldByFieldElementComparator().contains(expected);
}

@Test
void shouldDeserializeEnabledToggle() throws Exception {
FeatureToggleEvaluation result = objectMapper.readValue(resource("toggle-enabled-no-segments.json"), FeatureToggleEvaluation.class);

assertThat(result.getName()).isEqualTo("My Feature");
assertThat(result.getSlug()).isEqualTo("my-feature");
assertThat(result.isEnabled()).isTrue();
assertThat(result.getSegments()).isEmpty();
Expand All @@ -53,10 +52,10 @@ void shouldDeserializeToggleWithSegments() throws Exception {
FeatureToggleEvaluation result = objectMapper.readValue(
resource("toggle-with-segments.json"), FeatureToggleEvaluation.class);

assertThat(result.getSegments()).hasSize(2);
assertSegmentsContain(result.getSegments(),
new Segment("license-type", "free"),
new Segment("country", "au")
assertThat(result.getSegments().orElseThrow()).hasSize(2);
assertSegmentsContain(
result.getSegments(),
new Segment("license-type", "free")
);
}
Comment thread
liamhughes marked this conversation as resolved.
Outdated

Expand Down Expand Up @@ -121,7 +120,6 @@ void shouldIgnoreExtraneousProperties() throws Exception {
FeatureToggleEvaluation result = objectMapper.readValue(
resource("toggle-with-extraneous-properties.json"), FeatureToggleEvaluation.class);

assertThat(result.getName()).isEqualTo("My Feature");
assertThat(result.getSlug()).isEqualTo("my-feature");
assertThat(result.isEnabled()).isTrue();
assertSegmentsContain(result.getSegments(), new Segment("license-type", "free"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,22 @@
import dev.openfeature.sdk.MutableContext;
import dev.openfeature.sdk.Reason;
import dev.openfeature.sdk.exceptions.FlagNotFoundError;
import dev.openfeature.sdk.exceptions.ParseError;
import org.junit.jupiter.api.*;

import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.*;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;


class OctopusContextTests {

private static final FeatureToggles sampleFeatureToggles = new FeatureToggles(
Arrays.asList(
new FeatureToggleEvaluation("Enabled Feature", "enabled-feature", true, null),
new FeatureToggleEvaluation("Disabled Feature", "disabled-feature", false, null),
new FeatureToggleEvaluation("Feature With Segments", "feature-with-segments", true, Arrays.asList(new Segment("license-type", "free"), new Segment("country", "au")) )
new FeatureToggleEvaluation("enabled-feature", true, UUID.randomUUID().toString(), Collections.emptyList(), 100),
new FeatureToggleEvaluation("disabled-feature", false, null, null, null),
new FeatureToggleEvaluation("feature-with-segments", true, UUID.randomUUID().toString(), Arrays.asList(new Segment("license-type", "free"), new Segment("country", "au")), 100)
),
new byte[0]
);
Expand All @@ -29,39 +28,79 @@ class OctopusContextTests {
@Test
void shouldEvaluateToTrueIfFeatureToggleIsPresentAndEnabled() {
var subject = new OctopusContext(sampleFeatureToggles);
var result = subject.evaluate("enabled-feature", false, null);
var result = subject.evaluate("enabled-feature", false, null);

assertThat(result.getValue()).isTrue();
}

@Test
void keyShouldBeCaseInsensitiveWhenEvaluating() {
var subject = new OctopusContext(sampleFeatureToggles);
var result = subject.evaluate("Enabled-Feature", false, null);

assertThat(result.getValue()).isTrue();
}

@Test
void shouldEvaluateToFalseIfFeatureToggleIsPresentAndDisabled() {
var subject = new OctopusContext(sampleFeatureToggles);
var result = subject.evaluate("disabled-feature", false, null);
var result = subject.evaluate("disabled-feature", false, null);

assertThat(result.getValue()).isFalse();
}

@Test
void shouldThrowFlagNotFoundErrorIfFeatureToggleIsNotFound() {
var defaultValue = false;
var subject = new OctopusContext(sampleFeatureToggles);
assertThrows(FlagNotFoundError.class, () -> subject.evaluate("key-not-present", defaultValue, null));
}

}

@Test
void shouldThrowParseErrorWhenEnabledToggleIsMissingEvaluationKey() {
var toggles = new FeatureToggles(
List.of(new FeatureToggleEvaluation("feature-a", true, null, Collections.emptyList(), 100)),
new byte[0]
);
var subject = new OctopusContext(toggles);
assertThrows(ParseError.class, () -> subject.evaluate("feature-a", false, null));
}

@Test
void shouldThrowParseErrorWhenEnabledToggleIsMissingSegments() {
var toggles = new FeatureToggles(
List.of(new FeatureToggleEvaluation("feature-b", true, "evaluation-key", null, 100)),
new byte[0]
);
var subject = new OctopusContext(toggles);
assertThrows(ParseError.class, () -> subject.evaluate("feature-b", false, null));
}

@Test
void shouldThrowParseErrorWhenEnabledToggleIsMissingClientRolloutPercentage() {
var toggles = new FeatureToggles(
List.of(new FeatureToggleEvaluation("feature-c", true, "evaluation-key", Collections.emptyList(), null)),
new byte[0]
);
var subject = new OctopusContext(toggles);
assertThrows(ParseError.class, () -> subject.evaluate("feature-c", false, null));
}

@Test
void shouldThrowParseErrorWhenEnabledToggleIsMissingAllClientEvaluationFields() {
var toggles = new FeatureToggles(
List.of(new FeatureToggleEvaluation("feature-d", true, null, null, null)),
new byte[0]
);
var subject = new OctopusContext(toggles);
assertThrows(ParseError.class, () -> subject.evaluate("feature-d", true, null));
}

@TestFactory
Iterable<DynamicTest> shouldCorrectlyDynamicallyEvaluateSegmentsWhenSupplied() {
return Arrays.asList(
evaluationTest("no-segments", "enabled-feature",

evaluationTest("no-segments", "enabled-feature",
buildEvaluationContext(List.of(
Map.entry("user-id", "123456")
)), true, Reason.DEFAULT.toString()),
Expand All @@ -70,7 +109,7 @@ Iterable<DynamicTest> shouldCorrectlyDynamicallyEvaluateSegmentsWhenSupplied() {
buildEvaluationContext(List.of()
), false, Reason.TARGETING_MATCH.toString()),

evaluationTest("all-segments-match", "feature-with-segments",
evaluationTest("all-segments-match", "feature-with-segments",
buildEvaluationContext(Arrays.asList(
Map.entry("license-type", "free"), Map.entry("country", "au"))
), true, Reason.TARGETING_MATCH.toString()),
Expand All @@ -91,23 +130,23 @@ Iterable<DynamicTest> shouldCorrectlyDynamicallyEvaluateSegmentsWhenSupplied() {
), false, Reason.TARGETING_MATCH.toString())
);
}
private DynamicTest evaluationTest(String testName, String featureToggleKey, EvaluationContext evaluationContext,

private DynamicTest evaluationTest(String testName, String featureToggleKey, EvaluationContext evaluationContext,
boolean expectedResult, String expectedReason) {
return DynamicTest.dynamicTest(testName, () -> {
var subject = new OctopusContext(sampleFeatureToggles);
var result = subject.evaluate(featureToggleKey, false, evaluationContext);
assertThat(result.getValue()).isEqualTo(expectedResult);
assertThat(result.getReason()).isEqualTo(expectedReason);
});
return DynamicTest.dynamicTest(testName, () -> {
var subject = new OctopusContext(sampleFeatureToggles);
var result = subject.evaluate(featureToggleKey, false, evaluationContext);
assertThat(result.getValue()).isEqualTo(expectedResult);
assertThat(result.getReason()).isEqualTo(expectedReason);
});
}

private EvaluationContext buildEvaluationContext(List<Map.Entry<String, String>> entries) {
var context = new MutableContext();
entries.forEach(entry -> {
context.add(entry.getKey(), entry.getValue());
});
return context;
}
}

}
2 changes: 1 addition & 1 deletion src/test/java/com/octopus/openfeature/provider/Server.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class Server {
*/
String configure(String responseJson) {
String token = UUID.randomUUID().toString();
wireMock.stubFor(get(urlPathEqualTo("/api/featuretoggles/v3/"))
wireMock.stubFor(get(urlPathEqualTo("/api/toggles/evaluations/v3/"))
.withHeader("Authorization", equalTo("Bearer " + token))
.willReturn(aResponse()
.withStatus(200)
Expand Down
Loading