Skip to content
This repository has been archived by the owner on Aug 6, 2021. It is now read-only.

[feature] Add beforeAction and afterAction event callback #13

Closed
guiltry opened this issue Feb 22, 2015 · 18 comments
Closed

[feature] Add beforeAction and afterAction event callback #13

guiltry opened this issue Feb 22, 2015 · 18 comments

Comments

@guiltry
Copy link

guiltry commented Feb 22, 2015

Hi,

What if we add an event hook base on sails service.
For example, we can change this part,

/* if ( req.user && req.user.id ) {
   sails.log.debug( 'Injecting req.user into blueprint create -> data.' );
  data.user = req.user.id;
} else {
  // exception for creating new users, otherwise any creative act needs a logged in user
  if ( Model.identity !== 'user' ) return res.forbidden( "Create blueprint needs an authenticated user!" );
} */

to this,

// call sails service
// Note: it's model, not Model
beforeCreate.process(req, res, model, data);

and inside beforeCreate service,

module.exports = {
  process: function ( req, res, model, data ) {

    // We can do a lot of thing here without need to override your blueprint
    // Check if you want to do something with this kind of model
    // Ex: you want to add user.id just only for foo model
    if ( model === 'foo' ) {

      // Do the logic
      if ( req.user && req.user.id ) {
        sails.log.debug( 'Injecting req.user into blueprint create -> data.' );
        data.user = req.user.id;
      } else {
        if ( Model.identity !== 'user' ) return res.forbidden( "Create blueprint needs an authenticated user!" );
      } 
    }
  }
};

List action:

etc.

Then, people will move their logic outside and we can easily support for backward compatibility.
If you want, I'll create a PR about this.

Btw. thanks for this awesome repo. 😄

@mphasize
Copy link
Owner

Hi @guiltry – indeed this seems pretty similar to what we're discussing in #9. I like that it enables to use the blueprints in whichever style you want and makes the model lifecycle more powerful. I always wondered why the original Sails beforeCreate lifecycle method was so limited and didn't give access to the request from the client.
@Globegitter What do you think? Should we enable a more powerful set of lifecycle callbacks?

@guiltry
Copy link
Author

guiltry commented Feb 22, 2015

OMG, sorry I didn't read PR and that just 22 hours ago. Haha.

Doesn't it good? I guess it's the sails team purpose.
So, developers don't make their model more fatter, instead they pull it off to be a reusable component.

@IanVS
Copy link

IanVS commented Feb 28, 2015

@mphasize I've been trying to find a good way to catch 500 errors thrown by MongoDB when adding duplicate values to a unique key. I tried using a beforeValidate lifecycle callback to check up-front if the unique key was already in the database (which I could do) and then send a res.customResponse() to send a nice error back to the client (which I can't figure out how to do). If the lifecycle callbacks had access to the response, I think this would be possible.

I think this would also help solve the issue you raise at: https://github.com/balderdashy/waterline/issues/295#issuecomment-45596117

@Globegitter
Copy link
Contributor

@mphasize Just had some time to read through that properly and think about that properly. Yeah I think that is a great idea. And not sure if you want to handle this with your PR, but I think any addition from @guiltry would be great!

@mphasize
Copy link
Owner

mphasize commented Mar 2, 2015

@guiltry @Globegitter

I put together a Policy that removes the need for the uncommented lines in the create blueprint. See Gist here: https://gist.github.com/mphasize/a69d86b9722ea464deca

I'm planning to put together another Policy that enables the before* hooks with access to req on the model definition. This way we can keep this stuff out of the blueprints and have it easily extendable.

Thoughts?

@Globegitter
Copy link
Contributor

@mphasize That looks pretty good from what I can tell, but I think would also interest @mgenev who is mostly responsible for https://github.com/Globegitter/sane-auth. To make sure they work nicely together :)

@Globegitter
Copy link
Contributor

Also @mphasize I just had a chat with @huafu the creator of https://github.com/huafu/ember-data-sails. It would be really nice to have these two plugins work nicely together to just get Websockets 'for free'. He said he does want to switch to conform to the RESTAdapter, his biggest question do you know if these blueprints also just work with websockets?
Then he said he could just write a websocket adapter for it.

But maybe you can also have a little chat here yourself :)

@mphasize
Copy link
Owner

mphasize commented Mar 2, 2015

@huafu @Globegitter I kept the websocket specific stuff from the original blueprints and didn't modify it. So there's a chance that websockets "just work", however I've never tried it because I'm not yet using sockets.

@mphasize
Copy link
Owner

mphasize commented Mar 2, 2015

@guiltry @Globegitter
Here's the next Policy, enabling "before*Blueprint" hooks into the model with a (req, res, next) footprint.
https://gist.github.com/mphasize/e9ed62f9d139d2152445

@guiltry
Copy link
Author

guiltry commented Mar 3, 2015

@mphasize Beautiful!

@huafu
Copy link

huafu commented Mar 3, 2015

@mphasize @Globegitter the latests master branch of my adapters (REST and Socket) do work with the latest Sails (0.11) without blueprints by just setting pluralize: true of the blueprints config of Sails.

I do believe that you can either use one or the other adapter (original DS.RESTAdapter or my SailsSocketAdapter) for any model, but not both. In that case, as @mphasize blueprints are only for http, it'll work from scratch.

You could have DS.RESTAdapter as your application adapter, and my SailsSocketAdapter for your message and online-status models for example.

@mphasize
Copy link
Owner

mphasize commented Mar 3, 2015

@huafu Yep, thinking along the same lines here. Somebody just needs to go check if it works. :-D

@mphasize mphasize mentioned this issue Mar 3, 2015
@mphasize
Copy link
Owner

mphasize commented Mar 3, 2015

We do not have a solution for an after*Blueprint hook yet, but the use cases for that are a lot rarer than for before and we got a nice policy solution for that, so I will close this issue for the moment.

@huafu
Copy link

huafu commented Mar 8, 2015

@mphasize so I've fixed some issue appearing with the prototype extension from Ember conflicting with some stuff in sails io js, and published 0.0.13 of my ember-data-sails addon. Then I tested with your blueprints, and it appears that they do change the format of the web socket responses too...

So either there is a simple way to disable your blueprints for websocket and enable them only for http, or I'll have to add an option in my addon to be set when used with your blueprints :-/ (and of course change the code to handle that).

Any clue if it is possible to simply use your blueprints only for http?

@huafu
Copy link

huafu commented Mar 8, 2015

@mphasize ok, it DOES work by setting the defaultSerializer to '-rest' in the adapter definition of your application ;-)

huafu added a commit to huafu/ember-data-sails that referenced this issue Mar 8, 2015
@mphasize
Copy link
Owner

mphasize commented Mar 9, 2015

@huafu So we really have an out-of-the-box solution here? That's great news and thanks a lot for figuring out how to do the right setup!

@huafu
Copy link

huafu commented Mar 9, 2015

yup! still want to work on more unit tests, but yup, all working ;-)

@mgenev
Copy link

mgenev commented May 11, 2015

@huafu do you have the working example somewhere lying around that can go up on github? :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants