MYFACES-4623 check for duplicate ids

[volosied]

Draft test fix for https://issues.apache.org/jira/browse/MYFACES-4623

Feel like a band-aid solution on top of another band-aid :/

This could use some refactoring to avoid code duplication?

[volosied]

Thanks - I'm gonna do some testing with this and if everything looks good, I'll merge next week. I'll also get a draft PR up soon for the other duplicate ID exception.

[volosied]

I'll do TCK testing before any release, so I'll just merge this now now.

Edit: Scratch that. I'll keep this PR open, but merge in 4.0 instead.

[tandraschko]

i would like to have some tests before merging such a change

[volosied]

Tried to create a simple scenario, but so far no luck. This DupId Exception is also difficult to replicate easily.

I'm not familiar with p:calendar but my suspicion is that the tree under it is creating elements which aren't marked. Thus during the restore phase, the children are simply added. Specifically, it relates to the component resources created by p:calendar. (target is ComponentResourceContainer and the child is jakarta.faces.component.UIOutput. )

This getChildList.add causes the duplicate id exception since restored child elements match an id of an existing element. I believe the faulty element is inputmask/inputmask.js.xhtml?ln=primefaces&v=13.0.0"?

See error from the original test app:

org.apache.myfaces.view.facelets.compiler.DuplicateIdException: Component with duplicate id "j_id__rd_5" found. The first component is {Component-Path : [Class: jakarta.faces.component.UIViewRoot,ViewId: /test.xhtml][Class: org.apache.myfaces.component.ComponentResourceContainer,Id: jakarta_faces_location_head][Class: jakarta.faces.component.UIOutput,Id: j_id__rd_5]}

I'll take another look later.

[volosied]

I'm not able to reproduce this outside the app. Perhaps you could take a look? I can't create another custom component like p:calendar to replicate this problem with. @tandraschko

However. c:forEach is broken and we've had many issues in the past. I'm not too happy with this fix, but, given the state of things with c:forEach, I think we should merge this it.

Leo also mentioned here:

                 // By effect of c:forEach it is possible that the component can be duplicated
                // in the component tree, so what we need to do as a fallback is replace the
                // component in the spot with the restored version.

It seems to me that this very rare scenario with Resource Dependencies was overlooked?

[tandraschko]

i dont know what should be special on p:calendar? it just has @ResourceDependency like any other comp?

[volosied]

I've managed to reproduce it. I also realized my fix was not enough. I can add a test, but I'm not sure how to do it via unit testing. Can I add it as an integration test?

[volosied]

Test app: myfaces-4623-test.zip

[volosied]

Another thing -- the dup id exception only occurs if STRICT_JSF_2_FACELETS_COMPATIBILITY is true.

In general, I think it's best to stay away from the legacy for each handler, but I think this fix could be useful for those experiencing this exception.

One concern I have if how we could know that the same component is replaced. However, my thinking is that if the same id was added then the user have encountered a duplicate id exception anyway and the app wouldn't have worked. In this case, we check for the duplicate ids, replace it if found, and, if not, add it anyway. So I don't think it should break any existing users and only help those affected by this dup id issue.

Edit: CHECK_ID_PRODUCTION_MODE is auto for production , so I think think error should have still occurred. Testing shows the exception is thrown in production w/ auto.

CHECK_ID_PRODUCTION_MODE options:

• true : Check all client ids, including UILeaf components

• auto : check ids of components that does not encapsulate markup (like facelets UILeaf).
  Note ids of UILeaf instances are generated by facelets vdl, start with "j_id", are never rendered
  into the response and UILeaf instances are never used as a target for listeners, so in practice
  there is no need to check such ids. This reduce the overhead associated with generate client ids

• false : Do not check for duplicate ids in production.

Might be useful to check for j_id__rd since this seems to occur with Resource Dependencies. Added.

[volosied]

@tandraschko Please look over and provide feedback. Also, let me know where I should add the test.

[volosied]

@tandraschko Just following up -- let me know your thoughts and the new code & where to place the test. Thanks.

[tandraschko]

a integrationtests is ok for me, we just need some auto-testing to avoid breaking again in future