jsr372-experts@javaserverfaces-spec-public.java.net

[jsr372-experts] Re: [jsr372-experts mirror] Re: Re: Re: Validate client IDs in f:ajax execute and render

From: Bauke Scholtz <balusc_at_gmail.com>
Date: Mon, 08 Aug 2016 06:05:49 +0000

Leo,

Algorithm makes sense, however I wonder why UIComponent#findComponent()
couldn't be improved on this. This does not seem to be a breaking change
and the findComponent() could certainly take advantage of it. Moreover,
this new method suggests that an expression may contain iteration indices
and users would then start to wonder why it doesn't work on findComponent()
which also takes an expression as argument.

Cheers, B

On Sun, Aug 7, 2016, 01:05 Leonardo Uribe <leonardo.uribe_at_irian.at> wrote:

> Hi
>
> I have been working on this issue, and after some experiments I have
> realized
> some conclusions about this:
>
> - Late evaluation of "render" attribute is a chicken-egg problem that is
> not
> worth to solve it. The evidence suggest this is a rare case, only found by
> a
> couple of users and there is a workaround for this one that can be applied
> without change the spec.
> - 99.9% of the time the target component exists when the ajax script is
> generated and the "render" attribute is evaluated. So, check the clientIds
> in that location is valid.
>
> Discarding the previous ideas, the only problem left is fix the way how
> f:ajax "execute" and "render" attributes are handled, so the clientId
> validation
> can be consistent.
>
> After some tests and ideas, the final proposal is to add a method in
> UIComponent called:
>
> public String UIComponent#getClientIdFromExpression(FacesContext
> facesContext,
> String expression)
>
> the algorithm takes an expression String just like
> UIComponent#findComponent(...)
> and return a full clientId taking as base the expression and the component
> instance from where the expression is resolved.
>
> Take a look at this example:
>
> <h:form id="form" >
> <h:outputText id="formText" value="Form Text"/>
> <h:dataTable id="table" value="#{list}" var="row">
> <h:column>
> <h:outputText id="baseText" value="#{row.text}"/>
> <h:dataTable id="nested" value="#{row.nested}" var="item">
> <h:column>
> <h:outputText id="nestedText" value="#{item}"/>
> <h:commandButton id="nestedButton" value="Press Me!">
> <f:ajax .../>
>
> </h:commandButton>
> </h:column>
> </h:dataTable>
> </h:column>
> </h:dataTable>
> </h:form>
>
> A nested datatable and an nestedButton with a f:ajax. The idea is f:ajax
> ClientBehaviorRenderer resolves the keywords (all, none, form,...) and for
> the
> remaining expressions invoke UIComponent#getClientIdFromExpression(...) one
> by one.
>
> So if nestedButton has a clientId like:
>
> form:table:3:nested:1:nestedButton
>
> We can imagine the following examples (expression --> clientId):
>
> form:formText --> form:formText
> nested:nestedText --> form:table:3:nested:1:nestedText
> table:baseText --> form:table:3:baseText
> table:0:baseText --> form:table:0:baseText
> nested:0:nestedText --> form:table:3:nested:0:nestedText
> table:nested --> form:table:3:nested
> table:1:nested:0:nestedText --> form:table:1:nested:0:nestedText
> :table --> null (':' redirects the base to
> UIViewRoot )
> :form:formText --> form:formText (form is child of
> UIViewRoot )
>
> What the algorithm does is:
>
> If the expression starts with
> facesContext.getNamingContainerSeparatorChar(),
> find the root component and invoke root.getClientIdFromExpression(...),
> passing
> the expression string but removing the separator char.
>
> First call findComponent(...) passing the expression, if a component is
> found return the clientId of that component.
>
> If no component is found, take the expression and extract the "base"
> which is the id of the closest NamingContainer. For example, the base for
> "nested:0:nestedText" is "nested". Then, use the UIComponent instance to
> get
> the closest parent NamingContainer instance with id "nested".
>
> From that point,
> - call parent.findComponent(...), passing the expression,
> - if a component is returned return component.getClientId(),
> - otherwise create a clientId joining the clientId of the context
> component ("form:table:3:nested") and the remaining string from
> expression removing the base (":0:nestedText).
> - Use component.invokeOnComponent(...), passing the synthetized
> clientId and if the invocation is succesful (the callback is
> executed) return the calculated clientId string.
>
> If no clientId can be returned, return null.
>
> This algorithm is useful enough to include it at UIComponent level,
> because it
> reuse the existing API to locate the component. No matter which component
> is
> (datatable, tree, ui:repeat, ...) the algorithm will work without
> additional
> changes over the components (but include it as a method of UIComponent
> gives
> more flexibility in case of).
>
> Let me know what do you think about it, and if the proposal can be included
> or not.
>
> regards,
>
> Leonardo Uribe
>
> 2016-08-01 22:37 GMT-05:00 Leonardo Uribe <leonardo.uribe_at_irian.at>:
>
>> Hi
>>
>> I'll do a detailed explanation of this issue. I know most of it you
>> already
>> know the topic, but it is necessary to explain how it works and from where
>> things comes from in order to solve this properly.
>>
>> The problem starts with the interpretation of f:ajax "execute" and
>> "render" attributes.
>>
>> The most clear intention of these two attributes can be found in
>> AjaxBehavior
>> javadoc:
>>
>> "... Return a non-empty Collection<String> of component identifiers that
>> will
>> be used to identify components that should be processed during the
>> execute
>> (render) phase of the request processing lifecycle. ..."
>>
>> As you already know there are 4 special keywords that can be used:
>> "@all",
>> "@none", "@this", "@form".
>>
>> The first important point to notice is these special keywords are
>> relative to
>> an specific component that holds the f:ajax client behavior. The way how
>> jsf finds the base component is through "javax.faces.behavior.event" and
>> "javax.faces.source" attribute.
>>
>> When the script is encoded, there is a translation process, where the
>> component identifiers are translated into client identifiers. This is done
>> because once javascript API requires the client ids to be sent so the
>> inner
>> visitTree(...) call could be done in PartialViewContext implementation.
>>
>> In JSF 1.0 UIComponent#findComponent(...) was used to locate component
>> in attributes like h:inputLabel for="..." or h:message for="...". This
>> works well because usually the "source" and the "target" components are
>> enclosed
>> inside the same context or UINamingContainer instance.
>>
>> So, it feels natural to use UIComponent#findComponent(...) for f:ajax. But
>> the problem is f:ajax could point to components outside the current
>> component
>> context. For example:
>>
>> <h:dataTable id="base" ...>
>> <h:column>
>> <h:panelGroup id="content" layout="block" ...>
>> ...
>> </h:panelGroup>
>> <h:dataTable id="info">
>> <h:column>
>> ...
>> <h:commandButton id="button1" ...>
>> <f:ajax render="content"/>
>> </h:commandButton>
>> </h:column>
>> </h:dataTable>
>> </h:column>
>> </h:dataTable>
>>
>> A nested dataTable where there is an ajax button that requires render
>> something
>> outside the enclosing NamingContainer.
>>
>> The previous example with the current JSF spec does not work. No matter
>> how you
>> try with "content" or "base:content" or "form:base:content", it just does
>> not
>> work. The reason is findComponent has some restrictions:
>>
>> "... Component identifiers are required to be unique within the scope of
>> the
>> closest ancestor NamingContainer that encloses this component (which
>> might be
>> this component itself). If there are no NamingContainer components in the
>> ancestry of this component, the root component in the tree is treated as
>> if
>> it were a NamingContainer, whether or not its class actually implements
>> the
>> NamingContainer interface. ..."
>>
>> There is a good reason for this restriction. You don't want a method that
>> traverse the whole tree trying to find the component without reason
>> because
>> that can be expensive. UIComponent#findComponent(...) is also used in
>> other
>> situations, so it is better to let is as is to avoid side effects.
>>
>> If we have a component with a client id like this as source:
>>
>> base:2:info:3:button1
>>
>> And I want to invoke an ajax operation over:
>>
>> base:0:content
>>
>> Please note both share the same "prefix", that can be used to locate the
>> base
>> NamingContainer (in this case "base"). So, an algorithm that can detect
>> the
>> common NamingContainer instance and from that component use
>> UIComponent#invokeOnComponent(...) to detect if the right component
>> exists has
>> better chances to work as expected.
>>
>> One option I have been thinking on is a method that detects if an id
>> exists
>> like for example UIComponent#clientIdExists(String clientId) that uses
>> invokeOnComponent(...) internally to find the right instance on the
>> context,
>> but it is optional if you want to add it to the API or not.
>>
>> Now, "execute" and "render" attributes are a bit different. "execute"
>> uses the
>> component tree before ajax, so the validation has sense at the time the
>> script
>> is written into the response. "render" uses the component tree after
>> execute on
>> the ajax request, so its validation should be done after (like the bug
>> discussed
>> previously says so), but that means if the component is not in the
>> component
>> tree and the current environment is production, no exception should be
>> thrown,
>> and on development environment a FacesMessage should be added or maybe an
>> exception is valid anyway. So, it is completely valid to reference a
>> component
>> at "render" and nothing should happen if the component is not in the tree.
>>
>> The solution has 3 elements:
>>
>> - Modify f:ajax javadoc to use a findComponent(...)/invokeOnComponent
>> combo.
>> (Optional) if the algorithm is general enough include it on the api at
>> UIComponent level.
>> - Validate on "execute", warn on "render".
>> - (Optional) Ensure late evaluation of "render" attribute if EL is used
>> over
>> the attribute, and include in that point the "render" warning logic.
>>
>> I know the proposal is a bit blurry, but I hope it is good enough to get
>> an idea about what should we do.
>>
>> regards,
>>
>> Leonardo Uribe
>>
>> 2016-07-28 5:09 GMT-05:00 Bauke Scholtz <balusc_at_gmail.com>:
>>
>>> Leo,
>>>
>>> Thank you for your investigation and insights. It's after all a very
>>> good idea to override component's findComponent method to detect the
>>> iteration index which the component itself is internally already aware of.
>>> I will experiment with it.
>>>
>>> Cheers, B
>>>
>>> On Thu, Jul 28, 2016, 05:41 Leonardo Uribe <leonardo.uribe_at_irian.at>
>>> wrote:
>>>
>>>> Hi Bauke
>>>>
>>>> I have tested an example in both implementations and now I can see
>>>> what's wrong.
>>>>
>>>> MyFaces f:ajax ClientBehaviorRenderer implementation relies on
>>>> UIComponent.findComponent(...) to locate the component pointed by
>>>> "execute"
>>>> or "render" attribute. The exception is thrown because the lookup
>>>> algorithm does
>>>> not take into account "virtual" components, so it never has the chance
>>>> to find
>>>> the component, because UIComponent.findComponent(...) is not overriden
>>>> in UIData
>>>> to include components according to its model, so any component inside
>>>> ui:repeat
>>>> or h:dataTable with an index is just ignored. Mojarra behaves
>>>> different, so
>>>> I suppose there is something different there, in fact I was suprised by
>>>> this
>>>> behavior.
>>>>
>>>> In JSF 2.2 facelets taglib documentation, in the description of f:ajax
>>>> it says
>>>> this:
>>>>
>>>> "... The String value for ids specified for execute and render may be
>>>> specified
>>>> as a search expression as outlined in the JavaDocs for
>>>> UIComponent.findComponent(). The implementation must resolve these ids
>>>> as
>>>> specified for UIComponent.findComponent(). ..."
>>>>
>>>> MyFaces is doing what the spec says, but it is clear the syntax
>>>> proposed is valid.
>>>> The problem is change the behavior of UIComponent.findComponent(...) is
>>>> problematic, because there is a lot of code that depends on it. We need
>>>> a new
>>>> lookup method that works in some cases like
>>>> UIComponent.findComponent(...), but
>>>> behaves like invokeOnComponent(...).
>>>>
>>>> Let me think a bit about it, so I can make a proposal we can discuss.
>>>>
>>>> regards,
>>>>
>>>> Leonardo Uribe
>>>>
>>>>
>>>>
>>>> 2016-07-27 4:58 GMT-05:00 Bauke Scholtz <balusc_at_gmail.com>:
>>>>
>>>>> Hi Leo,
>>>>>
>>>>> I just took the opportunity to test MyFaces 2.2.10 on this. It throws
>>>>> an exception on this.
>>>>>
>>>>> javax.faces.FacesException: Component with id::form:list:1:item not
>>>>> found
>>>>> at
>>>>> org.apache.myfaces.renderkit.html.HtmlAjaxBehaviorRenderer.getComponentId(HtmlAjaxBehaviorRenderer.java:495)
>>>>> at
>>>>> org.apache.myfaces.renderkit.html.HtmlAjaxBehaviorRenderer.build(HtmlAjaxBehaviorRenderer.java:467)
>>>>> at
>>>>> org.apache.myfaces.renderkit.html.HtmlAjaxBehaviorRenderer.mapToString(HtmlAjaxBehaviorRenderer.java:439)
>>>>> at
>>>>> org.apache.myfaces.renderkit.html.HtmlAjaxBehaviorRenderer.makeAjax(HtmlAjaxBehaviorRenderer.java:158)
>>>>> at
>>>>> org.apache.myfaces.renderkit.html.HtmlAjaxBehaviorRenderer.getScript(HtmlAjaxBehaviorRenderer.java:102)
>>>>> at
>>>>> javax.faces.component.behavior.ClientBehaviorBase.getScript(ClientBehaviorBase.java:101)
>>>>>
>>>>>
>>>>> It's exactly that validation which's being discussed here. Mojarra
>>>>> removed the validation, but I find that it should be brought back because
>>>>> starters start complaining that ajax "doesn't work". It appears that this
>>>>> validation is nowhere in the spec (at least, Manfred said so in
>>>>> https://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1372
>>>>>
>>>>> Cheers, B
>>>>>
>>>>>
>>>>> On Wed, Jul 20, 2016 at 9:04 AM, Bauke Scholtz <balusc_at_gmail.com>
>>>>> wrote:
>>>>>
>>>>>> > In my understanding, it works in MyFaces. Am I missing something?
>>>>>>
>>>>>> I meant the use case of specifying an absolute client ID referring a
>>>>>> specific iteration round. E.g.
>>>>>>
>>>>>> <h:form id="form">
>>>>>> <ui:repeat id="list" value="#{['one','two','three']}" var="item">
>>>>>> <h:outputText id="item" value="#{item}" /><br/>
>>>>>> </ui:repeat>
>>>>>>
>>>>>> <h:commandButton value="Update second item">
>>>>>> <f:ajax render=":form:list:1:item" />
>>>>>> </h:commandButton>
>>>>>> </h:form>
>>>>>>
>>>>>> Cheers, B
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Jul 20, 2016 at 1:49 AM, Leonardo Uribe <
>>>>>> leonardo.uribe_at_irian.at> wrote:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> I would like to mention some issues mentioned in MyFaces some time
>>>>>>> ago, related
>>>>>>> to the behavior of this part of the spec.
>>>>>>>
>>>>>>> The discussion comes from this issue:
>>>>>>>
>>>>>>> - https://issues.apache.org/jira/browse/MYFACES-3542
>>>>>>> The render attribute of AjaxBehavior should support late
>>>>>>> value expression evaluation
>>>>>>>
>>>>>>> The issue is all about when "execute" and "render" attributes should
>>>>>>> be
>>>>>>> evaluated. This point is critical if you want to validate the
>>>>>>> clientids
>>>>>>> properly.
>>>>>>>
>>>>>>> The first point to remember is the values stored in f:ajax "execute"
>>>>>>> or "render"
>>>>>>> attribute are not clientIds. Instead, they are relative references
>>>>>>> to component
>>>>>>> instances, that in some cases, where the components are inside a
>>>>>>> ui:repeat or
>>>>>>> a h:dataTable, they are "virtual" or "row-specific".
>>>>>>>
>>>>>>> Checking MyFaces, there is a validation check inside f:ajax
>>>>>>> ClientBehaviorRenderer. This validation throws a FacesException if
>>>>>>> the
>>>>>>> component cannot be found. The algorithm use the existing
>>>>>>> findComponent,
>>>>>>> but it is called twice, once with the reference and the other with
>>>>>>> the
>>>>>>> separator and the reference.
>>>>>>>
>>>>>>> Let's take a look at the proposed options:
>>>>>>>
>>>>>>> BS>> 1. Strip off anything which matches the iteration index
>>>>>>> :[0-9]+: from
>>>>>>> BS>> f:ajax execute/render client ID before passing to
>>>>>>> findComponent() for
>>>>>>> BS>> validation.
>>>>>>>
>>>>>>> It won't work, because there is no reliable way to remove that, at
>>>>>>> least
>>>>>>> with the current spec.
>>>>>>>
>>>>>>> BS>> 2. Improve findComponent to recognize the iteration index and
>>>>>>> act
>>>>>>> BS>> accordingly.
>>>>>>>
>>>>>>> It looks like a simplification of UIComponent.invokeOnComponent(...)
>>>>>>> or
>>>>>>> visitTree(...), but after you have the UIComponent instance, you
>>>>>>> don't have
>>>>>>> the context set, so it will not be the "real" instance. I don't see
>>>>>>> how it
>>>>>>> can fit in the spec.
>>>>>>>
>>>>>>> But at this point, I have to say I think there is another bug on the
>>>>>>> RI.
>>>>>>>
>>>>>>> I found it was reported long time ago here:
>>>>>>>
>>>>>>> https://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-865
>>>>>>> Ajax inside a DataTable (getClientId append rowIndex)
>>>>>>>
>>>>>>> And MyFaces fixed it in 2.0.1 (see MYFACES-2744 for details)
>>>>>>>
>>>>>>> BS>> Being able to reference a specific ui:repeat/h:dataTable item
>>>>>>> BS>> via f:ajax render is absolutely a major step forward.
>>>>>>>
>>>>>>> In my understanding, it works in MyFaces. Am I missing something?
>>>>>>>
>>>>>>> regards,
>>>>>>>
>>>>>>> Leonardo Uribe
>>>>>>>
>>>>>>> 2016-07-19 1:58 GMT-05:00 Bauke Scholtz <balusc_at_gmail.com>:
>>>>>>>
>>>>>>>> There are basically two ways to solve this:
>>>>>>>>
>>>>>>>> 1. Strip off anything which matches the iteration index :[0-9]+:
>>>>>>>> from f:ajax execute/render client ID before passing to findComponent() for
>>>>>>>> validation.
>>>>>>>> 2. Improve findComponent to recognize the iteration index and act
>>>>>>>> accordingly.
>>>>>>>>
>>>>>>>> The first way is trivial. The second way, however, is best to
>>>>>>>> implement if all repeater components (UIData and UIRepeat at least) share a
>>>>>>>> common interface such as UIIterable. Only, this is not really nice for
>>>>>>>> backwards compatibility.
>>>>>>>>
>>>>>>>> What do you think?
>>>>>>>>
>>>>>>>> Cheers, B
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, Jul 11, 2016 at 6:00 AM, Josh Juneau <juneau001_at_gmail.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> +1, I think that since this change will help to clarify the spec
>>>>>>>>> for newcomers, it is a good idea.
>>>>>>>>>
>>>>>>>>> Josh Juneau
>>>>>>>>> juneau001_at_gmail.com
>>>>>>>>> http://jj-blogger.blogspot.com
>>>>>>>>> https://www.apress.com/index.php/author/author/view/id/1866
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Sun, Jul 10, 2016 at 5:39 AM, Bauke Scholtz <balusc_at_gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Since Mojarra 2.2.5 (
>>>>>>>>>> https://java.net/jira/browse/JAVASERVERFACES-2958) the f:ajax
>>>>>>>>>> stopped validating client IDs in execute and render attributes with the
>>>>>>>>>> reason to support updating a specific ui:repeat/h:dataTable iteration round
>>>>>>>>>> such as render="form:repeat:0:item" which updates only the first item of
>>>>>>>>>> ui:repeat.
>>>>>>>>>>
>>>>>>>>>> However, this has the side effect that new JSF users now start
>>>>>>>>>> asking questions why f:ajax execute and/or render doesn't seem to have any
>>>>>>>>>> effect while there's a clear syntax error or typo in the execute and/or
>>>>>>>>>> render attribute. Previously they are been notified about this much sooner
>>>>>>>>>> by a clear exception. It appears that this validation is not part of JSF
>>>>>>>>>> spec.
>>>>>>>>>>
>>>>>>>>>> Therefore I propose to bring back validation of client ID in
>>>>>>>>>> f:ajax execute and render attributes and make it part of the spec. I
>>>>>>>>>> already created an issue on this:
>>>>>>>>>> https://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1372
>>>>>>>>>>
>>>>>>>>>> I only wonder what's the best place in spec to state that.
>>>>>>>>>> AjaxBehavior class?
>>>>>>>>>>
>>>>>>>>>> Cheers, B
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>