keystonejs / keystone

The superpowered headless CMS for Node.js — built with GraphQL and React
https://keystonejs.com
MIT License
9.26k stars 1.16k forks source link

When creating items, if validation fails any relationship items remain. #7217

Closed qwacko closed 2 years ago

qwacko commented 2 years ago

Created three schema items (Parent, Child, Grandchild) - as below, And inserted them into the overall keystone schema.

import { list } from "@keystone-6/core";
import { text, relationship } from "@keystone-6/core/fields";
import { ListHooks, BaseListTypeInfo } from "@keystone-6/core/types";

const hooks = (title: string): ListHooks<BaseListTypeInfo> | undefined => ({
  validateInput: async () => {
    console.log(`${title} : validateInput`);
  },
  resolveInput: async ({ resolvedData }) => {
    console.log(`${title} : resolveInput`);
    return resolvedData;
  },
  validateDelete: async () => {
    console.log(`${title} : validateDelete`);
  },
  beforeOperation: async () => {
    console.log(`${title} : beforeOperation`);
  },
  afterOperation: async () => {
    console.log(`${title} : afterOperation`);
  },
});

export const Parent = list({
  fields: {
    title: text({ validation: { isRequired: true } }),
    children: relationship({
      ref: "Child.parent",
      many: true,
    }),
  },
  hooks: hooks("Parent"),
});

export const Child = list({
  fields: {
    title: text(),
    parent: relationship({
      ref: "Parent.children",
      many: false,
    }),
    children: relationship({
      ref: "Grandchild.parent",
      many: true,
    }),
  },
  hooks: hooks("Child"),
});

export const Grandchild = list({
  fields: {
    title: text(),
    parent: relationship({
      ref: "Child.children",
      many: false,
    }),
  },
  hooks: hooks("Grandchild"),
});

Run the following graphql query

mutation {
  createParent(
    data: {
      children: {
        create: [
          { title: "Child", children: { create: [{ title: "Grandchild" }] } }
        ]
      }
    }
  ) {
    id
    title
    children {
      id
      title
      children {
        id
        title
      }
    }
  }
}

This will result in a failure (due to there being no "title" of the Parent object), however the "Child" and "Grandchild" item will still be created.

The count can be checked using the following command (which indicates the Child and Grandchild counts increasing but not the parent count.

query {
  grandchildrenCount
  childrenCount
  parentsCount
}

Describe what you would expect to happen

I would expect that on failure of the validation of any part of a single mutation query (including multiple mutations within a single query), there would be no changes made to the DB.

It may be possibly to undo the changes through manual logic, but due to the ability to create / modify nested items easily, I would imagine performing this change in the provided hooks would become overly complex.

Other useful information

Keystone version = "@keystone-6/core": "^1.0.1"

With the logging that I implemented in the hooks, it would seem that the validation is run on the bottom most item, and works its way back. The code output in the console fo rthe above query is as follows:

Grandchild : resolveInput
Grandchild : validateInput
Grandchild : beforeOperation
Child : resolveInput
Child : validateInput
Child : beforeOperation
Parent : resolveInput
Parent : validateInput

Notice that the first hook called is the grandchild resolving input, and so there is no way thoguh the existing hooks to intervene and check the entire query before enacting any changes.

dcousens commented 2 years ago

The question distilled

To what extent do we have database transactions within singular mutation queries, and if not, why not?

molomby commented 2 years ago

We currently don't use transactions in single mutations; it's a known problem we're looking to rectify.

molomby commented 2 years ago

As to the "why" – due to the way nested mutations and hooks interact it's not possible to support transactions when using auto increments. When wrestling with the tradeoffs during dev, it was decided to remove transactions in the short term for simplicity sake.

We intend to re-enable transactions by default in cases when the lists being modified don't include any auto incs. For cases where the lists do includes an auto inc (ie. as the list id, with db.idField.kind = 'autoincrement', or as an integer field with defaultValue.kind = 'autoincrement') transactions will still not be supported.

qwacko commented 2 years ago

Thanks for the clear explanation of this and great to hear that it is being worked on. I can see how validating a complete transaction in KeystoneJS, and then compiling a Prisma transaction to execute would be pretty difficult.

My hacky workaround which worked for me was to implement this as a custom mutation, which took a bit of work but still ended up benefiting from the types / prisma client / authentication from Keystone JS. In implementing this I possibly had some conditions that can't be validated in Prisma / DB configuration, but needed to be validated in logic. Items like, make sure at least one user is marked as an admin of this group, or make sure there are at least two relationship items that match a specific criteria including when the item is created (where there can also be ones that don't match a criteria). Not sure how this could be implemented in the provided hook structure, as you need to be able to look at what is in the database, as well as modifications possibly being made to relationship items

Possibly I was pushing this system too much, and should have just started with custom mutations, because it is setup more as a CMS, but i was trying to use it as a full app backend.

dcousens commented 2 years ago

Unfortunate as the answer is for now, I am closing this question as a duplicate of the following https://github.com/keystonejs/keystone/discussions/7642, I have opened a feature request here (#8084).