Upload .JSON product list to MongoDB and upload image to AWS S3

Question

I'm using Node/Express/Mongoose to accept a JSON file containing a list of product details. These products are looped through, the images are uploaded to AWS S3, and the product is either accepted or rejected depending on validation. In the end the accepted products are uploaded to Mongo via Mongoose and all are returned to provide info to the uploader. This is my first time with any of these frameworks so I'm looking to improve best practices and find potential points of failure.

ProductRoutes.js

const keys = require("../config/keys.js");
const mongoose = require("mongoose");
const requireLogin = require("../middlewares/requireLogin");
var validator = require("validator");
const fileUpload = require("express-fileupload");
var fs = require("fs");
const aws = require("aws-sdk");
const S3_BUCKET = keys.awsBucket;
var path = require("path");

var request = require("request");
aws.config.update({
  region: "us-east-2",
  accessKeyId: keys.awsAccessKey,
  secretAccessKey: keys.awsSecretKey
});

require("../models/Product");

const Product = mongoose.model("product");

function validate(value, type) {
  switch (type) {
    case "string":
      return value && !validator.isEmpty(value, { ignore_whitespace: true });
    case "url":
      return (
        value &&
        !validator.isURL(value, {
          protocols: ["https, http"],
          require_protocol: true
        })
      );
    default:
      return value && validator.isEmpty(value, { ignore_whitespace: true });
  }
  return value == null || value.length === 0;
}
function saveImage(url, key) {
  let ext = path.extname(url);
  let params = {
    Key: key + ext,
    Bucket: S3_BUCKET,
    ACL: "public-read"
  };
  return new Promise(function(resolve, reject) {
    request.get(url).on("response", function(response) {
      if (response.statusCode === 200) {
        params.ContentType = response.headers["content-type"];
        var s3 = new aws.S3({ params })
          .upload({ Body: response })
          .send(function(err, data) {
            resolve(data);
          });
      } else {
        // return false;
        reject(false);
      }
    });
  });
}

module.exports = app => {
  app.use(fileUpload());

  app.post("/product/addProduct", requireLogin, async (req, res) => {
    let products = req.files.file.data;
    try {
      products = JSON.parse(products);
    } catch (e) {
      return res
        .status(400)
        .json({ success: false, message: "Invalid JSON product feed" });
    }
    let accepted = [];
    let rejected = [];

    for (const product of products) {
      if (!validate(product.sku, "string")) {
        rejected.push(product);
        return;
      }
      if (!validate(product.image_url, "url")) {
        rejected.push(product);
        return;
      }
      try {
        let result = await saveImage(product.image_url, `${product.owner}/${product.sku}`);
        product.image_url = result.Location;
      } catch (err) {
        // catches errors both in fetch and response.json
        return res.status(400).json({
          success: false,
          message: "Could not upload image",
          error: err
        });
      }

      let upsertProduct = {
        updateOne: {
          filter: { sku: product.sku },
          update: product,
          upsert: true
        }
      };
      accepted.push(upsertProduct);
    }

    // now bulkWrite (note the use of 'Model.collection')
    Product.collection.bulkWrite(accepted, function(err, docs) {
      if (err) {
        return res.status(400).json({
          success: false,
          message: "Something went wrong, please try again"
        });
      } else {
        return res.status(200).json({
          success: true,
          message: "Company successfully created",
          accepted: { count: accepted.length, list: accepted },
          rejected: { count: rejected.length, rejected: rejected },

          affiliate: docs
        });
      }
    });
  });

  app.get("/product/fetchAffiliateProducts", requireLogin, (req, res) => {
    var affiliateId = req.query.affiliateId;

    Product.find({ owner: affiliateId }, function(err, products) {
      if (err) {
        return res.status(400).json({
          success: false,
          message: "Could not find the requested company's products"
        });
      } else {
        return res.status(200).json({
          success: true,
          message: "Products successfully found",
          products: products
        });
      }
    });
  });
};

Product.js (model):

const mongoose = require('mongoose');
const {Schema} = mongoose;

const productSchema = new Schema({

    sku: {type: String, unique: true, required: true},
    name: {type: String, required: true},
    owner: {type: String, required: true},
    image_url: {type: String, required: true}
});

mongoose.model('product', productSchema);

Sample input

[
  {
    "sku": "123",
    "name": "Test Product 1",
    "owner": "Test Company 1",
    "image_url": "https://exmaple.com/src/assets/product1.png"
  },
  {
    "sku": "456",
    "name": "Test Product 3",
    "owner": "Test Company 2",
    "image_url": "https://exmaple.com/src/assets/product2.png"
  },
  {
    "sku": "789",
    "name": "Test Product 3",
    "owner": "Test Company 3",
    "image_url": "https://exmaple.com/src/assets/product3.png"
  }
]

If there are other contextual files/code needed let me know; this is all that seemed relevant.

Answer 1

In general I would say the code looks fine as far as functionality, though there are a few specific points which I will mention below with more detail. The broadest critique I have is that there are some variables declared with var or let that could use const since they are never re-assigned - for instance validator, ext, params, request, rejected, accepted etc. Even if items are pushed into an array, const can be used since it doesn't imply immutability.

Also, the HTTP response code returned are either 200 or 400, but in some cases other codes may be more appropriate:

• 404 for requests to /product/fetchAffiliateProducts that yield no results
• 500 for requests to /product/addProduct where the call to Product.collection.bulkWrite throws an error.

Additionally, as others have pointed out, the routes file is quite large. Typically a route file would reference functions (e.g. controller methods) in other files.

---

In the route for adding a product, the variable products is used for both the raw value as well as the parsed value:

app.post("/product/addProduct", requireLogin, async (req, res) => {
  let products = req.files.file.data;
  try {
    products = JSON.parse(products);
  }

To improve readability, don't re-use the variable name - instead use a separate one like rawProducts for the first one:

const rawProducts = req.files.file.data;
try {
  const products = JSON.parse(rawProducts);
}

That way const can be used because there is no need to re-assign that value. This can also avoid accidental re-assignment in other places too.

---

The logic that validates inputs is intriguing:

let accepted = [];
let rejected = [];

for (const product of products) {
  if (!validate(product.sku, "string")) {
    rejected.push(product);
    return;
  }
  if (!validate(product.image_url, "url")) {
    rejected.push(product);
    return;
  }

If the call to validate() returns false for any one of those elements in the products array, then the return will immediately end the entire function, thus rendering rejected (as well as accepted) useless and no response returned to the user (until the script times out). Perhaps you intended to use continue instead.

---

I also dug into the validate function and find it intriguing as well. For instance, the URL validation looks like this:

case "url":
  return (
    value &&
    !validator.isURL(value, {
      protocols: ["https, http"],
      require_protocol: true
    })
  );

which appears to ensure that value is not falsey and the call to validator.isURL() is a falsey value. Shouldn't the code expect the call to validator.isURL() to return a truthy value?

I'm also intrigued by the other cases in that switch statement:

case "string":
  return value && !validator.isEmpty(value, { ignore_whitespace: true });

The first conditional basically ensures that the value is not false, null, undefined, 0, NaN, an empty string/template literal and then the call to validator.isEmpty will "check if the string has a length of zero."¹

Then the default case does something similar except the second conditional is not negated:

default:
  return value && validator.isEmpty(value, { ignore_whitespace: true });

Your code sample doesn't show any cases where the default would be used but it makes me wonder when that would be helpful. And as others have pointed out, there is a return statement after the switch statement, which becomes unreachable by the default case.

¹_{https://www.npmjs.com/package/validator#user-content-validators}

Answer 2

There's nothing outstandingly wrong, there are some minor points that can be improved.

The main concern that I have is the amount of logic happening inside the ProductRoutes file. Your invocation of the AWS library and the validate & saveImage functions can be stored inside reusable files that can be referenced anywhere.

I normally would store the invocation of the AWS library inside the index file to ensure the connection starts alongside the Express server start.

It also seems to me that there are some features afforded by modern (ES6) javascript that you aren't consistently using, like your request promise wrapper inside saveImage.

---

Inside the validate function, the final return statement is unreachable as all other cases will be caught in the default statement:

function validate(value, type) {
  switch (type) {
    <...>
    default:
      return value && validator.isEmpty(value, { ignore_whitespace: true });
  }
  return value == null || value.length === 0;
}

---

return new Promise(function(resolve, reject) {
  request.get(url).on("response", function(response) {
    if (response.statusCode === 200) {
      params.ContentType = response.headers["content-type"];
      var s3 = new aws.S3({ params })
        .upload({ Body: response })
        .send(function(err, data) {
          resolve(data);
        });
    } else {
      // return false;
      reject(false);
    }
  });
});

As I mentioned briefly above, this is a bit unnecessary. Instead of using request you can use request-promise or request-promise-native and to simplify things further, you can use async/await:

const response = await request(url, { resolveWithFullResponse: true });
if (response.statusCode !== 200) {
  return Promise.reject();
}
params.ContentType = response.headers["content-type"];
const data = await new aws.S3({ params })
  .upload({ Body: response })
  .promise();

---

fs is unused in ProductRoutes.js:

var fs = require("fs");

---

The final return statement in validate is unreachable as all other cases will be caught inside the default case of the switch.

function validate(value, type) {
  switch (type) {
    ...
    default:
      return value && validator.isEmpty(value, { ignore_whitespace: true });
  }
  return value == null || value.length === 0;
}

---

Your use of ES6 features is fairly inconsistent, you should be using things like template strings here:

let params = {
  Key: key + ext,

let params = {
  Ket: `${key}${ext}`

Answer 3

I agree with Quill - 90% of the stuff in your routes should be in other files. Routes must be concise, otherwise it's hard to tell which routes you have.

Additionally, think about the processes, not the objects. Here's what your addProduct handler does:

• Iteration over every product
• Validation
• Uploading of image
• Saving of product data

Ultimately, you want to filter out bad products, upload all the images, then save the data for the whole set of products.

The thing that might not be obvious, is that the uploading of images is happening sequentially, and you could be doing this in parallel.

You also need to think through your error handling. Maybe you've done this, but I'd argue it's better to upload and save as many products as possible, rather than aborting on the first failure.

const saveProducts = async products => {
  // Start image uploads in parallel
  const upload_operations = upload_images(products); // Don't await!

  // Wait for everything (merge)
  const results = await Promise.all(upload_operations);

  // Generate upserts
  const final_products = results.filter(is_successful)
    .map(result_to_product)
  const upserts = final_products.map(product_to_upsert)

  // Store product collection
  try {
    await Product.collection.bulkWrite(upserts);
    return final_products;
  } catch (e) {
    await Promise.all(uploads.map(rollback)); // Warning - this could also throw
    return [];
  }
}

const upload_images = products => products.filter(is_valid)
  .map(saveImage); // Don't await

const saveImage = async product => {
  // Note: this function is essentially a Promise – we can wait on it whenever
  try {
    await uploadImage(product);
    // Note: this just suspends execution of saveImage()
    // ... callers can await whenever.
    return {
      product
    };
  } catch (e) {
    console.log('failure!'); // Never forget this
    return {
      fail: true
      product
    }
  }
};

const is_valid = product => { /* Return true if it's valid */ };
const is_successful = operation => !operation.fail;
const result_to_product = result => result.product;
const product_to_upsert = result => { /* convert result.product to upsert JSON */ };
const rollback = upload => { /* Delete result.product */ };