Introduce Qute templating extensions #5793

mkouba · 2019-11-27T09:01:15Z

qute core as an independent project
qute core extension
qute resteasy extension
first version of guide

NOTE: Extensions are considered preview.

gastaldi

LGTM

docs/src/main/asciidoc/qute.adoc

gsmet · 2019-11-27T12:20:25Z

This PR is 134 files. Marking it as waiting-for-ci 3 hours after its opening is clearly not going to fly.

Please let more people review this stuff as it's a significant addition.

gsmet · 2019-11-27T12:21:28Z

I'll have a look myself before the end of the week but I still have a couple of other PRs to review first.

mkouba · 2019-11-27T12:30:06Z

This PR is 134 files.

@gsmet Yes, and unfortunately it can't be reduced.

Marking it as waiting-for-ci 3 hours after its opening is clearly not going to fly.

That's how @gastaldi works. You have to forgive him ;-). I think that nobody expected that this PR is merged immediately after the CI is green. However, 1) Most of the stuff was already discussed separately, 2) this is the first PR related to Qute but definitely not the last one - the extension is marked as preview and I'd like to merge the current state ASAP and improve what we have later because I see no benefits in discussing PRs for weeks.

gsmet · 2019-11-27T12:34:48Z

because I see no benefits in discussing PRs for weeks

It's not about discussing for weeks, it's about reviewing the code and the documentation. I'm not saying we should hold this PR to have more design discussions if it wasn't clear.

But we need a proper code review.

I'm just asking for others - including me - to have a chance at reviewing.

mkouba · 2019-11-27T12:39:22Z

I'm just asking for others - including me - to have a chance at reviewing.

No problem. And I think that nobody is against.

Build failures

gsmet · 2019-11-27T14:02:14Z

Btw, my remark on the size of the PR wasn't negative. I know it's a massive addition so no wonder it's a massive piece of code.

Just to clarify, adding waiting-for-ci basically means: please merge as soon as CI is green. So it should only be used in those specific cases.

gastaldi · 2019-11-27T14:05:48Z

@mkouba is there a reason why META-INF/resources/templates was chosen? Because the raw templates are publicly available in the webapp (eg. http://localhost:8080/templates/hello.txt)

gastaldi · 2019-11-27T14:07:06Z

Is there any way to fetch the raw template contents from Template?

FroMage · 2019-11-27T14:13:34Z

I'm not sure we have to review the Qute core code. The extension, sure, but the rest is an independent project. It's as if we asked to review the resteasy code when we bump the resteasy dependency version number.

docs/src/main/asciidoc/qute.adoc

gastaldi · 2019-11-27T14:25:26Z

The qute extension is never displayed in the list of loaded extensions: Installed features: [cdi, resteasy]

FroMage

LGTM: all the remarks and questions I have can be solved later in new issues.

docs/src/main/asciidoc/qute.adoc

FroMage · 2019-11-27T14:32:30Z

...qute-resteasy/deployment/src/test/java/io/quarkus/qute/resteasy/deployment/ItemResource.java

+public class ItemResource {
+
+    @Inject
+    VariantTemplate item;


I'm not sure why we need to make variants explicit here. It requires to modify the controller when we could just add a new template file and it could be picked up as a variant. The fact that more than one version exists should be enough to guide the implementation to know there are variants.

I disagree. You could have 2 files with different extensions that you want to use in different context.

I'm not totally sure, I would use the class to mark that but I think we need to have a way to enable it explicitly.

extensions/qute/runtime/src/main/java/io/quarkus/qute/runtime/DefaultTemplateExtensions.java

mkouba · 2019-11-27T16:56:57Z

I've renamed Rendering to TemplateInstance and added and renamed a few methods in the API. Tomorrow, I'd like to move the base location to src/main/resources/templates.

mkouba · 2019-11-27T18:32:55Z

It seems we have a problem with Windows and separators :-(. I'll try to address it tomorrow.

nimo23 · 2019-11-27T22:52:27Z

Why cannot the template engine and syntax be based on jakarta.el.jar (Jakarta Expression Language) ?

mkouba · 2019-11-28T11:42:33Z

The failure of the windows build is caused by #5821.

mkouba · 2019-11-28T15:13:57Z

@jaikiran now I can see a weird error:

[ERROR] io.quarkus.qute.deployment.TypeSafeLoopTest  Time elapsed: 0 s  <<< ERROR!
java.nio.file.DirectoryNotEmptyException: D:\a\1\s\extensions\qute\deployment\target\quarkus-unit-test8157101450501518519
java.nio.file.DirectoryNotEmptyException: D:\a\1\s\extensions\qute\deployment\target\quarkus-unit-test8157101450501518519
	at sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:266)
	at sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:103)
	at java.nio.file.Files.delete(Files.java:1126)
	at io.quarkus.test.QuarkusUnitTest$5.postVisitDirectory(QuarkusUnitTest.java:365)
	at io.quarkus.test.QuarkusUnitTest$5.postVisitDirectory(QuarkusUnitTest.java:345)
	at java.nio.file.Files.walkFileTree(Files.java:2688)
	at java.nio.file.Files.walkFileTree(Files.java:2742)
	at io.quarkus.test.QuarkusUnitTest.afterAll(QuarkusUnitTest.java:345)

AFAIK the directory should be empty (see https://github.com/quarkusio/quarkus/blob/master/test-framework/junit5-internal/src/main/java/io/quarkus/test/QuarkusUnitTest.java#L345-L368). I'll try to run a windows box to see what's really happening.

jaikiran · 2019-11-29T12:18:26Z

@mkouba, just a quick input - keeping aside those failed to delete issues (in the afterAll test methods), I think the real failures probably boil down to this logic here https://github.com/mkouba/quarkus/blob/qute-next/extensions/qute/deployment/src/main/java/io/quarkus/qute/deployment/QuteProcessor.java#L502

So the getPath in TemplateBuildItem is a Path that will be using OS/filesystem specific separator. Then these filesystem specific paths are compared against "resource" paths (which always is "/" separated - for example bar/bar.txt) here https://github.com/mkouba/quarkus/blob/qute-next/extensions/qute/deployment/src/main/java/io/quarkus/qute/deployment/QuteProcessor.java#L547 which I think gets into issues on Windows OS.

You know this code better than me, so ignore this input if it isn't right :) I had a few spare minutes and happened to find a Windows OS so just quickly ran the failing InjectionTest to see what might be the issue.

gsmet

I added some comments and questions.

There are a lot of very minor things but also a couple of more important interrogations.

bom/runtime/pom.xml

gsmet · 2019-11-29T10:08:03Z

core/deployment/src/main/java/io/quarkus/deployment/builditem/FeatureBuildItem.java

    public static final String REACTIVE_MYSQL_CLIENT = "reactive-mysql-client";
    public static final String NEO4J = "neo4j";
    public static final String OIDC = "oidc";
+    public static final String QUTE = "qute";


We don't advertise the RESTEasy one? There might be good reason not to, just asking.

A user will see [qute, resteasy]... I don't think there's a reason to add qute-resteasy or resteasy-qute.

Out of curiosity: Is it possible to automatically include qute-resteasy only if qute and resteasy are added to the project?

I don't think this could be done automatically ATM. But yes, it would make sense.

So... it's not that obvious to me.

If you have [qute, resteasy], that doesn't mean that you have the RESTEasy Qute support. The dependency could be missing and nothing will work.

So you could have two applications returning the exact same set of extensions and behaving differently. This is a problem IMHO.

And even for us or for support, when they will ask for the extension list, they will have a partial information.

Well, ignoring my comment doesn't magically resolve the issue ;)

I really think we should make it easy for support to know the list of installed extensions. As explained above, right now, they might have the same list of extensions and totally different behaviors.

RESTEasy Qute is a user exposed extension so it should be displayed here.

Well, ignoring my comment doesn't magically resolve the issue ;)

That's a pity ;-). Well, I didn't know we have resteasy-jsonb and others. I will add it but it looks weird. Just my 2c.

docs/src/main/asciidoc/qute.adoc

independent-projects/qute/core/src/main/java/io/quarkus/qute/MemberKey.java

independent-projects/qute/core/src/main/java/io/quarkus/qute/ReflectionValueResolver.java

independent-projects/qute/core/src/main/java/io/quarkus/qute/ValueResolvers.java

pom.xml

...easy/deployment/src/main/java/io/quarkus/qute/resteasy/deployment/QuteResteasyProcessor.java

mkouba · 2019-11-29T12:55:11Z

@jaikiran Yep, you're right. I also added some logging and found out that io.quarkus.runner.RuntimeClassLoader.createDefaultProtectionDomain(Path) always fails on windows because the windows separator is quoted:

java.net.URISyntaxException: Relative path in absolute URI: file:D:%5Ca%5C1%5Cs%5Cextensions%5Cmailer%5Cdeployment%5Ctarget%5Cquarkus-unit-test6389559223281079023

I'll fix the QuteProcessor first and we'll see if those DirectoryNotEmptyExceptions disappear ;-).

- qute core as an independent project - qute core extension - qute resteasy extension - first version of guide

…urce in RuntimeClassLoader

- also fix RuntimeClassLoader.createDefaultProtectionDomain(Path)

mkouba · 2019-12-02T14:10:32Z

@jaikiran it turns out that the DirectoryNotEmptyException was caused by the fact that we did not explicitly close the stream produced by java.nio.file.Files.list().

@gsmet this PR should be ready now.

I'm working on a next PR with reference documentation and few minor updates.

jaikiran · 2019-12-02T14:14:06Z

@jaikiran it turns out that the DirectoryNotEmptyException was caused by the fact that we did not explicitly close the stream produced by java.nio.file.Files.list().

Good to know :)

gsmet

@mkouba we are nearly there I think.

Could you have a look at my latest comments? It would be great if we could merge it tomorrow.

BTW, I'm wondering if it would be better to squash the resteasy-qute renaming commit? But maybe it was conflicting? If so don't bother.

gsmet · 2019-12-02T14:29:23Z

core/deployment/src/main/java/io/quarkus/deployment/builditem/FeatureBuildItem.java

    public static final String REACTIVE_MYSQL_CLIENT = "reactive-mysql-client";
    public static final String NEO4J = "neo4j";
    public static final String OIDC = "oidc";
+    public static final String QUTE = "qute";


Well, ignoring my comment doesn't magically resolve the issue ;)

I really think we should make it easy for support to know the list of installed extensions. As explained above, right now, they might have the same list of extensions and totally different behaviors.

RESTEasy Qute is a user exposed extension so it should be displayed here.

core/deployment/src/main/java/io/quarkus/runner/RuntimeClassLoader.java

extensions/qute/runtime/src/main/java/io/quarkus/qute/runtime/EngineProducer.java

...qute/deployment/src/main/java/io/quarkus/resteasy/qute/deployment/QuteResteasyProcessor.java

extensions/resteasy-qute/pom.xml

- plus other fixes

gsmet

OK, let's get this monster in :).

@mkouba thanks for your work and your patience :).

mkouba · 2019-12-02T19:12:18Z

The JPAFunctionalityInGraalITCase fails with:

2019-12-02 19:05:45,066 ERROR [org.hib.eng.jdb.spi.SqlExceptionHelper] (main) No timezone mapping entry for 'Etc/GMT'
javax.persistence.PersistenceException: [PersistenceUnit: templatePU] Unable to build Hibernate SessionFactory
	at io.quarkus.hibernate.orm.runtime.boot.FastBootEntityManagerFactoryBuilder.persistenceException(FastBootEntityManagerFactoryBuilder.java:113)
	at io.quarkus.hibernate.orm.runtime.boot.FastBootEntityManagerFactoryBuilder.build(FastBootEntityManagerFactoryBuilder.java:67)
	at io.quarkus.hibernate.orm.runtime.FastBootHibernatePersistenceProvider.createEntityManagerFactory(FastBootHibernatePersistenceProvider.java:54)
	at javax.persistence.Persistence.createEntityManagerFactory(Persistence.java:80)
	at javax.persistence.Persistence.createEntityManagerFactory(Persistence.java:55)
	at io.quarkus.hibernate.orm.runtime.JPAConfig$LazyPersistenceUnit.get(JPAConfig.java:109)
	at io.quarkus.hibernate.orm.runtime.JPAConfig.startAll(JPAConfig.java:57)
	at io.quarkus.hibernate.orm.runtime.HibernateOrmRecorder.startAllPersistenceUnits(HibernateOrmRecorder.java:82)
	at io.quarkus.deployment.steps.HibernateOrmProcessor$startPersistenceUnits182.deploy_0(HibernateOrmProcessor$startPersistenceUnits182.zig:70)
	at io.quarkus.deployment.steps.HibernateOrmProcessor$startPersistenceUnits182.deploy(HibernateOrmProcessor$startPersistenceUnits182.zig:36)
	at io.quarkus.runner.ApplicationImpl.doStart(ApplicationImpl.zig:134)
	at io.quarkus.runtime.Application.start(Application.java:87)
	at io.quarkus.runtime.Application.run(Application.java:210)
	at io.quarkus.runner.GeneratedMain.main(GeneratedMain.zig:41)
Caused by: org.hibernate.exception.GenericJDBCException: Unable to open JDBC Connection for DDL execution
	at org.hibernate.exception.internal.StandardSQLExceptionConverter.convert(StandardSQLExceptionConverter.java:47)
	at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:113)
	at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:99)
	at org.hibernate.resource.transaction.backend.jta.internal.DdlTransactionIsolatorJtaImpl.<init>(DdlTransactionIsolatorJtaImpl.java:62)
	at org.hibernate.resource.transaction.backend.jta.internal.JtaTransactionCoordinatorBuilderImpl.buildDdlTransactionIsolator(JtaTransactionCoordinatorBuilderImpl.java:46)
	at org.hibernate.tool.schema.internal.HibernateSchemaManagementTool.getDdlTransactionIsolator(HibernateSchemaManagementTool.java:175)
	at org.hibernate.tool.schema.internal.HibernateSchemaManagementTool.buildGenerationTargets(HibernateSchemaManagementTool.java:135)
	at org.hibernate.tool.schema.internal.SchemaDropperImpl.doDrop(SchemaDropperImpl.java:110)
	at org.hibernate.tool.schema.spi.SchemaManagementToolCoordinator.performDatabaseAction(SchemaManagementToolCoordinator.java:145)
	at org.hibernate.tool.schema.spi.SchemaManagementToolCoordinator.process(SchemaManagementToolCoordinator.java:73)
	at org.hibernate.internal.SessionFactoryImpl.<init>(SessionFactoryImpl.java:320)
	at org.hibernate.boot.internal.SessionFactoryBuilderImpl.build(SessionFactoryBuilderImpl.java:462)
	at io.quarkus.hibernate.orm.runtime.boot.FastBootEntityManagerFactoryBuilder.build(FastBootEntityManagerFactoryBuilder.java:65)
	... 12 more
Caused by: java.sql.SQLException: No timezone mapping entry for 'Etc/GMT'
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:129)

It's very likely unrelated. I'll try to re-run the test once again.

gsmet · 2019-12-02T22:32:34Z

Yes, it's unrelated. Looks like CI cache issue all over again.

mkouba force-pushed the qute-next branch from 97885b4 to 3812e3a Compare November 27, 2019 11:19

gastaldi previously approved these changes Nov 27, 2019

View reviewed changes

gastaldi reviewed Nov 27, 2019

View reviewed changes