Allow Passing in Route Middleware on Resource #8

Open
mhemesath opened this Issue Apr 15, 2011 · 31 comments

Projects

None yet
@mhemesath

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
Member
tj commented Apr 15, 2011

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

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
Member
tj commented Apr 15, 2011

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

@mhemesath

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
emostar commented Jul 27, 2011

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

app.resource

Use

app.(get|post|etc) ?

@flockonus

+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
Member
tj commented Aug 10, 2011

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

@flockonus

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

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

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

@nicholaswyoung

Any updates on this issue?

@tj
Member
tj commented Dec 18, 2011

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

@nicholaswyoung

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
Member
tj commented Dec 18, 2011

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
Member
tj commented Dec 18, 2011

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

@nicholaswyoung

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

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

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.

@nicholaswyoung

@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

Just issued a pull request for this:

#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

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

#45

@flockonus

The way of use looks really fine, definitely a improvement

@tj
Member
tj commented Jan 3, 2012

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
Member
tj commented Jan 4, 2012

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

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
Member
tj commented Jan 4, 2012

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

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
panta commented Sep 25, 2012

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

#66

@joscha
joscha commented Dec 6, 2012

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

@tj
Member
tj commented Dec 12, 2012

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

@joscha
joscha commented Dec 12, 2012

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment