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 UID matching issues #4777

Merged
merged 4 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export type ActiveAppReleaseQuery = {
name: string
appModules: {
uuid: string
userIdentifier: string
handle: string
config: JsonMapType
specification: {identifier: string; externalIdentifier: string; name: string}
Expand All @@ -30,6 +31,7 @@ export type ActiveAppReleaseQuery = {

export type ReleasedAppModuleFragment = {
uuid: string
userIdentifier: string
handle: string
config: JsonMapType
specification: {identifier: string; externalIdentifier: string; name: string}
Expand All @@ -46,6 +48,7 @@ export const ReleasedAppModuleFragmentDoc = {
kind: 'SelectionSet',
selections: [
{kind: 'Field', name: {kind: 'Name', value: 'uuid'}},
{kind: 'Field', name: {kind: 'Name', value: 'userIdentifier'}},
{kind: 'Field', name: {kind: 'Name', value: 'handle'}},
{kind: 'Field', name: {kind: 'Name', value: 'config'}},
{
Expand Down Expand Up @@ -176,6 +179,7 @@ export const ActiveAppRelease = {
kind: 'SelectionSet',
selections: [
{kind: 'Field', name: {kind: 'Name', value: 'uuid'}},
{kind: 'Field', name: {kind: 'Name', value: 'userIdentifier'}},
{kind: 'Field', name: {kind: 'Name', value: 'handle'}},
{kind: 'Field', name: {kind: 'Name', value: 'config'}},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export type AppVersionByIdQuery = {
metadata: {versionTag?: string | null}
appModules: {
uuid: string
userIdentifier: string
handle: string
config: JsonMapType
specification: {identifier: string; externalIdentifier: string; name: string}
Expand Down Expand Up @@ -89,6 +90,7 @@ export const AppVersionById = {
kind: 'SelectionSet',
selections: [
{kind: 'Field', name: {kind: 'Name', value: 'uuid'}},
{kind: 'Field', name: {kind: 'Name', value: 'userIdentifier'}},
{kind: 'Field', name: {kind: 'Name', value: 'handle'}},
{kind: 'Field', name: {kind: 'Name', value: 'config'}},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export type ListAppsQuery = {
name: string
appModules: {
uuid: string
userIdentifier: string
handle: string
config: JsonMapType
specification: {identifier: string; externalIdentifier: string; name: string}
Expand Down Expand Up @@ -147,6 +148,7 @@ export const ListApps = {
kind: 'SelectionSet',
selections: [
{kind: 'Field', name: {kind: 'Name', value: 'uuid'}},
{kind: 'Field', name: {kind: 'Name', value: 'userIdentifier'}},
{kind: 'Field', name: {kind: 'Name', value: 'handle'}},
{kind: 'Field', name: {kind: 'Name', value: 'config'}},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export type CreateAppVersionMutation = {
id: string
appModules: {
uuid: string
userIdentifier: string
handle: string
config: JsonMapType
specification: {identifier: string; externalIdentifier: string; name: string}
Expand Down Expand Up @@ -157,6 +158,7 @@ export const CreateAppVersion = {
kind: 'SelectionSet',
selections: [
{kind: 'Field', name: {kind: 'Name', value: 'uuid'}},
{kind: 'Field', name: {kind: 'Name', value: 'userIdentifier'}},
{kind: 'Field', name: {kind: 'Name', value: 'handle'}},
{kind: 'Field', name: {kind: 'Name', value: 'config'}},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ query activeAppRelease($appId: ID!) {

fragment ReleasedAppModule on AppModule {
uuid
userIdentifier
handle
config
specification {
Expand Down
21 changes: 7 additions & 14 deletions packages/app/src/cli/services/context/id-matching.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,20 @@ vi.mock('../dev/create-extension')

const REGISTRATION_A: RemoteSource = {
uuid: 'UUID_A',
uid: 'UID_A',
id: 'A',
title: 'EXTENSION_A',
type: 'CHECKOUT_POST_PURCHASE',
}

const REGISTRATION_A_2: RemoteSource = {
uuid: 'UUID_A_2',
uid: 'UID_A_2',
id: 'A_2',
title: 'EXTENSION_A_2',
type: 'CHECKOUT_POST_PURCHASE',
}

const REGISTRATION_A_3: RemoteSource = {
uuid: 'UUID_A_3',
uid: 'UID_A_3',
id: 'A_3',
title: 'EXTENSION_A_3',
type: 'CHECKOUT_POST_PURCHASE',
Expand All @@ -41,31 +38,27 @@ const REGISTRATION_A_4: RemoteSource = {

const REGISTRATION_B: RemoteSource = {
uuid: 'UUID_B',
uid: 'UID_B',
id: 'B',
title: 'EXTENSION_B',
type: 'SUBSCRIPTION_MANAGEMENT',
}

const REGISTRATION_C: RemoteSource = {
uuid: 'UUID_C',
uid: 'UID_C',
id: 'C',
title: 'EXTENSION_C',
type: 'THEME_APP_EXTENSION',
}

const REGISTRATION_D: RemoteSource = {
uuid: 'UUID_D',
uid: 'UID_D',
id: 'D',
title: 'EXTENSION_D',
type: 'WEB_PIXEL_EXTENSION',
}

const REGISTRATION_FUNCTION_A: RemoteSource = {
uuid: 'FUNCTION_UUID_A',
uid: 'FUNCTION_UID_A',
id: 'FUNCTION_A',
title: 'FUNCTION A',
type: 'FUNCTION',
Expand Down Expand Up @@ -107,7 +100,7 @@ beforeAll(async () => {
},
entrySourceFilePath: '',
devUUID: 'devUUID',
uid: 'UID_A',
uid: 'UUID_A',
})

EXTENSION_A_2 = await testUIExtension({
Expand All @@ -131,7 +124,7 @@ beforeAll(async () => {
},
entrySourceFilePath: '',
devUUID: 'devUUID',
uid: 'UID_A_2',
uid: 'UIUD_A_2',
})

EXTENSION_B = await testUIExtension({
Expand All @@ -155,7 +148,7 @@ beforeAll(async () => {
},
entrySourceFilePath: '',
devUUID: 'devUUID',
uid: 'UID_B',
uid: 'UUID_B',
})

EXTENSION_B_2 = await testUIExtension({
Expand All @@ -179,7 +172,7 @@ beforeAll(async () => {
},
entrySourceFilePath: '',
devUUID: 'devUUID',
uid: 'UID_B_2',
uid: 'UUID_B_2',
})

EXTENSION_C = await testUIExtension({
Expand All @@ -203,7 +196,7 @@ beforeAll(async () => {
},
entrySourceFilePath: '',
devUUID: 'devUUID',
uid: 'UID_C',
uid: 'UUID_C',
})

EXTENSION_D = await testUIExtension({
Expand All @@ -228,7 +221,7 @@ beforeAll(async () => {
outputPath: '',
entrySourceFilePath: '',
devUUID: 'devUUID',
uid: 'UID_D',
uid: 'UUID_D',
})

FUNCTION_A = await testFunctionExtension({
Expand Down Expand Up @@ -794,7 +787,7 @@ describe('automaticMatchmaking: with Atomic Deployments enabled', () => {

// Then
const expected = {
identifiers: {'extension-a': 'UID_A'},
identifiers: {'extension-a': 'UUID_A'},
toConfirm: [],
toCreate: [EXTENSION_A_2],
toManualMatch: {local: [], remote: []},
Expand Down
15 changes: 7 additions & 8 deletions packages/app/src/cli/services/context/id-matching.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ function matchByNameAndType(
/**
* Automatically match local and remote sources if they have the same UID
*/
function matchByUID(
function matchByUUID(
local: LocalSource[],
remote: RemoteSource[],
): {
Expand All @@ -79,9 +79,9 @@ function matchByUID(
const matched: IdentifiersExtensions = {}

local.forEach((localSource) => {
const possibleMatch = remote.find((remoteSource) => remoteSource.uid === localSource.uid)
const possibleMatch = remote.find((remoteSource) => remoteSource.uuid === localSource.uid)
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
if (possibleMatch) matched[localSource.localIdentifier] = possibleMatch.uid!
if (possibleMatch) matched[localSource.localIdentifier] = possibleMatch.uuid!
})

const toCreate = local.filter((elem) => !matched[elem.localIdentifier])
Expand Down Expand Up @@ -207,15 +207,14 @@ export async function automaticMatchmaking(
identifiers: IdentifiersExtensions,
developerPlatformClient: DeveloperPlatformClient,
): Promise<MatchResult> {
const useUidMatching = developerPlatformClient.supportsAtomicDeployments
const useUuidMatching = developerPlatformClient.supportsAtomicDeployments
const ids = getExtensionIds(localSources, identifiers)
const localIds = Object.values(ids)

const existsRemotely = (local: LocalSource) =>
remoteSources.some((remote) => {
if (remote.type !== developerPlatformClient.toExtensionGraphQLType(local.graphQLType)) return false
const remoteIdField = useUidMatching ? 'uid' : 'uuid'
return ids[local.localIdentifier] === remote[remoteIdField]
return ids[local.localIdentifier] === remote.uuid
})

const {migrated: migratedFunctions, pending: pendingAfterMigratingFunctions} = migrateLegacyFunctions(
Expand All @@ -225,8 +224,8 @@ export async function automaticMatchmaking(
)
const {local, remote} = pendingAfterMigratingFunctions

const {matched, toCreate, toConfirm, toManualMatch} = useUidMatching
? matchByUID(local, remote)
const {matched, toCreate, toConfirm, toManualMatch} = useUuidMatching
? matchByUUID(local, remote)
: matchByNameAndType(local, remote)

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,6 @@ async function createExtensions(
result[extension.localIdentifier] = {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
id: extension.uid!,
uid: extension.uid,
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
uuid: extension.uid!,
type: extension.type,
Expand Down
1 change: 0 additions & 1 deletion packages/app/src/cli/services/context/identifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ export interface EnsureDeploymentIdsPresenceOptions {

export interface RemoteSource {
uuid: string
uid?: string
type: string
id: string
title: string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {fetch} from '@shopify/cli-kit/node/http'
import {businessPlatformOrganizationsRequest} from '@shopify/cli-kit/node/api/business-platform'
import {appManagementRequestDoc} from '@shopify/cli-kit/node/api/app-management'
import {BugError} from '@shopify/cli-kit/node/error'
import {randomUUID} from '@shopify/cli-kit/node/crypto'

vi.mock('@shopify/cli-kit/node/http')
vi.mock('@shopify/cli-kit/node/api/business-platform')
Expand Down Expand Up @@ -47,6 +48,7 @@ const templateDisallowedByBetaFlag: GatedExtensionTemplate = {
function moduleFromExtension(extension: ExtensionInstance) {
return {
uuid: extension.uid,
userIdentifier: extension.uid,
handle: extension.handle,
config: extension.configuration,
specification: {
Expand Down Expand Up @@ -76,6 +78,36 @@ describe('diffAppModules', () => {
expect(removed).toEqual([moduleA])
expect(updated).toEqual([moduleB])
})

// This test considers the case where there are local and remote modules, which may have slightly different properties
test('extracts the added, removed and updated modules before deployment', async () => {
// Given
const [remoteModuleA, remoteModuleB] = [moduleFromExtension(extensionA), moduleFromExtension(extensionB)]
// Under some circumstances, local UUID may differ from remote.
// So we are testing that diffing happens based on the shared userIdentifier
// property, not the UUID.
const localModuleB = {
...remoteModuleB,
uuid: randomUUID(),
}
const localModuleC = {
...moduleFromExtension(extensionC),
uuid: randomUUID(),
}

const before = [remoteModuleA, remoteModuleB]
const after = [localModuleB, localModuleC]

// When
const {added, removed, updated} = diffAppModules({currentModules: before, selectedVersionModules: after})

// Then
expect(added).toEqual([localModuleC])
expect(removed).toEqual([remoteModuleA])
// Updated returns the remote module, not the local one. This shouldn't matter
// as the module identifiers are the same.
expect(updated).toEqual([remoteModuleB])
})
})

describe('templateSpecifications', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@
appIdentifiers: MinimalAppIdentifiers,
activeAppVersion?: AppVersion,
): Promise<AllAppExtensionRegistrationsQuerySchema> {
const app = activeAppVersion || (await this.activeAppVersion(appIdentifiers))

Check warning on line 397 in packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts#L397

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.

const configurationRegistrations: ExtensionRegistration[] = []
const extensionRegistrations: ExtensionRegistration[] = []
Expand Down Expand Up @@ -902,12 +902,12 @@
}

export function diffAppModules({currentModules, selectedVersionModules}: DiffAppModulesInput): DiffAppModulesOutput {
const currentModuleUids = currentModules.map((mod) => mod.uuid)
const selectedVersionModuleUids = selectedVersionModules.map((mod) => mod.uuid)
const removed = currentModules.filter((mod) => !selectedVersionModuleUids.includes(mod.uuid))
const added = selectedVersionModules.filter((mod) => !currentModuleUids.includes(mod.uuid))
const addedUids = added.map((mod) => mod.uuid)
const updated = selectedVersionModules.filter((mod) => !addedUids.includes(mod.uuid))
const currentModuleUids = currentModules.map((mod) => mod.userIdentifier)
const selectedVersionModuleUids = selectedVersionModules.map((mod) => mod.userIdentifier)
const added = selectedVersionModules.filter((mod) => !currentModuleUids.includes(mod.userIdentifier))
const removed = currentModules.filter((mod) => !selectedVersionModuleUids.includes(mod.userIdentifier))
const removedUids = removed.map((mod) => mod.userIdentifier)
const updated = currentModules.filter((mod) => !removedUids.includes(mod.userIdentifier))
shauns marked this conversation as resolved.
Show resolved Hide resolved
shauns marked this conversation as resolved.
Show resolved Hide resolved
return {added, removed, updated}
}

Expand Down Expand Up @@ -946,8 +946,8 @@

function appModuleVersion(mod: ReleasedAppModuleFragment): Required<AppModuleVersion> {
return {
registrationId: mod.uuid,
registrationUuid: mod.uuid,
registrationId: mod.userIdentifier,
registrationUuid: mod.userIdentifier,
registrationTitle: mod.handle,
type: mod.specification.externalIdentifier,
config: mod.config,
Expand Down
Loading