-
Notifications
You must be signed in to change notification settings - Fork 96
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
Support underscores in request ids. #176
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get a changelog entry for this, and hopefully a test case?
I agree with Rick's comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved - but also agree changelog at a minimum and test case would be nice.
5d73f39
to
c0c45d6
Compare
@Unroll | ||
def "The BardLoggingFilter does not add a malformed requestId '#requestId' to MDC"() { | ||
given: "A ContainerRequestContext with the specified malformed requestId" | ||
MultivaluedHashMap<String, String> fakeHeaders = new MultivaluedHashMap<>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shadows the class-level fakeHeaders
map. Do we need this declaration in this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could catch. It should be using the class level fakeHeaders
filter.filter(requestContext) | ||
|
||
then: | ||
!MDC.get(RequestLog.ID_KEY) || !MDC.get(RequestLog.ID_KEY).startsWith(requestId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could simplify this by using the null-safe coalescence operator:
!MDC.get(RequestLog.ID_KEY)?.startsWith(requestId)
If MDC.get(RequestLog.ID_KEY)
comes back null
, it returns false and doesn't call .startsWith(...)
, preventing the NPE.
Just the issue with shadowing. Once that's addressed, merge away. |
--Some clients want to have underscores in their unique request ids. Since it's kind of arbitrary what characters we allow or don't allow in the request header, we've added it.
c0c45d6
to
e29d5e3
Compare
--Some clients want to have underscores in their unique request ids.
Since it's kind of arbitrary what characters we allow or don't allow in
the request header, we've added it.