Dev UI: Add strictRendering=false to Qute engine

[phillip-kruger]

Fix #18413

Signed-off-by:Phillip Kruger [email protected]

[sberyozkin]

@phillip-kruger Thanks for a quick fix :-), LGTM, though indeed lets wait for more approvals before merging

[gastaldi]

I think that's okay for the DevConsole, but I'll let @mkouba have the final say here 😉

[gsmet]

I'm not sure it's the right fix. Why is the property not defined? I would rather fix the issue with this property rather than changing this globally (and thus miss potential errors).

[phillip-kruger]

The default has changed (see #18227) but I can investigate some more.

[sberyozkin]

@gsmet This fix only affects the dev console templates, it is not doing it globally - the user templates are not affected

[stuartwdouglas]

@mkouba was the intention of this to completely ban null values? We do:

                TemplateInstance devTemplateInstance = devTemplate
                        .data("currentExtensionName", extName)
                        .data("flash", FlashScopeUtil.getFlash(ctx))
                        .data("currentRequest", ctx.request());

where extName may be null, but there is now no way to check for a null value in the template.

[phillip-kruger]

Some more info: I am not sure if what case that var is null. In my basic tests there is always a value.
We use it when we build the page to create the title and the heading. In the template we check it like this:

{#if currentExtensionName}

So if it would be null, the title will be a default value.

[geoand]

If null can't be used, then what should be used and can be checked for in the templates?

[stuartwdouglas]

I think we should merge this for now, as it is better than the current state of affairs, however I do have reservations about making strict rendering the default, as it makes working with null values almost impossible.

I have pushed a smoke test to this PR so we don't accidentally break the main page again, when it passes we should merge it.

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 7376972

Status	Name	Step	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11	Build	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11 Windows	Build	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 16	Build	Test failures	Logs	Raw logs
✖	Native Tests - Misc4	Build	⚠️ Check →	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/vertx-http/deployment

✖ io.quarkus.vertx.http.devconsole.DevConsoleSmokeTest.testDevConsoleNotBroken line 23 - More details - Source on GitHub

---

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/vertx-http/deployment

✖ io.quarkus.vertx.http.devconsole.DevConsoleSmokeTest.testDevConsoleNotBroken line 23 - More details - Source on GitHub

---

⚙️ JVM Tests - JDK 16 #

📦 extensions/vertx-http/deployment

✖ io.quarkus.vertx.http.devconsole.DevConsoleSmokeTest.testDevConsoleNotBroken line 23 - More details - Source on GitHub

[mkouba]

@mkouba was the intention of this to completely ban null values?

@stuartwdouglas Not at all. null is a valid value that should be treated as falsy in the {#if} section. Strict rendering is about throwing an exception if no value is found, i.e. if the engine was not able to evaluate the expression correctly. Previously, we did throw an exception for "standalone" expressions, e.g. {foo.bar}. strictRendering=false means that we extend this behavior to any expression used, including section params such as {#if foo.bar}.

I need to check the original error though. It might be that we have some bug in there. In general, if a map has a key=null pair then {#if map.key}nok{#else}null{/if} should render null.

[mkouba]

I'm not sure it's the right fix. Why is the property not defined? I would rather fix the issue with this property rather than changing this globally (and thus miss potential errors).

@gsmet I'm going to create a separate issue to verify all DEV UI templates and switch to strict rendering eventually. For now it's probably safer to disable it globally.

[gsmet]

OK yeah, let's fix the issues first and reconsider later.

But what I would like is to enable it at some point and ALSO to have simple tests for Dev UI just checking that the pages of every extension don't return a 500. That should be relatively easy and allow to catch at least some errors with a small investment.

[sberyozkin]

@gsmet, So can your change request be dismissed at this stage ?

Let's do this for now.