Add Jackson Support for Objects stored in HttpSession #3812

Closed
wants to merge 64 commits into
from

Conversation

Projects
None yet
6 participants
@jeetmp3
Contributor

jeetmp3 commented Apr 14, 2016

Hi @rwinch sorry it's got delayed. I've created mix-ins to add Jackson support(#3736).
Please review the code and let me know feedback. I've modified some existing classes

  1. DefaultSavedRequest added builder class with private constructor
  2. WebAuthenticationDetails added one private constructor to exclude HttpServletRequest dependency in Jackson deserialization.
  3. CasAuthenticationToken, RememberMeAuthenticationToken, AnonymousAuthenticationToken added new private constructor to ensure keyHash correctly deserialized and added static methods in AbstractAuthenticationToken which helps in constructing these *AuthenticationToken implementations

PS: I havn't update Apache Licence yet. Need your help in updating the same

  • I have signed the CLA

jitendra-bisht added some commits Mar 30, 2016

Mix-ins added for AssertionImpl and AttributePrincipalImpl classes us…
…ed in CasAuthenticationToken jackson deserialization

@rwinch rwinch modified the milestones: 4.1.0 RC2, 4.2.0 M1 Apr 15, 2016

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Apr 18, 2016

Member

Thanks for the PR!

The more I think about this the more I am uncomfortable merging this so close to GA. There are a large amount of changes in this PR and I do not think I will be able to give this a proper review by Wednesday (the release). For this reason, I'm bumping this to 4.2

To be clear...I'm sad that this is getting bumped as I know it will be of great value to users. However, I know that it is more important we get a quality release out of this than trying to force this feature in.

One thing we will want to ensure is that we add documentation to the reference to use this. We can wait for the documentation until this feature is reviewed, but we will need this before this is "production ready".

Member

rwinch commented Apr 18, 2016

Thanks for the PR!

The more I think about this the more I am uncomfortable merging this so close to GA. There are a large amount of changes in this PR and I do not think I will be able to give this a proper review by Wednesday (the release). For this reason, I'm bumping this to 4.2

To be clear...I'm sad that this is getting bumped as I know it will be of great value to users. However, I know that it is more important we get a quality release out of this than trying to force this feature in.

One thing we will want to ensure is that we add documentation to the reference to use this. We can wait for the documentation until this feature is reviewed, but we will need this before this is "production ready".

@jeetmp3

This comment has been minimized.

Show comment
Hide comment
@jeetmp3

jeetmp3 Apr 19, 2016

Contributor

@rwinch thanks for the response! I agree we should focus on quality. Let me know when you done with code review, meanwhile i'll start documentation.

Contributor

jeetmp3 commented Apr 19, 2016

@rwinch thanks for the response! I agree we should focus on quality. Let me know when you done with code review, meanwhile i'll start documentation.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Jul 15, 2016

It is your project, but I just threw in my 50 cents here. 24 "real" files changes still sounds like a lot. I understand that Jackson is "defacto JSON" in Java, but it would be nice if I had this great "fast Johnson JSON serrializer" going on (I have not) that I could benifit from this PR and not having to add 24 more classes. Hiding it behind GenericJackson2JsonRedisSerializer wouldn't help, either -- because it's not abstract enough. It says so in its name.

But you get the point, I will quiet down now.

bep commented Jul 15, 2016

It is your project, but I just threw in my 50 cents here. 24 "real" files changes still sounds like a lot. I understand that Jackson is "defacto JSON" in Java, but it would be nice if I had this great "fast Johnson JSON serrializer" going on (I have not) that I could benifit from this PR and not having to add 24 more classes. Hiding it behind GenericJackson2JsonRedisSerializer wouldn't help, either -- because it's not abstract enough. It says so in its name.

But you get the point, I will quiet down now.

@jeetmp3

This comment has been minimized.

Show comment
Hide comment
@jeetmp3

jeetmp3 Aug 9, 2016

Contributor

Thanks for the feedback!
I've finish all the changes mentioned in comments and also updated Spring Session Redis Sample.

Contributor

jeetmp3 commented Aug 9, 2016

Thanks for the feedback!
I've finish all the changes mentioned in comments and also updated Spring Session Redis Sample.

@rwinch rwinch modified the milestone: 4.2.0 M1 Aug 15, 2016

@rwinch rwinch closed this in d77ca17 Sep 2, 2016

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Sep 2, 2016

Member

Thanks for the PR!

This is now merged into master via d77ca17

You will notice a series of commits 8ad0003 that perform some polish on the Pull Request you submitted. If you are able, please review my changes.

Member

rwinch commented Sep 2, 2016

Thanks for the PR!

This is now merged into master via d77ca17

You will notice a series of commits 8ad0003 that perform some polish on the Pull Request you submitted. If you are able, please review my changes.

@driverpt

This comment has been minimized.

Show comment
Hide comment
@driverpt

driverpt Oct 4, 2016

@rwinch , can this be backported to Spring Security?

driverpt commented Oct 4, 2016

@rwinch , can this be backported to Spring Security?

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Oct 4, 2016

Member

@driverpt Spring Security 4.2 will be out very soon, so I don't think we will be back porting it. What version do you want it to be back ported to?

Member

rwinch commented Oct 4, 2016

@driverpt Spring Security 4.2 will be out very soon, so I don't think we will be back porting it. What version do you want it to be back ported to?

@driverpt

This comment has been minimized.

Show comment
Hide comment
@driverpt

driverpt Oct 4, 2016

@rwinch , thanks for the quick reply. I'm currently using the one used in Spring Boot (4.1.1). Is 4.2.0 M1 a good drop-in replacement for 4.1.1 in Spring Boot ?

driverpt commented Oct 4, 2016

@rwinch , thanks for the quick reply. I'm currently using the one used in Spring Boot (4.1.1). Is 4.2.0 M1 a good drop-in replacement for 4.1.1 in Spring Boot ?

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Oct 4, 2016

Member

@driverpt Yes it is. You can also just copy paste the necessary classes into your project until Spring Security goes GA. Keep in mind we are making a change to how things are done in order to support newer versions of Jackson. See #4073

Member

rwinch commented Oct 4, 2016

@driverpt Yes it is. You can also just copy paste the necessary classes into your project until Spring Security goes GA. Keep in mind we are making a change to how things are done in order to support newer versions of Jackson. See #4073

@driverpt

This comment has been minimized.

Show comment
Hide comment
@driverpt

driverpt Oct 4, 2016

Problem is, Spring Boot uses Jackson 2.8.1

driverpt commented Oct 4, 2016

Problem is, Spring Boot uses Jackson 2.8.1

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Oct 4, 2016

Member

@driverpt Right. See the PR #4078 that works around the related Jackson regression

Member

rwinch commented Oct 4, 2016

@driverpt Right. See the PR #4078 that works around the related Jackson regression

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