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

Mocker doesn't urllib.parse.quote() URLs (requests does) #158

Closed
Cheaterman opened this issue Feb 9, 2021 · 7 comments · Fixed by #162
Closed

Mocker doesn't urllib.parse.quote() URLs (requests does) #158

Cheaterman opened this issue Feb 9, 2021 · 7 comments · Fixed by #162
Milestone

Comments

@Cheaterman
Copy link

Cheaterman commented Feb 9, 2021

Everything in the title - I have an URL with a space in it, it's urllib.parse.quote()-ed to %20 by requests but not by requests-mock, resulting in a failed match!

Thanks in advance :-) I could PR this, but I'm first wondering if that's maybe intentional...?

EDIT: (it's quote() not urlencode() haha)

@Cheaterman Cheaterman changed the title Mocker doesn't urlencode() URLs (requests does) Mocker doesn't urllib.parse.quote() URLs (requests does) Feb 9, 2021
@jamielennox
Copy link
Owner

I don't think it's intentional, if it works for requests it should work here too. Will try and have a look this week, but if you want to PR it please go ahead.

jamielennox added a commit that referenced this issue Apr 17, 2021
Requests-mock safely quotes things like spaces and non-url safe
characters. We should do the same thing so that the matchers work.

Fix tests that were passing even though they had invalid schema and
shouldn't have been working.

Closes: #158
jamielennox added a commit that referenced this issue Apr 27, 2021
Requests-mock safely quotes things like spaces and non-url safe
characters. We should do the same thing so that the matchers work.

Fix tests that were passing even though they had invalid schema and
shouldn't have been working.

Closes: #158
@jamielennox
Copy link
Owner

released as 1.9.0

@jamielennox jamielennox added this to the 1.9.0 milestone Apr 28, 2021
@OrangeDog
Copy link

IMO this was a breaking change. Everyone was encoding their paths already, and now they all fail to match.
At minimum, the error message is confusing:

requests_mock.get('http://test/ABC%20123')

requests_mock.exceptions.NoMockAddress: No mock address: GET http://test/ABC%20123

@ktdreyer
Copy link

I used to register my URLs like so (with requests-mock 1.8.0):

adapter.register_uri(
    'GET',
    'https://example.com/api/v1/user/coolmanager@example.com',
    json={'id': 123456})

Unfortunately those are registered as encoded now, so they no longer match. I stepped through my tests with pytest's --pdb option and looked at the paths that are set on the requests_mock pytest fixture matchers, like so:

pp([m._path for m in adapter._matchers])     
['/api/v1/products/foobar',
 '/api/v1/user/coolmanager%40example.com',
 '/api/v1/releases']

@ktdreyer
Copy link

Requests uses a slightly different quoting mechanism that quotes spaces, but not other characters.

>>> import requests.utils
>>> requests.utils.requote_uri('https://example.com/')
'https://example.com/'
>>> requests.utils.requote_uri('https://example.com/api/user@example.com')
'https://example.com/api/user@example.com'
>>> requests.utils.requote_uri('https://example.com/api/user example.com')
'https://example.com/api/user%20example.com'

Can we revert the current change for now? I've pushed #167

@m-melis
Copy link

m-melis commented Apr 29, 2021

This broke all our tests too. However, not sure that reverting this is the way to got, but at least the behavior should be identical to requests.

Matoking added a commit to Matoking/requests-mock that referenced this issue Apr 29, 2021
The previous fix to jamielennox#158 didn't match requests' own behavior
and quoted a lot of additional characters, causing test breakage.

Use `requests.utils.requote_uri` to quote the URL instead, which
should ensure the behavior matches requests more closely.
@jamielennox
Copy link
Owner

Ah - this is in hindsight an obvious break. Very sorry.

Tracking the issue as #170, i'll tag you all but please comment there.

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 a pull request may close this issue.

5 participants