-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adopt changes in Core 2.0 #76
Conversation
- .gradle changes - Add jitpack to repositories - Consume Core 2.0 via com.github.adobe.aepsdk-core-android:core:v2.0.0-SNAPSHOT from jitpack - Prevent transitive dependencies from other extensions - Change moduleVersion, mavenCoreVersion to 2.0.0. - Change IdentityConstants.EXTENSION_VERSION to 2.0.0 - Expose IdentityExtension.class via Idenity.EXTENSION - Switch to MobileCore.dispatchEventWithResponseCallback(event, timeout, callbackWithError) and ExtensionApi.dispatch(event) - Switch to using new shared state api - Remove multiple instance creation of SharedStateCallback. - Streamline shared state calls via SharedStateCallback within and outside the Identity extension - Implement IdentityExtension.onRegistered() - Change event listener registration to be done in onRegistered() instead of the IdentityExtension's constructor - Remove Listener*.java classes in favor of inline methods - Remove the inhouse executor service and mutex as the events are now incident via a single thread maintained by the extension container housing this extension. - Implement IdentityExtension.readyForEvent() - Add logic to wait for EventHub state and IdentityDirect registration check if ECID from persisted properties is unavailable. - Remove the logic to cache events internally. Take advantage of the Hub shared state fetching to boot up and disambiguate between Identity direct and EdgeIdentity. - Switch to use public utility classes and methods from Core where applicable - Use StringUtils.isEmpty() - Use JSONUtil.toMap() and JSONUtil.toList() - Use DataReader.opt*() to read and parse event data. Remove class cast exception guards and resolve unchecked cast warnings where possible. - General changes done on code path toched - private and finals where applicable - Use cascading constructors to eliminate constructor divergence risk and ease test injection Out of scope for this PR - Switching to Log from MobileCore.Log - Switching IdentityStorageService to NamedCollection - Test changes - PowerMock elimination
- .gradle changes - Add jitpack to repositories - Consume Core 2.0 via com.github.adobe.aepsdk-core-android:core:v2.0.0-SNAPSHOT from jitpack - Prevent transitive dependencies from other extensions - Change moduleVersion, mavenCoreVersion to 2.0.0. - Change IdentityConstants.EXTENSION_VERSION to 2.0.0 - Expose IdentityExtension.class via Idenity.EXTENSION - Switch to MobileCore.dispatchEventWithResponseCallback(event, timeout, callbackWithError) and ExtensionApi.dispatch(event) - Switch to using new shared state api - Remove multiple instance creation of SharedStateCallback. - Streamline shared state calls via SharedStateCallback within and outside the Identity extension - Implement IdentityExtension.onRegistered() - Change event listener registration to be done in onRegistered() instead of the IdentityExtension's constructor - Remove Listener*.java classes in favor of inline methods - Remove the inhouse executor service and mutex as the events are now incident via a single thread maintained by the extension container housing this extension. - Implement IdentityExtension.readyForEvent() - Add logic to wait for EventHub state and IdentityDirect registration check if ECID from persisted properties is unavailable. - Remove the logic to cache events internally. Take advantage of the Hub shared state fetching to boot up and disambiguate between Identity direct and EdgeIdentity. - Switch to use public utility classes and methods from Core where applicable - Use StringUtils.isEmpty() - Use JSONUtil.toMap() and JSONUtil.toList() - Use DataReader.opt*() to read and parse event data. Remove class cast exception guards and resolve unchecked cast warnings where possible. - General changes done on code path toched - private and finals where applicable - Use cascading constructors to eliminate constructor divergence risk and ease test injection Out of scope for this PR - Switching to Log from MobileCore.Log - Switching IdentityStorageService to NamedCollection - Test changes - PowerMock elimination
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/EventUtils.java
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityMap.java
Outdated
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityMap.java
Outdated
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityExtension.java
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityExtension.java
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityExtension.java
Outdated
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityExtension.java
Outdated
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityExtension.java
Outdated
Show resolved
Hide resolved
// Reuse the ECID from Identity Direct (if registered) or generate new ECID on first launch | ||
if (identityProperties.getECID() == null) { | ||
// Wait for all extensions to be registered as forth coming logic depends on Identity Direct state | ||
if (!areAllExtensionsRegistered(callback)) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to add similar logic in iOS to ensure hub ss is ready before bootup?
cc @kevinlind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be safe to add this to iOS as well. I don't remember the full conversation but I was under the impression that the EventHub shared state was the first (or one of the first) events dispatched when the SDK starts. Adding a check like this for the EventHub shared state makes the code more robust, so it's a good idea.
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityState.java
Outdated
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityState.java
Outdated
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/Utils.java
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/Identity.java
Show resolved
Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityExtension.java
Show resolved
Hide resolved
- Switch to using handlers for each event listener - Use TimeUtils where applicable - Eliminate two calls to get event hub shared state - Add comments and TODOs for verification and logging
|
||
/** | ||
* Defines the public APIs for the AEP Edge Identity extension. | ||
*/ | ||
public class Identity { | ||
|
||
public static final Class<? extends Extension> EXTENSION = IdentityExtension.class; | ||
private static final long CALLBACK_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documented API timeout is 500 ms. The timeout is short primarily because of getExperienceCloudId, which needs to be fast for customers implementing a hybrid application which contains a web view. As these APIs are time sensitive for customers, I'd suggest keeping the 500 ms value.
cc @emdobrin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not realize that the documented value of timeout is 500ms and so made it match the default max timeout for a response callback instead.
I have now changed it to 500ms to match the documented contract 👍🏼
Description
In this PR :
Out of scope for this PR
Related Issue
MOB-17065
Motivation and Context
Core 2.0 changes will allow architectural consistency with iOS.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: