fix: Discard Sessions when JSON is faulty

[philipphofmann]

📜 Description

When the JSON contains a started that can't be converted to an NSDate the SDK sets
the _started to nil, which leads to a crash later as _started is nonnull. This is fixed now
by returning nil when the JSON passed to initWithJSONObject contains an error.

💡 Motivation and Context

Fixes #937

💚 How did you test it?

Unit tests and SentrySessionGeneratorTests.

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the CHANGELOG if needed
• I updated the docs if needed
• Review from the native team if needed
• No breaking changes

🔮 Next steps

[marandaneto]

M: on Java we'd drop the session though, as started is a mandatory field, and if it has an invalid started-timestamp, duration can't be calculated properly as well.

[philipphofmann]

I would also like to do that, but we can't return nil as this would be a breaking change. See:

sentry-cocoa/Sources/Sentry/SentrySession.m

Lines 41 to 46 in 10d7a5b

	// We use the default constructor here to set the non nullable values to a default values,
	// because this could cause crashes, for example, in serialize.
	// With this approach we avoid crashes and accept the tradeoff that some session data might not
	// be 100% accurate.
	// Ideally we would return nil, if the passed JSON is not valid, which we can't do because it
	// would be a breaking change.

I put this already down for the next major, see #877 (comment)

[marandaneto]

I understand its a breaking change but SentrySession is internal right.
startSession and endSession do not return a Session, nor we do beforeSend with Sessions, so I'd prefer to fix the bug as this session would just mess up with session data on the server (eg wrong average duration).
I'm also fine keeping as it is and fixing it on v7 but I'd like to point this out anyway.

[philipphofmann]

SentrySession initWithJSONObject is public. So making it nullable would be a breaking change. We already had this discussion in the past and we agreed with sticking to the current approach and fix it in the next major.

I mean we can argue that this is clearly wrong and we just fix it now. I don't think anyone is using initWithJSONObject anyway. Why would they?

[bruno-garcia]

initializing SentrySession directly isn't really a documented API or expected to be used. I wouldn't bother bumping a major for this breaking change.

[bruno-garcia]

I agree with Manoel here this will generate trash data and we're better off discarding this instead

[codecov-io]

Codecov Report

Merging #939 (4703ae0) into master (10d7a5b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #939   +/-   ##
=======================================
  Coverage   94.69%   94.69%           
=======================================
  Files          76       76           
  Lines        3504     3508    +4     
=======================================
+ Hits         3318     3322    +4     
  Misses        186      186           

Impacted Files	Coverage Δ
Sources/Sentry/SentrySession.m	95.33% <100.00%> (+0.12%)	⬆️

---

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 10d7a5b...4193965. Read the comment docs.

[bruno-garcia]

Can we just return null instead and drop the session? This isn't affecting anyone using startSession/endSession nor the hybrid SDKs so it's a safe change to make

[marandaneto]

nice :) LGTM