Modify

Opened 14 years ago

Closed 10 years ago

#7857 closed defect (fixed)

[PATCH] Regression in Tag query functionality

Reported by: Steffen Hoffmann Owned by: Steffen Hoffmann
Priority: high Component: TagsPlugin
Severity: normal Keywords: regression
Cc: Ryan J Ollos, Itamar Ostricher 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 (3)

tagsplugin-7857-itamaro-v1.patch (538 bytes) - added by Itamar Ostricher 13 years ago.
patch against tagsplugin trunk
ListTaggedEmptyCall-r10708.png (7.0 KB) - added by Ryan J Ollos 13 years ago.
tagsplugin-7857-itamaro-v2.patch (1.7 KB) - added by Itamar Ostricher 13 years ago.
some more tweaks…

Download all attachments as: .zip

Change History (20)

comment:1 Changed 13 years ago by Chris Shenton

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.

comment:2 in reply to:  1 Changed 13 years ago by Steffen Hoffmann

Owner: changed from Alec Thomas to Michael Renzmann

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.

Changed 13 years ago by Itamar Ostricher

patch against tagsplugin trunk

comment:3 Changed 13 years ago by Itamar Ostricher

Cc: Itamar Ostricher added
Summary: Regression in Tag query funcionality[PATCH] Regression in Tag query funcionality

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

Changed 13 years ago by Ryan J Ollos

comment:4 Changed 13 years ago by Ryan J Ollos

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()

comment:5 in reply to:  4 ; Changed 13 years ago by Steffen Hoffmann

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.

comment:6 in reply to:  5 Changed 13 years ago by Ryan J Ollos

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.

comment:7 Changed 13 years ago by osimons

Make a unit test, perhaps?

Changed 13 years ago by Itamar Ostricher

some more tweaks...

comment:8 Changed 13 years ago by Itamar Ostricher

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...

comment:9 Changed 13 years ago by osimons

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

comment:10 Changed 13 years ago 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...

comment:11 Changed 12 years ago by Ryan J Ollos

Summary: [PATCH] Regression in Tag query funcionality[PATCH] Regression in Tag query functionality

comment:12 Changed 11 years ago by Steffen Hoffmann

Owner: changed from Michael Renzmann to Steffen Hoffmann
Priority: normalhigh

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.

comment:13 Changed 11 years ago by Steffen Hoffmann

(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.

comment:14 Changed 11 years ago by Steffen Hoffmann

(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:15 Changed 11 years ago by Steffen Hoffmann

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.

comment:16 in reply to:  15 Changed 11 years ago by Itamar Ostricher

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...

comment:17 Changed 10 years ago by Steffen Hoffmann

Resolution: fixed
Status: newclosed

In 13815:

TagsPlugin: Completing preparation for v0.7 release.

Availability of that code as stable, tagged release
closes #2429, #3359, #3610, #3624, #3677, #3754, #3864, #3947, #3983, #4078, #4277, #4503, #4799, #5523, #7787, #7857, #8638, #9057, #9058, #9059, #9060, #9061, #9062, #9063, #9149, #9210, #9521, #9630, #9636, #10032, #10416, #10636, #11096, #11147, #11152, #11274, #11302, #11658 and #11659.

Additionally there are some issues and enhancement requests showing progress,
but known to require more work to resolve them satisfactorily, specifically
refs #2804, #4200, #8747 and #9064.

Thanks to all contributors and followers, that enabled and encouraged a good
portion of this development work.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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

 
Note: See TracTickets for help on using tickets.