[#233] Reactivating the Thymeleaf extension

[jelemux]

This reenables the Thymeleaf extension by using its 3.1.0.M1 milestone release (which uses the jakarta namespace).

To adapt to changes in Thymeleaf, CDIWebContext had to be changed to use the jakarta namespace and the new IWebExchange. Since JakartaServletWebApplication has been established to wrap ServletContext in Thymeleaf, there needed to be a producer to inject it into both DefaultTemplateEngineProducer and ThymeleafViewEngine. But because JakartaServletWebApplication is a final class, a wrapper for it had to be written in order to use CDI.

Fixes #233

[chkal]

I'm currently creating two instances of JakartaServletWebApplication (for DefaultTemplateEngineProducer and ThymeleafViewEngine respectively) which is not pretty, but it should not be the problem here.

Maybe we could simply "produce" an application-scoped instance of JakartaServletWebApplication and use it in both classes? Not sure if this works, as I didn't have a detailed look at the code yet.

The problem is that somehow the request from the ViewEngineContext has a ServletContext different from the one injected into the ViewEngine and Thymeleaf doesn't like that at all.

It is just a guess, but if we use @Inject ServletContext, we will most likely get a CDI proxy class instead of the real ServletContext instance. I guess there are a few ways to get a reference to the context:

// (A) Directly inject via CDI, but I guess you will get a wrapper
@Inject 
private ServletContext servletContext;

// (B) Inject the request and then call request.getServletContext(). Should return the "real" servlet context?
@Inject 
private HttpServletRequest request;

// (C) Like (B) but Krazo spezific and will return the same request as @Context HttpServletRequest
@Inject 
@JaxRsContext
private HttpServletRequest request;

// (D) via ViewEngineContext.
viewEngineContext.getRequest(HttpServletRequest.class).getServletContext()

To make things more complicated: HttpServletRequests can be wrapped via servlet filter. We actually even have some special handling in Krazo to "unwrap" these wrappers. The result is what you will get from ViewEngineContext. See:

krazo/core/src/main/java/org/eclipse/krazo/core/ViewableWriter.java

Lines 136 to 137 in d2467da

	HttpServletRequest request = unwrapOriginalRequest(injectedRequest);
	HttpServletResponse response = unwrapOriginalResponse(injectedResponse);

Maybe I'll find some time tomorrow to have a look at your branch.

[jelemux]

Maybe we could simply "produce" an application-scoped instance of JakartaServletWebApplication and use it in both classes? Not sure if this works, as I didn't have a detailed look at the code yet.

JakartaServletWebApplication is a final class so it cannot be proxied by CDI.
To solve this, I see three possibilities:

1. Create a wrapper object for JakartaServletWebApplication and produce that.
2. Create an issue/PR with Thymeleaf asking them to remove the final modifier.
3. Create an issue/PR with Thymeleaf including the buildExchange() method (which we need) in IServletWebApplication. That way we could just produce and inject that.
   (This would add the need for a wrapper around HttpServletRequest and HttpServletResponse in Thymeleaf.)

It is just a guess, but if we use @Inject ServletContext, we will most likely get a CDI proxy class instead of the real ServletContext instance.

Thanks, that seems to be the case. My current (local) codebase uses option D and that works.

[jelemux]

I now have it in a working state and pushed the changes. Have a look and tell me what you think.

[jelemux]

I don't like my current solution at all.
I'll try solution 3:

3. Create an issue/PR with Thymeleaf including the buildExchange() method (which we need) in IServletWebApplication.

[erdlet]

@chkal after a look into the PR I'm afraid we need a CQ because of the minor update, right? Do you know if we need one for the final 3.1.0 again after creating one for 3.1.0.M1?

[erdlet]

I don't like my current solution at all. I'll try solution 3:

3. Create an issue/PR with Thymeleaf including the buildExchange() method (which we need) in IServletWebApplication.

Good idea, thanks for doing that 🍻

[jelemux]

As we agreed yesterday in the MVC Spec Meeting: Since there is no reaction from Thymeleaf maintainers to my PR, we'll just merge this current solution.

The only open point is the CQ that @erdlet pointed out.

@chkal after a look into the PR I'm afraid we need a CQ because of the minor update, right? Do you know if we need one for the final 3.1.0 again after creating one for 3.1.0.M1?

[chkal]

Hey @jelemux. Sorry for the super long delay. I finally found some time to review your changes.

Overall, it looks great. I just added a few comments, but I would also be fine with merging the current state of the PR. Feel free to have a look at my comments. :-)

[chkal]

Maybe we should add the @Internal qualifier here? We use this qualifier for some beans used internally by Krazo to protect ourselves against potential conflicts with other libraries which produce the same type as well. We use it for HttpServletRequest for example.

I'm not super sure if we should do it here as well. I just wanted to mention it. :-)

[jelemux]

I tried to understand what the @Internal qualifier does but I don't know if I fully get it yet.
If using it means that the user cannot provide their own producer to override ours, then I would refrain from using it because it might be useful for the user to configure JakartaServletWebApplication themselves.

Also if you were afraid that the user might want to inject a JakartaServletWebApplicationWrapper somewhere in their code using their own producer: I don't think that this should happen as JakartaServletWebApplicationWrapper was written by us specifically for the purpose we are using it for and nothing else.

I hope that clarified things and that I correctly understood what @Internal does.

[erdlet]

CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23960 created.

[erdlet]

CQ is approved, so we can finally merge this PR :)

@jelemux please let me known if you want to squash something before merge or if everything is fine for you.

[jelemux]

I already squashed everything, so should be good to go for the merge