Resourceful PubSub crashes on 'Many-to-Many through' associations

TrueSkrillor commented 4 years ago

Node version: 13.6.0 Sails version (sails): 1.2.3 ORM hook version (sails-hook-orm): 2.1.1 Sockets hook version (sails-hook-sockets): 2.0.0 Organics hook version (sails-hook-organics): 1.0.0 Grunt hook version (sails-hook-grunt): 4.0.1 Uploads hook version (sails-hook-uploads): 0.4.3 DB adapter & version (e.g. sails-mysql@5.55.5): sails-disk@1.1.2 Skipper adapter & version (e.g. skipper-s3@5.55.5): skipper-s3@0.6.0

The issue

I stumbled across the issue while playing around with 'has many through' associations in terms of resourceful pubsub. When removing a model instance with this kind of association one would expect the resourceful pubsub to notify all associated model instances about the removal. This does not work and even worse the pubsub routine crashes when invoking a DELETE on a model with 'has many through' associations.

How to reproduce

Create two models Post and Tag
Create another model TagAssignment as an association table
Create the 'has many through' association in Post and Tag and the corresponding associations in TagAssignment
Spin up sails
Create one Post and one Tag and associate them (actually the association is not necessary as far as I am concerned, I am including this for the sake of completeness)
Delete either the created Post or Tag

Post.js

module.exports= {
  attributes: {
    tags: {
      collection: "tag",
      via: "post",
      through: "tagassignment"
    }
  }
};

Tag.js

module.exports= {
  attributes: {
    posts: {
      collection: "post",
      via: "tag",
      through: "tagassignment"
    }
  }
};

TagAssignment.js

module.exports= {
  attributes: {
    post: {
      model: 'post'
    },
    tag: {
      model: 'tag'
    }
  }
};

Behaviour of different database adapters

The issue behaves different when comparing the sails-disk adapter with the productive adapters (that's why this is not an DoS issue). When using sails-disk the exception thrown is not being caught thus killing the server immediately - rendering 'many-to-many through' associations in development nearly useless. All productive adapters catch the exception and log it to stdout (can result in heavy log spam if many models with this kind of association exist).

Exception with stacktrace

/home/fabian/Dokumente/laboratory/myapp/node_modules/sails/lib/hooks/pubsub/index.js:701
                if (reverseAttribute.model) {
                                     ^

TypeError: Cannot read property 'model' of undefined
    at /home/fabian/Dokumente/laboratory/myapp/node_modules/sails/lib/hooks/pubsub/index.js:701:38
    at arrayEach (/home/fabian/Dokumente/laboratory/myapp/node_modules/@sailshq/lodash/lib/index.js:1470:13)
    at Function.<anonymous> (/home/fabian/Dokumente/laboratory/myapp/node_modules/@sailshq/lodash/lib/index.js:3532:13)
    at newConstructor.<anonymous> (/home/fabian/Dokumente/laboratory/myapp/node_modules/sails/lib/hooks/pubsub/index.js:699:17)
    at /home/fabian/Dokumente/laboratory/myapp/node_modules/@sailshq/lodash/lib/index.js:3061:23
    at /home/fabian/Dokumente/laboratory/myapp/node_modules/@sailshq/lodash/lib/index.js:3236:15
    at Function.<anonymous> (/home/fabian/Dokumente/laboratory/myapp/node_modules/@sailshq/lodash/lib/index.js:3533:13)
    at newConstructor._publishDestroy (/home/fabian/Dokumente/laboratory/myapp/node_modules/sails/lib/hooks/pubsub/index.js:669:13)
    at newConstructor.wrapper [as _publishDestroy] (/home/fabian/Dokumente/laboratory/myapp/node_modules/@sailshq/lodash/lib/index.js:3282:19)
    at destroyedRecord (/home/fabian/Dokumente/laboratory/myapp/node_modules/sails/lib/hooks/blueprints/actions/destroy.js:68:15)
    at proceedToFinalAfterExecLC (/home/fabian/Dokumente/laboratory/myapp/node_modules/parley/lib/private/Deferred.js:1157:14)
    at proceedToInterceptsAndChecks (/home/fabian/Dokumente/laboratory/myapp/node_modules/parley/lib/private/Deferred.js:913:12)
    at proceedToAfterExecSpinlocks (/home/fabian/Dokumente/laboratory/myapp/node_modules/parley/lib/private/Deferred.js:845:10)
    at /home/fabian/Dokumente/laboratory/myapp/node_modules/parley/lib/private/Deferred.js:303:7
    at _afterIteratingOverRecords (/home/fabian/Dokumente/laboratory/myapp/node_modules/waterline/lib/waterline/methods/destroy.js:550:26)
    at /home/fabian/Dokumente/laboratory/myapp/node_modules/waterline/node_modules/async/dist/async.js:486:20
    at /home/fabian/Dokumente/laboratory/myapp/node_modules/waterline/node_modules/async/dist/async.js:855:24
    at eachOfLimit (/home/fabian/Dokumente/laboratory/myapp/node_modules/waterline/node_modules/async/dist/async.js:915:26)
    at /home/fabian/Dokumente/laboratory/myapp/node_modules/waterline/node_modules/async/dist/async.js:920:20
    at eachOf (/home/fabian/Dokumente/laboratory/myapp/node_modules/waterline/node_modules/async/dist/async.js:1052:9)
    at Object.eachLimit (/home/fabian/Dokumente/laboratory/myapp/node_modules/waterline/node_modules/async/dist/async.js:3111:7)
    at /home/fabian/Dokumente/laboratory/myapp/node_modules/waterline/lib/waterline/methods/destroy.js:522:23

Root cause

I am new to sails and do not understand every single aspect of the framework. But I tried to analyse the issue based on the exception thrown. By having a look at the source files I was able to determine the root cause of this issue. It seems like the pubsub routine tries to access the reverse attribute of the referenced model using the via key provided in the model. As in 'many-to-many through' associations one provides the via key of the association table (and not the reverse attribute of the referenced model) pubsub is unable to access the reverse attribute (e. g. when deleting a Post instance the pubsub routine tries to access the Attribute post of Tag - which is undefinied in the general case).

sailsbot commented 4 years ago

@TrueSkrillor Thanks for posting! We'll take a look as soon as possible.

In the mean time, there are a few ways you can help speed things along:

look for a workaround. (Even if it's just temporary, sharing your solution can save someone else a lot of time and effort.)
tell us why this issue is important to you and your team. What are you trying to accomplish? (Submissions with a little bit of human context tend to be easier to understand and faster to resolve.)
make sure you've provided clear instructions on how to reproduce the bug from a clean install.
double-check that you've provided all of the requested version and dependency information. (Some of this info might seem irrelevant at first, like which database adapter you're using, but we ask that you include it anyway. Oftentimes an issue is caused by a confluence of unexpected factors, and it can save everybody a ton of time to know all the details up front.)
read the code of conduct.
if appropriate, ask your business to sponsor your issue. (Open source is our passion, and our core maintainers volunteer many of their nights and weekends working on Sails. But you only get so many nights and weekends in life, and stuff gets done a lot faster when you can work on it during normal daylight hours.)
let us know if you are using a 3rd party plugin; whether that's a database adapter, a non-standard view engine, or any other dependency maintained by someone other than our core team. (Besides the name of the 3rd party package, it helps to include the exact version you're using. If you're unsure, check out this list of all the core packages we maintain.)

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

johnabrams7 commented 4 years ago

@TrueSkrillor Hey, thanks for taking the time to provide us detailed info on this. Appreciate the reproduction steps as well, can you make us a repo that reproduces this issue to help us verify everything is rendering as expected and make it easier for the community to clone & test?

TrueSkrillor commented 4 years ago

@johnabrams7 Sure, no problem. Here you go:

https://github.com/TrueSkrillor/sails-pubsub-association-crash

Just clone and run the reproduceCrash.sh script in the projects root directory. It will automatically clean up and spin up a fresh instance of sails, insert the data using curl and call a DELETE at the end to reproduce this bug.

vizio360 commented 4 years ago

I found the same issue while using the through association in development mode using the default disk adapter.

Sails v.1.2.3 Node v10.16.0

It looks like the data gets saved correctly, but the pubsub broadcast fails

sails/lib/hooks/blueprints/actions/add.js:122
              ChildModel.attributes[associationAttr.via].model &&
                                                         ^

TypeError: Cannot read property 'model' of undefined

A workaround is to disable sockets and pubsub for blueprint hooks in the .sailsrc file:

{
  "hooks": {
    "sockets": false,
    "pubsub": false 
  }
}

But obviously not a good one if you rely on them :)

mikermcneil commented 4 years ago

@vizio360 is right- that's a good workaround for the moment.

Although, I think you should be able to leave the sockets hook enabled, right?

balderdashy / sails