Skip to content
This repository has been archived by the owner on Dec 11, 2020. It is now read-only.

Support for email address ( resource with .) in resource uri #20

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support for email address ( resource with .) in resource uri #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41e6879
replacing ngx.var.request_uri with ngx.var.uri
Apr 6, 2016
76c352e
adding support for dot(.) in the uri resource .e.g /[email protected]/
Apr 6, 2016
4cc691c
updated test cases
Apr 6, 2016
f4722e0
updated test cases
Apr 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ http {

local ok, errmsg = r:execute(
ngx.var.request_method,
ngx.var.request_uri,
ngx.var.uri,
ngx.req.get_uri_args(), -- all these parameters
ngx.req.get_post_args(), -- will be merged in order
{other_arg = 1}) -- into a single "params" table
Expand Down
23 changes: 15 additions & 8 deletions router.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,25 @@ local router = {
local COLON_BYTE = string.byte(':', 1)
local WILDCARD_BYTE = string.byte('*', 1)
local HTTP_METHODS = {'get', 'post', 'put', 'patch', 'delete', 'trace', 'connect', 'options', 'head'}
local STR_STR_MATCH_SLASH = "[^/.]+"
local STR_STR_SPLIT_SLASH = "([^/]+)(.*)"
local STR_LEAF = "LEAF"
local STR_TABLE = "table"
local STR_STRING = "string"
local STR_ALL_METHODS = "get post put patch delete trace connect options head"
local STR_ANY = "any"

local function match_one_path(node, path, f)
for token in path:gmatch("[^/.]+") do
for token in path:gmatch(STR_STR_MATCH_SLASH) do
node[token] = node[token] or {}
node = node[token]
end
node["LEAF"] = f
node[STR_LEAF] = f
end

local function resolve(path, node, params)
local _, _, current_token, rest = path:find("([^/.]+)(.*)")
if not current_token then return node["LEAF"], params end
local _, _, current_token, rest = path:find(STR_STR_SPLIT_SLASH)
if not current_token then return node[STR_LEAF], params end

for child_token, child_node in pairs(node) do
if child_token == current_token then
Expand All @@ -65,15 +72,15 @@ local function resolve(path, node, params)
elseif child_token:byte(1) == WILDCARD_BYTE then -- it's a *
local param_name = child_token:sub(2)
params[param_name] = current_token .. rest
return node[child_token]["LEAF"], params
return node[child_token][STR_LEAF], params
end
end

return false
end

local function merge(destination, origin, visited)
if type(origin) ~= 'table' then return origin end
if type(origin) ~= STR_TABLE then return origin end
if visited[origin] then return visited[origin] end
if destination == nil then destination = {} end

Expand Down Expand Up @@ -114,7 +121,7 @@ function Router:execute(method, path, ...)
end

function Router:match(method, path, fun)
if type(method) == 'string' then -- always make the method to table.
if type(method) == STR_STRING then -- always make the method to table.
method = {[method] = {[path] = fun}}
end
for m, routes in pairs(method) do
Expand All @@ -131,7 +138,7 @@ for _,method in ipairs(HTTP_METHODS) do
end -- end
end

Router['any'] = function(self, path, f) -- match any method
Router[STR_ANY] = function(self, path, f) -- match any method
for _,method in ipairs(HTTP_METHODS) do
self:match(method:upper(), path, function(params) return f(params, method) end)
end
Expand Down
23 changes: 4 additions & 19 deletions spec/router_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,6 @@ describe("Router", function()
assert.same(dummy.params, {id="2"})
end)

it("supports an extension on a param", function()
r:match("GET", "/foo/:id.json", write_dummy)

r:execute("GET", "/foo/1.json")
assert.same(dummy.params, {id="1"})
end)
end)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we definitely want to support those

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding support for dot (.) conflict with your requirement of supporting extension for the resource.

e.g you have a test case /resource/1.json where you expect resource value to be 1
By adding support for ., it will return 1.json
e.g /users/[email protected] should return [email protected]
So unless we remove support for extension, dot support will not work.

Regarding defining constants, I had ran this test long ago and most of the time JIT does optimize but with millions of requests, I was able to see the difference. I do agree performance gain may not be significant compared to readability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a test testing support for the .. I just see removing support for partial matching.

The tests are testing that /:id.json extracts 1. I don't see how that could conflict with /:email matching the whole string. It might be not easy because of how it is implemented, but that does not mean it is not possible.

I can't merge such breaking functionality, sorry. I'd be fine merging it, if it would not break current tests and remove existing functionality.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will submit another PR with

  1. email support
  2. remove const strings


describe('when first param is a table', function()
Expand Down Expand Up @@ -83,15 +77,6 @@ describe("Router", function()
assert.same(dummy.params, {id="2"})
end)

it("supports an extension on a param", function()
r:match({
GET = {
["/foo/:id.json"] = write_dummy
}
})
r:execute("GET", "/foo/1.json")
assert.same(dummy.params, {id="1"})
end)
end)
end)

Expand Down Expand Up @@ -139,13 +124,13 @@ describe("Router", function()
it("gets url params with an extension", function()
local f, params = r:resolve("GET", "/s/21.json")
assert.equals(type(f), 'function')
assert.same(params, {id = "21"})
assert.same(params, {id = "21.json"})
end)

it("posts url params with an extension", function()
local f, params = r:resolve("POST", "/s/21.json")
assert.equals(type(f), 'function')
assert.same(params, {id = "21"})
assert.same(params, {id = "21.json"})
end)

it("gets with backtracking over url params", function()
Expand Down Expand Up @@ -194,7 +179,7 @@ describe("Router", function()

it("runs the specified function with a url param with an extension", function()
r:execute("GET", "/s/21.json")
assert.same(dummy.params, {id = '21'})
assert.same(dummy.params, {id = '21.json'})
end)

it("runs the specified function with a url param in a post", function()
Expand All @@ -204,7 +189,7 @@ describe("Router", function()

it("runs the specified function with a url param in a post with an extension", function()
r:execute("POST", "/s/21.json")
assert.same(dummy.params, {id = '21'})
assert.same(dummy.params, {id = '21.json'})
end)

describe('when given extra parameters', function()
Expand Down