Merge lp:~chipaca/u1db/split_out_bad_globbage into lp:u1db

[John Lenton]

Split the «user globbed wrong» error from the «user used the wrong number of arguments» error when querying indexes (u1db-client gains better error reporting).

Split the «user globbed wrong» error from the «user used the wrong number of arguments» error when querying indexes.

[Eric Casteleijn (thisfred)]

I don't think the text of the error message is always correct:

["value1*", "value2"] is not two partial searches, it's a case where the partial search is in the wrong place. To put it in language that covers all cases is left as an exercise to the reader.

[Eric Casteleijn (thisfred)]

otherwise fine, tests pass, no memory leaks

review: Approve

[John O'Brien (jdobrien)]

works for me!

review: Approve

[Eric Casteleijn (thisfred)]

Awesome thanks for making it clearer!

=== modified file 'include/u1db/u1db.h'
--- include/u1db/u1db.h	2012-05-23 14:58:00 +0000
+++ include/u1db/u1db.h	2012-05-23 16:48:17 +0000
@@ -67,6 +67,7 @@
 #define U1DB_INVALID_FIELD_SPECIFIER -16
 #define U1DB_DUPLICATE_INDEX_NAME -17
 #define U1DB_INDEX_DOES_NOT_EXIST -18
+#define U1DB_INVALID_GLOBBING -19
 #define U1DB_INTERNAL_ERROR -999
 
 // Used by put_doc_if_newer

=== modified file 'src/u1db_query.c'
--- src/u1db_query.c	2012-05-21 16:29:25 +0000
+++ src/u1db_query.c	2012-05-23 16:48:17 +0000
@@ -725,7 +725,7 @@
             wildcard[i] = 2;
             if (have_wildcard) {
                 //globs not allowed after another wildcard
-                status = U1DB_INVALID_VALUE_FOR_INDEX;
+                status = U1DB_INVALID_GLOBBING;
                 goto finish;
             }
             have_wildcard = 1;
@@ -734,7 +734,7 @@
             wildcard[i] = 0;
             if (have_wildcard) {
                 // Can't have a non-wildcard after a wildcard
-                status = U1DB_INVALID_VALUE_FOR_INDEX;
+                status = U1DB_INVALID_GLOBBING;
                 goto finish;
             }
             add_to_buf(&cur, &buf_size, " AND d%d.value = ?", i);

=== modified file 'u1db/backends/inmemory.py'
--- u1db/backends/inmemory.py	2012-05-23 13:11:46 +0000
+++ u1db/backends/inmemory.py	2012-05-23 16:48:17 +0000
@@ -302,14 +302,14 @@
                     # We have an 'x*' style wildcard
                     if is_wildcard:
                         # We were already in wildcard mode, so this is invalid
-                        raise errors.InvalidValueForIndex()
+                        raise errors.InvalidGlobbing
                     last = idx + 1
                 is_wildcard = True
             else:
                 if is_wildcard:
                     # We were in wildcard mode, we can't follow that with
                     # non-wildcard
-                    raise errors.InvalidValueForIndex()
+                    raise errors.InvalidGlobbing
                 last = idx + 1
         if not is_wildcard:
             return -1

=== modified file 'u1db/backends/sqlite_backend.py'
--- u1db/backends/sqlite_backend.py	2012-05-23 13:11:46 +0000
+++ u1db/backends/sqlite_backend.py	2012-05-23 16:48:17 +0000
@@ -592,13 +592,13 @@
                         if is_wildcard:
                             # We can't have a partial wildcard following
                             # another wildcard
-                            raise errors.InvalidValueForIndex()
+                            raise errors.InvalidGlobbing
                         where.append(like_where[idx])
                         args.append(self._transform_glob(value))
                     is_wildcard = True
                 else:
                     if is_wildcard:
-                        raise errors.InvalidValueForIndex()
+                        raise errors.InvalidGlobbing
                     where.append(exact_where[idx])
                     args.append(value)
             statement = ("SELECT d.doc_id, d.doc_rev, d.content"

=== modified file 'u1db/commandline/client.py'
--- u1db/commandline/client.py	2012-05-23 14:20:12 +0000
+++ u1db/commandline/client.py	2012-05-23 16:48:17 +0000
@@ -351,26 +351,41 @@
             self.stderr.write("Index does not exist.\n")
         except errors.InvalidValueForIndex:
             index_def = db._get_index_definition(index)
-            msg = ["Invalid query;"]
             len_diff = len(index_def) - len(values)
-            if len_diff:
-                msg.append("index %r requires %d query expression%s"
-                           % (index, len(index_def),
-                              "s" if len(index_def) > 1 else ""))
-                if len(values):
-                    msg[-1] += ", not %d" % len(values)
-                if len_diff > 0:
-                    msg[-1] += ".\nPerhaps you meant:"
-                    argv = self.argv if self.argv is not None else sys.argv
-                    msg.extend(argv[:2])
-                    msg.append(repr(database))
-                    msg.extend(map(repr, values))
-                    msg.extend(["'*'" for i in range(len_diff)])
-            else:
+            if len_diff == 0:
                 # can't happen (HAH)
-                msg.append("not sure how to help you (read the docs?)")
-            self.stderr.write(" ".join(msg))
-            self.stderr.write(".\n")
+                raise
+            argv = self.argv if self.argv is not None else sys.argv
+            self.stderr.write(
+                "Invalid query: "
+                "index %r requires %d query expression%s%s.\n"
+                "For example, the following would be valid:\n"
+                "    %s %s %r %r %s\n"
+                % (index,
+                   len(index_def),
+                   "s" if len(index_def) > 1 else "",
+                   ", not %d" % len(values) if len(values) else "",
+                   argv[0], argv[1], database, index,
+                   " ".join(map(repr,
+                                values[:len(index_def)]
+                                + ["*" for i in range(len_diff)])),
+                   ))
+        except errors.InvalidGlobbing:
+            argv = self.argv if self.argv is not None else sys.argv
+            fixed = []
+            for (i, v) in enumerate(values):
+                fixed.append(v)
+                if v.endswith('*'):
+                    break
+            # values has at least one element, so i is defined
+            fixed.extend('*'*(len(values)-i-1))
+            self.stderr.write(
+                "Invalid query: a star can only be followed by stars.\n"
+                "For example, the following would be valid:\n"
+                "    %s %s %r %r %s\n"
+                % (argv[0], argv[1], database, index,
+                   " ".join(map(repr, fixed))))
+
         else:
             self.stdout.write("[")
             for i, doc in enumerate(docs):

=== modified file 'u1db/errors.py'
--- u1db/errors.py	2012-05-21 15:01:18 +0000
+++ u1db/errors.py	2012-05-23 16:48:17 +0000
@@ -44,10 +44,11 @@
 
 
 class InvalidValueForIndex(U1DBError):
-    """The values supplied does not match the index definition.
-
-    Can also be raised if wildcard matches are not strictly at the tail of the
-    request.
+    """The values supplied does not match the index definition."""
+
+
+class InvalidGlobbing(U1DBError):
+    """Raised if wildcard matches are not strictly at the tail of the request.
     """
 
 

=== modified file 'u1db/tests/c_backend_wrapper.pyx'
--- u1db/tests/c_backend_wrapper.pyx	2012-05-23 14:58:00 +0000
+++ u1db/tests/c_backend_wrapper.pyx	2012-05-23 16:48:17 +0000
@@ -116,6 +116,7 @@
     int U1DB_NOT_IMPLEMENTED
     int U1DB_INVALID_JSON
     int U1DB_INVALID_VALUE_FOR_INDEX
+    int U1DB_INVALID_GLOBBING
     int U1DB_BROKEN_SYNC_STREAM
     int U1DB_DUPLICATE_INDEX_NAME
     int U1DB_INDEX_DOES_NOT_EXIST
@@ -559,6 +560,8 @@
                                   % (context,))
     if status == U1DB_INVALID_VALUE_FOR_INDEX:
         raise errors.InvalidValueForIndex()
+    if status == U1DB_INVALID_GLOBBING:
+        raise errors.InvalidGlobbing()
     if status == U1DB_INTERNAL_ERROR:
         raise errors.U1DBError("internal error")
     if status == U1DB_BROKEN_SYNC_STREAM:

=== modified file 'u1db/tests/commandline/test_client.py'
--- u1db/tests/commandline/test_client.py	2012-05-23 14:20:12 +0000
+++ u1db/tests/commandline/test_client.py	2012-05-23 16:48:17 +0000
@@ -524,20 +524,28 @@
     def test_get_from_index_two_expr_instead_of_one(self):
         self.db.create_index("index", ["key1"])
         cmd = self.make_command(client.CmdGetFromIndex)
+        cmd.argv = ["XX", "YY"]
         retval = cmd.run(self.db_path, "index", ["value1", "value2"])
         self.assertEqual(retval, 1)
         self.assertEqual(cmd.stdout.getvalue(), '')
-        self.assertEqual(cmd.stderr.getvalue(), "Invalid query; index"
-                         " 'index' requires 1 query expression, not 2.\n")
+        self.assertEqual("Invalid query: index 'index' requires"
+                         " 1 query expression, not 2.\n"
+                         "For example, the following would be valid:\n"
+                         "    XX YY %r 'index' 'value1'\n"
+                         % self.db_path, cmd.stderr.getvalue())
 
     def test_get_from_index_three_expr_instead_of_two(self):
         self.db.create_index("index", ["key1", "key2"])
         cmd = self.make_command(client.CmdGetFromIndex)
+        cmd.argv = ["XX", "YY"]
         retval = cmd.run(self.db_path, "index", ["value1", "value2", "value3"])
         self.assertEqual(retval, 1)
         self.assertEqual(cmd.stdout.getvalue(), '')
-        self.assertEqual(cmd.stderr.getvalue(), "Invalid query; index"
-                         " 'index' requires 2 query expressions, not 3.\n")
+        self.assertEqual("Invalid query: index 'index' requires"
+                         " 2 query expressions, not 3.\n"
+                         "For example, the following would be valid:\n"
+                         "    XX YY %r 'index' 'value1' 'value2'\n"
+                         % self.db_path, cmd.stderr.getvalue())
 
     def test_get_from_index_one_expr_instead_of_two(self):
         self.db.create_index("index", ["key1", "key2"])
@@ -546,21 +554,24 @@
         retval = cmd.run(self.db_path, "index", ["value1"])
         self.assertEqual(retval, 1)
         self.assertEqual(cmd.stdout.getvalue(), '')
-        self.assertEqual(cmd.stderr.getvalue(), "Invalid query; index"
-                         " 'index' requires 2 query expressions, not 1.\n"
-                         "Perhaps you meant: XX YY %r 'value1' '*'.\n"
-                         % self.db_path)
+        self.assertEqual("Invalid query: index 'index' requires"
+                         " 2 query expressions, not 1.\n"
+                         "For example, the following would be valid:\n"
+                         "    XX YY %r 'index' 'value1' '*'\n"
+                         % self.db_path, cmd.stderr.getvalue())
 
     def test_get_from_index_cant_bad_glob(self):
         self.db.create_index("index", ["key1", "key2"])
         cmd = self.make_command(client.CmdGetFromIndex)
-        retval = cmd.run(self.db_path, "index", ["*", "a"])
+        cmd.argv = ["XX", "YY"]
+        retval = cmd.run(self.db_path, "index", ["value1*", "value2"])
         self.assertEqual(retval, 1)
         self.assertEqual(cmd.stdout.getvalue(), '')
-        # temp error until we split exceptions in the backend
-        # (in a later commit)
-        self.assertEqual(cmd.stderr.getvalue(), "Invalid query;"
-                         " not sure how to help you (read the docs?).\n")
+        self.assertEqual("Invalid query:"
+                         " a star can only be followed by stars.\n"
+                         "For example, the following would be valid:\n"
+                         "    XX YY %r 'index' 'value1*' '*'\n"
+                         % self.db_path, cmd.stderr.getvalue())
 
 
 class RunMainHelper(object):

=== modified file 'u1db/tests/test_backends.py'
--- u1db/tests/test_backends.py	2012-05-22 18:58:04 +0000
+++ u1db/tests/test_backends.py	2012-05-23 16:48:17 +0000
@@ -769,12 +769,12 @@
 
     def test_get_from_index_illegal_wildcard_order(self):
         self.db.create_index('test-idx', ['k1', 'k2'])
-        self.assertRaises(errors.InvalidValueForIndex,
+        self.assertRaises(errors.InvalidGlobbing,
             self.db.get_from_index, 'test-idx', [('*', 'v2')])
 
     def test_get_from_index_illegal_glob_after_wildcard(self):
         self.db.create_index('test-idx', ['k1', 'k2'])
-        self.assertRaises(errors.InvalidValueForIndex,
+        self.assertRaises(errors.InvalidGlobbing,
             self.db.get_from_index, 'test-idx', [('*', 'v*')])
 
     def test_get_all_from_index(self):
@@ -891,13 +891,13 @@
 
     def test_get_from_index_illegal_glob_before_value(self):
         self.db.create_index('test-idx', ['k1', 'k2'])
-        self.assertRaises(errors.InvalidValueForIndex,
+        self.assertRaises(errors.InvalidGlobbing,
             self.db.get_from_index, 'test-idx', [('v*', 'v2')])
 
     def test_get_from_index_illegal_glob_after_glob(self):
         self.db.create_index('test-idx', ['k1', 'k2'])
-        self.assertRaises(errors.InvalidValueForIndex,
-            self.db.get_from_index, 'test-idx', [('*', 'v*')])
+        self.assertRaises(errors.InvalidGlobbing,
+            self.db.get_from_index, 'test-idx', [('v*', 'v*')])
 
     def test_get_from_index_with_sql_wildcards(self):
         self.db.create_index('test-idx', ['key'])

=== modified file 'u1db/tests/test_inmemory.py'
--- u1db/tests/test_inmemory.py	2012-04-27 14:22:47 +0000
+++ u1db/tests/test_inmemory.py	2012-05-23 16:48:17 +0000
@@ -124,5 +124,5 @@
             idx._find_non_wildcards, ('a', 'b'))
         self.assertRaises(errors.InvalidValueForIndex,
             idx._find_non_wildcards, ('a', 'b', 'c', 'd'))
-        self.assertRaises(errors.InvalidValueForIndex,
+        self.assertRaises(errors.InvalidGlobbing,
             idx._find_non_wildcards, ('*', 'b', 'c'))