Skip to content

Commit

Permalink
Make sure to include querystring when redirecting. Make the AnyConver…
Browse files Browse the repository at this point in the history 
…ter case-aware, add CasingError to help with this.
  • Loading branch information
@ckknight
ckknight committed Apr 10, 2011
1 parent f4e40df commit cfc1826
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 15 deletions.
44 changes: 37 additions & 7 deletions lib/escort.js
View file
Expand Up @@ -790,10 +790,12 @@ var calculateConverterArgs, generateUrlFunction;
try {
var url = request.url;
var questionIndex = url.indexOf("?");
var querystring = "";
if (questionIndex !== -1) {
// if there is a querystring, we'll just chop that part off
// it's still preserved in request.url, and should be handled
// by other middleware.
querystring = url.substring(questionIndex);
url = url.substring(0, questionIndex);
}

Expand All @@ -812,7 +814,7 @@ var calculateConverterArgs, generateUrlFunction;
var staticRoute = staticRoutes[lowerUrl];
if (staticRoute) {
if (staticRoute.route !== url) {
movedPermanently(response, staticRoute.route);
movedPermanently(response, staticRoute.route + querystring);
return;
}

Expand All @@ -839,7 +841,7 @@ var calculateConverterArgs, generateUrlFunction;

var cachedResult = dynamicRedirectCache[url];
if (cachedResult) {
movedPermanently(response, cachedResult);
movedPermanently(response, cachedResult + querystring);
return;
} else {
cachedResult = dynamicRouteCache[url];
Expand Down Expand Up @@ -886,8 +888,6 @@ var calculateConverterArgs, generateUrlFunction;
params = {};
for (var k = 0, lenK = argumentNames.length; k < lenK; k += 1) {
var unconvertedValue = match[k * 2 + 2];
properUrl += unconvertedValue;
properUrl += literals[k + 1];
var convertedValue;
try {
convertedValue = converters[k](unconvertedValue);
Expand All @@ -897,10 +897,15 @@ var calculateConverterArgs, generateUrlFunction;
// thus, we need to keep searching for the correct route.
callback = undefined;
continue searchWithinPrefix;
} else if (ex instanceof escort.CasingError) {
unconvertedValue = ex.expectedCasing;
convertedValue = converters[k](unconvertedValue); // guaranteed not to error
} else {
throw ex;
}
}
properUrl += unconvertedValue;
properUrl += literals[k + 1];
params[argumentNames[k]] = convertedValue;
}
// params needs to be frozen since it will be potentially used in later requests through the dynamic route caching system
Expand All @@ -911,7 +916,7 @@ var calculateConverterArgs, generateUrlFunction;
};
if (url !== properUrl) {
dynamicRedirectCache[url] = properUrl;
movedPermanently(response, properUrl);
movedPermanently(response, properUrl + querystring);
return;
}
// we're done, so we need to break out of the outer for loop
Expand All @@ -933,7 +938,7 @@ var calculateConverterArgs, generateUrlFunction;
}

if (endsInSlash) {
movedPermanently(response, url);
movedPermanently(response, url + querystring);
return;
}

Expand Down Expand Up @@ -985,9 +990,20 @@ var calculateConverterArgs, generateUrlFunction;
* friendly object.
*/
var ValidationError = function () {};
ValidationError.prototype = new Error();
ValidationError.prototype = Object.create(Error.prototype);
escort.ValidationError = ValidationError;

/**
* CasingError is thrown when converting from a URL to specify that the route
* matches except that the case of the value should be different from what is
* specified.
*/
var CasingError = function (expectedCasing) {
this.expectedCasing = expectedCasing;
};
CasingError.prototype = Object.create(Error.prototype);
escort.CasingError = CasingError;

/**
* BaseConverter is the base of all the internal converters provided.
* When specifying one's own converters, it is not necessary to inherit from this.
Expand Down Expand Up @@ -1154,12 +1170,26 @@ var calculateConverterArgs, generateUrlFunction;
throw new Error("Must specify at least one argument to AnyConverter");
}

var values = {};
for (var i = 0, len = args.length; i < len; i += 1) {
var arg = args[i];
values[arg.toLowerCase()] = arg;
}

return spawn(AnyConverter.prototype, {
_values: values,
regex: "(?:" + args.map(regexpEscape).join("|") + ")",
weight: 200
});
};
AnyConverter.prototype = Object.create(BaseConverter.prototype);
AnyConverter.prototype.fromUrl = function (value) {
var result = this._values[value.toLowerCase()];
if (result !== value) {
throw new CasingError(result);
}
return result;
};
escort.converters.any = escort.AnyConverter = AnyConverter;
}());

Expand Down
41 changes: 33 additions & 8 deletions test/escort.test.js
View file
Expand Up @@ -1075,7 +1075,7 @@ module.exports = {

assert.response(app,
{ url: "/thing/", method: "GET" },
{ statusCode: 301, header: { Location: "/thing" } });
{ statusCode: 301, headers: { Location: "/thing" } });
},
"retrieving a known URL with a slash should return a MovedPermanently and preserve querystring": function () {
var app = connect(
Expand All @@ -1088,7 +1088,7 @@ module.exports = {

assert.response(app,
{ url: "/thing/?hello=there", method: "GET" },
{ statusCode: 301, header: { Location: "/thing?hello=there" } });
{ statusCode: 301, headers: { Location: "/thing?hello=there" } });
},
"retrieving an unknown URL with a slash should return a NotFound": function () {
var app = connect(
Expand Down Expand Up @@ -1118,11 +1118,11 @@ module.exports = {

assert.response(app,
{ url: "/thing", method: "GET" },
{ statusCode: 301, header: { Location: "/Thing" } });
{ statusCode: 301, headers: { Location: "/Thing" } });

assert.response(app,
{ url: "/THING", method: "GET" },
{ statusCode: 301, header: { Location: "/Thing" } });
{ statusCode: 301, headers: { Location: "/Thing" } });
},
"redirect on case difference (dynamic)": function () {
var app = connect(
Expand All @@ -1144,23 +1144,48 @@ module.exports = {

assert.response(app,
{ url: "/thing/" + name, method: "GET" },
{ statusCode: 301, header: { Location: "/Thing/" + name } });
{ statusCode: 301, headers: { Location: "/Thing/" + name } });

assert.response(app,
{ url: "/THING/" + name, method: "GET" },
{ statusCode: 301, header: { Location: "/Thing/" + name } });
{ statusCode: 301, headers: { Location: "/Thing/" + name } });

assert.response(app,
{ url: "/Thing/" + name + "/Blah", method: "GET" },
{ statusCode: 200, body: "GET /Thing/" + name + "/Blah" });

assert.response(app,
{ url: "/thing/" + name + "/blah", method: "GET" },
{ statusCode: 301, header: { Location: "/Thing/" + name + "/Blah" } });
{ statusCode: 301, headers: { Location: "/Thing/" + name + "/Blah" } });

assert.response(app,
{ url: "/THING/" + name + "/BLAH", method: "GET" },
{ statusCode: 301, header: { Location: "/Thing/" + name + "/Blah" } });
{ statusCode: 301, headers: { Location: "/Thing/" + name + "/Blah" } });
});
},
"any converter case sensitivity": function () {
var app = connect(
escort(function (routes) {
routes.get("post", "/posts/{id:any('Alpha', 'Bravo', 'Charlie')}", function (req, res, params) {
assert.strictEqual("string", typeof params.id);

res.end("GET /posts/" + params.id);
});
})
);

["Alpha", "Bravo", "Charlie"].forEach(function (name) {
assert.response(app,
{ url: "/posts/" + name, method: "GET" },
{ body: "GET /posts/" + name });

assert.response(app,
{ url: "/posts/" + name.toLowerCase(), method: "GET" },
{ statusCode: 301, headers: { Location: "/posts/" + name } });

assert.response(app,
{ url: "/posts/" + name.toUpperCase(), method: "GET" },
{ statusCode: 301, headers: { Location: "/posts/" + name } });
});
}
};

0 comments on commit cfc1826

Please sign in to comment.