feat(routing): add support for multiple path segment fields

[CaselIT]

Summary of Changes

This adds support for a converter to be declared as consuming all the remaining path.

The current implementation does not allow nesting these converters and is quite conservative (I think), but I believe we can improve on this later if we need to.

Related Issues

Closes #423
Fixes #648
Relates to #1895

Pull Request Checklist

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

• Applied changes to both WSGI and ASGI code paths and interfaces (where applicable).
• Added tests for changed code.
• Prefixed code comments with GitHub nick and an appropriate prefix.
• Coding style is consistent with the rest of the framework.
• Updated documentation for changed code.
  • Added docstrings for any new classes, functions, or modules.
  • Updated docstrings for any modifications to existing code.
  • Updated both WSGI and ASGI docs (where applicable).
  • Added references to new classes, functions, or modules to the relevant RST file under docs/.
  • Updated all relevant supporting documentation files under docs/.
  • A copyright notice is included at the top of any new modules (using your own name or the name of your organization).
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
• Changes (and possible deprecations) have towncrier news fragments under docs/_newsfragments/, with the file name format {issue_number}.{fragment_type}.rst. (Run towncrier --draft to ensure it renders correctly.)

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

PR template inspired by the attrs project.

[codecov]

Codecov Report

Merging #1945 (18fcc6a) into master (83682f6) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1945   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        63           
  Lines         6715      6752   +37     
  Branches      1243      1254   +11     
=========================================
+ Hits          6715      6752   +37     

Impacted Files	Coverage Δ
falcon/routing/__init__.py	100.00% <100.00%> (ø)
falcon/routing/compiled.py	100.00% <100.00%> (ø)
falcon/routing/converters.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83682f6...18fcc6a. Read the comment docs.

[CaselIT]

After #1990 is merged we should update the docs there to mention that for simple cases :path may be used

[vytas7]

@CaselIT Have you considered to buck the trend seen among other frameworks where this is packaged as a converter, and add new syntax to router templates instead?

I don't know what the most aesthetic way to express that could be, but the advantage would be that one could use any converter with a path segment, or even use no converter for simple cases?
Something like /route/{itemid}/repository/{url/} or /route/{itemid}/repository/{/url} maybe?
(And with converters /route/{itemid}/repository/{url/:relativeurl} or /route/{itemid}/repository/{/url:relativeurl}.)

(This is just a random idea though, maybe it makes sense to have a path converter after all.)

[CaselIT]

I'm not really sold on the idea, since it's another thing that should be documented, explained etc.

I think your example would work better with /route/{itemid}/repository/{url:relativepath}. path is not special, meaning that the way it indicates that it can captures more than one segment is available to other converters

[vytas7]

Agreed that my idea ain't very pretty either.
But functionality wise, it depends on the way you view it. Say that you have a useful converter that might accept path, but in another case you would want to restrict it to a single field, or just use it in another field than the very last one (suffix). I guess you could subclass it to override that flag, and then register the new type as well, but it won't be particularly pretty to document either.

[CaselIT]

Since the interface of the converters changes, I think it makes sense to have two implementation in that case. (path like converters get passed a list of path segments, not a single segment)

[vytas7]

Another issue that is useful to have in mind when discussing the big picture: #1567 (which would actually favour your current design, but maybe we could devise a more generic way to provide hints?)

[CaselIT]

From a quick look I don't think that the current implementation is incompatible with that issue

[vytas7]

I know that although Falcon has moved away from RFC 6570, the said paper has all kinds of funky expansions, so maybe something familiar could be adapted from Section 3.2.6, like {list*} ➡️ /red/green/blue.
(Or maybe nobody cares about RFC 6570 any longer?)

Maybe we could have a discussion [meeting?] about this on chat at some point 🤔 I don't have any elegant suggestions, but maybe someone else does.

[CaselIT]

That may be an option, but we risk having two ways of doing something, having both the converters and this special syntax.
Also what would happen if both a converter and * are used together? Like {list*:int}

[vytas7]

Dunno, I was just thinking what the options were before we move forward with this. But we should move forward in one or another way 🙂

[vytas7]

It would be also good to reason about this change in the perspective of adding re converter (#857). Requiring a CONSUME_PATH attribute on the converter class would force a decision to make re of one or another type, however, I can imagine both could be useful 🤔

Of course, there are different ways to accommodate this in the CONSUME_PATH design as well. One could add two different types, like re and repath, or just make path accept an optional regexp argument?

[vytas7]

Yes so, I think no converter should match 0 fields, because that would be a substantial change in comparison to how the framework behaves now. Matching an empty string after a trailing slash is a bit special edge case (or oversight) which already exists now, so path converter can also do the same, but /foo/{bar}/{baz:path} ought not to match /foo/bar.

[vytas7]

This is shaping up, well done 👍

A couple of inline nitpicks aside, we also ought to revise this FAQ item: How can I handle forward slashes within a route template field?

[CaselIT]

thanks for the review

but /foo/{bar}/{baz:path} ought not to match /foo/bar.

it does not at the moment.
without / in the last segment the behavior will be the same as if using /foo/{bar}/{baz} as template

[kgriffs]

vytas7 requested changes 20 hours ago

lol, this is why it is good to require >= 2 approvals. You guys inevitably catch things I miss.

[vytas7]

LGTM sans the still pending FAQ update: How can I handle forward slashes within a route template field?

[CaselIT]

LGTM sans the still pending FAQ update: How can I handle forward slashes within a route template field?

sorry I missed the previous message. Will do that

[vytas7]

LGTM 👍