Edgewall Software

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#11545 closed defect (fixed)

Search returns milestone resource when only one word matches

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: search system Version: 1.0-stable
Severity: normal Keywords: roadmap
Cc: Branch:
Release Notes:

Fixed milestone search returning results when only one word from phrase matched.

API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos)

For a long time I've been noticing that the milestones would almost always appear in my search results, even for cases that I wouldn't expect them to appear. It looks like a milestone will be returned in the search results when there is a match from only one word of a multiword phrase.

The following test was done to prove the case:

  1. Generated 3 words at http://listofrandomwords.com: Semiphilosophic opportunistic sulphur
  2. Created 4 tickets: one ticket with the phrase as the summary and three tickets containing one word of the phrase as the summary.
  3. Created 4 tickets: one ticket with the phrase as the description and three tickets containing one word of the phrase as the description.
  4. Created 4 milestones: one milestone with the phrase in the description and three milestones containing one word of the phrase in the description.
  5. Created 4 wiki pages: one wiki page with the phrase in the content and three wiki pages containing one word of the phrase in the content.

Here are the search results:

Attachments (1)

SearchResults.png (46.0 KB ) - added by Ryan J Ollos 8 years ago.

Download all attachments as: .zip

Change History (13)

by Ryan J Ollos, 8 years ago

Attachment: SearchResults.png added

comment:1 by Peter Suter, 8 years ago

I think it was changed in [11525] and could be fixed by changing any() to all(). (untested)

comment:2 by Ryan J Ollos, 8 years ago

Description: modified (diff)

comment:3 by Christian Boos, 8 years ago

Good catch! Right, all() seems appropriate to reproduce the behavior of the former SQL query in trunk/trac/search/api.py@11528#L54.

comment:4 by Ryan J Ollos, 8 years ago

Milestone: next-stable-1.0.x1.0.2
Owner: set to Ryan J Ollos
Status: newassigned

Thanks for providing a fix. I've prepared the change along with a unit test in log:rjollos.git:t11545.

comment:5 by Ryan J Ollos, 8 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to 1.0-stable in [12593], merged to trunk in [12595].

comment:6 by Jun Omae, 8 years ago

Resolution: fixed
Status: closedreopened

After [12593], I got the following.

ERROR: test_get_search_results_milestone_not_in_filters (trac.ticket.tests.roadmap.MilestoneModuleTestCase)
Traceback (most recent call last):
  File "/home/jun66j5/src/trac/edgewall/svn/branches/1.0-stable/trac/ticket/tests/roadmap.py", line 155, in setUp
  File "/home/jun66j5/src/trac/edgewall/svn/branches/1.0-stable/trac/ticket/model.py", line 1042, in insert
    to_utimestamp(self.completed), self.description))
  File "/home/jun66j5/src/trac/edgewall/svn/branches/1.0-stable/trac/db/util.py", line 121, in execute
    cursor.execute(query, params)
  File "/home/jun66j5/src/trac/edgewall/svn/branches/1.0-stable/trac/db/util.py", line 54, in execute
    r = self.cursor.execute(sql_escape_percent(sql), args)
  File "/home/jun66j5/src/trac/edgewall/svn/branches/1.0-stable/trac/db/sqlite_backend.py", line 78, in execute
    result = PyFormatCursor.execute(self, *args)
  File "/home/jun66j5/src/trac/edgewall/svn/branches/1.0-stable/trac/db/sqlite_backend.py", line 56, in execute
    args or [])
  File "/home/jun66j5/src/trac/edgewall/svn/branches/1.0-stable/trac/db/sqlite_backend.py", line 48, in _rollback_on_error
    return function(self, *args, **kwargs)
IntegrityError: column name is not unique

FAIL: test_get_search_results_matches_all_terms (trac.ticket.tests.roadmap.MilestoneModuleTestCase)
Traceback (most recent call last):
  File "/home/jun66j5/src/trac/edgewall/svn/branches/1.0-stable/trac/ticket/tests/roadmap.py", line 174, in test_get_search_results_matches_all_terms
    self.assertEqual(1, len(results))
AssertionError: 1 != 2

I've investigated.

  • We must call reset_db() in tearDown() in the case.
  • If /usr/share/dict/words might not be existent, random_word() generates using 10 words at most.
  • trac/ticket/tests/roadmap.py

    diff --git a/trac/ticket/tests/roadmap.py b/trac/ticket/tests/roadmap.py
    index 31dbb7c..d7fab12 100644
    a b class MilestoneModuleTestCase(unittest.TestCase):  
    145145        self.env = EnvironmentStub()
    146146        self.req = Mock(href=self.env.href, perm=MockPerm())
    147147        self.mmodule = MilestoneModule(self.env)
    148         self.terms = [random_word() for i in range(0, 3)]
     148        terms = set()
     149        while len(terms) != 3:
     150            terms.add(random_word())
     151        self.terms = list(terms)
    149152        self.milestones = []
    150153        for term in self.terms + [' '.join(self.terms)]:
    151154            m = Milestone(self.env)
    class MilestoneModuleTestCase(unittest.TestCase):  
    155158            m.insert()
    156159            self.milestones.append(m)
     161    def tearDown(self):
     162        self.env.reset_db()
    158164    def test_get_search_filters(self):
    159165        filters = self.mmodule.get_search_filters(self.req)
    160166        filters = list(filters)

comment:7 by Ryan J Ollos, 8 years ago

Okay, thanks. Maybe we should add random_unique_words(word_count, min_length=1) to trac.tests.contentgen.

comment:8 by Jun Omae, 8 years ago

Also, I noticed that the /usr/share/dict/words file has words which contains other words, e.g. in v.s. winch. That terms are not unique.

It would be simple to be hard-coded rather than random_word().

-        self.terms = [random_word() for i in range(0, 3)]
+        self.terms = ['MilestoneAlpha', 'MilestoneBeta', 'MilestoneGamma']

comment:9 by Ryan J Ollos, 8 years ago

Resolution: fixed
Status: reopenedclosed

Committed to 1.0-stable in [12603] and merged to trunk in [12604].

comment:10 by Ryan J Ollos, 8 years ago

I was confused as to why it didn't seem to be necessary to call reset_db in tearDown, but it appears that is due to the reset_db call in the EnvironmentStub constructor: branches/1.0-stable/trac/test.py@12531:311-312#L292. Is there a reason to also call reset_db in tearDown?

I found the destroying parameter to be a bit confusing. It appears to mean, don't initialize the database because the EnvironmentStub object is being constructed to call destroy_db. At least, that is the use-case in trac.tests.functional.testenv:FunctionalTestEnvironment. Initially I assumed it meant, construct a EnvironmentStub object and destroy existing data in the db. I'll add an entry in the docstring to clarify the behavior.

in reply to:  10 comment:11 by Jun Omae, 8 years ago

Is there a reason to also call reset_db in tearDown?

The failures have gone away on r12603. But I still think we should call the reset_db to keep the database clean.

All test cases should reset database in tearDown() if records are created by setUp() or the test methods of the test case. Also, I don't think that the next test case always calls reset_db() from EnvironmentStub().

comment:12 by Ryan J Ollos, 8 years ago

Refactoring and call to reset_db in tearDown committed in [12607], merged in [12608]. EnvironmentStub docstring improved in [12609], merged in [12610].

Modify Ticket

Change Properties
Set your email in Preferences
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment

E-mail address and name can be saved in the Preferences .
Note: See TracTickets for help on using tickets.