Using hooks with arrays causes after hooks to run x number of times.

[marshallswain]

I discovered this odd behavior by using an afterHook on create with a beforeHook on another service's create. I'm not sure where this is happening, yet. Update: it's due to how some of the adapters handle arrays.

Below is an example to duplicate the issue. Switch between the create statements and you'll see the difference.

var feathers = require('feathers');
var feathersHooks = require('feathers-hooks');
var bodyParser = require('body-parser');
var rest = require('feathers-rest');
var memory = require('feathers-memory');

// Create a feathers instance.
const app = feathers()
  .configure(feathersHooks())
  .configure(rest())
  // Turn on JSON parser for REST services
  .use(bodyParser.json())
  // Turn on URL-encoded parser for REST services
  .use(bodyParser.urlencoded({ extended: true }));

// Create an in-memory Feathers service with a default page size of 2 items
// and a maximum size of 4
app.use('/todos', memory());
app.use('/tests', memory());

app.service('/todos').after({
  create: [
    function(hook){
      console.log('todos:after');
      return new Promise(function(resolve){
        console.log('hi');
        app.service('/tests').create({
          text: 'Server test'
        }).then(function(test){
          resolve(hook);
          console.log(test);
        });
      });
    }
  ]
});

app.service('/tests').before({
  create: [
    function(hook){
      console.log('tests:before');
      return new Promise(function(resolve){
        console.log('hi');
        resolve(hook);
      });
    }
  ]
});

// Create Todos
app.service('todos').create([
  {
    text: 'Server todo',
    complete: false
  },
  {
    text: 'Client todo',
    complete: false
  }
]);

// Create a dummy Todo
// app.service('todos').create({
//   text: 'Server todo',
//   complete: false
// });

app.service('todos').find().then(function(todos) {
  console.log('Found todos', todos);
});

// Start the server.
var port = 3030;

app.listen(port, function() {
  console.log(`Feathers server listening on port ${port}`);
});

Here's the output when creating a single object:

Bitovis-MacBook-Pro:mma-api Bitovi$ node test
todos:after
hi
tests:before
hi
Found todos [ { text: 'Server todo', complete: false, id: 0 } ]
{ text: 'Server test', id: 0 }

Here's the output when using create with the array:

todos:after
hi
todos:after
hi
tests:before
hi
tests:before
hi
Found todos []
{ text: 'Server test', id: 0 }
{ text: 'Server test', id: 1 }
todos:after
hi
tests:before
hi
{ text: 'Server test', id: 2 }

[marshallswain]

It's actually running the hooks data.length number of times.

[marshallswain]

Found it:
feathers-memory: https://github.com/feathersjs/feathers-memory/blob/master/src/index.js#L74
feathers-mongoose: https://github.com/feathersjs/feathers-mongoose/blob/master/src/service.js#L105
feathers-knex: https://github.com/feathersjs/feathers-knex/blob/master/src/index.js#L152

These ones are different:
feathers-sequelize: https://github.com/feathersjs/feathers-sequelize/blob/master/src/index.js#L70.
feathers-nedb: https://github.com/feathersjs/feathers-nedb/blob/master/src/index.js#L83
feathers-waterline: https://github.com/feathersjs/feathers-waterline/blob/master/src/index.js#L72

I think this one would even work and not call hooks repeatedly:
feathers-mongodb: https://github.com/feathersjs/feathers-mongodb/blob/master/lib/index.js#L138

[marshallswain]

If this is the behavior that we expect, then we probably do not want hooks to handle arrays. It would be better for hooks to ignore arrays and only process single objects. I change my mind. It doesn't make sense to not take advantage of bulk updates.

[marshallswain]

Sequelize, nedb, waterline, and mongodb are different. I think the feathers-sequelize adapter gets it right.
feathers-sequelize: https://github.com/feathersjs/feathers-sequelize/blob/master/src/index.js#L70.

This is the way that makes more sense to me. Instead of doing x number of rapid-fire requests to the database, it seems smarter to take advantage of the bulk update feature (that's present in all of the main db adapters) and make a single request to the database.

We already have the code in place to provide events for all of the items in the array, which is really the only thing that I think needs to be done individually.

[daffl]

This is a great catch. Thinking about it (on a Friday night past wine'o clock) I would say the adapters need to have a way to call the original methods without any mixin functionality. One way might be to just store the original methods as _patch, _create etc. during initialization.

There was similar problems when using pagination. feathersjs-ecosystem/feathers-sequelize#8 is very likely related to this.

[marshallswain]

I think we definitely want to treat arrays as single requests to the db, otherwise our apps will hit 5000+ requests per second much too easily.

[daffl]

I don't think it is that common to create thousands of items in an array but we should indeed use any bulk create mechanism that the database provides.

The specific problem here, that hooks and other mixins should not run if the method is called internally and the issue I referenced can probably be fixed by simply adding a _create and _find that runs the DB operations directly. I will try to get to it today.

[marshallswain]

I'm not as concerned about thousands of item in an array as I am about a couple hundred with a lot of users.

I'm able to get feathers-mongoose to work correctly by removing the array check completely. It passes all of the tests.

[marshallswain]

OK. It took me a while to figure out how _create and _find would be useful, but nobody said I was smart. That will be useful for modules like feathers-memory where you have to roll your own array support. For modules with bulk support we can use it, otherwise we fall back to the new functions. Am I following you correctly?

[daffl]

I'm doing an update of the database adapters today (just like feathersjs-ecosystem/feathers-memory#13). Just keeping track of which ones I've done:

• feathers-memory
• feathers-nedb
• feathers-mongoose
• feathers-sequelize
• feathers-knex
• feathers-waterline

[ekryski]

@marshallswain great find man! I know @corymsmith ran into this a couple times but he wasn't sure if he was just doing something wrong.

[marshallswain]

Thanks! I think this is what was causing issues for @BigAB the other day, too.

[daffl]

All database adapters have been updated accordingly. Closing this issue now.

[orverduzco]

@daffl I'm running into this issue again using feathers-rethinkdb, any fix for rethinkdb specifically? I don't see it in the list above.

[daffl]

This is a different issue, tracked in feathersjs-ecosystem/feathers-rethinkdb#80

[lock]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue with a link to this issue for related bugs.