[MAGNOLIA-6047] Provide possibility to use RenderExceptionHandler without RenderingContext Created: 23/Jan/15  Updated: 16/Feb/15  Resolved: 04/Feb/15

Status: Closed
Project: Magnolia
Component/s: rendering
Affects Version/s: None
Fix Version/s: 5.4

Type: Improvement Priority: Neutral
Reporter: Roman Kovařík Assignee: Roman Kovařík
Resolution: Fixed Votes: 0
Labels: m3
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
dependency
is depended upon by MSITEMESH-15 Render error when a fragment cannot b... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:
Epic Link: DPC

 Description   

We currently have RenderExceptionHandler which is able to render errors (e.g. from Freemarker). It's not possible to use it outside of rendering since informations from RenderingContext are not relevant.

Add method void handleException(RenderException renderException, Appendable out);



 Comments   
Comment by Magnolia International [ 29/Jan/15 ]
  • The deprecated constructor doing this.aggregationStateProvider = Components.getComponent(Provider.class) will never work.
    • It's not used (I thought it might be, in tests), so might as well just delete it.
  • aggregationState is used through a Provider to print the current URL in exception and log messages. I'd rather either pass the aggState to the method, or just not use it at all. (use log4j's MDC : %X{key}, see ContextFilter for available values)
  • It's also used to determine if we're in preview mode; while that's a more valid reason, I'm not sure we really need it. I'd rather either pass the aggState to the method, or just not use it at all. Code cleaner, testing easier.
  • The try/catch blocks in handleException() make the code a bit clumsy. I'd suggest simplifying that. And avoid stuff like catch RepositoryException for the sake of printing an error message (what's a case where getPath() would throw a RepositoryException and you'd still care that your template couldn't be rendered ? You likely have bigger problems)
    • I realize some of that stuff was already there, but might as well use the occasion to clean it up
  • Should we deprecate the "old" method in the interface now ? Or do both provide (different) value ?
    • Why is the first not delegating to the second ? They do pretty much the same thing don't they ?
  • Test:
    • I'm pretty you didn't mean to use com.sun.xml.internal.fastinfoset.stax.events.Util.isEmptyString; bsides it's used with assertThat but it's not a matcher. The test is probably not doing what you think it's doing.
    • Don't use org.hamcrest.CoreMatchers, just static import org.hamcrest.Matchers.* from hamcrest-library
    • Don't use org.hamcrest.MatcherAssert.assertThat, use org.junit's.
    • I feel the test has become more complex (maybe because fields were added. I guess that's a matter of taste, but it seems to me that fields in tests make the test a bit obscure (gotta look in many places to figure out the fixture).
    • You should maybe look into using UtilMatchers to match the exceptions or the error messages with regular expressions.
Comment by Roman Kovařík [ 02/Feb/15 ]

The old constructor removed.

AggregationState is used through a Provider ...I'd rather either pass the aggState to the method, or just not use it at all...It's also used to determine if we're in preview mode...I'm not sure we really need it...

We need to know from the AggregationState if we are in preview mode to see how a page would look like on a public instance.

I'd rather either pass the aggState to the method.

I want to keep the old method signature if possible. The aggregationState was originally retrieved from MgnlContext.

The try/catch blocks in handleException() make the code a bit clumsy. I'd suggest simplifying that.

Exception handling at least simplified.

Should we deprecate the "old" method in the interface now ? Or do both provide (different) value? Why is the first not delegating to the second ?

The old one still provides the current template name. Both delegates to info.magnolia.rendering.engine.ModeDependentRenderExceptionHandler#handleException

Tests cleaned up.

Comment by Magnolia International [ 03/Feb/15 ]

Sorry, gotta reopen for something that didn't occur to me earlier. We need to document how the 2 methods are different (it was more obvious when there was only one); why there are 2, in which case one would use which, etc.

Regarding AggregationState - I finally agree with not adding it as a param to the methods. Rationale:

  • its usage for logging URLs is debatable, but could be easily replaced
  • its usage for isPreviewMode() is more necessary, but sooner or later we'll use a different mechanism than the AggregationState to determine this. (it might already be the case in the Pages app)
  • there are bunch of places that say "we should not use the AggregationState anymore" (e.g in AggregationStateBasedRenderingContext), and it's true that we've considered it a deprecated component without actually going ahead and marking it as such, because we had no time/vision on how exactly to go about that.
Generated at Mon Feb 12 04:10:49 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.