Qute enhancements

[FroMage]

Finally found the time to follow up on the discussions we had at the last f2f, here's my proposal for improving UX for templating.

ATM, in order to do a non-type-safe template you need this template in templates/hello.txt:

Salut {name}!

And this controller:

@Path("hello")
public class HelloResource {
    @Inject
    Template hello;

    @GET
    public TemplateInstance get(@QueryParam String name) {
        return hello.data("name", name);
    }
}

My proposal for non-type-safe templates is to move the template to templates/HelloResource/hello.txt and write the following code:

@Path("hello")
public class HelloResource {
    @GET
    public TemplateInstance hello(@QueryParam String name) {
// we can change the class/method name, add a static import, or move it to a superinterface to get it in scope easier
        return ResteasyTemplate.data("name", name);
    }
}

This will default to using a template under {className}/{methodName}, and will (this is still missing) take an optional template name as parameter if you want to override it.

Type-safe templates

On the other hand (and orthogonally), ATM if you want type-safe templates you need to write this template:

{@java.lang.String name}
Salut {name.toLowerCase()}!

While the invocation (in any style: old or new) is not ever type-safe, letting you do this:

@Path("hello")
public class HelloResource {
    @GET
    public TemplateInstance hello(@QueryParam String name) {
        return ResteasyTemplate.data("name", 23); // will fail at run-time
    }
}

I propose the following variant, guaranteeing type-safety as discussed at the f2f:

The template doesn't have types in there:

Salut {name.toLowerCase()}!

And the template declares a constructor for the template instance:

@Path("hello")
public class HelloResource {
    public static class hello extends CheckedTemplateInstance {
        public hello(String name) {
        }
    }

    @GET
    public TemplateInstance hello(@QueryParam String name) {
        return new hello(name); // this is type-safe
    }
}

This is implemented in this branch and a little bit tested.

Opinions @mkouba @maxandersen @emmanuelbernard @cescoffier @nicmarti ?

[FroMage]

Forgot @gavinking :)

[FroMage]

Now with 100% more correct formatting.

[FroMage]

Noone? @mkouba @maxandersen @emmanuelbernard @cescoffier @nicmarti

[mkouba]

@FroMage I'll take a look later this week. Anyway, I don't like the fact that the template itself will not declare the type info...

[maxandersen]

@FroMage I'll take a look later this week. Anyway, I don't like the fact that the template itself will not declare the type info...

As I grok this, the declaration of type info in templates are still optional as it is today thus I see this as just a java compatible way of declaring the typeinfo and if you also have typeinfo in the template and they are incompatible I would expect a failure.

also this feels like something that would fit nicely with java 14 records.

@FroMage remind me again why we use "extend CheckedTemplateInstance" and not a annotation ?

[FroMage]

@FroMage remind me again why we use "extend CheckedTemplateInstance" and not a annotation ?

Because we need to be able to return an instance of this type that can later be checked by the renderer filter to see if it can render it. Although I could inject the supertype based on an annotation.

So yeah, I could do this with an annotation too, I'm just too used to RAILS working with subtyping. Do you think it'd be better?

[FroMage]

@FroMage I'll take a look later this week. Anyway, I don't like the fact that the template itself will not declare the type info...

Oh come on, to be fair, you don't like the type info at all ;)

[mkouba]

Oh come on, to be fair, you don't like the type info at all ;)

I got used to it ;-).

I have few questions...

1. ResteasyTemplate.data("name", name) looks good but the path {className}/{methodName} would not work once there are multiple resource classes with the same name which is legal although not very common.
2. The CheckedTemplateInstance approach looks also good but there are few things I'm not excited about. The template itself does not contain the type info which is imho not a win, esp. for someone working on the template side. Also the fact that one has to create an empty public constructor seems weird to me. Not to mention that a class name starting with lowercase hurts my eyes!

I'll take a look at the implementation and will try to play with the API. It could be that my concerns are just unfounded...

[maxandersen]

So yeah, I could do this with an annotation too, I'm just too used to RAILS working with subtyping. Do you think it'd be better?

I like ducktyping from rails too :) so at least having annotation as an option would let me put it on something that is not necessarily a CheckedTemplateInstance but simply just any bean with a proper constructor and bean/field accessors.

[FroMage]

ResteasyTemplate.data("name", name) looks good but the path {className}/{methodName} would not work once there are multiple resource classes with the same name which is legal although not very common.

Yeah, but we don't want to encourage that, or support FQDN paths. We should just tell people to use a single .controller package by convention and detect clashes at build time.

The CheckedTemplateInstance approach looks also good but there are few things I'm not excited about. The template itself does not contain the type info which is imho not a win, esp. for someone working on the template side. Also the fact that one has to create an empty public constructor seems weird to me. Not to mention that a class name starting with lowercase hurts my eyes!

I'm not sure about the value of having the type info itself in the template, unless it can be used by the IDE (though I'm sure the IDE can use the type info from the type class too). Plus it's super hard to write since the IDE doesn't help you and you have to type the FQN.

The empty constructor is the only type-safe trick I could think of. If you have better I'll take it.

As for the lowercase class name:

More seriously, we can make them uppercase if you want, but I found lowercase to be more intuitive when it came to making it clear they were method-related.

[FroMage]

having annotation as an option would let me put it on something that is not necessarily a CheckedTemplateInstance but simply just any bean with a proper constructor and bean/field accessors.

Why would you need to do that? Those constructors and accessors would never be used. The class is just a placeholder, the only kind we could think of to declare a list of template parameters in Java.

[mkouba]

We should just tell people to use a single .controller package by convention and detect clashes at build time.

+1

I'm not sure about the value of having the type info itself in the template...

The value is that once you open a template the presence of a type info is a hint that tells you whether you need to care about about types or not.

The empty constructor is the only type-safe trick I could think of. If you have better I'll take it.

I have none ;-).

More seriously, we can make them uppercase if you want...

Well, that's a matter of taste and users can choose their own convention.

BTW I also think we don't need annotation for this.

[FroMage]

Well, that's a matter of taste and users can choose their own convention.

Litteraly the ONLY thing that makes Convention-Over-Configuration work is that users don't choose their own conventions ;)

[mkouba]

@FroMage Yeah, sorry "convention" was a poor choice. It should be "style" or something like that... ;-). I know that you'd like to avoid any name transformations so that it's 1:1 but this would only work for simple template names like hello. But what about item-detail.html, footer-old-nobanner.html, etc. For these cases itemDetail/ItemDetail -> item-detail would be probably more appropriate...

[FroMage]

Sure, we can support that. But I'm not sure we need to. I'd rather tell people to name their files itemDetail.html.

[mkouba]

Fair enough. From what I see the CheckedTemplateInstance approach does not work outside a JAX-RS resource? Is it intentional? I mean it could be useful even without JAX-RS. Just declare a top-level class that extends the CheckedTemplateInstance etc?

[FroMage]

We could make it work outside a jax-rs resource, indeed.

[maxandersen]

itemDetail.html

we use kebab case everywhere else (except java class names); why are we suddenly wanting users to use camel case in file names ?

[maxandersen]

I'm not sure about the value of having the type info itself in the template, unless it can be used by the IDE (though I'm sure the IDE can use the type info from the type class too). Plus it's super hard to write since the IDE doesn't help you and you have to type the FQN.

The IDE can be enhanced to give content assist and verification when its in the template - just saying.

[stuartwdouglas]

Just throwing this out there, but another approach you could use is annotated interfaces.

For a templates in templates/MyTemplates/hello.txt

@TemplateController
class MyTemplates {
  TemplateInstance hello(String name);
}

You can then just inject this template controller anywhere you want, and use it in a type safe manner.

[FroMage]

You can then just inject this template controller anywhere you want, and use it in a type safe manner.

This does replace one injection point per method with one per class, but I like the idea, and it's led me to come up with another idea that does away with injection altogether. I'm pretty sure we can use the native trick to do this:

@Path("hello")
public class HelloResource {
    private native TemplateInstance renderHello(String name);
    private native TemplateInstance renderGoodbye(String name);

    @GET
    public TemplateInstance hello(@QueryParam String name) {
        return renderHello(name); // this is type-safe
    }

    @Path("bye")
    @GET
    public TemplateInstance goodbye(@QueryParam String name) {
        return renderGoodbye(name); // this is type-safe
    }
}

It's shorter than my constructors proposal and doesn't require injection. I think they can even be static to allow usage from other classes.

[mkouba]

How does this native trick work? In any case, I believe we should try to to avoid any tricks if possible...

This does replace one injection point per method with one per class...

I like Stuart's proposal. The "controller" could be used for all templates in your app. We should at least explore this possibility.

@FroMage BTW the methods in your HelloResource should return CheckedTemplateInstance because it does not implement TemplateInstance.

[FroMage]

How does this native trick work?

It works by us replacing the bytecode of the method with a normal method. It's pretty much the only way to declare a method without a body unless it's an abstract class or interface. I think @gunnarmorling showed it to me, IIRC.

In any case, I believe we should try to to avoid any tricks if possible...

Well, the entire Panache APIs are based on using any trick necessary to make UX better, so I disagree :)

BTW the methods in your HelloResource should return CheckedTemplateInstance because it does not implement TemplateInstance.

Ah yes.

[mkouba]

Well, the entire Panache APIs are based on using any trick necessary to make UX better...

I disagree. I don't think that Panache users are not encouraged to use native which is a keyword with a well-defined meaning AFAIK. Less code does not always mean better.

[gunnarmorling]

I think @gunnarmorling showed it to me, IIRC.

I wish I could claim ownership, but it wasn't my idea :) Might be an interesting effort though to make it a small JEP to have an official keyword for body-less methods in non-abstract classes?

[emmanuelbernard]

It was @gavinking or I that proposed native if I recall (which I clearly don't so this message is just waste of bandwidth).

[FroMage]

New:

• Verify that native methods have at least one template variant present
• Verify that native methods are static and a valid return type
• Support native methods for emails
• Support native method template variants
• Moved Panache's JandexUtil into quarkus-core JandexUtil and (new) AsmUtil (this was required because those methods are useful everywhere, as discussed on quarkus-dev)
• Support boxing/primitives in native method parameters
• Removed the constructor variant
• Support toplevel templates

Now in theory I've only got documentation to write before this is ready.

[FroMage]

I've added docs.

I'm not quite sure on which feet to dance about it, because we have conflicting opinions as to what should be the default style, the one we should start with.

ATM, I've updated the docs in this manner:

• how to use templates using @Inject (no parameter validation)
• how to declare templates with @CheckedTemplate (type-safe declaration but no parameter validation)
• how to declare type-safe parameters with @CheckedTemplate (type-safe declaration and type-safe parameters)
• how to declare type-safe parameters using @Inject and parameter declarations in the template (type-safe parameters)

As a result, I've moved the template extension docs to their own section, and added some text about the convention of grouping them in a single class, or by target.

I've also updated the mailer docs to mention the type-safe declaration first @cescoffier.

Please @mkouba @stuartwdouglas @emmanuelbernard @cescoffier can you take a look at the docs and let me know if I can improve it: I'm really not sure how to organise it so it's not overly verbose (there's a lot of overlap between the alternative approaches)?

[FroMage]

Rebased due to the JandexUtil commit having been merged in master.

[FroMage]

Rebased yet again.

[mkouba]

I added few comments. It looks good (although I'm still against using the native modifier in the API ;-). I still need to checkout the code and play with the API a little.

[FroMage]

All updated and rebased. Let's see what CI says.

[FroMage]

Windows failure doesn't seem related:

2020-06-16T17:13:08.1485725Z 2020-06-16 17:13:08,136 INFO  [io.quarkus] (Quarkus Main Thread) acme 1.0-SNAPSHOT on JVM (powered by Quarkus 999-SNAPSHOT) started in 0.476s. Listening on: http://0.0.0.0:8080
2020-06-16T17:13:08.1486507Z 2020-06-16 17:13:08,137 INFO  [io.quarkus] (Quarkus Main Thread) Profile dev activated. Live Coding activated.
2020-06-16T17:13:08.1487990Z 2020-06-16 17:13:08,139 INFO  [io.quarkus] (Quarkus Main Thread) Installed features: [cdi, resteasy, servlet, undertow-websockets]
2020-06-16T17:13:08.1488358Z 2020-06-16 17:13:08,139 INFO  [io.qua.dep.dev] (vert.x-worker-thread-3) Hot replace total time: 1.059s 
2020-06-16T17:13:11.8111125Z [WARNING] An exceptionCaught() event was fired, and it reached at the tail of the pipeline. It usually means the last handler in the pipeline did not handle the exception.
2020-06-16T17:13:11.8124136Z java.io.IOException: An existing connection was forcibly closed by the remote host
2020-06-16T17:13:11.8130607Z     at sun.nio.ch.SocketDispatcher.read0 (Native Method)
2020-06-16T17:13:11.8157386Z     at sun.nio.ch.SocketDispatcher.read (SocketDispatcher.java:43)
2020-06-16T17:13:11.8180653Z     at sun.nio.ch.IOUtil.readIntoNativeBuffer (IOUtil.java:276)
2020-06-16T17:13:11.8194447Z     at sun.nio.ch.IOUtil.read (IOUtil.java:233)
2020-06-16T17:13:11.8196698Z     at sun.nio.ch.IOUtil.read (IOUtil.java:223)
2020-06-16T17:13:11.8217621Z     at sun.nio.ch.SocketChannelImpl.read (SocketChannelImpl.java:358)
2020-06-16T17:13:11.8224839Z     at io.netty.buffer.PooledByteBuf.setBytes (PooledByteBuf.java:253)
2020-06-16T17:13:11.8227337Z     at io.netty.buffer.AbstractByteBuf.writeBytes (AbstractByteBuf.java:1133)
2020-06-16T17:13:11.8237364Z     at io.netty.channel.socket.nio.NioSocketChannel.doReadBytes (NioSocketChannel.java:350)
2020-06-16T17:13:11.8265315Z     at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read (AbstractNioByteChannel.java:148)
2020-06-16T17:13:11.8285478Z     at io.netty.channel.nio.NioEventLoop.processSelectedKey (NioEventLoop.java:714)
2020-06-16T17:13:11.8287157Z     at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized (NioEventLoop.java:650)
2020-06-16T17:13:11.8343758Z     at io.netty.channel.nio.NioEventLoop.processSelectedKeys (NioEventLoop.java:576)
2020-06-16T17:13:11.8502849Z     at io.netty.channel.nio.NioEventLoop.run (NioEventLoop.java:493)
2020-06-16T17:13:11.8505994Z     at io.netty.util.concurrent.SingleThreadEventExecutor$4.run (SingleThreadEventExecutor.java:989)
2020-06-16T17:13:11.8677402Z     at io.netty.util.internal.ThreadExecutorMap$2.run (ThreadExecutorMap.java:74)
2020-06-16T17:13:11.8679231Z     at io.netty.util.concurrent.FastThreadLocalRunnable.run (FastThreadLocalRunnable.java:30)
2020-06-16T17:13:11.9055838Z     at java.lang.Thread.run (Thread.java:834)
2020-06-16T17:13:12.5671453Z Connection closed CloseReason[1001]

@mkouba can you rereview and we get this PR behind us?

[mkouba]

@FroMage could you pls squash the commits? It's not possible to do "squash and merge" in the quarkus repo.

[FroMage]

Done.

[mkouba]

Awesome, thanks!

[gsmet]

@mkouba does this one break something for the users?

[mkouba]

@gsmet AFAIK it should not. Also for qute we don't care that much about compatibility because it's still an experimental feature.