fix: validation in put product

Author: maxakuru

src/catalog/update.js

@@ -10,8 +10,7 @@
  * governing permissions and limitations under the License.
  */
 
-/* eslint-disable no-await-in-loop */
-
+import { assertValidProduct } from '../utils/product.js';
 import { callAdmin } from '../utils/admin.js';
 import { errorResponse, errorWithResponse } from '../utils/http.js';
 import { saveProducts } from '../utils/r2.js';
@@ -41,44 +40,78 @@ export async function putProduct(ctx, config, product) {
  */
 export async function handleProductSaveRequest(ctx, config, request) {
   try {
-    let requestBody;
-
-    try {
-      requestBody = await request.json();
-    } catch (jsonError) {
-      ctx.log.error('Invalid JSON in request body:', jsonError);
-      return errorResponse(400, 'invalid JSON');
-    }
+    let product;
 
     if (config.sku === '*') {
       return errorResponse(501, 'not implemented');
+    } else {
+      try {
+        product = await request.json();
+      } catch (jsonError) {
+        ctx.log.error('Invalid JSON in request body:', jsonError);
+        return errorResponse(400, 'invalid JSON');
+      }
+
+      try {
+        assertValidProduct(product);
+      } catch (e) {
+        return errorResponse(400, e.message);
+      }
     }
 
-    const product = await putProduct(ctx, config, requestBody);
-    const products = [product];
+    const { base: _ = undefined, ...otherPatterns } = config.confEnvMap[config.env] ?? {};
+    const matchedPathPatterns = Object.entries(otherPatterns)
+      .reduce((acc, [pattern, matchConf]) => {
+        // find only configs that match the provided store & view codes
+        if (config.storeCode === matchConf.storeCode
+          && config.storeViewCode === matchConf.storeViewCode) {
+          acc.push(pattern);
+        }
+        return acc;
+      }, []);
 
-    const { base: _ = undefined, ...otherPatterns } = (config.env in config.confEnvMap)
-      ? config.confEnvMap[config.env]
-      : {};
-    const matchedPathPatterns = Object.keys(otherPatterns);
+    if (!matchedPathPatterns.length) {
+      return errorResponse(404, 'no path patterns found');
+    }
 
-    if (matchedPathPatterns.length !== 0) {
-      for (const purgeProduct of products) {
-        for (const pattern of matchedPathPatterns) {
-          let path = pattern.replace('{{sku}}', purgeProduct.sku);
+    await putProduct(ctx, config, product);
 
-          if (path.includes('{{urlkey}}') && purgeProduct.urlKey) {
-            path = path.replace('{{urlkey}}', purgeProduct.urlKey);
-          }
+    const purgePaths = matchedPathPatterns.map(
+      (pattern) => pattern
+        .replace('{{sku}}', product.sku)
+        .replace('{{urlkey}}', product.urlKey),
+    );
 
-          for (const op of ['preview', 'live']) {
-            const response = await callAdmin(config, op, path, { method: 'post' });
-            if (!response.ok) {
-              return errorResponse(400, `failed to ${op} product`);
-            }
+    const errors = [];
+    await Promise.all(
+      purgePaths.map(async (path) => {
+        for (const op of ['preview', 'live']) {
+          // eslint-disable-next-line no-await-in-loop
+          const response = await callAdmin(config, op, path, { method: 'POST' });
+          if (!response.ok) {
+            errors.push({
+              op,
+              path,
+              status: response.status,
+              message: response.headers.get('x-error') ?? response.statusText,
+            });
+            break; // don't hit live if preview fails
           }
         }
-      }
+      }),
+    );
+
+    if (errors.length) {
+      // use highest error code as status,
+      // so 5xx will be forwarded but all 404s will be treated as a 404
+      const status = errors.reduce((errCode, { status: code }) => Math.max(errCode, code), 0);
+      return new Response(JSON.stringify({ errors }), {
+        status,
+        headers: {
+          'Content-Type': 'application/json',
+          'x-error': 'purge errors',
+        },
+      });
     }
 
     return new Response(undefined, { status: 201 });

src/utils/product.js

@@ -58,3 +58,13 @@ export function findProductImage(product, variants = []) {
   }
   return variants.find((v) => v.images?.length)?.images?.[0];
 }
+
+/**
+ * @param {any} product
+ * @returns {asserts product is Product}
+ */
+export function assertValidProduct(product) {
+  if (typeof product !== 'object' || !product.sku) {
+    throw new Error('Invalid product');
+  }
+}

test/catalog/update.test.js

@@ -10,28 +10,28 @@
  * governing permissions and limitations under the License.
  */
 
+// @ts-nocheck
+
 import { strict as assert } from 'assert';
 import sinon from 'sinon';
 import esmock from 'esmock';
 
 describe('Product Save Tests', () => {
   let putProduct;
+  /** @type {import('../../src/catalog/update.js').handleProductSaveRequest} */
   let handleProductSaveRequest;
+  /** @type {sinon.SinonStub} */
   let saveProductsStub;
+  /** @type {sinon.SinonStub} */
   let callAdminStub;
-  let errorResponseStub;
-  let errorWithResponseStub;
 
   beforeEach(async () => {
     saveProductsStub = sinon.stub();
     callAdminStub = sinon.stub();
-    errorResponseStub = sinon.stub();
-    errorWithResponseStub = sinon.stub();
 
     const mocks = {
       '../../src/utils/r2.js': { saveProducts: saveProductsStub },
       '../../src/utils/admin.js': { callAdmin: callAdminStub },
-      '../../src/utils/http.js': { errorResponse: errorResponseStub, errorWithResponse: errorWithResponseStub },
     };
 
     ({ putProduct, handleProductSaveRequest } = await esmock('../../src/catalog/update.js', mocks));
@@ -43,16 +43,10 @@ describe('Product Save Tests', () => {
 
   describe('putProduct', () => {
     it('should throw error when product SKU is missing', async () => {
-      const error = new Error('invalid request body: missing sku');
-      errorWithResponseStub.throws(error);
-
       const product = {};
       await assert.rejects(async () => {
         await putProduct({}, {}, product);
       }, /invalid request body: missing sku/);
-
-      assert(errorWithResponseStub.calledOnce);
-      assert(errorWithResponseStub.calledWith(400, 'invalid request body: missing sku'));
     });
 
     it('should save the product when SKU is present', async () => {
@@ -72,13 +66,9 @@ describe('Product Save Tests', () => {
       const ctx = { log: { error: sinon.stub() } };
       const request = { json: sinon.stub().resolves({ sku: '1234' }) };
 
-      const mockResponse = new Response(null, { status: 501, headers: { 'x-error': 'not implemented' } });
-      errorResponseStub.returns(mockResponse);
-
       const response = await handleProductSaveRequest(ctx, config, request);
 
       assert.equal(response.status, 501);
-      assert(errorResponseStub.calledWith(501, 'not implemented'));
       assert.equal(response.headers.get('x-error'), 'not implemented');
     });
 
@@ -106,7 +96,7 @@ describe('Product Save Tests', () => {
       assert.equal(callAdminStub.callCount, 4);
     });
 
-    it('should skip calling callAdmin when confMap has no matching env', async () => {
+    it('should return 404 when no matching path patterns found', async () => {
       const config = {
         sku: '1234',
         confEnvMap: {
@@ -124,10 +114,11 @@ describe('Product Save Tests', () => {
       const response = await handleProductSaveRequest(ctx, config, request);
 
       assert(callAdminStub.notCalled);
-      assert.equal(response.status, 201);
+      assert.equal(response.status, 404);
+      assert.equal(response.headers.get('x-error'), 'no path patterns found');
     });
 
-    it('should return error when callAdmin fails', async () => {
+    it('should return error when purging fails', async () => {
       const config = {
         sku: '1234',
         confEnvMap: {
@@ -137,37 +128,38 @@ describe('Product Save Tests', () => {
         },
         env: 'test',
       };
-      const ctx = { log: { error: sinon.stub() } };
+      const ctx = { log: console };
       const request = { json: sinon.stub().resolves({ sku: '1234', urlKey: 'product-url-key' }) };
 
       saveProductsStub.resolves();
-      callAdminStub.onFirstCall().resolves({ ok: false });
-
-      const mockResponse = new Response(null, { status: 400, headers: { 'x-error': 'failed to preview product' } });
-      errorResponseStub.returns(mockResponse);
+      callAdminStub.onFirstCall().resolves(new Response('', { status: 500, headers: { 'x-error': 'bad thing happen' } }));
 
       const response = await handleProductSaveRequest(ctx, config, request);
 
-      assert.equal(response.status, 400);
-      assert(callAdminStub.calledOnce);
-      assert(errorResponseStub.calledWith(400, 'failed to preview product'));
-      assert.equal(response.headers.get('x-error'), 'failed to preview product');
+      assert.equal(response.status, 500);
+      assert.ok(callAdminStub.calledOnce);
+      assert.equal(response.headers.get('x-error'), 'purge errors');
+      const respBody = await response.json();
+      assert.deepStrictEqual(respBody, {
+        errors: [
+          {
+            op: 'preview',
+            path: '/path/to/1234',
+            status: 500,
+            message: 'bad thing happen',
+          },
+        ],
+      });
     });
 
     it('should return 400 if request.json throws a JSON parsing error', async () => {
       const config = { sku: '1234', confEnvMap: {} };
       const ctx = { log: { error: sinon.stub() } };
       const request = { json: sinon.stub().rejects(new Error('Unexpected token < in JSON at position 0')) };
 
-      const mockResponse = new Response(null, { status: 400, headers: { 'x-error': 'invalid JSON' } });
-      errorResponseStub.returns(mockResponse);
-
       const response = await handleProductSaveRequest(ctx, config, request);
 
       assert.equal(response.status, 400);
-
-      assert(errorResponseStub.calledWith(400, 'invalid JSON'));
-
       assert.equal(response.headers.get('x-error'), 'invalid JSON');
 
       assert(ctx.log.error.calledOnce);