Dynamic actions not correctly processed

[fanste]

Describe the bug

We have an application that is havily based on programmatic changes to the view tree. In this case, a datatable of primefaces with a static column (== defined in xHTML) is extended with some programmatically created columns. The static column is additionally moved to another position. This is done by removing the column from the view and later on it is added again.

The static column looks like this:

<p:column id="col_static" ...>
    <p:commandLink ... />
</p:column>

On remove, the component itself and ALL of it's children are added to the list of dynamic actions as REMOVE. The very same components are added as ADD to the same list of dynamic actions.

The list looks like this:

// list of dynamic actions
[
    REMOVE: Column,
    REMOVE: CommandLink,
    ADD: Column
    ADD: CommandLink
]

On the next request (here AJAX) the view needs to be restored. All dynamic actions are processed. It first removes the column itself. No problem, the component could be found. But as the first child of the first component should be removed too, the logic failes in UIData (the one from faces).

java.lang.NumberFormatException: Trying to extract rowIndex from clientId 'frmDialogs:j_idt905:j_idt906' there is no numeric segment
    at jakarta.faces.impl@4.0.7//jakarta.faces.component.UIData.invokeOnComponent(UIData.java:953)

UIData tries a fallback to resolve the component. Therefore it tries to parse j_idt906 as a row index, which is not possible. UIData is not prepared for this case.

I tried to fix it by switching the order of how the elements are removed from view (bottom-up instead of top-down). This fixes the REMOVEs. But then it fails again for the ADDs on the same code. Reason is, that the logic tries to find the component on the view before it will be restored from the list StateContext#getDynamicComponents(). And the part "find the component on the view" uses again UIData#invokeOnComponent which then tries to parse e.g. j_idt906 as a row index.

So the only possible solution seems to be to fix UIData to it ignores the case "no row index". I compared it to MyFaces, they do it the same - they simply ignore that case. And according to the DOCs this is ok.

Returns false if no component with the given clientId is found.

To Reproduce

I will try to build a WAR for this, that is not related to our specific infrastructure.

Expected behavior

It should work without UIData throwing a NumberFormatException.

Screenshots

Stack on REMOVE (p:commandLink)

Stack on ADD

Additional context

Faces version 4.0.7 on WildFly 32

[fanste]

I compared the old 2.2.19 (on which the code is working) with 4.0.7 and the overall logic is the same (looking for a parsable row index), but it differs significantly in the details.

The new logic got implemented because of https://github.com/eclipse-ee4j/mojarra/commit/9ca1bf1fa0d01c31b4c84db33212329a43c349ac.

Before 9ca1bf1:

• client ID of to be searched component: frmDialogs:j_idt905:j_idt906
• search for a the separator char : directly after the clientID of the table (== preRowIndexSep)
• search for the next : right after preRowIndexSep (== postRowIndexSep)
• in this case postRowIndexSep == -1 => if evaluates to false => return false from invokeOnComponent

After 9ca1bf1:

• client ID of to be searched component: frmDialogs:j_idt905:j_idt906
• extract the sub-clientID (remaining part after clientID of datatable): :j_idt906
• search for the first numeric, not found => NumberFormatException("there is no numeric segment")
• wrap the NFE in a new NFE => NumberFormatException("Trying to extract...)

The old code worked, because of the check on "required two separators after the datatable client ID" which was removed on the refactoring.

Unchecked, just an assumption: most likeley the old logic will fail too, if a naming container is used inside p:column.

[fanste]

I just confirmed my assumption:

<p:column id="col_static" ...>
    <f:subview id="x">
        <p:commandLink id="lnk" ... />
    </f:subview>
</p:column>

This will fail also in 2.2.19, as the to be searched client ID contains two separators after the datatable client ID (:x:lnk) and so the logic tries to parse x as a number.

=> a completely new logic is required that works on just component based client IDs without any iteration information (aka row index)

[fanste]

Some more input, sorry for the amount of messages ;)

1. Why is UIData limited to UIColumn? This makes it unnecessarily hard for e.g. frameworks like Primefaces with their component p:columns, which is the main cause for the fix from 9ca1bf1. Espceially because the logic 'look for a row index and use that to find the component' simply forwards to super.invokeOnComponent, which then uses the unfiltered facet and children list.
2. The assumption from 9ca1bf1 / #5153, that the row index might be ANYWHERE in the client ID is wrong, in my opinion.

Regarding 2: The assumption is, that a client ID form:table:columns:0:button the 0 is the to-be-used row index of the UIData from from:table. This is not correct, as 0 is the row index of p:columns.

The fix does additionally not really fix the problem of 5153. It simply does not find anything but does this without throwing an exception. The example of 5153 suddenly work, because primefaces simply uses the provided expression to update the frontend instead of the cliend ID, that it tried to resolve (see https://github.com/primefaces/primefaces/blob/master/primefaces/src/main/java/org/primefaces/PrimeFaces.java#L363).

Side note: MyFaces also failes to resolve the components and relies on the fallback of Primefaces

[fanste]

Possible solution (tested in the application) - but as the changes appear in the API, it might be a spec change?

@@ -902,14 +902,14 @@
         // check column level facets, if any
         if (getChildCount() > 0) {
             for (UIComponent column : getChildren()) {
-                if (column instanceof UIColumn) {
+//                if (column instanceof UIColumn) {
                     if (column.getFacetCount() > 0) {
                         for (UIComponent facet : column.getFacets().values()) {
                             if (facet.invokeOnComponent(context, clientId, callback)) {
                                 return true;
                             }
                         }
-                    }
+//                    }
                 }
             }
         }
@@ -919,11 +919,11 @@
          */
         if (getChildCount() > 0) {
             for (UIComponent column : getChildren()) {
-                if (column instanceof UIColumn) {
+//                if (column instanceof UIColumn) {
                     if (column.invokeOnComponent(context, clientId, callback)) {
                         return true;
                     }
-                }
+//                }
             }
         }

@@ -944,9 +944,15 @@
         // outerData:3:data:3:input for a nested table.
         // We need to find the first occurring client ID segment which is parseable as a number.
         if (clientId.startsWith(myId)) {
+           int nextSegmentStartSep = clientId.indexOf(sepChar, myId.length());
+           int nextSegmentEndSep = clientId.indexOf(sepChar, nextSegmentStartSep + 1);
+           if(nextSegmentStartSep == -1 || nextSegmentEndSep == -1 || !Character.isDigit(clientId.charAt(nextSegmentStartSep + 1))) {
+               return false;
+           }
+
             try {
                 try {
-                    newRow = extractFirstNumericSegment(clientId.substring(myId.length()), sepChar);
+                    newRow = Integer.parseInt(clientId.substring(nextSegmentStartSep + 1, nextSegmentEndSep));
                 } catch (NumberFormatException ex) {
                     // PENDING(edburns): I18N
                     String message = "Trying to extract rowIndex from clientId \'" + clientId + "\' " + ex.getMessage();

[BalusC]

Thank you very much for the detailed report!

Initially I couldn't reproduce your problem because it never hit the extractFirstNumericSegment() method in my attempts. Only your last comment whereby you mentioned that the solution was to outcomment the instanceof UIColumn checks made me realize that the dynamically added component to your UIData parent wasn't an instance of UIColumn. When I dynamically added an UIOutput instead of UIColumn to the UIData, I reproduced the problem and finally understood the use case. In fact, removing only the outcomments and leaving the extractFirstNumericSegment() intact would still solve your problem.

The instanceof UIColumn checks are nowhere mandated in the spec wrt invokeOnComponent(), it's only mandated for lifecycle processing of decode/validate/update and already implemented separately from invokeOnComponent(), so we can theoretically indeed safely remove these instanceof UIColumn checks from the invokeOnComponent().

Reverting the extractFirstNumericSegment() check would hide potential future bugs. If the extractFirstNumericSegment() was never implemented in first place, we'd never have learnt about the dynamic component related invokeOnComponent() bug which you discovered so I'd rather keep it in to spot and fix future bugs in this area.

[BalusC]

For the record and TCK, this is the smallest reproducer I could boil down:

<h:form>
    <h:dataTable binding="#{dataTable}" />
    <h:commandButton value="add non-UIColumn child" action="#{issue5445Bean.addNonUIColumnChild(dataTable)}" />
</h:form>

@Named
@RequestScoped
public class Issue5445Bean {
    public void addNonUIColumnChild(UIData table) {
        table.getChildren().add(new UIOutput());
    }
}

[fanste]

Thanks for your applied fix.

I've seen another change done and reverted again by yourself, that might be relevant and is also part of MyFaces.

You replaced the clientId-match check on UIData facets by getFacetsAndChildren() and simply invoked invokeOnComponent on them. That was reverted again.

I did not test it yet, but what happens if a naming container is used inside the facet? Theoreticaly children inside the naming container will never be found.

Some construct like: (is this even valid? idk)

<h:dataTable>
    <f:facet name="...">
        <f:subview id="...">
            <h:outputText ... />
        </f:subview>
    </f:facet>
</h:dataTable>

Edit: And why do we need to process the column facets inside UIData? According to UIComponent#invokeOnComponent this should be done by the child itself. Effectively this doubles the work(?)

[fanste]

As mentioned in the first post

I tried to fix it by switching the order of how the elements are removed from view (bottom-up instead of top-down)

the bottom-up fix is now necessary after all.

I found a case, where FaceletViewHandlingStrategy#locateComponentByClientId uses the full traversal using #visitTree and then failes inside ui:repeat. The reason is, that the full traversal does not skip unrendered children.

This reproducer is based on the real-world use-case [*]:

<h:form>
    <ui:repeat value="#{[1]}" var="x">
        <ui:fragment rendered="#{x eq 2}">
            <ui:repeat value="#{testBean.nonExistingGetter}">

            </ui:repeat>
        </ui:fragment>
    </ui:repeat>

    <h:dataTable binding="#{dt}">
        <h:column id="col">
            <h:outputText id="txt" value="test" />
        </h:column>
    </h:dataTable>

    <h:commandButton value="Remove Column" actionListener="#{testBean.doRemoveColumn(dt, 'col')}" />
    <h:commandButton value="Remove Column Reverse" actionListener="#{testBean.doRemoveColumnBottomUp(dt, 'col')}" />
</h:form>

@RequestScoped
@Named
public class TestBean {
    public void doRemoveColumn(UIData dataTable, String columnIdFragment) {
        final UIComponent column = dataTable.findComponent(columnIdFragment);
        dataTable.getChildren().remove(column);
    }

    public void doRemoveColumnBottomUp(UIData dataTable, String columnIdFragment) {
        final UIComponent column = dataTable.findComponent(columnIdFragment);
        _removeBottomUp(column);
    }

    private void _removeBottomUp(UIComponent component) {
        for(UIComponent child : component.getChildren()) {
            _removeBottomUp(child);
        }

        component.getParent().getChildren().remove(component);
    }
}

While Remove Column will throw an exception, Remove Column Reverse will do the job.

This effectively simulates, that UIComponentBase#disconnectFromView fires the PreRemoveFromViewEvent after the children, not before.

private static void disconnectFromView(FacesContext context, Application application, UIComponent component) {

    [...]

    application.publishEvent(context, PreRemoveFromViewEvent.class, component);
    component.setInView(false);
    component.compositeParent = null;
 }

[*] The real-world use case builds a form where each input is defined in a list. The pre-defined layout of those inputs is ui:included. The correct layout is rendered by the check on ui:fragment. Some inputs have an additional ui:repeat which then failes, as ui:fragment rendered is ignored.

[BalusC]

Github automatically closed issue due to a trigger in merge message, reopening.

[BalusC]

I've seen another change done and reverted again by yourself, that might be relevant and is also part of MyFaces.

I wanted to minimize potential regression because I did not fully understand why exactly the original author implemented the UIData#invokeOnComponent as such. I currently believe more and more it's simply premature optimization. I'll take a second look.

[fanste]

I'm sorry to bother you. But any thoughts on my last comment?

[BalusC]

Very valid point.