25

I am unfortunately new to node and running into some confusion regarding the asynchronous/synchronous execution of node.

I am using node, sequelize with sqlite and async.js.

I have a series of Articles, each of which has a number of Authors.

For each Authors in each Article, I'd like to check if the Author exists. If not, create it.

The problem is, on the initial run, duplicate authors are being created, I assume due to asynchronous functionality causing an issue with checking for existence.

For example, with the array: authors = ['A. Test', 'B. Test', 'C. Test', 'A. Test']

and the code:

async.each(authors, function(item, callback){
    Author.sync().then(function(){
      Author.count({ where: {name: item.trim()} }).then(function(count){
        if (count != 0) {
          console.log('Author already exists')
        } else {
          console.log('Creating author...')
          Author.create({
            name: item.trim()
          })
        }
      })
    })
  })

On the first run, will create a table:

ID | name
------------
0  | A. Test
1  | B. Test
2  | C. Test
3  | A. Test

What am I doing wrong? I seem to be missing a fundamental concept of asynchronous vs synchronous execution in Node.

(I've also tried async.eachSeries which is supposed to execute in series rather than in parallel?)

Edit: Slightly refactored, but still creating duplicates

async.eachSeries(authors, function(authorName, callback){
    Author.findOne({ where: {name: authorName.trim()} }).
    then(function(author){
      if (author) {
        // Author exists...
        callback()
      } else {
        // Author does not exist...
        Author.create({
          name: authorName.trim()
        }).then(function(author){
          callback()
        })
      }
    })
  })
waffl
  • 4,140
  • 5
  • 60
  • 108

2 Answers2

27

Author.count isn't really needed, unless you need the count. See findOrCreate().

With findOrCreate() you could have the following. (Edited trex005's snippet for this)

async.eachSeries(authors, function(item, callback) {
  Author.sync().then(function() {
    Author.findOrCreate({
      where: {
        name: item.trim()
      },
      defaults: { // set the default properties if it doesn't exist
        name: item.trim()
      }
    }).then(function(result) {
      var author = result[0], // the instance of the author
        created = result[1]; // boolean stating if it was created or not

      if (!created) { // false if author already exists and was not created.
        console.log('Author already exists');
      }

      console.log('Created author...');
      callback();
    });
  })
})
Akash
  • 8,503
  • 1
  • 21
  • 19
Joshua F
  • 768
  • 6
  • 11
  • Hi thanks for the reply, actually I tried `findOrCreate` (and refactored to a somewhat cleaner snippet added to my answer above) but `findOrCreate` was causing all sorts of issues with sqlite blocking/database locking - apparently this was happening in postgres for some users as well. Unfortunately, my new snippet still doesn't actually work, and is still creating duplicates. – waffl Aug 20 '15 at 14:21
  • 1
    @joshua-f btw, you can use spread method instead of then method for seperating promise arguments. Example: `.spread(function(author, isCreated) { ... });` – Ali BARIN Aug 20 '15 at 18:24
  • @AliBARIN Oh sweet, did not know that. – Joshua F Aug 20 '15 at 21:35
  • Is there a reason for calling sync() inside the async function? – Ronen Teva Mar 03 '16 at 10:22
  • @RonenTeva No, that should probably be outside of the async function. – Joshua F Apr 17 '16 at 07:22
0

Change your each to eachSeries and actually call the callback and you should be golden.

async.eachSeries(authors, function(item, callback){
    Author.sync().then(function(){
      Author.count({ where: {name: item.trim()} }).then(function(count){
        if (count != 0) {
          console.log('Author already exists')
          callback(); //assuming you want it to keep looping, if not use callback(new Error("Author already exists"))
        } else {
          console.log('Creating author...')
          Author.create({
            name: item.trim()
          }).then(function(author){
            callback();
          })
        }
      })
    })
  })
trex005
  • 4,637
  • 1
  • 22
  • 39
  • Hm, this doesn't appear to be working, it's only adding the first author. – waffl Aug 05 '15 at 00:02
  • Does your Author.create support a callback like I mentioned in the comments? – trex005 Aug 05 '15 at 00:03
  • Sorry, how do you mean? The `create` method is [implemented by sequelize](http://docs.sequelizejs.com/en/latest/api/model/#createvalues-options-promiseinstance) which results in a promise – waffl Aug 05 '15 at 00:06
  • Ok, it works, thankyou! Just to clear my understanding of the flow, what is the `callback()` function that is called? I find the wording of the [async.js documentation](https://github.com/caolan/async#each) confusing. – waffl Aug 05 '15 at 08:09
  • It tells the loop to continue – trex005 Aug 06 '15 at 01:33
  • Sorry, unfortunately after further testing, the function it is still creating duplicate results :( – waffl Aug 20 '15 at 14:26