hasura / graphql-engine

Blazing fast, instant realtime GraphQL APIs on your DB with fine grained access control, also trigger webhooks on database events.
https://hasura.io
Apache License 2.0
31.13k stars 2.76k forks source link

Previous mutations are not rolled back when one fails #8658

Open SBNTT opened 2 years ago

SBNTT commented 2 years ago

Version Information

Server Version: 2.8.3

What is the current behaviour?

When executing a mutation composed of several operations, and when one of them failed, previous ones are not rolled back.

What is the expected behaviour?

https://hasura.io/docs/latest/graphql/core/databases/postgres/mutations/multiple-mutations/#execution

If multiple mutations are part of the same request, they are executed sequentially in a single transaction. If any of the mutations fail, all the executed mutations will be rolled back.

How to reproduce the issue?

Given these three postgres tables:

CREATE TABLE "articles"
  (
     "identifier"  UUID NOT NULL DEFAULT Uuid_generate_v4(),
     "title"       TEXT NOT NULL,
     CONSTRAINT "PK_eccda5e33eeb028ae7adc9ce71e" PRIMARY KEY ("identifier")
  )

CREATE TABLE "images"
  (
     "identifier" UUID NOT NULL DEFAULT Uuid_generate_v4(),
     "url"        TEXT NOT NULL,
     CONSTRAINT "PK_5dd26d55bd7a786386b840469cc" PRIMARY KEY ("identifier")
  ) 

CREATE TABLE "articles_images"
  (
     "identifier"         UUID NOT NULL DEFAULT Uuid_generate_v4(),
     "article_identifier" UUID NOT NULL,
     "image_identifier"   UUID NOT NULL,
     CONSTRAINT "PK_7f24e82a97c6c961bef5d97213a" PRIMARY KEY ("identifier")
  ) 

and this graphQL mutation:

mutation updateOneArticleMutation(
  $identifier: uuid!, 
  $fields: articles_set_input!, 
  $imagesToAdd: [articles_images_insert_input!]!,
  $imagesToDelete: [uuid!]!
) {
  insert_articles_images(objects: $imagesToAdd) {
    affected_rows
  }

  delete_articles_images(
    where: {
      article_identifier: {_eq: $identifier}, 
      image_identifier: {_in: $imagesToDelete}
    }
  ) {
    affected_rows
  }

  update_articles_by_pk(
    pk_columns: {identifier: $identifier}, 
    _set: $fields
  ) {
    identifier
    title
    images {
      image {
        identifier
        url
      }
    }
  }
}

When the third operation fails, the two ones before are not rolled back.

What i've done to determine that:

These two cases leads to the same result: insert_articles_images and delete_articles_images mutations were commited to the database.

Thanks

wildsurfer commented 8 months ago

I'm facing the same issue

klondikedragon commented 8 months ago

We're also experiencing this on v2.37.0-cloud.2. It's a very serious issue that leaves garbage behind in the database and really breaks the point of using a relational database backend. @0x777 what do you think, since it's a serious data integrity issue vs documented behavior, could this one be prioritized?

klondikedragon commented 8 months ago

On further investigation I've found what is causing the mutation operation to not run in a postgresql transaction, at least in our case. We're using a graphQL client that is code-generated by a framework, and it's adding __typename to each operation in the mutation, as well as one at the end. For example, the mutation looks something like this:

mutation changesomething($a: uuid!, $b: uuid!, $c: String!, $d: String!) {
  insert_tablea_one(object: {id: $b, c: $c}, on_conflict: {constraint: tablea_pkey, update_columns: [c]}) {
    created_at
    updated_at
    __typename
  }
  insert_tablea_tableb_one(object: {a: $a, b: $b, d: $d}, on_conflict: {constraint: tablea_tableb_pkey, update_columns: [d]}) {
    created_at
    updated_at
    __typename
  }
  __typename
}

It's the presence of that last __typename that is causing Hasura not to execute the whole thing in a transaction. If you take out the outermast __typename but leave in __typename inside of the insert_tablea_one and insert_tablea_tableb_one it still executes within a transaction. Hasura is treating the last __typename as not from the same postgresql source, and thus doesn't execute it in a transaction. In the code that determines if the operation is eligible to run in a transaction, it should ignore meta operations like __typename