0

So I am running a few insert queries using the mysql npm package and I'm doing so with Promise.all to try and run them at the same time. I have a function that creates a new connection for each query and returns a promise. I am also wrapping the mysql queries with promises. Here is the code that builds the queries and calls them with Promise.all 

const Database = require('./database');

const queryBuilder = record =>
  new Promise((resolve, reject) => {
    try {
      const filename = record.s3.object.key.split('/').pop();
      const filesize = record.s3.object.size;
      const s3path = `${record.s3.bucket.name}/${record.s3.object.key}`;
      const createdAt = record.eventTime.split('T').shift();
      const sql = 'INSERT INTO raw_files (filename, filesize, s3path, created_at, client_subdomain) ' +
       `VALUES ('${filename}', ${filesize}, '${s3path}', '${createdAt}', '${record.s3.bucket.name}')`;
      resolve(sql);
    } catch (err) {
      reject(err);
    }
  });

const connectionWrapper = (params, record) =>
  new Promise((resolve, reject) => {
    const db = new Database(params);
    db.connect()
      .then(res => queryBuilder(record))
      .then(sql => db.query(sql))
      .then((res) => {
        db.close();
        resolve(res);
      })
      .catch((err) => {
        db.close();
        reject(err);
      });
  });

exports.handler = (event, context, callback) => {
  const connectionParams = {
    host: '127.0.0.1',
    port: 3306,
    user: 'root',
    password: 'some_password',
    database: 'space_util',
  };

  Promise.all(event.Records.map(record => connectionWrapper(connectionParams, record)))
    .then(res => callback(null, res))
    .catch(err => callback(err));
};

And then here is the wrapper module 

const mysql = require('mysql');

// Promise-ify the mysql connection
const Database = function Database(connectionParams) {
  this.connection = mysql.createConnection(connectionParams);
};

Database.prototype.connect = function connect() {
  return new Promise((resolve, reject) => {
    this.connection.connect((err) => {
      if (err) reject(err);
      resolve();
    });
  });
};

Database.prototype.query = function query(sql) {
  return new Promise((resolve, reject) => {
    this.connection.query(sql, (err, results, field) => {
      if (err) reject(err);
      resolve(results);
    });
  });
};

Database.prototype.close = function close() {
  return new Promise((resolve, reject) => {
    this.connection.close((err) => {
      if (err) reject(err);
      resolve('Connection closed');
    });
  });
};

module.exports = Database;

It works, but I am curious if the way I am doing this seems like a hack or not? Specifically the way I am calling the Promise.all with a map function as the argument. I didn't know how to make an array of connectionWrappers with their params without invoking them, hence the map function in Promise.all(). Any advice would be appreciated!

rdeg
  • 169
  • 3
  • 15
  • 1
    You should [avoid the explicit promise construction anti-pattern](https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it) - in `connectionWrapper` – Jaromanda X Mar 08 '18 at 21:43
  • If this is code you're actually using, and you'd like a review of all aspects of the code, and you've [read this article](https://codereview.meta.stackexchange.com/questions/5777/a-guide-to-code-review-for-stack-overflow-users) and [this one](https://codereview.stackexchange.com/help/on-topic), this might be on topic for [codereview.se]. But do read those and make sure before posting there. – Heretic Monkey Mar 08 '18 at 22:37
  • @JaromandaX I think this a caveat though. `return somePromise` should be the first thing in a function otherwise an error can be thrown in `new Database` that is not a rejected promise and the `.catch` in `handler` won't pick up the error as expected. – Matt Mar 09 '18 at 00:21

0 Answers0