Skip to content

Fix: Avoid NPE when MDC contains null values - #1385

Merged
marandaneto merged 9 commits into
getsentry:mainfrom
wreulicke:patch-1
Apr 9, 2021
Merged

Fix: Avoid NPE when MDC contains null values#1385
marandaneto merged 9 commits into
getsentry:mainfrom
wreulicke:patch-1

Conversation

@wreulicke

@wreulicke wreulicke commented Apr 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

📜 Description

Avoid NPE when MDC contains null values
Follow up #1364

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

@wreulicke wreulicke changed the title Avoid NPE when MDC contains null values Fix: Avoid NPE when MDC contains null values Apr 8, 2021
@marandaneto

marandaneto commented Apr 8, 2021

Copy link
Copy Markdown
Contributor

@wreulicke why is it a follow-up of #1364 ? doesn't #1364 fix the issue at all?

@wreulicke

wreulicke commented Apr 8, 2021

Copy link
Copy Markdown
Contributor Author

@marandaneto
To be exact, This fixes the issues similar to #1364.
follow up might not be accurate.

marandaneto
marandaneto reviewed Apr 8, 2021
View reviewed changes
Comment thread CHANGELOG.md Outdated
@marandaneto

marandaneto commented Apr 8, 2021

Copy link
Copy Markdown
Contributor

@marandaneto
To be exact, This fixes the issues similar to #1364.
follow up might not be accurate.

I just noticed its pretty much the same fix but for sentry-jul, that was the issue, I thought you were just fixing the same issue for sentry-logback :)

mind writing a unit test? thanks

wreulicke and others added 3 commits April 8, 2021 21:39
@wreulicke @marandaneto
Update CHANGELOG.md
aa67f39 
Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
@wreulicke
Avoid NPE properly
7dec970
@wreulicke
Add test to check avoiding NPE
cbe4ae4
@codecov-io

codecov-io commented Apr 9, 2021

Copy link
Copy Markdown

Codecov Report

Merging #1385 (cbe4ae4) into main (142dd44) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1385      +/-   ##
============================================
+ Coverage     75.68%   75.72%   +0.03%     
- Complexity     1858     1861       +3     
============================================
  Files           185      185              
  Lines          6363     6368       +5     
  Branches        636      637       +1     
============================================
+ Hits           4816     4822       +6     
  Misses         1258     1258              
+ Partials        289      288       -1
Impacted Files Coverage Δ Complexity Δ
...jul/src/main/java/io/sentry/jul/SentryHandler.java 75.00% <100.00%> (+1.89%) 34.00 <2.00> (+3.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 142dd44...cbe4ae4. Read the comment docs.

@wreulicke

wreulicke commented Apr 9, 2021

Copy link
Copy Markdown
Contributor Author

@marandaneto

I have written the unit test.
Could you check this PR?

marandaneto
marandaneto approved these changes Apr 9, 2021
View reviewed changes

@marandaneto marandaneto left a comment

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.

LGTM

@marandaneto

marandaneto commented Apr 9, 2021

Copy link
Copy Markdown
Contributor

@marandaneto

I have written the unit test.
Could you check this PR?

all good, thanks for the contribution @wreulicke

@marandaneto marandaneto mentioned this pull request Apr 9, 2021
Closed
@marandaneto
marandaneto merged commit 9d8b774 into getsentry:main Apr 9, 2021
@wreulicke
wreulicke deleted the patch-1 branch August 5, 2021 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maciejwalkowiak maciejwalkowiak Awaiting requested review from maciejwalkowiak
1 more reviewer
@marandaneto marandaneto marandaneto approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wreulicke @marandaneto @codecov-io