Merge lp:~adeuring/launchpad/bug-1020443 into lp:launchpad

Proposed by Abel Deuring
Status: Merged
Approved by: j.c.sackett
Approved revision: 15588
Merge reported by: Abel Deuring
Merged at revision: not available
Proposed branch: lp:~adeuring/launchpad/bug-1020443
Merge into: lp:launchpad
Diff against target: 328 lines (+191/-30)
2 files modified
database/schema/patch-2209-24-3.sql (+124/-0)
lib/lp/services/database/doc/textsearching.txt (+67/-30)
To merge this branch: bzr merge lp:~adeuring/launchpad/bug-1020443
Reviewer Review Type Date Requested Status
j.c.sackett (community) Approve
Review via email: mp+115721@code.launchpad.net

Commit message

Ignore the symbols "&|!" in full text searches; don't treat a leading '-' as the "NOT" operator.

Description of the change

This branch fixes bug 1020443: Search terms consisting of a tsquery operator
surrounded by punctuation can lead to OOPSes.

This bug is fallout from my previous work on bug 29713,
lp:~adeuring/launchpad/bug-29713-db-dev .

Background: We have a DB procedure ftq() which processes full text
query strings so that they can be passed to the Postgres procedure
to_tsquery() which in turn generates a query object that can be used
in SQL expressions like

    SELECT ... FROM bugtaskflat WHERE bugtaskflat.fti @@ query_object;

to_tsquery() supports more complex expressions, including grouping
terms with parentheses and the logical operators '&', '|' and '!',
with a simiar meaning as in C or Python.

It is obviously easy to provide syntactically incorrect search
expressions, so ftq() tries hard to fix possible errors -- but I broke
the proper treatment of "badly placed" logical operators in the branch
mentioned above.

The example query string from bug 1020443, "?!.", shows that users do
not always expect that "&", "|", "!" are treated in any special way,
so I asked in https://lists.launchpad.net/launchpad-dev/msg09536.html
if we should continue to treat "&|!" in full text searches as operators
or not. The email contains two more examples where typical source code
text pasted as a full text query leads to surprising results, due to
the interpretation of "&|!" as logical operators.

I got one "vote" from Curtis that ignoring "&|!" in search queries
makes sense and none votes againt doing this; since I think too that
using "&|!" as operators causes more confusion that being useful,
I removed this option.

The technical side: to_tsquery() interpret exactly the characters
"&|!" as logical operators; if they apper in text that is indexed,
they are treated like spaces. SO ftq() now replaces them too with
spaces.

To increase "search consistency" a bit more, I also removed the
"feature" that a '-' preceding a word is interpreted as a the "NOT"
operator because it makes it impossible to search for negative
numbers (which are stored including the '-' in the full text index).

Since ftq() is a stored procedure, I added the usual DB patch file.
The new file patch-2209-24-2.sql is an only slightly modified variant
of patch-2209-24-1.sql. The diff betwwen the two files:

+++ database/schema/patch-2209-24-2.sql 2012-07-19 11:41:02.170952338 +0200
@@ -17,11 +17,13 @@
         query = args[0].decode('utf8')
         ## plpy.debug('1 query is %s' % repr(query))

+ # Replace tsquery operators with ' '.
+ query = re.sub('[|&!]', ' ', query)
+
         # Normalize whitespace
         query = re.sub("(?u)\s+"," ", query)

- # Convert AND, OR, NOT and - to tsearch2 punctuation
- query = re.sub(r"(?u)(?:^|\s)-([\w\(])", r" !\1", query)
+ # Convert AND, OR, NOT to tsearch2 punctuation
         query = re.sub(r"(?u)\bAND\b", "&", query)
         query = re.sub(r"(?u)\bOR\b", "|", query)
         query = re.sub(r"(?u)\bNOT\b", " !", query)
@@ -34,9 +36,6 @@
         query = re.sub(r"(?u)%s+" % (punctuation,), " ", query)
         ## plpy.debug('3 query is %s' % repr(query))

- # Strip ! characters inside and at the end of a word
- query = re.sub(r"(?u)(?<=\w)[\!]+", " ", query)
-
         # Now that we have handle case sensitive booleans, convert to lowercase
         query = query.lower()

@@ -122,4 +121,4 @@
         return query or None
         $_$;

-INSERT INTO LaunchpadDatabaseRevision VALUES (2209, 24, 1);
+INSERT INTO LaunchpadDatabaseRevision VALUES (2209, 24, 2);

The part "@@ -34,9 +36,6 @@" removes a substitution that is not
longer needed since any "!" symbols are already removed from the
query string.

Since this a "hot patch"
(https://dev.launchpad.net/PolicyAndProcess/DatabaseSchemaChangesProcess)
-- just the change of a procedure butno schema changes, trigger change,
index change etc --, this MP uses the devel branch as the target.

test: ./bin/test services.database -vvt textsearching.txt

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  database/schema/patch-2209-24-2.sql
  lib/lp/services/database/doc/textsearching.txt

./lib/lp/services/database/doc/textsearching.txt
     734: want exceeds 78 characters.
make: *** [lint] Fehler 1

That's not caused by my changes -- and hard to fix...

To post a comment you must log in.
Revision history for this message
j.c.sackett (jcsackett) wrote :

This looks good, thanks.

review: Approve
Revision history for this message
j.c.sackett (jcsackett) wrote :

Erhm, spoke to soon--I didn't see that there appears to be a conflict in need of resolution.

review: Needs Fixing
lp:~adeuring/launchpad/bug-1020443 updated
15588. By Abel Deuring

DB patch file renamed.

Revision history for this message
j.c.sackett (jcsackett) wrote :

Looks good now, thanks. Remember to update the INSERT INTO LaunchpadDatabaseRevision line so the numbers match with the patch number file name.

review: Approve
lp:~adeuring/launchpad/bug-1020443 updated
15589. By Abel Deuring

DB Patch number adjusted.

15590. By Abel Deuring

devel merged

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'database/schema/patch-2209-24-3.sql'
2--- database/schema/patch-2209-24-3.sql 1970-01-01 00:00:00 +0000
3+++ database/schema/patch-2209-24-3.sql 2012-07-26 11:30:38 +0000
4@@ -0,0 +1,124 @@
5+-- Copyright 2012 Canonical Ltd. This software is licensed under the
6+-- GNU Affero General Public License version 3 (see the file LICENSE).
7+
8+SET client_min_messages=ERROR;
9+
10+CREATE OR REPLACE FUNCTION _ftq(text) RETURNS text
11+ LANGUAGE plpythonu IMMUTABLE STRICT
12+ AS $_$
13+ import re
14+
15+ # I think this method would be more robust if we used a real
16+ # tokenizer and parser to generate the query string, but we need
17+ # something suitable for use as a stored procedure which currently
18+ # means no external dependancies.
19+
20+ # Convert to Unicode
21+ query = args[0].decode('utf8')
22+ ## plpy.debug('1 query is %s' % repr(query))
23+
24+ # Replace tsquery operators with ' '.
25+ query = re.sub('[|&!]', ' ', query)
26+
27+ # Normalize whitespace
28+ query = re.sub("(?u)\s+"," ", query)
29+
30+ # Convert AND, OR, NOT to tsearch2 punctuation
31+ query = re.sub(r"(?u)\bAND\b", "&", query)
32+ query = re.sub(r"(?u)\bOR\b", "|", query)
33+ query = re.sub(r"(?u)\bNOT\b", " !", query)
34+ ## plpy.debug('2 query is %s' % repr(query))
35+
36+ # Deal with unwanted punctuation.
37+ # ':' is used in queries to specify a weight of a word.
38+ # '\' is treated differently in to_tsvector() and to_tsquery().
39+ punctuation = r'[:\\]'
40+ query = re.sub(r"(?u)%s+" % (punctuation,), " ", query)
41+ ## plpy.debug('3 query is %s' % repr(query))
42+
43+ # Now that we have handle case sensitive booleans, convert to lowercase
44+ query = query.lower()
45+
46+ # Remove unpartnered bracket on the left and right
47+ query = re.sub(r"(?ux) ^ ( [^(]* ) \)", r"(\1)", query)
48+ query = re.sub(r"(?ux) \( ( [^)]* ) $", r"(\1)", query)
49+
50+ # Remove spurious brackets
51+ query = re.sub(r"(?u)\(([^\&\|]*?)\)", r" \1 ", query)
52+ ## plpy.debug('5 query is %s' % repr(query))
53+
54+ # Insert & between tokens without an existing boolean operator
55+ # ( not proceeded by (|&!
56+ query = re.sub(r"(?u)(?<![\(\|\&\!])\s*\(", "&(", query)
57+ ## plpy.debug('6 query is %s' % repr(query))
58+ # ) not followed by )|&
59+ query = re.sub(r"(?u)\)(?!\s*(\)|\||\&|\s*$))", ")&", query)
60+ ## plpy.debug('6.1 query is %s' % repr(query))
61+ # Whitespace not proceded by (|&! not followed by &|
62+ query = re.sub(r"(?u)(?<![\(\|\&\!\s])\s+(?![\&\|\s])", "&", query)
63+ ## plpy.debug('7 query is %s' % repr(query))
64+
65+ # Detect and repair syntax errors - we are lenient because
66+ # this input is generally from users.
67+
68+ # Fix unbalanced brackets
69+ openings = query.count("(")
70+ closings = query.count(")")
71+ if openings > closings:
72+ query = query + " ) "*(openings-closings)
73+ elif closings > openings:
74+ query = " ( "*(closings-openings) + query
75+ ## plpy.debug('8 query is %s' % repr(query))
76+
77+ # Strip ' character that do not have letters on both sides
78+ query = re.sub(r"(?u)((?<!\w)'|'(?!\w))", "", query)
79+
80+ # Brackets containing nothing but whitespace and booleans, recursive
81+ last = ""
82+ while last != query:
83+ last = query
84+ query = re.sub(r"(?u)\([\s\&\|\!]*\)", "", query)
85+ ## plpy.debug('9 query is %s' % repr(query))
86+
87+ # An & or | following a (
88+ query = re.sub(r"(?u)(?<=\()[\&\|\s]+", "", query)
89+ ## plpy.debug('10 query is %s' % repr(query))
90+
91+ # An &, | or ! immediatly before a )
92+ query = re.sub(r"(?u)[\&\|\!\s]*[\&\|\!]+\s*(?=\))", "", query)
93+ ## plpy.debug('11 query is %s' % repr(query))
94+
95+ # An &,| or ! followed by another boolean.
96+ query = re.sub(r"(?ux) \s* ( [\&\|\!] ) [\s\&\|]+", r"\1", query)
97+ ## plpy.debug('12 query is %s' % repr(query))
98+
99+ # Leading & or |
100+ query = re.sub(r"(?u)^[\s\&\|]+", "", query)
101+ ## plpy.debug('13 query is %s' % repr(query))
102+
103+ # Trailing &, | or !
104+ query = re.sub(r"(?u)[\&\|\!\s]+$", "", query)
105+ ## plpy.debug('14 query is %s' % repr(query))
106+
107+ # If we have nothing but whitespace and tsearch2 operators,
108+ # return NULL.
109+ if re.search(r"(?u)^[\&\|\!\s\(\)]*$", query) is not None:
110+ return None
111+
112+ # Convert back to UTF-8
113+ query = query.encode('utf8')
114+ ## plpy.debug('15 query is %s' % repr(query))
115+
116+ return query or None
117+ $_$;
118+
119+CREATE OR REPLACE FUNCTION ftq(text) RETURNS pg_catalog.tsquery
120+ LANGUAGE plpythonu IMMUTABLE STRICT
121+ AS $_$
122+ p = plpy.prepare(
123+ "SELECT to_tsquery('default', _ftq($1)) AS x", ["text"])
124+ query = plpy.execute(p, args, 1)[0]["x"]
125+ return query or None
126+ $_$;
127+
128+INSERT INTO LaunchpadDatabaseRevision VALUES (2209, 24, 3);
129
130=== modified file 'lib/lp/services/database/doc/textsearching.txt'
131--- lib/lp/services/database/doc/textsearching.txt 2012-06-26 09:40:38 +0000
132+++ lib/lp/services/database/doc/textsearching.txt 2012-07-26 11:30:38 +0000
133@@ -172,23 +172,16 @@
134 >>> ftq('hi AND mom')
135 hi&mom <=> 'hi' & 'mom'
136
137- >>> ftq('hi & mom')
138- hi&mom <=> 'hi' & 'mom'
139-
140 >>> ftq('hi OR mom')
141 hi|mom <=> 'hi' | 'mom'
142
143- >>> ftq('hi | mom')
144- hi|mom <=> 'hi' | 'mom'
145-
146- >>> ftq('hi & -dad')
147+ >>> ftq('hi AND NOT dad')
148 hi&!dad <=> 'hi' & !'dad'
149
150
151-
152 Brackets are allowed to specify precidence
153
154- >>> ftq('(HI OR HELLO) & mom')
155+ >>> ftq('(HI OR HELLO) AND mom')
156 (hi|hello)&mom <=> ( 'hi' | 'hello' ) & 'mom'
157
158 >>> ftq('Hi(Mom)')
159@@ -203,19 +196,16 @@
160 >>> ftq('foo(bar OR baz)') # Bug #32071
161 foo&(bar|baz) <=> 'foo' & ( 'bar' | 'baz' )
162
163- >>> ftq('foo (bar OR baz)')
164- foo&(bar|baz) <=> 'foo' & ( 'bar' | 'baz' )
165-
166
167 We also support negation
168
169- >>> ftq('!Hi')
170+ >>> ftq('NOT Hi')
171 !hi <=> !'hi'
172
173- >>> ftq('-(Hi & Mom)')
174+ >>> ftq('NOT(Hi AND Mom)')
175 !(hi&mom) <=> !( 'hi' & 'mom' )
176
177- >>> ftq('Foo & ! Bar')
178+ >>> ftq('Foo AND NOT Bar')
179 foo&!bar <=> 'foo' & !'bar'
180
181
182@@ -224,7 +214,7 @@
183 >>> ftq('Hi Mom')
184 hi&mom <=> 'hi' & 'mom'
185
186- >>> ftq('Hi -mom')
187+ >>> ftq('Hi NOT mom')
188 hi&!mom <=> 'hi' & !'mom'
189
190 >>> ftq('hi (mom OR mum)')
191@@ -233,18 +223,34 @@
192 >>> ftq('(hi OR hello) mom')
193 (hi|hello)&mom <=> ( 'hi' | 'hello' ) & 'mom'
194
195- >>> ftq('(hi OR hello) -mom')
196+ >>> ftq('(hi OR hello) NOT mom')
197 (hi|hello)&!mom <=> ( 'hi' | 'hello' ) & !'mom'
198
199 >>> ftq('(hi ho OR hoe) work go')
200 (hi&ho|hoe)&work&go <=> ( 'hi' & 'ho' | 'hoe' ) & 'work' & 'go'
201
202
203-If a single '-' precedes a word, it is converted into the '!' operator.
204-Note also that a trailing '-' is dropped by to_tsquery().
205-
206- >>> ftq('-foo bar-')
207- !foo&bar- <=> !'foo' & 'bar'
208+'-' symbols are treated by the Postgres FTI parser context sensitive.
209+If they precede a word, they are removed.
210+
211+ >>> print search_same('foo -bar')
212+ FTI data: 'bar':2 'foo':1
213+ query: 'foo' & 'bar'
214+ match: True
215+
216+If a '-' precedes a number, it is retained.
217+
218+ >>> print search_same('123 -456')
219+ FTI data: '-456':2 '123':1
220+ query: '123' & '-456'
221+ match: True
222+
223+Trailing '-' are always ignored.
224+
225+ >>> print search_same('bar- 123-')
226+ FTI data: '123':2 'bar':1
227+ query: 'bar' & '123'
228+ match: True
229
230 Repeated '-' are simply ignored by to_tsquery().
231
232@@ -259,6 +265,12 @@
233 query: 'foo-bar' & 'foo' & 'bar'
234 match: True
235
236+A '-' surrounded by numbers is treated as the sign of the right-hand number.
237+
238+ >>> print search_same('123-456')
239+ FTI data: '-456':2 '123':1
240+ query: '123' & '-456'
241+ match: True
242
243 Punctuation is handled consistently. If a string containing punctuation
244 appears in an FTI, it can also be passed to ftq(),and a search for this
245@@ -342,11 +354,36 @@
246 >>> print search('some text <div>whatever</div>', 'div')
247 FTI data: 'text':2 'whatev':3 query: 'div' match: False
248
249-Treatment of characters that are used as operators in to_tsquery():
250+The symbols '&', '|' and '!' are treated as operators by to_tsquery();
251+to_tsvector() treats them as whitespace. ftq() converts the words 'AND',
252+'OR', 'NOT' are into these operators expected by to_tsquery(), and it
253+replaces the symbols '&', '|' and '!' with spaces. This avoids
254+surprising search results when the operator symbols appear accidentally
255+in search terms, e.g., by using a plain copy of a source code line as
256+the search term.
257
258 >>> ftq('cool!')
259 cool <=> 'cool'
260
261+ >>> print search_same('Shell scripts usually start with #!/bin/sh.')
262+ FTI data: '/bin/sh':6 'script':2 'shell':1 'start':4 'usual':3
263+ query: 'shell' & 'script' & 'usual' & 'start' & '/bin/sh'
264+ match: True
265+
266+ >>> print search_same('int foo = (bar & ! baz) | bla;')
267+ FTI data: 'bar':3 'baz':4 'bla':5 'foo':2 'int':1
268+ query: 'int' & 'foo' & 'bar' & 'baz' & 'bla'
269+ match: True
270+
271+Queries containing only punctuation symbols yield an empty ts_query
272+object. Note that _ftq() first replaces the '!' with a ' '; later on,
273+_ftq() joins the two remaining terms '?' and '.' with the "AND"
274+operator '&'. Finally, to_tsquery() detects the AND combination of
275+two symbols that are not tokenized and returns null.
276+
277+ >>> ftq('?!.') # Bug 1020443
278+ ?&. <=> None
279+
280 Email addresses are retained as a whole, both by to_tsvector() and by
281 ftq().
282
283@@ -434,7 +471,7 @@
284 Dud queries are 'repaired', such as doubled operators, trailing operators
285 or invalid leading operators
286
287- >>> ftq('hi & OR mom')
288+ >>> ftq('hi AND OR mom')
289 hi&mom <=> 'hi' & 'mom'
290
291 >>> ftq('(hi OR OR hello) AND mom')
292@@ -443,7 +480,7 @@
293 >>> ftq('(hi OR AND hello) AND mom')
294 (hi|hello)&mom <=> ( 'hi' | 'hello' ) & 'mom'
295
296- >>> ftq('(hi OR -AND hello) AND mom')
297+ >>> ftq('(hi OR NOT AND hello) AND mom')
298 (hi|!hello)&mom <=> ( 'hi' | !'hello' ) & 'mom'
299
300 >>> ftq('(hi OR - AND hello) AND mom')
301@@ -452,13 +489,13 @@
302 >>> ftq('hi AND mom AND')
303 hi&mom <=> 'hi' & 'mom'
304
305- >>> ftq('& hi & mom')
306+ >>> ftq('AND hi AND mom')
307 hi&mom <=> 'hi' & 'mom'
308
309- >>> ftq('(& hi | hello) AND mom')
310+ >>> ftq('(AND hi OR hello) AND mom')
311 (hi|hello)&mom <=> ( 'hi' | 'hello' ) & 'mom'
312
313- >>> ftq('() hi mom ( ) ((! |((&)))) :-)')
314+ >>> ftq('() hi mom ( ) ((NOT OR((AND)))) :-)')
315 (hi&mom&-) <=> 'hi' & 'mom'
316
317 >>> ftq("(hi mom")
318@@ -502,10 +539,10 @@
319
320 Bug #160236
321
322- >>> ftq("foo&&bar-baz")
323+ >>> ftq("foo AND AND bar-baz")
324 foo&bar-baz <=> 'foo' & 'bar-baz' & 'bar' & 'baz'
325
326- >>> ftq("foo||bar.baz")
327+ >>> ftq("foo OR OR bar.baz")
328 foo|bar.baz <=> 'foo' | 'bar.baz'
329
330