Skip to content
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

fix: Prevent overwriting with fallbacks when updating endpoint #13330

Merged
merged 1 commit into from
May 6, 2024

Conversation

cshfang
Copy link
Member

@cshfang cshfang commented May 2, 2024

Description of changes

When calling initializePushNotifications, the library calls an internal updateEndpoint API. Anytime updateEndpoint is called, it automatically populates the demographic property with client info and the userId with the identity id. When calling identifyUser, these values can be provided explicitly but will always be pre-populated. This can have unexpected consequences since initializePushNotifications is typically called on every app launch - and it will opaquely overwrite existing values previously set on the endpoint.

This PR updates the updateEndpoint API to only pre-populate these fields when creating an endpoint to avoid repeatedly overwriting the endpoint with the default/fallback values.

Issue #, if available

#13174

Description of how you validated changes

  • yarn test
  • Added unit tests that were expected to fail prior to code changes and verified they are now passing
  • Tested on sample app to see that demographic and userId are only on the network request when an endpoint has yet to be cached (endpoint creation) and subsequent calls do not.
// Endpoint creation
{
  "RequestId": "8808844f-5e12-440b-a914-d3ae7191b0f8",
  "EffectiveDate": "2024-05-02T19:55:22.795Z",
  "ChannelType": "APNS_SANDBOX",
  "Address": "<apns-token>",
  "Demographic": {
    "AppVersion": "ios/17.0",
    "Make": null,
    "Model": null,
    "ModelVersion": "17.0",
    "Platform": "ios"
  },
  "User": { "UserId": "<cognito-identity-id>" }
}
// Endpoint update
{
  "RequestId": "8178652f-5842-44b6-a02c-27616f4d6cb9",
  "EffectiveDate": "2024-05-02T19:57:28.751Z",
  "ChannelType": "APNS_SANDBOX",
  "Address": "<apns-token>",
}

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@cshfang cshfang requested review from a team as code owners May 2, 2024 19:59
@cshfang cshfang force-pushed the fix/notifications-issues branch 2 times, most recently from d904325 to 612cd84 Compare May 2, 2024 20:32
@cshfang cshfang requested a review from a team as a code owner May 2, 2024 20:32
Copy link
Member

@erinleigh90 erinleigh90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Chris! Good catch.

Had one nit and one question but LGTM!

// only automatically populate the endpoint with client info and identity id upon endpoint creation to
// avoid overwriting the endpoint with these values every time the endpoint is updated
const demographicsFromClientInfo: UserProfile['demographic'] = {};
const resolvedUserId = createdEndpointId ? userId ?? identityId : userId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Did we decide that || is better to use than ??

Suggested change
const resolvedUserId = createdEndpointId ? userId ?? identityId : userId;
const resolvedUserId = createdEndpointId ? userId || identityId : userId;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure I understand: the identityId property is always a default value and userId is always customer provided or empty, correct?

Copy link
Member Author

@cshfang cshfang May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this call creates an endpoint

  • use userId if provided, otherwise fall back to identityId

If this call updates an endpoint

  • use userId if provided but don't fall back to anything

To make sure I understand: the identityId property is always a default value and userId is always customer provided or empty, correct?

identityId is always only a default/fallback value but is only set during endpoint creation. userId is always customer provided or empty, yes.

Nit: Did we decide that || is better to use than ??

Generally speaking, ?? is almost always what we intend unless we explicitly want falsy values like 0 or '' to qualify as "empty"

@cshfang cshfang merged commit d7b837e into aws-amplify:main May 6, 2024
30 checks passed
@cshfang cshfang deleted the fix/notifications-issues branch May 6, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants