requests / requests-oauthlib

OAuthlib support for Python-Requests!
https://requests-oauthlib.readthedocs.org/
ISC License
1.72k stars 424 forks source link

There is a problem with python 3.8 and escape sequence: #473

Closed jtroussard closed 2 years ago

jtroussard commented 2 years ago

There is a problem with python 3.8 and escape sequence: 'invalid escape sequence \*

________________________ ERROR collecting test session _________________________
/opt/virtualenvs/py38/lib/python3.8/site-packages/py/_path/common.py:383: in visit
    for x in Visitor(fil, rec, ignore, bf, sort).gen(self):
/opt/virtualenvs/py38/lib/python3.8/site-packages/py/_path/common.py:435: in gen
    for p in self.gen(subdir):
/opt/virtualenvs/py38/lib/python3.8/site-packages/py/_path/common.py:435: in gen
    for p in self.gen(subdir):
/opt/virtualenvs/py38/lib/python3.8/site-packages/py/_path/common.py:424: in gen
    dirs = self.optsort([p for p in entries
/opt/virtualenvs/py38/lib/python3.8/site-packages/py/_path/common.py:425: in <listcomp>
    if p.check(dir=1) and (rec is None or rec(p))])
/opt/virtualenvs/py38/lib/python3.8/site-packages/_pytest/main.py:667: in _recurse
    ihook = self.gethookproxy(dirpath)
/opt/virtualenvs/py38/lib/python3.8/site-packages/_pytest/main.py:482: in gethookproxy
    my_conftestmodules = pm._getconftestmodules(fspath)
/opt/virtualenvs/py38/lib/python3.8/site-packages/_pytest/config/__init__.py:431: in _getconftestmodules
    mod = self._importconftest(conftestpath.realpath())
/opt/virtualenvs/py38/lib/python3.8/site-packages/_pytest/config/__init__.py:470: in _importconftest
    raise ConftestImportFailure(conftestpath, sys.exc_info())
E   _pytest.config.ConftestImportFailure: (local('/conftest.py'), (<class 'SyntaxError'>, SyntaxError('invalid escape sequence \\*', ('/opt/virtualenvs/py38/lib/python3.8/site-packages/requests_oauthlib/oauth1_session.py', 258, 9, '        """Fetch a request token.\n')), <traceback object at 0x7fee844d6140>))

Originally posted by @matejsp in https://github.com/requests/requests-oauthlib/issues/443#issuecomment-954664334

hugovk commented 2 years ago

Cannot reproduce locally:

$ tox -e py38
GLOB sdist-make: /private/tmp/requests-oauthlib/setup.py
py38 inst-nodeps: /private/tmp/requests-oauthlib/.tox/.tmp/package/1/requests-oauthlib-1.3.0.zip
py38 installed: certifi==2021.10.8,cffi==1.15.0,charset-normalizer==2.0.9,coverage==5.5,coveralls==3.2.0,cryptography==3.4.8,docopt==0.6.2,idna==3.3,mock==4.0.3,oauthlib==3.1.1,pycparser==2.21,PyJWT==2.3.0,requests==2.26.0,requests-mock==1.9.3,requests-oauthlib @ file:///private/tmp/requests-oauthlib/.tox/.tmp/package/1/requests-oauthlib-1.3.0.zip,six==1.16.0,urllib3==1.26.7
py38 run-test-pre: PYTHONHASHSEED='1197210308'
py38 run-test: commands[0] | coverage run --source=requests_oauthlib -m unittest discover
....................................................................
----------------------------------------------------------------------
Ran 68 tests in 2.295s

OK
_________________________________________________________________ summary _________________________________________________________________
  py38: commands succeeded
  congratulations :)

Or on CI:

https://github.com/requests/requests-oauthlib/runs/4681605483?check_suite_focus=true

Is something else needed?

matejsp commented 2 years ago

Unfortunately Python is very nondeterministic.

In our case, this issue was triggered when collecting tests (some of tests importing requests-oauthlib) using pytest and PYTHONOPTIMIZE=1 with coverage enabled on our CI. When running application normally it didn't or locally on the machine this didn't happen.

I really don't know how to better reproduce this. However patching requests-oauthlib did help (\*\* -> ** -> no other changes). And \* is invalid escape sequence.

matejsp commented 2 years ago

Trying with sample test.py and current python that I use. It is not the same but similar behaviour.

test.py

def test_me():
    """ some \*\* """ 
    print("ok")

pytest test.py

(env39) ➜  ~ pytest test.py
=============================================================================================================== test session starts ================================================================================================================
platform darwin -- Python 3.9.9, pytest-4.6.11, py-1.10.0, pluggy-0.13.1
Using --random-order-bucket=module
Using --random-order-seed=93520

rootdir: /Users/user
plugins: freezegun-0.4.2, mock-1.11.2, metadata-1.10.0, random-order-0.8.1+bts1, celery-4.4.7, cov-2.11.1, django-3.10.0, html-1.22.1, forked-1.3.0, xdist-1.27.0
collected 1 item                                                                                                                                                                                                                                   

test.py .                                                                                                                                                                                                                                    [100%]

================================================================================================================= warnings summary =================================================================================================================
test.py:2
  /Users/user/test.py:2: DeprecationWarning: invalid escape sequence \*
    """ some \*\* """

-- Docs: https://docs.pytest.org/en/latest/warnings.html
======================================================================================================= 1 passed, 1 warnings in 0.06 seconds =======================================================================================================
(env39) ➜  ~ 
hugovk commented 2 years ago

What file and line of requests-oauthlib is the problematic \*\*?

matejsp commented 2 years ago

requests_oauthlib/oauth1_session.py' line 258 (start of the docs lines)

Actual line is: :param \*\*request_kwargs: Optional arguments passed to ''post''

https://github.com/requests/requests-oauthlib/blob/master/requests_oauthlib/oauth1_session.py#L270

jtroussard commented 2 years ago

This doc string is flagged as a raw string, and that seems to be the fix for this warning. See stackoverflow link...

def fetch_request_token(self, url, realm=None, **request_kwargs):
        r"""Fetch a request token.

        This is the first step in the OAuth 1 workflow. A request token is
        obtained by making a signed post request to url. The token is then
        parsed from the application/x-www-form-urlencoded response and ready
        to be used to construct an authorization url.

        :param url: The request token endpoint URL.
        :param realm: A list of realms to request access to.
        :param \*\*request_kwargs: Optional arguments passed to ''post''
        ..."""

https://stackoverflow.com/questions/52335970/how-to-fix-string-deprecationwarning-invalid-escape-sequence-in-python

and more insight into this patch https://bugs.python.org/issue27364

I think we are safe to say that this issue has been covered.

hugovk commented 2 years ago

Thanks!

The r was added in https://github.com/requests/requests-oauthlib/pull/347 / https://github.com/requests/requests-oauthlib/commit/9d571a8205772173633b991119bb1da76d35b10b in December 2018 and released in v1.1.0.

If I delete the r and run the tests with pytest (which reports DeprecationWarnings by default), I see the error. Restoring the r fixes it.

Similarly, if I edit tox.ini to run tests with deprecation warnings turned on, I get the warning without the r and no warning with it:

-commands=coverage run --source=requests_oauthlib -m unittest discover
+commands=python -Wd -m coverage run --source=requests_oauthlib -m unittest discover

So I agree, this looks fixed.

@matejsp What version of requests-oauthlib are you using?

I would also suggest enabling deprecation warnings in tests, to help find and fix the relevant code is removed. Shall I create a PR with the above diff?

matejsp commented 2 years ago

We are using 1.0.0. That's the reason why we are missing "r" and getting this error. Seems to be fixed in later version but we are not able to upgrade yet. Sry for the false alarm.

Btw what should \*\* even represent? It is not valid escape sequence anyway (just in regex). Does a Sphinx or any other tool that needs this? Why not just using **kwargs ? star is not a reserved keyword (only in regex).

jtroussard commented 2 years ago

@matejsp It looks like it's a literal double asterisk to match the double asterisk in the signature, denoting an optional parameter, of course. Looks like this additional marking is breaking with the existing pattern, where the optional arguments are tagged with the double asterisk in the signature only. I think we can probably remove these, and the raw flag and close this issue.

Screen Shot 2022-01-02 at 11 23 49 AM

raised this PR #476 to level set this files documentation convention/pattern.