Skip to content

Fix regression introduced by #11008#11013

Merged
suneet-s merged 2 commits intoapache:masterfrom
samarthjain:fix11012
Mar 20, 2021
Merged

Fix regression introduced by #11008#11013
suneet-s merged 2 commits intoapache:masterfrom
samarthjain:fix11012

Conversation

@samarthjain
Copy link
Copy Markdown
Contributor

Fixes #11012 .

This PR has:

  • [X ] been self-reviewed.
    • [X ] using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
  • been tested in a test Druid cluster.

if (authorizer instanceof AllowAllAuthorizer) {
return resources;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we add a test to prevent someone accidentally making this addition in the future. I read the previous patch and thought this was a good optimization too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call, @suneet-s . I added a unit test in the new commit and in the process I found that we can still skip authorizing the resources when AllowAllAuthorizer is configured.

@jihoonson jihoonson added the Bug label Mar 19, 2021
…on when AllowAllAuthorizer is configured.

Add a unit test to validate that the change doesn't introduce new behavior.
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s left a comment

Choose a reason for hiding this comment

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

Nice! LGTM

Maybe we should wait for @abhishekagarwal87 to take a look too, since he spotted the regression in the previous patch.

@abhishekagarwal87
Copy link
Copy Markdown
Contributor

LGTM

@suneet-s suneet-s merged commit 5fae7df into apache:master Mar 20, 2021
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix regression introduced by #11008

5 participants