Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(URI template): allow complex vars coexist with simple var #647

Merged

Conversation

philiptzou
Copy link
Contributor

Allow situation like this:

api.add_route('/files/{file_id}', resource_1)
api.add_route('/files/{file_id}.{ext}', resource_2)

Which ValueError was raised due to only one var node was allowed whether
other nodes are complex or not. This fix changes the behaviour so now it
only applys to simple vars.

See #564

@philiptzou
Copy link
Contributor Author

So {file_id} is a simple var, but {file_id}.{ext} is a complex var (which is converted to compiled regexp).

@philiptzou philiptzou changed the title bug(URI template): allow complex vars coexist with simple var fix(URI template): allow complex vars coexist with simple var Nov 4, 2015
@philiptzou philiptzou force-pushed the pull-request-conflicting-complex-var branch from e6e2d5a to 336f7a1 Compare November 4, 2015 00:08
@yohanboniface
Copy link
Contributor

Humm, actually, two remarks:

  • with this patch, we can register /compare/{file_id}/ then /compare/{file_id}.{ext}/, but not the other way around; given that nodes are then reordered, I'm not sure the order we register the routes should have importance, but I may be wrong
  • we can now register /compare/{file_id}.{ext}/ then /compare/{file_name}.{ext}/which should conflict in reality IMHO

@yohanboniface
Copy link
Contributor

we can now register /compare/{file_id}.{ext}/ then /compare/{file_name}.{ext}/ which should conflict in reality IMHO

This was already the case before sorry. Is that on purpose, @kgriffs?

@yohanboniface
Copy link
Contributor

OK, complex means also {usr0}:{branch0}...{usr1}:{branch1} vs {usr0}:{branch0}, so it seems we don't want to enter in trying to detect complex nodes conflicts (and thus order they are registered is important).

@philiptzou
Copy link
Contributor Author

@yohanboniface Good point on the complex nodes conflicts! 👍 I think I have a simple way to detect such conflicts before we feed them to the compiled router. But perhaps in another pull-request.

nodes = sorted(nodes, key=lambda node: node.is_var)
# NOTE(kgriffs & philiptzou): Sort nodes in this sequence:
# static nodes(0), complex var nodes(1) and simple var nodes(2).
# so that none of them get masked.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yohanboniface The registering order is not important since we'll sort the nodes inside our code. That's here. Before this case, static nodes (for example /foo/bar) are always sorted before var nodes (/foo/{var}).

So put '/compare/{file_id}' after/before '/compare/{file_id}.{ext}' are both acceptable and the results are the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

humm, but then this should work equally, right?

In [1]: from falcon.routing import DefaultRouter; router = DefaultRouter()

In [2]: router.add_route('/files/{file_id}', {}, 'xxxx2')

In [3]: router.add_route('/files/{file_id}.{ext}', {}, 'xxxx2')
In [1]: from falcon.routing import DefaultRouter; router = DefaultRouter()

In [2]: router.add_route('/files/{file_id}.{ext}', {}, 'xxxx2')

In [3]: router.add_route('/files/{file_id}', {}, 'xxxx2')
ValueError: The URI template for this route conflicts with another route's template.

@philiptzou
Copy link
Contributor Author

Btw, I think we can simply detect conflicts amongst complex nodes like this:

import re

def is_conflict(complex_nodes):
    pseudo_segments = [re.sub(r'({[-_a-zA-Z0-9]+})', '{v}', n.raw_segment)
                       for n in complex_nodes]
    return len(set(pseudo_segments)) < len(pseudo_segments)

Perhaps another pull-request, @kgriffs? (Update see below)

@philiptzou
Copy link
Contributor Author

@yohanboniface Good catch on the collision problem. See if the new revision I just submitted fix the issue you encountered.

@yohanboniface
Copy link
Contributor

@philiptzou excellent! Your last commit fixes both collision issue and order issue :)

@kgriffs LGTM

@yohanboniface
Copy link
Contributor

(@philiptzou your should rebase the branch on master, github says there are conflicts ;) )

@philiptzou
Copy link
Contributor Author

@yohanboniface Well, I'll rebase after @kgriffs reviewed this. I have a feeling that he will find something too.

Have done the rebasing.

@philiptzou philiptzou force-pushed the pull-request-conflicting-complex-var branch from a4278d3 to b98715c Compare November 24, 2015 04:42
@kgriffs
Copy link
Member

kgriffs commented Dec 21, 2015

@philiptzou Nice work, LGTM! Could you please rebase once more and then I think this is ready to merge.

Allow situation like this:

```python
api.add_route('/files/{file_id}', resource_1)
api.add_route('/files/{file_id}.{ext}', resource_2)
```

Which ValueError was raised due to only one var node was allowed whether
other nodes are complex or not. This fix changes the behaviour so now it
only applys to simple vars.

See falconry#564
@philiptzou philiptzou force-pushed the pull-request-conflicting-complex-var branch from b98715c to 32c8f22 Compare December 21, 2015 21:57
@philiptzou
Copy link
Contributor Author

@kgriffs Done :)

@kgriffs
Copy link
Member

kgriffs commented Dec 21, 2015

@philiptzou Thanks! And thanks @yohanboniface for reviewing!

kgriffs added a commit that referenced this pull request Dec 21, 2015
…lex-var

fix(URI template): allow complex vars coexist with simple var
@kgriffs kgriffs merged commit a8154de into falconry:master Dec 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants