-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resourceful PubSub crashes on 'Many-to-Many through' associations #6933
Comments
@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:
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. |
@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? |
@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. |
I found the same issue while using the Sails v.1.2.3 It looks like the data gets saved correctly, but the pubsub broadcast fails
A workaround is to disable sockets and pubsub for blueprint hooks in the
But obviously not a good one if you rely on them :) |
@vizio360 is right- that's a good workaround for the moment. Although, I think you should be able to leave the |
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
Post
andTag
TagAssignment
as an association tablePost
andTag
and the corresponding associations inTagAssignment
Post.js
Tag.js
TagAssignment.js
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
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 thevia
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 aPost
instance the pubsub routine tries to access the Attributepost
ofTag
- which is undefinied in the general case).The text was updated successfully, but these errors were encountered: