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.

Sign up for GitHub

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

Jump to bottom

fix: Prevent overwriting with fallbacks when updating endpoint #13330

Merged
merged 1 commit into from
May 6, 2024
Merged

fix: Prevent overwriting with fallbacks when updating endpoint #13330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/aws-amplify/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@
"name": "[Analytics] record (Pinpoint)",
"path": "./dist/esm/analytics/index.mjs",
"import": "{ record }",
"limit": "17.05 kB"
"limit": "17.08 kB"
},
{
"name": "[Analytics] record (Kinesis)",
Expand All @@ -317,7 +317,7 @@
"name": "[Analytics] identifyUser (Pinpoint)",
"path": "./dist/esm/analytics/index.mjs",
"import": "{ identifyUser }",
"limit": "15.53 kB"
"limit": "15.57 kB"
},
{
"name": "[Analytics] enable",
Expand Down
55 changes: 30 additions & 25 deletions packages/core/__tests__/providers/pinpoint/apis/testUtils/getExpectedInput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

import {
appId,
clientDemographic,
endpointId as defaultEndpointId,
uuid,
} from '../../testUtils/data';
Expand All @@ -12,7 +11,7 @@ export const getExpectedInput = ({
address,
attributes,
channelType,
demographic = clientDemographic as any,
demographic,
endpointId = defaultEndpointId,
location,
metrics,
Expand All @@ -28,30 +27,36 @@ export const getExpectedInput = ({
EffectiveDate: expect.any(String),
ChannelType: channelType,
Address: address,
Attributes: attributes,
Demographic: {
AppVersion: demographic.appVersion,
Locale: demographic.locale,
Make: demographic.make,
Model: demographic.model,
ModelVersion: demographic.modelVersion ?? demographic.version,
Platform: demographic.platform,
PlatformVersion: demographic.platformVersion,
Timezone: demographic.timezone,
},
Location: {
City: location?.city,
Country: location?.country,
Latitude: location?.latitude,
Longitude: location?.longitude,
PostalCode: location?.postalCode,
Region: location?.region,
},
...(attributes && { Attributes: attributes }),
...(demographic && {
Demographic: {
AppVersion: demographic.appVersion,
Locale: demographic.locale,
Make: demographic.make,
Model: demographic.model,
ModelVersion: demographic.modelVersion ?? demographic.version,
Platform: demographic.platform,
PlatformVersion: demographic.platformVersion,
Timezone: demographic.timezone,
},
}),
...(location && {
Location: {
City: location?.city,
Country: location?.country,
Latitude: location?.latitude,
Longitude: location?.longitude,
PostalCode: location?.postalCode,
Region: location?.region,
},
}),
Metrics: metrics,
OptOut: optOut,
User: {
UserId: userId,
UserAttributes: userAttributes,
},
...((userId || userAttributes) && {
User: {
UserId: userId,
UserAttributes: userAttributes,
},
}),
}),
});
58 changes: 57 additions & 1 deletion packages/core/__tests__/providers/pinpoint/apis/updateEndpoint.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ import {
clientDemographic,
credentials,
endpointId,
identityId,
region,
userAttributes,
userId,
userProfile,
uuid,
Expand Down Expand Up @@ -130,7 +132,8 @@ describe('Pinpoint Provider API: updateEndpoint', () => {
);
});

it('merges demographics', async () => {
it('merges demographics with client info on endpoint creation', async () => {
mockGetEndpointId.mockReturnValue(undefined);
const partialDemographic = { ...demographic } as any;
delete partialDemographic.make;
delete partialDemographic.model;
Expand All @@ -146,6 +149,7 @@ describe('Pinpoint Provider API: updateEndpoint', () => {
expect(mockClientUpdateEndpoint).toHaveBeenCalledWith(
{ credentials, region },
getExpectedInput({
endpointId: createdEndpointId,
demographic: {
...demographic,
make: clientDemographic.make,
Expand All @@ -155,6 +159,58 @@ describe('Pinpoint Provider API: updateEndpoint', () => {
);
});

it('does not merge demographics with client info on endpoint update', async () => {
const partialDemographic = { ...demographic } as any;
delete partialDemographic.make;
delete partialDemographic.model;
await updateEndpoint({
appId,
category,
credentials,
region,
userProfile: {
demographic: partialDemographic,
},
});
expect(mockClientUpdateEndpoint).toHaveBeenCalledWith(
{ credentials, region },
getExpectedInput({ demographic: partialDemographic }),
);
});

it('falls back to idenity id on endpoint creation', async () => {
mockGetEndpointId.mockReturnValue(undefined);
await updateEndpoint({
appId,
category,
credentials,
identityId,
region,
});
expect(mockClientUpdateEndpoint).toHaveBeenCalledWith(
{ credentials, region },
getExpectedInput({
endpointId: createdEndpointId,
userId: identityId,
}),
);
});

it('does not fall back to idenity id on endpoint update', async () => {
await updateEndpoint({
appId,
category,
credentials,
identityId,
region,
userAttributes,
});
expect(mockClientUpdateEndpoint).toHaveBeenCalledWith(
{ credentials, region },
getExpectedInput({ userAttributes }),
);
});

it('creates an endpoint if one is not already cached', async () => {
mockGetEndpointId.mockReturnValue(undefined);
await updateEndpoint({ appId, category, credentials, region });
Expand Down
3 changes: 3 additions & 0 deletions packages/core/__tests__/providers/pinpoint/testUtils/data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ export const event = {
};
export const identityId = 'identity-id';
export const region = 'region';
export const userAttributes = {
attr: ['attr-value-one', 'attr-value-two'],
};
export const userId = 'user-id';
export const userProfile = {
customProperties: {
Expand Down
83 changes: 51 additions & 32 deletions packages/core/src/providers/pinpoint/apis/updateEndpoint.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

import { amplifyUuid } from '../../../utils/amplifyUuid';
import { getClientInfo } from '../../../utils/getClientInfo';
import {
UpdateEndpointInput,
updateEndpoint as clientUpdateEndpoint,
} from '../../../awsClients/pinpoint';
import { UserProfile } from '../../../types';
import { amplifyUuid } from '../../../utils/amplifyUuid';
import { getClientInfo } from '../../../utils/getClientInfo';
import { PinpointUpdateEndpointInput } from '../types';
import { cacheEndpointId } from '../utils/cacheEndpointId';
import {
Expand Down Expand Up @@ -46,22 +47,34 @@ export const updateEndpoint = async ({
name,
plan,
} = userProfile ?? {};
const clientInfo = getClientInfo();

// 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 •
edited
Loading

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"

if (createdEndpointId) {
const clientInfo = getClientInfo();
demographicsFromClientInfo.appVersion = clientInfo.appVersion;
demographicsFromClientInfo.make = clientInfo.make;
demographicsFromClientInfo.model = clientInfo.model;
demographicsFromClientInfo.modelVersion = clientInfo.version;
demographicsFromClientInfo.platform = clientInfo.platform;
}
const mergedDemographic = {
appVersion: clientInfo.appVersion,
make: clientInfo.make,
model: clientInfo.model,
modelVersion: clientInfo.version,
platform: clientInfo.platform,
...demographicsFromClientInfo,
...demographic,
};
const shouldAddAttributes = email || customProperties || name || plan;
const attributes = {
...(email && { email: [email] }),
...(name && { name: [name] }),
...(plan && { plan: [plan] }),
...customProperties,
};

const shouldAddDemographics = createdEndpointId || demographic;
const shouldAddAttributes = email || customProperties || name || plan;
const shouldAddUser = resolvedUserId || userAttributes;

const input: UpdateEndpointInput = {
ApplicationId: appId,
EndpointId: endpointId ?? createdEndpointId,
Expand All @@ -70,31 +83,37 @@ export const updateEndpoint = async ({
EffectiveDate: new Date().toISOString(),
ChannelType: channelType,
Address: address,
Attributes: shouldAddAttributes ? attributes : undefined,
Demographic: {
AppVersion: mergedDemographic.appVersion,
Locale: mergedDemographic.locale,
Make: mergedDemographic.make,
Model: mergedDemographic.model,
ModelVersion: mergedDemographic.modelVersion,
Platform: mergedDemographic.platform,
PlatformVersion: mergedDemographic.platformVersion,
Timezone: mergedDemographic.timezone,
},
Location: {
City: location?.city,
Country: location?.country,
Latitude: location?.latitude,
Longitude: location?.longitude,
PostalCode: location?.postalCode,
Region: location?.region,
},
...(shouldAddAttributes && { Attributes: attributes }),
...(shouldAddDemographics && {
Demographic: {
AppVersion: mergedDemographic.appVersion,
Locale: mergedDemographic.locale,
Make: mergedDemographic.make,
Model: mergedDemographic.model,
ModelVersion: mergedDemographic.modelVersion,
Platform: mergedDemographic.platform,
PlatformVersion: mergedDemographic.platformVersion,
Timezone: mergedDemographic.timezone,
},
}),
...(location && {
Location: {
City: location.city,
cshfang marked this conversation as resolved.
Show resolved Hide resolved
Country: location.country,
Latitude: location.latitude,
Longitude: location.longitude,
PostalCode: location.postalCode,
Region: location.region,
},
}),
Metrics: metrics,
OptOut: optOut,
User: {
UserId: userId ?? identityId,
UserAttributes: userAttributes,
},
...(shouldAddUser && {
User: {
UserId: resolvedUserId,
UserAttributes: userAttributes,
},
}),
},
};
try {
Expand Down
Loading