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

Redis Sample with GenericJackson2JsonRedisSerializer #434

Closed
wants to merge 23 commits into from

Conversation

jeetmp3
Copy link

@jeetmp3 jeetmp3 commented Mar 17, 2016

@rwinch here is sample web-app which includes Spring Security, Spring session with GenericJackson2JsonRedisSerializer (#406). Kindly review the code and let me know if any changes are required.

@wgorder
Copy link

wgorder commented Mar 18, 2016

@jeetmp3 Nice I have not had a chance to dig into this too much yet but I wonder how much of the default auto-configure this negates?

https://github.com/spring-projects/spring-boot/blob/b260c20df0105842218ce9788130634f9b1f5dfa/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jackson/JacksonAutoConfiguration.java

I wonder if it could be packaged up to auto-configures after the default configuration and enhance it if a conditional property is set or some such thing.

I am sure it was probably not your intention to worry about this for a sample, but I am thinking about making it reusable as well.

Thanks,

@wgorder
Copy link

wgorder commented Mar 18, 2016

Hmm nvm I see there is no @Bean on your ObjectMapper and its probably better to use a separate one anyway considering the need for default typing etc. I will have to try it out. Someone on my team just asked me for this so maybe it will save me some time, thanks!

@rwinch
Copy link
Member

rwinch commented Mar 18, 2016

@jeetmp3 Wow this is awesome! I need to spend a little more time reviewing this on Monday. Thank you for submitting this.

We may want to get some of the Jackson Support moved to Spring Security as this would be beneficial in many other contexts. I'll review on Monday and we can go from there. Thanks for this!

@rwinch
Copy link
Member

rwinch commented Mar 18, 2016

@jeetmp3 PS If contributing the Jackson Support is something that interests you, please comment on spring-projects/spring-security#3736 If we can get some tests added for the support, that will be a good start getting this into Spring Security proper.

Thanks again!

@jeetmp3
Copy link
Author

jeetmp3 commented Mar 21, 2016

@wgorder this sample doesn't use Jackson default auto-configure. I haven't focused on JacksonAutoConfiguration to configure ObjectMapper.
I think we can use Jackson2ObjectMapperBuilder to build ObjectMapper and register all the Mix-ins. By doing this we can take advantage of default auto-configure.

@jeetmp3
Copy link
Author

jeetmp3 commented Mar 21, 2016

@rwinch sounds good to me I'll comment on that. Just wanted to confirm about test cases, you want tests for mix-in classes right?

@rwinch
Copy link
Member

rwinch commented Mar 21, 2016

@jeetmp3 Thanks for the fast reply. Yes I would like to ensure we have some tests for the mix-in classes. At that point we can add the mixins to Spring Security.

@rwinch
Copy link
Member

rwinch commented Mar 21, 2016

@jeetmp3 Also...in terms of JacksonAutoConfiguration this is something we can do later. If we have the support there at least it can be done even if it is a little more work.

We will be releasing Spring Security 4.1 RC1 on Wednesday, so it would be ideal for us to get it in before that. However, that is probably not realistic so we may need to wait until another release.

@jeetmp3
Copy link
Author

jeetmp3 commented Mar 23, 2016

@rwinch I've added tests for mix-in classes.

@jeetmp3
Copy link
Author

jeetmp3 commented Jul 11, 2016

ObjectMapper configuration changed. Now all mixins will be used from pull request 3812. Mixin classes in this code sample will be removed once pull request 3812 is merged into spring-security.

@rwinch rwinch closed this in 8b97a32 Sep 12, 2016
rwinch pushed a commit that referenced this pull request Sep 12, 2016
rwinch pushed a commit that referenced this pull request Sep 12, 2016
Issue gh-434
@rwinch rwinch self-assigned this Sep 12, 2016
@rwinch rwinch added this to the 1.3.0 M1 milestone Sep 12, 2016
@rwinch rwinch added type: enhancement A general enhancement samples labels Sep 12, 2016
@rwinch rwinch added the in: docs An issue in Documentation or samples label May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: docs An issue in Documentation or samples type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants