- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.7k
Fix NPE in MDCContextMap #430
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
Conversation
Accomodate for the fact that MDC.getCopyOfContextMap() may return null
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.
Thanks, the change looks good. Would you mind adding a test that fails without the fix to prevent regressions?
LOG4J2-2939;
| Thanks! I'll handle the merge and cherry-pick to relevant release branches. | 
| Thank you very much! | 
| assertNull("Setup wrong", MDC.getCopyOfContextMap()); | ||
| assertTrue(ThreadContext.isEmpty()); | ||
| assertFalse(ThreadContext.containsKey("something")); | ||
| } | 
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.
I would separate these into two tests, one for each API.
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.
An edge case worth testing IMO is getting a null key.
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.
Thank you for the feedback! I split the tests and added some tests concerning null keys.
LOG4J2-2939;
Accomodate for the fact that MDC.getCopyOfContextMap() may return null
PR for https://issues.apache.org/jira/browse/LOG4J2-2939
Accomodate for the fact that MDC.getCopyOfContextMap() may return null.