feat: ProviderEvaluation json de/serialization support #487
Conversation
Signed-off-by: Kavindu Dodanduwa <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov Report
@@ Coverage Diff @@
## main #487 +/- ##
=========================================
Coverage 94.44% 94.45%
- Complexity 268 269 +1
=========================================
Files 25 25
Lines 612 613 +1
Branches 36 36
=========================================
+ Hits 578 579 +1
Misses 17 17
Partials 17 17
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| <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> | ||
|
|
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
@NoArgsConstructorannotation and leave it with that. Looking at this class tells me also, that it is fine, as it is a@Dataannotated class and not a@Valueannotated 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@toddbaert Thank you for the #491
With this conclusion, I will close this PR as we agree on not exposing any specific annotation support
|
we will not support a specific JSON de/serialization framework, however #491 will help with empty constructor support. |
This PR
Fixes #483 by adding annotations and accessors to required classes.
Testing
Unit tests were added to validate each class's compliance on json de/serialization support