Merge pull request #10977 from marmelab/fix-use-mutation-with-mutation-modes

Fix `useMutationWithMutationMode` in `optimistic` and `undoable` mode may pass invalid parameters to the mutation

Author: slax57

packages/ra-core/src/dataProvider/useMutationWithMutationMode.spec.tsx

@@ -1,7 +1,10 @@
 import * as React from 'react';
-import { render, waitFor } from '@testing-library/react';
+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
 import expect from 'expect';
-import { useMutationWithMutationMode } from './useMutationWithMutationMode';
+import {
+    useMutationWithMutationMode,
+    UseMutationWithMutationModeOptions,
+} from './useMutationWithMutationMode';
 import { CoreAdminContext } from '../core/CoreAdminContext';
 import { useDataProvider } from './useDataProvider';
 import { DataProvider } from '../types';
@@ -12,7 +15,17 @@ describe('useMutationWithMutationMode', () => {
         updateUserProfile: ({ data }: { data: any }) => Promise<{ data: any }>;
     };
 
-    const useUpdateUserProfile = (args?: { data?: any }) => {
+    const useUpdateUserProfile = (
+        args?: { data?: any },
+        options?: Pick<
+            UseMutationWithMutationModeOptions<
+                Error,
+                { data: any },
+                { data?: any }
+            >,
+            'mutationMode'
+        >
+    ) => {
         const dataProvider = useDataProvider<MyDataProvider>();
         return useMutationWithMutationMode<
             Error,
@@ -33,6 +46,7 @@ describe('useMutationWithMutationMode', () => {
             getSnapshot: () => {
                 return [];
             },
+            ...options,
         });
     };
 
@@ -91,4 +105,53 @@ describe('useMutationWithMutationMode', () => {
             });
         });
     });
+
+    it('uses the latest params at execution time in optimistic mode', async () => {
+        const dataProvider = testDataProvider({
+            updateUserProfile: jest.fn(({ data }) =>
+                Promise.resolve({ data: { id: 1, ...data } } as any)
+            ),
+        }) as MyDataProvider;
+        const Dummy = () => {
+            const [data, setData] = React.useState({ value: 'value1' });
+            const [update] = useUpdateUserProfile(
+                {
+                    data,
+                },
+                { mutationMode: 'optimistic' }
+            );
+            return (
+                <>
+                    <p>{data.value}</p>
+                    <button onClick={() => setData({ value: 'value2' })}>
+                        Update data
+                    </button>
+                    <button onClick={() => update()}>Update</button>
+                </>
+            );
+        };
+
+        render(
+            <CoreAdminContext dataProvider={dataProvider}>
+                <Dummy />
+            </CoreAdminContext>
+        );
+        fireEvent.click(screen.getByText('Update'));
+        // In case of undoable, clicking the _Update data_ button would trigger the mutation
+        fireEvent.click(screen.getByText('Update data'));
+        await screen.findByText('value2');
+        fireEvent.click(screen.getByText('Update'));
+        await waitFor(() => {
+            expect(dataProvider.updateUserProfile).toHaveBeenCalledWith({
+                data: { value: 'value1' },
+            });
+        });
+
+        // Ensure the next call uses the latest data
+        await waitFor(() => {
+            expect(dataProvider.updateUserProfile).toHaveBeenCalledWith({
+                data: { value: 'value2' },
+            });
+        });
+    });
 });

packages/ra-core/src/dataProvider/useMutationWithMutationMode.ts

@@ -55,10 +55,10 @@ export const useMutationWithMutationMode = <
         mode.current = mutationMode;
     }, [mutationMode]);
 
+    // This ref won't be updated when params change in an effect, only when the mutate callback is called (See L247)
+    // This ensures that for undoable and optimistic mutations, the params are not changed by side effects (unselectAll for instance)
+    // _after_ the mutate function has been called, while keeping the ability to change declaration time params _until_ the mutation is called.
     const paramsRef = useRef<Partial<TVariables>>(params);
-    useEffect(() => {
-        paramsRef.current = params;
-    }, [params]);
 
     // Ref that stores the snapshot of the state before the mutation to allow reverting it
     const snapshot = useRef<Snapshot>([]);
@@ -96,7 +96,6 @@ export const useMutationWithMutationMode = <
         {
             mutationKey,
             mutationFn: async params => {
-                const callTimeParams = { ...paramsRef.current, ...params };
                 if (params == null) {
                     throw new Error(
                         'useMutationWithMutationMode mutation requires parameters'
@@ -105,7 +104,7 @@ export const useMutationWithMutationMode = <
 
                 return (
                     mutateWithMiddlewares
-                        .current(callTimeParams as TVariables)
+                        .current(params as TVariables)
                         // Middlewares expect the data property of the dataProvider response
                         .then(({ data }) => data)
                 );
@@ -210,12 +209,15 @@ export const useMutationWithMutationMode = <
             ...otherCallTimeOptions
         } = callTimeOptions;
 
+        // store the hook time params *at the moment of the call*
+        // because they may change afterwards, which would break the undoable mode
+        // as the previousData would be overwritten by the optimistic update
+        paramsRef.current = params;
+
         // Store the mutation with middlewares to avoid losing them if the calling component is unmounted
         if (getMutateWithMiddlewares) {
             mutateWithMiddlewares.current = getMutateWithMiddlewaresEvent(
                 (params: TVariables) => {
-                    // Store the final parameters which might have been changed by middlewares
-                    paramsRef.current = params;
                     return mutationFnEvent(params);
                 }
             );
@@ -230,11 +232,6 @@ export const useMutationWithMutationMode = <
         callTimeOnError.current = onError;
         callTimeOnSettled.current = onSettled;
 
-        // store the hook time params *at the moment of the call*
-        // because they may change afterwards, which would break the undoable mode
-        // as the previousData would be overwritten by the optimistic update
-        paramsRef.current = params;
-
         if (mutationMode) {
             mode.current = mutationMode;
         }
@@ -288,7 +285,7 @@ export const useMutationWithMutationMode = <
             if (onSuccess) {
                 onSuccess(
                     optimisticResult,
-                    callTimeParams,
+                    { ...paramsRef.current, ...callTimeParams },
                     {
                         snapshot: snapshot.current,
                     },
@@ -304,7 +301,7 @@ export const useMutationWithMutationMode = <
             ) {
                 mutationOptions.onSuccess(
                     optimisticResult,
-                    callTimeParams,
+                    { ...paramsRef.current, ...callTimeParams },
                     {
                         snapshot: snapshot.current,
                     },
@@ -319,24 +316,36 @@ export const useMutationWithMutationMode = <
 
         if (mode.current === 'optimistic') {
             // call the mutate method without success side effects
-            return mutation.mutate(callTimeParams);
+            return mutation.mutate({
+                ...paramsRef.current,
+                ...callTimeParams,
+            });
         } else {
             // Undoable mutation: add the mutation to the undoable queue.
             // The Notification component will dequeue it when the user confirms or cancels the message.
             addUndoableMutation(({ isUndo }) => {
                 if (isUndo) {
                     if (onUndo) {
-                        onUndoEvent(callTimeParams, {
-                            mutationMode: mode.current,
-                        });
+                        onUndoEvent(
+                            {
+                                ...paramsRef.current,
+                                ...callTimeParams,
+                            },
+                            {
+                                mutationMode: mode.current,
+                            }
+                        );
                     }
                     // rollback
                     snapshot.current.forEach(([key, value]) => {
                         queryClient.setQueryData(key, value);
                     });
                 } else {
                     // call the mutate method without success side effects
-                    mutation.mutate(callTimeParams);
+                    mutation.mutate({
+                        ...paramsRef.current,
+                        ...callTimeParams,
+                    });
                 }
             });
         }