Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register Jackson modules, based on global user configuration #704

Closed
nipafx opened this issue Jan 10, 2023 · 6 comments
Closed

Register Jackson modules, based on global user configuration #704

nipafx opened this issue Jan 10, 2023 · 6 comments

Comments

@nipafx
Copy link
Member

nipafx commented Jan 10, 2023

One way to address #624 (JSON argument source fails with LocalDate property deserialization) is for Pioneer to globally register Jackson modules based on a configuration option set by the user. The option is necessary because there are different possible behaviors (e.g. only register well-known modules vs registering all modules) that don't work equally well across all test suites.

After conversations in #624 and #629, I propose the following:

  • add configuration parameter junitpioneer.jackson.modules.registration
  • implement strategies for the values none, well_known, all

The current behavior is none and since switching to any other would be a backwards-incompatible change, I recommend to have none as default for now. Overall, I think well_known is the better default, though, so I propose we make that the default in 3.0 and already document that now.


If this proposal is accepted and implemented, a follow-up issue could add a more sophisticated option:

  • for junitpioneer.jackson.modules.registration, add the new value list
  • add new parameter junitpioneer.jackson.modules.list
  • implement a strategy that if ...modules.registration is set to list, the value of ...modules.list is interpreted as a comma-separated list of Jackson modules that will be registered
  • if ...modules.list is set and ...modules.registration is set to any other value than list, there should be a warning or even error

I only added this additional proposal here to foreshadow a more detailed option in case the three values none, well_known, all aren't flexible enough, so for this issue we can focus on implementing those three.

@nipafx nipafx changed the title Register Jackson modules Register Jackson modules, based on global user configuration Jan 10, 2023
@nipafx
Copy link
Member Author

nipafx commented Jan 10, 2023

(Not up for grabs yet. Need to discuss it first.)

@meredrica
Copy link

meredrica commented Jan 10, 2023

Another approach would be to simply allow to list the classes that should be registered. Grab the list, instanciate via reflection, crash on wrong input. This would be future proof and allows to register custom modules etc without having to go down the route of defining what well known means.

@nipafx
Copy link
Member Author

nipafx commented Jan 10, 2023

That's what I meant with the follow-up proposal (under the vertical divider).

@Bukama
Copy link
Member

Bukama commented Jan 11, 2023

I agree with @nipafx. The defensive approach with none as the default should be the way to go.

@meredrica
Copy link

That's what I meant with the follow-up proposal (under the vertical divider).

I'm sorry, I have absolutely no idea why I didn't see this.

I agree that none should be the default.

@cwensel
Copy link

cwensel commented Feb 4, 2023

I like the idea of having the list escape hatch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants