Make the report query key calculations saner, by sanitizing the query and...

Make the report query key calculations saner, by sanitizing the query and actually using it to execute the queries. Otherwise there's too much potential for differences in the key calculation and the actual index search code.

doc/CHANGES.rst

@@ -35,6 +35,13 @@ Bugs Fixed
 Restructuring
 +++++++++++++
 
+- Cleaned up the Products.ZCatalog search API's. The deprecated support for
+  using `<index id>_usage` arguments in the request has been removed. Support
+  for overriding operators via the `<index id>_operator` syntax has been
+  limited to the query value for each index and no longer works directly on
+  the request. The query is now brought into a canonical form before being
+  passed into the `_apply_index` method of each index.
+
 - Factored out the `Products.MailHost` package into its own distributions. It
   will no longer be included by default in Zope 2.14 but live on as an
   independent add-on.

src/Products/PluginIndexes/DateRangeIndex/DateRangeIndex.py

@@ -250,10 +250,6 @@ class DateRangeIndex(UnIndex):
             If the request does not contain the needed parameters, then
             return None.
 
-            If the request contains a parameter with the name of the
-            column + "_usage", snif for information on how to handle
-            applying the index.
-
             Otherwise return two objects.  The first object is a ResultSet
             containing the record numbers of the matching records.  The
             second object is a tuple containing the names of all data fields

src/Products/PluginIndexes/README.txt

@@ -66,7 +66,7 @@ Changes to ZCatalog
     - Method 2: The query and all parameters to be passed to an index XXX are
       passed as dictionary inside the request dictionary. Example:
       
-        old: <dtml-in myCatalog(myindex='xx yy',myindex_usage':'blabla')
+        old: <dtml-in myCatalog(myindex='xx yy')
         new: <dtml-in myCatalog(myindex={'query':'xx yy','XXXXX':'blabla')
 
         Please check the indexes documentation for informations about additional 

src/Products/PluginIndexes/common/UnIndex.py

@@ -322,12 +322,8 @@ class UnIndex(SimpleItem):
 
           - if the value is a sequence, return a union search.
 
-        If the request contains a parameter with the name of the
-        column + '_usage', it is sniffed for information on how to
-        handle applying the index.
-
-        If the request contains a parameter with the name of the
-        column = '_operator' this overrides the default method
+          - If the value is a dict and contains a key of the form
+            '<index>_operator' this overrides the default method
             ('or') to combine search results. Valid values are "or"
             and "and".
 

src/Products/PluginIndexes/common/util.py

@@ -11,17 +11,18 @@
 #
 ##############################################################################
 """PluginIndexes utils.
-
-$Id$
 """
+
 from types import InstanceType
-from warnings import warn
 
 from DateTime.DateTime import DateTime
 
 
-class parseIndexRequest:
+class IndexRequestParseError(Exception):
+    pass
 
+
+class parseIndexRequest:
     """
     This class provides functionality to hide the internals of a request
     send from the Catalog/ZCatalog to an index._apply_index() method.
@@ -30,7 +31,6 @@ class parseIndexRequest:
 
     - old-style parameters where the query for an index as value inside
       the request directory where the index name is the name of the key.
-      Additional parameters for an index could be passed as index+"_usage" ...
 
     - dictionary-style parameters specify a query for an index as
       an entry in the request dictionary where the key corresponds to the
@@ -50,7 +50,7 @@ class parseIndexRequest:
      parameters
     """
 
-    ParserException = 'IndexRequestParseError'
+    ParserException = IndexRequestParseError
 
     def __init__(self, request, iid, options=[]):
         """ parse a request  from the ZPublisher and return a uniform
@@ -66,16 +66,6 @@ class parseIndexRequest:
             self.keys = None
             return
 
-        # We keep this for backward compatility
-        usage_param = iid + '_usage'
-        if request.has_key(usage_param):
-            self.usage = request[usage_param]
-            warn("ZCatalog query using '%s' detected.\n"
-                 "Using query parameters ending with '_usage' is deprecated.\n"
-                 "Consider using record-style parameters instead "
-                 "(see lib/python/Products/PluginIndexes/README.txt for "
-                 "details)" % usage_param, DeprecationWarning)
-
         param = request[iid]
         keys = None
 
@@ -85,7 +75,7 @@ class parseIndexRequest:
             record = param
 
             if not hasattr(record, 'query'):
-                raise self.ParserException, (
+                raise self.ParserException(
                     "record for '%s' *must* contain a "
                     "'query' attribute" % self.id)
             keys = record.query

src/Products/PluginIndexes/interfaces.py

@@ -63,10 +63,6 @@ class IPluggableIndex(Interface):
         If the request does not contain the needed parameters, then
         None is returned.
 
-        If the request contains a parameter with the name of the column
-        + "_usage", it is sniffed for information on how to handle applying
-        the index. (Note: this style or parameters is deprecated)
-
         If the request contains a parameter with the name of the
         column and this parameter is either a Record or a class
         instance then it is assumed that the parameters of this index

src/Products/ZCatalog/Catalog.py

@@ -440,7 +440,40 @@ class Catalog(Persistent, Acquisition.Implicit, ExtensionClass.Base):
 
 ## This is the Catalog search engine. Most of the heavy lifting happens below
 
-    def search(self, request, sort_index=None, reverse=0, limit=None, merge=1):
+    def make_query(self, request):
+        # This is a bit of a mess, but the ZCatalog API has traditionally
+        # supported passing in query restrictions in almost arbitary ways
+        real_req = None
+        if isinstance(request, dict):
+            query = request.copy()
+        elif isinstance(request, CatalogSearchArgumentsMap):
+            query = {}
+            query.update(request.keywords)
+            real_req = request.request
+            if isinstance(request.request, dict):
+                query.update(real_req)
+            else:
+                real_req = request.request
+        else:
+            real_req = request
+
+        if real_req is not None:
+            # TODO: This deserves depreaction
+            known_keys = query.keys()
+            # The request has too many places where an index restriction
+            # might be specified. Putting all of request.form,
+            # request.other, ... into the query isn't what we want.
+            # So we iterate over all known indexes instead and see if they
+            # are in the request.
+            for iid in self.indexes.keys():
+                if iid in known_keys:
+                    continue
+                value = real_req.get(iid)
+                if value:
+                    query[iid] = value
+        return query
+
+    def search(self, query, sort_index=None, reverse=0, limit=None, merge=1):
         """Iterate through the indexes, applying the query to each one. If
         merge is true then return a lazy result set (sorted if appropriate)
         otherwise return the raw (possibly scored) results for later merging.
@@ -453,13 +486,16 @@ class Catalog(Persistent, Acquisition.Implicit, ExtensionClass.Base):
         rs = None # resultset
 
         # Indexes fulfill a fairly large contract here. We hand each
-        # index the request mapping we are given (which may be composed
+        # index the query mapping we are given (which may be composed
         # of some combination of web request, kw mappings or plain old dicts)
         # and the index decides what to do with it. If the index finds work
-        # for itself in the request, it returns the results and a tuple of
+        # for itself in the query, it returns the results and a tuple of
         # the attributes that were used. If the index finds nothing for it
         # to do then it returns None.
 
+        # Canonicalize the request into a sensible query before passing it on
+        query = self.make_query(query)
+
         # For hysterical reasons, if all indexes return None for a given
         # request (and no attributes were used) then we append all results
         # in the Catalog. This generally happens when the search values
@@ -469,7 +505,7 @@ class Catalog(Persistent, Acquisition.Implicit, ExtensionClass.Base):
         # Note that if the indexes find query arguments, but the end result
         # is an empty sequence, we do nothing
 
-        cr = self.getCatalogReport(request)
+        cr = self.getCatalogReport(query)
         cr.start()
 
         for i in self.indexes.keys():
@@ -479,7 +515,7 @@ class Catalog(Persistent, Acquisition.Implicit, ExtensionClass.Base):
                 continue
 
             cr.split(i)
-            r = _apply_index(request)
+            r = _apply_index(query)
             cr.split(i)
 
             if r is not None:
@@ -491,7 +527,7 @@ class Catalog(Persistent, Acquisition.Implicit, ExtensionClass.Base):
         cr.stop()
 
         if rs is None:
-            # None of the indexes found anything to do with the request
+            # None of the indexes found anything to do with the query
             # We take this to mean that the query was empty (an empty filter)
             # and so we return everything in the catalog
             if sort_index is None:
@@ -729,9 +765,12 @@ class Catalog(Persistent, Acquisition.Implicit, ExtensionClass.Base):
             return None
 
     def searchResults(self, REQUEST=None, used=None, _merge=1, **kw):
+        # You should pass in a simple dictionary as the request argument,
+        # which only contains the relevant query.
         # The used argument is deprecated and is ignored
         if REQUEST is None and not kw:
             # Try to acquire request if we get no args for bw compat
+            # TODO: Should be deprecated
             REQUEST = getattr(self, 'REQUEST', None)
         args = CatalogSearchArgumentsMap(REQUEST, kw)
         sort_index = self._getSortIndex(args)
@@ -747,12 +786,12 @@ class Catalog(Persistent, Acquisition.Implicit, ExtensionClass.Base):
                 
     __call__ = searchResults
 
-    def getCatalogReport(self, request=None):
+    def getCatalogReport(self, query=None):
         """Reports about the duration of queries.
         """
         parent = Acquisition.aq_base(Acquisition.aq_parent(self))
         threshold = getattr(parent, 'long_query_time', 0.1)
-        return CatalogReport(self, request, threshold)
+        return CatalogReport(self, query, threshold)
 
 
 class CatalogSearchArgumentsMap:

src/Products/ZCatalog/report.py

@@ -75,37 +75,9 @@ addCleanUp(clear_value_indexes)
 del addCleanUp
 
 
-def make_query(indexes, request):
-    # This is a bit of a mess, but the ZCatalog API supports passing
-    # in query restrictions in almost arbitary ways
-    if isinstance(request, dict):
-        query = request.copy()
-    else:
-        query = {}
-        query.update(request.keywords)
-        real_req = request.request
-        if isinstance(real_req, dict):
-            query.update(real_req)
-
-        known_keys = query.keys()
-        # The request has too many places where an index restriction might be
-        # specified. Putting all of request.form, request.other, ... into the
-        # key isn't what we want either, so we iterate over all known indexes
-        # instead and see if they are in the request.
-        for iid in indexes.keys():
-            if iid in known_keys:
-                continue
-            value = real_req.get(iid)
-            if value:
-                query[iid] = value
-    return query
-
-
-def make_key(catalog, request):
+def make_key(catalog, query):
     indexes = catalog.indexes
     valueindexes = determine_value_indexes(indexes)
-
-    query = make_query(indexes, request)
     key = keys = query.keys()
 
     values = [name for name in keys if name in valueindexes]
@@ -167,11 +139,11 @@ class CatalogReport(StopWatch):
     """Catalog report class to meassure and identify catalog queries.
     """
 
-    def __init__(self, catalog, request=None, threshold=0.1):
+    def __init__(self, catalog, query=None, threshold=0.1):
         super(CatalogReport, self).__init__()
 
         self.catalog = catalog
-        self.request = request
+        self.query = query
         self.threshold = threshold
 
         parent = aq_parent(catalog)
@@ -195,7 +167,7 @@ class CatalogReport(StopWatch):
         # The key calculation takes a bit itself, we want to avoid that for
         # any fast queries. This does mean that slow queries get the key
         # calculation overhead added to their runtime.
-        key = make_key(self.catalog, self.request)
+        key = make_key(self.catalog, self.query)
 
         reports_lock.acquire()
         try:

src/Products/ZCatalog/tests/testCatalog.py

@@ -389,9 +389,11 @@ class TestCatalogObject(unittest.TestCase):
         a = self._catalog({})
         self.assertEqual(len(a), upper,
                          'length should be %s, its %s' % (upper, len(a)))
-        # Queries consisting of empty strings should do the same
+        # Queries used to do the same, because of a bug in the
+        # parseIndexRequest function, mistaking a CatalogSearchArgumentsMap
+        # for a Record class
         a = self._catalog({'col1':'', 'col2':'', 'col3':''})
-        self.assertEqual(len(a), upper,
+        self.assertEqual(len(a), 0,
                          'length should be %s, its %s' % (upper, len(a)))
 
     def testFieldIndexLength(self):