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

Add web-flux jackson module and add check for javax.servlet package in classpath in web jackson module #6293

Closed
wants to merge 1 commit into from

Conversation

finke-ba
Copy link
Contributor

  1. Add new module WebServerJackson2Module for spring-web-flux.
    I made it because DefaultCsrfServerToken exists in two packages at the moment(web and web.server) but only one mixin is configured(for web module).
    I don't know what what to add to WebServerJackson2Module except DefaultCsrfServerTokenMixin, but in my feb-flux project that's enough.
  2. Add check for javax.servlet package in classpath in WebJackson2Module. This also applies to web-flux, since there is no javax.servlet in web-flux module.

Fixes #5623

Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @finke-ba! I have commented inline. Also...Can we split this into two commits and tickets. The first is to conditionally add servlet based support and the second is to add support for WebFlux?

context.setMixInAnnotations(SavedCookie.class, SavedCookieMixin.class);
SecurityJackson2Modules.enableDefaultTyping(context.getOwner());
if (ClassUtils.isPresent("javax.servlet.http.Cookie", this.getClass().getClassLoader())) {
context.setMixInAnnotations(Cookie.class, CookieMixin.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on the JDK this can cause issues. The reason is that many JDK's will load all the classes necessary for the entire class to run at once even if the code is not necessarily going to be executed. Instead, this code needs to be isolated in a separate module and then conditionally loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that's interesting, I didn't know that..
Is that means that some JDK(if it's fast could you give me example of that specific JDK or link to specification?) will try to load all the classes that presents in that class in initialization step of the that class? And that 'Cookie.class' will be try to load without looking at the condition?
But if I put everything in brackets into a separate class(new module) that could be load but will not instantiate, base on condition, that will work?
And that loading of all necessary classes will done only if I will try to create instance of that new module?

@finke-ba
Copy link
Contributor Author

@rwinch thank you for quick feedback!
Of cource we could split it. Should I create two new tickets and link them with #5623 or you will do it?
And after that should I create two pull requests for that tickets based on corresponding commits or what is the best option?

@finke-ba
Copy link
Contributor Author

Created two issues - #6302, #6303

@finke-ba
Copy link
Contributor Author

Created two PR - #6304, #6305.

@rwinch could you please review that?

@rwinch
Copy link
Member

rwinch commented Dec 18, 2018

Thanks! Closing in favor of the two PRs

@rwinch rwinch closed this Dec 18, 2018
@rwinch rwinch self-assigned this Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants