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

Consider restructuring Spring Session configuration properties #17397

Open
vpavic opened this issue Jul 2, 2019 · 7 comments
Open

Consider restructuring Spring Session configuration properties #17397

vpavic opened this issue Jul 2, 2019 · 7 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@vpavic
Copy link
Contributor

vpavic commented Jul 2, 2019

This is a follow-up on #17278, which handled changes to flush mode related configuration properties. More changes in configuration support infrastructure are expected for Spring Session Corn-M3, and when that's in place it might be good idea to revisit the overall structure of Spring Session related configuration properties.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 2, 2019
@wilkinsona
Copy link
Member

One change that we've already identified is to introduce spring.session.servlet.<store>.* properties where there's a store-specific configuration setting that applies to the Servlet web stack and not to the reactive web stack.

@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 2, 2019
@wilkinsona wilkinsona added this to the 2.2.x milestone Jul 2, 2019
@wilkinsona wilkinsona added the status: blocked An issue that's blocked on an external project change label Jul 8, 2019
@snicoll
Copy link
Member

snicoll commented Jul 9, 2019

Unless I misunderstood your comment @wilkinsona, it feels inconsistent to me to introduce the servlet prefix only if necessary. Shouldn't we harmonize and have a servlet and reactive for everything, excluding reactive for store that may not work (yet?) with the reactive stack?

@wilkinsona
Copy link
Member

I'd envisaged three "sets" of properties:

  • spring.session.<store>.* for store-specific configuration that applies to both web stacks
  • spring.session.servlet.<store>.* for store-specific configuration that only applies to the Servlet web stack
  • spring.session.reactive.<store>.* for store-specific configuration that only applies to the reactive web stack

@snicoll
Copy link
Member

snicoll commented Jul 9, 2019

Thanks for the feedback. Considering that we may add things in one stack or the other (or both) I think I prefer going with the consistency to not have the first case at all and just duplicate configuration options in servlet and reactive when both are available.

@vpavic
Copy link
Contributor Author

vpavic commented Jul 16, 2019

I agree with what @snicoll outlined - that was also my original understanding from #17278 discussion.

Omitting stack prefix seems risky in terms of breaking users, as it's not always easy to know whether a SessionRepository implementation will adopt some feature, and if it does at what pace compared to other implementations.

@snicoll snicoll added for: team-attention An issue we'd like other members of the team to review and removed for: team-attention An issue we'd like other members of the team to review status: blocked An issue that's blocked on an external project change labels Jul 16, 2019
@snicoll
Copy link
Member

snicoll commented Jul 19, 2019

We've discussed this one this week and the plan is to go with a store specific configuration and keep the existing properties in 2.2 in a deprecated fashion. For stores that only support servlet, a replacement can be provided right away. However, for stores that support both, we need some logic and the user will have to choose explicitly.

Looking at the current package structure, introducing additional types do not spark joy so I am considering moving things in a servlet and reactive sub-packages.

@snicoll
Copy link
Member

snicoll commented Jul 22, 2019

for stores that support both, we need some logic

Unfortunately, it's not that easy. If we have both a spring.session.redis.namespace and a spring.session.reactive.redis.namespace, we need to keep track of the default value to determine if it was set or not as just checking for null isn't enough. That isn't perfect either in case the user has set a property explicitly with the default value and a better way would be to track binding by checking if the setter was called. This seems a lot of logic to me.

@philwebb philwebb assigned philwebb and unassigned philwebb Aug 30, 2019
@philwebb philwebb modified the milestones: 2.2.x, 2.x Aug 30, 2019
@philwebb philwebb self-assigned this Aug 30, 2019
@philwebb philwebb modified the milestones: 2.x, 3.x May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants