Ticket #7857 (new defect)

Opened 3 years ago

Last modified 6 months ago

[PATCH] Regression in Tag query functionality

Reported by: hasienda Assigned to: hasienda
Priority: high Component: TagsPlugin
Severity: normal Keywords: regression
Cc: rjollos, itamarost Trac Release: 0.11

Description

Empty query as in 

[[ListTagged()]]

raises NotImplementedError

This has been most relevant in some applications, that I upgraded today from v0.6 to recent trunk with devastating effect on the whole environments. In short, I had to temporarily disable KeywordSuggestPlugin installed there as well, and only later found a quick but ugly hack, to get it working again (see #7856).

It looks like KeywordSuggestPlugin has been built on a default query behavior to return all known tags, that got lost in later versions of TagsPlugin. So it should be fixed here, or I totally mistake the situation.

Attachments

tagsplugin-7857-itamaro-v1.patch (0.5 kB) - added by itamarost on 09/29/11 10:12:39.
patch against tagsplugin trunk
ListTaggedEmptyCall-r10708.png (7.0 kB) - added by rjollos on 09/30/11 02:19:15.
tagsplugin-7857-itamaro-v2.patch (1.7 kB) - added by itamarost on 10/01/11 14:27:44.
some more tweaks…

Change History

(follow-up: ↓ 2 ) 11/20/10 09:12:28 changed by shentonfreude

I was bitten by this as well, also using the KeywordSuggestPlugin with the autocomplete patch. I ended up backing out Tags to the 0.6 version.

I'm using Trac-0.12.

(in reply to: ↑ 1 ) 11/20/10 13:29:00 changed by hasienda

  • owner changed from athomas to otaku42.

Replying to shentonfreude:

I was bitten by this as well, also using the KeywordSuggestPlugin with the autocomplete patch. [...]

Thanks for the confirmation.

However, knowing the issue is there is just the first step. Did you investigate the offending change(s) in TagsPlugin by now? I'd love to help fix this, but have not enough time yet to research it on my own.

Don't hesitate to propose a patch against trunk. I'm well able to review and commit, if suitable.

09/29/11 10:12:39 changed by itamarost

  • attachment tagsplugin-7857-itamaro-v1.patch added.

patch against tagsplugin trunk

09/29/11 10:14:09 changed by itamarost

  • cc changed from rjollos to rjollos, itamarost.
  • summary changed from Regression in Tag query funcionality to [PATCH] Regression in Tag query funcionality.

I don't fully understand the underlying issue, but the attached patch fixed this for me.

09/30/11 02:19:15 changed by rjollos

  • attachment ListTaggedEmptyCall-r10708.png added.

(follow-up: ↓ 5 ) 09/30/11 02:21:11 changed by rjollos

On Trac 0.11.7, with the latest version of the TagsPlugin trunk (r10708), [[ListTagged()]] gives me:

The traceback is: 

2011-09-29 19:17:30,662 Trac[formatter] ERROR: Macro ListTagged() failed:
Traceback (most recent call last):
  File "/usr/lib/python2.6/site-packages/Trac-0.11.7-py2.6.egg/trac/wiki/formatter.py", line 484, in _macro_formatter
    return macro.process(args, in_paragraph=True)
  File "/usr/lib/python2.6/site-packages/Trac-0.11.7-py2.6.egg/trac/wiki/formatter.py", line 180, in process
    text = self.processor(text)
  File "/usr/lib/python2.6/site-packages/Trac-0.11.7-py2.6.egg/trac/wiki/formatter.py", line 167, in _macro_processor
    text)
  File "/usr/lib/python2.6/site-packages/TracTags-0.7dev_r10708-py2.6.egg/tractags/macros.py", line 173, in expand_macro
    if len(realms) == 0:
TypeError: object of type 'NoneType' has no len()

(in reply to: ↑ 4 ; follow-up: ↓ 6 ) 09/30/11 18:32:12 changed by hasienda

Replying to rjollos:

On Trac 0.11.7, with the latest version of the TagsPlugin trunk (r10708), [[ListTagged()]] gives me: 

...
TypeError: object of type 'NoneType' has no len()

This latest observation is with source patched with Itamar's patch, or not? I've not seen this error, just the old one. But I'll try to fix this parser issue before tractags-0.7 for sure.

(in reply to: ↑ 5 ) 09/30/11 20:10:37 changed by rjollos

Replying to hasienda:

This latest observation is with source patched with Itamar's patch, or not?

Observation is with a clean version of r10708; no patch.

09/30/11 22:20:33 changed by osimons

Make a unit test, perhaps?

10/01/11 14:27:44 changed by itamarost

  • attachment tagsplugin-7857-itamaro-v2.patch added.

some more tweaks...

10/01/11 14:40:45 changed by itamarost

Since updating to r10708, on 0.12.3dev-r10799, with my previous patch, I ran into some more issues (including the one mentioned by rjollos above), and the v2 of the patch helps a little, although it seems there are still deeper issues (indeed - some unit tests are appropriate).

As it is now, with the v2 patch, I don't get errors, but there's something weird with the Query object. I used the listtagged_exclude_realms = ticket option and created several tagged wiki pages and tickets (lets say the tag is bla for all).

  • If I call [[ListTagged(bla)]] I get the expected result - a list of only wiki pages with that tag.
  • If I call [[ListTagged()]] I get the unexpected result of wiki AND tickets.
  • If I specify a realm explicitly but with no tags, e.g. [[ListTagged(realm:wiki)]] or [[ListTagged(realm:ticket)]] I do get the expected result.

Trying to debug it, I noticed that in all cases, in exapnd_macro() the query looks OK when it gets to query_result = tag_system.query(req, query):

  • [[ListTagged(bla)]] --> query = '(bla) (realm:wiki)'
  • [[ListTagged()]] --> query = '() (realm:wiki)'

But the Query object gets messed up somehow in the process...

10/01/11 16:06:39 changed by osimons

(In [10714]) TagsPlugin: Fix TypeError: object of type 'NoneType' has no len() for [[ListTagged()]] with test. References #7857.

10/01/11 16:10:30 changed by osimons

Just wanted to get the macro tests going, and fixing the error message seemed like a good start. As I don't actually use the plugin, I'll leave the other issues and tests for others...

10/25/12 05:09:42 changed by rjollos

  • summary changed from [PATCH] Regression in Tag query funcionality to [PATCH] Regression in Tag query functionality.

11/25/12 18:35:20 changed by hasienda

  • priority changed from normal to high.
  • owner changed from otaku42 to hasienda.

As far as I traced it down, this should probably be fixed inside the query parser, but I've been unable to finally fix it for many months now.

Confirming that the fix will become even more important to me within the next release cycle. I would certainly consider a maintenance release after getting it done. Sorry for the delay.

11/27/12 23:16:28 changed by hasienda

(In [12391]) TagsPlugin: Make realm configuration for ListTagged more effective, refs #7857 and #9063.

Settings for 'listtagged_exclude_realms' had no effect without a query. Itamar Ostricher discovered this issue in the initial implementation of the feature and suggested this change. Thanks for taking care.

Additionally the broken 'test_empty_content' test in tractags.tests.macros.ListTaggedMacroTestCase is fixed, and a regression test for the pending 'empty query' issue (#7857) has been introduced as well.

11/27/12 23:23:00 changed 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.

(follow-up: ↓ 16 ) 11/27/12 23:26:41 changed by hasienda

All 34 tests pass for all supported configurations including Trac versions ranging from 0.11 to 1.0, tested with the PostgreSQL db backend as well - what a relief.

(in reply to: ↑ 15 ) 11/30/12 15:39:29 changed by itamarost

Replying to hasienda:

All 34 tests pass for all supported configurations including Trac versions ranging from 0.11 to 1.0, tested with the PostgreSQL db backend as well - what a relief.

That's awesome! I'm glad you were able to get this into the plugin :-)

Hope I'll find an opportunity to test this in my environments, though I don't see one in the near future...

Add/Change #7857 ([PATCH] Regression in Tag query functionality)




Change Properties
Action