Open CaselIT opened 2 years ago
One unambiguous way slightly extending your prototype could be still supporting only one field of type path
, but it could be placed anywhere in the template. Like /{foo:uuid}/{bar:path}/baz/{peb}/{cak}
.
I personally think this should be a follow on PR, since it's not trivial to support a case like your example
Yes, absolutely, I meant splitting or simplifying this issue, not by further complicating #1945.
Oh ok, I misunderstood.
I don't feel supporting two or more multiple segment converter is that much harder than supporting tailing segments.
In any case, even if we decide that the fist (or last) one "wins" I'm currently of the option that we should not allow them since it may be confusing on the users. I guess the main issue is the that path
allows everything, if we do end up having a repath
that would probably make more sense?
Still I agree with you that we could go one step at the time, and have fist a version that does not support multiple multi-segment converters
copying from the PR:
@kgriffs
There is a decision to take here: should a "consume multiple segments" converter be allowed to match 0 segments? the current implementation allows it.
Personally I would expect it to never match if there is no path to consume, because otherwise what's the point of using the
path
converter? Or at least, if there is another route registered for the exact path prefix, that one would take precedence over the route containing thepath
converter.One could argue it my be useful to be able to handle
/foo/bar/bang
and /foo/bang` within the same resource controller, but that doesn't feel like a good API design to me (though it may be useful for re-implementing a legacy API?). So perhaps we could introduce an option to allow 0 segments but it is disabled by default? Or we introduce that along with #2061 ?
@vytas7 If I understand the problem right, IMHO, for the sake of consistency, the new converter should behave like fields behave now. They do match an empty segment after a trailing slash, and they do not otherwise.
Given the following test app,
import falcon class Resource: def on_get(self, req, resp, resourceid): resp.media = {'path': req.path, 'resourceid': resourceid} (app := falcon.App()).add_route('/api/{resourceid}', Resource())
Cf
$ xh http://localhost:8000/api/ HTTP/1.1 200 OK Content-Length: 35 Content-Type: application/json { "path": "/api/", "resourceid": "" }
vs
$ xh http://localhost:8000/api HTTP/1.1 404 Not Found Content-Length: 26 Content-Type: application/json Vary: Accept { "title": "404 Not Found" }
@CaselIT Also note that my question was only once we add support for additional parts after the path. Basically the question what if /foo/{bar:path}/baz should match /foo/baz or not.
@vytas7 Aha, right, I didn't realize the question was only about when we lift the requirement for a
path
segment to be last. Then maybe we don't need to make a decision right now?In any case, I would perceive routing as strange and confusing if
/foo/{bar:path}/baz
matched/foo/baz
, it wouldn't be consistent with the current behaviour where a field essentially needs a preceding/
to match, and in this case thepath
field would rob/baz
of that slash. So my vote is on "it shouldn't match".
Aha, right, I didn't realize the question was only about when we lift the requirement for a
path
segment to be last. Then maybe we don't need to make a decision right now?In any case, I would perceive routing as strange and confusing if
/foo/{bar:path}/baz
matched/foo/baz
, it wouldn't be consistent with the current behaviour where a field essentially needs a preceding/
to match, and in this case thepath
field would rob/baz
of that slash. So my vote is on "it shouldn't match".
The issue with shouldn't match is that /foo//baz
should by the rule above, and it seems strange.
/foo//baz
theoretically should match, yes, but IIRC we explicitly disallow two slashes in a row.
note that this is the current behaviour:
/foo/{bar}/baz
matches /foo//baz
but not /foo/baz
Aha, it does... that's maybe oversight too, we do disallow a double slash in the templates at least, but maybe not when matching...
Well, in either case we should follow the current behaviour, and IMHO /foo/{bar:converter}/baz
SHOULD NOT match /foo/baz
REGARDLESS of the converter
's type. But just IMHO :slightly_smiling_face:
I guess that in that sense we should keep this behavior, so the code above would not work. We probably need to check that remaining_path_len
has at lest 1+ other segments, so that there is always one segment for the path
converter
yes, replacing remaining_path_len > 1:
with remaining_path_len > 2:
and remaining_path_len > 0
with remaining_path_len > 1
produce the appropriate behaviour
Well, in either case we should follow the current behaviour, and IMHO
/foo/{bar:converter}/baz
SHOULD NOT match/foo/baz
REGARDLESS of theconverter
's type. But just IMHO 🙂
I tend to agree, considering it's the current behaviour of the framework and changing that would entail quite a bit more work, since it's not easy to communicate. It may be unfortunate that //
does produce different result, but it's still fine for me.
Should we document it?
The initial version of the converters that support matching multiple paths added in #1945 prevents adding additional segments after the path, meaning that something like
/foo/{x:path}/bar
is not supported.Adding support to it is not trivial. A prototype find would be something like this: (edit: see reply https://github.com/falconry/falcon/issues/2061#issuecomment-1122812436 regarding how to prevent empty matches)
running the above yields
A decision to take would be what to do regarding supporting multiple "consume multiple segments" in a single url. Like what would
/foo/{bar:path}/{baz:path}
match in case of/foo/bar/baz
? I think the issue would be there also if they two converter are separated by a literal segment or normal converted, since this template/foo/{bar:path}/x/{baz:path}
would still be ambiguous for a route like/foo/bar/x/y/x/other/part
ref: https://github.com/falconry/falcon/pull/1945#issuecomment-1120176732