openapi-tools / jackson-dataformat-hal

Dataformat extension for Jackson to handle HAL JSON
MIT License
24 stars 6 forks source link

Provide a HAL-specific JacksonJsonProvider #32

Closed arucard21 closed 3 years ago

arucard21 commented 3 years ago

I extended the default JacksonJsonProvider from Jackson to provide one that is specifically for HAL-based resources. It is only used by JAX-RS when the media type is application/hal+json. This extended provider mainly modifies the ObjectMapper by registering a JacksonHALModule (same as a HALMapper).

This allows you to just register JacksonHALJsonProvider instead of a JacksonJsonProvider with HALMapper instance.

A big benefit here is that you can now register both a JacksonHALJsonProvider and a JacksonJsonProvider. This allows your JAX-RS implementation to serialize/deserialize both plain JSON and HAL+JSON at runtime (determined by the provided media type).

In order for this to work correctly, if the ObjectMapper is configured through a JAX-RS provider, this JacksonHALJsonProvider will use a copy of it. This allows you to configure and provide only a single ObjectMapper which will be used for both plain JSON and HAL+JSON.

I verified that this works in my own JAX-RS implementation though that didn't cover all the different ways that this provider can be created. But I'm not sure if it makes sense to write unit tests for this. I've allowed edits by maintainers in the PR so you can edit this branch on my fork as needed.

codecov[bot] commented 3 years ago

Codecov Report

Merging #32 (681940d) into master (f2834e1) will decrease coverage by 4.35%. The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #32      +/-   ##
============================================
- Coverage     77.27%   72.92%   -4.36%     
  Complexity       63       63              
============================================
  Files            10       11       +1     
  Lines           352      373      +21     
  Branches         67       68       +1     
============================================
  Hits            272      272              
- Misses           72       93      +21     
  Partials          8        8              
Impacted Files Coverage Δ Complexity Δ
...jackson/dataformat/hal/JacksonHALJsonProvider.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f2834e1...681940d. Read the comment docs.

langecode commented 3 years ago

While I have also over the time written something similar to this in JAX-RS based projects I am a bit weary about putting in annotations like @Named, @Provider, @Produces, and @Consumes. Basically this would trigger functionality that you may or may not want to trigger. But I acknowledge that this is something that does not make sense to write over and over.

Considering this maybe we should create another very simple project that basically just draws in jackson-dataformat-hal as a dependency and provides the "bootstrapping" class? If you put this new project as dependency in your JAX-RS service you are very consciously making a decision to do "auto bootstrap". Could that be a solution?

arucard21 commented 3 years ago

Looking at how this is structured in Jackson itself, there does seem to be a separate jackson-dataformat-<x> and jackson-jaxrs-<x>-provider dependency. So since this project is already provides the jackson-dataformat-hal dependency, I think it makes sense to create a jackson-jaxrs-hal-provider project under openapi-tools. This JacksonHALJsonProvider can then be included there and it can depend on this dataformat project, as you suggested. I think that does make more sense than including it here.

As for the annotations, I think @Provider, @Produces and @Consumes are necessary here. If you look at Jackson's own JacksonJsonProvider, it has those same annotations. They are all JAX-RS anotations so it makes sense that you need to add them on a JAX-RS provider.

The only one I added myself is @Named. This one could indeed be removed. I only added it to ensure that this provider can be found with classpath scanning by any CDI framework, not just JAX-RS frameworks (which detect it through the @Provider annotation). This really only makes sense when you use something like Spring Boot together with Jersey so you have 2 injection frameworks that work together. Even then, it's probably not needed so I do think that @Named should be removed.

If you want to create a separate project for this, I think you can just copy this code there. If you reference this PR in the commit, I think it should be fine to close this PR then. That should be enough to find this discussion from that new project, if needed.

langecode commented 3 years ago

Great, I will create a project for the jax-rs "binding" then. I have no problem with the annotations when the class is in a separate project - the user is making a conscious choice adding that as a dependency whose only purpose is to trigger the bootstrap - so the annotations are fine there, imho.

Been a while since I did much Java. It used to be with CDI that you had to include a beans.xml in the jar file for the @Named beans to be discovered. I notice you did not include this with your PR. Do you know if it is still necessary?

arucard21 commented 3 years ago

Sounds good. I do think the @Named annotation can still be removed. It really does seem unnecessary and may actually cause problems.

I've used Spring and Guice a bit for CDI and I've actually never had to use or write a beans.xml file. Spring usually detects beans through resource scanning, looking for classes that are annotated with @Named or @Component (the Spring-specific equivalent of @Named). And with Guice you usually bind the bean programmatically. But there are many ways to do this, so this is just what I'm personally familiar with.

From what I could find about beans.xml, that should still be available. It seems to be an older approach to bean discovery. Though one that should be compatible with any CDI framework and doesn't require annotations. I've never used it though.

langecode commented 3 years ago

Create first shot at a simple provider implementation in version 1.0.0: https://github.com/openapi-tools/jackson-dataformat-hal-provider/tree/main

Lets iterator over that if it need any additional features.