TCK Challenge: <ui:repeat> save state not required by specification

[brideck]

Challenged Tests:
ee.jakarta.tck.faces.test.servlet30.ajax.Issue3833IT#testUIRepeatStateSaving

TCK Version:
Jakarta Faces 4.0.x

Tested Implementation:
Open Liberty -- containing MyFaces 4.0

Description:
The challenged test covers scenarios involving the saving and restoring of component state within <ui:repeat> elements, a behavior that does not seem to be required by the Faces specification as far as we can see. This test is another instance of one that was created in response to a Mojarra issue with no corresponding change driven into the specification.

MyFaces has never received a JIRA from a user in this space, and therefore has never implemented the function that would be needed for this test. Saving the required state would consume additional memory and CPU, just in order to provide function that no MyFaces user has ever requested and that the spec doesn't seem to require. As such, MyFaces wishes to have this test removed from the TCK rather than be required to degrade its performance.

There's even a comment by Leo Uribe that used to be in the MyFaces code stating "I haven't found any use case that require to store the rowStates in the component state, mostly because EditableValueHolder instances render the value into the client and then this value are taken back at the beginning of the next request." Similarly, a blog post by Leo raises the similar point that Faces should be striving to keep the state small: "That's the reason why h:dataTable or ui:repeat create just one component and share it for all rows: to keep state small and view rendering fast." The post details alternatives for Faces users who might seek this sort of functionality.

[BalusC]

Discussion understood but there need to be a distinction of 2 different things:

1. saving/restoring state of UIRepeat component itself (e.g. begin, end, var, etc)
2. saving/restoring state of EditableValueHolder children nested in UIRepeat (submitted value, etc)

1 should indeed not be needed at all. MyFaces doesn't do this, but Mojarra indeed unnecessarily does this.
2 is however needed and the test tests exactly this. Mojarra indeed unnecessarily saves a copy of their state in UIRepeat itself but this is in turn unrelated to the fact that the test case should pass regardless of how the implementation deals with the use case.

Best what I could do is to rename the test case to e.g. testUIRepeatDescendantStateSaving and clarify somewhere that it's referring to context explained in 2.

[tandraschko]

@BalusC is 2) really a real world case?
i just wonder as i'm using MF since ~15 years and never had any issues with inputs in ui:repeat - also we didnt have any issue reports regarding this

[BalusC]

It must be said that it really surprised me that this specific test fails in MyFaces. I can take a closer look.

[BalusC]

I had a deja vu and found this discussion eclipse-ee4j/mojarra#4957 and subsequently a potential fix for MyFaces https://issues.apache.org/jira/browse/MYFACES-4425?focusedCommentId=17461482&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17461482

[brideck]

I think what's at issue here is whether this is a case that is worthy of being in the TCK. It was a Mojarra reported bug, and Mojarra handled it, but that doesn't necessarily mean that other implementations need to deal with it as well.

[BalusC]

I meant to say that there should be a way to fix the issue without the need to save the row index state of UIRepeat itself. This information is already present in the client ID represented by ajax source param.

[pnicolucci]

@BalusC @tandraschko just following up here. Any updates on a path forward?

[pnicolucci]

Since we've never had any reported issues in MyFaces for this and it isn't clear from the spec if this should be supported or not can we accept and exclude the test?

[pnicolucci]

@arjantijms any thoughts?

[pnicolucci]

We discussed this on the platform call today and I'm going to accept this challenge and merge the PR that excludes this test. Fyi, @arjantijms and @BalusC

[pnicolucci]

Closing as completed now that the test is excluded.

[arjantijms]

Thanks @pnicolucci for taking the initiative!