Cleanup RealmContextFilter #2747
Conversation
Summary of changes: 1. Remove the call to ContextLocals (context: apache#2720 (comment)). 2. Don't include the exception's message in the response as it can leak details about Polaris internals. 3. Add a small test for success and failure cases.
snazy
left a comment
snazy
left a comment
There was a problem hiding this comment.
LGTM
just a few now failing test assertions
Whoops there were tests for that already 😅 I didn't know. I will merge the two test classes together. |
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
|
||
| @Test | ||
| public void testDefaultRealm() { | ||
| givenTokenRequest("client1", "secret1") |
There was a problem hiding this comment.
What's the difference between this test and the one above it? Per what I'm seeing, it's the exact same request for different users who both have correct credentials as per L51.
I don't see the require-header variable changing between the two so I'm confused whether this is testing the default header properly or not, as the test names suggest? And if the default header was triggering the IllegalArgumentException, wouldn't that be a 400 Bad Request error code rather than Unauthorized?
There was a problem hiding this comment.
The behavior is different because the default realm is realm1.
In the original test class there were comments explaining this, which I forgot to port over. I will add them now.
| .onItem() | ||
| .invoke(realmContext -> rc.setProperty(REALM_CONTEXT_KEY, realmContext)) | ||
| // ContextLocals is used by RealmIdTagContributor to add the realm id to metrics | ||
| .invoke(realmContext -> ContextLocals.put(REALM_CONTEXT_KEY, realmContext)) |
There was a problem hiding this comment.
Trying to understand why RealmIdTagContributor cannot read from the ContainerRequestContext instead of relying on ContextLocals to pass around any object needed for a HttpServerMetricsTagsContributor. Or would it be reasonable to copy the whole rc into a ContextLocals by default? So that any operation on the ContextLocals can use the ContainerRequestContext. Would you mind sharing your thought on it?
adnanhemani
left a comment
adnanhemani
left a comment
There was a problem hiding this comment.
LGTM - thanks @adutra!
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
|
Thanks a lot for working on it @adutra ! Thanks @snazy @dimas-b @adnanhemani for the review! |
5 participants
Summary of changes:
Remove the call to.ContextLocals(context: Extract interface for RequestIdGenerator #2720 (comment))UPDATE: we cannot remove
ContextLocals, it's used byRealmIdTagContributor.