datasette.client internal requests mechanism #1000

simonw · 2020-10-08T23:58:25Z

simonw · 2020-10-08T23:59:01Z

Needs tests and documentation.

codecov · 2020-10-09T00:03:29Z

Codecov Report

Merging #1000 into main will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1000      +/-   ##
==========================================
+ Coverage   84.37%   84.52%   +0.15%     
==========================================
  Files          28       28              
  Lines        3871     3878       +7     
==========================================
+ Hits         3266     3278      +12     
+ Misses        605      600       -5

Impacted Files	Coverage Δ
datasette/app.py	`96.34% <100.00%> (+0.02%)`	⬆️
datasette/cli.py	`74.35% <100.00%> (ø)`
datasette/utils/testing.py	`95.16% <100.00%> (-4.84%)`	⬇️
datasette/views/base.py	`93.94% <100.00%> (+0.11%)`	⬆️
datasette/utils/asgi.py	`91.92% <0.00%> (ø)`
datasette/views/special.py	`93.51% <0.00%> (+8.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7249ac5...8a80c79. Read the comment docs.

simonw · 2020-10-09T00:37:02Z

I'm going to route the existing TestClient through this mechanism to exercise it during the tests.

simonw · 2020-10-09T00:50:49Z

Almost all of the tests are passing:

=========================== short test summary info ============================
FAILED tests/test_api.py::test_table_with_slashes_in_name - assert 404 == 200
FAILED tests/test_api.py::test_row_strange_table_name - assert 404 == 200
FAILED tests/test_html.py::test_row_strange_table_name_with_url_hash - assert...
FAILED tests/test_html.py::test_css_classes_on_body[/fixtures/table%2Fwith%2Fslashes.csv-expected_classes5]
FAILED tests/test_html.py::test_templates_considered[/fixtures/table%2Fwith%2Fslashes.csv-table-fixtures-tablewithslashescsv-fa7563.html, *table.html]
FAILED tests/test_html.py::test_base_url_config[/fixtures/compound_three_primary_keys-https://example.com/]
FAILED tests/test_html.py::test_base_url_config[/fixtures/compound_three_primary_keys/a,a,a-https://example.com/]
FAILED tests/test_html.py::test_base_url_config[/fixtures/paginated_view-https://example.com/]
FAILED tests/test_html.py::test_base_url_config[/fixtures/facetable-https://example.com/]
FAILED tests/test_messages.py::test_messages_are_displayed_and_cleared - KeyE...
FAILED tests/test_plugins.py::test_hook_register_magic_parameters - Assertion...
============ 11 failed, 718 passed, 6 warnings in 225.77s (0:03:45) ============

simonw · 2020-10-09T01:46:06Z

For this failing test I'm suspicious that the AsyncClient may be persisting cookies in between requests:

    def test_actor_cookie(app_client):
        "A valid actor cookie sets request.scope['actor']"
        cookie = app_client.actor_cookie({"id": "test"})
        response = app_client.get("/", cookies={"ds_actor": cookie})
>       assert {"id": "test"} == app_client.ds._last_request.scope["actor"]
E       AssertionError: assert {'id': 'test'} == {'id': 'root'}
E         Differing items:
E         {'id': 'test'} != {'id': 'root'}
E         Use -v to get the full diff

simonw · 2020-10-09T01:51:05Z

The topic of disabling cookie persistence is discussed a little here: encode/httpx#422 (comment)

We have a cookiejar abstraction, I think setting it to an always-empty jar like you describe is best. :)

simonw · 2020-10-09T01:51:44Z

I'm going to switch back to having each request run through a new client. I'm worried about the impact on test performance though. I'll run a microbenchmark before and after.

simonw · 2020-10-09T01:55:01Z

With the single client that is reused for all tests:

% time pytest tests/test_api.py
...
6.73s user 9.91s system 81% cpu 20.365 total

After switching back to this class:

class DatasetteClient:
    def __init__(self, ds):
        self.app = ds.app()

    def _fix(self, path):
        if path.startswith("/"):
            path = "http://localhost{}".format(path)
        return path

    async def get(self, path, **kwargs):
        async with httpx.AsyncClient(app=self.app) as client:
            return await client.get(self._fix(path), **kwargs)

    async def options(self, path, **kwargs):
        async with httpx.AsyncClient(app=self.app) as client:
            return await client.options(self._fix(path), **kwargs)

    async def head(self, path, **kwargs):
        async with httpx.AsyncClient(app=self.app) as client:
            return await client.head(self._fix(path), **kwargs)

    async def post(self, path, **kwargs):
        async with httpx.AsyncClient(app=self.app) as client:
            return await client.post(self._fix(path), **kwargs)

    async def put(self, path, **kwargs):
        async with httpx.AsyncClient(app=self.app) as client:
            return await client.put(self._fix(path), **kwargs)

    async def patch(self, path, **kwargs):
        async with httpx.AsyncClient(app=self.app) as client:
            return await client.patch(self._fix(path), **kwargs)

    async def delete(self, path, **kwargs):
        async with httpx.AsyncClient(app=self.app) as client:
            return await client.delete(self._fix(path), **kwargs)

    async def request(self, method, path, **kwargs):
        async with httpx.AsyncClient(app=self.app) as client:
            return await client.request(method, self._fix(path), **kwargs)

The time taken is:

% time pytest tests/test_api.py
...
7.26s user 10.02s system 82% cpu 21.014 total

That's close enough that I don't feel I need to investigate this further.

simonw · 2020-10-09T02:14:14Z

Still need to handle these six failing tests:

FAILED tests/test_html.py::test_base_url_config[/fixtures/compound_three_primary_keys-https://example.com/] - AssertionError: {'base_url': 'https://example.com/', 'elemen...
FAILED tests/test_html.py::test_base_url_config[/fixtures/compound_three_primary_keys/a,a,a-https://example.com/] - AssertionError: {'base_url': 'https://example.com/', '...
FAILED tests/test_html.py::test_base_url_config[/fixtures/paginated_view-https://example.com/] - AssertionError: {'base_url': 'https://example.com/', 'element_parent': '<...
FAILED tests/test_html.py::test_base_url_config[/fixtures/facetable-https://example.com/] - AssertionError: {'base_url': 'https://example.com/', 'element_parent': '<p cla...
FAILED tests/test_messages.py::test_messages_are_displayed_and_cleared - KeyError: 'ds_messages'
FAILED tests/test_plugins.py::test_hook_register_magic_parameters - AssertionError: assert [{'line': '1.0', 'rowid': 1}] == [{'line': '1.1', 'rowid': 1}]
=========================================================== 6 failed, 731 passed, 6 warnings in 129.17s (0:02:09) ===========================================================

simonw · 2020-10-09T02:15:38Z

FAILED tests/test_messages.py::test_messages_are_displayed_and_cleared - KeyError: 'ds_messages'

That one is caused by response.cookies skipping cookies that were set to the empty string. Same fix as this:

datasette/tests/test_auth.py

Lines 90 to 95 in a168735

    
           # The ds_actor cookie should have been unset 
        
           assert [ 
        
               h 
        
               for h in response4.headers.get_list("set-cookie") 
        
               if h.startswith('ds_actor=""; ') 
        
           ]

simonw · 2020-10-09T02:52:53Z

These failures are giving me a severe "how did this ever work in the first place?" vibe:

FAILED tests/test_html.py::test_base_url_config[/fixtures/compound_three_primary_keys-https://example.com/]
FAILED tests/test_html.py::test_base_url_config[/fixtures/compound_three_primary_keys/a,a,a-https://example.com/]
FAILED tests/test_html.py::test_base_url_config[/fixtures/paginated_view-https://example.com/]
FAILED tests/test_html.py::test_base_url_config[/fixtures/facetable-https://example.com/]

I have a fix for them, no idea why they weren't already failing though.

simonw · 2020-10-09T03:04:15Z

This is really weird: new set of test failures that I wasn't seeing before, and those tests aren't failing on my laptop:

=========================== short test summary info ============================
FAILED tests/test_api.py::test_table_with_slashes_in_name - assert 404 == 200
FAILED tests/test_api.py::test_row_strange_table_name - assert 404 == 200
FAILED tests/test_html.py::test_row_strange_table_name_with_url_hash - assert...
FAILED tests/test_html.py::test_css_classes_on_body[/fixtures/table%2Fwith%2Fslashes.csv-expected_classes5]
FAILED tests/test_html.py::test_templates_considered[/fixtures/table%2Fwith%2Fslashes.csv-table-fixtures-tablewithslashescsv-fa7563.html, *table.html]
================== 5 failed, 738 passed in 194.73s (0:03:14) ===================

simonw · 2020-10-09T03:08:43Z

Most likely reason for those failures is that path and raw_path are not being simulated correctly. I used to do those here:

datasette/datasette/utils/testing.py

Lines 116 to 125 in 402cf87

    
           scope = { 
        
               "type": "http", 
        
               "http_version": "1.0", 
        
               "method": method, 
        
               "path": unquote(path), 
        
               "raw_path": raw_path, 
        
               "query_string": query_string, 
        
               "headers": asgi_headers, 
        
           } 
        
           instance = ApplicationCommunicator(self.asgi_app, scope)

But now I'm delegating that to httpx to handle.

WEIRD that it passes on my laptop but fails in GitHub Actions CI though.

simonw · 2020-10-09T03:24:48Z

I'm testing this with a print(scope) and pytest -k test_table_with_slashes_in_name -s.

Against the main branch:

{'type': 'http', 'http_version': '1.0', 'method': 'GET', 'path': '/fixtures/table/with/slashes.csv', 'raw_path': b'/fixtures/table%2Fwith%2Fslashes.csv', 'query_string': b'_shape=objects&_format=json', 'headers': [[b'host', b'localhost']], 'csrftoken': <function asgi_csrf_decorator.<locals>._asgi_csrf_decorator.<locals>.app_wrapped_with_csrf.<locals>.get_csrftoken at 0x10e2e6040>}

Against the broken branch:

tests/test_api.py {'type': 'http', 'asgi': {'version': '3.0'}, 'http_version': '1.1', 'method': 'GET', 'headers': [(b'host', b'localhost'), (b'accept', b'*/*'), (b'accept-encoding', b'gzip, deflate'), (b'connection', b'keep-alive'), (b'user-agent', b'python-httpx/0.15.0')], 'scheme': 'http', 'path': '/fixtures/table%2Fwith%2Fslashes.csv', 'query_string': b'_shape=objects&_format=json', 'server': ('localhost', None), 'client': ('127.0.0.1', 123), 'root_path': '', 'csrftoken': <function asgi_csrf_decorator.<locals>._asgi_csrf_decorator.<locals>.app_wrapped_with_csrf.<locals>.get_csrftoken at 0x109e0eca0>}

This is on my laptop though so both of those pass the tests.

Key difference: the httpx version doesn't set a raw_path at all. BUT.. it does set path and sets it to '/fixtures/table%2Fwith%2Fslashes.csv'

The non-httpx version sets raw_path to b'/fixtures/table%2Fwith%2Fslashes.csv' and path to '/fixtures/table/with/slashes.csv'.

simonw · 2020-10-09T03:27:05Z

I may need to fuss around with how the httpx client sends things to the ASGI app.

https://github.com/encode/httpx/blob/92ca4d0cc654859fc2257c492e55d8752370d427/httpx/_transports/asgi.py#L26 is relevant:

Alternatively, you can setup the transport instance explicitly.
This allows you to include any additional configuration arguments specific
to the ASGITransport class:
```
transport = httpx.ASGITransport(
    app=app,
    root_path="/submount",
    client=("1.2.3.4", 123)
)
client = httpx.AsyncClient(transport=transport)
```

simonw · 2020-10-09T03:28:08Z

Here's where httpx sets up the ASGI scope: https://github.com/encode/httpx/blob/92ca4d0cc654859fc2257c492e55d8752370d427/httpx/_transports/asgi.py#L82-L97

        # ASGI scope.
        scheme, host, port, full_path = url
        path, _, query = full_path.partition(b"?")
        scope = {
            "type": "http",
            "asgi": {"version": "3.0"},
            "http_version": "1.1",
            "method": method.decode(),
            "headers": [(k.lower(), v) for (k, v) in headers],
            "scheme": scheme.decode("ascii"),
            "path": unquote(path.decode("ascii")),
            "query_string": query,
            "server": (host.decode("ascii"), port),
            "client": self.client,
            "root_path": self.root_path,
        }

Sure enough, it doesn't set the raw_path.

simonw · 2020-10-09T15:57:15Z

My httpx pull request adding raw_path support was just merged: encode/httpx#1357 - but it's not in a release yet.

I'm going to mark these tests as xfail so I can land this change - I'll remove that once an httpx release comes out that I can use to get the tests passing.

simonw · 2020-10-09T16:08:49Z

I'm going to document this in a separate issue.

Refs #1000

WIP datasette.client, refs #943

fa3ba68

simonw added this to the Datasette 0.50 milestone Oct 8, 2020

simonw self-assigned this Oct 8, 2020

Requires httpx>=0.15

402cf87

WIP running tests through datasette.client

8b9e860

Speed up tests by reusing AsincClient

f37cbcc

simonw added 3 commits October 8, 2020 18:57

Don't reuse client to avoid persisted cookies

dfe774e

Support OPTIONS without 500, closes #1001

5d73cc0

Internals tests for datasette.client methods

a168735

simonw added 2 commits October 8, 2020 19:24

Fix for deleted cookie test failure

c64fe1b

Merge branch 'main' into datasette-client

5e4771e

simonw added 2 commits October 8, 2020 19:56

Tests simulate HTTP 1.1 now

484c620

Added base_url in a bunch more places

68dc2f2

simonw mentioned this pull request Oct 9, 2020

ASGITransport does not correctly simulate raw_path in the scope encode/httpx#1356

Closed

simonw mentioned this pull request Oct 9, 2020

Remove xfail tests when new httpx is released #1005

Closed

Mark some tests as xfail

8a80c79

simonw mentioned this pull request Oct 9, 2020

Documentation for datasette.client #1006

Closed

simonw merged commit 8f97b9b into main Oct 9, 2020

simonw deleted the datasette-client branch October 9, 2020 16:11

simonw added a commit that referenced this pull request Oct 9, 2020

Documentation for datasette.client, closes #1006

c12b7a5

Refs #1000

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datasette.client internal requests mechanism #1000

datasette.client internal requests mechanism #1000

simonw commented Oct 8, 2020

simonw commented Oct 8, 2020

codecov bot commented Oct 9, 2020 •

edited

Loading

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

datasette.client internal requests mechanism #1000

datasette.client internal requests mechanism #1000

Conversation

simonw commented Oct 8, 2020

simonw commented Oct 8, 2020

codecov bot commented Oct 9, 2020 • edited Loading

Codecov Report

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

simonw commented Oct 9, 2020

codecov bot commented Oct 9, 2020 •

edited

Loading