quarkiverse / quarkus-renarde

Server-side Web Framework with Qute templating, magic/easier controllers, auth, reverse-routing
Apache License 2.0
73 stars 16 forks source link

add X-Fragment and X-Template http headers #198

Closed gbourant closed 4 months ago

gbourant commented 4 months ago

I'm currently in the process of writing Renarde tests and would like to ensure that the correct templates are being returned. While I can certainly test the template content itself, I believe there is room for improvement.

One suggestion is to include the following http headers in dev/test mode:

X-Template: "template_name" | "template_name.fragment_name" X-Fragment: "true|false"

gbourant commented 4 months ago

At the moment this is good but let me keep some notes for the future.

  1. I might need to create a new method than using the registerTemplateInstanceLocaleAndRenderArgs one.
  2. Can we detect that the templateInstance is a MailTemplateInstance so we won't add the headers in the first place?
  3. Maybe detect the variant is html and inject <span data-testid="X-Fragment" style="display:none">{fragmentValue}</span> and <span data-testid="X-Template" style="display:none">{templateValue}</span>
  4. Make this a Qute feature instead of Renarde one?
FroMage commented 4 months ago

Thanks for this. This is an interesting use-case. I've never thought of asserting over the template being used. Usually I assert over the contents of the template returned, or sent by email.

I'm not sure how I feel about adding HTTP headers for this. Headers are not easy to use, though in this case they are a convenient place to attach metadata about the HTTP body.

But some templates are a combination of multiple fragments, especially with Htmx (right @ia3andy ?) and this doesn't seem to handle more than one template per HTTP body.

Also, as you noted, this won't help you with email templates, though again in this case emails have headers we could set.

I suppose an alternative way of doing this would be the quarkus-mail pattern where we have a mailbox-like object where we record every rendered template, its data, options, whatever. You could clear that mailbox prior to making an HTTP request, and observe it after, and check that it contains 3 fragments, one main template, and 2 email templates?

Wouldn't that be more useful for your testing?

WDYT @mkouba ? Also, check out the PR, it's trying to access some private classes and fields that might make sense to expose?

ia3andy commented 4 months ago

But some templates are a combination of multiple fragments, especially with Htmx (right @ia3andy ?) and this doesn't seem to handle more than one template per HTTP body.

https://github.com/quarkiverse/quarkus-renarde/blob/main/runtime/src/main/java/io/quarkiverse/renarde/htmx/HxController.java#L84

Example: https://github.com/ia3andy/renotes/blob/main/src/main/java/rest/Notes.java#L45

gbourant commented 4 months ago

Thank you @mkouba for the suggestions.

I suppose an alternative way of doing this would be the quarkus-mail pattern where we have a mailbox-like object where we record every rendered template, its data, options, whatever. You could clear that mailbox prior to making an HTTP request, and observe it after, and check that it contains 3 fragments, one main template, and 2 email templates?

Yes @FroMage, i think this makes more sense + makes my suggestion more feature complete.

FroMage commented 4 months ago

@mkouba what do you think of the idea of recording rendered templates in a mailbox pattern for test/dev mode? Similar to what we do for emails. Would you like this in Quarkus/Qute or outside?

mkouba commented 4 months ago

@mkouba what do you think of the idea of recording rendered templates in a mailbox pattern for test/dev mode? Similar to what we do for emails. Would you like this in Quarkus/Qute or outside?

You mean something like MockMailbox? Hm, that's an interesting idea. But we don't have an API yet to do this transparently. In theory, we could provide an API that would decorate the template instance and record rendered templates but that's not very convenient (you would need to use it in your application code directly). Wait a minute, maybe we could just decorate an injected template? @CheckedTemplate also obtains an injectable template, so it should work... I'll try to test this idea in the qute extension.

FroMage commented 4 months ago

Yes, like MockMailbox. It could be the Engine itself adding to it when it renders, or via a registered listener like this PR does.

mkouba commented 4 months ago

@mkouba what do you think of the idea of recording rendered templates in a mailbox pattern for test/dev mode? Similar to what we do for emails. Would you like this in Quarkus/Qute or outside?

You mean something like MockMailbox? Hm, that's an interesting idea. But we don't have an API yet to do this transparently. In theory, we could provide an API that would decorate the template instance and record rendered templates but that's not very convenient (you would need to use it in your application code directly). Wait a minute, maybe we could just decorate an injected template? @CheckedTemplate also obtains an injectable template, so it should work... I'll try to test this idea in the qute extension.

@FroMage So I have it working in this branch but now when I think about it I don't see many use cases where this could be useful :thinking:. I mean, it's nice if a component renders a template and does not expose the result in any way but aside from that...

Or did you mean something different? This PR does not assert the rendered content, it merely modifies the HTTP headers based on the template id/type.

Yes, like MockMailbox. It could be the Engine itself adding to it when it renders, or via a registered listener like this PR does.

FTR a template instance initializer may not be used to assert the rendered content.

FroMage commented 4 months ago

@mkouba I did mean exactly that indeed. From RenderedResults you can get the number of rendered templates, what they rendered to, the time. I suppose it might be useful to also be able to get their IDs (your commit assumes you know the ID to get the result), the data that was passed to them perhaps, and a way to clear the results?

This way you could write a test asserting that 2 HTML templates and 1 email template were were rendered as part of a single action, and check their IDs and data.

Honestly I'm not entirely sure of why this is going to be useful, but it seems a better API than the current PR, no? Provided there's a use for this sort of assertion…

mkouba commented 4 months ago

I did mean exactly that indeed. From RenderedResults you can get the number of rendered templates, what they rendered to, the time. I suppose it might be useful to also be able to get their IDs (your commit assumes you know the ID to get the result), the data that was passed to them perhaps, and a way to clear the results?

Yes, so RenderedResults is an iterable of Entry<String, List<RenderedResult>> where the string is the template id.

There's also RenderedResults#clear() which removes all results. We could even add a variant with a Predicate to test the template id. Or even some shortcut methods to assert the results...

This way you could write a test asserting that 2 HTML templates and 1 email template were were rendered as part of a single action, and check their IDs and data.

Yes, that should be possible. Except that I need to check if mailer is using injected templates as well.

Honestly I'm not entirely sure of why this is going to be useful, but it seems a better API than the current PR, no? Provided there's a use for this sort of assertion…

I like the API and idea TBH ;-). So let's do it. I will add some more tests and then I'll ping @gbourant to test this approach...

FroMage commented 4 months ago

Excellent! Thanks a lot!

mkouba commented 4 months ago

Ping @gbourant, the PR is here: https://github.com/quarkusio/quarkus/pull/38954

FroMage commented 4 months ago

Great. Can you try it @gbourant and if it fits your use-case, close this?

gbourant commented 4 months ago

Let me build mkouba's branch and i'll tell you.

gbourant commented 4 months ago

I think we can add the isFragment field at the RenderedResult record.

Also if i'm not mistaken it does not work with fragments. I did debugged it a bit and the RenderedResults#idToResults is empty when my controller just returns a fragment.

I have the following @CheckedTemplate

@CheckedTemplate
public class AdminTemplate {
    public static native TemplateInstance auth();
    public static native TemplateInstance auth$authForm();
}

The result of renderedResults.getResults("auth.html") contains the template and works as expected. The result of renderedResults.getResults("auth.html.authForm") should contain the fragment.

Last but not least, maybe change the default value of quarkus.qute.test-mode.record-rendered-results to false? If not it will make the tests slower for those who do not assert the templates.

mkouba commented 4 months ago

Also if i'm not mistaken it does not work with fragments. I did debugged it a bit and the RenderedResults#idToResults is empty when my controller just returns a fragment.

TBH I did not test fragments... will take a look.

Last but not least, maybe change the default value of quarkus.qute.test-mode.record-rendered-results to false? If not it will make the tests slower for those who do not assert the templates.

It depends. If it's not enabled by default then it won't be used... OTOH it might slow down the tests... although I don't think a user would ever notice in most cases (unless you render thousands of templates).

mkouba commented 4 months ago

@gbourant I've force-pushed the branch. Type-safe fragments should work now, except that the template id would be auth.html$authForm in your case.

gbourant commented 4 months ago

Indeed, now it's working with fragments but i had to use authForm instead of auth.html$authForm as you mentioned.

P.S. After your update I only did built and installed the Qute extension.

mkouba commented 4 months ago

P.S. After your update I only did built and installed the Qute extension.

You need to build both the qute-core and qute extension because the template id is extracted in RenderedResults: https://github.com/quarkusio/quarkus/pull/38954/files#diff-93dc35c292b0af98816ef53f0dceb356de566e198d70cebd3ca0a39f12aa28d4R49-R53.

gbourant commented 4 months ago

Yeap, after a full build it works with auth.html$authForm.

FroMage commented 4 months ago

Well, if the new API is great, I'll close this PR then :) Thanks everyone!