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

"flash messages" mechanism #790

Closed
simonw opened this issue Jun 1, 2020 · 20 comments
Closed

"flash messages" mechanism #790

simonw opened this issue Jun 1, 2020 · 20 comments
Labels

Comments

@simonw
Copy link
Owner

simonw commented Jun 1, 2020

Passing ?_success like this isn't necessarily the best approach. Potential improvements include:

  • Signing this message so it can't be tampered with (I could generate a signing secret on startup)
  • Using a cookie with a temporary flash message in it instead
  • Using HTML5 history API to remove the ?_success= from the URL bar when the user lands on the page

If I add an option to redirect the user to another page after success I may need a mechanism to show a flash message on that page as well, in which case I'll need a general flash message solution that works for any page.

Originally posted by @simonw in #703

@simonw simonw changed the title Need a "flash messages" solution "flash messages" mechanism Jun 1, 2020
@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

I can use the new signed values support from #785 to help build this.

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

Actually I'm inclined to use cookies now, ala Django: https://docs.djangoproject.com/en/3.0/ref/contrib/messages/

This class stores the message data in a cookie (signed with a secret hash to prevent manipulation) to persist notifications across requests. Old messages are dropped if the cookie data size would exceed 2048 bytes.

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

Setting messages just needs access to the response. Reading messages needs access to both request AND response, since it needs to clear the messages that are being displayed.

That's if the messages are persisted exclusively in cookies - which makes sense for Django since it's designed to run as many different load-balanced processes.

Since Datasette is a single process which can access an on-file database, maybe consider storing the flash messages within Datasette memory itself - a sort of session mechanism?

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

I'm going to build the first version of this with signed cookies.

I'm inclined to do this all on the request object, since it's the object representing the current request as it flows through the application. I need the ability to remember which messages were set and which need to be cleared, so I need to do that on something that is available for the lifetime of the request.

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

Maybe two utility functions:

add_message(request, message) - adds a Flash message (will be set later)

read_and_clear_messages(request) - reads messages and sets them to be cleared

Problem: the request object isn't created at the very top of the stack - it's actually created within each view. So maybe I need to move its creation up to the top of the routing stuff so that the code that returns the response can see it?

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

Alternative: datasette.add_message(message) and datasette.read_and_clear_messages() - these would need some kind of dark magic to ensure that the message was associated with the current request flowing through the system though, since that datasette object is shared my multiple concurrent requests.

Maybe use a request correlation ID that gets added to the scope? This is all getting a bit messy.

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

Here's how the Django stuff works: https://github.com/django/django/blob/master/django/contrib/messages/storage/base.py

Notably the messages are mostly dealt with on the request object, with a piece of middleware that reads from the request and modifies the response (to set or clear cookies) right at the end: https://github.com/django/django/blob/master/django/contrib/messages/middleware.py

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

What if I use a mutable key on scope to track messages for the duration of the request? Is that an OK thing to do?

ASGI spec says this: https://asgi.readthedocs.io/en/latest/specs/main.html#middleware

Middleware

It is possible to have ASGI "middleware" - code that plays the role of both server and application, taking in a scope and the send/receive awaitables, potentially modifying them, and then calling an inner application.

When middleware is modifying the scope, it should make a copy of the scope object before mutating it and passing it to the inner application, as changes may leak upstream otherwise. In particular, you should not assume that the copy of the scope you pass down to the application is the one that it ends up using, as there may be other middleware in the way; thus, do not keep a reference to it and try to mutate it outside of the initial ASGI constructor callable that receives scope. Your one and only chance to add to it is before you hand control to the child application.

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

If scope had an immutable correlation ID I could use that with a dict somewhere mapping correlation_id to a messages object.

The problem then is how do I know to clean up the memory used by that dictionary when the request flows out of the system? I guess the code that updates the cookies in the response could do that.

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

Flask and Django both support "types" of message - info, warning etc. I think I should do the same.

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

It would be neat if this was driven by a method on datasette just because that's already the object passed to plugins as a documented API.

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

datasette.add_message(request, message, type=datasette.INFO)

Then later:

datasette.write_messages_to_response(request, response)
Writes the messages as cookies in the response.

datasette.fetch_and_clear_messages(request, response)
To display messages and clears them from the response.

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

Problem with datasette.fetch_and_clear_messages(request, response) is that I want to call it from the template (so we only clear messages if they have been displayed) - but by that point in the code the Response object has not yet been created, so it can't have cookie set on it to clear the list of messages.

Solution: call it datasette.show_messages(request) and have it update internal state on the datasette object such that a later call to write_messages_to_response(request, response) knows to clear the cookie.

@simonw
Copy link
Owner Author

simonw commented Jun 1, 2020

I'm going to stash these on the request object after all, so the memory used for the messages gets automatically cleaned up at the end of the request.

@simonw
Copy link
Owner Author

simonw commented Jun 2, 2020

I'm going to use a output renderer plugin to test this, since then my unit tests can run against custom code that both sets and displays messages.

@simonw
Copy link
Owner Author

simonw commented Jun 2, 2020

I need to make sure that any time cookies are set there's no cache-control header (or it is set to private).

@simonw
Copy link
Owner Author

simonw commented Jun 2, 2020

The /-/messages debug tool will need CSRF protection or people will be able to add messages using a hidden form on another website.

@simonw
Copy link
Owner Author

simonw commented Jun 2, 2020

From #698 (comment)

Concept for displaying a success message:

fixtures__compound_three_primary_keys__1_001_rows CSS:
.success {
    padding: 1em;
    border: 1px solid green;
    background-color: #c7fbc7;
}

simonw added a commit that referenced this issue Jun 2, 2020
@simonw simonw closed this as completed in 4fa7cf6 Jun 2, 2020
@simonw
Copy link
Owner Author

simonw commented Jun 2, 2020

@simonw
Copy link
Owner Author

simonw commented Jun 2, 2020

Message CSS is now demonstrated on https://latest.datasette.io/-/patterns

@simonw simonw added this to the Datasette 1.0 milestone Jun 2, 2020
@simonw simonw modified the milestones: Datasette 1.0, Datasette 0.44 Jun 6, 2020
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

1 participant