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

Use LoadingCache in CachingAuthorizer #2096

Merged
merged 1 commit into from Jul 5, 2017

Conversation

Projects
None yet
3 participants
@FredDeschenes
Contributor

FredDeschenes commented Jul 5, 2017

As done in #1615 for CachingAuthenticator. Since Authorizer::authorize doesn't have a throws clause, I've only handled UncheckedExecutionExceptions.

Result when null is returned by the authorizer will differ from previously, where a NullPointerException would have been thrown by the cache's put method, whereas now a InvalidCacheLoadException will be thrown by the cache itself when the loader returns null. There might be a way to have the same result as previously, let me know if you want me to look into it (but personally I don't think it really matters as most users won't be catching those exceptions anyway, and it might even be clearer as to what happened).

EDIT: Looking at this again, Authorizer::authorize returns a boolean and not a Boolean so null returns shouldn't happen anyway.

@coveralls

This comment has been minimized.

coveralls commented Jul 5, 2017

Coverage Status

Coverage decreased (-0.01%) to 84.949% when pulling 197f121 on FredDeschenes:caching-authorizer-loadingcache into 8dfc67c on dropwizard:master.

@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Jul 5, 2017

Coverage went down because this line is unreachable but needed for compilation (since Throwables.propagate is now deprecated). Not much I can do I think.

@arteam

arteam approved these changes Jul 5, 2017

Looks good to me. Thank you very much for refactoring this! I've added a small suggestions regarding the unit test.

public void shouldPropagateRuntimeException() throws AuthenticationException {
final RuntimeException e = new NullPointerException();
when(underlying.authorize(principal, role)).thenThrow(e);
expected.expect(CoreMatchers.sameInstance(e));

This comment has been minimized.

@arteam

arteam Jul 5, 2017

Member

I think we could use AssertJ's exception assertion, which looks concise and provide a more wide range of assertions than the ExpectedException rule. Something like that:

assertThatNullPointerException()
            .isThrownBy(() -> cached.authorize(principal, role))
            .isSameAs(e);

This comment has been minimized.

@FredDeschenes

FredDeschenes Jul 5, 2017

Contributor

Oh that's nice, I didn't even know it existed! I'll change this and update the PR.

@arteam arteam added the improvement label Jul 5, 2017

@arteam arteam added this to the 1.2.0 milestone Jul 5, 2017

Use LoadingCache in CachingAuthorizer
As done in #1615 for CachingAuthenticator

@FredDeschenes FredDeschenes force-pushed the FredDeschenes:caching-authorizer-loadingcache branch from 197f121 to 9fc9e4f Jul 5, 2017

@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Jul 5, 2017

No problem, I was looking at the code for CachedAuthorizer and CachedAuthenticator and noticed the authorizer hadn't been updated.

I've pushed a new commit with the modification to the unit test.

@coveralls

This comment has been minimized.

coveralls commented Jul 5, 2017

Coverage Status

Coverage decreased (-0.01%) to 84.949% when pulling 9fc9e4f on FredDeschenes:caching-authorizer-loadingcache into 8dfc67c on dropwizard:master.

@arteam arteam merged commit 00ec17f into dropwizard:master Jul 5, 2017

3 of 4 checks passed

coverage/coveralls Coverage decreased (-0.01%) to 84.949%
Details
codeclimate no new or fixed issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arteam

This comment has been minimized.

Member

arteam commented Jul 5, 2017

Thanks for your contribution!

@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Jul 5, 2017

You're welcome!

@FredDeschenes FredDeschenes deleted the FredDeschenes:caching-authorizer-loadingcache branch Jul 5, 2017

sankate pushed a commit to sankate/dropwizard that referenced this pull request Nov 21, 2017

aaanders added a commit to aaanders/dropwizard that referenced this pull request Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment