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.

Sign up for GitHub

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

Jump to bottom

[SHIRO-778] onInit method on AuthenticatingRealm is called twice #240

Merged
merged 2 commits into from
Jul 15, 2020
Merged

[SHIRO-778] onInit method on AuthenticatingRealm is called twice #240

merged 2 commits into from
Jul 15, 2020

Conversation

fpapon
Copy link
Member

@fpapon fpapon commented Jun 14, 2020

No description provided.

@fpapon fpapon requested a review from bdemers June 14, 2020 19:53
@fpapon fpapon force-pushed the SHIRO-778 branch 3 times, most recently from 54fe26e to b43c8af Compare June 15, 2020 19:21
@fpapon
Copy link
Member Author

fpapon commented Jun 15, 2020

@bdemers I made some polish, I think it's better now.

@bmarwell bmarwell self-requested a review June 15, 2020 20:12
bmarwell
bmarwell requested changes Jun 15, 2020
View reviewed changes
Copy link
Contributor

@bmarwell bmarwell left a comment

Choose a reason for hiding this comment

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

Just a little detail left if you don't mind

config/ogdl/src/main/java/org/apache/shiro/config/ogdl/ReflectionBuilder.java Outdated Show resolved Hide resolved
@bdemers
Copy link
Member

bdemers commented Jun 15, 2020

Looks good,! it would be great to have a test for this one. (possibly just tweaking the existing test InitializableBean to count the number of times init() was called)

@bmarwell bmarwell self-requested a review June 16, 2020 05:13
@bmarwell
Copy link
Contributor

Looks good,! it would be great to have a test for this one. (possibly just tweaking the existing test InitializableBean to count the number of times init() was called)

Or just create a mock objects and see if values() was called using an appropriate verify() method. This way you would not need to change production code for tests.

@fpapon fpapon force-pushed the SHIRO-778 branch 2 times, most recently from bf7837e to 0bca45b Compare July 4, 2020 20:52
@fpapon fpapon requested a review from bmarwell July 4, 2020 20:59
bmarwell
bmarwell requested changes Jul 5, 2020
View reviewed changes
Copy link
Contributor

@bmarwell bmarwell left a comment

Choose a reason for hiding this comment

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

one testcase missing, see description. It is similar to my previous one, but in mine, null was called after the defined objects, whereas we should have a "correct" test case where we call the method with null first (which is what actually happens in shiro).

config/ogdl/src/test/groovy/org/apache/shiro/config/ogdl/ReflectionBuilderTest.groovy Outdated Show resolved Hide resolved
@bmarwell
Copy link
Contributor

bmarwell commented Jul 6, 2020

LGTM 👍
@bdemers WDYT?

@fpapon fpapon merged commit acef145 into apache:master Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmarwell bmarwell bmarwell approved these changes

@bdemers bdemers bdemers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fpapon @bdemers @bmarwell