Example in DELETE docs does not close connection on error

See comment in snippet.

pool.get_connection(qb => {
        qb.get('comments', {id: id}, (err, res) => {
            // CONNECTION SHOULD BE CLOSED BEFORE RETURNING
            if (err) return console.error(err);
            const article_id = res.article_id;
 
            qb.delete('comments', {id: id}, (err, res) => {
                qb.release();
                if (err) return console.error(err);
 
                const page_data = {
                    num_removed: res.affected_rows,
                }
                return res.render('/article/' + article_id, page_data);
            });
        });
    });

Once I need to make more than 1 DB call, this library starts to become very hard to manage due to lack of promise support.

The preceding issue would be avoided by using the following async/await syntax with promises returned from node-querybuilder.

async function render() {
  let db
  try {
    db = await pool.get_connection()
    let comments = await qb.get('comments', {id})
    const article_id = comments.article_id;
    const res = await qb.delete('comments', {id})
    const page_data = {num_removed: res.affected_rows}
    return res.render('/article/' + article_id, page_data)
  } catch (err) {
    console.error(err)
  } finally {
    if (db) db.release()
  }
}

Good catch on the bad example. I would happily accept a PR to fix it.

As for a Promise API, I definitely want this to happen as well. Callback hell should be a think of the past. I, for one, welcome our new async/await overlords. I think I set Promise support for v3. For now, you can get away with it (mostly, I think) by "promisifying" any method using Node's util.promisify() method.

IIRC promisify was not working in my use case, at least with V1.

Have you tried with the v2 branch. Also, looks like I haven't actually made v2 the master yet. Have you been using the v2 branch?

Yes. All in with V2. Except my use case is webpacking everything into a single script, and then running inside embedded V8 runtime inside Java (with promise but not async/await support), and that running as a WebApp inside Tomcat. Not really typical...

Haha, yeah, I would say that is pretty atypical indeed. Okay then, I'll go ahead and make v2 the master branch so you don't have to keep using /next.

If you want to tackle making the API support promises (without breaking the CB API), I would be very please to merge that PR. Definitely try the util.promisify first, though, as a stop-gap solution and lemme know if that works at all.

I have actually added some promise support and am using a hacked branch...

@kylefarris

@kylefarris I moved some of my code to a pure node environment and promisify (from util) did not work. It's also a PITA to first get async pool db and then promisify, as getting the db from pool should also return a promise IMO. So first order of business should be to return a promise from pool.getDb()...

I will start a new branch forked off of v2, and will not add support to v1. V2 seems stable enough for my uses.

This seems to be how it's done

        let promise;

        if (!callback && typeof Promise === 'function') {
            promise = new Promise((resolve, reject) => {
                callback = shared.callbackPromise(resolve, reject);
            });
        }
        // ...
       return promise

Sorry to pollute this thread...

I don't think it's polluting the thread at all. We have to have this conversation somewhere.

I've written several project where I've had to introduce Promises into a callback-centric API. It's actually easier than you might think.

There's really only a few methods (the main execution method and the connection methods) we'd need to change in order to add the Promise functionality. Without testing this theory, there's one place we'd actually have to add it since all the exec methods ultimately call the _exec method on their driver's query_exec class. So, it'd just be a matter of returning a Promise if not callback is provided on that one.

As for the connection methods, we just need to worry about the get_connection() method for pools and the connect() method for single connections. We'd also have to add it to the disconnect() method for all adapter types.

I'll see if I can come up with an example. Overall, it should be a pretty small overall change. Maybe even small enough to make it a v2.1 (as long as we don't break anything).

Navigation Menu

Repository navigation

Example in DELETE docs does not close connection on error #44

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Example in DELETE docs does not close connection on error #44

Description

Activity

kylefarris commented on Sep 27, 2018

Flamenco commented on Sep 27, 2018

kylefarris commented on Sep 27, 2018

Flamenco commented on Sep 27, 2018

kylefarris commented on Sep 27, 2018

kylefarris commented on Sep 27, 2018

Flamenco commented on Sep 27, 2018

Flamenco commented on Sep 29, 2018

kylefarris commented on Oct 1, 2018

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions