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

Fix no check if the parameter is null. #6775

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

JokerSun
Copy link
Contributor

@JokerSun JokerSun commented Apr 13, 2019

@pivotal-issuemaster
Copy link

@JokerSun Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@JokerSun Thank you for signing the Contributor License Agreement!

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.

@JokerSun, thanks for such a quick turnaround! I've left some feedback inline.

@jzheaux jzheaux self-assigned this Apr 14, 2019
@jzheaux jzheaux added in: core An issue in spring-security-core status: duplicate A duplicate of another issue type: enhancement A general enhancement labels Apr 14, 2019
@jzheaux jzheaux added this to the 5.2.0.M2 milestone Apr 14, 2019
@JokerSun
Copy link
Contributor Author

@jzheaux Thanks for your review, i guess your are right , and can you make some suggestion about strategy of 'null value' it's import for my developing, because i don't know when should i catch the null value or throw exception.

@jgrandja jgrandja modified the milestones: 5.2.0.M2, 5.2.0.RC1 Apr 15, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Apr 16, 2019

Thanks, @JokerSun for the update!

To answer your question about checking for null, remember that the more possible inputs you allow, the more you have to support. Ask yourself if it would be a normal use case to pass a null value to your method - if not, throw an IllegalArgumentException.

Regarding your PR, will you please do some cleanup before it gets merged? It looks like there are five commits, but only one line changed. Can you make so that there is only one commit? And then please format the commit this way:

AuthorityUtils Null Check

Fixes: gh-6773

Thanks!

@JokerSun
Copy link
Contributor Author

JokerSun commented Apr 17, 2019

@jzheaux I am sorry about reply later, i have clean the commits, and thanks for your suggestion very much.

@jzheaux jzheaux merged commit 19e823f into spring-projects:master Apr 18, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Apr 18, 2019

Thanks, again, @JokerSun! This has been merged into master.

@JokerSun JokerSun deleted the JokerSun branch April 20, 2019 05:51
@JokerSun JokerSun restored the JokerSun branch April 20, 2019 05:52
@JokerSun JokerSun deleted the JokerSun branch April 20, 2019 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants