Fix: client breaks when evaluation contains segments or unknown response fields#5
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Jackson deserialization failures in the provider client when the evaluation response includes segments or introduces previously-unknown fields, by adding targeted Jackson configuration and tests.
Changes:
- Added a custom Jackson
SegmentDeserializerto map{ "key": "...", "value": "..." }intoMap.Entry<String,String>. - Updated
FeatureToggleEvaluationto (a) ignore unknown response properties and (b) wire the custom deserializer forsegments. - Added unit tests + JSON fixtures covering segments, missing/null segments, list responses, and extraneous properties.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/main/java/com/octopus/openfeature/provider/SegmentDeserializer.java |
Adds custom element deserialization for segments entries. |
src/main/java/com/octopus/openfeature/provider/FeatureToggleEvaluation.java |
Ignores unknown fields and applies the segment entry deserializer. |
src/test/java/com/octopus/openfeature/provider/FeatureToggleEvaluationDeserializationTests.java |
Adds coverage for the previously failing deserialization scenarios. |
src/test/resources/com/octopus/openfeature/provider/*.json |
Adds fixture payloads for the deserialization tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/com/octopus/openfeature/provider/SegmentDeserializer.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dylanlerch
left a comment
There was a problem hiding this comment.
Looks good. One question about case-insensitivity.
| return null; | ||
| } | ||
|
|
||
| JsonNode keyNode = node.get("key"); |
There was a problem hiding this comment.
And can we add a test for it to make sure that it is (I couldn't find one but I also might just be a silly boy that missed it).
There was a problem hiding this comment.
To answer your question: none of this JSON deserialisation was case insensitive.
Implementing case insensitivity led me to two major changes:
- a shared
ObjectMapperwhich also replaced the previous solution to extraneous fields - introducing a
Segmenttype rather than further complicate theSegmentDeserializer
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class OctopusObjectMapper { | ||
| static final ObjectMapper INSTANCE = JsonMapper.builder() | ||
| .enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES) | ||
| .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) | ||
| .build(); |
There was a problem hiding this comment.
OctopusObjectMapper.INSTANCE is a mutable ObjectMapper that can be reconfigured by any code in the package, which can lead to surprising cross-test or runtime behavior. Consider making the class final with a private constructor and exposing the mapper via a method that returns the shared instance (or a copy) while preventing external reconfiguration.
| class OctopusObjectMapper { | |
| static final ObjectMapper INSTANCE = JsonMapper.builder() | |
| .enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES) | |
| .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) | |
| .build(); | |
| final class OctopusObjectMapper { | |
| private static final ObjectMapper INSTANCE = JsonMapper.builder() | |
| .enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES) | |
| .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) | |
| .build(); | |
| private OctopusObjectMapper() { | |
| // Utility class; prevent instantiation. | |
| } | |
| static ObjectMapper getInstance() { | |
| // Return a defensive copy to avoid shared mutable configuration. | |
| return INSTANCE.copy(); | |
| } |
There was a problem hiding this comment.
Hmm...the whole point was to reuse the same instance. Making the wrapper final wouldn't hurt, but seems unnecessary?
|
|
||
| import java.io.InputStream; | ||
| import java.util.List; | ||
| import java.util.Map; |
There was a problem hiding this comment.
Unused import java.util.Map can be removed to keep the test focused and avoid IDE/linter noise.
| import java.util.Map; |
Background
The provider was failing to deserialise evaluation API responses in two scenarios:
https://linear.app/octopus/issue/DEVEX-80/java-client-breaks-on-segments-and-new-response-fields
Changes
SegmentclassMap.Entry<String, String>as the type for segment data, withrequiredkeyandvaluefields.@JsonCreator/@JsonPropertyfor deserialisation.OctopusObjectMapperObjectMapperinstance configured with case-insensitive property matching andFAIL_ON_UNKNOWN_PROPERTIESdisabled, so API responses with differing capitalisation or new fields deserialise correctly.getSegments()returns an immutable list.FeatureToggleEvaluationDeserializationTestssegmentsfield, toggles with one or more segments, a list of toggles, responses containing extraneous/unknown properties, and responses with differing field capitalisation.Notes for review
equals/hashCodeonSegmentbecause it's verbose and not needed at this time. I think we can rely on tests to make sure we don't get caught out, but happy for a difference of opinion here.