Fix potential NPE by separating transaction contexts. #1164
Conversation
|
@marandaneto there are few tests to add there, but before I put more time into it please take a look if you believe we should go this direction |
Codecov Report
@@ Coverage Diff @@
## main #1164 +/- ##
============================================
- Coverage 75.25% 74.83% -0.43%
- Complexity 1662 1681 +19
============================================
Files 173 176 +3
Lines 5812 5912 +100
Branches 568 583 +15
============================================
+ Hits 4374 4424 +50
- Misses 1174 1216 +42
- Partials 264 272 +8
Continue to review full report at Codecov.
|
| @@ -97,6 +97,9 @@ | |||
| /** List of breadcrumbs recorded before this event. */ | |||
| private List<Breadcrumb> breadcrumbs; | |||
|
|
|||
| /** Contexts describing the environment (e.g. device, os or browser). */ | |||
| private final @NotNull Contexts contexts = new Contexts(); | |||
There was a problem hiding this comment.
in theory, this is a Nullable field.
There was a problem hiding this comment.
This has been changed to not-nullable in previous PR.
| JsonElement json, Type typeOfT, JsonDeserializationContext context) | ||
| throws JsonParseException { | ||
| try { | ||
| if (json != null && !json.isJsonNull()) { |
There was a problem hiding this comment.
should we extract the content of this method to a Util class and reuse it in ContextsDeserializerAdapter and ContextsSerializerAdapter? they look pretty much the same.
There was a problem hiding this comment.
I'll see what i can do - they don't share the same class hierarchy, at least not at the moment.
| } | ||
|
|
||
| final JsonObject object = new JsonObject(); | ||
| object.add(SpanContext.TYPE, context.serialize(src.getTrace(), Object.class)); |
There was a problem hiding this comment.
ContextsDeserializerAdapter uses SpanContext.TYPE and apparently after this PR, it'd not be necessary anymore, right?
There was a problem hiding this comment.
It will be, as SpanContext (aka "trace") is also a part of events.
|
@marandaneto if we go this path, perhaps it makes sense to change also |
| import org.jetbrains.annotations.NotNull; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| public final class TransactionContexts { |
There was a problem hiding this comment.
while I agree that this would avoid all sort of NPE aroundContexts and trace, looks like a junk of code duplication, wondering if just moving the Contexts inheritance away and adding an other field in there would not solve the problem for good (not adding a new class TransactionContexts).
Also, Contexts would need to become @NotNull by default on SentryBaseEvent.
maybe @bruno-garcia would like to chime in
There was a problem hiding this comment.
Then we would eliminate in practical terms the possibility of NPE but we won't get away with warnings in the IDE.
|
Not valid anymore. |
📜 Description
Fix potential NPE by separating transaction contexts from event contexts.
💡 Motivation and Context
Even though event contexts look similar to transaction contexts, there are different nullability constraints - with transactions, its guaranteed that trace cannot be null. With this change, we fix potential NPE reported in #1160.
💚 How did you test it?
Unit tests.
📝 Checklist
🔮 Next steps
Perhaps change the
Contextsclass to also not inherit from the HashMap?