[MOB-16479] Allow path overwrite for Media Edge requests by addb · Pull Request #258 · adobe/aepsdk-edge-ios

addb · 2022-08-02T22:01:15Z

Description

Add support to override interact request path. This is primarily being added to support Media edge workflow, but is generic, so that any other extension can make use of it.

Format	https:{EdgeDomain}/{Environment}/{EdgeRegionId}/{custom path}
Example	https://edge.domain/ee/or2/va/v1/sessionStart

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 · 2022-08-02T22:18:09Z

Codecov Report

Merging #258 (c65aa48) into dev (15632d5) will decrease coverage by 2.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #258      +/-   ##
==========================================
- Coverage   94.59%   92.54%   -2.05%     
==========================================
  Files          27       27              
  Lines         998     1032      +34     
==========================================
+ Hits          944      955      +11     
- Misses         54       77      +23

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

Sources/EdgeNetworkHandlers/EdgeEndpoint.swift

…Config

Sources/EdgeNetworkHandlers/EdgeEndpoint.swift

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

Tests/UnitTests/EdgeHitProcessorTests.swift

kevinlind · 2022-08-04T04:58:26Z

Do you know why the coverage drops 2.5% with these changes? The report states that your changes are 100% covered by tests, however coverage is lost on lines this PR doesn't change.

addb · 2022-08-04T05:00:12Z

Do you know why the coverage drops 2.5% with these changes? The report states that your changes are 100% covered by tests, however coverage is lost on lines this PR doesn't change.

It has to do with the Xcode 13.0.0 update. We have noticed this in other repos as well that the coverage drops when we update to xcode 13.0.0

emdobrin

@addb can you also update the description for this PR?

Sources/EdgeConstants.swift

emdobrin · 2022-08-04T23:07:47Z

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

    }

-    /// Extracts all the Edge configuration keys from the Configuration shared state
+    /// Extracts all the Edge configuration keys needed for this request. Configuration keys are extracted from the Configuration shared state and the given `event` instance.


I recommend not mixing config keys with event keys in the same payload here to keep this function code specialized on a single item at a time. We could instead use a new helper getRequestProperties(event) that extract the request path overwrite and maybe additional request settings in the future.

Additionally getEdgeConfig is used for all the endpoints we integrate with (currently interact and consent), we should allow the path overwrite only in the edge requestContent (interact) requests, and not for consent. Let's also include a test that overwrites are ignored for consent

Should we revert this doc change?

Don't forget to revert this doc change.

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

…t for similar requests in future. Added validation to drop paths with invalid cahracters.

…paths to have leading /

kevinlind

Can you check the coverage report and increase your diff coverage back to 100%? You may just need to add some negative tests to isValidPath.

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

Tests/UnitTests/EdgeHitProcessorTests.swift

… place.

…anged unitTest and functionTest schemes settings to build when run.

kevinlind

One additional small comment, otherwise looks good.

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

Sources/EdgeNetworkHandlers/EdgeEndpoint.swift

emdobrin · 2022-08-12T18:38:17Z

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

    }

-    /// Extracts all the Edge configuration keys from the Configuration shared state
+    /// Extracts all the Edge configuration keys needed for this request. Configuration keys are extracted from the Configuration shared state and the given `event` instance.


Should we revert this doc change?

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

Tests/FunctionalTests/EdgePathOverwriteTests.swift

Tests/UnitTests/EdgeHitProcessorTests.swift

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

emdobrin · 2022-08-12T18:52:12Z

Tests/FunctionalTests/EdgePathOverwriteTests.swift

+import XCTest
+
+/// End-to-end testing for the AEPEdge public APIs
+class AEPEdgePathOverwriteTests: FunctionalTestBase {


can you also add a test that it does not include the "request" portion in the request body?

I see the request being sent as event payload.

hmm.. that should not be the case, we should handle this and not include the client-side "request" portion in the request to Konductor

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

…rs present in custom path.

…payload. Added test for the same.

kevinlind

Looks good. One small clean up comment before you merge to revert a doc change after you refactored some code.

kevinlind · 2022-09-02T03:32:01Z

Sources/EdgeNetworkHandlers/EdgeHitProcessor.swift

    }

-    /// Extracts all the Edge configuration keys from the Configuration shared state
+    /// Extracts all the Edge configuration keys needed for this request. Configuration keys are extracted from the Configuration shared state and the given `event` instance.


Don't forget to revert this doc change.

[MOB-16479] Allow path overwrite for Media Edge requests

5ae1ed7

addb requested review from emdobrin and kevinlind August 2, 2022 22:01

addb added 2 commits August 2, 2022 15:01

Merge branch 'dev' into MOB-16479

1644531

[MOB-16479] Update Circle CI config Xcode version to 13.0.0

bdd8382

kevinlind reviewed Aug 2, 2022

View reviewed changes

[MOB-16479] Refactor code to extract and store overwrite path in edge…

e62bb8a

…Config

kevinlind reviewed Aug 4, 2022

View reviewed changes

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

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

Tests/UnitTests/EdgeHitProcessorTests.swift Outdated Show resolved Hide resolved

[MOB-16479] Review fixes

2e831a0

emdobrin reviewed Aug 4, 2022

View reviewed changes

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

addb added 2 commits August 5, 2022 16:18

[MOB-16479] Refactored code to have requestProperties array to suppor…

edbce41

…t for similar requests in future. Added validation to drop paths with invalid cahracters.

[MOB-16479] Fix functional tests with the latest changes to requires …

f909016

…paths to have leading /

kevinlind reviewed Aug 9, 2022

View reviewed changes

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

Tests/UnitTests/EdgeHitProcessorTests.swift Show resolved Hide resolved

addb added 4 commits August 9, 2022 14:59

[MOB-16479] Added _ ~ in allowed characters for URL path and added tests

ab4da07

[MOB-16479] Refactored code to have request path validation in single…

81087d4

… place.

[MOB-16479] Added test for onUnregistered

7a2015d

[MOB-16479] Refactored code to simplify and improve coverage. Also ch…

6e3136b

…anged unitTest and functionTest schemes settings to build when run.

kevinlind approved these changes Aug 10, 2022

View reviewed changes

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

[MOB-16479] Minor refactoring

fb2e9bf

emdobrin reviewed Aug 12, 2022

View reviewed changes

addb added 3 commits August 16, 2022 14:21

[MOB-16479] Review fixes

b6462c2

Fix build error

88ec055

Fix test

11b27f2

emdobrin reviewed Aug 17, 2022

View reviewed changes

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

addb added 2 commits August 16, 2022 22:19

[MOB-16479] Refactored code to throw error only when invalid characte…

48f82e1

…rs present in custom path.

[MOB-16479] Update log message for path overwrite happy path

1d1fb49

[MOB-16479] Added logic to not send custom request object in request …

09008e2

…payload. Added test for the same.

kevinlind approved these changes Sep 2, 2022

View reviewed changes

[MOB-16479] Revert back doc changes

c65aa48

addb merged commit ebffb92 into adobe:dev Sep 2, 2022

addb deleted the MOB-16479 branch June 1, 2023 17:30

Conversation

addb commented Aug 2, 2022 • edited 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 Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinlind commented Aug 4, 2022

Uh oh!

addb commented Aug 4, 2022

Uh oh!

emdobrin left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

kevinlind left a comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

kevinlind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

addb commented Aug 2, 2022 •

edited

Loading

codecov bot commented Aug 2, 2022 •

edited

Loading