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

datasette.add_message() doesn't work inside plugins #864

Closed
simonw opened this issue Jun 24, 2020 · 6 comments
Closed

datasette.add_message() doesn't work inside plugins #864

simonw opened this issue Jun 24, 2020 · 6 comments

Comments

@simonw
Copy link
Owner

simonw commented Jun 24, 2020

Similar problem to #863 - calling datasette.add_message() in a view registered using the register_routes() plugin hook doesn't work, because the code that writes accumulated messages to the ds_messages signed cookie lives in the BaseView class here:

response = await super().dispatch_request(request, *args, **kwargs)
if self.ds:
self.ds._write_messages_to_response(request, response)
return response

@simonw simonw added this to the Datasette 0.45 milestone Jun 24, 2020
@simonw
Copy link
Owner Author

simonw commented Jun 24, 2020

Urgh, fixing this is going to be a bit of a pain.

Here's where I added that custom dispatch_request() method - it was to implement flash messaging in #790: https://github.com/simonw/datasette/blame/1a5b7d318fa923edfcefd3df8f64dae2e9c49d3f/datasette/views/base.py#L85

If I want this to be made available to register_routes() views as well, I'm going to have to move the logic somewhere else. In particular I need to make sure that the request object is created once and used throughout the whole request cycle.

Currently register_routes() view functions get their own separate request object which is created here:

datasette/datasette/app.py

Lines 1057 to 1068 in 1a5b7d3

def wrap_view(view_fn, datasette):
async def asgi_view_fn(scope, receive, send):
response = await async_call_with_supported_arguments(
view_fn,
scope=scope,
receive=receive,
send=send,
request=Request(scope, receive),
datasette=datasette,
)
if response is not None:
await response.asgi_send(send)

So I'm going to have to refactor this quite a bit to get that shared request object which can be passed both to register_routes views and to my various BaseView subclasses.

@simonw
Copy link
Owner Author

simonw commented Jun 24, 2020

The ideal fix here would be to rework my BaseView subclass mechanism to work with register_routes() so that those views don't have any special privileges above plugin-provided views.

@simonw
Copy link
Owner Author

simonw commented Jun 29, 2020

I've made enough progress on this to be able to solve the messages issue in #864. I may still complete this overall goal (registering internal views with register_routes()) as part of Datasette 0.45 but it would be OK if it slipped to a later release.
#870 (comment)

@simonw simonw closed this as completed in 7ac4936 Jun 29, 2020
@simonw
Copy link
Owner Author

simonw commented Jun 29, 2020

Re-opening: plugins may get to set messages but they don't display them, even if they render a template that extends base.html. For example, this code in a plugin:

        return Response.html(
            await datasette.render_template(
                "write.html",
                {"databases": databases, "sql": request.args.get("sql") or ""},
                request=request,
            )
        )

This won't display messages. The reason is that the messages are made available to the template context in the BaseView.render() method here:

async def render(self, templates, request, context=None):
context = context or {}
template = self.ds.jinja_env.select_template(templates)
template_context = {
**context,
**{
"database_url": self.database_url,
"database_color": self.database_color,
"show_messages": lambda: self.ds._show_messages(request),

@simonw simonw reopened this Jun 29, 2020
@simonw
Copy link
Owner Author

simonw commented Jun 29, 2020

I think the fix is to move the "show_messages" variable to here:

datasette/datasette/app.py

Lines 735 to 748 in 7ac4936

template_context = {
**context,
**{
"app_css_hash": self.app_css_hash(),
"zip": zip,
"body_scripts": body_scripts,
"format_bytes": format_bytes,
"extra_css_urls": self._asset_urls("extra_css_urls", template, context),
"extra_js_urls": self._asset_urls("extra_js_urls", template, context),
"base_url": self.config("base_url"),
"csrftoken": request.scope["csrftoken"] if request else lambda: "",
},
**extra_template_vars,
}

@simonw
Copy link
Owner Author

simonw commented Jun 29, 2020

To test this I'll need a plugin test that renders a custom template. Here's an example I can imitate:

def test_register_routes_csrftoken(restore_working_directory, tmpdir_factory):
templates = tmpdir_factory.mktemp("templates")
(templates / "csrftoken_form.html").write_text(
"CSRFTOKEN: {{ csrftoken() }}", "utf-8"
)
with make_app_client(template_dir=templates) as client:
response = client.get("/csrftoken-form/")
expected_token = client.ds._last_request.scope["csrftoken"]()
assert "CSRFTOKEN: {}".format(expected_token) == response.text

@simonw simonw closed this as completed in a8a5f81 Jun 29, 2020
simonw added a commit that referenced this issue Jun 29, 2020
@simonw simonw unpinned this issue Jun 30, 2020
simonw added a commit that referenced this issue Jul 1, 2020
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