Wicket
  1. Wicket
  2. WICKET-3741

Don't throw MarkupNotFoundException when the markup is acceptable to not be available yet

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5-RC4
    • Fix Version/s: 1.5-RC5
    • Component/s: wicket
    • Labels:
      None

      Description

      Some areas in Wicket code do expensive creation of WicketRuntimeException and then just ignores it.

      The most common case is :

      at java.lang.Throwable.fillInStackTrace(Native Method)
      at java.lang.Throwable.<init>(Throwable.java:218)
      at java.lang.Exception.<init>(Exception.java:59)
      at java.lang.RuntimeException.<init>(RuntimeException.java:61)
      at org.apache.wicket.WicketRuntimeException.<init>(WicketRuntimeException.java:49)
      at org.apache.wicket.MarkupContainer.getMarkupType(MarkupContainer.java:476)
      at org.apache.wicket.markup.DefaultMarkupCacheKeyProvider.getCacheKey(DefaultMarkupCacheKeyProvider.java:85)
      at org.apache.wicket.markup.MarkupCache.getMarkup(MarkupCache.java:274)
      at org.apache.wicket.markup.MarkupFactory.getMarkup(MarkupFactory.java:218)
      at org.apache.wicket.markup.MarkupFactory.getMarkup(MarkupFactory.java:192)
      at org.apache.wicket.MarkupContainer.getAssociatedMarkup(MarkupContainer.java:410)
      at org.apache.wicket.markup.html.panel.PanelMarkupSourcingStrategy.getMarkup(PanelMarkupSourcingStrategy.java:70)
      at org.apache.wicket.MarkupContainer.getMarkup(MarkupContainer.java:456)
      at org.apache.wicket.Component.getMarkup(Component.java:734)
      at org.apache.wicket.MarkupContainer.add(MarkupContainer.java:173)
      ...

      and is ignored in MarkupContainer#add().
      Such an exception is created for every component which is added in the constructor of another component (in contrast to onInitialize() where the markup will be available).

      1. 3741-panel-not-registered.log
        9 kB
        Martin Grigorov
      2. wicket-3741.patch
        89 kB
        Juergen Donnerstag
      3. WICKET-3741.patch
        41 kB
        Martin Grigorov
      4. WICKET-3741.patch
        33 kB
        Martin Grigorov
      5. wicket-3741-2.patch
        3 kB
        Juergen Donnerstag

        Activity

        Hide
        Martin Grigorov added a comment -

        @Juergen: with your last changes there are no exceptions thrown anymore. Close the ticket if you don't have more planned changes.

        Show
        Martin Grigorov added a comment - @Juergen: with your last changes there are no exceptions thrown anymore. Close the ticket if you don't have more planned changes.
        Hide
        Juergen Donnerstag added a comment -

        Because the solution can be much simpler IMO. Probably 99% of all exceptions in question can easibly be avoided without complicating things.

        Show
        Juergen Donnerstag added a comment - Because the solution can be much simpler IMO. Probably 99% of all exceptions in question can easibly be avoided without complicating things.
        Hide
        Igor Vaynberg added a comment -

        the core issue is that exceptions are too expensive. the remaining problem is that "null" is not enough information. so why not return a Result object that looks like this

        Result { boolean success, enum FailureReason

        {not found, not yet available}

        , Markup markup }

        this should be cheap and still provide a lot of information.

        Show
        Igor Vaynberg added a comment - the core issue is that exceptions are too expensive. the remaining problem is that "null" is not enough information. so why not return a Result object that looks like this Result { boolean success, enum FailureReason {not found, not yet available} , Markup markup } this should be cheap and still provide a lot of information.
        Hide
        Juergen Donnerstag added a comment -

        Hi Martin,

        the attached patch still has some severe issues. Because of your suggested getMarkupType change (and subsequent changes) upon return of getMarkup() we don't know anymore whether the markup can not be found or can not be loaded because of missing information. That in turn leads to plenty of additional getMarkupType() == null checks scattered throughout the code.

        The exception in the current code is an indicator that the markup could not be loaded yet (compared to "not found"). I accept that plenty of exceptions are thrown because we essentially test upon add() whether markup is already available. As you found out getMarkupType() == null is the main reason why makup can not be loaded and an exception is thrown. May be instead introducing throwException, we should simply test getMarkupType() before trying (test) to load the markup thus avoiding the majority of exceptions

        -Juergen

        Show
        Juergen Donnerstag added a comment - Hi Martin, the attached patch still has some severe issues. Because of your suggested getMarkupType change (and subsequent changes) upon return of getMarkup() we don't know anymore whether the markup can not be found or can not be loaded because of missing information. That in turn leads to plenty of additional getMarkupType() == null checks scattered throughout the code. The exception in the current code is an indicator that the markup could not be loaded yet (compared to "not found"). I accept that plenty of exceptions are thrown because we essentially test upon add() whether markup is already available. As you found out getMarkupType() == null is the main reason why makup can not be loaded and an exception is thrown. May be instead introducing throwException, we should simply test getMarkupType() before trying (test) to load the markup thus avoiding the majority of exceptions -Juergen
        Hide
        Martin Grigorov added a comment -

        In PanelMarkupSourcingStrategy I see this change:

        @Override

        • public IMarkupFragment getMarkup(final MarkupContainer parent, final Component child)
          + public IMarkupFragment getMarkup(final MarkupContainer parent, final Component child,
          + boolean throwException)
          {
        • IMarkupFragment markup = parent.getAssociatedMarkup();
          + IMarkupFragment markup = parent.getAssociatedMarkup(true);

        i.e. getAssociatedMarkup() uses 'true', not 'throwException'. Is this how it should be ?

        Show
        Martin Grigorov added a comment - In PanelMarkupSourcingStrategy I see this change: @Override public IMarkupFragment getMarkup(final MarkupContainer parent, final Component child) + public IMarkupFragment getMarkup(final MarkupContainer parent, final Component child, + boolean throwException) { IMarkupFragment markup = parent.getAssociatedMarkup(); + IMarkupFragment markup = parent.getAssociatedMarkup(true); i.e. getAssociatedMarkup() uses 'true', not 'throwException'. Is this how it should be ?
        Hide
        Martin Grigorov added a comment -

        Thanks Juergen!

        Are you working on the TODOs or I can jump and update your patch ?
        I want to avoid both of us working on the same task.

        Show
        Martin Grigorov added a comment - Thanks Juergen! Are you working on the TODOs or I can jump and update your patch ? I want to avoid both of us working on the same task.
        Hide
        Juergen Donnerstag added a comment -

        I review especially the lower level stuff and added more comments.
        NO_MARKUP should only be used insight MarkupCache since ConcurrentHashMap neither allows null keys nor null value. MarkupCache / MarkupFactory "user" API should return null upon MarkupNotFound
        Found problems with the markup still throw exception. Only markup not found exception should be controlled via the added attribute
        All tests successfully pass
        Still to be done:

        • replace common boilerplate with MarkupUtils.throwException ...
        • getAssociatedMarkup(throwException) is not yet implemented
        • I'd like to remove the getMarkup() and getAssociatedMarkup() versions without parameter, which why they are discouraged for now. Haven't eliminated the warnings yet.
        • getMarkupType() should as well get the attribuite in order to be consistent. It returning null if the parent can not be found is not very obvious in my opinion.
        Show
        Juergen Donnerstag added a comment - I review especially the lower level stuff and added more comments. NO_MARKUP should only be used insight MarkupCache since ConcurrentHashMap neither allows null keys nor null value. MarkupCache / MarkupFactory "user" API should return null upon MarkupNotFound Found problems with the markup still throw exception. Only markup not found exception should be controlled via the added attribute All tests successfully pass Still to be done: replace common boilerplate with MarkupUtils.throwException ... getAssociatedMarkup(throwException) is not yet implemented I'd like to remove the getMarkup() and getAssociatedMarkup() versions without parameter, which why they are discouraged for now. Haven't eliminated the warnings yet. getMarkupType() should as well get the attribuite in order to be consistent. It returning null if the parent can not be found is not very obvious in my opinion.
        Hide
        Martin Grigorov added a comment -

        The other change is that now MarkupContainer#getMarkupType() returns null when the type is still unknown, as its javadoc says. This was the main reason for most of the MarkupNotFoundExceptions. Because of this change I needed to change some code related to MarkupCache's cacheKey and DefaultMarkupResourceStreamProvider.

        Another thing that confused me for a while is Markup#toString() which prints both the resourceStream and the markupElements and thus the same markup is printed twice.

        And the last thing is Markup#NO_MARKUP - once it is used, other times 'null' is used. Maybe we should introduce Optional<T> from Igor's patch for AjaxFallbackXYZ components.

        Show
        Martin Grigorov added a comment - The other change is that now MarkupContainer#getMarkupType() returns null when the type is still unknown, as its javadoc says. This was the main reason for most of the MarkupNotFoundExceptions. Because of this change I needed to change some code related to MarkupCache's cacheKey and DefaultMarkupResourceStreamProvider. Another thing that confused me for a while is Markup#toString() which prints both the resourceStream and the markupElements and thus the same markup is printed twice. And the last thing is Markup#NO_MARKUP - once it is used, other times 'null' is used. Maybe we should introduce Optional<T> from Igor's patch for AjaxFallbackXYZ components.
        Hide
        Juergen Donnerstag added a comment -

        I suggest a couple more changes:

        • "if (throwException) throw new MNE else return null".... is used probably a dozen times. A little helper will clean up the code a little bit
        • getAssociatedMarkup has no throwException. For consistency reasons I'd suggest to add the param. That change will also allow to remove hasAssociatedMarkup which executes partially the same code as getAssociatedMarkup
        • since getMarkup() was introduced with 1.5 only, I suggest to remove getMarkup() in favor of getMarkup(throwE) instead of keeping both
        • not sure I've seen it right, but it seems as if all exceptions are now "ignored" (return null). If that is true, than I'd change it to be "only markup not found" to return null. Invalid markup should still throw an exception and be handled as such

        I'll provide an updated patch soon

        Show
        Juergen Donnerstag added a comment - I suggest a couple more changes: "if (throwException) throw new MNE else return null".... is used probably a dozen times. A little helper will clean up the code a little bit getAssociatedMarkup has no throwException. For consistency reasons I'd suggest to add the param. That change will also allow to remove hasAssociatedMarkup which executes partially the same code as getAssociatedMarkup since getMarkup() was introduced with 1.5 only, I suggest to remove getMarkup() in favor of getMarkup(throwE) instead of keeping both not sure I've seen it right, but it seems as if all exceptions are now "ignored" (return null). If that is true, than I'd change it to be "only markup not found" to return null. Invalid markup should still throw an exception and be handled as such I'll provide an updated patch soon
        Hide
        Martin Grigorov added a comment -

        Updating the patch with few improvements.

        Show
        Martin Grigorov added a comment - Updating the patch with few improvements.
        Hide
        Martin Grigorov added a comment -

        Another problem that is also suppressed by the try/catch in MarkupContainer is:
        Caused by: org.apache.wicket.markup.WicketParseException: Unknown tag name with Wicket namespace: 'panel'. Might be you haven't installed the appropriate resolver? '<wicket:panel>' (line 1, column 1)
        at org.apache.wicket.markup.parser.filter.WicketTagIdentifier.onComponentTag(WicketTagIdentifier.java:99)
        ....
        This is because PanelMarkupSourcingStrategy class is not loaded yet. See the attached 3741-panel-not-registered.log for the full stacktrace.

        Show
        Martin Grigorov added a comment - Another problem that is also suppressed by the try/catch in MarkupContainer is: Caused by: org.apache.wicket.markup.WicketParseException: Unknown tag name with Wicket namespace: 'panel'. Might be you haven't installed the appropriate resolver? '<wicket:panel>' (line 1, column 1) at org.apache.wicket.markup.parser.filter.WicketTagIdentifier.onComponentTag(WicketTagIdentifier.java:99) .... This is because PanelMarkupSourcingStrategy class is not loaded yet. See the attached 3741-panel-not-registered.log for the full stacktrace.
        Hide
        Martin Grigorov added a comment -

        Another test that fails with similar problem is org.apache.wicket.contrib.markup.html.velocity.VelocityPanelTest.testVelocityPanelWithMarkupParsing().

        Show
        Martin Grigorov added a comment - Another test that fails with similar problem is org.apache.wicket.contrib.markup.html.velocity.VelocityPanelTest.testVelocityPanelWithMarkupParsing().
        Hide
        Igor Vaynberg added a comment -

        i would send juergen an email to get his attention and feedback on this one

        Show
        Igor Vaynberg added a comment - i would send juergen an email to get his attention and feedback on this one
        Hide
        Martin Grigorov added a comment - - edited

        A patch that solves the problem.

        There is only one test that fails when executed in suite. When executed alone it passes -
        org.apache.wicket.markup.html.form.FormComponentPanelProcessingTest#testProcessingOrder().
        This test fails with:

        testProcessingOrder(org.apache.wicket.markup.html.form.FormComponentPanelProcessingTest) Time elapsed: 0.009 sec <<< ERROR!
        org.apache.wicket.markup.MarkupException: Close tag not found for tag: <wicket:panel>. Component: [TestFormComponentPanel [Component id = panel]]
        [markup = org.apache.wicket.util.resource.StringResourceStream@37b3e1c9: <wicket:panel><input wicket:id='text' type='text'/></wicket:panel>
        <wicket:panel><input wicket:id="text" type="text"/></wicket:panel>, index = 1, current = '<input wicket:id="text" type="text"/>' (line 0, column 0)]

        Show
        Martin Grigorov added a comment - - edited A patch that solves the problem. There is only one test that fails when executed in suite. When executed alone it passes - org.apache.wicket.markup.html.form.FormComponentPanelProcessingTest#testProcessingOrder(). This test fails with: testProcessingOrder(org.apache.wicket.markup.html.form.FormComponentPanelProcessingTest) Time elapsed: 0.009 sec <<< ERROR! org.apache.wicket.markup.MarkupException: Close tag not found for tag: <wicket:panel>. Component: [TestFormComponentPanel [Component id = panel] ] [markup = org.apache.wicket.util.resource.StringResourceStream@37b3e1c9: <wicket:panel><input wicket:id='text' type='text'/></wicket:panel> <wicket:panel><input wicket:id="text" type="text"/></wicket:panel>, index = 1, current = '<input wicket:id="text" type="text"/>' (line 0, column 0)]
        Hide
        Martin Grigorov added a comment -

        I'm going to introduce MarkupContainer.getMarkup(Component, boolean throwException). So far it seems to solve the problem.

        Show
        Martin Grigorov added a comment - I'm going to introduce MarkupContainer.getMarkup(Component, boolean throwException). So far it seems to solve the problem.
        Hide
        Martin Grigorov added a comment -

        org.apache.wicket.Component.getMarkup() may throw MarkupNotFoundException (extends WicketRuntimeException) if the parent is unknown yet, but many of its callers (e.g.
        org.apache.wicket.MarkupContainer.add(Component...),
        com.unitedinternet.mam.phoenix.client.ui.page.base.CustomPageMarkupSourcingStrategy.findInTransparentContainers(MarkupContainer, String),
        org.apache.wicket.Page.configureResponse(), org.apache.wicket.MarkupContainer.createAndAddComponentsForWicketTags(), org.apache.wicket.markup.html.internal.HtmlHeaderContainer.getMarkup(),
        org.apache.wicket.markup.html.border.Border.BorderBodyContainer.getMarkup(Component),
        org.apache.wicket.markup.html.panel.DefaultMarkupSourcingStrategy.getMarkup(MarkupContainer, Component)
        ) actually can deal with 'null' response in such cases.

        Show
        Martin Grigorov added a comment - org.apache.wicket.Component.getMarkup() may throw MarkupNotFoundException (extends WicketRuntimeException) if the parent is unknown yet, but many of its callers (e.g. org.apache.wicket.MarkupContainer.add(Component...), com.unitedinternet.mam.phoenix.client.ui.page.base.CustomPageMarkupSourcingStrategy.findInTransparentContainers(MarkupContainer, String), org.apache.wicket.Page.configureResponse(), org.apache.wicket.MarkupContainer.createAndAddComponentsForWicketTags(), org.apache.wicket.markup.html.internal.HtmlHeaderContainer.getMarkup(), org.apache.wicket.markup.html.border.Border.BorderBodyContainer.getMarkup(Component), org.apache.wicket.markup.html.panel.DefaultMarkupSourcingStrategy.getMarkup(MarkupContainer, Component) ) actually can deal with 'null' response in such cases.
        Hide
        Martin Grigorov added a comment -

        The problem is even doubled because MarkupContainer.getMarkupType() calls getPage() which may fail and then it will create one instance of WicketRuntimeException, then getMarkupType() will catch it and wrap it in a new WicketRuntimeException.

        Show
        Martin Grigorov added a comment - The problem is even doubled because MarkupContainer.getMarkupType() calls getPage() which may fail and then it will create one instance of WicketRuntimeException, then getMarkupType() will catch it and wrap it in a new WicketRuntimeException.

          People

          • Assignee:
            Juergen Donnerstag
            Reporter:
            Martin Grigorov
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development