Modify

Opened 4 years ago

Last modified 11 months ago

#7856 reopened enhancement

[patch] Temporary workaround for regression in Tag query funcionality

Reported by: hasienda Owned by: rjollos
Priority: normal Component: KeywordSuggestPlugin
Severity: major Keywords: regression
Cc: rjollos Trac Release: 0.11

Description

I've upgraded TagsPlugin to latest trunk version with following devastating effect:

  • NotImplementedError on any ticket page - full DoS
  • NotImplementedError on any wiki page, as soon as I try to edit - partial DoS

Analysis the provided Traceback led quite straight-forward to disabling the KeywordSuggestPlugin installed there as well. Effect: Full recovery minus keyword suggest functionality, naturally.

This seems to be a regression in Tag query functionality, but as our users really like/need this for everyday work to prevent tag bloat by misspelling/casing-only differences/etc. I've made a quick hack to restore it.

Publishing this is mainly for reference to serve a temporary solution. To fix this inside the TagsPlugin will be the preferred way, but much harder to be done, as the query module there is a different piece of cake, or I've just not tried hard enough.

Attachments (2)

keywordsuggest_workaround-for-tags-regression.patch (515 bytes) - added by hasienda 4 years ago.
quick fix for utilizing Tag query in an insane way, that just works for now
20131116_kwsuggest_performance-tweak.patch (845 bytes) - added by hasienda 11 months ago.
replace query work-around by new, performance-conscious class method

Download all attachments as: .zip

Change History (14)

Changed 4 years ago by hasienda

quick fix for utilizing Tag query in an insane way, that just works for now

comment:1 Changed 4 years ago by hasienda

  • Summary changed from Temporary workaround for regression in Tag query funcionality to [patch] Temporary workaround for regression in Tag query funcionality
  • Type changed from defect to enhancement

This issue has been reported as #7857 for TagsPlugin to get it fixed there.

From the perspective of KeywordSuggestPlugin(s maintainer) this might even be seen just as an enhancement request.? Anyway, we should have a solution here while KeywordSuggestPlugin remainings incompatible with TagsPlugin breaking the nearly the whole Trac application.

comment:2 Changed 3 years ago by rjollos

  • Owner changed from scratcher to rjollos
  • Status changed from new to assigned

comment:3 Changed 3 years ago by rjollos

I'll push this to the trunk along with the patch in #4201.

Sorry this ticket sat here for so long!

comment:4 Changed 3 years ago by rjollos

While working on applying the patch in #4201 to the trunk, and working with the TagsPlugin trunk @ r10800, prior to applying your patch I was seeing:

File "/home/rjollos/Workspace/th4201/trac-0.11-stable/trac/web/main.py", line 450, in _dispatch_request
  dispatcher.dispatch(req)
File "/home/rjollos/Workspace/th4201/trac-0.11-stable/trac/web/main.py", line 227, in dispatch
  data, content_type)
File "/home/rjollos/Workspace/th4201/trac-0.11-stable/trac/web/chrome.py", line 745, in render_template
  stream |= self._filter_stream(req, method, filename, stream, data)
File "/home/rjollos/Workspace/th4201/genshi-trunk/genshi/core.py", line 132, in __or__
  return Stream(_ensure(function(self)), serializer=self.serializer)
File "/home/rjollos/Workspace/th4201/trac-0.11-stable/trac/web/chrome.py", line 848, in inner
  data)
File "/home/rjollos/Workspace/keywordsuggestplugin/0.11/keywordsuggest/keywordsuggest.py", line 45, in filter_stream
  for resource, tags in query_result:
File "/home/rjollos/Workspace/tagsplugin/trunk/tractags/api.py", line 250, in query
  for m in self._realm.finditer(query.as_string()):
File "/home/rjollos/Workspace/tagsplugin/trunk/tractags/query.py", line 360, in as_string
  return _convert(self)
File "/home/rjollos/Workspace/tagsplugin/trunk/tractags/query.py", line 359, in _convert
  raise NotImplementedError

comment:5 Changed 3 years ago by hasienda

Seem, like I've lost track of this ticket.

I think there is a real solution on the way to fix this without my ugly work-around. But do it anyway, as it can easily get reverted when applying a more elaborated fix of the TagsQuery code, as I'm working on #7857.

comment:6 Changed 3 years ago by rjollos

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [10935]) Refs #4201, Fixes #7856: Version 0.4: Added support for TagsPlugin. Tags from the TagsPlugin will be used whenever the TagsPlugin is installed. In the future, an option might be added to determine whether or not to use the TagsPlugin when it is installed. The changes here were derived from the source as modified by georgesoon, keywordsuggest.py in #4201.

These changes have been lightly tested, so please open a new ticket if you encounter any issues.

Compatibility note: Previously, the separator option was defined in quotes, e.g. ', '. Now, the separator is just defined as a character, without quotes. A space is added to all separators.

comment:7 Changed 2 years ago by hasienda

(In [12392]) TagsPlugin: Make query without arguments a valid query expression, refs #7856 and #7857.

Fixes NotImplementedError meaning a regression, because it has been parsed
gracefully before, even got used as valid 'all tags' expression in former
plugin versions, and i.e. KeywordSuggestPlugin even relied on this behavior.

Thanks to Itamar Ostricher for contributing the key changes of this changeset.

Some code and template clean-up is done as well.

comment:8 Changed 2 years ago by hasienda

So the hot-fix has been made obsolete 1 year later. Not a short fix, but at least done now. Thanks for your patience.

comment:9 Changed 11 months ago by hasienda

TagsPlugin now has better way to get queried for existing tags than using TagSystem.query()

I'll attach a patch in a minute to propose changes for this plugin, that replace this work-around for recent versions of TagsPlugin and are recommended especially for performance anyway. See #4503 for details, please.

comment:10 follow-up: Changed 11 months ago by hasienda

  • Resolution fixed deleted
  • Status changed from closed to reopened

Please consider the new proposed fix for giving decent performance (~ 45 s less per 5k tickets) on bigger Trac applications.

comment:11 Changed 11 months ago by hasienda

I re-opened for keeping focus on the subject, but will create a new ticket, if you prefer that.

Changed 11 months ago by hasienda

replace query work-around by new, performance-conscious class method

comment:12 in reply to: ↑ 10 Changed 11 months ago by rjollos

Replying to hasienda:

Please consider the new proposed fix for giving decent performance (~ 45 s less per 5k tickets) on bigger Trac applications.

Thanks, please commit it.

Is it correct to say that this will work in TagsPlugin 0.6 and 0.7, (I see athomas added the get_all_tags method way back in [3882]), but the performance will be significantly improved in recent revisions leading up to 0.7?

Add Comment

Modify Ticket

Action
as reopened .
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.