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

Query string arg names starting with equal sign #1370

Merged
merged 5 commits into from
Feb 28, 2016
Merged

Query string arg names starting with equal sign #1370

merged 5 commits into from
Feb 28, 2016

Conversation

timka
Copy link
Contributor

@timka timka commented Jul 1, 2014

I use %3D (URL encoded equal sign) prefix in argument name for filter parametrization:

GET /users?%3Dphone_number=1234567

Since request_param supports matching specific values with equal sign syntax, this doesn't work when route registered with explicit filter args names in request_param.

I use %3D (URL encoded equal sign) prefix in argument name for filter parametrization:
GET /users?%3Dphone_number=1234567

Since request_param supports matching specific values with equal sign syntax, this doesn't work when route registered with explicit filter args names in request_param.
@mmerickel
Copy link
Member

I feel that this patch is too specific to your use case. It would be more appropriate to use your own predicate instead of pyramid's default predicate for such a thing. See http://docs.pylonsproject.org/projects/pyramid/en/1.5-branch/narr/hooks.html#adding-a-third-party-view-route-or-subscriber-predicate for more info on creating custom predicates.

@mcdonc
Copy link
Member

mcdonc commented Jul 1, 2014

I'm on the fence about it but I think I'm going to agree with @mmerickel because there just isn't a "right" way to fix this given that some textual sentinel separator needs to be used. That said, it's unambiguous that it should not be at the beginning of the string, so it might be reasonable to restrict the patch to startswith.

@timka
Copy link
Contributor Author

timka commented Jul 2, 2014

@mmerickel Well, this is what I do to get it working. On the other hand, having arg name starting with '=' is perfectly correct and compatible with current syntax.

@mcdonc I agree regarding the restriction. That was my initial idea actually, but I couldn't resist the temptation to generalize, you know :)

@mcdonc
Copy link
Member

mcdonc commented Jul 2, 2014

I'd accept a patch that did the startswith thing, although an additional test would be nice too.

@timka
Copy link
Contributor Author

timka commented Jul 6, 2014

Added some tests for this case based on existing similar tests

@mmerickel
Copy link
Member

So this PR states that the arg starts with an equals sign, but it appears as if the patch prevents the arg from having a value. For example, =foo=bar will not be properly split into '=foo' == 'bar'. If you want to update the patch with a fix/test for that, I'll be happy to merge it.

@sontek
Copy link
Member

sontek commented Apr 13, 2015

@timka Did you see @mmerickel's request?

@timka
Copy link
Contributor Author

timka commented Apr 14, 2015

Sorry for the delay

@mmerickel
Copy link
Member

Hey @timka, I'm sorry I didn't mention this earlier. Can you please add another commit with your name in CONTRIBUTORS.txt?

@timka
Copy link
Contributor Author

timka commented Apr 20, 2015

No prob

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.

5 participants