Qute: display custom error page during dev mode #12907
Conversation
mkouba
commented
Oct 23, 2020
mkouba
commented
- resolves Qute: nice error page #8756
|
|
|
Looks like CI is not happy because of some formatting issues: |
|
@machi1990 Yeah, as usual in my case ;-) |
|
That is very ... well ... cute and it's gonna be very useful. I would maybe add a little more spacing between the details of the error and the code. And also between the end of the code and the second title. |
mkouba
force-pushed
the
issue-8756
branch
3 times, most recently
from
f753649
to
d0d8b3c
Compare
|
@stuartwdouglas |
|
@mkouba is this ready to be merged? Wasn't sure because of your last comment. |
|
It should be if there are no objections to the implementation... |
|
I will review this today if you let me. |
| @@ -76,6 +76,16 @@ public static String rootCauseFirstStackTrace(final Throwable exception) { | |||
| return generateStackTrace(modifiedRoot).replace("Caused by:", "Resulted in:"); | |||
| } | |||
|
|
|||
| public static Throwable getRootCause(Throwable exception) { | |||
| final List<Throwable> exceptionChain = new ArrayList<>(); | |||
| Throwable curr = exception; | |||
There was a problem hiding this comment.
That looks like the list is not useful, and it feels weird that if the exception has no cause it's not considered the root cause, since null will be returned.
There was a problem hiding this comment.
It should only return null if the exception is null, otherwise the list if never empty... However, we should handle the recursive cause structures as well. I'll fix it and add a test.
extensions/qute/runtime/src/main/java/io/quarkus/qute/runtime/devmode/QuteErrorPageSetup.java
Show resolved
Hide resolved
| * In order to avoid classloading issues the generators should not access the root cause directly but use reflection instead | ||
| * (the exception class could be loaded by a different class loader). | ||
| */ | ||
| public class ErrorPageGenerators { |
There was a problem hiding this comment.
This is obviously useful, but I wonder if we should not put something more declarative in place, like ExceptionMapper<T> which JAX-RS has?
There was a problem hiding this comment.
I think that this could cause class loading issues...
| private static final String TEMPLATE_EXCEPTION = "io.quarkus.qute.TemplateException"; | ||
| private static final String ORIGIN = "io.quarkus.qute.TemplateNode$Origin"; | ||
|
|
||
| private static final String PROBLEM_TEMPLATE = "" |
There was a problem hiding this comment.
Can we put this in a template file? I strongly hate that in Quarkus core we have HTML in Java strings, but I understand why, no matter how mind-numbing frustrating this is. But in Qute this feels wrong, when this can be in a standalone Qute template file. Or is there a reason I'm not seeing?
There was a problem hiding this comment.
I wasn't sure where to put the template contents. We could extract it later...
| } | ||
|
|
||
| String generatePage(Throwable exception) { | ||
| Escaper escaper = Escaper.builder().add('"', """).add('\'', "'") |
There was a problem hiding this comment.
Doesn't this look like generally useful for pretty much anything HTML? Shouldn't we have something like Escaper.htmlBuilder() or Escaper.builder().forHtml()?
There was a problem hiding this comment.
We have an optimized version of a result mapper in the runtime module: https://github.com/quarkusio/quarkus/blob/master/extensions/qute/runtime/src/main/java/io/quarkus/qute/runtime/HtmlEscaper.java
Maybe we could replace the generic escaper with this one. I'll try to prepare a follow-up PR...
| problems = Arrays.asList(suppressed); | ||
| } | ||
|
|
||
| String problemsFound = "Found " + problems.size() + " Qute problems"; |
There was a problem hiding this comment.
Can you please sort the errors by increasing location? You example screenshot shows the last error shown first and it's confusing :)
