[
https://issues.apache.org/jira/browse/MYFACES-4775?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Thomas Andraschko resolved MYFACES-4775.
----------------------------------------
Resolution: Fixed
> c:forEach writes a submitted value into the element saved with the view state
> instead of into the live item
> -----------------------------------------------------------------------------------------------------------
>
> Key: MYFACES-4775
> URL: https://issues.apache.org/jira/browse/MYFACES-4775
> Project: MyFaces Core
> Issue Type: Bug
> Affects Versions: 5.0.0, 4.1.4
> Reporter: Bauke Scholtz
> Assignee: Thomas Andraschko
> Priority: Major
> Fix For: 5.0.0
>
>
> While analyzing what the Faces view state contains, inputs bound to a
> property of the {{var}} of a {{<c:forEach>}} turned out to lose their
> submitted value without any message, also when nothing changes between the
> requests.
> h3. Reproducer
> {code:xml}
> <h:form id="viewForm">
> <c:forEach items="#{viewItems.list}" var="item">
> <h:inputText id="name_#{item.id}" value="#{item.name}" />
> </c:forEach>
> <h:commandButton id="submit" value="Submit" />
> </h:form>
> <h:outputText value="Bean values: #{viewItems.list[0].name},
> #{viewItems.list[1].name}" />
> <h:form id="requestForm">
> <c:forEach items="#{requestItems.list}" var="item">
> <h:inputText id="name_#{item.id}" value="#{item.name}" />
> </c:forEach>
> <h:commandButton id="submit" value="Submit" />
> </h:form>
> <h:outputText value="Bean values: #{requestItems.list[0].name},
> #{requestItems.list[1].name}" />
> {code}
> {code:java}
> public class Item implements Serializable {
> private final String id;
> private String name;
> // Constructor, getters and setter.
> }
> @Named @ViewScoped
> public class ViewItems implements Serializable {
> private List<Item> list;
> @PostConstruct
> public void init() {
> list = new ArrayList<>(List.of(new Item("a", "original a"), new
> Item("b", "original b")));
> }
> public List<Item> getList() {
> return list;
> }
> }
> @Named @RequestScoped
> public class RequestItems {
> private List<Item> list;
> @PostConstruct
> public void init() {
> list = List.of(new Item("a", "original a"), new Item("b", "original
> b"));
> }
> public List<Item> getList() {
> return list;
> }
> }
> {code}
> Open the page, change both names of a form to "edited a" and "edited b", and
> submit it. Nothing changes between the requests, so every build makes the
> same decisions. The response renders the names from the bean:
> ||Items in||MyFaces 4.1.4, server||MyFaces 4.1.4, client||Mojarra 4.1.20,
> server||Mojarra 4.1.20, client||
> |{{@ViewScoped}} bean|edited a, edited b|*original a, original b*|edited a,
> edited b|edited a, edited b|
> |{{@RequestScoped}} bean|*original a, original b*|*original a, original
> b*|edited a, edited b|edited a, edited b|
> MyFaces 4.1.3 gives the same result. The same inputs in a {{<ui:repeat>}}
> instead of a {{<c:forEach>}} receive the values in all cases.
> h3. Cause
> {{ForEachHandler.getVarExpr()}} binds the {{var}} of each row to the element
> itself, with {{new IteratedValueExpression(ve, value)}}, also for a {{List}}
> and an array. {{setVar()}} then stores that expression in the
> {{FaceletState}} via {{putBinding()}}, so the element is saved with the view
> state. On the postback, {{#\{item.name\}}} resolves against that saved
> element:
> * With client side state saving, it is always a deserialized copy.
> * With server side state saving, it is a copy as soon as the bean holds new
> instances, as a {{@RequestScoped}} bean does on every request, or a bean
> which loads the items from a database.
> The submitted value then lands in an object which nobody refers to anymore.
> h3. Specification
> MYFACES-4029 and MYFACES-4318 report the same behavior. Both were closed as
> not a bug, because {{<c:forEach>}} "just does not play well with JSF
> lifecycle". That was a fair reading of the JSF 2.x specification, which
> indeed did not clearly specify how the rows of a build time iteration are
> restored.
> The Faces 5.0 specification, which is in progress, specifies it since
> [jakartaee/faces#2234|https://github.com/jakartaee/faces/pull/2235/changes].
> The javadoc of {{StateManagementStrategy.restoreView()}} sets the contract:
> bq. This method returns the view that was saved: restoring the {{Object}}
> that {{saveView}} returned for a view must produce that view again, holding
> the same components at the same positions with the same state.
> In short, the spec now finally requires {{restore(save(view))}} to equal
> exactly {{view}}, in all cases.
> The rows of a {{<c:forEach>}} are reproduced from the build time decisions
> saved with the view, such as the range and the row count. The elements are
> not part of the view: each row reads its element from the items. Restore
> View, in the description of build time conditions:
> bq. The items an iteration was rendered over are a further exception, since
> its rows read their element from those items rather than from the state:
> where the items no longer hold them, the value submitted for such a row
> reaches nothing that outlives the request.
> So the elements themselves are not needed to restore the view, and the
> reproducer above is a valid page. The javadoc of {{restoreView()}} calls the
> case where the items no longer hold the elements "the case which cannot be
> reproduced from saved state at all".
> The Faces 5.0 TCK covers that case in
> {{Issue5961IT.testIterationOverVanishedItems()}}: the rows are restored, but
> the submitted values must not reach the model. With server side state saving,
> MyFaces 4.1.5-SNAPSHOT fails that test: the saved element is then the same
> session scoped instance, so the submitted values reach the model, and the
> names read "first,second" instead of "a,b". With client side state saving, it
> passes only because the values land in a deserialized copy. This was run with
> the page and beans of that test deployed on Tomcat, not with the TCK harness.
> MyFaces itself has replayed the decisions of {{<c:if>}}, {{<c:choose>}} and
> {{ui:include}} already correctly since 2.0.10 and 2.1.4 (MYFACES-3329), and
> the rows of {{<c:forEach>}} since 2.2.0 (MYFACES-3811). Only for
> {{<c:forEach>}} does it save more than the decision: the elements themselves.
> Mojarra implements this only since 4.0.23 and 4.1.14
> ([eclipse-ee4j/mojarra#5965|https://github.com/eclipse-ee4j/mojarra/pull/5965]),
> with {{com.sun.faces.restoreBuildTimeDecisions}} set to {{true}}, which
> becomes default in Mojarra 5.0. Each row then reads its element live, by
> position or by key. A row whose element is gone reads a stand-in, and
> [{{ForEachHandler}} logs a
> warning|https://github.com/eclipse-ee4j/mojarra/blob/f4d595f502b864b3e57bff6c6ec93564a28a7f07/impl/src/main/java/com/sun/faces/facelets/tag/jstl/core/ForEachHandler.java#L453-L455]
> which names the {{<c:forEach>}}. On the page of that TCK test, Mojarra
> 4.1.21-SNAPSHOT passes and logs:
> {noformat}
> WARNING: The build which restored this postback reproduced 2 of the 2 rows
> /issue5961-vanisheditems.xhtml @10,51 <c:forEach> rendered over items which
> no longer hold their elements, so a value submitted for such a row is decoded
> into a stand-in for the element that is gone rather than into the model.
> {noformat}
> h3. Proposed fix
> Let a row read its element from the live items, by index for a {{List}} and
> an array, as {{IndexedValueExpression(ve, i)}} did before MYFACES-3811. Keep
> the saved state only for what decides the restored rows, such as the range
> and the row count, and not for the elements themselves.
> {{LegacyForEachHandler}}, enabled with
> {{org.apache.myfaces.STRICT_JSF_2_FACELETS_COMPATIBILITY=true}}, already
> binds the {{var}} of a {{List}} with {{new IndexedValueExpression(ve, i)}}
> and does not put it in the {{FaceletState}}. With that handler, the
> reproducer above writes "edited a, edited b" into both beans, with server and
> client side state saving alike. That only shows the approach works in
> MyFaces: the context param also affects the scope of {{<c:set>}} and
> {{<ui:param>}} across inclusions (see
> [jakartaee/faces#2248|https://github.com/jakartaee/faces/issues/2248]), and
> the {{<c:forEach>}} fix must apply by default, independent of any context
> param.
> Fix is needed in MyFaces 5.0.0 in order to pass Faces 5.0 TCK and just a nice
> to have in MyFaces 4.1.5.
> h3. History
> * MYFACES-3811
> ([a2b11c073|https://github.com/apache/myfaces/commit/a2b11c073]) replaced
> {{new IndexedValueExpression(ve, i)}} by {{new IteratedValueExpression(ve,
> value)}}. The old line is still there as a comment.
> * MYFACES-3825
> ([2d351df95|https://github.com/apache/myfaces/commit/2d351df95]) put the
> {{var}} expression in the {{FaceletState}}, which saves it with the view
> state.
> 🤖 Generated with Claude Opus 5.5
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
