Skip to content

Common Mistakes

Jump to bottom
Vitaly Tomilov edited this page Jun 25, 2017 · 54 revisions

Just before Christmas 2015 I took time to go over some 100+ projects on GitHub that use pg-promise.

Based on what I saw, I have compiled a list of the most common mistakes developers make when using this library. I surely hope this list will help somebody understand this library better and avoid making such mistakes in the future.

Happy coding into 2016 everyone! :)

Invalid query formatting with manual string concatenation and ES6 template strings

Perhaps the most frequent mistake in the growing popularity of ES6 notation is the misuse of ES6 template strings to format SQL queries.

ES6 template strings have absolutely no knowledge of how to format parameters or data types within SQL queries. This library implements a powerful formatting engine to properly escape all JavaScript data types and all SQL types, in accordance with the PostgreSQL requirements, which includes: SQL parameters, SQL names and identifiers - relying on which is essential for writing safe and reliable code.

If instead you start using manual string concatenation and ES6 template strings, below is just some of the troubles this invites:

  • Strings with apostrophe ' in them will break the query
  • Manual string concatenation opens the floodgates to SQL injection
  • Presence of double-quote " within SQL Names will break the query
  • Unescaped SQL Names/Identifiers create a possibility for SQL injection
  • Many specific types will become broken due to their specific presentation required by PostgreSQL. For example, floating-point values, with their special support for such values as NaN, +Infinity and -Infinity.

You should always format queries via either an array of parameters or [Named Parameters](Invalid query formatting with manual concatenation and ES6 template strings). Use SQL Names to format such elements as schema name, table name or column name.

Away from callbacks, into promises, and back into callbacks?

I know it can be challenging to break away from the callback pattern, but promises do make it worthwhile, turning your otherwise unreadable nested callbacks into a nice, streamlined logic that anyone can read and understand. And if you haven't invested enough time into understanding promises better, switching to pg-promise is really a good time to do that.

Unfortunately, I see a lot of examples where people convert promise results back into callbacks, effectively taking the perfect result-processing pattern and crippling it back where it came from - the callbacks nightmare.

Tasks versus root/direct queries

As explained in Chaining Queries, tasks are here when you need to execute more than 1 query. They allocate the connection to execute all your queries and then release the connection.

Somehow this message gets lost, and what I'm seeing a lot instead:

  • use of multiple queries (dependent or not) without using tasks;
  • use of tasks with just one query in them.

None of the above make sense, and both are bad patterns. The first one is bad because you effectively allocate and release connection for every single call, instead of doing it once, on the task level. And the second one is pointless in most cases, unless there is a reason you want a single query to be considered a task and track it independently (didn't look like from the code I saw).

Below is the worst example of them all (avoid such coding like a plague):

function badCode(someData) {
    // someData = your array of objects for inserts;
    var queries = [];
    someData.forEach(function (data) {
        // querying against root database protocol:
        queries.push(db.none('insert into...', data));
    });
    return Promise.all(queries); // doesn't settle queries;
}

And the right way to do it:

function goodCode(someData) {
    // someData = your array of objects for inserts;
    return db.task(function (t) {
        var queries = [];
        someData.forEach(function (data) {
            // querying against the task protocol:
            queries.push(t.none('insert into...', data));
        });
        return t.batch(queries); // settles all queries;
    });
}

which can be further simplified into this:

function goodCode(data) {
    return db.task(t=>t.batch(data.map(d=>t.none('insert into...', d))));
}

Also, making multiple changes in one operation is typically done via a transaction (method tx instead of task).

Named Parameter formatting is not used where it should be

Constant use of the following query formatting:

db.query("INSERT INTO table VALUES($1, $2, $3)", [obj.name, obj.title, obj.code]);

instead of this one:

db.query("INSERT INTO table VALUES(${name}, ${title}, ${code})", obj);

If you are using just one object to format the query, why not make explicit use of its property names, rather than the nameless indexes? Named Parameter formatting is so much nicer in this case.

Redundant Named Parameter re-referencing

I can see a lot of code written like this:

db.query("INSERT INTO table VALUES(${name}, ${title}, ${code})",
    {name: obj.name, title: obj.title, code: obj.code});

...while the result of such formatting is exactly as of this one:

db.query("INSERT INTO table VALUES(${name}, ${title}, ${code})", obj);

i.e. named parameters are re-referenced needlessly.

Inclusion and use of the pg package (node-postgres) independently

This library already references and uses the latest version of node-postgres, and this is how it should be used in cases when it is needed, as explained in the FAQ section.

Instead, I see it all over the place when node-postgres is referenced to from package.json in addition to pg-promise. You really should avoid this.

Skipped Initialization and Monitoring

Most developers do not get as far as going through the Initialization Options of the library, not to mention the use of pg-monitor, even though those are the best parts of this library, ones that help you customize it, diagnose and log everything about your database.

pg-promise

Clone this wiki locally