On Fri, Mar 30, 2012 at 3:57 PM, Sven Meier <s...@meiers.net> 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<je...@virtualpostman.co.za> >> 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,<svenme...@apache.org> 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<svenme...@apache.org> >>>>>>>> Authored: Thu Mar 29 18:43:07 2012 +0200 >>>>>>>> Committer: Sven Meier<svenme...@apache.org> >>>>>>>> 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); >>>>>>>> } >>>>>>>> >>>> >
