Update contentful client to use system id

.changeset/twenty-garlics-count.md

@@ -0,0 +1,14 @@
+---
+"saleor-app-cms-v2": minor
+---
+
+Changed how Contentful ID is created.
+
+Previously, Saleor's ID was used to create entry in Contentful. 
+However, this implementation was failing when special characters in ID occurred (like "=="). 
+
+Now, entry in Contentful is created using it's own system ID, generated by Contentful itself.
+
+App will first query entries via variant ID field, so it still works without a breaking change.
+
+It's recommended to set "unique" parameter on variant ID field

apps/cms-v2/src/modules/providers/contentful/contentful-bulk-sync-processor.ts

@@ -12,7 +12,7 @@ export class ContentfulBulkSyncProcessor implements BulkSyncProcessor {
 
   async uploadProducts(
     products: BulkImportProductFragment[],
-    hooks: BulkSyncProcessorHooks
+    hooks: BulkSyncProcessorHooks,
   ): Promise<void> {
     const contentful = new ContentfulClient({
       accessToken: this.config.authToken,
@@ -41,7 +41,9 @@ export class ContentfulBulkSyncProcessor implements BulkSyncProcessor {
               },
             })
             .then((r) => {
-              if (r?.metadata) {
+              const resp = Array.isArray(r) ? r[0] : r;
+
+              if (resp?.metadata) {
                 if (hooks.onUploadSuccess) {
                   hooks.onUploadSuccess({ variantId: variant.id });
                 }

apps/cms-v2/src/modules/providers/contentful/contentful-client.test.ts

@@ -50,7 +50,8 @@ const mockGetEnvironment = vi.fn();
 const mockGetEnvironments = vi.fn();
 const mockGetEnvEntry = vi.fn();
 const mockGetContentTypes = vi.fn();
-const mockCreateEntryWithId = vi.fn();
+const mockCreateEntry = vi.fn();
+const mockGetEntries = vi.fn();
 
 const mockContentfulSdk: ContentfulApiClientChunk = {
   getSpace: mockGetSpace.mockReturnValue(
@@ -60,10 +61,13 @@ const mockContentfulSdk: ContentfulApiClientChunk = {
           items: [{}],
         }),
         getEntry: mockGetEnvEntry.mockReturnValue({}),
-        createEntryWithId: mockCreateEntryWithId.mockReturnValue({}),
+        getEntries: mockGetEntries.mockReturnValue({
+          items: [],
+        }),
+        createEntry: mockCreateEntry.mockReturnValue({}),
       }),
       getEnvironments: mockGetEnvironments.mockReturnValue({}),
-    })
+    }),
   ),
 };
 
@@ -78,7 +82,7 @@ describe("ContentfulClient", () => {
         accessToken: "test-token",
         space: "test-space",
       },
-      () => mockContentfulSdk
+      () => mockContentfulSdk,
     );
   });
 
@@ -106,6 +110,7 @@ describe("ContentfulClient", () => {
       };
 
       mockGetEnvEntry.mockReturnValue(mockEntry);
+      mockGetEntries.mockReturnValue({ items: [mockEntry] });
 
       const mockConfig = getMockContenfulConfiguration();
       const mockMapping = mockConfig.productVariantFieldsMapping;
@@ -117,7 +122,14 @@ describe("ContentfulClient", () => {
         variant: mockVariant,
       });
 
-      expect(mockGetEnvEntry).toHaveBeenCalledWith(mockVariant.id);
+      /**
+       * Query Contentful first, to get the entry. content_type is configured by user.
+       * Then field path, matching variant id, also provided by user.
+       */
+      expect(mockGetEntries).toHaveBeenCalledWith({
+        content_type: mockConfig.contentId,
+        [`fields.${mockConfig.productVariantFieldsMapping.variantId}`]: mockVariant.id,
+      });
 
       /**
        * Fields must reflect mapping config to variant real data
@@ -156,6 +168,7 @@ describe("ContentfulClient", () => {
       };
 
       mockGetEnvEntry.mockReturnValue(mockEntry);
+      mockGetEntries.mockReturnValue({ items: [mockEntry] });
 
       const mockConfig = getMockContenfulConfiguration();
       const mockVariant = getMockWebhookProductVariant();
@@ -165,13 +178,20 @@ describe("ContentfulClient", () => {
         variant: { id: mockVariant.id },
       });
 
-      expect(mockGetEnvEntry).toHaveBeenCalledWith(mockVariant.id);
+      /**
+       * Query Contentful first, to get the entry. content_type is configured by user.
+       * Then field path, matching variant id, also provided by user.
+       */
+      expect(mockGetEntries).toHaveBeenCalledWith({
+        content_type: mockConfig.contentId,
+        [`fields.${mockConfig.productVariantFieldsMapping.variantId}`]: mockVariant.id,
+      });
       expect(mockEntry.delete).toHaveBeenCalled();
     });
   });
 
   describe("uploadProductVariant", () => {
-    it("Calls contentful createEntryWithId method with correct mapped fields", async () => {
+    it("Calls contentful createEntry method with correct mapped fields", async () => {
       const mockConfig = getMockContenfulConfiguration();
       const mockMapping = mockConfig.productVariantFieldsMapping;
 
@@ -182,7 +202,7 @@ describe("ContentfulClient", () => {
         variant: mockVariant,
       });
 
-      expect(mockCreateEntryWithId).toHaveBeenCalledWith(mockConfig.contentId, mockVariant.id, {
+      expect(mockCreateEntry).toHaveBeenCalledWith(mockConfig.contentId, {
         fields: {
           [mockMapping.productId]: {
             "en-US": mockVariant.product.id,
@@ -212,6 +232,9 @@ describe("ContentfulClient", () => {
       const mockConfig = getMockContenfulConfiguration();
       const mockMapping = mockConfig.productVariantFieldsMapping;
 
+      mockGetEnvEntry.mockReturnValue(undefined);
+      mockGetEntries.mockReturnValue({ items: [] });
+
       const mockVariant = getMockWebhookProductVariant();
 
       await contentfulClient.upsertProductVariant({
@@ -220,7 +243,7 @@ describe("ContentfulClient", () => {
       });
 
       expect(mockGetEnvEntry).not.toHaveBeenCalled();
-      expect(mockCreateEntryWithId).toHaveBeenCalledWith(mockConfig.contentId, mockVariant.id, {
+      expect(mockCreateEntry).toHaveBeenCalledWith(mockConfig.contentId, {
         fields: {
           [mockMapping.productId]: {
             "en-US": mockVariant.product.id,
@@ -244,30 +267,26 @@ describe("ContentfulClient", () => {
       });
     });
 
-    it("Calls update method if SDK returned 409 error", async () => {
+    it("Calls update method if entries exist", async () => {
       const mockConfig = getMockContenfulConfiguration();
       const mockMapping = mockConfig.productVariantFieldsMapping;
 
       const mockVariant = getMockWebhookProductVariant();
 
-      mockCreateEntryWithId.mockRejectedValue({
-        message: JSON.stringify({
-          status: 409,
-        }),
-      });
-
       const mockEntry = {
         fields: {},
         update: vi.fn().mockReturnValue(Promise.resolve({})),
       };
 
       mockGetEnvEntry.mockReturnValue(mockEntry);
+      mockGetEntries.mockReturnValue({ items: [mockEntry] });
 
       await contentfulClient.upsertProductVariant({
         configuration: mockConfig,
         variant: mockVariant,
       });
 
+      // todo fix
       expect(mockEntry.fields).toEqual({
         [mockMapping.productId]: {
           "en-US": mockVariant.product.id,

apps/cms-v2/src/modules/providers/contentful/contentful-client.ts

@@ -1,15 +1,11 @@
-import { createClient, ClientAPI } from "contentful-management";
+import { createClient, ClientAPI, Environment } from "contentful-management";
 import { WebhookProductVariantFragment } from "../../../../generated/graphql";
 import { ContentfulProviderConfig } from "@/modules/configuration";
 import { z } from "zod";
 
 import * as Sentry from "@sentry/nextjs";
 import { createLogger } from "@saleor/apps-shared";
 
-const ContentfulErrorMessageSchema = z.object({
-  status: z.number(),
-});
-
 type ConstructorOptions = {
   space: string;
   accessToken: string;
@@ -82,6 +78,23 @@ export class ContentfulClient {
     };
   };
 
+  private getEntriesBySaleorId =
+    ({
+      contentId,
+      env,
+      variantIdFieldName,
+    }: {
+      env: Environment;
+      contentId: string;
+      variantIdFieldName: string;
+    }) =>
+    (saleorId: string) => {
+      return env.getEntries({
+        content_type: contentId,
+        [`fields.${variantIdFieldName}`]: saleorId,
+      });
+    };
+
   async getContentTypes(env: string) {
     this.logger.trace("Attempting to get content types");
 
@@ -116,28 +129,48 @@ export class ContentfulClient {
     const space = await this.client.getSpace(this.space);
     const env = await space.getEnvironment(configuration.environment);
 
-    const entry = await env.getEntry(variant.id);
-
-    entry.fields = this.mapVariantToConfiguredFields(
-      variant,
-      configuration.productVariantFieldsMapping,
+    const contentEntries = await this.getEntriesBySaleorId({
+      contentId: configuration.contentId,
+      env,
+      variantIdFieldName: configuration.productVariantFieldsMapping.variantId,
+    })(variant.id);
+
+    return Promise.all(
+      contentEntries.items.map((item) => {
+        item.fields = this.mapVariantToConfiguredFields(
+          variant,
+          configuration.productVariantFieldsMapping,
+        );
+
+        return item.update();
+      }),
     );
-
-    return entry.update();
   }
 
   async deleteProductVariant(opts: {
     configuration: ContentfulProviderConfig.FullShape;
     variant: Pick<WebhookProductVariantFragment, "id">;
   }) {
-    this.logger.debug("Attempting to delete product variant");
+    this.logger.debug({ variantId: opts.variant.id }, "Attempting to delete product variant");
 
     const space = await this.client.getSpace(this.space);
     const env = await space.getEnvironment(opts.configuration.environment);
+    const contentEntries = await this.getEntriesBySaleorId({
+      contentId: opts.configuration.contentId,
+      env,
+      variantIdFieldName: opts.configuration.productVariantFieldsMapping.variantId,
+    })(opts.variant.id);
 
-    const entry = await env.getEntry(opts.variant.id);
+    this.logger.trace(contentEntries, "Found entries to delete");
 
-    return await entry.delete();
+    /**
+     * In general it should be only one item, but in case of duplication run through everything
+     */
+    return Promise.all(
+      contentEntries.items.map(async (item) => {
+        return item.delete();
+      }),
+    );
   }
 
   async uploadProductVariant({
@@ -156,42 +189,53 @@ export class ContentfulClient {
      * TODO: add translations
      * TODO: - should it create published? is draft
      */
-    return env.createEntryWithId(configuration.contentId, variant.id, {
+    return env.createEntry(configuration.contentId, {
       fields: this.mapVariantToConfiguredFields(variant, configuration.productVariantFieldsMapping),
     });
   }
 
-  async upsertProductVariant(opts: {
+  async upsertProductVariant({
+    configuration,
+    variant,
+  }: {
     configuration: ContentfulProviderConfig.FullShape;
     variant: WebhookProductVariantFragment;
   }) {
     this.logger.debug("Attempting to upsert product variant");
 
     try {
-      this.logger.trace("Attempting to upload product variant first");
-
-      return await this.uploadProductVariant(opts);
-    } catch (e: unknown) {
-      this.logger.trace("Upload failed");
-
-      if (typeof e !== "object" || e === null) {
-        Sentry.captureMessage("Contentful error is not expected shape");
-        Sentry.captureException(e);
+      const space = await this.client.getSpace(this.space);
+      const env = await space.getEnvironment(configuration.environment);
 
-        throw e;
-      }
+      const entries = await this.getEntriesBySaleorId({
+        contentId: configuration.contentId,
+        env,
+        variantIdFieldName: configuration.productVariantFieldsMapping.variantId,
+      })(variant.id);
 
-      const parsedError = ContentfulErrorMessageSchema.parse(JSON.parse((e as Error).message));
+      this.logger.trace(entries, "Found entries");
 
-      if (parsedError.status === 409) {
-        this.logger.trace("Contentful returned 409 status, will try to update instead");
+      if (entries.items.length > 0) {
+        this.logger.debug("Found existing entry, will update");
+        Sentry.addBreadcrumb({
+          message: "Found entry for variant",
+          level: "debug",
+        });
 
-        return this.updateProductVariant(opts);
+        return this.updateProductVariant({ configuration, variant });
       } else {
-        Sentry.captureMessage("Contentful error failed and is not handled");
-        Sentry.captureException(e);
-        throw e;
+        this.logger.debug("No existing entry found, will create");
+        Sentry.addBreadcrumb({
+          message: "Did not found entry for variant",
+          level: "debug",
+        });
+
+        return this.uploadProductVariant({ configuration, variant });
       }
+    } catch (err) {
+      Sentry.captureException(err);
+
+      throw err;
     }
   }
 }