Router improvements

[adam-fowler]

Capture paths caught by recursive wildcard

router.get("**") { request in
    request.parameters.getCatchAll()
}

GET /test/this will return test/this

Wildcards with both prefix and suffix

router.get("*.jpg") { request in
}
router.get("file.*") { request in
}

Capture with both prefix and suffix. The parameter name is surrounded by ":" instead of just being prefixed with a ":"

router.get(":image:.jpg") { request in
    request.parameters.get("image")
}
router.get("file.:ext:") { request in
    request.parameters.get("ext")
}

[codecov]

Codecov Report

Patch coverage: 93.90% and project coverage change: +0.23 🎉

Comparison is base (610117e) 83.63% compared to head (a379521) 83.86%.

❗ Current head a379521 differs from pull request most recent head c9aa172. Consider uploading reports for the commit c9aa172 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #179      +/-   ##
==========================================
+ Coverage   83.63%   83.86%   +0.23%     
==========================================
  Files          77       77              
  Lines        4277     4351      +74     
==========================================
+ Hits         3577     3649      +72     
- Misses        700      702       +2     

Impacted Files	Coverage Δ
Sources/Hummingbird/Router/Parameters.swift	65.15% <76.92%> (+2.88%)	⬆️
Sources/Hummingbird/Router/RouterPath.swift	94.80% <93.75%> (-0.94%)	⬇️
Sources/Hummingbird/Router/Router.swift	100.00% <100.00%> (ø)
Sources/Hummingbird/Router/TrieRouter.swift	100.00% <100.00%> (ø)
Sources/Hummingbird/Utils/FlatDictionary.swift	71.64% <100.00%> (+4.97%)	⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[github-actions]

Pull request benchmark comparison [ubuntu-latest] with 'main' run at 2023-03-20T14:02:15+00:00

[tib]

I've found an issue with the prefix / suffix capture.

router.get(":name:/:ext:") { req in
    req.parameters.get("name")! + "." + req.parameters.get("ext")!
}

GET /file.jpg => 404
Expected: 200, file.jpg

So we've got the following parameter cases:

• :foo -> Standard named parameter
• :foo: -> Named wildcard parameter
• * -> Wildcard (anything) parameter
• ** -> Catch all parameter

Honestly the colon thing makes my head a bit dizzy to think about parameters.

What if we use the following syntax, to make things a bit more unified:

• hello/[name] = hello/* -> hello + catch 1 component
• hello/[name].jpg = hello/*.jpg -> hello + catch 1 component with .jpg suffix
• hello/[name].[ext] = hello/*.* -> hello + catch 1 component (files only)
• */* -> catch only 2 path components -> catch 2 components
• ** -> catch every path component -> catch all components recursively
• hello/** -> catch all after the hello part -> hello + all components
• [name]/** -> name + catch all after name -> name param + all components

I'd prefer [] characters instead of colons, it makes paths more readable.
This way you could easily capture named params in [] and use wildcards (* + **).

What do you think? Does this make any sense? 🤔

[adam-fowler]

I assume you meant :name:.:ext:. Unfortunately you can't have prefix and suffix capture in one path component.

I agree on the double :. Maybe [] would work. My only concern is it is completely different from full path component capture which uses the single :. I can't really change that as it is a breaking change and also a standard for most router libs

[tib]

Yeah, so this won't work either: *.*, I just checked it.

What if we keep the :named version to follow the 'standards', but allow people the [named] syntax too and extend the capabilities to support capturing things within a single component? This could be a huge benefit and maybe others will follow this approach... 🤔

[tib]

Also one more thing:

router.get("**") { req -> String? in

    print(req.parameters.getAll(":**:")) // [String]
    print(req.parameters.getAll(
        // won't work internal protection level
        String(HBParameters.recursiveCaptureKey) 
    ))
    
    print(req.uri.path) // String
    print(req.parameters.getCatchAll()) // String?
    
    // req.parameters.getCatchAll() // [String]
    
    return req.parameters.getCatchAll()
}

The getCatchAll method returns an optional String, but that's not very helpful, I'd prefer to get back an array of strings (empty if nothing was catched). I can get more or less the same value using the req.uri.path property, and the only way I can access the params is the getAll(":**:") hack.

[tib]

Anyway, I don't mind if we can't have multiple named captures within a component, I only needed catchAll, so this is already more than enough. Thanks for your hard work. 🙏

[adam-fowler]

@tib I ended up using ${parameter} for parameter capture as there is prior art using this form in both shell and javascript.
You have your catchAll split into path components as well

[tib]

That's a very good idea! I'll take a look at the PR shortly and come back to you with my feedbacks. Many thanks again, this is going to be super useful. 🙏

[tib]

Everything looks really great, one minor suggestion:

public func getCatchAll() -> [String]

Could you return [String] type instead of a Substring array?

I suppose most of us will have to cast the Substring into a String, so it'd make sense...

[adam-fowler]

Everything looks really great, one minor suggestion:

public func getCatchAll() -> [String]

You can do pretty much everything that String does with Substring. Returning an array of String's means I have to allocate memory for each String as well as the array. I would rather have the option where only the memory for the array is allocated. If you need String you can always create one from the Substring

[tib]

Make sense, all right then. 👍