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

Bug - mutations with optimisticUpdate are failing since 2.9.1 #1878

Closed
roomman opened this issue Nov 20, 2024 · 12 comments
Closed

Bug - mutations with optimisticUpdate are failing since 2.9.1 #1878

roomman opened this issue Nov 20, 2024 · 12 comments
Milestone

Comments

@roomman
Copy link
Contributor

roomman commented Nov 20, 2024

@ymc9 I've updated to 2.9.1 which includes your PR #1842, and I am seeing failing mutations, and no network requests being made, when optimisticUpdate: true.

The cases that are failing include optional "belongs-to" relationships where no relationship has yet been formed. For example:

model SomeModel {
  id                           String   @id @default(uuid())
  title                        String
  business               Business?          @relation(fields: [businessId], references: [id], onDelete: Cascade)
  businessId            String?

}

I am seeing this error:
Error - there was an error updating SomeModel TypeError: Cannot convert undefined or null to object

The error is about to the "business" relationship from the above schema, and an evaluation on this line from mutator.ts:

for (const [key, value] of Object.entries(resultData)) {

Because no business has been related to SomeModel the value of both business and businessId is null. Your change expects either an array or an object.

If I set optimisticUpdates: false for the mutation the error goes away and mutations are successful.

Should L165 be doing a null check?

@roomman
Copy link
Contributor Author

roomman commented Nov 21, 2024

@ymc9 I've created a minimal reproduction.

Set up:

  • Clone the repo
  • Run npm install
  • Run npm run generate
  • Run npm run seed
  • Run npm run dev

Open your browser and you will see that you can click on an "Enter Competition" button for any Competition, including those with no related Business and it works.

Next run npm run up to install the latest and greatest. Run npm run dev again and retry entering a competition. Check the dev tools and see failed mutations. Under state your should see the following error: {"name":"TypeError","message":"Cannot convert undefined or null to object"}

Got to src/app/page.tsx L37 and comment out the optimistic update:

  // create a new competition entry
  const { mutate: enterCompetition } = useCreateCompetitionEntry({
    // optimisticUpdate: true,
  });

Refresh and retry entering a competition and note that the mutations are now working as expected.

Revert the change to L37. Instead, comment out L14-19 (i.e. don't include the optional relationship):

  // fetch all competitions
  const { data: allCompetitions, isPending } = useFindManyCompetition({
    include: {
      // business: {
      //   select: {
      //     id: true,
      //     name: true,
      //   },
      // },
      entries: true,
    },
  });

Refresh and retry entering a competition and note the mutations are working still.

As I suggested in my original post, the inclusion of an optional relationship, where the default value returned by Prisma is null, is not accounted for in the evaluation on L165 of mutator.ts.

@ymc9
Copy link
Member

ymc9 commented Nov 21, 2024

Hi @roomman , thanks for reporting the issue. Let me have a check and follow up.

@ymc9
Copy link
Member

ymc9 commented Nov 21, 2024

Yes, you're right, a null checking is missing there. Probably safer to check it to be a non-null and object. Will be interested in creating a PR for it 😄 @roomman

@roomman
Copy link
Contributor Author

roomman commented Nov 22, 2024

Yes, @ymc9, happy to. Will sort asap. 👍🏼

@ymc9
Copy link
Member

ymc9 commented Nov 22, 2024

Yes, @ymc9, happy to. Will sort asap. 👍🏼

Thank you. Appreciate it!

@ymc9 ymc9 added this to the v2.9.x milestone Nov 22, 2024
@ymc9
Copy link
Member

ymc9 commented Nov 25, 2024

Hi @roomman , two more users reported the same issue. I don't want to push but just wanted to check if you've got time to work on the fix 😅. Happy to help.

@roomman
Copy link
Contributor Author

roomman commented Nov 25, 2024

Yes, was working on the tests this afternoon so hopefully that something tomorrow morning.

@roomman
Copy link
Contributor Author

roomman commented Nov 25, 2024

@ymc9 just a quick sense check if you have the time?

If I fetch all Users and include deeply nested relationships, like this:

useModelQuery(
    'User',
    makeUrl('User', 'findMany'),
    {
        include: {
            posts: {
                include: {
                    category: true,
                },
            },
        },
    },
    { optimisticUpdate: true }
),

And then add a new Post and "connect" it to a Category (where a Post can have one Category, and a Category can have many Posts), would you expect the User in the cache to have:

// where the category id and nested category object is included
{
    id: "1",
    name: "user1",
    posts: [
        {
            id: "1",
            title: "post1",
            categoryId: "1",
            category: {
                id: "1",
                name: "category1"
            }
        }
    ]
}

Or:

// where the nested category object is committed:
{
    id: "1",
    name: "user1",
    posts: [
        {
            id: "1",
            title: "post1",
            categoryId: "1"
        }
    ]
}

I had been testing on the basis that the Visitor would add the object too, but I only get the foreign key. Not had time to get into the internals of the Visitor and mindful that, with other users registering the issue, we need a quick fix.

Perhaps I should fix the null issue now and then skip the nested object test any look at that later, to you can take that up if it's better?

@ymc9
Copy link
Member

ymc9 commented Nov 25, 2024

@ymc9 just a quick sense check if you have the time?

If I fetch all Users and include deeply nested relationships, like this:

useModelQuery(
    'User',
    makeUrl('User', 'findMany'),
    {
        include: {
            posts: {
                include: {
                    category: true,
                },
            },
        },
    },
    { optimisticUpdate: true }
),

And then add a new Post and "connect" it to a Category (where a Post can have one Category, and a Category can have many Posts), would you expect the User in the cache to have:

// where the category id and nested category object is included
{
    id: "1",
    name: "user1",
    posts: [
        {
            id: "1",
            title: "post1",
            categoryId: "1",
            category: {
                id: "1",
                name: "category1"
            }
        }
    ]
}

Or:

// where the nested category object is committed:
{
    id: "1",
    name: "user1",
    posts: [
        {
            id: "1",
            title: "post1",
            categoryId: "1"
        }
    ]
}

I had been testing on the basis that the Visitor would add the object too, but I only get the foreign key. Not had time to get into the internals of the Visitor and mindful that, with other users registering the issue, we need a quick fix.

Perhaps I should fix the null issue now and then skip the nested object test any look at that later, to you can take that up if it's better?

Hi, thanks for looking into this detail @roomman ! Ideally the category should be a nested object inside the User cache, but I believe today it's not the case. I think we can fix the null issue for now, I can revisit this problem. The relation analysis part can be a little messy 😄

@roomman
Copy link
Contributor Author

roomman commented Nov 25, 2024

@ymc9 I've submitted the PR with a couple of notes. Cheers 👍🏼

@ymc9
Copy link
Member

ymc9 commented Nov 26, 2024

Thank you! The change looks great to me. I've merged it and will publish a new patch release.

@ymc9
Copy link
Member

ymc9 commented Nov 26, 2024

Fixed in 2.9.3

@ymc9 ymc9 closed this as completed Nov 26, 2024
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

No branches or pull requests

2 participants