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

Constraint violations during commit are lost #50

Closed
studds opened this Issue Aug 28, 2013 · 4 comments

Comments

Projects
None yet
2 participants
@studds
Contributor

studds commented Aug 28, 2013

I'm using postgres, and I've got all my foreign key constraints set up as DEFERRABLE INITIALLY DEFERRED. This means I don't need to ensure each query in the transaction is executed in the right order to satisfy foreign key constraints, so long as the transaction overall is valid. This means that the constraint violation isn't triggered on the original query, but rather on commit.

I'm finding that commits are failing but in my app (using Bookshelf), the transaction gets resolved and not rejected.

I can't see anyway of finding out whether the commit was successful or not... should commit return a promise?

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Aug 28, 2013

So the issue here is that the commits are failing, but the transaction block is resolving with a successful promise? That seems odd, can you paste some code showing a brief example of how you have the transaction setup so I can get a better idea of what the issue might be?

@studds

This comment has been minimized.

Contributor

studds commented Aug 28, 2013

That's right, the transaction block is resolving with a successful promise but the commit is actually failed. I've mocked up a simple example:

(function () {

    var query = 'drop schema if exists transdemo cascade;' +
        'create schema transdemo;' +
        'CREATE TABLE "transdemo"."one" (' +
        '"id" int4 NOT NULL,' +
        'PRIMARY KEY ("id")' +
        ')' +
        'WITH (OIDS=FALSE);' +
        'CREATE TABLE "transdemo"."two" (' +
        '"id" int4 NOT NULL,' +
        '"oneId" int4 NOT NULL,' +
        'PRIMARY KEY ("id")' +
        ')' +
        'WITH (OIDS=FALSE);' +
        'ALTER TABLE "transdemo"."two" ADD CONSTRAINT "two_one_fk"' +
        'FOREIGN KEY ("oneId") REFERENCES "transdemo"."one" ("id")' +
        'ON UPDATE NO ACTION ON DELETE NO ACTION DEFERRABLE INITIALLY DEFERRED;';

    var Bookshelf = require('bookshelf');
    var connectionString = process.env.DATABASE_URL || 'postgres://localhost:5432/traceapp';
    var pg = require('pg');
    var Client = new pg.Client(connectionString);

    Bookshelf.Initialize({
        client: 'pg',
        connection: {
            user: Client.connectionParameters.user,
            database: Client.connectionParameters.database,
            port: Client.connectionParameters.port,
            host: Client.connectionParameters.host,
            password: Client.connectionParameters.password,
            binary: Client.connectionParameters.binary,
            ssl: Client.connectionParameters.ssl,
            charset  : 'utf8'
        },
        debug: true
    });

    return Bookshelf.Knex.Raw(query)
        .then(function () {
            var One = Bookshelf.Model.extend({
                tableName: 'transdemo.one'
            });
            var Two = Bookshelf.Model.extend({
                tableName: 'transdemo.two'
            });

            return Bookshelf.Transaction(function (t) {
                Two.forge({id: 1, oneId: 1})
                    .save(null, {transacting: t})
                    .then(function () {
                        console.log('save succeeds because constraint is deferred');
                        t.commit();
                    })
                    .otherwise(function (err) {
                        console.log('save has failed', err, err.stack);
                        t.rollback();
                    });
            })
                .then(function () {
                    console.log('transaction has succeeded, so should be a row in two');
                    return Two.forge({id: 1})
                        .fetch()
                        .then(function (two) {
                            if (two) {
                                console.log('huh, how did that get created?');
                            } else {
                                console.log('transaction was successful, but the rows really were not created');
                            }
                        })
                        .otherwise(function (err) {
                            console.error(err, err.stack);
                        });
                })
                .then(function () {
                    process.exit(0);
                })
                .otherwise(function () {
                    process.exit(1);
                });

        })
        .otherwise(function () {
            process.exit(1);
        });

})();

The results I get are:

{ sql: 'drop schema if exists transdemo cascade;create schema transdemo;CREATE TABLE "transdemo"."one" ("id" int4 NOT NULL,PRIMARY KEY ("id"))WITH (OIDS=FALSE);CREATE TABLE "transdemo"."two" ("id" int4 NOT NULL,"oneId" int4 NOT NULL,PRIMARY KEY ("id"))WITH (OIDS=FALSE);ALTER TABLE "transdemo"."two" ADD CONSTRAINT "two_one_fk"FOREIGN KEY ("oneId") REFERENCES "transdemo"."one" ("id")ON UPDATE NO ACTION ON DELETE NO ACTION DEFERRABLE INITIALLY DEFERRED;',
  bindings: [],
  __cid: '__cid1' }
{ sql: 'update "transdemo"."two" set "id" = $1, "oneId" = $2 where "id" = $3',
  bindings: [ 1, 1, 1 ],
  __cid: '__cid2' }
save succeeds because constraint is deferred
transaction has succeeded, so should be a row in two
{ sql: 'select * from "transdemo"."two" where "id" = $1 limit 1',
  bindings: [ 1 ],
  __cid: '__cid3' }
transaction was successful, but the rows really were not created
@tgriesser

This comment has been minimized.

Owner

tgriesser commented Aug 28, 2013

So it looks like the issue here might not be with the transaction, but rather the fact that if you set the primary key, Bookshelf assumes you're going for an update... I think you're looking to insert without having it create the primary key for you, you'll have to do:

Two.forge({id: 1, oneId: 1}).save(null, {transacting: t, method: 'insert'});

Let me know if that isn't the source of the issue.

@studds

This comment has been minimized.

Contributor

studds commented Aug 29, 2013

Ahh, woops, normally I'm overriding new() and using the serverCreatedAt instead of id (as per bookshelf/bookshelf#24), but I forgot about that in this example.

And, it turns out, the problem is completely different to what I thought!

The promise is actually not resolved at all. I thought it was resolving because of an error in my main code.

Updated example:

(function () {

    var query = 'drop schema if exists transdemo cascade;' +
        'create schema transdemo;' +
        'CREATE TABLE "transdemo"."one" (' +
        '"id" int4 NOT NULL,' +
        '"serverCreatedAt" timestamp NOT NULL,' +
        '"serverUpdatedAt" timestamp NOT NULL,' +
        'PRIMARY KEY ("id")' +
        ')' +
        'WITH (OIDS=FALSE);' +
        'CREATE TABLE "transdemo"."two" (' +
        '"id" int4 NOT NULL,' +
        '"oneId" int4 NOT NULL,' +
        '"serverCreatedAt" timestamp NOT NULL,' +
        '"serverUpdatedAt" timestamp NOT NULL,' +
        'PRIMARY KEY ("id")' +
        ')' +
        'WITH (OIDS=FALSE);' +
        'ALTER TABLE "transdemo"."two" ADD CONSTRAINT "two_one_fk"' +
        'FOREIGN KEY ("oneId") REFERENCES "transdemo"."one" ("id")' +
        'ON UPDATE NO ACTION ON DELETE NO ACTION DEFERRABLE INITIALLY DEFERRED;';

    var Bookshelf = require('bookshelf');
    var connectionString = process.env.DATABASE_URL || 'postgres://localhost:5432/traceapp';
    var pg = require('pg');
    var Client = new pg.Client(connectionString);

    Bookshelf.Initialize({
        client: 'pg',
        connection: {
            user: Client.connectionParameters.user,
            database: Client.connectionParameters.database,
            port: Client.connectionParameters.port,
            host: Client.connectionParameters.host,
            password: Client.connectionParameters.password,
            binary: Client.connectionParameters.binary,
            ssl: Client.connectionParameters.ssl,
            charset  : 'utf8'
        },
        debug: true
    });

    // A model is new if it has never been saved to the server, and lacks an 'serverCreatedAt' date
    Bookshelf.Model.prototype.hasTimestamps = ['serverCreatedAt', 'serverUpdatedAt'];
    Bookshelf.Model.prototype.isNew = function () {
        return !this.get('serverCreatedAt');
    };

    return Bookshelf.Knex.Raw(query)
        .then(function () {
            var One = Bookshelf.Model.extend({
                tableName: 'transdemo.one'
            });
            var Two = Bookshelf.Model.extend({
                tableName: 'transdemo.two'
            });
            var Twos = Bookshelf.Model.extend({

            });

            return Bookshelf.Transaction(function (t) {
                Two.forge({id: 1, oneId: 1})
                    .save(null, {transacting: t})
                    .then(function () {
                        console.log('save succeeds because constraint is deferred');
                        t.commit();
                    })
                    .otherwise(function (err) {
                        console.log('save has failed', err, err.stack);
                        t.rollback();
                    });
            })
                .then(function () {
                    console.log('transaction has succeeded, so should be a row in two');
                    return Two.forge({id: 1})
                        .fetch()
                        .then(function (two) {
                            if (two) {
                                console.log('huh, how did that get created?');
                            } else {
                                console.log('transaction was successful, but the rows really were not created');
                            }
                        })
                        .otherwise(function (err) {
                            console.error(err, err.stack);
                        });
                })
                .then(function () {
                    process.exit(0);
                });

        })
        .otherwise(function (err) {
            console.log('transaction failed', err, err.stack);
            process.exit(1);
        });

})();

Run normally, it hangs forever because the promise doesn't resolve. If, on the other hand, I add an 'otherwise' in finishTransaction in knex/clients/base.js:

  finishTransaction: function(type, trans, dfd, msg) {
    var ctx = this;
    nodefn.call(trans.connection.query.bind(trans.connection), type + ';', []).then(function(resp) {
        console.log(resp);
      if (type === 'commit') dfd.resolve(msg || resp);
      if (type === 'rollback') dfd.reject(msg || resp);
    })
        .otherwise(function (err) {
            dfd.reject(err);
        })
        .ensure(function() {
      ctx.releaseConnection(trans.connection);
      trans.connection = null;
    });
  }

... then it works a treat.

studds added a commit to studds/knex that referenced this issue Aug 29, 2013

tgriesser added a commit that referenced this issue Aug 29, 2013

Merge pull request #52 from studds/master
reject transaction promise if commit fails (fixes #50)

tgriesser added a commit that referenced this issue Sep 4, 2013

Merge branch 'master' into migrate
* master:
  release 0.2.6
  prep for 0.2.6
  reject transaction promise if commit fails (fixes #50)
  release 0.2.5
  fix for #49, prep for 0.2.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment