Support extracting messages in one pass?

[ryanmcgrath]

(I'm curious for thoughts on this, could PR it if it sounds acceptable)

Messages right now locks its inner Mutex on every pass - could it possibly have an into_inner() method that locks once and returns the entire set? Adding on to this, could it have a method for checking if it's empty or not?

The general idea here is that the common rendering case, as far as I understand, is something akin to:

{% if !messages.is_empty() %}
<ul id="my-errors-list-wrapper-that-should-only-exist-if-not-empty">
    {% for message in messages.into_iter() %}
        <li>{{ message }}</li>
    {% endfor %}
</ul>
{% endif %}

[maxcountryman]

I certainly think adding is_empty makes sense.

Regarding into_iter, I think this makes sense provided that you could e.g. call into_iter and later push new messages. (Maybe not a common use case, but the idea is that consuming messages shouldn't affect adding new ones.)

[ryanmcgrath]

but the idea is that consuming messages shouldn't affect adding new ones

Hmm, can you expand more on this? I feel like the only use case where you'd need to consume it is for "last-mile" rendering (e.g, right now if you wanted to pass it to an Askama rendering context, you can't iter on it without a workaround since the rendering context itself isn't mutable).

[ryanmcgrath]

A couple other comments that I've run up against after an hour or so of using this lib:

Is there any opposition to having mutable setters (&mut) on Messages? Right now, taking self in every method works fine if you're using a builder pattern for one area of code, but if you're writing a generic web service it can get in the way - and since it's not Copy you can't easily update the value in place.

On the iter points, is there any reason to have iter not go through pending_messages if messages is depleted? Right now what happens is that pending_messages needs a reload to show up, but if I error out of a handler early and render a form with errors, the pending_messages are held and show on the next page load.

[maxcountryman]

Is there any opposition to having mutable setters (&mut) on Messages? Right now, taking self in every method works fine if you're using a builder pattern for one area of code, but if you're writing a generic web service it can get in the way - and since it's not Copy you can't easily update the value in place.

You mean such that messages = messages.info("Hello, world!"); doesn't work for the use case? I think an example of what you mean here would be helpful.

On the iter points, is there any reason to have iter not go through pending_messages if messages is depleted?

I'm not sure I'm exactly following, but do you mean if the messages field is empty you'd want to iterate through the pending_messages field transparently? Maybe you mean something else?

[ryanmcgrath]

Ah, sorry - looks like I braindumped too late at night. Lemme back up and break down these two last points.

For context, in my application, I define my handlers as the following:

pub async fn my_handler(mut request: RequestData<T>) -> crate::Result<Response>;

T is a stand-in here for whatever custom type a handler wants to store in the request data, and Response is a mostly standard axum::Response. RequestData<T> itself is a custom extractor that does the work of globbing the litany of other extractors into one type (e.g AuthSession, Messages, etc), which alleviates quite a bit of tedious code when using Askama: any Askama template I render now just accepts a request property, rather than needing to always wire up various properties. [1]

You mean such that messages = messages.info("Hello, world!"); doesn't work for the use case? I think an example of what you mean here would be helpful.

Given the above RequestData<()> type, and the current APIs on Messages, I can't do the following:

request.messages.error("Oh no, an error");

The issue here is that Messages is a builder pattern and returns itself, but Messages itself isn't Copy - and as a result it becomes trickier to work with in contexts where I want something higher-level to just manage it. I know I need Messages on every view for a global layout area.

This can be worked around by doing something with Clone, but needing to Clone internally just to push to a type that I have mutable ownership of feels weird. I understand not wanting to break a public API, but I'd like to respectfully suggest that the builder interface isn't an appropriate pattern here - prior to Rust I did most of my web dev career with Django, and I never needed to build a chain of flash messages in one spot in the codebase, they were almost always injected from different validation spots in the codebase.

Maybe methods like the following could be added?

impl Messages {
    pub fn add_error(&mut self, message: impl Into<String>) { ... }
}

messages.add_error("Oh no, an error");

The second issue I noted is the separation of pending_messages and messages, which I surmise is owing to needing to commit pending_messages at the end of the request lifecycle. This makes sense from a framework perspective, but the following scenario breaks down with it - let's consider a very simplified "Reset Password" form handler:

pub async fn update_password(
    mut request: RequestData<T>,
    Form(form): Form<MyForm>
) -> crate::Result<Response> {
    if let Err(e) = my_validation_method(&form) {
        // This flash message will not render in the template, because it's added to 
        // pending_messages and not moved into messages (which iter() pulls from)
        // until the request lifecycle is finished. The render to string technically happens
        // here though.
        request.messages.error("Internal error flash message");

        return render(MyTemplate {
            request,
            form
        });
    }

    // This flash message *will* render, because the redirect goes through the full request lifecycle
    // and rehydrates the session after saving
    request.messages.info("Great success");
    redirect(StatusCode::SEE_OTHER, "/my/url/")
}

In the case of this error, I want to re-render the form (with anything they modified on the form so far) and display an error to the user. It's unfortunately not possible as messages.iter() only pulls from messages, but this .error pushes into pending_messages.

To work around this, I currently just use a local fork that has the following method added:

    /// Drains the *entire* messages stack.
    pub fn drain(&self) -> Vec<Message> {
        let mut data = self.data.lock();
        let mut messages = vec![];

        for queue in [
            std::mem::take(&mut data.messages),
            std::mem::take(&mut data.pending_messages)
        ] {
            for message in queue.into_iter() {
                messages.push(message);
            }
        }

        if !self.is_modified() {
            self.is_modified.store(true, atomic::Ordering::Release);
        }

        messages
    }

Then in my template, I can now just do:

<section id="messages">
{% for message in request.messages.drain().into_iter() %}
    <div class="msg msg-{{ message.level }}">{{ message }}</div>
{% endfor %}
</section>

Hopefully that breaks down better what I was spewing late at night, but let me know if I can illustrate further - they're not full code samples but I think they should get the gist across. Also, I just want to be clear - I think your work is great and I 100% don't want to come across as trashing it or anything, these are mostly aiming to be feedback that might help a wider set of use-cases. :)

[1] I'll refrain from ranting about why the extractor pattern that Rust frameworks tend to use isn't always great

[maxcountryman]

Why can't you do: request.messages = request.messages.error("Oh no, an error");? In your snippet the request is already mut so maybe I'm not understanding the problem.

[ryanmcgrath]

There might be something there, but I'd need to revert my code back to confirm - but for what it's worth, I did experiment with a method on RequestData like the following:

impl<T> RequestData<T> {
    pub fn info(&mut self, message: impl Into<String>) {
        self.messages = self.messages.info(message);
    }
}

...which created the following error:

error[E0507]: cannot move out of `self.messages` which is behind a mutable reference
   --> libs/apex-utils/src/request.rs:37:25
    |
37  |         self.messages = self.messages.info(message);
    |                         ^^^^^^^^^^^^^ ------------- `self.messages` moved due to this method call
    |                         |
    |                         move occurs because `self.messages` has type `Messages`, which does not implement the `Copy` trait
    |
note: `Messages::info` takes ownership of the receiver `self`, which moves `self.messages`

It could be worked around by the following, which I alluded to in my last comment regarding Clone:

impl<T> RequestData<T> {
    pub fn info(&mut self, message: impl Into<String>) {
        self.messages = self.messages.clone().info(message);
    }
}

Personally speaking, I also am just not a fan of needing to pepper request.messages = throughout handlers, the assignment aspect just isn't relevant to what's usually going on - but that's more subjective so I defer to you.

[maxcountryman]

The idea with taking self and returning self is that you rebind the variable. The obvious thing that enables is a fluent call chain but less obviously you're meant to rebind in cases where you can't use just the fluent API.

Regarding the second question, we have similar code in the axum-login SQLite example, but by using two HTTP verbs there's no issue with setting and reading the messages: GET renders the form, POST handles the form business logic (and e.g. error messages). The POST redirects via 303 See Other to the same path and the user agent re-renders the page with the messages. You can try this for yourself using the axum-login SQLite example and a bad password.

Just to be clear here, I'm not opposed to extending this crate: I would like to continue to expand it in ways that help folks make use of it. I also want to make sure we're building around clear problems and use cases that don't have reasonable solutions.

I think we should at least add is_empty and I'm open to other things as well with more use case vetting.