Skip to content
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

Allow Passing in Route Middleware on Resource #8

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

Allow Passing in Route Middleware on Resource #8

mhemesath opened this issue Apr 15, 2011 · 31 comments

Comments

@mhemesath
Copy link

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
Copy link
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
Copy link
Author

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
Copy link
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
Copy link
Author

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
Copy link

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
Copy link

+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
Copy link
Member

tj commented Aug 10, 2011

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

@flockonus
Copy link

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
Copy link

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
Copy link

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

@secretfader
Copy link

Any updates on this issue?

@tj
Copy link
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

@secretfader
Copy link

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
Copy link
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
Copy link
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

@secretfader
Copy link

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
Copy link

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
Copy link

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
Copy link

@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
Copy link

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
Copy link

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

#45

@flockonus
Copy link

The way of use looks really fine, definitely a improvement

@tj
Copy link
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
Copy link
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
Copy link

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
Copy link
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
Copy link

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
Copy link

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
Copy link

joscha commented Dec 6, 2012

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

@tj
Copy link
Member

tj commented Dec 12, 2012

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

@joscha
Copy link

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
Labels
None yet
Projects
None yet
Development

No branches or pull requests