cadence-java-client icon indicating copy to clipboard operation
cadence-java-client copied to clipboard

Add Jackson DataConverter

Open mfateev opened this issue 6 years ago • 5 comments

Gson in the java client is printing this error when using Java 12: WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by com.google.gson.internal.reflect.UnsafeReflectionAccessor (jar:file:/cloud.pco.orchestrator.jar!/BOOT-INF/lib/gson-2.8.5.jar!/) to constructor java.util.Optional() WARNING: Please consider reporting this to the maintainers of com.google.gson.internal.reflect.UnsafeReflectionAccessor WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release Looks like there is an issue for it here: https://github.com/google/gson/issues/1540 But at the same time there hasn't been a release of Gson since over a year.

Consider switching the default data converter to Jackson.

mfateev avatar Oct 03 '19 16:10 mfateev

Would like to +1 this as Jackson has built in support for Java 8 Time objects allowing you to send your java.time.Instant or java.time.ZonedDateTime as a string.

mhubbard avatar Dec 12 '19 21:12 mhubbard

Would be a good thing indeed to be adding, especially when one day we want to be able to provide a Spring Boot starter for Cadence. +1

RemcoBuddelmeijer avatar Dec 28 '19 10:12 RemcoBuddelmeijer

I suggest testing all changes done for this issue on both JDK 8 and JDK 13 so that both the current supported Java LTS version and the latest Java version have been covered. What is your view on this? @mfateev

To reach this goal, I will be making a separate Docker and Docker-compose file that make this possible.

RemcoBuddelmeijer avatar Jan 15 '20 19:01 RemcoBuddelmeijer

Should we use Multi-Release Jars to support multiple Java versions?

mfateev avatar Jan 15 '20 20:01 mfateev

I think it would be a nice touch, however, I don't think it's something really for the Cadence Client, as it would require a lot more change and a lot more complications in the development cycle to implement this feature to it's fullest (versus having multiple builds of the client). I do understand that this feature is more targeted towards (third-party) libraries and frameworks, which then makes this feature eligible for this Java Client; which then means that we would have to upgrade to either Java 9 or 11. (More on this in a later block)

When it comes to testing, I don't think that using it for the client, which is "bound" to a specific version, makes it easier to maintain the code. We will need to also maintain the tests which might have dependencies which are bound to a specific version, limiting us into even using these Java features. For example, we maybe want to use a new Java 13 feature but in order to test it, we might have to completely build a specific part of a testing framework our selves as these do not support these Java 13 features yet.

I do rather have it to support from a specific LTS version and to have sub-versions of the Cadence Java Client on other non-main (as in non-main Cadence supported) Java versions (kind of like development/POC versions of the Java client). I would suggest to instead of having the Multi-Release Jars, to support up to Java 11 (LTS) and to have versions that support certain features from Java 12, 13, etc.

RemcoBuddelmeijer avatar Jan 15 '20 20:01 RemcoBuddelmeijer