[Patch] Exclude PageTemplates from tag-extraction

Good catch, just what I noticed recently too.

+1 for fixing this. Suggestions, patch? :-)

patch against tagsplugin trunk

Replying to hasienda:

Good catch, just what I noticed recently too. +1 for fixing this. Suggestions, patch? :-)

You ask so nicely, so I could not resist :-)

Patch attached, maybe somewhat dirty, as I add in api a specific test for the wiki realm, but it works for me :-)

On the way - the patch contains a fix for a possible issue my IDE warned me about (resource not defined in the context of the PermissionError exception).

While your fix for the undefined variable resource seems perfectly valid, I wonder how to alter the suggested change for the main issue.

Intercepting get_tagged_resources seems fine only on first look. But it would mask wiki templates for the admin tag replace feature too, and I seriously doubt that's desirable or one of your explicit intentions.

attempting to fix the tag-replace issue with previous patch

Replying to hasienda:

Intercepting get_tagged_resources seems fine only on first look. But it would mask wiki templates for the admin tag replace feature too, and I seriously doubt that's desirable or one of your explicit intentions.

You're absolutely right. A second version of the patch tries to resolve this (quite a hack I guess, but couldn't think of a better approach).

Very well. That should work indeed.

Reason, why I don't apply this right-away is, that after starting to work on #4503 I've never let performance go out of sight. I guess that not retrieving data is better than filtering it out later, so I tend towards implementing a more sophisticated SQL statement inside get_tagged_resources that could be fed by a new optional filter argument. Just finalizing and testing this right now ...

Great. Good thing you're keeping an eye on performance - not quite my strong side :-)

That said, I'm not completely sure "more sophisticated SQL" to prevent data retrieval would be better performance-wise compared to dead-simple SQL with simple filtering out.

Using more sophisticated SQL just passes on the bottleneck the the SQL library, so it's not explicit.

Also I generally prefer readability and maintainability over performance when the effect is not considerable.

But, as I said - I don't know much about these things. Just my 2c.

(In [11105]) TagsPlugin: Fix undefined resource in PermissionError? exception context, refs #9636.

This has been reported and corrected in KISS-style by Itamar Ostricher.

Just to point a work-around:

1. add another Tag like "PageTemplate" and
2. then exclude it when using [[ListTagged(TagOfInterest1 TagOfInterest2 -PageTemplate)]] macro.

But hold on, if you add a tag to the PageTemplate then it will be propagated to new pages and therefore those pages will be excluded.

Replying to fommil:

But hold on, if you add a tag to the PageTemplate then it will be propagated to new pages and therefore those pages will be excluded.

Indeed. Still I don't see a reason, why we shouldn't implement this in a way, that is usable for Trac-0.11 onwards, not just 1.0, right?

Replying to anonymous:

That said, I'm not completely sure "more sophisticated SQL" to prevent data retrieval would be better performance-wise compared to dead-simple SQL with simple filtering out. Using more sophisticated SQL just passes on the bottleneck the the SQL library, so it's not explicit.

I think I've found a valid answer testing with 30.000 auto-generated tickets. On my development system it took around 100-300 ms to pull the keywords from all tickets, but more than 3 seconds to check access permission on each one. Well, maybe a good share of the time has been constructing the Trac Resource objects for the check, but it remains questionable to me, if there is such a thing like "simple filtering". Only by using rather heavy recursion in the SQL statement I could imagine to hit db performance to the point, where post processing in Python could gain speed.

Also I generally prefer readability and maintainability over performance when the effect is not considerable.

The aforementioned Test environment takes 15 s or longer to render an all-tagged-resources list of these 30.000 tickets. It still takes about 6 s for each tag cloud render pass. Scaling this up to 500.000+ tickets know to sit in some other, real-world Trac applications, there is definitely a big point for performance. I've learned this and will not sleep relaxed before we've gained at least ~ 10 times of the current speed.

(In [12390]) TagsPlugin: Don't use tags on wiki templates for anything else than copying tags, refs #3753 and #9636.

Once again this has been reported by Itamar Ostricher. Thank you very much for nudging me to fix it.

I deduced from digging into source code, tags in wiki pages located under specific "sud-dir" will not be "tagged" anymore. Example:

• from source def get_tagged_resources(req, tags=None, filter=None)
  • setting under [PageTemplates] section in trac.ini file filter = PageTemplates

Am I correct?

Replying to fommil:

But hold on, if you add a tag to the PageTemplate then it will be propagated to new pages and therefore those pages will be excluded.

Yes, it propagates PageTemplates too. But still very handy (in our use case at least ).

Replying to AdrianFritz:

I deduced from digging into source code, tags in wiki pages located under specific "sud-dir" will not be "tagged" anymore. Example: * from source def get_tagged_resources(req, tags=None, filter=None) * setting under [PageTemplates] section in trac.ini file filter = PageTemplates Am I correct?

Yes, tags on any page below PageTemplates (technically correct: sharing common path 'PageTemplates/') are excluded from 'all-realm' or 'all-Trac' tag lists, but a per-resource query still shows them, as does the display of the wiki page view or a corresponding field in wiki page editor.

Replying to fommil:

But hold on, if you add a tag to the PageTemplate then it will be propagated to new pages and therefore those pages will be excluded.

Yes, so I felt urged to implement the better approach: The new (implicit) default configuration keeps tag lists working and in-depended of such meta-tag hacks, that require user action to not exclude new pages from tag lists derived from the respective template as well.