Skip to content

Django backend positional arg support #110

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

Conversation

massover
Copy link

@massover massover commented Nov 15, 2019

Description of the Change

json rpc allows params to be sent by position. The quickstart includes an example of this.

import requests
import json


def main():
    url = "http://localhost:4000/jsonrpc"
    headers = {'content-type': 'application/json'}

    # Example echo method
    payload = {
        "method": "echo",
        "params": ["echome!"],
        "jsonrpc": "2.0",
        "id": 0,
    }
    response = requests.post(
        url, data=json.dumps(payload), headers=headers).json()

    assert response["result"] == "echome!"
    assert response["jsonrpc"]
    assert response["id"] == 0

Using the django backend, params by position fail because they're missing the required request param. This code adds the request as the first argument to the method when called through the dispatcher. It makes this test pass that would be failing before.

    def test_positional_args(self):
        @api.dispatcher.add_method
        def positional_args(request, name):
            return name

        json_data = {
            "id": "0",
            "jsonrpc": "2.0",
            "method": "positional_args",
            "params": ["echome!"],
        }
        response = self.client.post(
            '',
            json.dumps(json_data),
            content_type='application/json',
        )
        self.assertEqual(response.status_code, 200)
        data = json.loads(response.content.decode('utf8'))
        self.assertEqual(data['result'], json_data['params'][0])

Additionally:

  • I updated the names of the backend tests so they can be found by pytest
  • Upgraded the version of pytest due to an attrs compatibility issue

@pavlov99 pavlov99 self-assigned this Nov 17, 2019
@pavlov99 pavlov99 self-requested a review November 17, 2019 15:42
Copy link
Owner

@pavlov99 pavlov99 left a comment

Choose a reason for hiding this comment

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

LGTM

@pavlov99
Copy link
Owner

Hi @massover, thank you for your contribution! Your code looks good to me, however, I have a couple of questions:

  1. Could you please describe the issue with pytest 4.0.3 vs 4.3.0, what does the latter fixes?
  2. Could you please confirm that pytest could not find tests.py file but could find test_backend.py?

Additionally, several other contributors added support for python 3.8 and it required pytest/tox configuration. To make the source code clean, would you mind:

  • Removing your commit with pytest update (current mater version might already fix the issue).
  • Squashing code and test commit, so there is only one commit per feature.

Appreciate your effort and looking forward to merging this to master soonest.

@massover massover force-pushed the django-backend-positional-arg-support branch from 6a06133 to 2024ac3 Compare November 18, 2019 14:37
@massover
Copy link
Author

Could you please describe the issue with pytest 4.0.3 vs 4.3.0, what does the latter fixes?

It fixes this

I do have the most recent code from master in my branch and still have this issue. I have removed my fixes from the pr as per your request.

$ tox -e py37            
GLOB sdist-make: /Development/json-rpc/setup.py
py37 recreate: /Development/json-rpc/.tox/py37
py37 installdeps: pytest==4.0.2, Django==1.11, Flask==0.12.2
py37 inst: /Development/json-rpc/.tox/.tmp/package/1/json-rpc-1.12.1.zip
py37 installed: atomicwrites==1.3.0,attrs==19.3.0,Click==7.0,Django==1.11,Flask==0.12.2,importlib-metadata==0.23,itsdangerous==1.1.0,Jinja2==2.10.3,json-rpc==1.12.1,MarkupSafe==1.1.1,more-itertools==7.2.0,pluggy==0.13.0,py==1.8.0,pytest==4.0.2,pytz==2019.3,six==1.13.0,Werkzeug==0.16.0,zipp==0.6.0
py37 run-test-pre: PYTHONHASHSEED='2002233529'
py37 run-test: commands[0] | pytest
Traceback (most recent call last):
  File "/Development/json-rpc/.tox/py37/bin/pytest", line 8, in <module>
    sys.exit(main())
  File "/Development/json-rpc/.tox/py37/lib/python3.7/site-packages/_pytest/config/__init__.py", line 58, in main
    config = _prepareconfig(args, plugins)
  File "/Development/json-rpc/.tox/py37/lib/python3.7/site-packages/_pytest/config/__init__.py", line 182, in _prepareconfig
    config = get_config()
  File "/Development/json-rpc/.tox/py37/lib/python3.7/site-packages/_pytest/config/__init__.py", line 153, in get_config
    pluginmanager.import_plugin(spec)
  File "/Development/json-rpc/.tox/py37/lib/python3.7/site-packages/_pytest/config/__init__.py", line 522, in import_plugin
    __import__(importspec)
  File "/Development/json-rpc/.tox/py37/lib/python3.7/site-packages/_pytest/tmpdir.py", line 25, in <module>
    class TempPathFactory(object):
  File "/Development/json-rpc/.tox/py37/lib/python3.7/site-packages/_pytest/tmpdir.py", line 35, in TempPathFactory
    lambda p: Path(os.path.abspath(six.text_type(p)))
TypeError: attrib() got an unexpected keyword argument 'convert'
ERROR: InvocationError for command /Development/json-rpc/.tox/py37/bin/pytest (exited with code 1)

Could you please confirm that pytest could not find tests.py file but could find test_backend.py?

$ git revert c193d95
[django-backend-positional-arg-support d613fa9] Revert "Rename files so they're found by pytest"
 2 files changed, 20 deletions(-)
 rename jsonrpc/tests/test_backend_django/{test_backend.py => tests.py} (82%)
 rename jsonrpc/tests/test_backend_flask/{test_backend.py => tests.py} (100%)
$ tox -e py37
...                                                                                                                                                                                                                              
jsonrpc/tests/test_base.py ..                                                                                                                                                                                                                                     [  0%]
jsonrpc/tests/test_bug29.py .                                                                                                                                                                                                                                     [  1%]
jsonrpc/tests/test_dispatcher.py .............                                                                                                                                                                                                                    [  7%]
jsonrpc/tests/test_examples20.py ..............                                                                                                                                                                                                                   [ 14%]
jsonrpc/tests/test_jsonrpc1.py ................................................                                                                                                                                                                                   [ 37%]
jsonrpc/tests/test_jsonrpc2.py ................................................................................                                                                                                                                                   [ 75%]
jsonrpc/tests/test_jsonrpc_errors.py ...................                                                                                                                                                                                                          [ 85%]
jsonrpc/tests/test_manager.py ................                                                                                                                                                                                                                    [ 92%]
jsonrpc/tests/test_pep3107.py .                                                                                                                                                                                                                                   [ 93%]
jsonrpc/tests/test_utils.py ..............                                                                                                                                                                                                                        [100%]
...
$ git revert d613fa9
[django-backend-positional-arg-support de11f28] Revert Revert Rename files so theyre found by pytest
 2 files changed, 20 insertions(+)
 rename jsonrpc/tests/test_backend_django/{tests.py => test_backend.py} (82%)
 rename jsonrpc/tests/test_backend_flask/{tests.py => test_backend.py} (100%)
$ tox -e py37 
...
jsonrpc/tests/test_base.py ..                                                                                                                                                                                                                                     [  0%]
jsonrpc/tests/test_bug29.py .                                                                                                                                                                                                                                     [  1%]
jsonrpc/tests/test_dispatcher.py .............                                                                                                                                                                                                                    [  6%]
jsonrpc/tests/test_examples20.py ..............                                                                                                                                                                                                                   [ 12%]
jsonrpc/tests/test_jsonrpc1.py ................................................                                                                                                                                                                                   [ 33%]
jsonrpc/tests/test_jsonrpc2.py ................................................................................                                                                                                                                                   [ 68%]
jsonrpc/tests/test_jsonrpc_errors.py ...................                                                                                                                                                                                                          [ 76%]
jsonrpc/tests/test_manager.py ................                                                                                                                                                                                                                    [ 83%]
jsonrpc/tests/test_pep3107.py .                                                                                                                                                                                                                                   [ 83%]
jsonrpc/tests/test_utils.py ..............                                                                                                                                                                                                                        [ 90%]
jsonrpc/tests/test_backend_django/test_backend.py .........                                                                                                                                                                                                       [ 93%]
jsonrpc/tests/test_backend_flask/test_backend.py ..............  
...

@pavlov99
Copy link
Owner

Merged into master.

@pavlov99 pavlov99 closed this Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants