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

render_cell() hook should support returning an awaitable #1425

Closed
simonw opened this issue Aug 8, 2021 · 11 comments
Closed

render_cell() hook should support returning an awaitable #1425

simonw opened this issue Aug 8, 2021 · 11 comments
Labels

Comments

@simonw
Copy link
Owner

simonw commented Aug 8, 2021

Many of the plugin hooks can return an awaitable - e.g. https://docs.datasette.io/en/stable/plugin_hooks.html#plugin-hook-extra-template-vars - but render_cell() doesn't support this.

I recently found myself wanting to execute an additional SQL query from that hook, but it wasn't possible to do that since I couldn't use await.

@simonw simonw added the plugins label Aug 8, 2021
@simonw
Copy link
Owner Author

simonw commented Aug 8, 2021

I can do this with the await_me_maybe() function, as seen here:

datasette/datasette/app.py

Lines 864 to 873 in a21853c

for extra_vars in pm.hook.extra_template_vars(
template=template.name,
database=context.get("database"),
table=context.get("table"),
columns=context.get("columns"),
view_name=view_name,
request=request,
datasette=self,
):
extra_vars = await await_me_maybe(extra_vars)

@simonw
Copy link
Owner Author

simonw commented Aug 8, 2021

simonw added a commit that referenced this issue Aug 8, 2021
@simonw
Copy link
Owner Author

simonw commented Aug 9, 2021

Still one test failure:

    def test_hook_render_cell_link_from_json(app_client):
        sql = """
            select '{"href": "http://example.com/", "label":"Example"}'
        """.strip()
        path = "/fixtures?" + urllib.parse.urlencode({"sql": sql})
        response = app_client.get(path)
        td = Soup(response.body, "html.parser").find("table").find("tbody").find("td")
        a = td.find("a")
>       assert a is not None, str(a)
E       AssertionError: None
E       assert None is not None

The weird thing about this one is that I can't replicate it on my laptop - but it happens in CI every time, including when I shell in and try to run that single test.

@simonw
Copy link
Owner Author

simonw commented Aug 9, 2021

My hunch is that the "skip this render_cell() result if it returns None" logic isn't working correctly, ever since I added the await_me_maybe line.

Could that be because Pluggy handles the "do the next if None is returned" logic itself, but I'm no-longer returning None, I'm returning an awaitable which when awaited returns None.

This would suggest that all of the await_me_maybe() plugin hooks have the same bug. That's definitely possible - it may well be that no-one has yet stumbled across a bug caused by a plugin returning an awaitable and hence not being skipped, because plugin hooks that return awaitable are rare enough that no-one has tried two plugins which both use that trick.

Still don't see why it would pass on my laptop but fail in CI though.

@simonw
Copy link
Owner Author

simonw commented Aug 9, 2021

Good news: render_cell() is the only hook to use firstresult=True:

@hookspec(firstresult=True)
def render_cell(value, column, table, database, datasette):
"""Customize rendering of HTML table cell values"""

https://pluggy.readthedocs.io/en/latest/#first-result-only

@simonw
Copy link
Owner Author

simonw commented Aug 9, 2021

Here's the code in pluggy that implements this: https://github.com/pytest-dev/pluggy/blob/0a064fe275060dbdb1fe6e10c888e72bc400fb33/src/pluggy/callers.py#L31-L43

                if hook_impl.hookwrapper:
                    try:
                        gen = hook_impl.function(*args)
                        next(gen)  # first yield
                        teardowns.append(gen)
                    except StopIteration:
                        _raise_wrapfail(gen, "did not yield")
                else:
                    res = hook_impl.function(*args)
                    if res is not None:
                        results.append(res)
                        if firstresult:  # halt further impl calls
                            break

@simonw
Copy link
Owner Author

simonw commented Aug 9, 2021

I could extract that code out and write my own function which implements the equivalent of calling pm.hook.render_cell(...) but runs await_me_maybe() before checking if res is not None.

That's pretty nasty.

Could I instead call the plugin hook normally, but then have additional logic which says "if I await it and it returns None then try calling the hook again but skip this one" - not sure if there's a way to do that either.

I could remove the firstresult=True from the hookspec - which would cause it to call and return ALL hooks - but then in my own code use only the first one. This is slightly less efficient (since it calls all the hooks and then discards all-but-one value) but it's the least unpleasant in terms of the code I would have to write - plus I don't think it's going to be THAT common for someone to have multiple expensive render_cell() hooks installed at once (they are usually pretty cheap).

@simonw
Copy link
Owner Author

simonw commented Aug 9, 2021

I'm trying the version where I remove firstresult=True.

@simonw
Copy link
Owner Author

simonw commented Aug 9, 2021

Demo: https://latest.datasette.io/fixtures/simple_primary_key shows RENDER_CELL_ASYNC_RESULT where the CSV version shows RENDER_CELL_ASYNC: https://latest.datasette.io/fixtures/simple_primary_key.csv - because of this test plugin code:

@hookimpl
def render_cell(value, column, table, database, datasette):
async def inner():
# Render some debug output in cell with value RENDER_CELL_DEMO
if value == "RENDER_CELL_DEMO":
return json.dumps(
{
"column": column,
"table": table,
"database": database,
"config": datasette.plugin_config(
"name-of-plugin",
database=database,
table=table,
),
}
)
elif value == "RENDER_CELL_ASYNC":
return (
await datasette.get_database(database).execute(
"select 'RENDER_CELL_ASYNC_RESULT'"
)
).single_value()
return inner

simonw added a commit that referenced this issue Aug 9, 2021
@simonw simonw closed this as completed Aug 9, 2021
@abdusco
Copy link
Contributor

abdusco commented Aug 9, 2021

I believe this also provides a workaround for the problem I face in #1300.

Now I should be able to get table PKs and generate a row URL. I'll test this out and report my findings.

from datasette.utils import path_from_row_pks

pks = await db.primary_keys(table)
url = self.ds.urls.row_blob(
    database,
    table,
    path_from_row_pks(row, pks, not pks),
    column,
)

simonw added a commit that referenced this issue Oct 14, 2021
simonw added a commit that referenced this issue Oct 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants