Skip to content

feat: ProviderEvaluation json de/serialization support #487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all 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
19 changes: 19 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,18 @@
<version>2.0.7</version>
</dependency>

<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-annotations</artifactId>
<version>2.15.2</version>
</dependency>

<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
<version>2.15.2</version>
</dependency>

Comment on lines +66 to +77
Copy link
Member

@toddbaert toddbaert Jun 23, 2023

Choose a reason for hiding this comment

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

I'm not sure how I feel about adding these jackson dependencies.

I believe jackson can use javax.xml annotations. I think we should use those instead since they are standard.

Copy link
Contributor Author

@Kavindu-Dodan Kavindu-Dodan Jun 23, 2023

Choose a reason for hiding this comment

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

I haven't personally used jaxb for json de/serialization. Besides, built-in support for Java SE for jaxb is removed from Java 9+ [1]

If we need to support de/serialization, I believe we will have some sort of dependency to a library. Here, my observation was thatcom.fasterxml.jackson.core: jackson-databind is anyway needed for @Jacksonized annotation.

[1] - https://openjdk.org/jeps/320

Copy link
Member

@toddbaert toddbaert Jun 23, 2023

Choose a reason for hiding this comment

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

Besides, built-in support for Java SE for jaxb is removed from Java 9+

That's a bummer.

I have to think about this a bit more. As I mentioned in the original issue, my enthusiasm for supporting this was predicated on it not adding more dependencies. Right now we have only slf4j as an actual dependency. This adds significantly more.

Here, my observation was thatcom.fasterxml.jackson.core: jackson-databind is anyway needed for @Jacksonized annotation.

Oh - I didn't read enough into the lombok annotation. I just though it added POJO methods that jackson used via reflection or something. You're right that it seems to be adding jackson annotations though.

I don't think this is a great idea at this point. I feel like adding a json parsing related dep is too far. I wonder what @lopitz and @justinabrahms think.

Copy link
Contributor

Choose a reason for hiding this comment

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

@toddbaert , @Kavindu-Dodan

Jackson is just on json serializer/deserializer. There's also JSON and JAXB implementations and I guess a gazillion others. imho, we should not rely on jackson, just because a single user complains about it being hard to use. seems like the real issue is the missing default constructor in the dto. hence, i'd simply add a @NoArgsConstructor annotation and leave it with that. Looking at this class tells me also, that it is fine, as it is a @Data annotated class and not a @Value annotated one. I guess, we should have a default constructor for all data classes to stay consistent and allow for things like object creation via reflection...

Copy link
Member

Choose a reason for hiding this comment

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

@toddbaert , @Kavindu-Dodan

Jackson is just on json serializer/deserializer. There's also JSON and JAXB implementations and I guess a gazillion others. imho, we should not rely on jackson, just because a single user complains about it being hard to use. seems like the real issue is the missing default constructor in the dto. hence, i'd simply add a @NoArgsConstructor annotation and leave it with that. Looking at this class tells me also, that it is fine, as it is a @Data annotated class and not a @Value annotated one.

I agree. I was hopeful that JAXB bindings would solve this issue for us, but I found out in this thread they're non-standard after Java9.

I guess, we should have a default constructor for all data classes to stay consistent and allow for things like object creation via reflection...

Ya, like I said in the associated issue, I think supporting an empty constructor is a good idea, and we should probably do so for all such "DTO" classes.

Copy link
Contributor Author

@Kavindu-Dodan Kavindu-Dodan Jun 26, 2023

Choose a reason for hiding this comment

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

Thank you @lopitz & @toddbaert for your suggestions.

I checked your proposals but still, there's a blocker to proceed here. The de/serialization error is there because of the ImmutableMetadata class [1]. This class was made immutable as this is a recommendation from the spec [2]. Given that ImmutableMetadata is a special class, I do not see a workaround to this.

I also think adding 3rd party libraries to support such a use case is NOT justifiable as this goes away from being simple. And the best recommendation would be to propose custom wrappers for de/serialization purposes.

[1] - https://github.com/open-feature/java-sdk/blob/main/src/main/java/dev/openfeature/sdk/ProviderEvaluation.java#L17
[2] - https://github.com/open-feature/spec/blob/main/specification/sections/01-flag-evaluation.md#conditional-requirement-14141

Copy link
Member

@toddbaert toddbaert Jun 26, 2023

Choose a reason for hiding this comment

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

I think we can still add the empty constructor. I t might make things easier, though it doesn't solve the entire problem.

I'll open a PR for that if there's no objections.

UPDATE: opened #491

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@toddbaert Thank you for the #491

With this conclusion, I will close this PR as we agree on not exposing any specific annotation support

<!-- test -->
<dependency>
<groupId>org.mockito</groupId>
Expand Down Expand Up @@ -152,6 +164,13 @@
<version>4.2.0</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-core</artifactId>
<version>2.15.2</version>
<scope>test</scope>
</dependency>
</dependencies>

<dependencyManagement>
Expand Down
11 changes: 10 additions & 1 deletion src/main/java/dev/openfeature/sdk/ImmutableMetadata.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package dev.openfeature.sdk;

import com.fasterxml.jackson.annotation.JsonGetter;
import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.extern.slf4j.Slf4j;

import java.util.HashMap;
Expand All @@ -13,10 +15,17 @@
public class ImmutableMetadata {
private final Map<String, Object> metadata;

private ImmutableMetadata(Map<String, Object> metadata) {
private ImmutableMetadata(@JsonProperty("metadata") Map<String, Object> metadata) {
this.metadata = metadata;
}

// This is required for serialization support
@SuppressWarnings({"PMD.UnusedPrivateMethod"})
@JsonGetter("metadata")
private Map<String, Object> getMetadata() {
return this.metadata;
}

/**
* Retrieve a {@link String} value for the given key. A {@code null} value is returned if the key does not exist
* or if the value is of a different type.
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/dev/openfeature/sdk/ProviderEvaluation.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@

import lombok.Builder;
import lombok.Data;
import lombok.extern.jackson.Jacksonized;

import javax.annotation.Nullable;

@SuppressWarnings("checkstyle:MissingJavadocType")
@Data @Builder
@Data
@Builder
@Jacksonized
public class ProviderEvaluation<T> implements BaseEvaluation<T> {
T value;
@Nullable String variant;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package dev.openfeature.sdk;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;

class FlagMetadataTest {
class ImmutableMetadataTest {

@Test
@DisplayName("Test metadata payload construction and retrieval")
Expand Down Expand Up @@ -61,4 +63,39 @@ public void notfound_error_validation() {
// then
assertThat(flagMetadata.getBoolean("string")).isNull();
}

@Test
@DisplayName("Make sure class is se/deserializable")
public void test_serialize_deserialization() throws JsonProcessingException {
// given
ImmutableMetadata original = ImmutableMetadata.builder()
.addString("string", "string")
.addInteger("integer", 1)
.addLong("long", Long.MAX_VALUE)
.addFloat("float", Float.MAX_VALUE)
.addDouble("double", Double.MAX_VALUE)
.addBoolean("boolean", Boolean.FALSE)
.build();


final ObjectMapper mapper = new ObjectMapper();

// when
final String json = mapper.writeValueAsString(original);
final ImmutableMetadata converted = mapper.readValue(json, ImmutableMetadata.class);

// then
assertThat(json).isEqualTo("{\"metadata\":{\"boolean\":false,\"string\":\"string\"," +
"\"double\":1.7976931348623157E308,\"integer\":1,\"float\":3.4028235E38,\"long\":9223372036854775807}}");

assertThat(converted.getValue("string", String.class)).isEqualTo(original.getString("string"));
assertThat(converted.getValue("integer", Integer.class)).isEqualTo(original.getInteger("integer"));
assertThat(converted.getValue("boolean", Boolean.class)).isEqualTo(original.getBoolean("boolean"));
assertThat(converted.getValue("long", Long.class)).isEqualTo(original.getValue("long", Long.class));
assertThat(converted.getValue("double", Number.class)).isEqualTo(original.getValue("double", Number.class));

// float get converted to double, hence representation comparison
assertThat(converted.getValue("float", Double.class).toString())
.isEqualTo(original.getValue("float", Float.class).toString());
}
}
46 changes: 46 additions & 0 deletions src/test/java/dev/openfeature/sdk/ProviderEvaluationTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package dev.openfeature.sdk;

import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import java.io.IOException;
import java.io.Reader;
import java.io.StringReader;

import static org.assertj.core.api.Assertions.assertThat;

class ProviderEvaluationTest {

@Test
@DisplayName("Provider evaluation must support JSON se/deserialization")
public void test_serialization_deserialization() throws IOException {
// given
final ProviderEvaluation<String> original = ProviderEvaluation.<String>builder()
.reason(Reason.STATIC.toString())
.variant("Default")
.value("StringValue")
.flagMetadata(ImmutableMetadata.builder()
.addString("key", "value")
.addInteger("integer", 10)
.build()
)
.build();


// when
final String json = new ObjectMapper().writer().writeValueAsString(original);
final ObjectMapper objectMapper = new ObjectMapper();
final Reader reader = new StringReader(json);
final ProviderEvaluation converted = objectMapper.readValue(reader, ProviderEvaluation.class);

// then
assertThat(converted.getReason()).isEqualTo(original.getReason());
assertThat(converted.getVariant()).isEqualTo(original.getVariant());
assertThat(converted.getValue()).isEqualTo(original.getValue());
assertThat(converted.getFlagMetadata().getString("key"))
.isEqualTo(original.getFlagMetadata().getString("key"));
assertThat(converted.getFlagMetadata().getInteger("integer"))
.isEqualTo(original.getFlagMetadata().getInteger("integer"));
}
}