wimdeblauwe / htmx-spring-boot

Spring Boot and Thymeleaf helpers for working with htmx
Apache License 2.0
436 stars 41 forks source link

@HxTrigger & @ResponseBody cannot work together (due to postHandle()) #42

Closed credmond closed 1 year ago

credmond commented 1 year ago

Hi -- firstly, great library -- thank you!

I have noticed an issue, however. I'm using a fairly typical Spring Boot + Thymeleaf setup, using Spring Boot 3.1.0-M1.

As an example:

    @HxTrigger("this-never-gets-set")
    @GetMapping("/cormac")
    @ResponseBody
    public String cormac(Model model, HttpServletResponse response) {
        response.setHeader("this-will", "work-okay");
        return "irrelevant";
    }

I am showing via code where I can set a header manually (i.e., this will work), but HxTrigger does NOT ultimately manage to set a header. It does try though (so that's fine), but, down the stack the header setting gets ignored because the request is actually committed already, and so the final response.setHeader(...) (after all the wrappers) doesn't ultimately get called. E.g., isCommitted() is true here in org.apache.catalina.connector.ResponseFacade's:

    @Override
    public void setHeader(String name, String value) {
        if (isCommitted()) {
            return;
        }

        response.setHeader(name, value);
    }

Ultimately, if you keep following this, you can see that the underlying Response.java's committed boolean is true.

HandlerInterceptorAdapters will not work with @ResponseBody and ResponseEntitymethods because they are handled by StringHttpMessageConverter, which writes to and commits the response before postHandle(...) is ever called, which basically makes it impossible to change the response (although, it doesn't complain -- stuff just silently gets ignored).

I do notice you have a @ResponseBody test, but I think without a more realistic Spring context setup, you wouldn't see this issue in a what's basically unit test.

Check here, some people chatting about very similar issues:

https://github.com/spring-projects/spring-framework/issues/13864 https://stackoverflow.com/questions/48823794/spring-interceptor-doesnt-add-header-to-restcontroller-services

So, I think preHandle(...) is the way to go and your use-case (just checking for annotated methods) is pretty simple, so I don't see any downsides to it or any need to do this in postHandle(...).

Update: I opened a PR with the suggested change.

checketts commented 1 year ago

I'm curious how you are using @ResponseBody with HTMX. Or is this more just a general improvement of the library?

credmond commented 1 year ago

Not for anything useful, actually. Sometimes just to return a "" or void (same outcome). Without @ResponseBody, Thymeleaf assumes the response refers to a template name and goes looking for it.

In this example, I simply wanted HTMX to make a request, and I wanted a HX-Trigger in the response header to cause something else to happen somewhere else. So I wanted the header, but no actual response body (no template or partial or hx-swap or anything like that).

But you could also use it for practical purposes to just return a piece of text or JSON, rather than have Thymeleaf involved at all; for certain cases.

Besides that, there is a test in:

HtmxHandlerInterceptorTest

... which uses:

TestController

...implying it's supported and has been considered before:

    @GetMapping("/with-trigger")
    @HxTrigger("eventTriggered")
    @ResponseBody
    public String methodWithHxTrigger() {
        return "";
    }

I would say almost nobody uses @ResponseBody though, with this library (until myself!) , and so the "bug" hasn't been obvious.

wimdeblauwe commented 1 year ago

As an alternative, you can try this:

 @GetMapping("/with-trigger")
    @HxTrigger("eventTriggered")
    @ResponseStatus(OK)
    public void methodWithHxTrigger() {

    }

So use a void return type and add the @ResponseStatus annotation. According to the docs this should work:

A method with a void return type (or null return value) is considered to have fully handled the response if it also has a ServletResponse, an OutputStream argument, or an @ResponseStatus annotation.

credmond commented 1 year ago

Yep, that might cause HxTrigger to behave correctly -- but what if you do just want to return some text though -- which can be useful sometimes...?

I still think the PR is a proper and simple/safe fix: https://github.com/wimdeblauwe/htmx-spring-boot-thymeleaf/pull/43

wimdeblauwe commented 1 year ago

This has been released in version 2.0.1. I am sorry it took so long to find the time to release this. Thank you for contributing this!

credmond commented 1 year ago

Cheers!