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

extra_template_vars() sending wrong view_name for index #716

Closed
simonw opened this issue Apr 4, 2020 · 8 comments
Closed

extra_template_vars() sending wrong view_name for index #716

simonw opened this issue Apr 4, 2020 · 8 comments

Comments

@simonw
Copy link
Owner

simonw commented Apr 4, 2020

See simonw/museums#20 (comment) - at some point between 286ed28 and current master (e0e7a0f today) a bug was introduced where the extra_template_vars(request, view_name) plugin hook started being passed None instead of index for the view_name parameter on the site index page.

@simonw
Copy link
Owner Author

simonw commented Apr 4, 2020

I think I'll use git bisect run for this one, mainly to practice using it. https://git-scm.com/docs/git-bisect#_bisect_run

@simonw
Copy link
Owner Author

simonw commented Apr 5, 2020

Once I fix this bug I should update https://github.com/simonw/museums to deploy using the latest Datasette release as opposed to being anchored to 286ed28.

https://github.com/simonw/museums/blob/1bbed542617757e9e276a5098193d6288b7f421d/.github/workflows/push.yml#L61

@simonw
Copy link
Owner Author

simonw commented Apr 5, 2020

In order to use git bisect run I need to write a standalone script that can tell if the bug is present or not.

I'm going to use something I can execute with pytest, inspired loosely by this code:

def test_plugins_extra_template_vars(restore_working_directory):
for client in make_app_client(
template_dir=str(pathlib.Path(__file__).parent / "test_templates")
):
response = client.get("/-/metadata")
assert response.status == 200
extra_template_vars = json.loads(
Soup(response.body, "html.parser").select("pre.extra_template_vars")[0].text
)
assert {
"template": "show_json.html",
"scope_path": "/-/metadata",
} == extra_template_vars
extra_template_vars_from_awaitable = json.loads(
Soup(response.body, "html.parser")
.select("pre.extra_template_vars_from_awaitable")[0]
.text
)
assert {
"template": "show_json.html",
"awaitable": True,
"scope_path": "/-/metadata",
} == extra_template_vars_from_awaitable

@simonw
Copy link
Owner Author

simonw commented Apr 5, 2020

OK, here's the test harness. Three files:

check_view_name.py

import asyncio
import pathlib
from datasette.app import Datasette
import httpx

root = pathlib.Path(__file__).parent


async def run_check():
    ds = Datasette(
        [], template_dir=str(root / "templates"), plugins_dir=str(root / "plugins")
    )
    async with httpx.AsyncClient(app=ds.app()) as client:
        response = await client.get("http://localhost/")
        assert 200 == response.status_code
        assert b"view_name:index" == response.content, response.content


if __name__ == "__main__":
    loop = asyncio.get_event_loop()
    loop.run_until_complete(run_check())

templates/index.html

view_name:{{ view_name }}

plugins/extra_vars.py

from datasette import hookimpl


@hookimpl
def extra_template_vars(view_name):
    return {"view_name": view_name}

@simonw
Copy link
Owner Author

simonw commented Apr 5, 2020

python check_view_name.py against 286ed28 exits cleanly.

python check_view_name.py against current master (e0e7a0f) fails:

$ python check_view_name.py 
Traceback (most recent call last):
  File "check_view_name.py", line 16, in <module>
    loop.run_until_complete(run_check())
  File "/usr/local/Cellar/python/3.7.5/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/base_events.py", line 579, in run_until_complete
    return future.result()
  File "check_view_name.py", line 11, in run_check
    assert b"view_name:index" == response.content, response.content
AssertionError: b'view_name:None'

@simonw
Copy link
Owner Author

simonw commented Apr 5, 2020

You start git bisect by giving it a known bad commit and a known good one:

git bisect start master 286ed28 

Then you tell it to start running your script:

git bisect run python ../datasette-issue-716/check_view_name.py

Here's what I got:

(datasette) ~/Dropbox/Development/datasette $ git bisect start master 286ed28
Bisecting: 30 revisions left to test after this (roughly 5 steps)
[dc80e779a2e708b2685fc641df99e6aae9ad6f97] Handle scope path if it is a string
(datasette) ~/Dropbox/Development/datasette $ git bisect run python ../datasette-issue-716/check_view_name.py
running python ../datasette-issue-716/check_view_name.py
Traceback (most recent call last):
...
Bisecting: 15 revisions left to test after this (roughly 4 steps)
[7c6a9c35299f251f9abfb03fd8e85143e4361709] Better tests for prepare_connection() plugin hook, refs #678
running python ../datasette-issue-716/check_view_name.py
Traceback (most recent call last):
...
Bisecting: 7 revisions left to test after this (roughly 3 steps)
[0091dfe3e5a3db94af8881038d3f1b8312bb857d] More reliable tie-break ordering for facet results
running python ../datasette-issue-716/check_view_name.py
Traceback (most recent call last):
...
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[ce12244037b60ba0202c814871218c1dab38d729] Release notes for 0.35
running python ../datasette-issue-716/check_view_name.py
Traceback (most recent call last):
...
Bisecting: 0 revisions left to test after this (roughly 1 step)
[4d7dae9eb75e5430c3ee3c369bb5cd9ba0a148bc] Added a bunch more plugins to the Ecosystem page
running python ../datasette-issue-716/check_view_name.py
Traceback (most recent call last):
...
70b915fb4bc214f9d064179f87671f8a378aa127 is the first bad commit
commit 70b915fb4bc214f9d064179f87671f8a378aa127
Author: Simon Willison <swillison@gmail.com>
Date:   Tue Feb 4 12:26:17 2020 -0800

    Datasette.render_template() method, closes #577
    
    Pull request #664.

:040000 040000 def9e31252e056845609de36c66d4320dd0c47f8 da19b7f8c26d50a4c05e5a7f05220b968429725c M	datasette
bisect run success

So 70b915f introduced the bug!

@simonw
Copy link
Owner Author

simonw commented Apr 5, 2020

Found it. Prior to that change I passed view_name to the callbacks like this:

for script in pm.hook.extra_body_script(
template=template.name,
database=context.get("database"),
table=context.get("table"),
view_name=self.name,
datasette=self.ds,
):
body_scripts.append(jinja2.Markup(script))
extra_template_vars = {}
# pylint: disable=no-member
for extra_vars in pm.hook.extra_template_vars(
template=template.name,
database=context.get("database"),
table=context.get("table"),
view_name=self.name,
request=request,
datasette=self.ds,
):

I switched over to doing this:

return Response.html(
await self.ds.render_template(template, template_context, request=request)
)

But forgot to pass in the optional view_name= argument to that render_template() method:

datasette/datasette/app.py

Lines 554 to 556 in 2aaad72

async def render_template(
self, templates, context=None, request=None, view_name=None
):

Next step: write a failing test, then fix it.

@simonw simonw closed this as completed in 0925381 Apr 5, 2020
@simonw
Copy link
Owner Author

simonw commented Apr 5, 2020

The test ended up being a bit fiddly - here it is:

@pytest.fixture(scope="session")
def view_names_client(tmp_path_factory):
tmpdir = tmp_path_factory.mktemp("test-view-names")
templates = tmpdir / "templates"
templates.mkdir()
plugins = tmpdir / "plugins"
plugins.mkdir()
for template in (
"index.html",
"database.html",
"table.html",
"row.html",
"show_json.html",
"query.html",
):
(templates / template).write_text("view_name:{{ view_name }}", "utf-8")
(plugins / "extra_vars.py").write_text(
textwrap.dedent(
"""
from datasette import hookimpl
@hookimpl
def extra_template_vars(view_name):
return {"view_name": view_name}
"""
),
"utf-8",
)
db_path = str(tmpdir / "fixtures.db")
conn = sqlite3.connect(db_path)
conn.executescript(TABLES)
return TestClient(
Datasette(
[db_path], template_dir=str(templates), plugins_dir=str(plugins)
).app()
)
@pytest.mark.parametrize(
"path,view_name",
(
("/", "index"),
("/fixtures", "database"),
("/fixtures/units", "table"),
("/fixtures/units/1", "row"),
("/-/metadata", "json_data"),
("/fixtures?sql=select+1", "database"),
),
)
def test_view_names(view_names_client, path, view_name):
response = view_names_client.get(path)
assert response.status == 200
assert "view_name:{}".format(view_name) == response.body.decode("utf8")

I used tmp_path_factory here because the tmpdir fixture I usually use isn't compatible with scope="session", and I wanted to only create those temporary plugins and templates directories once rather than create them for each run of the parametrized test function.

In writing this I realized that the name on the QueryView class wasn't being used, because that class is currently just used for its .data() method:

sql = request.raw_args.pop("sql")
validate_sql_select(sql)
return await QueryView(self.ds).data(
request, database, hash, sql, _size=_size, metadata=metadata
)

canned_query = self.ds.get_canned_query(database, table)
if canned_query:
return await QueryView(self.ds).data(
request,

So I removed the name = "query" line since it was misleading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant