Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
tandraschko merged PR #607: URL: https://github.com/apache/myfaces/pull/607 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
tandraschko commented on PR #607: URL: https://github.com/apache/myfaces/pull/607#issuecomment-1776697343 a integrationtests is ok for me, we just need some auto-testing to avoid breaking again in future -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
volosied commented on PR #607: URL: https://github.com/apache/myfaces/pull/607#issuecomment-1775452681 @tandraschko Just following up -- let me know your thoughts and the new code & where to place the test. Thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
volosied commented on PR #607: URL: https://github.com/apache/myfaces/pull/607#issuecomment-1771280872 @tandraschko Please look over and provide feedback. Also, let me know where I should add the test. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
volosied commented on PR #607: URL: https://github.com/apache/myfaces/pull/607#issuecomment-1771218314 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
volosied commented on PR #607: URL: https://github.com/apache/myfaces/pull/607#issuecomment-1771196134 Test app: [myfaces-4623-test.zip](https://github.com/apache/myfaces/files/13044382/myfaces-4623-test.zip) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
volosied commented on PR #607: URL: https://github.com/apache/myfaces/pull/607#issuecomment-1771181712 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? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
volosied commented on code in PR #607:
URL: https://github.com/apache/myfaces/pull/607#discussion_r1364827913
##
impl/src/main/java/org/apache/myfaces/view/facelets/PartialStateManagementStrategy.java:
##
@@ -642,14 +642,25 @@ public void invokeContextCallback(FacesContext context,
UIComponent target)
{
try
{
-target.getChildren().add(childIndex, child);
+UIComponent parent = target;
+UIComponent dup = parent.getChildren().get(childIndex);
+if (child.getId().equals(dup.getId())) // avoid
DuplicateIdException - myfaces-4623
Review Comment:
Actually, I cannot because of the initial check:
`child.getAttributes().containsKey(ComponentSupport.MARK_CREATED)` is false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
volosied commented on code in PR #607:
URL: https://github.com/apache/myfaces/pull/607#discussion_r1364504418
##
impl/src/main/java/org/apache/myfaces/view/facelets/PartialStateManagementStrategy.java:
##
@@ -642,14 +642,25 @@ public void invokeContextCallback(FacesContext context,
UIComponent target)
{
try
{
-target.getChildren().add(childIndex, child);
+UIComponent parent = target;
+UIComponent dup = parent.getChildren().get(childIndex);
+if (child.getId().equals(dup.getId())) // avoid
DuplicateIdException - myfaces-4623
Review Comment:
Maybe I should check for
`dup.getAttributes().get(ComponentSupport.MARK_CREATED)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
tandraschko commented on PR #607: URL: https://github.com/apache/myfaces/pull/607#issuecomment-1769181462 i dont know what should be special on p:calendar? it just has `@ResourceDependency` like any other comp? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
volosied commented on PR #607: URL: https://github.com/apache/myfaces/pull/607#issuecomment-1769148959 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](https://github.com/apache/myfaces/blob/50d6e0ec3507f0b5d8c2b7492505ac683180b9a0/impl/src/main/java/org/apache/myfaces/view/facelets/PartialStateManagementStrategy.java#L604C1-L606C72): // 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? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
volosied commented on PR #607:
URL: https://github.com/apache/myfaces/pull/607#issuecomment-1767128386
Tried to create a simple scenario, but so far so luck. This DupId Exception
is also difficult to replicate easily.
Requirements for this bug to occur are
- Composite Component
- c:forEach
- p:calendar
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=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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MYFACES-4623 check for duplicate ids [myfaces]
tandraschko commented on PR #607: URL: https://github.com/apache/myfaces/pull/607#issuecomment-1766566921 i would like to have some tests before merging such a change -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] myfaces-4623 check for duplicate ids [myfaces]
volosied commented on PR #607: URL: https://github.com/apache/myfaces/pull/607#issuecomment-1753459517 I'll do TCK testing before any release, so I'll just merge this now now. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@myfaces.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
