Query based routing. #39
Conversation
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
==========================================
+ Coverage 54.86% 58.19% +3.32%
==========================================
Files 7 7
Lines 113 122 +9
==========================================
+ Hits 62 71 +9
Misses 51 51
Continue to review full report at Codecov.
|
src/routing.jl
Outdated
| function matchquery(q, req) | ||
| qdict = parsequerystring(req[:query]) | ||
| length(q) != length(qdict) && return false | ||
| for pair in q |
There was a problem hiding this comment.
for (key, value) in ... might be nicer here
|
A good idea and a good pr! Just a couple comments to look at. |
src/routing.jl
Outdated
| @@ -19,6 +19,7 @@ function matchpath(target, path) | |||
| params = d() | |||
| for i = 1:length(target) | |||
| if startswith(target[i], ":") | |||
| params = d() | |||
|
Fixed the problems that you pointed out, thank you. Is there a problem if it didn't pass the Travis CI build for Julia-0.5 and 0.4 version ? |
|
@MikeInnes This will help me to prove my understanding of the Mux project for the GSoC. So could you please review the updated (fixed the problems that you pointed) PR soon? Also, Is it OK if it doesn't pass the Travis CI build for Julia 0.5 and 0.4 version ? |
fix deprecations
no other IO types do the latter
| # Query routing | ||
|
|
||
| function matchquery(q, req) | ||
| qdict = parsequerystring(req[:query]) |
There was a problem hiding this comment.
I'm trying to see if I can rebase this PR on top of master, but this one call parsequerystring doesn't exist, and I'm having trouble locating it in the git history. Does anyone know what this function is supposed to do? @sarvjeetsinghghotra @MikeInnes
There was a problem hiding this comment.
|
So this PR is going to require adding a dependency on HttpCommon; are we okay with that? It needs URIParser and Nullables. If so, we should just be able to rebase this on top of Mux master (not sure if that's what Avik's second commit is doing or if it's just a merge) and then add in the necessary REQUIRE lines and |
|
I think functionality like parsequerystring belong in an optional utilty package like HttpCommon. Moving "everything" to Http.jl is a way to facilitate making some larger structure changes which were necessary. But this will be hard to maintain in one package over time since too few people know everything. That said, have you checked if similar functionality now exists in Http.jl? |
|
No, we cannot take dependency on HTTPCommon, that package is effectively deprecated. There should be equivalent functionality in HTTP.jl To @hustf's point, that may be the case eventually, but for now everything in HTTP.jl is simpler to maintain. |
Query based routing (continuation of #39)
|
continued in #81 . Thanks @sarvjeetsinghghotra for getting this started. |
Added a
queryfunction to allow query based routing. It takes adict(String, String)as input for target query.Following assumptions are made:
requestquery and target query should be exactly same i.e both should have same key and value pairs.dict("one" => "1", "two" => "")in this case it matches for any value oftwobuttwokey must be there inrequestwith some value. (Not sure about this case so need some inputs)