refactor ObjectMapperFactory and other util classes that deal with Jackson

[pjfanning]

Jackson plays an important role in Swagger-Core. Jackson code is evolving towards an eventual Jackson 3.0 release and the existing ObjectMapperFactory probably needs a little refactoring with this in mind.

Jackson 2 already has the Mapper.Builder concept added to it and aim is to deprecate and remove anything on ObjectMapper that allows it to be modified. That is, you configure a Mapper.Builder and this config can be changed but once you create an ObjectMapper from it, that ObjectMapper is supposed to be immutable.

It would also be nice to allow users to add extra features to the Jackson mapper to suit their needs.

My proposal would be:

• Keep existing public methods but also add a few new methods
• Have ObjectMapperFactory use the Jackson JsonMapper.Builder (and YamlMapper.Builder) under the hood
  • these builder instances will have the same swagger init that
• Add an API that allows user to modify these builders
• Possibly, add a factory reset method for the builders which goes back to Swagger defaults for the builders
• Json.mapper() exposes an ObjectMapper created from the ObjectMapperFactory JsonMapper.Builder instance
  • this mapper instance is lazily created and reused (like today)
  • an API is added that allows the stored mapper to be nulled and a new mapper instance can be created lazily from the latest copy of the ObjectMapperFactory JsonMapper.Builder instance
• A similar set of changes to Yaml.mapper()

[pjfanning]

@frantuma I have done a draft PR to illustrate my idea - if the proposal makes sense, I can extend the work to include requested changes and test coverage

[bbguitar77]

@pjfanning I think what you propose is a good idea and I'm interested in trying to push this through. I'm not 100% familiar with the swagger-core code-base but debugging a recent issue on my end led me to the Jackson object mapper exposed in ObjectMapperFactory

My use-case is pretty simple - I have a Kotlin/Jersey integration leveraging JAX-RS annotations and I'm trying to generate the OpenAPI JSON spec via the swagger-gradle plugin. In order for the integration to work, the Kotlin field parameter annotations need to site target the constructor parameters. However, the JAX-RS annotation detection logic in DefaultParameterExtension currently won't detect constructor-level annotations. Under the hood, DefaultParameterExtension uses ObjectMapperFactory. A potential clean solution for me would be to register the KotlinModule on the Jackson object mapper, in which case it can detect the additional JAX-RS annotations on the Kotlin types

I did look over your draft PR and the only thing that I'm confused about is how to potentially integrate with this logic. For instance, the swagger-jaxrs2 module provides a SwaggerLoader in which you can pass a objectMapperProcessorClass. SwaggerLoader just delegates to SwaggerConfiguration in the swagger-integration module. I haven't deep-dived yet to determine how this objectMapperProcessorClass is used but it seems like this could be a potential hook-in if it could be integrated with ObjectMapperFactory.

Let me know your thoughts. Thanks!

[bbguitar77]

There's also a suggestion here to leverage ObjectMapper#findAndRegisterModules but I've never used that and might result in inconsistent behavior

[pjfanning]

Unfortunately, I have no say on what gets merged to swagger-core. Without some indication from someone who can merge that there is hope that my work will not go to waste, I'm reluctant to spend much more time on this,

[bbguitar77]

@pjfanning Understood

@frantuma Any thoughts on the above discussion or anyone else we can potentially discuss solutions with?

[frantuma]

@pjfanning @bbguitar77

Thanks for your contributions in this area!
About the update of ObjectMapperFactory (and Json/Yaml classes) and work in draft PR:

1. At first glance changes in PR look ok, if I am not mistaken they basically provide the same behaviour using more recent logic via the Builder. in terms of output there is no change. Would need to have a deeper insight but I guess we can merge such changes. The only problem is that we are relying on Jackson 2.10+ which means breaking possibly clients using Jackson < 2.10.

2. I am not sure though about the scenarios where changes like the "reset" APIs would make sense. This is slightly related to the issue reported by @bbguitar77 :

At the moment (also because of historical reasons) we have basically three mapper instances used in code:

A. outputJsonMapper: this mapper is originally a copy of Json.mapper() and is customizeable by providing an ObjectMapperProcessor. It is used for all cases of serialization of an OpenAPI instance, e.g. when exposing the generated spec in url, in maven/gradle plugins, and so on.

Customizing the output seems to be the most popular "mapper customization" scenario and it's handled by the processor

B. ModelResolver mapper

The mapper used by ModelResolver (the class responsible for building Schema class hierarchies from Java POJOs). This mapper is a different instance than Json.mapper() only when the ObjectMapperProcessor class is provided, and can be also customized.

C. Json.mapper()

In all the rest of the code, Json.mapper() instance is used; so all deserializers (to deserialize from a string), some constructs in other classes of swagger-core module, and JAX-RS related classes, like Reader or DefaultParameterExtension.
The usage in JAX-RS module is the one causing the issue for @bbguitar77 , as at the moment this is not customizeable.

One solution might be something like what @pjfanning proposes (being able to update the Json.mapper() instance, but I am not really sure of the consequences, as e.g. deserializers probably wouldn't like at all some changes related to the JAX-RS side of things, or at least this may caused troubles.

An alternative approach would be to have the mapper used by JAX-RS being separate, similarly to what done for the outputJsonMapper or at least being the same used by ModelResolver (as probably the same logic would be applied in the "resolving" scenario), so that user could customize that one without affecting other behaviour.

This is not so trivial (but would be really really nice to have!), also because of the current status with e.g. DefaultParameterExtension being loaded/executed via extension chain, but s surely doable with not so much effort at the cose of some breaking of API compatibility possibly.

For @bbguitar77 particuar issue with current version of swagger core, providing an extension (see DefaultParameterExtension for its usage) to process the kotlin code (here you can use your own customized mapper) could possibly work.

@pjfanning @bbguitar77 please let me know your thoughts about this.

[pjfanning]

@frantuma the swagger build uses jackson 2.12 already - if you compile with that version, are you really hoping that users can override the transitive dependency and use an older version of jackson? I am part of the Jackson team and that wouldn't be something that we'd recommend.

Generally, with the existing PR draft, I focused on swagger-core but if swagger-integration has its own classes, similar changes can be made there - I guess I'd prefer to get the swagger-core changes agreed and then move onto swagger-integration.

[frantuma]

@pjfanning I have actually seen several cases where keeping the explicit Jackson version defined in their project was a requirement for the users, there are probably tickets in this repo about that as well.

Not sure what you mean with if swagger-integration has its own classes, can you elaborate a bit more?

Also can you clarify which scenarios are you trageting with the proposed changes, if any? Or would this mainy be done to "modernize" Jackson usage?

[pjfanning]

https://github.com/swagger-api/swagger-core/blob/master/modules/swagger-integration/src/main/java/io/swagger/v3/oas/integration/GenericOpenApiContext.java -- swagger-integration appears to be the name of this 'module' - this is one of the links you provided @frantuma

public class IntegrationObjectMapperFactory extends ObjectMapperFactory {

    public static ObjectMapper createJson() {
        return ObjectMapperFactory.createJson();
    }
}

This class seems to delegate to the class that I've already modified in the PR.

[pjfanning]

the main aim is to allow of this enhancement is to allow users to update the jackson object mapper used by swagger - swagger does a bit of its own configuration already - so we need to ensure this is not lost - so we don't want users just replacing the jackson object mapper and leaving out config that swagger needs but at the same time it is useful to allow users to add extra config (extra jackson modules, etc).

[pjfanning]

I searched through swagger issues - there are a lot - but I found none where people were complaining about jackson version being too new - the issues that mention jackson versions typically are about upgrading.

Jackson MapperBuilder used in my PR was introduced in Jackson 2.10.0 that was released on Sep 26, 2019 - that's a fair while ago. If Log4Shell issue teaches us anything is that trying to support open source users who insist on not upgrading their dependencies is bad for everyone.

If swagger wants to support multiple jackson versions, then the jackson code should be shifted off into its own module with a standardised API and different variants of that module can be published for different jackson versions -- this adds a fair amount of complexity

[frantuma]

I searched through swagger issues - there are a lot - but I found none where people were complaining about jackson version being too new - the issues that mention jackson versions typically are about upgrading.

Jackson MapperBuilder used in my PR was introduced in Jackson 2.10.0 that was released on Sep 26, 2019 - that's a fair while ago. If Log4Shell issue teaches us anything is that trying to support open source users who insist on not upgrading their dependencies is bad for everyone.

If swagger wants to support multiple jackson versions, then the jackson code should be shifted off into its own module with a standardised API and different variants of that module can be published for different jackson versions -- this adds a fair amount of complexity

my comment The only problem is that we are relying on Jackson 2.10+ which means breaking possibly clients using Jackson < 2.10. meant to include everything into consideration, not as a must have, and if we gain more feature/performance/customization that's not a big issue.

the main aim is to allow of this enhancement is to allow users to update the jackson object mapper used by swagger - swagger does a bit of its own configuration already - so we need to ensure this is not lost - so we don't want users just replacing the jackson object mapper and leaving out config that swagger needs but at the same time it is useful to allow users to add extra config (extra jackson modules, etc).

So we agree here I guess. As I mentioned in comment above there are potentially different mappers involved, and it would be nice to allow customization specific to each one. Not entirely sure about your proposal (on top of changes to factory), but we can proceed in steps

[pjfanning]

These changes would only probably be used by a small number of users. I'd prefer not to complicate things by adding jackson customisation in different places. If use cases pop up later, it should be easy to support them.

For instance, if IntegrationObjectMapperFactory needs separate customisation, the API could be something like:

public static void setManageSeparateJacksonMapper(boolean)
//and then a copy of the main ObjectMapperFactory customisation API

[pjfanning]

I might need to go back to the drawing board on the PR. The Jackson 3 Mapper.Builder is due to return fresh mapper instances on each build call but the Jackson 2 version of the code returns the same mapper so my code acts up if a different thread modifies its mapper instance. In the swagger unit tests, there are just 2 tests that do this but I had to disable them in my PR to get the other tests to pass. I'm starting a conversation with Jackson team about whether the Jackson 2 Mapper.Builder can be made to work more like the Jackson 3 version.

[bbguitar77]

@frantuma Sorry for the late reply, but your suggestion on providing my own OpenAPIExtension class worked perfectly to get around the issue I was having with reading Kotlin constructor parameter annotations. I did a bit of digging and missed the fact that the swagger-gradle-reader plugin let's you provide your own custom io.swagger.v3.jaxrs2.Reader readerClass, which lets you set the OpenAPI extensions statically via OpenAPIExtensions.setExtensions(...). So that's another route for folks who might have the same issue. Anyway, thanks for your help! Might be worth adding to the documentation / README

Anyway, regarding the other comments, I don't have too much input other than having a centralized Jackson objectMapper could make the code easier to understand and easier to configure based on various integration use cases (like mine). Though I understand that perhaps having separate Jackson mappers (e.g. one for reading JAX-RS annotations vs. one for writing the OpenAPI spec) could be beneficial as well, so tough to say what might be the best approach. Just my two cents

[pjfanning]

I'm going to close this