stubailo / meteor-rest

:sleeping: simple:rest - make your Meteor app accessible over HTTP and DDP alike
https://atmospherejs.com/simple/rest
MIT License
382 stars 85 forks source link

Compatibility with mdg:validated-method and roadmap #77

Open tcastelli opened 8 years ago

tcastelli commented 8 years ago

Hello, this package looks nice and a really easy way to get a quick REST API. Still, I was looking for a few things before jumping in (comparing this with restvivus atm) so I would like to know the current positioning regarding:

stubailo commented 8 years ago

validated method should work out of the box, since this literally just overrides Meteor's method registration mechanism.

I don't have a lot of time right now to look at integration with restivus. What kind of integration were you thinking? I can't see any reason you wouldn't just use both in the same app.

As for defining methods for API use only, I'm not sure why you would do that - thoughts?

tcastelli commented 8 years ago

Thanks for the quick reply, and yes, quite hard to find time around holidays, so no hurries :)

If I get this correctly you mean that we can do for example

const methodBody = function(){} //...
const restOptions ={}; //url,...

const method = new ValidatedMethod({
  name:"return-five", 
  validate:null,
  run: methodBody 
});
 and then

Meteor.method("return-five", methodBody , restOptions );

Is it possible to add this directly into validated-method(something like this)?

const method = new ValidatedMethod({
  name:"return-five", 
  validate:null,
  run: methodBody 
  rest:restOptions
});

As for the integration with restivus, I have been checking again and I have thought of how to do previously commented things without adding both packages (In my opinion having both packages for doing basically the same thing makes no sense and just adds an extra effort to follow the updates of both projects).

Correct me if I'm wrong but:

tcastelli commented 8 years ago

@stubailo any thoughts about adding this to the validatedmethod package and the other 3 topics? I later saw that the API versioning topic was already debated but not sure what was taken as a result of that discussion(I guess that If we want that we have to use retivus for now), but also would like to know your opinion about custom-auth and DDP/HTTP only

aldeed commented 8 years ago

@tcastelli Some thoughts I have:

  1. I'm generally with @stubailo that everything should be available through both, even if you don't intend for it to be used that way or don't document it. In the end it's just a matter of which protocol is used, so your app logic shouldn't care. BUT, this does not seem like something the package should be opinionated about, so I think the easiest solution is to add this.isDDP and this.isHTTP booleans that you can check in the method, like if (this.isHTTP) return; The endpoints would still be created, but would be no-op if you want.
  2. You can use your own middleware to reject requests from certain users. Add simple:rest-accounts-password and simple:rest-json-error-handler unmodified. Then

    JsonRoutes.Middleware.use(function (req, res, next) {
     if (!checkUser(req.userId)) throw new Meteor.Error('unauthorized', 'Sorry, you cannot access this');
    });
  3. I think it would be great if core methods eventually had extendable options that various packages could use to enable validation, rest, and other icing. Maybe validated-method could try out an API for that and then pull into core after fine tuning if it makes sense.
stubailo commented 8 years ago

I'm testing out something like this:

ValidatedMethod mixins: https://github.com/meteor/validated-method#mixins

simple:rest mixin: https://github.com/stubailo/meteor-rest/blob/master/packages/rest-method-mixin/README.md

tcastelli commented 8 years ago

@aldeed 1) this.isDDP and this.isHTTP would be fine for me, but in the end, if we already have those, making general abstraction to return no-op for the methods that you want should be trivial (so maybe it could be enabled by default in validated-method or a mixin before exceuting the run function). what do you thinkg about something like this

ValidatedMethod.http_methods = ["login","logout"]; //by default all of them
ValidatedMethod.ddp_methods = ["register"]; //by default all of them

const method = new ValidatedMethod({
  name:"login", 
  validate:null,
  run: methodBody
  restOptions: {...}
});

and then in validated-method before executing the run function add something like

if (this.isHTTP && _.contains(ValidatedMethod.http_methods,this.name) || this.isDDP &&  _.contains(ValidatedMethod.ddp_methods,this.name) )
   return this.run()
else exception or ignore
} 

2) Will dig into that and try it out, thanks for the idea 3) Mixins seem a great addition to introduce customization in validated-method so will try that soon too thanks @stubailo :)