-
Notifications
You must be signed in to change notification settings - Fork 43
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
Kavindu-Dodan
wants to merge
1
commit into
open-feature:main
from
Kavindu-Dodan:feat/serialization-support
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
46 changes: 46 additions & 0 deletions
46
src/test/java/dev/openfeature/sdk/ProviderEvaluationTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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")); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 that
com.fasterxml.jackson.core: jackson-databind
is anyway needed for@Jacksonized
annotation.[1] - https://openjdk.org/jeps/320
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.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.
There was a problem hiding this comment.
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...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
There was a problem hiding this comment.
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 thatImmutableMetadata
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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