Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document how to batch RTK Query mutations properly #2203

Closed
torhovland opened this issue Apr 5, 2022 · 16 comments · Fixed by #3116
Closed

Document how to batch RTK Query mutations properly #2203

torhovland opened this issue Apr 5, 2022 · 16 comments · Fixed by #3116
Milestone

Comments

@torhovland
Copy link

I feel that the documentation needs to cover the following scenario.

I have an array of objects that I need to send to the API. The API only allows mutating one object per request. What is a good way to do this while:

  • avoiding a race condition where the final cache invalidation is ignored because the second to last cache provide comes later.
  • even better, firing just one cache invalidation after all mutations have been sent.

I understand mutations can probably use a queryFn to do this, but the docs only cover how to do multiple fetches.

@torhovland torhovland changed the title Document how to batch mutations properly Document how to batch RTK Query mutations properly Apr 5, 2022
@phryneas
Copy link
Member

phryneas commented Apr 5, 2022

From the "endpoint specification" side, there is literally no difference between queries and mutations, apart that queries provideTags and mutations invalidateTags. Doing it with a queryFn is a fine way to do it - with exactly the code as you would do with a query.

@torhovland
Copy link
Author

Thanks, that helped. I was able to get it working using this:

    updateAccessLevelRequests: builder.mutation<number, AccessLevelRequest[]>({
      async queryFn(accessLevelRequests, _queryApi, _extraOptions, baseQuery) {
        for (const accessLevelRequest of accessLevelRequests) {
          await baseQuery({
            url: `access-level-requests/${accessLevelRequest.id}/`,
            method: 'PUT',
            body: accessLevelRequest,
          });
        }
        return { data: accessLevelRequests.length };
      },
      invalidatesTags: ['AccessRequests'],
    }),

I still think it would be good to show this in the docs, though.

@phryneas
Copy link
Member

phryneas commented Apr 5, 2022

PRs to the docs are always welcome :)

@torhovland
Copy link
Author

I know. For now I wanted to at least record it as an issue.

@giladv
Copy link

giladv commented Jul 3, 2022

@phryneas just wanted to add that this is not a good solution when using an auto generated RTK api file.
manually adding queries in this use case is beating the purpose.
RTK Query needs a proper way to batch multiple mutations and merge their respective invalidation tags when they're both done. otherwise there are use cases where race conditions occur.

@slashwhatever
Copy link

Agree with @giladv

I have to call two mutations on an update of a record. Two different endpoints. Both invalidate the same tag. Frequently results in race conditions and the user only sees part of the update in the UI.

And the API file is generated from OpenAPI

@markerikson
Copy link
Collaborator

Short answer is that isn't a use case we considered in the initial design, there haven't been many requests for it (other than this thread), and designing that functionality isn't a thing that's on our radar.

Not saying it's impossible or will never be done, just that it's not a thing we've spent time thinking about.

@slashwhatever
Copy link

Removing the actual implementation details for a moment, is it not a concern that two mutations called in quick succession - irrespective of what they are doing - would cause a race condition if they try an invalidate the same tag?

To me that sounds like a bug that could raise it's head in any number of scenarios outside of the one in this thread.

@markerikson
Copy link
Collaborator

@slashwhatever : I don't see it as a "bug" in the sense of "the library code is behaving in an unexpected or unintended way", no. I would potentially see it as a limitation in the design, but the library code is working as intended as far as I know.

@alinnert
Copy link

Are more use-cases helpful for you? If so:

Currently, I have a list of document ids of unknown length and I need to fetch metadata and preview content for all those documents. But every document needs its own request. And I need this metadata for further calculations, not in separate components where this wouldn't be a problem.

E.g.:

const documentIds = ['1', '2', '3', '4']

Would result in those requests:

GET /api/document/1/metadata
GET /api/document/1/preview
GET /api/document/2/metadata
GET /api/document/2/preview
etc.

@slashwhatever
Copy link

Here's an example of how I resolved my use case:

    updateAllTheThings: build.mutation<
      UpdateThingsResponse | FetchBaseQueryError,
      UpdateThingsRequest & SetOtherThingsRequest
    >({
      async queryFn(queryArg, _queryApi, _extraOptions, fetchWithBaseQuery) {
        const { thingId, foo, bar } = queryArg.body;
        const { baz } = queryArg.body;

        const updateOneThing = await fetchWithBaseQuery({
          url: `thingEndpoint/thing/${queryArg.thingId}/set-things`,
          method: "PATCH",
          body: { baz },
        });

        if (updateOneThing.error) return { error: updateOneThing.error };

        const updateOtherThing = await fetchWithBaseQuery({
          url: `otherThingEndpoint/otherThing/${queryArg.thingId}`,
          method: "PATCH",
          body: { foo, bar },
        });

        if (updateOtherThing.error) return { error: updateOtherThing.error };

        return { data: updateOtherThing.data as UpdateThingsResponse };
      },
      invalidatesTags: ["Things"],
    }),

In this specific case, the order of requests was important. If the first failed, it should not send the second and return the error. There wasn't any way around the problem that we generate our code from OpenAPI but we've dealt with that internally. You could, I guess, set up another extended api for your manually created ones and just inject it with api.injectEndpoints

@alinnert Not sure if this helps you think about how to solve for your use case, but I figured it was worth sharing in case someone gets some use from it.

@TKasperczyk
Copy link

TKasperczyk commented Jan 10, 2023

Here's another use case with a workaround.

Use case:

const { data: items, isFetching: itemsIsFetching, isSuccess: itemsIsSuccess } = useGetItemQuery();
const [ updateItemName ] = usePutItemNameMutation();
const [ updateItemDescription ] = usePutItemDescriptionMutation();

React.useEffect(() => {
    // Run only when fetching is complete
    if (itemsIsFetching || !itemsIsSuccess) return;
    // Perform actions based on the updated item list
    // ...
}, [itemsIsFetching]);

const onSave = async () => {
    await updateItemName("itemId", "someName");
    await updateItemDescription("itemId", "someDescription");
};

The item endpoint represents an object in the database. Each of its properties can be updated with a separate PUT method, which makes it easier to implement granular permissions - some users can update the description and other nested properties, while others cannot. Sometimes, we want to update two separate properties at once. This, however, results in a race condition where the re-fetch action is fired immediately after the first update. The second update is performed before the GET request finishes and doesn't trigger another refetch. This causes the useEffect hook to be fired after the first update, but before the second one, so it cannot access the updated items[number].description value. In this particular case, we don't have to send the updates sequentially, as they don't depend on each other, but that's beside the point.
We are using @rtk-query/codegen-openapi to generate and inject the endpoints, which makes it impossible to use the queryFn workaround.

Workaround:

const [skip, setSkip] = React.useState(false);
const { data: items, isFetching: itemsIsFetching, isSuccess: itemsIsSuccess } = useGetItemQuery({ skip });
//...
const onSave = async () => {
    setSkip(true); // The items list is emptied
    await updateItemName("someName");
    await updateItemDescription("someDescription");
    setSkip(false); // Triggers a reload
};

There are many drawbacks to this workaround, but it works - it disables refetching of the item list while the updates are being applied, and then enables it, which causes the useEffect hook to fire after the second update.

It would be best if we could batch multiple mutations, as proposed by giladv.

@marcus13371337
Copy link

I'm also having this issue. Consider the following example:

list: builder.query({
   providesTags: () => ["Item"]
}),
update: builder.mutation({
   invalidatesTags: () => ["Item"]
})

const [list] = useListQuery()
const [update] = useUpdateMutation()

const onChange = async () => {
    const result1 = await update(...) // Update item 1
    if ("error" in result1) {
         showErrorToast()
    } else {
        const result2 = await update(...) // Update item 2
    }
}

After onChange has run I'm left with a list of items that doesnt have the updates to item 2. It seems as you can't invalidte a query while it's running? Tried to add a manual refetch after update2 has finished but if that happens while the list-query is still fetching the invalidation seems to be invalidated?

@phryneas
Copy link
Member

@marcus13371337 You could give the CI build of #3116 a try and see if that is what you need.

@markerikson
Copy link
Collaborator

A hopeful fix for this is now available in https://github.com/reduxjs/redux-toolkit/releases/tag/v2.0.0-beta.4 ! Please try it out and let us know if it resolves the concerns.

@markerikson markerikson added this to the 2.0 milestone Oct 28, 2023
@markerikson
Copy link
Collaborator

Haven't heard anything. Will assume this is fixed unless someone says otherwise!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants