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

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

Closed
credmond opened this issue Mar 30, 2023 · 6 comments
Closed

Comments

@credmond
Copy link
Contributor

credmond commented Mar 30, 2023

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 ResponseEntity methods 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:

spring-projects/spring-framework#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.

@credmond credmond changed the title @HxTrigger's setHeader() never actually gets set when @ResponseBody @HxTrigger & @ResponseBody cannot work together (due to postHandle()) Mar 30, 2023
credmond added a commit to credmond/htmx-spring-boot-thymeleaf that referenced this issue Mar 30, 2023
…andle to be compatible with @responsebody

Due to the nature of how ResponseBody is processed, a request is committed (therefore silently immutable) *before* an interceptor's postHandle is ever called. Therefore, these headers were being lost in this ResponseBody situation (and maybe other unknowns). The fix is to set the headers instead in preHandle(), as it makes no difference to existing functionality but fixes this issue.
@checketts
Copy link
Collaborator

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

@credmond
Copy link
Contributor Author

credmond commented Mar 31, 2023

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
Copy link
Owner

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
Copy link
Contributor Author

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: #43

wimdeblauwe added a commit that referenced this issue Jun 4, 2023
…to-response-committed

Issue #42: Moving HxTrigger and HxRefresh handling to preHandle to be…
@wimdeblauwe
Copy link
Owner

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
Copy link
Contributor Author

credmond commented Jun 4, 2023

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants