visionmedia / express-resource

Resourceful routing for Express
1.41k stars 140 forks source link

Allow Passing in Route Middleware on Resource #8

Open mhemesath opened 13 years ago

mhemesath commented 13 years ago

It would be nice to be able to set route middleware. Specifically, when mapping the users resource: var user = app.resource('users', require('./user'));

It would be great to do something like:

var user = app.resource('users', require('./user'), { before: authenticateUser });

which is would be the equivalent of app.get('/users', authenticateUser, require('./user').index);

tj commented 13 years ago

yeah its a bit tough since certain actions will want some middleware, some will not, which is why in general I'm not a huge fan of rails-ish stuff haha, but we will figure out something that works

mhemesath commented 13 years ago

what about allowing for the action to be a function, or an array of functions if middleware is involved?

exports.index = function(req, res){ res.send('forum index'); };

or

exports.index = [authenticateUser, function(req, res){ res.send('forum index'); }];

tj commented 13 years ago

i dunno, i just dont want it to become more ugly than just using app.get() and friends

mhemesath commented 13 years ago

Yeah, app.get works fine, especially with app.param, thats what I'm doing now. The routes are very clean, I'm just lazy and don't like typing them all out :p

emostar commented 13 years ago

It looks like this isn't being advanced. How can I get middleware working? Instead of

app.resource

Use

app.(get|post|etc) ?

flockonus commented 13 years ago

+1 Middleware working would be a huge plus, since I am trying to do a API that requires auth for it routes, like so:

app.get('admin/*', [mustAuth], function(req, res, next){
  if( req.authed ){
    next()
  } else {
    res.json(['must auth'])
  }
});

app.resource('admin/users',  require('./users'));

And this isn't working. Any insights?

tj commented 13 years ago

that looks ok @flockonus, that's essentially what express-resource middleware would be expanding to

flockonus commented 13 years ago

Thanks! In this case, the Middleware request may be postponed :P

I just had 1 slash wrong..

For anyone looking for future references;

 app.all('/admin*', function(req, res, next){

   if( req.session.authed ){
     next()
   } else {
     res.json(['must auth'])
   }
 });

 app.resource('admin/users',  require('./users'));

That means that all requests beginning by /admin... will be filtered and resources bellow will be unavailable unless session.authed is set

j-mcnally commented 13 years ago

I implimented applying middleware to all resource routes however the middleware array must always be present because i havent worked out checking types and moving params yet. Also i plan to switch the middleware array for an object that would let u define seperate middleware to each action, here what i got so far:

https://gist.github.com/1155433

erick2red commented 12 years ago

@flockonus. That's works. and that will have to till they decide to include middleware route to the module. Thxs

secretfader commented 12 years ago

Any updates on this issue?

tj commented 12 years ago

i still feel gross about the api, at this point the resource stuff looks worse than some simple app.VERB calls, i wouldnt mind brainstorming some nicer alternatives

secretfader commented 12 years ago

I hear that. I've been trying to think of alternatives myself, and have resorted to namespaces until I find a better resource oriented API.

I'll try to put some time in on this idea during the week ahead. I've reached the point where namespacing is getting ugly, and I crave something cleaner.

tj commented 12 years ago

yeah i dont like namespaces either. personally i think there's nothing cleaner or simpler than just listing them out https://github.com/visionmedia/express/blob/master/examples/route-separation/app.js#L24

sure it's a bit verbose, but it's dead simple to implement, simple to scan, simple to grep if necessary.

perhaps just:

app.resource('users', users)
  .use(authenticateUser)
  .use(whatever)

but that obscures placing middleware specifically after, though that is usually less important

tj commented 12 years ago

I guess my point is that these sort of abstractions always get in the way, but I agree we need something

secretfader commented 12 years ago

Not to bash the current API, because you did great work with it, but what exists is tolerable. If we could add in a way to pass in route specific middleware, then I think everyone would be happy. But on my previous attempts to add route middleware to the current API, I always made it dirtier.

Perhaps a re-think is in order.

flockonus commented 12 years ago

Been using this resource before, I found it kinda hard to fit it in any use other than JSON API, how are you guys use-case?

I find it most useful on administrative interface only, in such case where URL don't really matter. This is that case I believe a such a router may solve.

As @visionmedia is accepting some brainstorm, I came up with this sort of CRUD code generator (using Mongoose), very alpha, https://gist.github.com/1495306 let me know what you guys think of it.

My ultimate goal with this is to make something generic enough, so people can easily build some administrative interface basically out of models( with validations and associations). Kinda out of the subject, right?

hunterloftis commented 12 years ago

We've been just listing out the app.VERBs and that works well at the beginning - but eventually you have hundreds of lines of routes and it can become a headache. Also with several devs working on a project something like express-resource can help to enforce a standard routing pattern, make updates faster, etc... So I would love to see this worked out.

Just ended up here after trying this, hoping it might work like the arrays you can pass to VERBs:

module.exports = {
  create: [
    filters.is_user,
    function(req, res) {
      // ...
    }
  ],
};

What are people's thoughts on an API like that? I don't like the idea of enforcing the middleware to all routes of a given resource because, as TJ said, most of them will be unnecessary so now your middleware logic gets messy since you're ignoring middleware half the time.

secretfader commented 12 years ago

@hunterloftis That's the kind of API I would prefer. Simple, and to the point. It also would allow us to auto-generate the routes from the extension, and easily add custom routes and verbs as well. It definitely improves the existing extension, while retaining the good parts.

hunterloftis commented 12 years ago

Just issued a pull request for this:

https://github.com/visionmedia/express-resource/pull/44

Drop separate functions per format?

I omitted more complex handling of formats in middleware'd routes. What do you guys think? I recommend we drop that feature since it makes complex something that is pretty simple. eg:

we're already creating routes that include ?format & saving req.format, so:

exports.show = function(req, res) {
  // common business logic here, no matter what format we're returning
  // ...
  // now that we've done our logic we can respond based on the format:
  if (req.format === 'json') // ...
  else if (req.format === 'xml') // ...
  else // ...
};

Is the separate function thing actually something people use? Am I overlooking a use-case?

hunterloftis commented 12 years ago

A better solution that breaks backwards-compatibility for function-mapped-formats:

https://github.com/visionmedia/express-resource/pull/45

flockonus commented 12 years ago

The way of use looks really fine, definitely a improvement

tj commented 12 years ago

if you break down some of the business logic into middleware you can use the format callbacks quite easily but I dont disagree with what you're saying

tj commented 12 years ago

IMHO it's still easier to reason with:

exports.show = [
  loadUser,
  ensureRole('admin'),
  function(req, res) {
    if (req.format == 'json') {
      ...
    } else {
      ...
    }
  }
]

app.get('/users*', loadUser, ensureRole('admin'));
app.get('/users.json', users.json);
app.get('/users', users.html);

but I'll pull the request down and have a better look

hunterloftis commented 12 years ago

I agree that middleware is an even better solution for most use cases.

I just wanted to be clear that my implementations omit the action: { format1: ..., format2: ... } support, in favor of the type of response you just posted.

tj commented 12 years ago

too bad they're not like lua tables we could just have both cleanly. for now i'll probably merge the one that doesn't break backwards compat

gcmurphy commented 12 years ago

I'm not the best with js but thought I'd share the hack I'm considering running with as a solution to this problem: https://gist.github.com/3102077. Just out of interest has anything been decided on this?

panta commented 12 years ago

FYI, I've create a pull request supporting middleware. It's fully backward compatible and it includes tests and documentation.

https://github.com/visionmedia/express-resource/pull/66

joscha commented 11 years ago

What is the status on this? Is express-resource discontinued?

tj commented 11 years ago

it's not discontinued, im just not a huge fan of this sort of API

joscha commented 11 years ago

I can understand that - maybe close the pull then, so people won't wait for it? I went with the app.all and app.get routes - seems to work like a charm so far.