thingdom / node-neo4j

[RETIRED] Neo4j graph database driver (REST API client) for Node.js
Apache License 2.0
925 stars 135 forks source link

Can We omit the callback of tx.commit() ? #197

Closed linonetwo closed 8 years ago

linonetwo commented 8 years ago

When using transaction, I have to use:

tx.commit(() => {})

or it will complain about 「error given was: TypeError: cb is not a function」

But there is not really something to do next……

aseemk commented 8 years ago

The callback is pretty important: even though it doesn't receive a result, it still does receive an error if the commit failed! So you should wait for the callback to get called so you know whether the commit was successful. If the commit wasn't successful, none of your queries took effect!

For example, if you're performing this transaction in response to a user request, you shouldn't respond to the user until you know whether the full transaction — including commit — succeeded.

E.g. via Express:

app.post('/photos', function (req, res, next) {
    var tx = db.beginTransaction();
    // ...
    tx.commit(function (err) {
        if (err) {
            // handle the error somehow; assuming you have an error-handling middleware:
            return next(err);
        }
        // handle success, e.g.:
        res.render('photo', {...});
    });
});
aseemk commented 8 years ago

I'm going to go ahead close this issue, but feel free to re-open if you think you have a legit case. =)

Also, feel free to ask questions like these on either the mailing list or Slack, so others in the community can also chime in.

https://github.com/thingdom/node-neo4j/tree/v2#help

linonetwo commented 8 years ago

My god, I finally found it's usefullness!

instead of :

    .then(result => { // 如果上面都没有 reject 的话,下面完结一个 transaction
      return run({query: ''}, false).then(tx => {
        tx.commit(() => {});
        return userInfo;
      });
    })

which can't invoke further action inside .then() properly, since tx.commit is not finished yet!

I should use this callback like:

    .then(result => { // 如果上面都没有 reject 的话,下面完结一个 transaction
      return run({query: ''}, false).then(tx => {
        return new Promise((resolve, reject) => tx.commit(() => resolve(userInfo))) ;
      });
    })

Thanks for providing example so patiently!

should I do something like this?

tx.commit(function (err) {
        if (err) {
            tx.rollback(() => {});
        }
}

Thank for your reply!

linonetwo commented 8 years ago

Trying to use promise, I wrap things inside my own implementation ugly:

function useNeo4jDB(url, userName, passWord) {
  let db = new neo4j.GraphDatabase({
            url, // 表示 Neo4j 运行在哪里
            auth: {username: userName, password: passWord} //登陆一下数据库
        });
  let _db = db;
  let _lastTransaction = db;

  return (cypherQuery, transaction = undefined) => {

    if (transaction !== undefined) { // 只有 transaction 开关被显式指定了,才进入此分支
      _db = transaction ? db.beginTransaction() : db; // 如果打开事务性开关,就把接下来的所有命令
      // 都放在 db.beginTransaction() 里执行
      if (transaction == true) {
        _lastTransaction = _db; // 每次 transaction 开始后就保存一个 db.beginTransaction() 的副本,以便 commit
      }
    }
    return new Promise(function (resolve, reject) {
      if (cypherQuery.query.length == 0 && transaction !== undefined) {
        // cypherQuery 中的请求为空字符串的情况视作只是想修改 transaction 模式
        resolve(_lastTransaction); // 见下方事务性调用的例子
      }
        _db.cypher(cypherQuery, function callback(err, result) {
          if (err) {
            reject(err); // 出错的时候
          }
          if (!result || result.length == 0) {
            resolve(NO_RESULT); // 没啥结果的时候
          } else {
            resolve(result); // 还是有点结果的时候
          }
        });
    });
  };
}

and always found something didn't created properly, now I know the reason is my didn't using callback of tx.commit() properly!

aseemk commented 8 years ago

I would tweak this line:

return new Promise((resolve, reject) => tx.commit(() => resolve(userInfo)));

To be this instead:

return new Promise((resolve, reject) => tx.commit((err) =>
  if (err) reject(err) else resolve(userInfo)
));

I don't think you need to explicitly roll back if the commit failed — all errors roll back. But you can sanity check that with an assertion if you want:

return new Promise((resolve, reject) => tx.commit((err) =>
  if (err) {
    assert.equal(tx.state, tx.STATE_ROLLED_BACK, 'Tx should be rolled back after commit error')
    reject(err)
  } else {
    resolve(userInfo)
  }
));

I can't help you with your broader code, but I do want to add promises to node-neo4j one day!

linonetwo commented 8 years ago

Thanks, that helps a lot.

Looking forword to promisified version!