Skip to content

Remove redundant branches from SessionManagementConfigurer #7879

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

Closed
wants to merge 1 commit into from

Conversation

OrangeDog
Copy link
Contributor

I've complained about the style in which these are written before, but this was a prime example of how it over-complicates the control flow, leading to (in this case thankfully not dangerous) errors.

Making use of else and && would simplify this, and other configuration code, even further.
For this case something like this would be best, but I've had changes like this rejected before.

if (this.invalidSessionStrategy == null && this.invalidSessionUrl != null) {
    this.invalidSessionStrategy = new SimpleRedirectInvalidSessionStrategy(
            this.invalidSessionUrl);
}
return this.invalidSessionStrategy;

Copy link
Contributor

@jzheaux jzheaux 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 pull request, @OrangeDog. I've left a bit of feedback.

If you rebase, I believe you'll see that failing integration test turn green.

@@ -561,13 +561,6 @@ InvalidSessionStrategy getInvalidSessionStrategy() {
this.invalidSessionStrategy = new SimpleRedirectInvalidSessionStrategy(
this.invalidSessionUrl);
}
if (this.invalidSessionUrl == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@OrangeDog Thanks for looking into this.

Taking a look at getExpiredSessionStrategy below, it looks like there may be an opportunity for cleanup there as well.

What would you think of making these two methods look the same, something like:

if (strategy != null) {
    return strategy;
}

if (url == null) {
    return null;
}

strategy = // get the default strategy
return strategy;

I believe this would get rid of the dead branches as well as keep each if statement simple.

@OrangeDog OrangeDog changed the title Remove dead code in getInvalidSessionStrategy Remove redundant branches from SessionManagementConfigurer Jan 31, 2020
@jzheaux jzheaux self-assigned this Jan 31, 2020
@jzheaux jzheaux added in: config An issue in spring-security-config type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 31, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Jan 31, 2020

Thanks, @OrangeDog, this is now merged into master via ee6df17.

Note that I locally rebased the branch and then updated the commit to comply with our commit formatting guidelines.

@jzheaux jzheaux closed this Jan 31, 2020
@OrangeDog OrangeDog deleted the patch-4 branch February 4, 2020 17:10
@jzheaux jzheaux added this to the 5.3.0.RC1 milestone Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants