Merge lp:~ibid-dev/ibid/factoid-544493 into lp:~ibid-core/ibid/old-trunk-1.6

[Stefano Rivera]

Fix deletion of factoids containing args.

[marcog (marco-gallotta)]

This appears to work. I hope I understand what it's doing. :)

[Stefano Rivera (stefanor)]

Can you rebase this off release-0.1, or trunk r908, so it can be merged into both branches, please?

review: Needs Resubmitting

[Stefano Rivera (stefanor)]

Then request merge into 0.1, trunk will come after 0.1 has the merge

[Stefano Rivera (stefanor)]

OK, until we sort tarmac out, please rebase off trunk r908 and request merge into trunk

[marcog (marco-gallotta)]

> OK, until we sort tarmac out, please rebase off trunk r908 and request merge
> into trunk

Done

[Stefano Rivera (stefanor)]

Works for me

review: Approve

[Michael Gorven (mgorven)]

 review approve

review: Approve

[marcog (marco-gallotta)]

Right, I ended up finding and fixing quite a bit more than the original bug.

One bug I can't manage to fix (due to a lack of understanding of wtf the code is actually doing :P), is searching factoids containing $arg doesn't always work as I show here:

Query: a $arg is b
Response: One learns a new thing every day
Query: search b
Response: a $arg [1] <- works correctly
Query: search arg
Response: I couldn't find anything that matched 'arg' <- 'a $arg' should match 'arg', but doesn't

Then, could someone possibly explain what the two-pass search is for in get_factoid (see lines 221-225 and 230-234 in this branch)? I know what I changed will remove the second pass, but I want to understand it's intention so that I can get it working with my patch. I don't think I've ever seen wildcards used, so I'm not sure what wildcards are supported.

[Stefano Rivera (stefanor)]

> .all()[number:]
> .all()[number]

Those alls are required for "I only know foo things", please add a comment explaining that.

> x.like(y, escape='\\')

You can't do that with MySQL. See the equivalent in get_factoid.

The new search code is too complicated, I think.
Does search really need to be implemented as two pass? Why not just skip $arg matching in search and translate $arg into _ instead?

review: Needs Fixing

[Stefano Rivera (stefanor)]

Actually don't review this quite yet, still needs some work...

Unmerged revisions

930. By Stefano Rivera on 2011-01-24

Merged regexp-595423

929. By Stefano Rivera on 2011-01-24

Use # as escape to get around all LIKE escaping issues

928. By Stefano Rivera on 2011-01-24

Use # as escape to get around all LIKE escaping issues

927. By marcog on 2010-03-27

Return number of values known for append when given index is too large

926. By marcog on 2010-03-27

all or literal -> all

925. By marcog on 2010-03-27

Make factoid search aware of args (for non-regex searches)

924. By marcog on 2010-03-27

All implies literal, so set it on top rather than check continuously

923. By marcog on 2010-03-27

Go through the 2nd pass of get_factoid() when no matches found in the 1st pass

922. By marcog on 2010-03-27

Report number of matches in factoid searching when outside the range

921. By marcog on 2010-03-27

Fix start/limit bounds for factoid searching

=== modified file 'ibid/plugins/factoid.py'
--- ibid/plugins/factoid.py	2011-01-24 18:54:59 +0000
+++ ibid/plugins/factoid.py	2011-01-24 18:55:00 +0000
@@ -81,8 +81,12 @@
             # http://www.sqlalchemy.org/trac/ticket/1400:
             # We can't use .like() in MySQL
             for row in self.upgrade_session.query(FactoidName) \
+<<<<<<< TREE
                     .filter('lower(name) LIKE lower(:pattern) ESCAPE :escape') \
                     .params(pattern='%\\_\\%%', escape='\\') \
+=======
+                    .filter(FactoidName.name.like('%#_#%%', escape='#')) \
+>>>>>>> MERGE-SOURCE
                     .all():
                 row.wild = True
                 self.upgrade_session.save_or_update(row)
@@ -210,18 +214,23 @@
 
 action_re = re.compile(r'^\s*<action>\s*')
 reply_re = re.compile(r'^\s*<reply>\s*')
-escape_like_re = re.compile(r'([%_\\])')
+escape_like_re = re.compile(r'([%_#])')
 
 def get_factoid(session, name, number, pattern, is_regex, all=False,
                 literal=False):
     """session: SQLAlchemy session
     name: Factoid name (can contain arguments unless literal query)
-    number: Return factoid[number] (or factoid[number:] for literal queries)
-    pattern: Return factoids matching this pattern (cannot be used in conjuction with number)
+    number: If not None, restrict to factoid[number] (or factoid[number:] for literal queries)
+    pattern: If not None, restrict to factoids matching this pattern (cannot be used in conjuction with number)
     is_regex: Pattern is a real regex
     all: Return a random factoid from the set if False
     literal: Match factoid name literally (implies all)
+    if all or literal, a list is returned; otherwise a single factoid is returned
+    if nothing is found, either an empty list or None is returned
     """
+    assert not (number and pattern), u'number and pattern cannot be used together'
+    if literal:
+        all = True # as mentioned in the docstring
     # First pass for exact matches, if necessary again for wildcard matches
     if literal:
         passes = (False,)
@@ -235,9 +244,14 @@
         if wild:
             # Reversed LIKE because factoid name contains SQL wildcards if
             # factoid supports arguments
+<<<<<<< TREE
             query = query.filter(
                     'lower(:fact) LIKE lower(name) ESCAPE :escape'
                 ).params(fact=name, escape='\\')
+=======
+            query = query.filter(':fact LIKE name ESCAPE :escape') \
+                         .params(fact=name, escape='#')
+>>>>>>> MERGE-SOURCE
         else:
             query = query.filter(FactoidName.name == escape_name(name))
         # For normal matches, restrict to the subset applicable
@@ -249,22 +263,29 @@
                 op = get_regexp_op(session)
                 query = query.filter(op(FactoidValue.value, pattern))
             else:
+<<<<<<< TREE
                 pattern = '%%%s%%' % escape_like_re.sub(r'\\\1', pattern)
                 # http://www.sqlalchemy.org/trac/ticket/1400:
                 # We can't use .like() in MySQL
                 query = query.filter(
                         'lower(value) LIKE lower(:pattern) ESCAPE :escape'
                     ).params(pattern=pattern, escape='\\')
+=======
+                pattern = '%%%s%%' % escape_like_re.sub(r'#\1', pattern)
+                query = query.filter(FactoidValue.value.like(pattern,
+                                                             escape='#'))
+>>>>>>> MERGE-SOURCE
 
-        if number:
+        if number is not None:
+            number = max(0, int(number) - 1)
             try:
                 if literal:
-                    return query.order_by(FactoidValue.id)[int(number) - 1:]
+                    return query.order_by(FactoidValue.id).all()[number:]
                 else:
-                    factoid = query.order_by(FactoidValue.id)[int(number) - 1]
+                    factoid = query.order_by(FactoidValue.id).all()[number]
             except IndexError:
                 continue
-        if all or literal:
+        if all:
             if factoid is not None:
                 return [factoid]
             else:
@@ -273,8 +294,9 @@
             factoid = factoid or query.order_by(func.random()).first()
         if factoid:
             return factoid
-        if all:
-            return []
+    if all:
+        return []
+    return None
 
 class Utils(Processor):
     usage = u'literal <name> [( #<from number> | /<pattern>/[r] )]'
@@ -289,6 +311,15 @@
             event.addresponse(u', '.join(u'%i: %s'
                 % (index + number, value.value)
                   for index, (factoid, name, value) in enumerate(factoids)))
+        else:
+            factoids = get_factoid(event.session, name, None, pattern, is_regex,
+                literal=True)
+            if factoids:
+                event.addresponse(u"I only know %(number)d things about %(name)s", {
+                    u'number': len(factoids),
+                    u'name': name,
+                })
+
 
 class Forget(Processor):
     usage = u"""forget <name> [( #<number> | /<pattern>/[r] )]
@@ -302,17 +333,17 @@
     @match(r'^forget\s+(.+?)(?:\s+#(\d+)|\s+(?:/(.+?)/(r?)))?$')
     @authorise(fallthrough=False)
     def forget(self, event, name, number, pattern, is_regex):
-        factoids = get_factoid(event.session, name, number, pattern, is_regex, all=True)
+        factoids = get_factoid(event.session, name, number, pattern, is_regex, all=True, literal=True)
         if factoids:
             factoidadmin = auth_responses(event, u'factoidadmin')
             identities = get_identities(event)
             factoid = event.session.query(Factoid).get(factoids[0][0].id)
 
-            if (number or pattern):
-                if len(factoids) > 1:
-                    event.addresponse(u'Pattern matches multiple factoids, please be more specific')
-                    return
+            if len(factoids) > 1 and pattern is not None:
+                event.addresponse(u'Pattern matches multiple factoids, please be more specific')
+                return
 
+            if number or pattern:
                 if factoids[0][2].identity_id not in identities and not factoidadmin:
                     return
 
@@ -355,7 +386,14 @@
 
             event.addresponse(True)
         else:
-            event.addresponse(u"I didn't know about %s anyway", name)
+            factoids = get_factoid(event.session, name, None, pattern, is_regex, all=True, literal=True)
+            if factoids:
+                event.addresponse(u"I only know %(number)d things about %(name)s", {
+                    u'number': len(factoids),
+                    u'name': name,
+                })
+            else:
+                event.addresponse(u"I didn't know about %s anyway", name)
 
     @match(r'^(.+)\s+is\s+the\s+same\s+as\s+(.+)$')
     @authorise(fallthrough=False)
@@ -415,35 +453,39 @@
             pattern = m.group(1)
             is_regex = bool(m.group(2))
 
+        if is_regex:
+            filter_op = get_regexp_op(event.session)
+            name_pattern = pattern.replace(r'\$arg', '_%')
+        else:
+            filter_op = lambda x, y: x.like(y, escape='#')
+            pattern = '%%%s%%' % escape_like_re.sub(r'#\1', pattern)
+            name_pattern = pattern.replace('$arg', '#_#%')
+
         query = event.session.query(Factoid)\
-                .join(Factoid.names).add_entity(FactoidName)\
-                .join(Factoid.values)
+                             .join(Factoid.names).add_entity(FactoidName)\
+                             .join(Factoid.values)
 
         if search_type.startswith('fact'):
-            filter_on = (FactoidName.name,)
+            query = query.filter(filter_op(FactoidName.name, name_pattern))
         elif search_type.startswith('value'):
-            filter_on = (FactoidValue.value,)
-        else:
-            filter_on = (FactoidName.name, FactoidValue.value)
-
-        if is_regex:
-            filter_op = get_regexp_op(event.session)
-        else:
-            pattern = "%%%s%%" % escape_like_re.sub(r'\\\1', pattern)
-            filter_op = lambda x, y: x.like(y)
-
-        if len(filter_on) == 1:
-            query = query.filter(filter_op(filter_on[0], pattern))
-        else:
-            query = query.filter(or_(filter_op(filter_on[0], pattern), filter_op(filter_on[1], pattern)))
-
-        # Pre-evalute the iterable or the if statement will be True in SQLAlchemy 0.4 [Bug #383286]
-        matches = [match for match in query[start:start+limit]]
-
-        if matches:
+            query = query.filter(filter_op(FactoidValue.value, pattern))
+        else:
+            query = query.filter(or_(filter_op(FactoidName.name, name_pattern),
+                                     filter_op(FactoidValue.value, pattern)))
+
+        # Pre-evalute the iterable or the if statement will be True in SQLAlchemy 0.4. LP: #383286
+        matches = [match for match in query.all()]
+
+        bounded_matches = matches[start:start+limit]
+        if bounded_matches:
             event.addresponse(u'; '.join(
                 u'%s [%s]' % (fname.name, len(factoid.values))
                 for factoid, fname in matches))
+        elif len(matches):
+            event.addresponse(u"I could only find %(number)d things that matched '%(pattern)s'", {
+                u'number': len(matches),
+                u'pattern': origpattern,
+            })
         else:
             event.addresponse(u"I couldn't find anything that matched '%s'" % origpattern)
 
@@ -623,7 +665,7 @@
     @authorise(fallthrough=False)
     def append(self, event, name, number, pattern, is_regex, suffix):
         name = strip_name(name)
-        factoids = get_factoid(event.session, name, number, pattern, is_regex, all=True)
+        factoids = get_factoid(event.session, name, number, pattern, is_regex, all=True, literal=True)
         if len(factoids) == 0:
             if pattern:
                 event.addresponse(u"I don't know about any %(name)s matching %(pattern)s", {
@@ -631,8 +673,15 @@
                     'pattern': pattern,
                 })
             else:
-                event.addresponse(u"I don't know about %s", name)
-        elif len(factoids) > 1:
+                factoids = get_factoid(event.session, name, None, pattern, is_regex, all=True)
+                if factoids:
+                    event.addresponse(u"I only know %(number)d things about %(name)s", {
+                        u'number': len(factoids),
+                        u'name': name,
+                    })
+                else:
+                    event.addresponse(u"I don't know about %s", name)
+        elif len(factoids) > 1 and number is None:
             event.addresponse(u"Pattern matches multiple factoids, please be more specific")
         else:
             factoidadmin = auth_responses(event, u'factoidadmin')