Jackson3 support with opt-in jackson2 support

[jjacobs44]

Summary

Following the precedent that OSS Spring Boot 4 set, these changes make Jackson 3 the default that's autoconfigured for the framework to use and Jackson 2 remains available as an opt-in via the graphql-dgs-jackson2 module.

Core changes

• DgsJsonMapper interface abstracts Jackson version from DGS internals
• Jackson3DgsJsonMapper: default implementation using Jackson 3
• graphql-dgs-jackson2: new module with Jackson2DgsJsonMapper and DgsJackson2AutoConfiguration
• BaseDgsQueryExecutor.objectMapper/.parseContext: kept as deprecated lazy vals for backward compatibility (HIDDEN from new code, helpful error if Jackson 2 absent at runtime). I could also just remove them, it would be breaking but I don't think there's many usages
• json-path upgraded from 2.x to 3.x for Jackson 3 provider support

Client changes

• DgsGraphQLResponse interface extracted from GraphQLResponse
• New jackson agnostic client classes: DgsCustomGraphQLClient, DgsRestClientGraphQLClient, DgsWebClientGraphQLClient, DgsCustomMonoGraphQLClient, etc..
• Existing classes that tie their impl to Jackson 2 are deprecated

Example projects

graphql-dgs-example-jackson3-only | OSS default works, Jackson 2 clients fail to load
graphql-dgs-example-jackson2-only | Jackson 2 opt-in works, Jackson 3 clients fail to load
graphql-dgs-example-jackson-both | Mixed classpath works, Jackson 2 wins autoconfig unless configuration specifies otherwise, both client sets usable

---

Why New Classes/Interfaces for the Client Module?

In the client module we have multiple cases where ObjectMapper is exposed in the public api (constructor). I evaluated a couple approaches for how to support jackson3 in this case. After discussing internally option 3 was picked for the best overall migration story for libraries

Approach 1: Constructor Overloads

Add JsonMapper (Jackson 3) overloads alongside existing ObjectMapper constructors on the same class.

This is very cleanly backwards compatible giving people a clean migration path, but requires both jackson2 and 3 to be on the consumers compile classpath. The whole point of this effort is to unblock people from having to have jackson2 on any part of their classpath if they'd like to embrace jackson3.

Approach 2: Change Primary Constructor to Abstraction

Change the clients' primary constructor from ObjectMapper to a version-agnostic ClientMapper sealed interface.

This would move us away from exposing jackson directly in the api which has it's advantages long term. It's ultimately a breaking change for everyone constructing clients today with ObjectMapper which we'd like to avoid.

Approach 3: New Classes w/ Abstraction in Primary Constructors (what this pr implements)

Similar to Approach 2 but putting these into new classes avoids the breaking aspect of the change. We will backport the abstractions to give libraries on older versions the opportunity to migrate before fully removing the old versions of the classes.

We admit the naming is confusing in the interim period, but we deem it worth it and hope the deprecation warnings/kdoc will be informative enough.

[sjohnr]

@jjacobs44, this is great work to get to this point! I have some comments below, but the most important thing (worth discussing if it's not clear from my comments) is around GraphQLClient. See comments on Jackson3RestClientGraphQLClient.

To expand more generally, I think a breaking change of some kind is needed to actually smooth out migration, as well as keeping the framework healthy. I would be in favor of a minor (or major) release to separately introduce the new GraphQLClientResponse interface. You could then also introduce a small breaking change to return that type from non-deprecated GraphQLClient methods, and also remove the deprecated methods (or increase deprecation level to error). Maybe some other cleanup and deprecations could be done as needed to provide visibility for other coming changes.

Afterwards, this PR could be part of a follow-up major release to switch over the default to Jackson 3 and introduce this support with the ability to fall-back to Jackson 2. Any additional necessary breaking changes would then be on the table, but would be much smoother for users since the groundwork is laid in the previous release.

[sjohnr]

Could a compileOnly dep on graphql-dgs-spring-graphql allow the non-string reference? I am thinking it's not at risk of cycles since the jackson2 module is an opt-in?

[paulbakker]

I'm really on the fence about what to do with the client interfaces.
It's not obvious for users why DgsGraphQLClient is jackson version agnostic, while GraphQLClient is Jackson 2.
Having Jackson3CustomGraphQLClient explicitly in the name is also not great for the future.

However, if we "fix" this now, we're introducing a breaking change, and there is a lot of use of these interfaces and classes.

One alternative idea, could we introduce the new stuff in a new separate module, so we can make this depend on the classspath?

[paulbakker]

It looks like this is unused?

[jjacobs44]

Removed all usages in the framework but it's a public api so removing it is technically breaking (some people are calling BaseDgsQueryExecutor.objectMapper). Very few internal usages though so I could remove it and proactively move users off of it.

[paulbakker]

Approach looks good, few minor things I noticed in the code

[iparadiso]

Feel free to push back on this, but since we have an opportunity to start new, I'm wondering if these interfaces are doing too much. Rather than all of these overloads that every client has to implement we have one method.

interface DgsGraphQLClient {                                                                                                                                         
      fun executeQuery(request: DgsGraphQLRequest): DgsGraphQLResponse
  }         

Where DgsGraphQLRequest represents the request and is not a function of the client itself and uses a builder pattern for all the required and optional params which is a cleaner API contract for future changes.

Likewise, we can trim the DgsGraphQLResponse to stable caller contracts

  interface DgsGraphQLResponse {                                                                                                                                       
      val errors: List<GraphQLError>
      fun hasErrors(): Boolean                                                                                                                                         
      fun <T> dataAsObject(clazz: Class<T>): T                                                                                                                         
      fun <T> extractValue(path: String): T   
      fun <T> extractValueAsObject(path: String, clazz: Class<T>): T                                                                                                   
      fun <T> extractValueAsObject(path: String, typeRef: TypeRef<T>): T
  }      

The other response fields move to DefaultDgsGraphQLResponse

Where this might be more challenging is in migrations. :(

[jjacobs44]

I'm on the fence with this so wanna hear others opinions. client.executeQuery("{ foo }"); becoming client.executeQuery(DgsGraphQLRequest.builder().query("{ foo }").build()); is quite a ergonomics downgrade for users imo, but I agree with the general point you're making about the complexity of the overloads not being ideal. Same thing for your comment below.

[iparadiso]

The academic angle here is if builders become too much boilerplate and telescoping constructors reek of code smell, we have an API that's simply too flexible.

The decision then becomes refactoring. Use more static factory methods, focus only on required constructor params and moving optionals to configuration setters.

[iparadiso]

We don't need to answer all of these questions in this major version, but I think we'll eventually have to break away from this pattern as client functionality grows.

[iparadiso]

General nit: This (and other client variants) carries forward the previous multi-constructor overloads of the old clients. Consider if a builder pattern would be cleaner here to prevent a future 6th or 7th constructor.

Similar to my previous comment though can make migrations a bit busier.

[sjohnr]

This looks great @jjacobs44! Only thing I am thinking about is surface area of the API. For changes like this in OSS, I like to look for opportunities to reduce visibility, and open it up later if/when users see a need for accessing the classes and provide that feedback.

You might consider making some/all of the following classes internal:

1. DgsJsonMapperMissingException → internal — zero risk, only thrown inside the auto-config
2. Jackson3DgsJsonMapperAdapter → internal — no external instantiation found; client wiring detail
3. Jackson2DgsJsonMapper → internal — only constructed within graphql-dgs-jackson2
4. Jackson3DgsJsonMapperConfiguration inner class → internal open class — Spring auto-config inner class
5. DefaultDgsGraphQLResponse → internal (judgment call) — its KDoc invites users to construct it for tests; either commit to that or hide it

There are a few others where it would be nice to hide (e.g. Jackson2DgsJsonMapperAdapter, DgsJackson2AutoConfiguration, Jackson3DgsJsonMapper) but I believe these are needed/referenced in other places and can't be reduced.

[sjohnr]

This exception has no message. It might not produce very nice details for DX? Since you have a failure analyzer, might be a moot point but still probably would be nice to include a message.