search

jakartaee / faces

Jakarta Faces
Other
109 stars 55 forks source link

Clarify if partial rendering should resetValues with SKIP_UNRENDERED #1877

Closed tandraschko closed 9 months ago

tandraschko commented 9 months ago

I found a case where UIViewRoot#resetValues called from #processPartialRendering also visits unrendered components, which can lead to errors. See: https://github.com/primefaces/primefaces/issues/11408

    at jakarta.faces.component.UIComponentBase.visitTree(UIComponentBase.java:1196)
    at jakarta.faces.component.UIViewRoot.resetValues(UIViewRoot.java:1484)
    at org.apache.myfaces.context.servlet.PartialViewContextImpl.processPartialRendering(PartialViewContextImpl.java:485)
    at org.apache.myfaces.context.servlet.PartialViewContextImpl.processPartial(PartialViewContextImpl.java:420)
    at org.primefaces.context.PrimePartialViewContext.processPartial(PrimePartialViewContext.java:46)

should we add VisitHint.SKIP_UNRENDERED when called from #processPartialRendering?

Not sure if it could break other cases

BalusC commented 9 months ago

It makes perfect sense to skip unrendered (and transient) components. This is also implemented as such in the orignal ResetInputAjaxActionListener of OmniFaces.

tandraschko commented 9 months ago

so should we just fix both MF and Mojarra implementations - also older ones?

BalusC commented 9 months ago

I fixed Mojarra https://github.com/eclipse-ee4j/mojarra/pull/5397. I noticed that the existing visit logic for execute and render already take into account SKIP_UNRENDERED (and EXECUTE_LIFECYCLE) so I decided to reuse these. I've left SKIP_TRANSIENT for what it is as it wouldn't harm -- is just kind of microoptimization in the specific case of resetValues.

As to UIViewRoot#resetValues() API, I think we can keep it as-is for now. Best what we could do while keeping backwards compatibility is to let it take VisitHint as varargs argument for 5.0. Then we can later improve the PartialViewContext impl to utilize it.

tandraschko commented 9 months ago

also in MF now: https://github.com/apache/myfaces/commit/052a32a20b1d69ed3d035c8dd0ef49ef5388cfbb

i will create a new issue to enhance UIViewRoot#resetValues()