Fixing a bug causing infinite PATCH requests (#2457)

* Changing from 'any' to 'unknown' and fixing the root of the problem

* Adding a regression test in Cypress

* Fixing data model assumptions

---------

Co-authored-by: Ole Martin Handeland <git@olemartin.org>

src/features/formData/jsonPatch/createPatch.test.ts

@@ -302,12 +302,22 @@ describe('createPatch', () => {
     });
   });
 
-  describe('should avoid overwriting differing simple arrays in current', () => {
+  describe('should overwrite simple arrays in current if the next value is new', () => {
     testPatch({
       prev: { a: ['foo', 'bar', 'baz', 'qux'] },
       next: { a: ['foo', 'bar2', 'baz', 'qux'] },
       current: { a: ['foo', 'bar', 'baz', 'qux'] },
-      final: { a: ['foo', 'bar', 'baz', 'qux'] },
+      final: { a: ['foo', 'bar2', 'baz', 'qux'] },
+      expectedPatch: [{ op: 'replace', path: '/a', value: ['foo', 'bar2', 'baz', 'qux'] }],
+    });
+  });
+
+  describe('should avoid overwriting simple arrays in current if the current value is newer', () => {
+    testPatch({
+      prev: { a: ['foo', 'bar', 'baz', 'qux'] },
+      next: { a: ['foo', 'bar2', 'baz', 'qux'] },
+      current: { a: ['foo', 'bar', 'baz', 'qux2'] },
+      final: { a: ['foo', 'bar', 'baz', 'qux2'] },
       expectedPatch: [],
     });
   });

src/features/formData/jsonPatch/createPatch.ts

@@ -43,19 +43,17 @@ export function createPatch({ prev, next, current }: Props<object>): JsonPatch {
   return patch;
 }
 
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-function isObject(value: any): value is object {
+function isObject(value: unknown): value is object {
   return typeof value === 'object' && value !== null && !Array.isArray(value);
 }
 
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-function compareAny(props: CompareProps<any>) {
+function compareAny(props: CompareProps<unknown>) {
   const { prev, next } = props;
   if (isObject(prev) && isObject(next)) {
-    return compareObjects(props);
+    return compareObjects(props as CompareProps<object>);
   }
   if (Array.isArray(prev) && Array.isArray(next)) {
-    return compareArrays(props);
+    return compareArrays(props as CompareProps<unknown[]>);
   }
   compareValues(props);
 }
@@ -74,8 +72,7 @@ function compareObjects({ prev, next, current, path, ...rest }: CompareProps<obj
  * This comparison function is used to determine if two values in an array can be considered the same. This is used to
  * determine if an item (i.e. a row in a repeating group) has been removed or added.
  */
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-function isSameRow(left: any, right: any): boolean {
+function isSameRow(left: unknown, right: unknown): boolean {
   if (isObject(left) && isObject(right)) {
     if (!(ALTINN_ROW_ID in left && ALTINN_ROW_ID in right)) {
       window.logWarn(
@@ -102,20 +99,20 @@ function isSameRow(left: any, right: any): boolean {
  * produce the JsonPatch to create. Do not be fooled by the format returned from getPatch, it is not a JsonPatch
  * even if it looks like it at a glance.
  */
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-function compareArrays({ prev, next, current, hasCurrent, patch, path }: CompareProps<any[]>) {
+function compareArrays({ prev, next, current, hasCurrent, patch, path }: CompareProps<unknown[]>) {
   const diff = getPatch(prev, next, isSameRow);
-  const allNextValuesIsString = next.length > 0 && next.every((item) => typeof item === 'string');
-  if (allNextValuesIsString) {
+  const allValues = [...next, ...prev, ...(current || [])];
+  const allValuesAreStrings = allValues.length > 0 && allValues.every((item) => typeof item === 'string');
+  if (allValuesAreStrings) {
     if (next.length > 0) {
-      if (current) {
+      if (current && !deepEqual(current, prev)) {
         // Special case. If we have a current model, and that model is an array of strings that is different from
         // next, we'll keep the current value. This can happen if you for example have a file uploader that saves
         // IDs to a list in the data model. When one attachment got uploaded, and got saved to the backend, we started
         // uploading a new attachment (which got added to the current model), but before the previous save response
         // came back from the backend.
         return;
-      } else {
+      } else if (!current) {
         patch.push({
           op: 'test',
           path: pointer(path),
@@ -128,10 +125,17 @@ function compareArrays({ prev, next, current, hasCurrent, patch, path }: Compare
         value: next,
       });
     } else {
-      patch.push({
-        op: 'remove',
-        path: pointer(path),
-      });
+      if (current && !deepEqual(current, prev)) {
+        // Same special case as above, but for when the next array is empty (i.e. the backend hasn't seen any values in
+        // this array yet, but while saving the user has added some values to the current model, for example by
+        // uploading an attachment which adds a UUID reference).
+        return;
+      } else {
+        patch.push({
+          op: 'remove',
+          path: pointer(path),
+        });
+      }
     }
     return;
   }
@@ -196,8 +200,7 @@ function compareArrays({ prev, next, current, hasCurrent, patch, path }: Compare
   patch.push(...childPatches);
 }
 
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-function compareValues({ prev, next, hasCurrent, current, patch, path }: CompareProps<any>) {
+function compareValues({ prev, next, hasCurrent, current, patch, path }: CompareProps<unknown>) {
   if (prev === next) {
     return;
   }

test/e2e/integration/frontend-test/all-process-steps.ts

@@ -370,6 +370,7 @@ const knownDataModels: { [key: string]: unknown } = {
           Name: 'Katt',
           NumLegs: 4,
           Color: 'BLACK,BROWN',
+          Colors: [],
           Comments: [
             {
               Type: 'CRITICISM',
@@ -388,6 +389,7 @@ const knownDataModels: { [key: string]: unknown } = {
           Name: 'Tiger',
           NumLegs: 5,
           Color: 'RED,PINK',
+          Colors: [],
           Comments: [
             {
               Type: 'SUGGESTION',

test/e2e/integration/frontend-test/auto-save-behavior.ts

@@ -235,4 +235,41 @@ describe('Auto save behavior', () => {
         });
     });
   });
+
+  it('new list items from the backend should never trigger runaway saving', () => {
+    let formDataReqCounter = 0;
+    cy.intercept('PATCH', '**/data/**', () => {
+      formDataReqCounter++;
+    }).as('saveFormData');
+
+    cy.gotoHiddenPage('conflicting-options');
+    cy.get('#group-animals-table-body tr').eq(0).find('td').eq(3).should('have.text', 'Svart');
+
+    cy.waitUntilSaved();
+    cy.get('#group-animals').then(() => {
+      // The form has loaded completely, and the labels have been saved to the data model. Let's reset the counter
+      // to make sure we only count the requests that are made after this point.
+      formDataReqCounter = 0;
+    });
+
+    cy.findByRole('button', { name: 'Rediger Katt' }).click();
+    cy.get('#animalColor-0').click();
+    cy.findByRole('option', { name: 'Grønn' }).click(); // The problematic list is only added if green is selected
+    cy.get('body').type('{esc}'); // Close the combobox
+    cy.findByRole('button', { name: 'Lagre og lukk' }).click();
+
+    // We used to have a bug where the backend code that splits the colors into separate strings in a List<string>
+    // in the data model would cause the frontend to think the data model in the frontend was newer than the backend,
+    // meaning it would save the data model with an empty list, causing the backend to again split the colors into
+    // separate strings, and such an infinite loop of saving would occur. In order to make sure this never happens
+    // again, we'll do what we shouldn't, and wait for at least 2 debounce timeouts to make sure we don't trigger
+    // runaway saving.
+
+    // eslint-disable-next-line cypress/no-unnecessary-waiting
+    cy.wait(1000);
+
+    cy.then(() => {
+      expect(formDataReqCounter).to.be.eq(1);
+    });
+  });
 });