Feature/query parameter router matching

[Landsi]

I recently stumbled upon a Service API, where the routing was done exclusively with the query parameters.

While that is not that pretty in my opinion, I still needed to mock it for my unit tests.

This PR includes Router matching of query parameters including wildcard values.

Basically, this route registration is now possible:

router.get("/users?language=:lng") { request in
    let language = request.components["lng"]
    ...
}

// send request to '.../users?language=xx'

[devluckybot]

	✅ Congrats.
✅	Any changes to library code need a summary in the Changelog.

SwiftLint found issues

Warnings

File	Line	Reason
RouterTests.swift	1114	File should contain 1000 lines or less: currently contains 1114

Generated by 🚫 danger

[MP0w]

Ty Max! considering query params only when they are there might make sense but (didn't check yet the implementation) the order of query params is usually not guaranteed so if you have:
myapi.com?param=1&param2=2 I would expect to match it also when they are swapped ... but can be tricky and make the Router more complex and slow.
Might be easier to have some APIs like:

router.get(URL, {...}).when { (params, request) in
//check params 
  return true
}

That way the server can easily check if the URL matches and the condition is true

[Landsi]

you might be right, i thought it's just easier to add it to the route directly.

however, i just realized that adding wildcard functionality is superflous, as all query parameters are added to the request anyway ...

[MP0w]

Let's also wait for @joanromano opinion and @zzarcon but I think the order shouldn't matter otherwise is quite error prone. Let's decide how to tackle it before

[MP0w]

If now the hash is affected by query parameters it means that if there is an extra parameter it won't match, right?

[Landsi]

Not sure what you mean.. matching is done in matchRoute(...) here it means en extra query param will result in a different hash, so a separate Route. /user?param=1 will not replace /user, but be separate. (Where we run into prioritization issues ... which route to execute when calling www.myapi.com/user?param=1?

[codecov-io]

Current coverage is 98.87% (diff: 94.66%)

Merging #140 into master will decrease coverage by 0.70%

@@             master       #140   diff @@
==========================================
  Files            10         10          
  Lines           481        535    +54   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            479        529    +50   
- Misses            2          6     +4   
  Partials          0          0          

Powered by Codecov. Last update cb25642...bb53fa5

[zzarcon]

How about this? Forget about the syntax errors :)

router.get(URL) { request in
  if (request.components["relevantQueryParam"]) {
    return fakeResponse;
  } else {
    return request.continue();
  }
}

The point is to have a request.continue method, which is nothing but giving the user the ability to continue the request whenever he wants AKA the method implementation is just making the same request since we have all the info we need there.

I think this is quite powerful and doesn't require any kind of boilerplate + keeps the routing logic in the same place.

Thoughts? @Landsi @MP0w @joanromano

[joanromano]

@zzarcon this means that the Router needs to be able to recreate the exact same request that was done by the client (with success/error handling scenarios which is not aware of, for instance).

Probably I am wrong, but I am afraid that's not possible.

[zzarcon]

@joanromano exactly! that's what my suggestion is about, but yeah I have no idea if that is even possible :(

So, if I understood properly, the problem is that we don't all the info we need available in the Router, like the handlers for instance right? If that's the case my suggestion will not be possible as you said 😭

[MP0w]

Hey guys with the holidays in the middle we forgot this.
TBH I think that the use case is really minimal and doesn't worth the added complexity...
Unfortunately @Landsi has to deal with a 💩 APIs but I would be more for not supporting it as Kakapo should be able to intercept routes and query params are not part of a route. The definition of Route would change in case we add support for params

[MP0w]

As another option I would prefer the more generic approach:

router.get(url) {
   return ...
}.when { (request) in
 ....
}

which requires some changes (syntax TBD) but is more generic and could be used easily with some currying like stuff:

// your convenience func that support partial apply
func hasParams(_ params: String...) -> (Request) -> Bool {
    return { (request) in
        return params.contains(request)
    }
}
....
// the proposed addition
func when(_ condition: (Request) -> Bool) {

}

// usage
router.get(url) {
   return ...
}.when(hasParams("foo", "bar"))

IMHO it's quite clean, opinions? @devlucky/kakapo-core @Landsi

[joanromano]

@MP0w @Landsi as mentioned I have already an implementation with that approach, sorry I couldn't find the time to add the tests and push it. Will try to do it now

[MP0w]

@joanromano Hector proposal is actually really easy to implement, we should probably do it:

1. Create an URLSession without Kakapo's Server in the protocol classes
2. Use it synchronously to do the real request
3. Create a wrapper class that conform to CustomSerializable and in customSerialize it just perform the synchronous request, not affected by Kakapo and returns the response
4. add a method to Request "continue" that returns an instance of that wrapper class

[joanromano]

@MP0w I was actually wrong when I mentioned the success/error handling scenarios that are handled by the client, true.

[MP0w]

As we discussed f2f (weeks ago) appears we don't want to add this because:

1. add lot of complexity for marginal gain
2. route in general are never affected by query params (not only in kakapo)
3. Doesn't make much sense for testing since you can have different Routers in different tests, or reset the Server

Generic filtering

For more generic filtering (just closed 2 WIP PRs one from me and @joanromano )
it could be done but we would need to change the hashing to consider the filter closure...

Provide ways to perform original request

This was the proposed approach from hez which is doable but not sure if we should add it to the stdlib
How to do it is described in #140 (comment)

If we feel like we should do something and/or use one of the 3 approach, or someone else needs something similar we can of course consider doing something but for the moment I will just close.
Anyway thank @Landsi for the PR