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

adding support for reading args without preceding / e.g /hello?a=b #14

Closed
wants to merge 3 commits into from
Closed

Conversation

rohitjoshi
Copy link

This PR fixes issue when query parameters are passed without preceding /
e.g below request uri doesn't match to ["/hello"]

GET  /hello?name=rjoshi  

will not match to

 r:match({
   GET = {
      ["/hello"]       = function(params) ngx.print("someone said hello " .. params.name) end
    }

With this PR, it will work for below both

GET /hello?name=rjoshi 
GET /hello/?name=rjoshi

@lloydzhou
Copy link
Contributor

@rohitjoshi
just rtrim the URI using "/", before call execute method can doing this feature.

@rohitjoshi
Copy link
Author

@lloydzhou issue is not / at the end. If request URI has query parameters without /, router doesn't match the request. e.g GET /hello?name=value doesn't match to ["/hello"].
See same issue is reported by #10

@lloydzhou
Copy link
Contributor

ngx.var.request_uri    include query string
ngx.var.uri            do not include query string

@rohitjoshi
Copy link
Author

Ok, got it. you might want to update the example at https://github.com/APItools/router.lua/blob/master/README.md#usage-with-openresty

Few weeks ago, I had tried router.lua and gave up. I see other person have reported same issue.
#10

@lloydzhou
Copy link
Contributor

@rohitjoshi i'm not the owner of this project. you can create new pull request or ask @kikito to update the example.

@kikito
Copy link
Contributor

kikito commented Jan 7, 2016

Hi there, I am no longer part of 3Scale or APItools so I can't directly make these changes neither. Pinging @mikz in case he wants to update the example in the blog and close this.

@mikz
Copy link
Contributor

mikz commented Jan 7, 2016

Sorry. Totally missed this. Thanks @kikito for bringing it up.

@mikz mikz self-assigned this Jan 7, 2016
@mikz
Copy link
Contributor

mikz commented Apr 6, 2016

I'm all in for merging this. But to do so I'd need:

  • understand why a simple change in readme would not fix this
  • a test case
  • green build

Now it looks like it is a README issue, rather than issue in the code.

ytr289 added 2 commits April 6, 2016 09:14
@rohitjoshi rohitjoshi closed this Apr 6, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants