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,<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);
}
