fix: in cases where both _ids and _id are used, there might be confusion

packages/dataprovider/src/buildVariables/buildData.ts

@@ -7,13 +7,14 @@ import {
   IntrospectionNonNullTypeRef,
   IntrospectionTypeRef,
 } from "graphql";
+import { uniq } from "lodash";
 import isEqual from "lodash/isEqual";
 import isNil from "lodash/isNil";
 import isObject from "lodash/isObject";
 import { IntrospectionResult } from "../constants/interfaces";
 import exhaust from "../utils/exhaust";
 import getFinalType from "../utils/getFinalType";
-import { getSanitizedFieldData } from "./sanitizeData";
+import { getSanitizedFieldData, hasFieldData } from "./sanitizeData";
 
 enum ModifiersParams {
   connect = "connect",
@@ -385,23 +386,45 @@ export const buildData = (
   }
   const data = params.data;
   const previousData = "previousData" in params ? params.previousData : null;
+
+  const allKeys = uniq([
+    ...Object.keys(data),
+    ...Object.keys(previousData ?? {}),
+  ]);
+
+  // we only deal with the changedData set
+  const changedData = Object.fromEntries(
+    allKeys
+      .filter((key) => {
+        if (!previousData) {
+          return true;
+        }
+        const value = data[key];
+        const previousValue = previousData[key];
+        if (isEqual(value, previousValue)) {
+          return false; // remove
+        }
+        if (isNil(value) && isNil(previousValue)) {
+          return false;
+        }
+        return true;
+      })
+      .map((key) => [key, data[key]]),
+  );
   return inputType.inputFields.reduce((acc, field) => {
-    const key = field.name;
+    // ignore unchanged field
+    if (!hasFieldData(changedData, field)) {
+      return acc;
+    }
     const fieldType =
       field.type.kind === "NON_NULL" ? field.type.ofType : field.type;
     // we have to handle the convenience convention that adds _id(s)  to the data
     // the sanitize function merges that with other data
-    const fieldData = getSanitizedFieldData(data, field);
-    const previousFieldData = previousData
-      ? getSanitizedFieldData(previousData, field)
-      : null;
-    // TODO in case the content of the array has changed but not the array itself?
-    if (
-      isEqual(fieldData, previousFieldData) ||
-      (isNil(previousFieldData) && isNil(fieldData))
-    ) {
-      return acc;
-    }
+    const { fieldData, previousFieldData } = getSanitizedFieldData(
+      changedData,
+      previousData,
+      field,
+    );
 
     const newVaue = buildNewInputValue(
       fieldData,
@@ -410,6 +433,7 @@ export const buildData = (
       fieldType,
       introspectionResults,
     );
+    const key = field.name;
 
     return {
       ...acc,

packages/dataprovider/src/buildVariables/buildVariables.test.ts

@@ -688,7 +688,7 @@ describe("buildVariables", () => {
           userSocialMedia_id: "newId",
         },
         previousData: {
-          userSocialMedia: "oldId",
+          userSocialMedia_id: "oldId",
         },
       };
 
@@ -707,6 +707,27 @@ describe("buildVariables", () => {
         },
       });
     });
+    it("throws when both suffixed and non suffixed version is passed", () => {
+      const params = {
+        data: {
+          id: "einstein",
+          userSocialMedia_id: "newId",
+          userSocialMedia: {
+            id: "newId",
+          },
+        },
+        previousData: {
+          userSocialMedia_id: "oldId",
+        },
+      };
+      const func = () =>
+        buildVariables(testIntrospection, options)(
+          testUserResource,
+          UPDATE,
+          params,
+        );
+      expect(func).toThrow();
+    });
 
     it("update an entity and update also it's related entity when id is the same", () => {
       const params = {

packages/dataprovider/src/buildVariables/sanitizeData.ts

@@ -1,46 +1,67 @@
 import { IntrospectionInputValue } from "graphql";
-import { isObject } from "lodash";
+import { has } from "lodash";
 
 /**
  * Due to some implementation details in react-admin, we have to add copies with suffixed keys of certain field data.
  *
- * the data contains then both the normal version and the _id version (which is just a string or an array of strings).
+ * We transform that back here.
  *
- * We cannot override the record, as users might use either the object version or the string version
+ * this function throws if both variants (suffixed and non suffixed is used) because we cannot decide which one takes precedence
  */
-
 export const getSanitizedFieldData = (
   data: Record<string, any>,
+  previousData: Record<string, any>,
+
   field: IntrospectionInputValue,
-) => {
+): { fieldData: any; previousFieldData: any } => {
   const key = field.name;
   const keyWithArraySuffix = `${key}_ids`;
-
-  if (data[keyWithArraySuffix]) {
-    // merge
-    if (data[key] && Array.isArray(data[key])) {
-      return data[key].map((entry, index) => {
-        if (isObject(entry)) {
-          return {
-            id: data[keyWithArraySuffix][index],
-            ...entry,
-          };
-        }
-        return data[keyWithArraySuffix][index];
-      });
+  if (has(data, keyWithArraySuffix)) {
+    if (has(data, key)) {
+      throw new Error(
+        `cannot update ${key} and ${keyWithArraySuffix} at the same time. Only use one in the form.`,
+      );
     }
-    return data[keyWithArraySuffix];
+    return {
+      fieldData: data[keyWithArraySuffix].map((id) => ({ id })),
+      previousFieldData: previousData
+        ? previousData[keyWithArraySuffix]?.map((id) => ({ id }))
+        : null,
+    };
   }
 
   const keyWithIdsSuffix = `${key}_id`;
-  if (data[keyWithIdsSuffix]) {
-    if (data[key] && isObject(data[key])) {
-      return {
-        id: data[keyWithIdsSuffix],
-        ...data[key],
-      };
+  if (has(data, keyWithIdsSuffix)) {
+    if (has(data, key)) {
+      throw new Error(
+        `cannot update ${key} and ${keyWithIdsSuffix} at the same time. Only use one in the form.`,
+      );
     }
-    return data[keyWithIdsSuffix];
+    return {
+      fieldData: {
+        id: data[keyWithIdsSuffix],
+      },
+      previousFieldData: previousData
+        ? {
+            id: previousData[keyWithIdsSuffix],
+          }
+        : null,
+    };
   }
-  return data[key];
+
+  return {
+    fieldData: data[key],
+    previousFieldData: previousData ? previousData[key] : null,
+  };
+};
+
+export const hasFieldData = (
+  data: Record<string, any>,
+  field: IntrospectionInputValue,
+) => {
+  return (
+    has(data, field.name) ||
+    has(data, `${field.name}_id`) ||
+    has(data, `${field.name}_ids`)
+  );
 };