sequelize: The "catch" block is not catching the error with one transaction using chain promises

What you are doing?

I’m using Promises in one transaction.

With my following code, the error is silent and the “catch” is not executed.

var Q = require('q');
var models = require("../models");

var forms = require('./data/forms.json').forms || [];

var createField = function(field, formDB){

    return models.field.findOrCreate({
        where: {
            name: field.apiName,
        },
        defaults: {
            type: field.typeField,
            keyItem: field.keyItem,
            parentId: field.parentId
        }
    }).then(function(fieldDB){
        fieldDB = fieldDB[0] || fieldDB;
        return models.formField.findOrCreate({
            where: {
                formId: formDB.formId,
                fieldId: fieldDB.fieldId,
            },
            defaults: {
                customName: field.customName,
                isVisible: field.isVisible,
                isRequired: field.isRequired,
                position: field.position
            }
        });
    });
};
var createForm = function(form){
    return models.form.findOrCreate({
        where: {
            name: form.name
        }
    }).then(function(formDB){
        formDB = formDB[0] || formDB;
        return Q.all(form.fields.map(function(field){
            return createField(field, formDB);
        }));
    });
};

models.sequelize.sync().then(function () {

    var result = Q();
    forms.forEach(function (form) {
        result = result.then(function(){
            return createForm(form);
        });
    });

    result.then(function(){
        console.log("Success!");
    }).catch(function(err){
        console.log("Error!"); //********************** Doesn't work ***************************
        var msg = err;
    });
});

But using transactions, the “catch” is executed (Why? I don’t have any idea about that):

var Q = require('q');
var models = require("../models");

var forms = require('./data/forms.json').forms || [];

var createField = function(field, formDB, t){

    return models.field.findOrCreate({
        where: {
            name: field.apiName,
        },
        defaults: {
            type: field.typeField,
            keyItem: field.keyItem,
            parentId: field.parentId,
        },
        transaction: t
    }).then(function(fieldDB){
        fieldDB = fieldDB[0] || fieldDB;
        return models.formField.findOrCreate({
            where: {
                formId: formDB.formId,
                fieldId: fieldDB.fieldId,
            },
            defaults: {
                customName: field.customName,
                isVisible: field.isVisible,
                isRequired: field.isRequired,
                position: field.position
            },
            transaction: t
        });
    });
};

var createForm = function(form){
    return models.sequelize.transaction(function (t1){ 
        return models.form.findOrCreate({
            where: {
                name: form.name
            },
            transaction: t1
        }).then(function(formDB){
            formDB = formDB[0] || formDB;
            return models.sequelize.transaction(function (t2){
                return Q.all(form.fields.map(function(field){
                    return createField(field, formDB, t2);
                }));
            });
        });
    });
};

models.sequelize.sync().then(function () {

    var result = Q();
    forms.forEach(function (form) {
        result = result.then(function(){
            return createForm(form);
        });
    });

    result.then(function(){
        console.log("Success!");
    }).catch(function(err){
        console.log("Error!"); //******************** Works perfect! ***************************
        var msg = err;
    });
});

What do you expect to happen?

The catch should be executed with one transaction.

What is actually happening?

The “catch” is not executed and therefore the error is silent.

Output (The error when the catch is executed)

{
    "name": "SequelizeUniqueConstraintError",
    "message": "Validation error",
    "errors": [{
        "message": "custom_name must be unique",
        "type": "unique violation",
        "path": "custom_name",
        "value": "Site"
    }],
    "fields": {
        "custom_name": "Site"
    }
}

Dialect: postgres Database version: 9.5.3 Sequelize version: 3.23.6

Thanks in advance, Nicholls

About this issue

Original URL
State: closed
Created 8 years ago
Reactions: 5
Comments: 29 (19 by maintainers)

Most upvoted comments

@jdnichollsc I’m going to put this less politely than @janmeier…

Are you bloody mad? I see you are very active on Github so I don’t know how you haven’t managed to understand this already.

Sending a massive load of stuff for people to wade through is rude. What it says is “My time is so important, I’m not going to bother putting any effort into this. Your time is worthless so you can spend an hour of it trying to figure out what’s going on in my huge example”.

I already gave you a long considered answer above working through your previous example as best I could with information you’d given. But you haven’t bothered to engage or reply to that either.

In conclusion: you need to put in some work yourself to help people help you.

Or put another way: wake up or fuck off!

overlookmotel on Oct 28, 2016

@jdnichollsc Going back to your very first example, does it output Success!?

I suspect this isn’t actually a matter of the error being “swallowed” in the early example, but is due to the code behaving differently depending on whether you run it in transactions or not.

There are many uses of findOrCreate() in your examples, and also you have some parallel execution going on (Q.all(form.fields.map(function(field){ ...). Within a transaction, the find() part of findOrCreate() can only see database changes made within its own transaction, so it may create() the same row again. But then when the transactions try to commit, you end up with two rows which both can’t be committed, creating the UniqueConstraint error.

You are passing the transaction explicitly between calls, so CLS is not being used. So it should not make any difference whether you use Q or Sequelize.Promise. Feel free to use whichever you prefer.

What I’m saying is that at this point, I suspect there’s no bug in Sequelize, and it’s a problem with your code.

If you want further help here, I think you first need to try to narrow down this problem yourself. In particular, you haven’t posted the data in forms so there’s no way for anyone else reading to replicate the problem.

If I were you, I would try to reduce that data down piece by piece and find out at what point the error doesn’t not occur any more. That I think will give you the clue what’s going on.

If I’m wrong, and you do still think it’s a bug in Sequelize, please come back with a single solid example of where an error is being thrown and you think it shouldn’t be. That example should include: model definition, data being used (i.e. what is in the fields.json file), and the code - everything someone else needs to get the example running on their own computer and replicate the error. Personally, I find the screencasts hard to follow.

I hope you understand that I’m trying to help!

overlookmotel on Sep 3, 2016

Incidentally, I haven’t read the OP in great detail, but I’m not 100% sure that #6319 would fix it - might be something else weird going on.

@jdnichollsc Could you apply the patch from #6319 (it’s only two lines!) and check if that solves your problem?

overlookmotel on Aug 10, 2016

@sushantdhiman It wasn’t actually me that introduced CLS for transactions to Sequelize. I think credit goes to @mickhansen. I just thought it was fantastic and jumped right on using it.

@felixfbecker There’s nothing wrong with passing context explicitly. It’s probably a more robust way of doing things. But I found my code was getting so full of {transaction: req.transaction, logging: req.log} that it became hard to read. And then occasionally I’d miss one out by accident and get weird bugs. So for me, CLS is a godsend.

I’m still dragging on with my new cls-bluebird shim. It’s almost there - written tests for every method except coroutine now. And along the way, I’ve uncovered a few bugs in bluebird, including one pretty nasty one. Only a few hours work left, I’d say, and it’ll be complete. But problem is I’m mad busy until end of month, so I doubt I’ll get those hours until Sept.

I’ve also realised that there’s a way to avoid using a patched bluebird internally within sequelize and still maintaining the ability to get transaction from CLS. But it’d require quite a lot of changes. If every sequelize public method at the start of the call (before any async action) checked the CLS namespace for transaction, and added it as options.transaction, then the transaction would be passed explicitly from that point on, and there’d be no need to ensure CLS context is maintained.

This would probably be a more robust solution than what we have now. But it’d take a bit of time and care to implement. But anyway, when my cls-bluebird patch is finally released, I’m pretty confident it’ll be bulletproof!

overlookmotel on Aug 10, 2016

@jdnichollsc Thats what https://github.com/sequelize/sequelize/pull/6319 fixed (to support mapSeries in transactions), but its not released to v3 yet. Its going to be released with v4 😃

sushantdhiman on Aug 10, 2016

Does the same happen when you use bluebird instead of Q? You can access it through Sequelize.Promise, it has all the methods like Promise.all() and many more aswell.

felixfbecker on Aug 10, 2016