Bugfix Null Safety and Code Consistency: Refactoring getAuthorizedClients(WebSession session)

[dukbong]

Original Code:

The getAuthorizedClients method retrieves a map of OAuth2 authorized clients from a WebSession. In the original code, if the session is null, the method assigns a default empty HashMap. This code does not explicitly assert that WebSession cannot be null, potentially leading to silent failures or unexpected behavior.

Closes #14975

Suggested Refactoring:

To ensure robustness and improve error handling, you can refactor the method by adding an assertion to prevent a null session. This approach uses Assert.notNull to raise an exception with a clear error message if the WebSession is null, helping developers quickly identify and fix the problem.

[sjohnr]

Hi @dukbong, thanks for the PR!

Just a heads up, I closed the original issue with this explanation. I appreciate what you're doing in this change by asserting the session cannot be null. I'm wondering though if this change is absolutely necessary?

This code does not explicitly assert that WebSession cannot be null, potentially leading to silent failures or unexpected behavior.

Can you explain the situation in which this would occur (other than the invalid configuration referred to in the original issue)?

[dukbong]

Hi @dukbong, thanks for the PR!

Just a heads up, I closed the original issue with this explanation. I appreciate what you're doing in this change by asserting the session cannot be null. I'm wondering though if this change is absolutely necessary?

This code does not explicitly assert that WebSession cannot be null, potentially leading to silent failures or unexpected behavior.

Can you explain the situation in which this would occur (other than the invalid configuration referred to in the original issue)?

Hi @sjohnr

Initially, I planned to submit a pull request to address the issue, but as I reviewed the code, I found the structure to be questionable, prompting a change in direction.

The getAuthorizedClient function is written with the assumption that the session might be null.
However, if the session is null, this leads to an exception in the saveAuthorizedClient function.
To prevent this, I believe it's better to ensure the session isn't null either in the getAuthorizedClient function or in the doOnSuccess block.

Based on these observations, I created a pull request to address the issue.

• Your feedback is invaluable for my growth and learning.

[sjohnr]

Thanks for the PR @dukbong. In addition to the feedback inline below, can you please add a test that asserts an IllegalArgumentException is thrown when WebSession is null?

[dukbong]

Thanks for the PR @dukbong. In addition to the feedback inline below, can you please add a test that asserts an IllegalArgumentException is thrown when WebSession is null?

Thanks @sjohnr for agreeing with my feedback.
I will make the changes and additions as per the comments below!

[sjohnr]

Thanks @dukbong. I added one minor comment below. Can you also please rebase your changes on main and squash commits? I will schedule this to be merged in the next minor so you won't see it merged for a few weeks.

[dukbong]

Thanks @dukbong. I added one minor comment below. Can you also please rebase your changes on main and squash commits? I will schedule this to be merged in the next minor so you won't see it merged for a few weeks.

Hello. @sjohnr

As you suggested, I rebased onto the main branch and squashed the commits into one.

If there's anything I did wrong or need to correct, please let me know!
I'll fix it right away!!