Copy data map when creating a new heartbeat event

[wzieba]

Closes: #60

Instead of referencing to the same Map<String, Object> data across all events, when updating timestamps of events in EngagementManager, we will now create a shallow copy of data. This way will stop introducing unwanted changes of data.ts to existing events.

Screen from debugger

You can see that after the change, each event has its own data HashMap so they can update it independently.

Before	After

Payload result

You can see that each event has now its own timestamp of record

Before	After

[ParaskP7]

👋 @wzieba !

I have reviewed and tested this PR (see My Test Instructions below), everything seems to be working as expected, nice fix! 🌟

---

FYI: Because I didn't know how to exactly test this, to verify that everything worked (bug) and works (fix) as expected, I used the below test instructions of mine. Let me know if the that look good to you, or if you tested this in some kind of another way. This will greatly help me understand how to best test future such PRs.

My Test Instructions (using the example app):

• Click START ENGAGEMENT button.
• Wait 1 second.
• Click START ENGAGEMENT button.
• Wait 1 second.
• Wait for the flushing queue to finish (30 seconds).
• Check the POST Data and ts (JSON).
• Click STOP ENGAGEMENT button.
• Wait 1 second.
• Click STOP ENGAGEMENT button.
• Wait 1 second.
• Wait for the flushing queue to finish (30 seconds).
• Check the POST Data and ts (JSON).

[ParaskP7]

1. Suggestion (💡): How would you feel about suppressing the unchecked warning on (Map<String, Object>) to make it explicit that we know what we are doing here?
2. Suggestion (💡): How would you feel about using requireNonNull(...) to make it explicit that we expect event.get("data") to never be null?
3. Minor (🔍): Not related to your change, but since you correctly used new HashMap<> instead of new HashMap, consider changing the above such construction from new HashMap(this.baseEvent) to new HashMap<>(this.baseEvent).

PS: With the above suggestions and minor improvements I am hoping to end-up with a warning-free doEnqueue(...) method. Feel free to ignore or apply any. 💯

[wzieba]

Thank you for the review @ParaskP7 !

1. Suggestion (💡): How would you feel about suppressing the unchecked warning on (Map<String, Object>) to make it explicit that we know what we are doing here?

Sure 👍

2. Suggestion (💡): How would you feel about using requireNonNull(...) to make it explicit that we expect event.get("data") to never be null?

I've decided to not force non-nullable event.get("data") to not crash users applications. It shouldn't ever happen, but I thought it's better safe than sorry in this case (especially for libraries). WDYT?

3. Minor (🔍): Not related to your change, but since you correctly used new HashMap<> instead of new HashMap, consider changing the above such construction from new HashMap(this.baseEvent) to new HashMap<>(this.baseEvent).

Sure! Now doEnqueue method is warning free. The PR is ready for the second round!

[ParaskP7]

Awesome, thanks for these changes @wzieba , I reviewed them and everything LGTM! 💯 🙇 ❤️

I've decided to not force non-nullable event.get("data") to not crash users applications. It shouldn't ever happen, but I thought it's better safe than sorry in this case (especially for libraries). WDYT?

I am usually on the fail-fast side when dealing with this kind of problems, meaning that I would prefer the app(s) to crash vs. having an incomplete or invalid flow. Going with the fail-safe road might not crash the app, not at this instance at least (maybe down the road), still it will take us much more time to get feedback on, debug and get to the bottom of it. 🤷

Having said that, I understand that for a library such as this, maybe we shouldn't be as aggressive. Instead, we might have agreed that first and foremost we don't want our library to NOT be the cause of any app crash. In that case, I am okay will having the check there and guard from crashes, no matter what. 🤔

Then again, this peace of code was there for years and already tested (by our users), thus I think I would be personally okay with failing-fast. But, I leave this decision up to you, I think you are the best engineer to decide for Parsely atm. 😊

[wzieba]

this peace of code was there for years and already tested (by our users)

On the second thought, actually that's a good point 👍 . I've brought back the fail-fast behavior. I used assert instead of requireNonNull as it's not Kotlin (yet) 😄. bc62a92

[ParaskP7]

Awesome, thank @wzieba ! 🚀

I've brought back the fail-fast behavior. I used assert instead of requireNonNull as it's not Kotlin (yet) 😄.

assert or whichever way you do it is totally fine by me, but just as an FYI Objects.requireNonNull(event.get("data") works for Java too. 😅

Map<String, Object> data = new HashMap<>(
        (Map<String, Object>) Objects.requireNonNull(event.get("data"))
);

Feel free to merge whenever, no need to change anything! 🚀

[wzieba]

TIL, thanks! I merged the PR before reading the comment but I'll know this for the future 🙂

[randyriback]

Hi there @wzieba -- quick question. Is the Android SDK currently in a place where we can recommend it to customers (who don't have video tracking), or best to wait until this is deployed?

[wzieba]

hi @randyriback ! Yes, IMO the current version of Android SDK is in a place where we can recommend it to customers. Saying this, the fix merged in this PR will be deployed today so maybe it's worth to wait just a little bit. I'll ping you here when the new version will be available.

[wzieba]

3.0.8 is released @randyriback