fix(sentry-java): Contexts belong on the Scope

[maciejwalkowiak]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring

📜 Description

Adds Contexts to scope together with deep cloning for known Cloneable objects.

💡 Motivation and Context

Continuation of #240.

💚 How did you test it?

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• All tests passing
• No breaking changes

🔮 Next steps

[marandaneto]

@maciejwalkowiak thanks for doing this, I left a few comments and it's looking good.

my main point here is, Contexts is cloned but not used, wondering if we could do the merging of contexts on this PR or to raise a new one, but I'd say that both PRs should land together.

if you look at DefaultAndroidEventProcessor, it accesses the event.getContexts() and we'd need to check if the scope has that value and also which one has precedence over it, event or scope?
eg: event.contexts.device is not null, so do we keep as it is or use scope.contexts.device?
or even event.contexts.device.bla exists but also scope.contexts.device.bla, which one wins? etc.

[maciejwalkowiak]

Another thing I have in mind if we should implement Cloneable interface and use clone method at all. If we go instead with our own custom Cloneable<T> interface we could let library users to implement it in their custom objects that are put to Contexts and we wouldn't be limited to clone only our well known objects. It's also generally not recommended to use implement Java Cloneable interfaces.

[maciejwalkowiak]

@marandaneto regarding actually using Scope#contexts - your and @bruno-garcia guidance is welcome here. I believe we should use it in SentryClient#applyScope like:

if (event.getContexts().isEmpty()) {
  event.setContexts(scope.getContexts());
}

but I might have missed something from the bigger picture.

[marandaneto]

Another thing I have in mind if we should implement Cloneable interface and use clone method at all. If we go instead with our own custom Cloneable<T> interface we could let library users to implement it in their custom objects that are put to Contexts and we wouldn't be limited to clone only our well known objects. It's also generally not recommended to use implement Java Cloneable interfaces.

that sounds a good idea, as I said, the Cloneable is just a marker interface anyway, as we're cloning the list/map etc manually.
if you create a Cloneable<T>, we'd need to fix all the clone methods and copy field by field.

[marandaneto]

@marandaneto regarding actually using Scope#contexts - your and @bruno-garcia guidance is welcome here. I believe we should use it in SentryClient#applyScope like:

if (event.getContexts().isEmpty()) {
  event.setContexts(scope.getContexts());
}

but I might have missed something from the bigger picture.

yep, this would be moved either to SentryClient or MainEventProcessor, but a note here is, in order to read some device context, we need to have the Android Context and that's why we have this DefaultAndroidEventProcessor.
So right now DefaultAndroidEventProcessor takes an event and mutates it in there, I believe this should change now for something like DefaultAndroidEventProcessor returns a Map with the things we need from Android and then either SentryClient or MainEventProcessor set to the event respecting the precedence of the event or scope

[bruno-garcia]

Please be aware:

if (event.getContexts().isEmpty()) {
  event.setContexts(scope.getContexts());
}

The values copied must be cloned. Otherwise while the event is queued for submission, the scope by be changed, affecting that event. The only references they'd share is any user-defined one, which we were not able to clone with our own deep cloning strategy (i.e Reference types in Extra and Contexts)

[maciejwalkowiak]

There's a PR to this PR: maciejwalkowiak#1

[maciejwalkowiak]

@marandaneto regarding actually using Scope#contexts - your and @bruno-garcia guidance is welcome here. I believe we should use it in SentryClient#applyScope like:

if (event.getContexts().isEmpty()) {
  event.setContexts(scope.getContexts());
}

but I might have missed something from the bigger picture.

yep, this would be moved either to SentryClient or MainEventProcessor, but a note here is, in order to read some device context, we need to have the Android Context and that's why we have this DefaultAndroidEventProcessor.
So right now DefaultAndroidEventProcessor takes an event and mutates it in there, I believe this should change now for something like DefaultAndroidEventProcessor returns a Map with the things we need from Android and then either SentryClient or MainEventProcessor set to the event respecting the precedence of the event or scope

What if we add this to SentryClient#applyScope before preprocessors do the work:

for (Map.Entry<String, Object> entry : scope.getContexts().clone().entrySet()) {
  if (!event.getContexts().containsKey(entry.getKey())) {
    event.getContexts().put(entry.getKey(), entry.getValue());
  }
}

DefaultAndroidEventProcessor will set Device and Operating system only if these properties are missing. This would mean that the contexts on SentryEvent take precedence over the contexts on Scope which takes precedence over what is applied in DefaultAndroidEventProcessor. What do you think about this logic @marandaneto?

[bruno-garcia]

DefaultAndroidEventProcessor will set Device and Operating system only if these properties are missing. This would mean that the contexts on SentryEvent take precedence over the contexts on Scope which takes precedence over what is applied in DefaultAndroidEventProcessor. What do you think about this logic @marandaneto?

Makes sense to me.

[bruno-garcia]

Approving but let's see what @marandaneto has to say

[marandaneto]

Approving but let's see what @marandaneto has to say

its LGTM for me too, well done @maciejwalkowiak