Ticket #2532941 (closed defect)

Reporter


mark.kharitonov
Opened: 11/7/12
Last modified: 11/18/12
Status: closed
Type: defect
Resolution: expired

Owner


Eric Ferraiuolo
Target Release:
Priority: P3 (normal)
Summary: URL parameters are not decoded and not visible to middleware registered at *
Description:

Please, refer to this thread for more information - http://yuilibrary.com/forum/viewtopic.php?f=92&t=10883

In short:
- middleware registered at '*' does not see any url parameters.
- middleware registered at the exact path sees the url parameters, but they are not decoded.

On the contrary no problem exists with the query parameters

You can check out a trivial example here - http://23.22.204.49/a.html#/customer/URL%20Param%20With%20Spaces?name=Query%20Param%20With%20Spaces

Type: defect Observed in Version: 3.7.3
Component: App Framework Severity: S3 (normal)
Assigned To: Eric Ferraiuolo Target Release:
Location: Priority: P3 (normal)
Tags: Relates To:
Browsers: N/A
URL: http://23.22.204.49/a.html#/customer/URL%20Param%20With%20Spaces?name=Query%20Param%20With%20Spaces
Test Information:

Change History

Eric Ferraiuolo

YUI Developer

  • Username: ericf
  • GitHub: ericf
Posted: 11/8/12
  • priority changed to P3 (normal)
  • resolution changed to invalid
  • status changed from new to closed

The captured params are specific to the individual routes, e.g.

app.route(/foo/:foo', function (req) {
Y.log(req.params.foo);
});

app.route(/foo/:bar', function (req) {
Y.log(req.params.bar);
});

app.navigate('/foo/bla');

// => "bla"
// => "bla"

mark.kharitonov

  • Username: mark.kharitonov
Posted: 11/8/12
  • resolution changed from invalid
  • status changed from closed to reopened

I am sorry, but this is your implementation detail.

A handler for the route '*' should have the information of the actual concrete route and the route's URL parameters should be visible to it. After all, query parameters are!

And besides, what about the URL parameters not being decoded, even at the concrete route level?

Eric Ferraiuolo

YUI Developer

  • Username: ericf
  • GitHub: ericf
Posted: 11/8/12
  • status changed from reopened to infoneeded

It's not an implementation detail, it's an explicit design decision.

Describe to me how you'd handle the following:

app.route('*', function (req, res, next) {
// What should `req.params` be?
next();
});

app.route('/:foo/' ...);
app.route('/users/:foo/', ...);

app.navigate('/users/123/');

In the `"*"` route, should `req.params.foo` be "users" or "123"?

The parameters not being decoded is a separate issue, could you please file a separate bug for it?

mark.kharitonov

  • Username: mark.kharitonov
Posted: 11/8/12
  • status changed from infoneeded to assigned

Imagine a different implementation for the '*' route.
Suppose that all the middleware from '*' is inserted at each and every route.
Then


app.route('*', function (req, res, next) {
// What should `req.params` be?
next();
});
app.route('/:foo/' ...);
app.route('/users/:foo/', ...);

becomes

app.route('*', function (req, res, next) {
// What should `req.params` be?
next();
});
app.route('/:foo/', [function (req, res, next) {
// What should `req.params` be?
next();
}, ...]);
app.route('/users/:foo/', [function (req, res, next) {
// What should `req.params` be?
next();
}, ...]);

This transformation is semantically correct, because the invariant that all the middleware registered for '*' executes the first is preserved. Of course, the imaginary insertion occurs only for the routes following the '*' route.

Anyway, in this case the answer for your question is trivial. /users/123 navigates to the last route and hence req.params.foo is "123".

The only problem is when the given route is not covered by any concrete route, in which case req.params should be empty as today.

If you agree with me that the presented imaginary transformation is a legitimate implementation, then the existing limitation is the result of the current implementation details. It does not have to be this way.

Eric Ferraiuolo

YUI Developer

  • Username: ericf
  • GitHub: ericf
Posted: 11/9/12
  • status changed from assigned to accepted

I had a typo in my example, I meant:


app.route('/:foo/*' ...);
app.route('/users/:foo/', ...);
app.navigate('/users/123/');

In any case, this would a drastic changes from the way router operates, and is not something I'd like to change to. If you'd like middleware to run for every route and be context-aware of that route's specific parameter mapping, then I'd suggest doing the following:


var router = new Y.Router();

router.upperCaseParams = function (req, res, next) {
Y.Object.each(req.params, function(param, name, params) {
params[name] = param.toUpperCase();
});

next();
};

router.logFoo = function (req, res, next) {
Y.log(req.params.foo);
next();
});

router.route('/:foo/*', 'upperCaseParams', 'logFoo');
router.route('/users/:foo', 'uppserCaseParams', 'logFoo');

router.save('/users/ericf');
// => "USERS"
// => "ERICF"

Eric Ferraiuolo

YUI Developer

  • Username: ericf
  • GitHub: ericf
Posted: 11/9/12
  • status changed from accepted to infoneeded

For the other issue of decoding the URL-encoded path segments for `req.params`. How does this look? Give it a try and let me know if it fixes your issue:
https://github.com/yui/yui3/pull/335

yuibuild

  • Username: yuibuild
Posted: 11/18/12
  • resolution changed to expired
  • status changed from infoneeded to closed

Ticket automatically closed due to no activity.