Re: git commit: WICKET-4484 do not change tag id when resolving component
Locked
Re: git commit: WICKET-4484 do not change tag id when resolving component
 
|
Hi Sven,
the change introduces a theoretical risk of duplicate ids. I don't know how likely it is and I know we have no tests for it. But Filters are created per markup file and modify the markup upon loading, prior to caching. Which also means that the Page is not available. In case of inheritance it might not even be the same Page. To be absolutely sure the ID is page-unique, it gets update in the Resolvers where the Page instance is available. The root cause is that we don't validate if the ID has been updated already and avoid subsequent updates. We have no means to do that check. A number at the end of the ID would not be sufficient. May be a Component's flag bit can be used for that purpose. I understand the problem in your application, but users will have an extremly hard time to identify the issue if it occurs. I'm not sure it's worth it. May be the approach outlined above solved your problem as well, but in a safe manner. Juergen On Thu, Mar 29, 2012 at 6:43 PM, <[hidden email]> wrote: > Updated Branches: > refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 > > > WICKET-4484 do not change tag id when resolving component > > > Project: http://git-wip-us.apache.org/repos/asf/wicket/repo > Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 > Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 > Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 > > Branch: refs/heads/wicket-1.5.x > Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 > Parents: c6c45a5 > Author: Sven Meier <[hidden email]> > Authored: Thu Mar 29 18:43:07 2012 +0200 > Committer: Sven Meier <[hidden email]> > Committed: Thu Mar 29 18:43:07 2012 +0200 > > ---------------------------------------------------------------------- > .../markup/parser/filter/WicketLinkTagHandler.java | 1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > ---------------------------------------------------------------------- > > > http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java > ---------------------------------------------------------------------- > diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java > index 689133e..8779b6f 100644 > --- a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java > +++ b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java > @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends AbstractMarkupFilter implements ICompo > if (wtag.isLinkTag() && (wtag.getNamespace() != null)) > { > String id = tag.getId() + "-" + container.getPage().getAutoIndex(); > - tag.setId(id); > > return new TransparentWebMarkupContainer(id); > } > |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
Hi Juergen,
I double checked with how other filters/resolvers are doing it and I found AutoLabelTextResolver to leave the tag id untouched too. >May be a Component's flag bit can be used for that purpose. Shouldn't that be a flag on the markup tag instead then? >I don't know how likely it is and I know we have no tests for it. Yeah, I didn't find one either. I'm not sure I understand where's a possible problem here. It's hard to discuss this issues without an actual case. Sven On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: > Hi Sven, > > the change introduces a theoretical risk of duplicate ids. I don't > know how likely it is and I know we have no tests for it. But Filters > are created per markup file and modify the markup upon loading, prior > to caching. Which also means that the Page is not available. In case > of inheritance it might not even be the same Page. To be absolutely > sure the ID is page-unique, it gets update in the Resolvers where the > Page instance is available. > > The root cause is that we don't validate if the ID has been updated > already and avoid subsequent updates. We have no means to do that > check. A number at the end of the ID would not be sufficient. May be a > Component's flag bit can be used for that purpose. > > I understand the problem in your application, but users will have an > extremly hard time to identify the issue if it occurs. I'm not sure > it's worth it. May be the approach outlined above solved your problem > as well, but in a safe manner. > > Juergen > > On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> wrote: >> Updated Branches: >> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >> >> >> WICKET-4484 do not change tag id when resolving component >> >> >> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >> Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >> >> Branch: refs/heads/wicket-1.5.x >> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >> Parents: c6c45a5 >> Author: Sven Meier<[hidden email]> >> Authored: Thu Mar 29 18:43:07 2012 +0200 >> Committer: Sven Meier<[hidden email]> >> Committed: Thu Mar 29 18:43:07 2012 +0200 >> >> ---------------------------------------------------------------------- >> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >> 1 files changed, 0 insertions(+), 1 deletions(-) >> ---------------------------------------------------------------------- >> >> >> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >> ---------------------------------------------------------------------- >> diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >> index 689133e..8779b6f 100644 >> --- a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >> +++ b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends AbstractMarkupFilter implements ICompo >> if (wtag.isLinkTag()&& (wtag.getNamespace() != null)) >> { >> String id = tag.getId() + "-" + container.getPage().getAutoIndex(); >> - tag.setId(id); >> >> return new TransparentWebMarkupContainer(id); >> } >> |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
|
Hi Sven
> I double checked with how other filters/resolvers are doing it and I found > AutoLabelTextResolver to leave the tag id untouched too. which is a fault as well. Same risk. > >>May be a Component's flag bit can be used for that purpose. > > Shouldn't that be a flag on the markup tag instead then? No, because the Pages are made of potentially many markups. Since the ID has no info about hierarchy, the number appended is the only thing that makes it page unique. Hence it can not be per markup. Since every tag gets associated with a Component, the Component is the right place for the flag. >>I don't know how likely it is and I know we have no tests for it. > > Yeah, I didn't find one either. I'm not sure I understand where's a possible > problem here. It's hard to discuss this issues without an actual case. An artifical markup which creates duplicate IDs can be constructed. I've not tried it but I don't expect it to be too difficult. Knowing there is a potential risk which will be very hard to find by a normal user, makes me belief we should fix it properly, To me the applied patch leaves us with a bigger problem than it fixes. Juergen > Sven > > > On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >> >> Hi Sven, >> >> the change introduces a theoretical risk of duplicate ids. I don't >> know how likely it is and I know we have no tests for it. But Filters >> are created per markup file and modify the markup upon loading, prior >> to caching. Which also means that the Page is not available. In case >> of inheritance it might not even be the same Page. To be absolutely >> sure the ID is page-unique, it gets update in the Resolvers where the >> Page instance is available. >> >> The root cause is that we don't validate if the ID has been updated >> already and avoid subsequent updates. We have no means to do that >> check. A number at the end of the ID would not be sufficient. May be a >> Component's flag bit can be used for that purpose. >> >> I understand the problem in your application, but users will have an >> extremly hard time to identify the issue if it occurs. I'm not sure >> it's worth it. May be the approach outlined above solved your problem >> as well, but in a safe manner. >> >> Juergen >> >> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> wrote: >>> >>> Updated Branches: >>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>> >>> >>> WICKET-4484 do not change tag id when resolving component >>> >>> >>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>> Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>> >>> Branch: refs/heads/wicket-1.5.x >>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>> Parents: c6c45a5 >>> Author: Sven Meier<[hidden email]> >>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>> Committer: Sven Meier<[hidden email]> >>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>> >>> ---------------------------------------------------------------------- >>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>> 1 files changed, 0 insertions(+), 1 deletions(-) >>> ---------------------------------------------------------------------- >>> >>> >>> >>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>> ---------------------------------------------------------------------- >>> diff --git >>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>> index 689133e..8779b6f 100644 >>> --- >>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>> +++ >>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>> AbstractMarkupFilter implements ICompo >>> if (wtag.isLinkTag()&& (wtag.getNamespace() != >>> null)) >>> { >>> String id = tag.getId() + "-" + >>> container.getPage().getAutoIndex(); >>> - tag.setId(id); >>> >>> return new >>> TransparentWebMarkupContainer(id); >>> } >>> > |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
|
Hi,
>An artifical markup which creates duplicate IDs can be constructed. all auto labels' tags have the same id already (its class name) so no need to create some artificial markup. >To me the applied patch leaves us with a bigger problem than it fixes. If the id of the tag in the cached markup is altered on each render, the id can get quite long pretty fast. As the reporter of the issues has stated, this is a 'big' problem ;). > we should fix it properly Agreed. Obviously I'm missing a key concept here. I'll try to debug this issue tomorrow to understand the problem you've identified. Sven On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: > Hi Sven > >> I double checked with how other filters/resolvers are doing it and I found >> AutoLabelTextResolver to leave the tag id untouched too. > which is a fault as well. Same risk. > >>> May be a Component's flag bit can be used for that purpose. >> Shouldn't that be a flag on the markup tag instead then? > No, because the Pages are made of potentially many markups. Since the > ID has no info about hierarchy, the number appended is the only thing > that makes it page unique. Hence it can not be per markup. Since every > tag gets associated with a Component, the Component is the right place > for the flag. > >>> I don't know how likely it is and I know we have no tests for it. >> Yeah, I didn't find one either. I'm not sure I understand where's a possible >> problem here. It's hard to discuss this issues without an actual case. > An artifical markup which creates duplicate IDs can be constructed. > I've not tried it but I don't expect it to be too difficult. Knowing > there is a potential risk which will be very hard to find by a normal > user, makes me belief we should fix it properly, To me the applied > patch leaves us with a bigger problem than it fixes. > > Juergen > >> Sven >> >> >> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>> Hi Sven, >>> >>> the change introduces a theoretical risk of duplicate ids. I don't >>> know how likely it is and I know we have no tests for it. But Filters >>> are created per markup file and modify the markup upon loading, prior >>> to caching. Which also means that the Page is not available. In case >>> of inheritance it might not even be the same Page. To be absolutely >>> sure the ID is page-unique, it gets update in the Resolvers where the >>> Page instance is available. >>> >>> The root cause is that we don't validate if the ID has been updated >>> already and avoid subsequent updates. We have no means to do that >>> check. A number at the end of the ID would not be sufficient. May be a >>> Component's flag bit can be used for that purpose. >>> >>> I understand the problem in your application, but users will have an >>> extremly hard time to identify the issue if it occurs. I'm not sure >>> it's worth it. May be the approach outlined above solved your problem >>> as well, but in a safe manner. >>> >>> Juergen >>> >>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> wrote: >>>> Updated Branches: >>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>> >>>> >>>> WICKET-4484 do not change tag id when resolving component >>>> >>>> >>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>> Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>> >>>> Branch: refs/heads/wicket-1.5.x >>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>> Parents: c6c45a5 >>>> Author: Sven Meier<[hidden email]> >>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>> Committer: Sven Meier<[hidden email]> >>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>> >>>> ---------------------------------------------------------------------- >>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>> ---------------------------------------------------------------------- >>>> >>>> >>>> >>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>> ---------------------------------------------------------------------- >>>> diff --git >>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>> index 689133e..8779b6f 100644 >>>> --- >>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>> +++ >>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>> AbstractMarkupFilter implements ICompo >>>> if (wtag.isLinkTag()&& (wtag.getNamespace() != >>>> null)) >>>> { >>>> String id = tag.getId() + "-" + >>>> container.getPage().getAutoIndex(); >>>> - tag.setId(id); >>>> >>>> return new >>>> TransparentWebMarkupContainer(id); >>>> } >>>> |
Re: Re: git commit: WICKET-4484 do not change tag id when resolving component
|
Hi Juergen, Sven,
We are not doing away with adding the "auto index" unique number to the resultant component id, we are only doing away with modifying the id in the cached markup tag. The uniqueness of the id of the component returned by the resolve() method is still being guaranteed (as far as I understand) because it takes the original id from the tag and append the current page's next auto index number. The component (TransparentWebMarkupContainer) is then created with this modified id. All I'm asking in this patch is to not modify the original id stored in the markup tag, because its not necessary. (AFAIK). Anyway, the resultant component id is now (after the patch) as unique as it is on the first render without the patch... This problem is actually very serious. I have a base page which 90% of my pages extend. Without the patch, on every single render, the id stored in the tag has a few bytes appended, causing the id of the resultant component to be a few bytes longer (for every render of every page in any session). I'm not sure if these ids get stored in memory after the render (I dont know wicket internals well enough to tell) or stored in the page store. If they do, its a big memory leak. At the very least its spews out huge amounts of data in the debug logging. Cheers, Jesse On 29/03/2012 22:39, Sven Meier wrote: > Hi, > > >An artifical markup which creates duplicate IDs can be constructed. > > all auto labels' tags have the same id already (its class name) so no > need to create some artificial markup. > > >To me the applied patch leaves us with a bigger problem than it fixes. > > If the id of the tag in the cached markup is altered on each render, > the id can get quite long pretty fast. As the reporter of the issues > has stated, this is a 'big' problem ;). > > > we should fix it properly > > Agreed. Obviously I'm missing a key concept here. > I'll try to debug this issue tomorrow to understand the problem you've > identified. > > Sven > > On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >> Hi Sven >> >>> I double checked with how other filters/resolvers are doing it and I >>> found >>> AutoLabelTextResolver to leave the tag id untouched too. >> which is a fault as well. Same risk. >> >>>> May be a Component's flag bit can be used for that purpose. >>> Shouldn't that be a flag on the markup tag instead then? >> No, because the Pages are made of potentially many markups. Since the >> ID has no info about hierarchy, the number appended is the only thing >> that makes it page unique. Hence it can not be per markup. Since every >> tag gets associated with a Component, the Component is the right place >> for the flag. >> >>>> I don't know how likely it is and I know we have no tests for it. >>> Yeah, I didn't find one either. I'm not sure I understand where's a >>> possible >>> problem here. It's hard to discuss this issues without an actual case. >> An artifical markup which creates duplicate IDs can be constructed. >> I've not tried it but I don't expect it to be too difficult. Knowing >> there is a potential risk which will be very hard to find by a normal >> user, makes me belief we should fix it properly, To me the applied >> patch leaves us with a bigger problem than it fixes. >> >> Juergen >> >>> Sven >>> >>> >>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>> Hi Sven, >>>> >>>> the change introduces a theoretical risk of duplicate ids. I don't >>>> know how likely it is and I know we have no tests for it. But Filters >>>> are created per markup file and modify the markup upon loading, prior >>>> to caching. Which also means that the Page is not available. In case >>>> of inheritance it might not even be the same Page. To be absolutely >>>> sure the ID is page-unique, it gets update in the Resolvers where the >>>> Page instance is available. >>>> >>>> The root cause is that we don't validate if the ID has been updated >>>> already and avoid subsequent updates. We have no means to do that >>>> check. A number at the end of the ID would not be sufficient. May be a >>>> Component's flag bit can be used for that purpose. >>>> >>>> I understand the problem in your application, but users will have an >>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>> it's worth it. May be the approach outlined above solved your problem >>>> as well, but in a safe manner. >>>> >>>> Juergen >>>> >>>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> wrote: >>>>> Updated Branches: >>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>> >>>>> >>>>> WICKET-4484 do not change tag id when resolving component >>>>> >>>>> >>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>> Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>> >>>>> Branch: refs/heads/wicket-1.5.x >>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>> Parents: c6c45a5 >>>>> Author: Sven Meier<[hidden email]> >>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>> Committer: Sven Meier<[hidden email]> >>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>> >>>>> ---------------------------------------------------------------------- >>>>> >>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>> ---------------------------------------------------------------------- >>>>> >>>>> >>>>> >>>>> >>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>> >>>>> ---------------------------------------------------------------------- >>>>> >>>>> diff --git >>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>> >>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>> >>>>> index 689133e..8779b6f 100644 >>>>> --- >>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>> >>>>> +++ >>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>> >>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>> AbstractMarkupFilter implements ICompo >>>>> if (wtag.isLinkTag()&& >>>>> (wtag.getNamespace() != >>>>> null)) >>>>> { >>>>> String id = tag.getId() + "-" + >>>>> container.getPage().getAutoIndex(); >>>>> - tag.setId(id); >>>>> >>>>> return new >>>>> TransparentWebMarkupContainer(id); >>>>> } >>>>> > > |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
|
Hi,
>We are not doing away with adding the "auto index" unique number to the resultant component id, >we are only doing away with modifying the id in the cached markup tag. indeed, this is why I didn't see a problem with your patch. I'm currently trying to build a test case which exhibits the problem Juergen has brought up. >I'm not sure if these ids get stored in memory after the render The auto components are removed after render, so the length of their ids doesn't matter much. The markup is cached separately, so the ids will stay in memory. Letting these increase indefinitely is really something we should try to avoid. Sven On 03/30/2012 03:01 AM, Jesse Long wrote: > Hi Juergen, Sven, > > We are not doing away with adding the "auto index" unique number to > the resultant component id, we are only doing away with modifying the > id in the cached markup tag. > > The uniqueness of the id of the component returned by the resolve() > method is still being guaranteed (as far as I understand) because it > takes the original id from the tag and append the current page's next > auto index number. The component (TransparentWebMarkupContainer) is > then created with this modified id. > > All I'm asking in this patch is to not modify the original id stored > in the markup tag, because its not necessary. (AFAIK). > > Anyway, the resultant component id is now (after the patch) as unique > as it is on the first render without the patch... > > This problem is actually very serious. I have a base page which 90% of > my pages extend. Without the patch, on every single render, the id > stored in the tag has a few bytes appended, causing the id of the > resultant component to be a few bytes longer (for every render of > every page in any session). I'm not sure if these ids get stored in > memory after the render (I dont know wicket internals well enough to > tell) or stored in the page store. If they do, its a big memory leak. > > At the very least its spews out huge amounts of data in the debug > logging. > > Cheers, > Jesse > > > On 29/03/2012 22:39, Sven Meier wrote: >> Hi, >> >> >An artifical markup which creates duplicate IDs can be constructed. >> >> all auto labels' tags have the same id already (its class name) so no >> need to create some artificial markup. >> >> >To me the applied patch leaves us with a bigger problem than it fixes. >> >> If the id of the tag in the cached markup is altered on each render, >> the id can get quite long pretty fast. As the reporter of the issues >> has stated, this is a 'big' problem ;). >> >> > we should fix it properly >> >> Agreed. Obviously I'm missing a key concept here. >> I'll try to debug this issue tomorrow to understand the problem >> you've identified. >> >> Sven >> >> On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >>> Hi Sven >>> >>>> I double checked with how other filters/resolvers are doing it and >>>> I found >>>> AutoLabelTextResolver to leave the tag id untouched too. >>> which is a fault as well. Same risk. >>> >>>>> May be a Component's flag bit can be used for that purpose. >>>> Shouldn't that be a flag on the markup tag instead then? >>> No, because the Pages are made of potentially many markups. Since the >>> ID has no info about hierarchy, the number appended is the only thing >>> that makes it page unique. Hence it can not be per markup. Since every >>> tag gets associated with a Component, the Component is the right place >>> for the flag. >>> >>>>> I don't know how likely it is and I know we have no tests for it. >>>> Yeah, I didn't find one either. I'm not sure I understand where's a >>>> possible >>>> problem here. It's hard to discuss this issues without an actual case. >>> An artifical markup which creates duplicate IDs can be constructed. >>> I've not tried it but I don't expect it to be too difficult. Knowing >>> there is a potential risk which will be very hard to find by a normal >>> user, makes me belief we should fix it properly, To me the applied >>> patch leaves us with a bigger problem than it fixes. >>> >>> Juergen >>> >>>> Sven >>>> >>>> >>>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>>> Hi Sven, >>>>> >>>>> the change introduces a theoretical risk of duplicate ids. I don't >>>>> know how likely it is and I know we have no tests for it. But Filters >>>>> are created per markup file and modify the markup upon loading, prior >>>>> to caching. Which also means that the Page is not available. In case >>>>> of inheritance it might not even be the same Page. To be absolutely >>>>> sure the ID is page-unique, it gets update in the Resolvers where the >>>>> Page instance is available. >>>>> >>>>> The root cause is that we don't validate if the ID has been updated >>>>> already and avoid subsequent updates. We have no means to do that >>>>> check. A number at the end of the ID would not be sufficient. May >>>>> be a >>>>> Component's flag bit can be used for that purpose. >>>>> >>>>> I understand the problem in your application, but users will have an >>>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>>> it's worth it. May be the approach outlined above solved your problem >>>>> as well, but in a safe manner. >>>>> >>>>> Juergen >>>>> >>>>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> wrote: >>>>>> Updated Branches: >>>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>>> >>>>>> >>>>>> WICKET-4484 do not change tag id when resolving component >>>>>> >>>>>> >>>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>>> Commit: >>>>>> http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>>> >>>>>> Branch: refs/heads/wicket-1.5.x >>>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>>> Parents: c6c45a5 >>>>>> Author: Sven Meier<[hidden email]> >>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>> Committer: Sven Meier<[hidden email]> >>>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>>> >>>>>> ---------------------------------------------------------------------- >>>>>> >>>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>>> ---------------------------------------------------------------------- >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>> >>>>>> ---------------------------------------------------------------------- >>>>>> >>>>>> diff --git >>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>> >>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>> >>>>>> index 689133e..8779b6f 100644 >>>>>> --- >>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>> >>>>>> +++ >>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>> >>>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>>> AbstractMarkupFilter implements ICompo >>>>>> if (wtag.isLinkTag()&& >>>>>> (wtag.getNamespace() != >>>>>> null)) >>>>>> { >>>>>> String id = tag.getId() + "-" + >>>>>> container.getPage().getAutoIndex(); >>>>>> - tag.setId(id); >>>>>> >>>>>> return new >>>>>> TransparentWebMarkupContainer(id); >>>>>> } >>>>>> >> >> > |
Re: Re: git commit: WICKET-4484 do not change tag id when resolving component
|
|
In reply to this post by Jesse Long
On Fri, Mar 30, 2012 at 3:01 AM, Jesse Long <[hidden email]> wrote:
> Hi Juergen, Sven, > > We are not doing away with adding the "auto index" unique number to the > resultant component id, we are only doing away with modifying the id in the > cached markup tag. The patched changed the resolver and not the filter. Hence the markup loaded into the cache is still the same, but the id presented at render time is now a different. > > The uniqueness of the id of the component returned by the resolve() method > is still being guaranteed (as far as I understand) because it takes the > original id from the tag and append the current page's next auto index > number. The component (TransparentWebMarkupContainer) is then created with > this modified id. sorry but this is also not correct. The markup of a page is composed of potentially many markup files (fragments, panels, inheritance, etc.). An id which is unique with markup file is not guranteed to be unique in the page markup. That's why resolvers are using the page.autoIndex to create a page unique id > > All I'm asking in this patch is to not modify the original id stored in the > markup tag, because its not necessary. (AFAIK). > > Anyway, the resultant component id is now (after the patch) as unique as it > is on the first render without the patch... sorry, but this is not true. > > This problem is actually very serious. I have a base page which 90% of my > pages extend. Without the patch, on every single render, the id stored in > the tag has a few bytes appended, causing the id of the resultant component > to be a few bytes longer (for every render of every page in any session). > I'm not sure if these ids get stored in memory after the render (I dont know > wicket internals well enough to tell) or stored in the page store. If they > do, its a big memory leak. > > At the very least its spews out huge amounts of data in the debug logging. I very well understand your pain point, but lets fix the problem properly and not introduce a bug with an enhancement. Juergen > > Cheers, > Jesse > > > > On 29/03/2012 22:39, Sven Meier wrote: >> >> Hi, >> >> >An artifical markup which creates duplicate IDs can be constructed. >> >> all auto labels' tags have the same id already (its class name) so no need >> to create some artificial markup. >> >> >To me the applied patch leaves us with a bigger problem than it fixes. >> >> If the id of the tag in the cached markup is altered on each render, the >> id can get quite long pretty fast. As the reporter of the issues has stated, >> this is a 'big' problem ;). >> >> > we should fix it properly >> >> Agreed. Obviously I'm missing a key concept here. >> I'll try to debug this issue tomorrow to understand the problem you've >> identified. >> >> Sven >> >> On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >>> >>> Hi Sven >>> >>>> I double checked with how other filters/resolvers are doing it and I >>>> found >>>> AutoLabelTextResolver to leave the tag id untouched too. >>> >>> which is a fault as well. Same risk. >>> >>>>> May be a Component's flag bit can be used for that purpose. >>>> >>>> Shouldn't that be a flag on the markup tag instead then? >>> >>> No, because the Pages are made of potentially many markups. Since the >>> ID has no info about hierarchy, the number appended is the only thing >>> that makes it page unique. Hence it can not be per markup. Since every >>> tag gets associated with a Component, the Component is the right place >>> for the flag. >>> >>>>> I don't know how likely it is and I know we have no tests for it. >>>> >>>> Yeah, I didn't find one either. I'm not sure I understand where's a >>>> possible >>>> problem here. It's hard to discuss this issues without an actual case. >>> >>> An artifical markup which creates duplicate IDs can be constructed. >>> I've not tried it but I don't expect it to be too difficult. Knowing >>> there is a potential risk which will be very hard to find by a normal >>> user, makes me belief we should fix it properly, To me the applied >>> patch leaves us with a bigger problem than it fixes. >>> >>> Juergen >>> >>>> Sven >>>> >>>> >>>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>>> >>>>> Hi Sven, >>>>> >>>>> the change introduces a theoretical risk of duplicate ids. I don't >>>>> know how likely it is and I know we have no tests for it. But Filters >>>>> are created per markup file and modify the markup upon loading, prior >>>>> to caching. Which also means that the Page is not available. In case >>>>> of inheritance it might not even be the same Page. To be absolutely >>>>> sure the ID is page-unique, it gets update in the Resolvers where the >>>>> Page instance is available. >>>>> >>>>> The root cause is that we don't validate if the ID has been updated >>>>> already and avoid subsequent updates. We have no means to do that >>>>> check. A number at the end of the ID would not be sufficient. May be a >>>>> Component's flag bit can be used for that purpose. >>>>> >>>>> I understand the problem in your application, but users will have an >>>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>>> it's worth it. May be the approach outlined above solved your problem >>>>> as well, but in a safe manner. >>>>> >>>>> Juergen >>>>> >>>>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> wrote: >>>>>> >>>>>> Updated Branches: >>>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>>> >>>>>> >>>>>> WICKET-4484 do not change tag id when resolving component >>>>>> >>>>>> >>>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>>> Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>>> >>>>>> Branch: refs/heads/wicket-1.5.x >>>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>>> Parents: c6c45a5 >>>>>> Author: Sven Meier<[hidden email]> >>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>> Committer: Sven Meier<[hidden email]> >>>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>>> >>>>>> ---------------------------------------------------------------------- >>>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>>> ---------------------------------------------------------------------- >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>> ---------------------------------------------------------------------- >>>>>> diff --git >>>>>> >>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>> >>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>> index 689133e..8779b6f 100644 >>>>>> --- >>>>>> >>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>> +++ >>>>>> >>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>>> AbstractMarkupFilter implements ICompo >>>>>> if (wtag.isLinkTag()&& (wtag.getNamespace() >>>>>> != >>>>>> null)) >>>>>> { >>>>>> String id = tag.getId() + "-" + >>>>>> container.getPage().getAutoIndex(); >>>>>> - tag.setId(id); >>>>>> >>>>>> return new >>>>>> TransparentWebMarkupContainer(id); >>>>>> } >>>>>> >> >> > |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
|
Hi Juergen and Jesse,
I've debugged WicketLinkTagHandler and everything worked as I have expected it: The WicketTag is always the same instance, coming from the cached markup. Changing its id doesn't make sense to me: This is global state which is accessed for *each* rendering of the container. >An id which is unique with markup file is not guranteed to be unique in the page markup. >That's why resolvers are using the page.autoIndex to create a page unique id Yes, the id has to be unique for components inside their container. That hasn't changed with the patch. Back to debugging Sven On 03/30/2012 12:40 PM, Juergen Donnerstag wrote: > On Fri, Mar 30, 2012 at 3:01 AM, Jesse Long<[hidden email]> wrote: >> Hi Juergen, Sven, >> >> We are not doing away with adding the "auto index" unique number to the >> resultant component id, we are only doing away with modifying the id in the >> cached markup tag. > The patched changed the resolver and not the filter. Hence the markup > loaded into the cache is still the same, but the id presented at > render time is now a different. > >> The uniqueness of the id of the component returned by the resolve() method >> is still being guaranteed (as far as I understand) because it takes the >> original id from the tag and append the current page's next auto index >> number. The component (TransparentWebMarkupContainer) is then created with >> this modified id. > sorry but this is also not correct. The markup of a page is composed > of potentially many markup files (fragments, panels, inheritance, > etc.). An id which is unique with markup file is not guranteed to be > unique in the page markup. That's why resolvers are using the > page.autoIndex to create a page unique id > >> All I'm asking in this patch is to not modify the original id stored in the >> markup tag, because its not necessary. (AFAIK). >> >> Anyway, the resultant component id is now (after the patch) as unique as it >> is on the first render without the patch... > sorry, but this is not true. > >> This problem is actually very serious. I have a base page which 90% of my >> pages extend. Without the patch, on every single render, the id stored in >> the tag has a few bytes appended, causing the id of the resultant component >> to be a few bytes longer (for every render of every page in any session). >> I'm not sure if these ids get stored in memory after the render (I dont know >> wicket internals well enough to tell) or stored in the page store. If they >> do, its a big memory leak. >> >> At the very least its spews out huge amounts of data in the debug logging. > I very well understand your pain point, but lets fix the problem > properly and not introduce a bug with an enhancement. > > Juergen > >> Cheers, >> Jesse >> >> >> >> On 29/03/2012 22:39, Sven Meier wrote: >>> Hi, >>> >>>> An artifical markup which creates duplicate IDs can be constructed. >>> all auto labels' tags have the same id already (its class name) so no need >>> to create some artificial markup. >>> >>>> To me the applied patch leaves us with a bigger problem than it fixes. >>> If the id of the tag in the cached markup is altered on each render, the >>> id can get quite long pretty fast. As the reporter of the issues has stated, >>> this is a 'big' problem ;). >>> >>>> we should fix it properly >>> Agreed. Obviously I'm missing a key concept here. >>> I'll try to debug this issue tomorrow to understand the problem you've >>> identified. >>> >>> Sven >>> >>> On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >>>> Hi Sven >>>> >>>>> I double checked with how other filters/resolvers are doing it and I >>>>> found >>>>> AutoLabelTextResolver to leave the tag id untouched too. >>>> which is a fault as well. Same risk. >>>> >>>>>> May be a Component's flag bit can be used for that purpose. >>>>> Shouldn't that be a flag on the markup tag instead then? >>>> No, because the Pages are made of potentially many markups. Since the >>>> ID has no info about hierarchy, the number appended is the only thing >>>> that makes it page unique. Hence it can not be per markup. Since every >>>> tag gets associated with a Component, the Component is the right place >>>> for the flag. >>>> >>>>>> I don't know how likely it is and I know we have no tests for it. >>>>> Yeah, I didn't find one either. I'm not sure I understand where's a >>>>> possible >>>>> problem here. It's hard to discuss this issues without an actual case. >>>> An artifical markup which creates duplicate IDs can be constructed. >>>> I've not tried it but I don't expect it to be too difficult. Knowing >>>> there is a potential risk which will be very hard to find by a normal >>>> user, makes me belief we should fix it properly, To me the applied >>>> patch leaves us with a bigger problem than it fixes. >>>> >>>> Juergen >>>> >>>>> Sven >>>>> >>>>> >>>>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>>>> Hi Sven, >>>>>> >>>>>> the change introduces a theoretical risk of duplicate ids. I don't >>>>>> know how likely it is and I know we have no tests for it. But Filters >>>>>> are created per markup file and modify the markup upon loading, prior >>>>>> to caching. Which also means that the Page is not available. In case >>>>>> of inheritance it might not even be the same Page. To be absolutely >>>>>> sure the ID is page-unique, it gets update in the Resolvers where the >>>>>> Page instance is available. >>>>>> >>>>>> The root cause is that we don't validate if the ID has been updated >>>>>> already and avoid subsequent updates. We have no means to do that >>>>>> check. A number at the end of the ID would not be sufficient. May be a >>>>>> Component's flag bit can be used for that purpose. >>>>>> >>>>>> I understand the problem in your application, but users will have an >>>>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>>>> it's worth it. May be the approach outlined above solved your problem >>>>>> as well, but in a safe manner. >>>>>> >>>>>> Juergen >>>>>> >>>>>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> wrote: >>>>>>> Updated Branches: >>>>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>>>> >>>>>>> >>>>>>> WICKET-4484 do not change tag id when resolving component >>>>>>> >>>>>>> >>>>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>>>> Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>>>> >>>>>>> Branch: refs/heads/wicket-1.5.x >>>>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>>>> Parents: c6c45a5 >>>>>>> Author: Sven Meier<[hidden email]> >>>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>>> Committer: Sven Meier<[hidden email]> >>>>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>>>> >>>>>>> ---------------------------------------------------------------------- >>>>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>>>> ---------------------------------------------------------------------- >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>> ---------------------------------------------------------------------- >>>>>>> diff --git >>>>>>> >>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>> >>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>> index 689133e..8779b6f 100644 >>>>>>> --- >>>>>>> >>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>> +++ >>>>>>> >>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>>>> AbstractMarkupFilter implements ICompo >>>>>>> if (wtag.isLinkTag()&& (wtag.getNamespace() >>>>>>> != >>>>>>> null)) >>>>>>> { >>>>>>> String id = tag.getId() + "-" + >>>>>>> container.getPage().getAutoIndex(); >>>>>>> - tag.setId(id); >>>>>>> >>>>>>> return new >>>>>>> TransparentWebMarkupContainer(id); >>>>>>> } >>>>>>> >>> |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
|
On Fri, Mar 30, 2012 at 3:57 PM, Sven Meier <[hidden email]> wrote:
> Hi Juergen and Jesse, > > I've debugged WicketLinkTagHandler and everything worked as I have expected > it: > > The WicketTag is always the same instance, coming from the cached markup. > Changing its id doesn't make sense to me: This is global state which is > accessed for *each* rendering of the container. > > >>An id which is unique with markup file is not guranteed to be unique in the >> page markup. >>That's why resolvers are using the page.autoIndex to create a page unique >> id > > Yes, the id has to be unique for components inside their container. That > hasn't changed with the patch. > From a component resolution point of view you are right, but I think you are ignoring Javascript here. wicket-ids may become become ids and they need to be page unique, correct? Juergen > Back to debugging > Sven > > > > On 03/30/2012 12:40 PM, Juergen Donnerstag wrote: >> >> On Fri, Mar 30, 2012 at 3:01 AM, Jesse Long<[hidden email]> >> wrote: >>> >>> Hi Juergen, Sven, >>> >>> We are not doing away with adding the "auto index" unique number to the >>> resultant component id, we are only doing away with modifying the id in >>> the >>> cached markup tag. >> >> The patched changed the resolver and not the filter. Hence the markup >> loaded into the cache is still the same, but the id presented at >> render time is now a different. >> >>> The uniqueness of the id of the component returned by the resolve() >>> method >>> is still being guaranteed (as far as I understand) because it takes the >>> original id from the tag and append the current page's next auto index >>> number. The component (TransparentWebMarkupContainer) is then created >>> with >>> this modified id. >> >> sorry but this is also not correct. The markup of a page is composed >> of potentially many markup files (fragments, panels, inheritance, >> etc.). An id which is unique with markup file is not guranteed to be >> unique in the page markup. That's why resolvers are using the >> page.autoIndex to create a page unique id >> >>> All I'm asking in this patch is to not modify the original id stored in >>> the >>> markup tag, because its not necessary. (AFAIK). >>> >>> Anyway, the resultant component id is now (after the patch) as unique as >>> it >>> is on the first render without the patch... >> >> sorry, but this is not true. >> >>> This problem is actually very serious. I have a base page which 90% of my >>> pages extend. Without the patch, on every single render, the id stored in >>> the tag has a few bytes appended, causing the id of the resultant >>> component >>> to be a few bytes longer (for every render of every page in any session). >>> I'm not sure if these ids get stored in memory after the render (I dont >>> know >>> wicket internals well enough to tell) or stored in the page store. If >>> they >>> do, its a big memory leak. >>> >>> At the very least its spews out huge amounts of data in the debug >>> logging. >> >> I very well understand your pain point, but lets fix the problem >> properly and not introduce a bug with an enhancement. >> >> Juergen >> >>> Cheers, >>> Jesse >>> >>> >>> >>> On 29/03/2012 22:39, Sven Meier wrote: >>>> >>>> Hi, >>>> >>>>> An artifical markup which creates duplicate IDs can be constructed. >>>> >>>> all auto labels' tags have the same id already (its class name) so no >>>> need >>>> to create some artificial markup. >>>> >>>>> To me the applied patch leaves us with a bigger problem than it fixes. >>>> >>>> If the id of the tag in the cached markup is altered on each render, the >>>> id can get quite long pretty fast. As the reporter of the issues has >>>> stated, >>>> this is a 'big' problem ;). >>>> >>>>> we should fix it properly >>>> >>>> Agreed. Obviously I'm missing a key concept here. >>>> I'll try to debug this issue tomorrow to understand the problem you've >>>> identified. >>>> >>>> Sven >>>> >>>> On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >>>>> >>>>> Hi Sven >>>>> >>>>>> I double checked with how other filters/resolvers are doing it and I >>>>>> found >>>>>> AutoLabelTextResolver to leave the tag id untouched too. >>>>> >>>>> which is a fault as well. Same risk. >>>>> >>>>>>> May be a Component's flag bit can be used for that purpose. >>>>>> >>>>>> Shouldn't that be a flag on the markup tag instead then? >>>>> >>>>> No, because the Pages are made of potentially many markups. Since the >>>>> ID has no info about hierarchy, the number appended is the only thing >>>>> that makes it page unique. Hence it can not be per markup. Since every >>>>> tag gets associated with a Component, the Component is the right place >>>>> for the flag. >>>>> >>>>>>> I don't know how likely it is and I know we have no tests for it. >>>>>> >>>>>> Yeah, I didn't find one either. I'm not sure I understand where's a >>>>>> possible >>>>>> problem here. It's hard to discuss this issues without an actual case. >>>>> >>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>> I've not tried it but I don't expect it to be too difficult. Knowing >>>>> there is a potential risk which will be very hard to find by a normal >>>>> user, makes me belief we should fix it properly, To me the applied >>>>> patch leaves us with a bigger problem than it fixes. >>>>> >>>>> Juergen >>>>> >>>>>> Sven >>>>>> >>>>>> >>>>>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>>>>> >>>>>>> Hi Sven, >>>>>>> >>>>>>> the change introduces a theoretical risk of duplicate ids. I don't >>>>>>> know how likely it is and I know we have no tests for it. But Filters >>>>>>> are created per markup file and modify the markup upon loading, prior >>>>>>> to caching. Which also means that the Page is not available. In case >>>>>>> of inheritance it might not even be the same Page. To be absolutely >>>>>>> sure the ID is page-unique, it gets update in the Resolvers where the >>>>>>> Page instance is available. >>>>>>> >>>>>>> The root cause is that we don't validate if the ID has been updated >>>>>>> already and avoid subsequent updates. We have no means to do that >>>>>>> check. A number at the end of the ID would not be sufficient. May be >>>>>>> a >>>>>>> Component's flag bit can be used for that purpose. >>>>>>> >>>>>>> I understand the problem in your application, but users will have an >>>>>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>>>>> it's worth it. May be the approach outlined above solved your problem >>>>>>> as well, but in a safe manner. >>>>>>> >>>>>>> Juergen >>>>>>> >>>>>>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> wrote: >>>>>>>> >>>>>>>> Updated Branches: >>>>>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>>>>> >>>>>>>> >>>>>>>> WICKET-4484 do not change tag id when resolving component >>>>>>>> >>>>>>>> >>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>>>>> Commit: >>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>>>>> >>>>>>>> Branch: refs/heads/wicket-1.5.x >>>>>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>>>>> Parents: c6c45a5 >>>>>>>> Author: Sven Meier<[hidden email]> >>>>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>> Committer: Sven Meier<[hidden email]> >>>>>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>> >>>>>>>> >>>>>>>> ---------------------------------------------------------------------- >>>>>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>>>>> >>>>>>>> ---------------------------------------------------------------------- >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>> >>>>>>>> ---------------------------------------------------------------------- >>>>>>>> diff --git >>>>>>>> >>>>>>>> >>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>> >>>>>>>> >>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>> index 689133e..8779b6f 100644 >>>>>>>> --- >>>>>>>> >>>>>>>> >>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>> +++ >>>>>>>> >>>>>>>> >>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>>>>> AbstractMarkupFilter implements ICompo >>>>>>>> if (wtag.isLinkTag()&& >>>>>>>> (wtag.getNamespace() >>>>>>>> != >>>>>>>> null)) >>>>>>>> { >>>>>>>> String id = tag.getId() + "-" + >>>>>>>> container.getPage().getAutoIndex(); >>>>>>>> - tag.setId(id); >>>>>>>> >>>>>>>> return new >>>>>>>> TransparentWebMarkupContainer(id); >>>>>>>> } >>>>>>>> >>>> > |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
|
Javascript ids are session unique anyway - just prefixed with the
component id in non-deployment config. Sven On 03/30/2012 04:11 PM, Juergen Donnerstag wrote: > On Fri, Mar 30, 2012 at 3:57 PM, Sven Meier<[hidden email]> wrote: >> Hi Juergen and Jesse, >> >> I've debugged WicketLinkTagHandler and everything worked as I have expected >> it: >> >> The WicketTag is always the same instance, coming from the cached markup. >> Changing its id doesn't make sense to me: This is global state which is >> accessed for *each* rendering of the container. >> >> >>> An id which is unique with markup file is not guranteed to be unique in the >>> page markup. >>> That's why resolvers are using the page.autoIndex to create a page unique >>> id >> Yes, the id has to be unique for components inside their container. That >> hasn't changed with the patch. >> > From a component resolution point of view you are right, but I think > you are ignoring Javascript here. wicket-ids may become become ids and > they need to be page unique, correct? > > Juergen > >> Back to debugging >> Sven >> >> >> >> On 03/30/2012 12:40 PM, Juergen Donnerstag wrote: >>> On Fri, Mar 30, 2012 at 3:01 AM, Jesse Long<[hidden email]> >>> wrote: >>>> Hi Juergen, Sven, >>>> >>>> We are not doing away with adding the "auto index" unique number to the >>>> resultant component id, we are only doing away with modifying the id in >>>> the >>>> cached markup tag. >>> The patched changed the resolver and not the filter. Hence the markup >>> loaded into the cache is still the same, but the id presented at >>> render time is now a different. >>> >>>> The uniqueness of the id of the component returned by the resolve() >>>> method >>>> is still being guaranteed (as far as I understand) because it takes the >>>> original id from the tag and append the current page's next auto index >>>> number. The component (TransparentWebMarkupContainer) is then created >>>> with >>>> this modified id. >>> sorry but this is also not correct. The markup of a page is composed >>> of potentially many markup files (fragments, panels, inheritance, >>> etc.). An id which is unique with markup file is not guranteed to be >>> unique in the page markup. That's why resolvers are using the >>> page.autoIndex to create a page unique id >>> >>>> All I'm asking in this patch is to not modify the original id stored in >>>> the >>>> markup tag, because its not necessary. (AFAIK). >>>> >>>> Anyway, the resultant component id is now (after the patch) as unique as >>>> it >>>> is on the first render without the patch... >>> sorry, but this is not true. >>> >>>> This problem is actually very serious. I have a base page which 90% of my >>>> pages extend. Without the patch, on every single render, the id stored in >>>> the tag has a few bytes appended, causing the id of the resultant >>>> component >>>> to be a few bytes longer (for every render of every page in any session). >>>> I'm not sure if these ids get stored in memory after the render (I dont >>>> know >>>> wicket internals well enough to tell) or stored in the page store. If >>>> they >>>> do, its a big memory leak. >>>> >>>> At the very least its spews out huge amounts of data in the debug >>>> logging. >>> I very well understand your pain point, but lets fix the problem >>> properly and not introduce a bug with an enhancement. >>> >>> Juergen >>> >>>> Cheers, >>>> Jesse >>>> >>>> >>>> >>>> On 29/03/2012 22:39, Sven Meier wrote: >>>>> Hi, >>>>> >>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>> all auto labels' tags have the same id already (its class name) so no >>>>> need >>>>> to create some artificial markup. >>>>> >>>>>> To me the applied patch leaves us with a bigger problem than it fixes. >>>>> If the id of the tag in the cached markup is altered on each render, the >>>>> id can get quite long pretty fast. As the reporter of the issues has >>>>> stated, >>>>> this is a 'big' problem ;). >>>>> >>>>>> we should fix it properly >>>>> Agreed. Obviously I'm missing a key concept here. >>>>> I'll try to debug this issue tomorrow to understand the problem you've >>>>> identified. >>>>> >>>>> Sven >>>>> >>>>> On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >>>>>> Hi Sven >>>>>> >>>>>>> I double checked with how other filters/resolvers are doing it and I >>>>>>> found >>>>>>> AutoLabelTextResolver to leave the tag id untouched too. >>>>>> which is a fault as well. Same risk. >>>>>> >>>>>>>> May be a Component's flag bit can be used for that purpose. >>>>>>> Shouldn't that be a flag on the markup tag instead then? >>>>>> No, because the Pages are made of potentially many markups. Since the >>>>>> ID has no info about hierarchy, the number appended is the only thing >>>>>> that makes it page unique. Hence it can not be per markup. Since every >>>>>> tag gets associated with a Component, the Component is the right place >>>>>> for the flag. >>>>>> >>>>>>>> I don't know how likely it is and I know we have no tests for it. >>>>>>> Yeah, I didn't find one either. I'm not sure I understand where's a >>>>>>> possible >>>>>>> problem here. It's hard to discuss this issues without an actual case. >>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>> I've not tried it but I don't expect it to be too difficult. Knowing >>>>>> there is a potential risk which will be very hard to find by a normal >>>>>> user, makes me belief we should fix it properly, To me the applied >>>>>> patch leaves us with a bigger problem than it fixes. >>>>>> >>>>>> Juergen >>>>>> >>>>>>> Sven >>>>>>> >>>>>>> >>>>>>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>>>>>> Hi Sven, >>>>>>>> >>>>>>>> the change introduces a theoretical risk of duplicate ids. I don't >>>>>>>> know how likely it is and I know we have no tests for it. But Filters >>>>>>>> are created per markup file and modify the markup upon loading, prior >>>>>>>> to caching. Which also means that the Page is not available. In case >>>>>>>> of inheritance it might not even be the same Page. To be absolutely >>>>>>>> sure the ID is page-unique, it gets update in the Resolvers where the >>>>>>>> Page instance is available. >>>>>>>> >>>>>>>> The root cause is that we don't validate if the ID has been updated >>>>>>>> already and avoid subsequent updates. We have no means to do that >>>>>>>> check. A number at the end of the ID would not be sufficient. May be >>>>>>>> a >>>>>>>> Component's flag bit can be used for that purpose. >>>>>>>> >>>>>>>> I understand the problem in your application, but users will have an >>>>>>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>>>>>> it's worth it. May be the approach outlined above solved your problem >>>>>>>> as well, but in a safe manner. >>>>>>>> >>>>>>>> Juergen >>>>>>>> >>>>>>>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> wrote: >>>>>>>>> Updated Branches: >>>>>>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>>>>>> >>>>>>>>> >>>>>>>>> WICKET-4484 do not change tag id when resolving component >>>>>>>>> >>>>>>>>> >>>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>>>>>> Commit: >>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>>>>>> >>>>>>>>> Branch: refs/heads/wicket-1.5.x >>>>>>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>>>>>> Parents: c6c45a5 >>>>>>>>> Author: Sven Meier<[hidden email]> >>>>>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>> Committer: Sven Meier<[hidden email]> >>>>>>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>> >>>>>>>>> >>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>>>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>>>>>> >>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>> >>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>> diff --git >>>>>>>>> >>>>>>>>> >>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>> >>>>>>>>> >>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>> index 689133e..8779b6f 100644 >>>>>>>>> --- >>>>>>>>> >>>>>>>>> >>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>> +++ >>>>>>>>> >>>>>>>>> >>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>>>>>> AbstractMarkupFilter implements ICompo >>>>>>>>> if (wtag.isLinkTag()&& >>>>>>>>> (wtag.getNamespace() >>>>>>>>> != >>>>>>>>> null)) >>>>>>>>> { >>>>>>>>> String id = tag.getId() + "-" + >>>>>>>>> container.getPage().getAutoIndex(); >>>>>>>>> - tag.setId(id); >>>>>>>>> >>>>>>>>> return new >>>>>>>>> TransparentWebMarkupContainer(id); >>>>>>>>> } >>>>>>>>> |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
|
but with setOutputMarkupId() we make sure the most appropriate ID is
used, if one hasn't already been assigned. And in case wicket:id is present, it'll be used. And IDs have to be page-unique. That is no tag on the same page must have the same ID. Juergen On Fri, Mar 30, 2012 at 4:33 PM, Sven Meier <[hidden email]> wrote: > Javascript ids are session unique anyway - just prefixed with the component > id in non-deployment config. > > Sven > > > On 03/30/2012 04:11 PM, Juergen Donnerstag wrote: >> >> On Fri, Mar 30, 2012 at 3:57 PM, Sven Meier<[hidden email]> wrote: >>> >>> Hi Juergen and Jesse, >>> >>> I've debugged WicketLinkTagHandler and everything worked as I have >>> expected >>> it: >>> >>> The WicketTag is always the same instance, coming from the cached markup. >>> Changing its id doesn't make sense to me: This is global state which is >>> accessed for *each* rendering of the container. >>> >>> >>>> An id which is unique with markup file is not guranteed to be unique in >>>> the >>>> page markup. >>>> That's why resolvers are using the page.autoIndex to create a page >>>> unique >>>> id >>> >>> Yes, the id has to be unique for components inside their container. That >>> hasn't changed with the patch. >>> >> From a component resolution point of view you are right, but I think >> you are ignoring Javascript here. wicket-ids may become become ids and >> they need to be page unique, correct? >> >> Juergen >> >>> Back to debugging >>> Sven >>> >>> >>> >>> On 03/30/2012 12:40 PM, Juergen Donnerstag wrote: >>>> >>>> On Fri, Mar 30, 2012 at 3:01 AM, Jesse Long<[hidden email]> >>>> wrote: >>>>> >>>>> Hi Juergen, Sven, >>>>> >>>>> We are not doing away with adding the "auto index" unique number to the >>>>> resultant component id, we are only doing away with modifying the id in >>>>> the >>>>> cached markup tag. >>>> >>>> The patched changed the resolver and not the filter. Hence the markup >>>> loaded into the cache is still the same, but the id presented at >>>> render time is now a different. >>>> >>>>> The uniqueness of the id of the component returned by the resolve() >>>>> method >>>>> is still being guaranteed (as far as I understand) because it takes the >>>>> original id from the tag and append the current page's next auto index >>>>> number. The component (TransparentWebMarkupContainer) is then created >>>>> with >>>>> this modified id. >>>> >>>> sorry but this is also not correct. The markup of a page is composed >>>> of potentially many markup files (fragments, panels, inheritance, >>>> etc.). An id which is unique with markup file is not guranteed to be >>>> unique in the page markup. That's why resolvers are using the >>>> page.autoIndex to create a page unique id >>>> >>>>> All I'm asking in this patch is to not modify the original id stored in >>>>> the >>>>> markup tag, because its not necessary. (AFAIK). >>>>> >>>>> Anyway, the resultant component id is now (after the patch) as unique >>>>> as >>>>> it >>>>> is on the first render without the patch... >>>> >>>> sorry, but this is not true. >>>> >>>>> This problem is actually very serious. I have a base page which 90% of >>>>> my >>>>> pages extend. Without the patch, on every single render, the id stored >>>>> in >>>>> the tag has a few bytes appended, causing the id of the resultant >>>>> component >>>>> to be a few bytes longer (for every render of every page in any >>>>> session). >>>>> I'm not sure if these ids get stored in memory after the render (I dont >>>>> know >>>>> wicket internals well enough to tell) or stored in the page store. If >>>>> they >>>>> do, its a big memory leak. >>>>> >>>>> At the very least its spews out huge amounts of data in the debug >>>>> logging. >>>> >>>> I very well understand your pain point, but lets fix the problem >>>> properly and not introduce a bug with an enhancement. >>>> >>>> Juergen >>>> >>>>> Cheers, >>>>> Jesse >>>>> >>>>> >>>>> >>>>> On 29/03/2012 22:39, Sven Meier wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>> >>>>>> all auto labels' tags have the same id already (its class name) so no >>>>>> need >>>>>> to create some artificial markup. >>>>>> >>>>>>> To me the applied patch leaves us with a bigger problem than it >>>>>>> fixes. >>>>>> >>>>>> If the id of the tag in the cached markup is altered on each render, >>>>>> the >>>>>> id can get quite long pretty fast. As the reporter of the issues has >>>>>> stated, >>>>>> this is a 'big' problem ;). >>>>>> >>>>>>> we should fix it properly >>>>>> >>>>>> Agreed. Obviously I'm missing a key concept here. >>>>>> I'll try to debug this issue tomorrow to understand the problem you've >>>>>> identified. >>>>>> >>>>>> Sven >>>>>> >>>>>> On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >>>>>>> >>>>>>> Hi Sven >>>>>>> >>>>>>>> I double checked with how other filters/resolvers are doing it and I >>>>>>>> found >>>>>>>> AutoLabelTextResolver to leave the tag id untouched too. >>>>>>> >>>>>>> which is a fault as well. Same risk. >>>>>>> >>>>>>>>> May be a Component's flag bit can be used for that purpose. >>>>>>>> >>>>>>>> Shouldn't that be a flag on the markup tag instead then? >>>>>>> >>>>>>> No, because the Pages are made of potentially many markups. Since the >>>>>>> ID has no info about hierarchy, the number appended is the only thing >>>>>>> that makes it page unique. Hence it can not be per markup. Since >>>>>>> every >>>>>>> tag gets associated with a Component, the Component is the right >>>>>>> place >>>>>>> for the flag. >>>>>>> >>>>>>>>> I don't know how likely it is and I know we have no tests for it. >>>>>>>> >>>>>>>> Yeah, I didn't find one either. I'm not sure I understand where's a >>>>>>>> possible >>>>>>>> problem here. It's hard to discuss this issues without an actual >>>>>>>> case. >>>>>>> >>>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>>> I've not tried it but I don't expect it to be too difficult. Knowing >>>>>>> there is a potential risk which will be very hard to find by a normal >>>>>>> user, makes me belief we should fix it properly, To me the applied >>>>>>> patch leaves us with a bigger problem than it fixes. >>>>>>> >>>>>>> Juergen >>>>>>> >>>>>>>> Sven >>>>>>>> >>>>>>>> >>>>>>>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>>>>>>> >>>>>>>>> Hi Sven, >>>>>>>>> >>>>>>>>> the change introduces a theoretical risk of duplicate ids. I don't >>>>>>>>> know how likely it is and I know we have no tests for it. But >>>>>>>>> Filters >>>>>>>>> are created per markup file and modify the markup upon loading, >>>>>>>>> prior >>>>>>>>> to caching. Which also means that the Page is not available. In >>>>>>>>> case >>>>>>>>> of inheritance it might not even be the same Page. To be absolutely >>>>>>>>> sure the ID is page-unique, it gets update in the Resolvers where >>>>>>>>> the >>>>>>>>> Page instance is available. >>>>>>>>> >>>>>>>>> The root cause is that we don't validate if the ID has been updated >>>>>>>>> already and avoid subsequent updates. We have no means to do that >>>>>>>>> check. A number at the end of the ID would not be sufficient. May >>>>>>>>> be >>>>>>>>> a >>>>>>>>> Component's flag bit can be used for that purpose. >>>>>>>>> >>>>>>>>> I understand the problem in your application, but users will have >>>>>>>>> an >>>>>>>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>>>>>>> it's worth it. May be the approach outlined above solved your >>>>>>>>> problem >>>>>>>>> as well, but in a safe manner. >>>>>>>>> >>>>>>>>> Juergen >>>>>>>>> >>>>>>>>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Updated Branches: >>>>>>>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> WICKET-4484 do not change tag id when resolving component >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>>>>>>> Commit: >>>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>>>>>>> >>>>>>>>>> Branch: refs/heads/wicket-1.5.x >>>>>>>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>>>>>>> Parents: c6c45a5 >>>>>>>>>> Author: Sven Meier<[hidden email]> >>>>>>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>>> Committer: Sven Meier<[hidden email]> >>>>>>>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>>>>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>> diff --git >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>> index 689133e..8779b6f 100644 >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>> +++ >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>>>>>>> AbstractMarkupFilter implements ICompo >>>>>>>>>> if (wtag.isLinkTag()&& >>>>>>>>>> (wtag.getNamespace() >>>>>>>>>> != >>>>>>>>>> null)) >>>>>>>>>> { >>>>>>>>>> String id = tag.getId() + "-" + >>>>>>>>>> container.getPage().getAutoIndex(); >>>>>>>>>> - tag.setId(id); >>>>>>>>>> >>>>>>>>>> return new >>>>>>>>>> TransparentWebMarkupContainer(id); >>>>>>>>>> } >>>>>>>>>> > |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
In reply to this post by Sven Meier
On Fri, Mar 30, 2012 at 4:33 PM, Sven Meier <[hidden email]> wrote:
> Javascript ids are session unique anyway - just prefixed with the component > id in non-deployment config. I didn't get this. Juergen says that now there could be a ComponentTag with unique id in its markup container (e.g. a Panel), but could clash with the id of another ComponentTag in another markup container in the same page. Sometimes the ComponentTag's id is directly set as Component's markupId (this is what you meant with Javascript id, right?). In such case the user application can have problems. I have seen code that does: setMarkupId(getId()). I haven't seen the same with componentTag.getId() but maybe someone does this ... > > Sven > > > On 03/30/2012 04:11 PM, Juergen Donnerstag wrote: >> >> On Fri, Mar 30, 2012 at 3:57 PM, Sven Meier<[hidden email]> wrote: >>> >>> Hi Juergen and Jesse, >>> >>> I've debugged WicketLinkTagHandler and everything worked as I have >>> expected >>> it: >>> >>> The WicketTag is always the same instance, coming from the cached markup. >>> Changing its id doesn't make sense to me: This is global state which is >>> accessed for *each* rendering of the container. >>> >>> >>>> An id which is unique with markup file is not guranteed to be unique in >>>> the >>>> page markup. >>>> That's why resolvers are using the page.autoIndex to create a page >>>> unique >>>> id >>> >>> Yes, the id has to be unique for components inside their container. That >>> hasn't changed with the patch. >>> >> From a component resolution point of view you are right, but I think >> you are ignoring Javascript here. wicket-ids may become become ids and >> they need to be page unique, correct? >> >> Juergen >> >>> Back to debugging >>> Sven >>> >>> >>> >>> On 03/30/2012 12:40 PM, Juergen Donnerstag wrote: >>>> >>>> On Fri, Mar 30, 2012 at 3:01 AM, Jesse Long<[hidden email]> >>>> wrote: >>>>> >>>>> Hi Juergen, Sven, >>>>> >>>>> We are not doing away with adding the "auto index" unique number to the >>>>> resultant component id, we are only doing away with modifying the id in >>>>> the >>>>> cached markup tag. >>>> >>>> The patched changed the resolver and not the filter. Hence the markup >>>> loaded into the cache is still the same, but the id presented at >>>> render time is now a different. >>>> >>>>> The uniqueness of the id of the component returned by the resolve() >>>>> method >>>>> is still being guaranteed (as far as I understand) because it takes the >>>>> original id from the tag and append the current page's next auto index >>>>> number. The component (TransparentWebMarkupContainer) is then created >>>>> with >>>>> this modified id. >>>> >>>> sorry but this is also not correct. The markup of a page is composed >>>> of potentially many markup files (fragments, panels, inheritance, >>>> etc.). An id which is unique with markup file is not guranteed to be >>>> unique in the page markup. That's why resolvers are using the >>>> page.autoIndex to create a page unique id >>>> >>>>> All I'm asking in this patch is to not modify the original id stored in >>>>> the >>>>> markup tag, because its not necessary. (AFAIK). >>>>> >>>>> Anyway, the resultant component id is now (after the patch) as unique >>>>> as >>>>> it >>>>> is on the first render without the patch... >>>> >>>> sorry, but this is not true. >>>> >>>>> This problem is actually very serious. I have a base page which 90% of >>>>> my >>>>> pages extend. Without the patch, on every single render, the id stored >>>>> in >>>>> the tag has a few bytes appended, causing the id of the resultant >>>>> component >>>>> to be a few bytes longer (for every render of every page in any >>>>> session). >>>>> I'm not sure if these ids get stored in memory after the render (I dont >>>>> know >>>>> wicket internals well enough to tell) or stored in the page store. If >>>>> they >>>>> do, its a big memory leak. >>>>> >>>>> At the very least its spews out huge amounts of data in the debug >>>>> logging. >>>> >>>> I very well understand your pain point, but lets fix the problem >>>> properly and not introduce a bug with an enhancement. >>>> >>>> Juergen >>>> >>>>> Cheers, >>>>> Jesse >>>>> >>>>> >>>>> >>>>> On 29/03/2012 22:39, Sven Meier wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>> >>>>>> all auto labels' tags have the same id already (its class name) so no >>>>>> need >>>>>> to create some artificial markup. >>>>>> >>>>>>> To me the applied patch leaves us with a bigger problem than it >>>>>>> fixes. >>>>>> >>>>>> If the id of the tag in the cached markup is altered on each render, >>>>>> the >>>>>> id can get quite long pretty fast. As the reporter of the issues has >>>>>> stated, >>>>>> this is a 'big' problem ;). >>>>>> >>>>>>> we should fix it properly >>>>>> >>>>>> Agreed. Obviously I'm missing a key concept here. >>>>>> I'll try to debug this issue tomorrow to understand the problem you've >>>>>> identified. >>>>>> >>>>>> Sven >>>>>> >>>>>> On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >>>>>>> >>>>>>> Hi Sven >>>>>>> >>>>>>>> I double checked with how other filters/resolvers are doing it and I >>>>>>>> found >>>>>>>> AutoLabelTextResolver to leave the tag id untouched too. >>>>>>> >>>>>>> which is a fault as well. Same risk. >>>>>>> >>>>>>>>> May be a Component's flag bit can be used for that purpose. >>>>>>>> >>>>>>>> Shouldn't that be a flag on the markup tag instead then? >>>>>>> >>>>>>> No, because the Pages are made of potentially many markups. Since the >>>>>>> ID has no info about hierarchy, the number appended is the only thing >>>>>>> that makes it page unique. Hence it can not be per markup. Since >>>>>>> every >>>>>>> tag gets associated with a Component, the Component is the right >>>>>>> place >>>>>>> for the flag. >>>>>>> >>>>>>>>> I don't know how likely it is and I know we have no tests for it. >>>>>>>> >>>>>>>> Yeah, I didn't find one either. I'm not sure I understand where's a >>>>>>>> possible >>>>>>>> problem here. It's hard to discuss this issues without an actual >>>>>>>> case. >>>>>>> >>>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>>> I've not tried it but I don't expect it to be too difficult. Knowing >>>>>>> there is a potential risk which will be very hard to find by a normal >>>>>>> user, makes me belief we should fix it properly, To me the applied >>>>>>> patch leaves us with a bigger problem than it fixes. >>>>>>> >>>>>>> Juergen >>>>>>> >>>>>>>> Sven >>>>>>>> >>>>>>>> >>>>>>>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>>>>>>> >>>>>>>>> Hi Sven, >>>>>>>>> >>>>>>>>> the change introduces a theoretical risk of duplicate ids. I don't >>>>>>>>> know how likely it is and I know we have no tests for it. But >>>>>>>>> Filters >>>>>>>>> are created per markup file and modify the markup upon loading, >>>>>>>>> prior >>>>>>>>> to caching. Which also means that the Page is not available. In >>>>>>>>> case >>>>>>>>> of inheritance it might not even be the same Page. To be absolutely >>>>>>>>> sure the ID is page-unique, it gets update in the Resolvers where >>>>>>>>> the >>>>>>>>> Page instance is available. >>>>>>>>> >>>>>>>>> The root cause is that we don't validate if the ID has been updated >>>>>>>>> already and avoid subsequent updates. We have no means to do that >>>>>>>>> check. A number at the end of the ID would not be sufficient. May >>>>>>>>> be >>>>>>>>> a >>>>>>>>> Component's flag bit can be used for that purpose. >>>>>>>>> >>>>>>>>> I understand the problem in your application, but users will have >>>>>>>>> an >>>>>>>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>>>>>>> it's worth it. May be the approach outlined above solved your >>>>>>>>> problem >>>>>>>>> as well, but in a safe manner. >>>>>>>>> >>>>>>>>> Juergen >>>>>>>>> >>>>>>>>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Updated Branches: >>>>>>>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> WICKET-4484 do not change tag id when resolving component >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>>>>>>> Commit: >>>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>>>>>>> >>>>>>>>>> Branch: refs/heads/wicket-1.5.x >>>>>>>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>>>>>>> Parents: c6c45a5 >>>>>>>>>> Author: Sven Meier<[hidden email]> >>>>>>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>>> Committer: Sven Meier<[hidden email]> >>>>>>>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>>>>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>> diff --git >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>> index 689133e..8779b6f 100644 >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>> +++ >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>>>>>>> AbstractMarkupFilter implements ICompo >>>>>>>>>> if (wtag.isLinkTag()&& >>>>>>>>>> (wtag.getNamespace() >>>>>>>>>> != >>>>>>>>>> null)) >>>>>>>>>> { >>>>>>>>>> String id = tag.getId() + "-" + >>>>>>>>>> container.getPage().getAutoIndex(); >>>>>>>>>> - tag.setId(id); >>>>>>>>>> >>>>>>>>>> return new >>>>>>>>>> TransparentWebMarkupContainer(id); >>>>>>>>>> } >>>>>>>>>> > -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
|
In reply to this post by jdonnerstag
On Fri, Mar 30, 2012 at 4:45 PM, Juergen Donnerstag
<[hidden email]> wrote: > but with setOutputMarkupId() we make sure the most appropriate ID is > used, if one hasn't already been assigned. And in case wicket:id is You mean if "id" attribute is present. 'wicket:id' should be there to be a Wicket Component. > present, it'll be used. And IDs have to be page-unique. That is no tag > on the same page must have the same ID. > > Juergen > > On Fri, Mar 30, 2012 at 4:33 PM, Sven Meier <[hidden email]> wrote: >> Javascript ids are session unique anyway - just prefixed with the component >> id in non-deployment config. >> >> Sven >> >> >> On 03/30/2012 04:11 PM, Juergen Donnerstag wrote: >>> >>> On Fri, Mar 30, 2012 at 3:57 PM, Sven Meier<[hidden email]> wrote: >>>> >>>> Hi Juergen and Jesse, >>>> >>>> I've debugged WicketLinkTagHandler and everything worked as I have >>>> expected >>>> it: >>>> >>>> The WicketTag is always the same instance, coming from the cached markup. >>>> Changing its id doesn't make sense to me: This is global state which is >>>> accessed for *each* rendering of the container. >>>> >>>> >>>>> An id which is unique with markup file is not guranteed to be unique in >>>>> the >>>>> page markup. >>>>> That's why resolvers are using the page.autoIndex to create a page >>>>> unique >>>>> id >>>> >>>> Yes, the id has to be unique for components inside their container. That >>>> hasn't changed with the patch. >>>> >>> From a component resolution point of view you are right, but I think >>> you are ignoring Javascript here. wicket-ids may become become ids and >>> they need to be page unique, correct? >>> >>> Juergen >>> >>>> Back to debugging >>>> Sven >>>> >>>> >>>> >>>> On 03/30/2012 12:40 PM, Juergen Donnerstag wrote: >>>>> >>>>> On Fri, Mar 30, 2012 at 3:01 AM, Jesse Long<[hidden email]> >>>>> wrote: >>>>>> >>>>>> Hi Juergen, Sven, >>>>>> >>>>>> We are not doing away with adding the "auto index" unique number to the >>>>>> resultant component id, we are only doing away with modifying the id in >>>>>> the >>>>>> cached markup tag. >>>>> >>>>> The patched changed the resolver and not the filter. Hence the markup >>>>> loaded into the cache is still the same, but the id presented at >>>>> render time is now a different. >>>>> >>>>>> The uniqueness of the id of the component returned by the resolve() >>>>>> method >>>>>> is still being guaranteed (as far as I understand) because it takes the >>>>>> original id from the tag and append the current page's next auto index >>>>>> number. The component (TransparentWebMarkupContainer) is then created >>>>>> with >>>>>> this modified id. >>>>> >>>>> sorry but this is also not correct. The markup of a page is composed >>>>> of potentially many markup files (fragments, panels, inheritance, >>>>> etc.). An id which is unique with markup file is not guranteed to be >>>>> unique in the page markup. That's why resolvers are using the >>>>> page.autoIndex to create a page unique id >>>>> >>>>>> All I'm asking in this patch is to not modify the original id stored in >>>>>> the >>>>>> markup tag, because its not necessary. (AFAIK). >>>>>> >>>>>> Anyway, the resultant component id is now (after the patch) as unique >>>>>> as >>>>>> it >>>>>> is on the first render without the patch... >>>>> >>>>> sorry, but this is not true. >>>>> >>>>>> This problem is actually very serious. I have a base page which 90% of >>>>>> my >>>>>> pages extend. Without the patch, on every single render, the id stored >>>>>> in >>>>>> the tag has a few bytes appended, causing the id of the resultant >>>>>> component >>>>>> to be a few bytes longer (for every render of every page in any >>>>>> session). >>>>>> I'm not sure if these ids get stored in memory after the render (I dont >>>>>> know >>>>>> wicket internals well enough to tell) or stored in the page store. If >>>>>> they >>>>>> do, its a big memory leak. >>>>>> >>>>>> At the very least its spews out huge amounts of data in the debug >>>>>> logging. >>>>> >>>>> I very well understand your pain point, but lets fix the problem >>>>> properly and not introduce a bug with an enhancement. >>>>> >>>>> Juergen >>>>> >>>>>> Cheers, >>>>>> Jesse >>>>>> >>>>>> >>>>>> >>>>>> On 29/03/2012 22:39, Sven Meier wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>>> >>>>>>> all auto labels' tags have the same id already (its class name) so no >>>>>>> need >>>>>>> to create some artificial markup. >>>>>>> >>>>>>>> To me the applied patch leaves us with a bigger problem than it >>>>>>>> fixes. >>>>>>> >>>>>>> If the id of the tag in the cached markup is altered on each render, >>>>>>> the >>>>>>> id can get quite long pretty fast. As the reporter of the issues has >>>>>>> stated, >>>>>>> this is a 'big' problem ;). >>>>>>> >>>>>>>> we should fix it properly >>>>>>> >>>>>>> Agreed. Obviously I'm missing a key concept here. >>>>>>> I'll try to debug this issue tomorrow to understand the problem you've >>>>>>> identified. >>>>>>> >>>>>>> Sven >>>>>>> >>>>>>> On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >>>>>>>> >>>>>>>> Hi Sven >>>>>>>> >>>>>>>>> I double checked with how other filters/resolvers are doing it and I >>>>>>>>> found >>>>>>>>> AutoLabelTextResolver to leave the tag id untouched too. >>>>>>>> >>>>>>>> which is a fault as well. Same risk. >>>>>>>> >>>>>>>>>> May be a Component's flag bit can be used for that purpose. >>>>>>>>> >>>>>>>>> Shouldn't that be a flag on the markup tag instead then? >>>>>>>> >>>>>>>> No, because the Pages are made of potentially many markups. Since the >>>>>>>> ID has no info about hierarchy, the number appended is the only thing >>>>>>>> that makes it page unique. Hence it can not be per markup. Since >>>>>>>> every >>>>>>>> tag gets associated with a Component, the Component is the right >>>>>>>> place >>>>>>>> for the flag. >>>>>>>> >>>>>>>>>> I don't know how likely it is and I know we have no tests for it. >>>>>>>>> >>>>>>>>> Yeah, I didn't find one either. I'm not sure I understand where's a >>>>>>>>> possible >>>>>>>>> problem here. It's hard to discuss this issues without an actual >>>>>>>>> case. >>>>>>>> >>>>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>>>> I've not tried it but I don't expect it to be too difficult. Knowing >>>>>>>> there is a potential risk which will be very hard to find by a normal >>>>>>>> user, makes me belief we should fix it properly, To me the applied >>>>>>>> patch leaves us with a bigger problem than it fixes. >>>>>>>> >>>>>>>> Juergen >>>>>>>> >>>>>>>>> Sven >>>>>>>>> >>>>>>>>> >>>>>>>>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>>>>>>>> >>>>>>>>>> Hi Sven, >>>>>>>>>> >>>>>>>>>> the change introduces a theoretical risk of duplicate ids. I don't >>>>>>>>>> know how likely it is and I know we have no tests for it. But >>>>>>>>>> Filters >>>>>>>>>> are created per markup file and modify the markup upon loading, >>>>>>>>>> prior >>>>>>>>>> to caching. Which also means that the Page is not available. In >>>>>>>>>> case >>>>>>>>>> of inheritance it might not even be the same Page. To be absolutely >>>>>>>>>> sure the ID is page-unique, it gets update in the Resolvers where >>>>>>>>>> the >>>>>>>>>> Page instance is available. >>>>>>>>>> >>>>>>>>>> The root cause is that we don't validate if the ID has been updated >>>>>>>>>> already and avoid subsequent updates. We have no means to do that >>>>>>>>>> check. A number at the end of the ID would not be sufficient. May >>>>>>>>>> be >>>>>>>>>> a >>>>>>>>>> Component's flag bit can be used for that purpose. >>>>>>>>>> >>>>>>>>>> I understand the problem in your application, but users will have >>>>>>>>>> an >>>>>>>>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>>>>>>>> it's worth it. May be the approach outlined above solved your >>>>>>>>>> problem >>>>>>>>>> as well, but in a safe manner. >>>>>>>>>> >>>>>>>>>> Juergen >>>>>>>>>> >>>>>>>>>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> Updated Branches: >>>>>>>>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> WICKET-4484 do not change tag id when resolving component >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>>>>>>>> Commit: >>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>>>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>>>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>>>>>>>> >>>>>>>>>>> Branch: refs/heads/wicket-1.5.x >>>>>>>>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>>>>>>>> Parents: c6c45a5 >>>>>>>>>>> Author: Sven Meier<[hidden email]> >>>>>>>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>>>> Committer: Sven Meier<[hidden email]> >>>>>>>>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>>>>>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>> diff --git >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> index 689133e..8779b6f 100644 >>>>>>>>>>> --- >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> +++ >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>>>>>>>> AbstractMarkupFilter implements ICompo >>>>>>>>>>> if (wtag.isLinkTag()&& >>>>>>>>>>> (wtag.getNamespace() >>>>>>>>>>> != >>>>>>>>>>> null)) >>>>>>>>>>> { >>>>>>>>>>> String id = tag.getId() + "-" + >>>>>>>>>>> container.getPage().getAutoIndex(); >>>>>>>>>>> - tag.setId(id); >>>>>>>>>>> >>>>>>>>>>> return new >>>>>>>>>>> TransparentWebMarkupContainer(id); >>>>>>>>>>> } >>>>>>>>>>> >> -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com |
Re: git commit: WICKET-4484 do not change tag id when resolving component
|
|
In reply to this post by Martin Grigorov-4
>I have seen code that does: setMarkupId(getId()). I haven't seen the
>same with componentTag.getId() but maybe someone does this ... There are many ways to shoot yourself in the foot ;). Note that I was talking about the component id in Wicket's cached markup, not the resulting markupId in the output. Perhaps Juergen misunderstood me too? IMHO it would be great to have an example which demonstrates the problem Juergen has pointed to. I still don't get why there's a problem. Sven On 03/30/2012 04:46 PM, Martin Grigorov wrote: > On Fri, Mar 30, 2012 at 4:33 PM, Sven Meier<[hidden email]> wrote: >> Javascript ids are session unique anyway - just prefixed with the component >> id in non-deployment config. > I didn't get this. > Juergen says that now there could be a ComponentTag with unique id in > its markup container (e.g. a Panel), but could clash with the id of > another ComponentTag in another markup container in the same page. > Sometimes the ComponentTag's id is directly set as Component's > markupId (this is what you meant with Javascript id, right?). In such > case the user application can have problems. > > I have seen code that does: setMarkupId(getId()). I haven't seen the > same with componentTag.getId() but maybe someone does this ... > >> Sven >> >> >> On 03/30/2012 04:11 PM, Juergen Donnerstag wrote: >>> On Fri, Mar 30, 2012 at 3:57 PM, Sven Meier<[hidden email]> wrote: >>>> Hi Juergen and Jesse, >>>> >>>> I've debugged WicketLinkTagHandler and everything worked as I have >>>> expected >>>> it: >>>> >>>> The WicketTag is always the same instance, coming from the cached markup. >>>> Changing its id doesn't make sense to me: This is global state which is >>>> accessed for *each* rendering of the container. >>>> >>>> >>>>> An id which is unique with markup file is not guranteed to be unique in >>>>> the >>>>> page markup. >>>>> That's why resolvers are using the page.autoIndex to create a page >>>>> unique >>>>> id >>>> Yes, the id has to be unique for components inside their container. That >>>> hasn't changed with the patch. >>>> >>> From a component resolution point of view you are right, but I think >>> you are ignoring Javascript here. wicket-ids may become become ids and >>> they need to be page unique, correct? >>> >>> Juergen >>> >>>> Back to debugging >>>> Sven >>>> >>>> >>>> >>>> On 03/30/2012 12:40 PM, Juergen Donnerstag wrote: >>>>> On Fri, Mar 30, 2012 at 3:01 AM, Jesse Long<[hidden email]> >>>>> wrote: >>>>>> Hi Juergen, Sven, >>>>>> >>>>>> We are not doing away with adding the "auto index" unique number to the >>>>>> resultant component id, we are only doing away with modifying the id in >>>>>> the >>>>>> cached markup tag. >>>>> The patched changed the resolver and not the filter. Hence the markup >>>>> loaded into the cache is still the same, but the id presented at >>>>> render time is now a different. >>>>> >>>>>> The uniqueness of the id of the component returned by the resolve() >>>>>> method >>>>>> is still being guaranteed (as far as I understand) because it takes the >>>>>> original id from the tag and append the current page's next auto index >>>>>> number. The component (TransparentWebMarkupContainer) is then created >>>>>> with >>>>>> this modified id. >>>>> sorry but this is also not correct. The markup of a page is composed >>>>> of potentially many markup files (fragments, panels, inheritance, >>>>> etc.). An id which is unique with markup file is not guranteed to be >>>>> unique in the page markup. That's why resolvers are using the >>>>> page.autoIndex to create a page unique id >>>>> >>>>>> All I'm asking in this patch is to not modify the original id stored in >>>>>> the >>>>>> markup tag, because its not necessary. (AFAIK). >>>>>> >>>>>> Anyway, the resultant component id is now (after the patch) as unique >>>>>> as >>>>>> it >>>>>> is on the first render without the patch... >>>>> sorry, but this is not true. >>>>> >>>>>> This problem is actually very serious. I have a base page which 90% of >>>>>> my >>>>>> pages extend. Without the patch, on every single render, the id stored >>>>>> in >>>>>> the tag has a few bytes appended, causing the id of the resultant >>>>>> component >>>>>> to be a few bytes longer (for every render of every page in any >>>>>> session). >>>>>> I'm not sure if these ids get stored in memory after the render (I dont >>>>>> know >>>>>> wicket internals well enough to tell) or stored in the page store. If >>>>>> they >>>>>> do, its a big memory leak. >>>>>> >>>>>> At the very least its spews out huge amounts of data in the debug >>>>>> logging. >>>>> I very well understand your pain point, but lets fix the problem >>>>> properly and not introduce a bug with an enhancement. >>>>> >>>>> Juergen >>>>> >>>>>> Cheers, >>>>>> Jesse >>>>>> >>>>>> >>>>>> >>>>>> On 29/03/2012 22:39, Sven Meier wrote: >>>>>>> Hi, >>>>>>> >>>>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>>> all auto labels' tags have the same id already (its class name) so no >>>>>>> need >>>>>>> to create some artificial markup. >>>>>>> >>>>>>>> To me the applied patch leaves us with a bigger problem than it >>>>>>>> fixes. >>>>>>> If the id of the tag in the cached markup is altered on each render, >>>>>>> the >>>>>>> id can get quite long pretty fast. As the reporter of the issues has >>>>>>> stated, >>>>>>> this is a 'big' problem ;). >>>>>>> >>>>>>>> we should fix it properly >>>>>>> Agreed. Obviously I'm missing a key concept here. >>>>>>> I'll try to debug this issue tomorrow to understand the problem you've >>>>>>> identified. >>>>>>> >>>>>>> Sven >>>>>>> >>>>>>> On 03/29/2012 09:44 PM, Juergen Donnerstag wrote: >>>>>>>> Hi Sven >>>>>>>> >>>>>>>>> I double checked with how other filters/resolvers are doing it and I >>>>>>>>> found >>>>>>>>> AutoLabelTextResolver to leave the tag id untouched too. >>>>>>>> which is a fault as well. Same risk. >>>>>>>> >>>>>>>>>> May be a Component's flag bit can be used for that purpose. >>>>>>>>> Shouldn't that be a flag on the markup tag instead then? >>>>>>>> No, because the Pages are made of potentially many markups. Since the >>>>>>>> ID has no info about hierarchy, the number appended is the only thing >>>>>>>> that makes it page unique. Hence it can not be per markup. Since >>>>>>>> every >>>>>>>> tag gets associated with a Component, the Component is the right >>>>>>>> place >>>>>>>> for the flag. >>>>>>>> >>>>>>>>>> I don't know how likely it is and I know we have no tests for it. >>>>>>>>> Yeah, I didn't find one either. I'm not sure I understand where's a >>>>>>>>> possible >>>>>>>>> problem here. It's hard to discuss this issues without an actual >>>>>>>>> case. >>>>>>>> An artifical markup which creates duplicate IDs can be constructed. >>>>>>>> I've not tried it but I don't expect it to be too difficult. Knowing >>>>>>>> there is a potential risk which will be very hard to find by a normal >>>>>>>> user, makes me belief we should fix it properly, To me the applied >>>>>>>> patch leaves us with a bigger problem than it fixes. >>>>>>>> >>>>>>>> Juergen >>>>>>>> >>>>>>>>> Sven >>>>>>>>> >>>>>>>>> >>>>>>>>> On 03/29/2012 08:42 PM, Juergen Donnerstag wrote: >>>>>>>>>> Hi Sven, >>>>>>>>>> >>>>>>>>>> the change introduces a theoretical risk of duplicate ids. I don't >>>>>>>>>> know how likely it is and I know we have no tests for it. But >>>>>>>>>> Filters >>>>>>>>>> are created per markup file and modify the markup upon loading, >>>>>>>>>> prior >>>>>>>>>> to caching. Which also means that the Page is not available. In >>>>>>>>>> case >>>>>>>>>> of inheritance it might not even be the same Page. To be absolutely >>>>>>>>>> sure the ID is page-unique, it gets update in the Resolvers where >>>>>>>>>> the >>>>>>>>>> Page instance is available. >>>>>>>>>> >>>>>>>>>> The root cause is that we don't validate if the ID has been updated >>>>>>>>>> already and avoid subsequent updates. We have no means to do that >>>>>>>>>> check. A number at the end of the ID would not be sufficient. May >>>>>>>>>> be >>>>>>>>>> a >>>>>>>>>> Component's flag bit can be used for that purpose. >>>>>>>>>> >>>>>>>>>> I understand the problem in your application, but users will have >>>>>>>>>> an >>>>>>>>>> extremly hard time to identify the issue if it occurs. I'm not sure >>>>>>>>>> it's worth it. May be the approach outlined above solved your >>>>>>>>>> problem >>>>>>>>>> as well, but in a safe manner. >>>>>>>>>> >>>>>>>>>> Juergen >>>>>>>>>> >>>>>>>>>> On Thu, Mar 29, 2012 at 6:43 PM,<[hidden email]> >>>>>>>>>> wrote: >>>>>>>>>>> Updated Branches: >>>>>>>>>>> refs/heads/wicket-1.5.x c6c45a510 -> d7e0d8845 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> WICKET-4484 do not change tag id when resolving component >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>>>>>>>>>> Commit: >>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/commit/d7e0d884 >>>>>>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/d7e0d884 >>>>>>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/d7e0d884 >>>>>>>>>>> >>>>>>>>>>> Branch: refs/heads/wicket-1.5.x >>>>>>>>>>> Commit: d7e0d8845dd509110e48ba965f0e11a3aae6a1f5 >>>>>>>>>>> Parents: c6c45a5 >>>>>>>>>>> Author: Sven Meier<[hidden email]> >>>>>>>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>>>> Committer: Sven Meier<[hidden email]> >>>>>>>>>>> Committed: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>> .../markup/parser/filter/WicketLinkTagHandler.java | 1 - >>>>>>>>>>> 1 files changed, 0 insertions(+), 1 deletions(-) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> http://git-wip-us.apache.org/repos/asf/wicket/blob/d7e0d884/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ---------------------------------------------------------------------- >>>>>>>>>>> diff --git >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> index 689133e..8779b6f 100644 >>>>>>>>>>> --- >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> a/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> +++ >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> b/wicket-core/src/main/java/org/apache/wicket/markup/parser/filter/WicketLinkTagHandler.java >>>>>>>>>>> @@ -208,7 +208,6 @@ public class WicketLinkTagHandler extends >>>>>>>>>>> AbstractMarkupFilter implements ICompo >>>>>>>>>>> if (wtag.isLinkTag()&& >>>>>>>>>>> (wtag.getNamespace() >>>>>>>>>>> != >>>>>>>>>>> null)) >>>>>>>>>>> { >>>>>>>>>>> String id = tag.getId() + "-" + >>>>>>>>>>> container.getPage().getAutoIndex(); >>>>>>>>>>> - tag.setId(id); >>>>>>>>>>> >>>>>>>>>>> return new >>>>>>>>>>> TransparentWebMarkupContainer(id); >>>>>>>>>>> } >>>>>>>>>>> > > |
«
Return to Forum for Wicket Core developers
|
1 view|%1 views
Loading...
| Powered by Nabble | Edit this page |