Handle reset identities event by nporter-adbe · Pull Request #169 · adobe/aepsdk-edge-ios

nporter-adbe · 2021-03-18T16:20:34Z

Description

Instead of just storing the Event we now store the current identity shared state and the current store payloads. This will allow for easier implementation of handling the reset request event.
Internal task: AMSDK-11387

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2021-03-18T16:26:12Z

Codecov Report

Merging #169 (482fd9e) into feature/consent (42c7db2) will increase coverage by 0.90%.
The diff coverage is 96.00%.

@@                 Coverage Diff                 @@
##           feature/consent     #169      +/-   ##
===================================================
+ Coverage            93.06%   93.96%   +0.90%     
===================================================
  Files                   24       25       +1     
  Lines                  793      861      +68     
===================================================
+ Hits                   738      809      +71     
+ Misses                  55       52       -3

kevinlind

This looks good. However, I have questions regarding how the response payload is updated from the server response. This implementation would mean that any queued hit's stored payload would not get updated by the server response of earlier hits. That's different than what we do currently when the stored payloads are retrieved right before the hit is sent.

Also, if the hit queue is stored for a long time (app closed for a while then relaunched), there's the possibility of a stored payload timing out. Those timed out payload elements are currently removed before sending a hit, but with this change they are not. Will that cause issues with the server?

nporter-adbe · 2021-03-18T18:48:01Z

@kevinlind Correct, the stored payload of queued hits are not able to be updated. I'm on the fence about which approach is better. On one end I see that we should get a snapshot of the current cookies when the hit was queued and use them, but I can also see that always getting the latest cookies may be nice to have. @emdobrin thoughts?

That's a good point regarding expired payloads... I would hope the server could handle it, but I made a small update to filter out expired payloads just in case. I should probably add a test for this edge case, but let me know your thoughts.
77f7b7b

emdobrin · 2021-03-18T19:13:04Z

I think you are right, we cannot easily update the persisted state store if they change in between hits, as you have to update certain events, but not all. One other option is to have a combination of the two approaches we discussed:

keep the state store on demand (don't persist it)
persist the reset event
when processing the reset event clear store and save its timestamp
if state:store response comes for a request event with timestamp < than reset event timestamp, ignore it (also don't dispatch this state:store for others to consume)

In other words:

If state:store is updated while processing events, the next hits will contain the updated information, as soon as the response is processed client-side.
If state:store is updated async after a reset event was processed, it doesn't overwrite the cleared store.

nporter-adbe · 2021-03-18T20:19:15Z

@kevinlind @emdobrin I pushed up some changes to handle the reset event in this PR as well and took into account some of the feedback of not persisting the state store payloads.

The PR isn't in final form and needs some tests added, but let me know what you think about this approach.

…t events

emdobrin

can you also add some integration tests involving experience events with resetIdentitities in between?

emdobrin · 2021-03-19T15:15:49Z

Sources/Edge.swift

+        } else if event.isResetIdentitiesEvent {
+            let configurationSharedState = getSharedState(extensionName: EdgeConstants.SharedState.Configuration.STATE_OWNER_NAME,
+                                                          event: event)
+            // use barrier to wait for EdgeIdentity to handle the reset


so are we still waiting for the reset request and not for the resetComplete?

Yeah, we could listen for the event coming from edge identity notifying it has reset or use the barrier flag to wait until we know its done processing

Sources/Edge.swift

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

Sources/EdgeNetworkHandlers/NetworkResponseHandler.swift

emdobrin · 2021-03-19T15:36:07Z

Sources/EdgeNetworkHandlers/NetworkResponseHandler.swift


        // Note: ExEdge does not return eventIndex when there is only one event in the request.
-        // The event handles and errrors are associated to that request event, so defaulting to 0 here.
+        // The event handles and errors are associated to that request event, so defaulting to 0 here.


nporter-adbe · 2021-03-19T16:12:07Z

Tests/UnitTests/AtomicTests.swift

+    private let queue = DispatchQueue(label: "test queue",
+                                      attributes: .concurrent)
+
+    func testAtomicWrite() {


We may want to add more tests in the future, but for now I think this is sufficient.

Sources/EdgeNetworkHandlers/NetworkResponseHandler.swift

Sources/Event+Edge.swift

emdobrin · 2021-03-19T19:44:32Z

Sources/EdgeNetworkHandlers/Atomic.swift

+
+/// Represents a thread-safe type for read and write operations
+final class Atomic<A> {
+    private let queue = DispatchQueue(label: "Atomic type serial queue")


if there are multiple atomic objects being used in this extension, they will all be queued based on the same serial queue, did you mean to have different queues for each?

They will all be different queues, the label is only used for debugging. I don't think it really makes sense to need to specify the queue label when creating an Atomic, so I used a general label in all cases. "Atomic type serial queue".

Sources/EdgeNetworkHandlers/NetworkResponseHandler.swift

kevinlind

Looks good to me. Thanks for all the updates.

Add EdgeDataEntity and persist identityMap and store payloads

aa8ca7c

nporter-adbe requested review from emdobrin and kevinlind March 18, 2021 16:27

kevinlind reviewed Mar 18, 2021

View reviewed changes

Filter out expired payloads before sending hit

77f7b7b

Handle reset event

d718635

nporter-adbe changed the title ~~Persist EdgeDataEntity on queuing~~ Handle reset identities event Mar 18, 2021

kevinlind mentioned this pull request Mar 19, 2021

Do not set ECID as primary identity. adobe/aepsdk-edgeidentity-ios#21

Merged

10 tasks

nporter-adbe added 3 commits March 19, 2021 09:11

Add tests

d4a0fca

Add empty event data check to hit processor for experience and consen…

0f232d1

…t events

lint

63c5323

emdobrin reviewed Mar 19, 2021

View reviewed changes

nporter-adbe added 6 commits March 19, 2021 09:51

Store tuples of uuid's and dates instead of entire event

a1a5fcc

Make date in network handler thread safe and update logs

230ac98

Extract ignoreStorePayloads to a functino

bbff298

Add comment about batching

af66a37

Add functional tests for clearing state store

efd31de

Add atomic test

3ee986c

nporter-adbe commented Mar 19, 2021

View reviewed changes

clean up test for atomic

53cc5e8

emdobrin reviewed Mar 19, 2021

View reviewed changes

kevinlind approved these changes Mar 19, 2021

View reviewed changes

Update comments, add log

482fd9e

nporter-adbe merged commit 9a3e95c into adobe:feature/consent Mar 22, 2021

Conversation

nporter-adbe commented Mar 18, 2021 • edited by emdobrin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov bot commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kevinlind left a comment

Choose a reason for hiding this comment

Uh oh!

nporter-adbe commented Mar 18, 2021

Uh oh!

emdobrin commented Mar 18, 2021

Uh oh!

nporter-adbe commented Mar 18, 2021

Uh oh!

emdobrin left a comment

Choose a reason for hiding this comment

Uh oh!

emdobrin Mar 19, 2021

Choose a reason for hiding this comment

Uh oh!

nporter-adbe Mar 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emdobrin Mar 19, 2021

Choose a reason for hiding this comment

Uh oh!

nporter-adbe Mar 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emdobrin Mar 19, 2021

Choose a reason for hiding this comment

Uh oh!

nporter-adbe Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinlind left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nporter-adbe commented Mar 18, 2021 •

edited by emdobrin

Loading

codecov bot commented Mar 18, 2021 •

edited

Loading