[MOB-16678] Handle edge network location hint response by kevinlind · Pull Request #259 · adobe/aepsdk-edge-ios

kevinlind · 2022-08-29T23:41:56Z

Description

Handle the EdgeNetwork location hint response from Konductor.

Reads location hint and ttl from Konductor response and stores in EdgeProperties. All requests from the Edge extension will include the location hint in the request URL if available and if not expired. The location hint and expiry date (calculated from the ttlSeconds) is persisted in User Defaults and loaded once the Edge extension launches.
The Edge extension now creates a shared state which includes just the location hint. A shared state is created on launch of the extension and when ever the location hint value changes. A shared state is not created, however, if only the expiry date is updated but the hint value doesn't change.

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.

…andlers to EdgeHitProcessor and NetworkResponseHandler. This makes the EdgeState the holder of the location hint while other classes must call it to get or update the location hint.

…atches a shared state

kevinlind · 2022-08-29T23:48:00Z

Sources/EdgeState.swift

        // else keep consent pending until the consent preferences update event is received

+        // TODO - Always set state or only when properties are not empty ???
+        createSharedState(edgeProperties.toEventData(), nil)


Always creates a shared state when EdgeState boots, regardless of location hint value. Another option is to only create the shared state if there is a valid location hint value, but I think creating an initial empty shared state on boot is better approach and more consistent with how other extensions are implemented.

kevinlind · 2022-08-29T23:57:25Z

Sources/EdgeState.swift

+                //             lower event numbers than states using nil. If this extension later needs to create shared
+                //             states from received events, then this code must be refactored to also use received
+                //             events as the state version.
+                createSharedState(self.edgeProperties.toEventData(), nil)


I'm using nil here for the shared state version, which will cause the Event Hub to get the next available version number. I'm using nil because this function is called by the NetworkResponseHandler which does not have a triggering event to create a shared state with. Using nil here means I must always use nil when creating a shared state even when I have one available (such as in bootupIfNeeded). The shared state is created mainly for Rules use-cases where a customer may want to build a postback URL using the location hint.

Another option is for NetworkResponseHandler to dispatch an event which is listened for by Edge to handle the location hint. That way there is an event to use to create the shared state. This is how Identity Direct extension handles this case. This option, however, will cause a delay when the location hint is handled and any queued hits will not include the hint in their URLs.

As there are not current plans for Edge to include other properties in it's shared state, I feel the first option of using 'nil' is the better approach, but I wanted to point this out to hear other's thoughts.

codecov · 2022-08-30T03:51:40Z

Codecov Report

Merging #259 (9e55759) into dev (ebffb92) will increase coverage by 2.91%.
The diff coverage is 99.22%.

@@            Coverage Diff             @@
##              dev     #259      +/-   ##
==========================================
+ Coverage   92.54%   95.45%   +2.91%     
==========================================
  Files          27       28       +1     
  Lines        1032     1143     +111     
==========================================
+ Hits          955     1091     +136     
+ Misses         77       52      -25

addb · 2022-08-30T16:25:48Z

.circleci/config.yml

  build-and-test:
    macos:
-      xcode: 12.0.1 # Specify the Xcode version to use
+      xcode: 13.4.0 # Specify the Xcode version to use


Is the code coverage issue fixed in 13.4?

I'm not seeing an issue with CodeCov. I've updated the version because version 12 of Xcode is no longer supported in CircleCI. The builds just fail now. Their documentation states that they support the latest 4 major.minor versions of Xcode. We've used Xcode 13.0.0 for other projects, but that is the 4th version. If there is a 13.5 version of Xcode, then 13.0.0 may not be supported. I didn't want to bump all the way to Xcode 14, so I picked that latest Xcode 13 image supported by CircleCI.

addb · 2022-08-31T16:52:50Z

Sources/EdgeConstants.swift

            }
+
+            enum LocationHint {
+                static let SCOPE = "scope"


Should we add the "locationHint" constant here only?
static let KEY = "locationHint"

Can you explain further? I guess I don't understand the question.

This approach is also fine. I was wondering if changing the location of the constant makes sense.
nit:
Instead of Edge.LOCATION_HINT -> we have it as LocationHint.KEY

Sources/EdgeConstants.swift

Sources/EdgeNetworkHandlers/NetworkResponseHandler.swift

praveek · 2022-08-31T21:07:47Z

Sources/Edge.swift

+    private func setLocationHint(hint: String?, ttlSeconds: TimeInterval?) {
+        guard let state = state else { return }
+        if let hint = hint, let ttlSeconds = ttlSeconds {
+            state.setLocationHint(hint: hint, ttlSeconds: ttlSeconds, createSharedState: createSharedState(data:event:))


Do other extensions use the locationHint from the shared state?

Not currently and it's not expected as they should send requests through the Edge extension. The shared state is intended for Rules use cases where a customer may want to send a postback directly to the Edge Network. There's no hard requirement for that use-case either, however.

praveek · 2022-08-31T21:12:22Z

Sources/EdgeNetworkHandlers/NetworkResponseHandler.swift

            } else {
-                handleStoreEventHandle(handle: eventHandle)
+                if let type = eventHandle.type {
+                    if EdgeConstants.JsonKeys.Response.EVENT_HANDLE_TYPE_STORE == type.lowercased() {


nit: Is type.lowercased() required? Calling it only for if condition is little confusing.

I don't think it's required as the response uses "state:store" (lowercased). However, the original code used it so I preserved the logic. The handle type "locationHint:result" is mix cased so I'm doing an exact match.

praveek · 2022-08-31T21:23:13Z

Sources/EdgeState.swift

@@ -74,4 +93,57 @@ class EdgeState {
        currentCollectConsent = status


This is not related to your PR but some of the variables are not guarded by DispatchQueue and this could cause race conditions and crashes.

I've updated EdgeState to guard the consent status. It's better to do it as it keep the design consistent, however I don't think the consent status is accessed from multiple threads. Can you take another look at this part of the PR?

Sources/EdgeState.swift

praveek

+1

…ationHint

…-mob-16678-region-id

kevinlind added 12 commits August 17, 2022 20:03

Add region id parameter to EdgeEndpoint

69c5152

Read and write location hint to requests

9d76e16

Move location hint code to EdgeState while passing get/set function h…

f09e928

…andlers to EdgeHitProcessor and NetworkResponseHandler. This makes the EdgeState the holder of the location hint while other classes must call it to get or update the location hint.

Set shared state when the location hint is updated

7ee871b

Add function to clear location hint.

774f0ad

Add EdgeHitProcessor unit tests

e985c2e

Add NetworkResponseHandler functional tests

0c1c648

Add EdgeProperties to store and manage persisted fields

0a69784

Add EdgeState unit tests

4153820

Add location hint functional tests in AEPEdgeFunctionalTests

d84e103

Update expected number of events in functional tests as Edge now disp…

90c3a2b

…atches a shared state

Add coverage capture to AEPEdge scheme

0a03462

kevinlind commented Aug 29, 2022

View reviewed changes

kevinlind added 2 commits August 29, 2022 17:05

Update comment when creating shared state in bootupIfNeeded

dbc0601

Rename EdgeEndpoint property from 'regionId' to 'locationHint'

43496dd

kevinlind requested review from addb and praveek August 30, 2022 00:08

update CircleCI macos xcode to 13.4.0

f247b88

Fix a few SwiftLint warnings

e28fa8f

addb reviewed Aug 30, 2022

View reviewed changes

addb reviewed Aug 31, 2022

View reviewed changes

Sources/EdgeConstants.swift Outdated Show resolved Hide resolved

addb reviewed Aug 31, 2022

View reviewed changes

Sources/EdgeNetworkHandlers/NetworkResponseHandler.swift Outdated Show resolved Hide resolved

praveek reviewed Aug 31, 2022

View reviewed changes

Sources/EdgeState.swift Outdated Show resolved Hide resolved

praveek approved these changes Aug 31, 2022

View reviewed changes

Move Event Handler Type constants under their own enum

3305c16

kevinlind added 6 commits September 1, 2022 15:08

Refactor handleLocationHintHandle loop to break on failure

6c72a02

Persist EdgeProperties to data store automatically from get/clear loc…

fe6dd2e

…ationHint

Guard access to current consent status in EdgeState

8895c34

Merge branch 'dev' of github.com:adobe/aepsdk-edge-ios into kevinlind…

cd27bc3

…-mob-16678-region-id

Add unit tests for Edge endpoint with path overwrite and location hint.

047db54

SwiftLint autocorrect

9e55759

addb approved these changes Sep 7, 2022

View reviewed changes

kevinlind merged commit 03096b0 into adobe:dev Sep 8, 2022

kevinlind deleted the mob-16678-region-id branch September 8, 2022 17:55

		@@ -74,4 +93,57 @@ class EdgeState {
		currentCollectConsent = status

Conversation

kevinlind commented Aug 29, 2022

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addb Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

praveek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Aug 30, 2022 •

edited

Loading

addb Sep 7, 2022 •

edited

Loading