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

allow requirements to be specified on Mapper.resource #57

Merged
merged 1 commit into from Jan 16, 2016
Merged

allow requirements to be specified on Mapper.resource #57

merged 1 commit into from Jan 16, 2016

Conversation

sdague
Copy link
Contributor

@sdague sdague commented Jan 11, 2016

In OpenStack Nova we've used the prefix_path on Mapper.resource to
specify additional variables we want to capture (specifically
{project_id}). Project_id is a uuid. When trying to restrict
project_id to only valid uuid format a couple of issues were exposed.

  • 1 '/{project_id:[a-f0-9]{32}}/...' builds an incorrect regex

    because of the nested {}

  • 2 the preferred method that works on connect() to pass

    requirements doesn't work here (requirements are reset to only an
    id match)

That leaves us with having to build a custom project_id match with 32
[a-f0-9] strings appended for every resource added to get the support
we need without effectively vendoring our own version of Mapper.resource.

This small change to allow requirements to pass through would make it
possible to get this tighter validation with much smaller regexes.

@bbangert
Copy link
Owner

I'm fine allowing the requirements to make it through, but this PR is missing

  • CHANGELOG.rst entry
  • Test to verify it works as intended
  • Docstring regarding the new option

If you could update it with those, I'd be happy to merge.

Thanks!

@sdague
Copy link
Contributor Author

sdague commented Jan 13, 2016

Thanks, updated with I think all the feedback you provided.

@bbangert
Copy link
Owner

Looks good, the only error is on an older Python where there is no assertIsNone apparently. Could just assertEqual to None for that.

@sdague
Copy link
Contributor Author

sdague commented Jan 13, 2016

Will update shortly with that fix.

    -Sean

Sean Dague
dague.net

On Jan 13, 2016, 10:41 AM, at 10:41 AM, Ben Bangert notifications@github.com wrote:

Looks good, the only error is on an older Python where there is no
assertIsNone apparently. Could just assertEqual to None for that.


Reply to this email directly or view it on GitHub:
#57 (comment)

This adds support for ``requirements`` option to mapper.resource,
which makes it possible to restrict matching in urls (most often
useful for capturing variables with path_prefix).

In OpenStack Nova we've used the prefix_path on Mapper.resource to
specify additional variables we want to capture (specifically
{project_id}). Project_id is a uuid. When trying to restrict
project_id to only valid uuid format a couple of issues were exposed.

 - #1 '/{project_id:[a-f0-9]{32}}/...' builds an incorrect regex
    because of the nested {}

 - #2 the preferred method that works on connect() to pass
    requirements doesn't work here (requirements are reset to only an
    id match)

That leaves us with having to build a custom project_id match with 32
[a-f0-9] strings appended for every resource added to get the support
we need without effectively vendoring our own version of Mapper.resource.

This small change to allow requirements to pass through would make it
possible to get this tighter validation with much smaller regexes.
bbangert added a commit that referenced this pull request Jan 16, 2016
allow requirements to be specified on Mapper.resource
@bbangert bbangert merged commit bafc153 into bbangert:master Jan 16, 2016
@bbangert
Copy link
Owner

Thanks!

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

2 participants