Merge branch 'catalog_join'

product/ERP5/bootstrap/erp5_mysql_innodb_catalog/CatalogMethodTemplateItem/portal_catalog/erp5_mysql_innodb/z_related_grand_parent.xml

@@ -14,8 +14,10 @@
         </item>
         <item>
             <key> <string>arguments_src</string> </key>
-            <value> <string>table_0\n
-table_1</string> </value>
+            <value> <string>table_0\r\n
+table_1\r\n
+RELATED_QUERY_SEPARATOR=" AND "\r\n
+query_table="catalog"</string> </value>
         </item>
         <item>
             <key> <string>cache_time_</string> </key>
@@ -55,10 +57,9 @@ table_1</string> </value>
             <key> <string>src</string> </key>
             <value> <string encoding="cdata"><![CDATA[
 
-<dtml-var table_0>.uid = catalog.parent_uid\n
-AND <dtml-var table_1>.uid = <dtml-var table_0>.parent_uid\n
-\n
-
+<dtml-var table_1>.uid = <dtml-var table_0>.parent_uid\n
+<dtml-var RELATED_QUERY_SEPARATOR>\n
+<dtml-var table_0>.uid = <dtml-var query_table>.parent_uid
 
 ]]></string> </value>
         </item>

product/ERP5Catalog/tests/testERP5Catalog.py

@@ -104,6 +104,12 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
   new_erp5_deferred_sql_connection = 'erp5_sql_deferred_connection2'
   new_catalog_id = 'erp5_mysql_innodb2'
 
+  __cleanups = ()
+
+  def _addCleanup(self, callable):
+    self.__cleanups += (callable,)
+    return callable
+
   def afterSetUp(self):
     uf = self.getPortal().acl_users
     uf._doAddUser(self.username, '', ['Manager'], [])
@@ -128,6 +134,8 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
       self.portal.manage_delObjects([self.new_erp5_deferred_sql_connection])
     if self.new_catalog_id in self.portal.portal_catalog.objectIds():
       self.portal.portal_catalog.manage_delObjects([self.new_catalog_id])
+    for cleanup in self.__cleanups:
+      cleanup(self)
     transaction.commit()
     self.tic()
 
@@ -707,7 +715,6 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
     default_parametrs['where_expression'] = ""
     default_parametrs['order_by_expression'] = None
     
-    #import pdb; pdb.set_trace()
     # check that we retrieve our 2 organisations by default.
     kw = default_parametrs.copy()
     kw.update( portal_catalog.buildSQLQuery(
@@ -955,14 +962,13 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
             ('catalog.title DESC', '`catalog`.`title` DESC'))
     
   def test_26_SortOnUnknownKeys(self, quiet=quiet, run=run_all_test):
-    if not run: return
     if not run: return
     if not quiet:
       message = 'Test Sort On Unknow Keys'
       ZopeTestCase._print('\n%s ' % message)
       LOG('Testing... ',0,message)
     self.assertEquals('',
-          self.getCatalogTool().buildSQLQuery(
+          self.getCatalogTool().buildSQLQuery(select_list=('uid', 'path'),
           sort_on=(('ignored', 'ascending'),))['order_by_expression'])
   
   def test_27_SortOnAmbigousKeys(self, quiet=quiet, run=run_all_test):
@@ -1028,6 +1034,7 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
               'sort_on parameter must be taken into account even if related key '
               'is not a parameter of the current query')
 
+
   def _makeOrganisation(self, **kw):
     """Creates an Organisation in it's default module and reindex it.
     By default, it creates a group/nexedi category, and make the organisation a
@@ -1332,11 +1339,13 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
     # catalog those objects
     transaction.commit()
     self.tic()
+    catalog_tool = self.getCatalogTool()
     self.assertEquals([ob],
-        [x.getObject() for x in self.getCatalogTool()(
-                portal_type='Organisation', SearchableText='title')])
-    self.assertEquals(1, self.getCatalogTool().countResults(
-              portal_type='Organisation', SearchableText='title')[0][0])
+        [x.getObject() for x in catalog_tool(portal_type='Organisation',
+                                             SearchableText='title')])
+    self.assertEquals(1,
+                      catalog_tool.countResults(portal_type='Organisation',
+                                                SearchableText='title')[0][0])
 
     # 'different' is found in more than 50% of records
     # MySQL ignores such a word, but Tritonn does not ignore.
@@ -4279,11 +4288,13 @@ VALUES
     self.assertEqual(catalog.countResults(parent_uid=module_uid)[0][0],
       module_len)
 
-    self.assertEquals(catalog.countResults(from_expression={
-      'catalog': '(SELECT sub_catalog.* FROM catalog AS sub_catalog' \
-        ' WHERE sub_catalog.parent_uid=%i)' \
+    from_expression = {
+      'catalog': '(SELECT sub_catalog.* FROM catalog AS sub_catalog'
+                 ' WHERE sub_catalog.parent_uid=%i)'
                  ' AS catalog' % (module_uid, ),
-    })[0][0], module_len)
+    }
+    count = catalog.countResults(from_expression=from_expression)[0][0]
+    self.assertEqual(count, module_len)
 
   def test_getParentUid(self, quiet=quiet):
     from Products.ERP5.Document.Assignment import Assignment
@@ -4328,6 +4339,176 @@ VALUES
     result = connector.manage_test('select 1 as foo;')
     self.assertEquals(1, result[0].foo)
 
+  def _createSomeGroupCategories(self):
+    portal_category = self.getCategoryTool()
+    group_category = portal_category.group
+    group_data_map = dict(nexedi=('Nexedi', 'Nexedi Group'),
+                          tiolive=('TIOLive', 'TioLive Group'),)
+    existing_group_id_list = group_category.objectIds()
+    for group_id, (title, description) in group_data_map.items():
+      if group_id in existing_group_id_list:
+        group = group_category[group_id]
+      else:
+        group = group_category.newContent(id=group_id)
+      group.edit(title=title, description=description)
+
+  def test_SelectDictWithDynamicRelatedKey(self, quiet=quiet, run=run_all_test):
+    if not run: return
+    if not quiet:
+      message = 'Select Dict With Dynamic Related Key'
+      ZopeTestCase._print('\n%s ' % message)
+      LOG('Testing... ', 0, message)
+
+    self._createSomeGroupCategories()
+
+    # Create some orgs associated with varying association with the
+    # groups created above.
+    module = self.portal.getDefaultModule('Organisation')
+    # org1 has no groups
+    org1 = module.newContent(portal_type='Organisation', title='org1')
+    # org2 has group nexedi
+    org2 = module.newContent(portal_type='Organisation', title='org2')
+    org2.setGroupList(['nexedi'])
+    # org3 has group tiolive
+    org3 = module.newContent(portal_type='Organisation', title='org3')
+    org3.setGroupList(['tiolive'])
+    # org4 has both groups
+    org4 = module.newContent(portal_type='Organisation', title='org4')
+    org4.setGroupList(['nexedi', 'tiolive'])
+    # check associations are correct
+    actual_group_title_map = dict((org.getTitle(), 
+                                   sorted(org.getGroupTitleList()))
+                                  for org in (org1, org2, org3, org4))
+    expected_group_title_map = dict(org1=[],
+                                    org2=['Nexedi'],
+                                    org3=['TIOLive'],
+                                    org4=['Nexedi', 'TIOLive'])
+    self.assertEquals(actual_group_title_map, expected_group_title_map)
+    # Flush message queue
+    transaction.commit()
+    self.tic()
+
+    # we will restrict our search to orgs with these ids to be resilient
+    # to preexisting orgs:
+    org_id_list = sorted(org.getId() for org in (org1, org2, org3, org4))
+    # and we'll sort on title to make the output predictable
+    search_kw = dict(id=org_id_list,
+                     sort_on='title')
+    # Try to get the organisations with the group title Nexedi to make sure
+    # searching works correctly
+    organisation_list = [x.getObject() for x in 
+                         module.searchFolder(strict_group_title='Nexedi',
+                                             **search_kw)]
+    self.assertEquals(organisation_list, [org2, org4])
+    # Now lets fetch the titles of groups of the above orgs using select_dict.
+    search_kw.update(select_dict=dict(strict_group_title=None))
+    records = module.searchFolder(**search_kw)
+    # by default the catalog only returns items containing the
+    # relationship we asked for (group). Besides, some entries will
+    # appear many times, according to the number of relationships each
+    # catalog entry has in that related key.
+    results = [(rec.title, rec.strict_group_title or '-None-')
+               for rec in records]
+    self.assertEquals(sorted(results), 
+                      [('org2', 'Nexedi'),
+                       ('org3', 'TIOLive'),
+                       ('org4', 'Nexedi'),
+                       ('org4', 'TIOLive')])
+    # But if we demand a left-join on that column, then we'll have all
+    # orgs we created. They'll still be repeated according to their
+    # relationships, though.
+    search_kw.update(left_join_list=('strict_group_title',))
+    records = module.searchFolder(**search_kw)
+    results = [(rec.title, rec.strict_group_title or '-None-')
+               for rec in records]
+    self.assertEquals(sorted(results), 
+                      [('org1', '-None-'),
+                       ('org2', 'Nexedi'),
+                       ('org3', 'TIOLive'),
+                       ('org4', 'Nexedi'),
+                       ('org4', 'TIOLive')])
+    # To get only one of each org, we need to group by one of the
+    # catalog keys.
+
+    # Note that this relies on a non-standard behaviour
+    # of MySQL: If a selected column is not present in the GROUP BY
+    # clause, only the first ocurrence is taken.  Other databases,
+    # like Oracle, assume that selected columns are either GROUPed BY
+    # or are inside an aggregation function (COUNT, SUM, GROUP_CONCAT,
+    # ...), and consider the query to be in error otherwise.
+    search_kw.update(group_by_list=('uid',))
+    organisation_list = [x.getObject() for x in 
+                         module.searchFolder(**search_kw)]
+    self.assertEquals(organisation_list, [org1, org2, org3, org4])
+
+  def test_BackwardCompatibilityWithOldMethods(self, quiet=quiet, 
+                                               run=run_all_test):
+    if not run: return
+    if not quiet:
+      message = 'Dealing with RelatedKey methods missing the proper separator'
+      ZopeTestCase._print('\n%s ' % message)
+      LOG('Testing... ', 0, message)
+
+    module = self.getOrganisationModule()
+    org_a = self._makeOrganisation(title='abc',default_address_city='abc')
+    org_a.setReference(org_a.getId())
+    # sometimes the module itself is not indexed yet...
+    module.reindexObject()
+
+    # Flush message queue
+    transaction.commit()
+    self.tic()
+
+    # make a query to fetch the address of the organisation above by
+    # querying, among other things, the grand_parent
+    query = dict(grand_parent_portal_type="Organisation Module",
+                 parent_reference=org_a.getReference())
+    catalog = self.getCatalogTool()
+    # check the query works normally
+    self.assertEqual([x.getObject() for x in catalog.searchResults(**query)],
+                     [org_a.default_address])
+
+    # even if we do a left_join
+    query_lj = query.copy()
+    query_lj.update(left_join_list=('grand_parent_portal_type',))
+    self.assertEqual([x.getObject() for x in catalog.searchResults(**query_lj)],
+                     [org_a.default_address])
+    
+    # now turn the z_related_grand_parent into an old-style method, without
+    # RELATED_QUERY_SEPARATOR
+    method = catalog.getSQLCatalog().z_related_grand_parent
+    old_src = method.src
+
+    @self._addCleanup
+    def cleanGrandParentMethod(self):
+      method.manage_edit(method.title, method.connection_id,
+                         method.arguments_src, old_src)
+
+    src = old_src.replace('<dtml-var RELATED_QUERY_SEPARATOR>', ' AND ')
+    method.manage_edit(method.title, method.connection_id, method.arguments_src,
+                       src)
+
+    # check that it still works
+    self.assertEqual([x.getObject() for x in catalog.searchResults(**query)],
+                     [org_a.default_address])
+
+    # now try to do a left-join on grand_parent_portal_type which
+    # shouldn't work
+    self.assertRaises(RuntimeError, lambda: catalog.searchResults(**query_lj))
+
+    # Neither should it work if a left-join is attempted in a column
+    # that has proper related-key rendering, but is present in the
+    # same query as a column that hasn't, as the whole query is
+    # converted into implicit inner joins.
+    self.tic()
+    query_lj.update(left_join_list=('strict_group_title',),
+                    select_dict=('strict_group_title',))
+    self.assertRaises(RuntimeError, lambda: catalog.searchResults(**query_lj))
+    # though it should work on queries that don't use the broken related-key
+    del query_lj['grand_parent_portal_type']
+    self.assertEqual([x.getObject() for x in catalog.searchResults(**query_lj)],
+                     [org_a.default_address])
+
 def test_suite():
   suite = unittest.TestSuite()
   suite.addTest(unittest.makeSuite(TestERP5Catalog))

product/ERP5Type/Core/Predicate.py

@@ -329,7 +329,10 @@ class Predicate(XMLObject):
       catalog_kw['where_expression'] = SQLQuery(sql_text)
     else:
       catalog_kw['where_expression'] = ''
+    # force implicit join
+    catalog_kw['implicit_join'] = True
     sql_query = portal_catalog.buildSQLQuery(**catalog_kw)
+    # XXX from_table_list is None most of the time after the explicit_join work
     for alias, table in sql_query['from_table_list']:
       if from_table_dict.has_key(alias):
         raise KeyError, "The same table is used twice for an identity criterion and for a membership criterion"

product/ZSQLCatalog/ColumnMap.py

@@ -27,11 +27,18 @@
 #
 ##############################################################################
 
+import re
+import itertools
 from zLOG import LOG, WARNING, INFO
 from interfaces.column_map import IColumnMap
 from zope.interface.verify import verifyClass
 from zope.interface import implements
-from SQLCatalog import profiler_decorator
+from Products.ZSQLCatalog.interfaces.column_map import IColumnMap
+from Products.ZSQLCatalog.SQLCatalog import profiler_decorator
+from Products.ZSQLCatalog.TableDefinition import (PlaceHolderTableDefinition,
+                                                  TableAlias,
+                                                  InnerJoin,
+                                                  LeftJoin)
 
 DEFAULT_GROUP_ID = None
 
@@ -43,12 +50,18 @@ MAPPING_TRACE = False
 #       currently, it's not possible because related_key_dict is indexed by related key name, which makes 'source_title_1' lookup fail. It should be indexed by group (probably).
 # TODO: rename all "related_key" references into "virtual_column"
 
+re_sql_as = re.compile("\s+AS\s[^)]+$", re.IGNORECASE | re.MULTILINE)
+
 class ColumnMap(object):
 
   implements(IColumnMap)
 
   @profiler_decorator
-  def __init__(self, catalog_table_name=None):
+  def __init__(self,
+               catalog_table_name=None,
+               table_override_map=None,
+               left_join_list=None,
+               implicit_join=False):
     self.catalog_table_name = catalog_table_name
     # Key: group
     # Value: set of column names
@@ -81,15 +94,28 @@ class ColumnMap(object):
     self.raw_column_dict = {}
     # Entries: column name
     self.column_ignore_set = set()
-    self.join_table_set = set()
-    self.straight_join_table_list = []
-    self.left_join_table_list = []
+    self.join_table_map = dict()
+    # BBB: Remove join_query_list and its uses when all RelatedKey
+    # methods have been converted to properly return each Join
+    # condition separately, and all uses of catalog's from_expression
+    # have been removed.
     self.join_query_list = []
+    self.table_override_map = table_override_map or {}
+    self.table_definition = PlaceHolderTableDefinition()
+    # We need to keep track of the original definition to do inner joins on it
+    self._inner_table_definition = self.table_definition
+    self.left_join_list = left_join_list
+    self.implicit_join = implicit_join
+    assert not (self.implicit_join and self.left_join_list), (
+      "Cannot do left_joins while forcing implicit join"
+    )
 
   @profiler_decorator
   def registerColumn(self, raw_column, group=DEFAULT_GROUP_ID, simple_query=None):
     assert ' as ' not in raw_column.lower()
     # Sanitize input: extract column from raw column (might contain COUNT, ...).
+    # XXX This is not enough to parse something like: 
+    # GROUP_CONCAT(DISTINCT foo ORDER BY bar)
     if '(' in raw_column:
       function, column = raw_column.split('(')
       column = column.strip()
@@ -120,7 +146,7 @@ class ColumnMap(object):
         # When a column is registered  in default group and is explicitely
         # mapped to a table, we must mark its table as requiring a join with
         # catalog table (unless it's the catalog table, of course).
-        self._addJoinTable(table, group)
+        self._addJoinTableForColumn(table, table + "." + column, group)
 
   def ignoreColumn(self, column):
     self.column_ignore_set.add(column)
@@ -255,7 +281,7 @@ class ColumnMap(object):
           #   Although the list of tables those columns belong to is known
           #   earlier (in "build"), mapping them here
           #   - avoids code duplication (registerTable, resolveColumn,
-          #     _addJoinTable)
+          #     _addJoinTableForColumn)
           #   - offers user to vote for an unknown table, overriding this
           #     forced mapping.
           use_allowed = table_name == catalog_table_name or \
@@ -300,10 +326,11 @@ class ColumnMap(object):
       self.registerTable(table_name, group=group)
       self.resolveColumn(column_name, table_name, group=group)
       if table_name != catalog_table_name:
-        self._addJoinTable(table_name, group)
+        self._addJoinTableForColumn(table_name, column_name, group)
 
   @profiler_decorator
   def build(self, sql_catalog):
+    join_query_to_build_list = []
     catalog_table_name = self.catalog_table_name
     if catalog_table_name is None:
       return
@@ -319,7 +346,7 @@ class ColumnMap(object):
         if related_key_definition is not None:
           join_query = sql_catalog.getSearchKey(column_name, 'RelatedKey').buildQuery(sql_catalog=sql_catalog, related_key_definition=related_key_definition)
           join_query.registerColumnMap(sql_catalog, self)
-          self._addJoinQuery(join_query)
+          join_query_to_build_list.append(join_query)
 
     # List all possible tables, with all used column for each
     for group, column_set in self.registry.iteritems():
@@ -397,6 +424,20 @@ class ColumnMap(object):
         table_alias_number_dict[alias_table_name] = table_alias_number
       self.resolveTable(table_name, alias, group=group)
 
+    # now that we have all aliases, calculate missing joins comming from
+    # non-RelatedKey relationships (like full_text).
+    self.registerCatalog()
+    self._calculateMissingJoins()
+    # and all left joins that did not come from explicit queries
+    # (i.e. joins comming from 'sort_on', 'select_dict', etc.)
+    for join_query in join_query_to_build_list:
+      # XXX ugly use of inner attribute of join_query. Please Refactor:
+      # search_keys don't actually return SQLExpressions, but they add
+      # join definitions in the column_map
+      join_query.search_key.buildSQLExpression(sql_catalog=sql_catalog,
+                                               column_map=self,
+                                               only_group_columns=False,
+                                               group=join_query.group,)
     if MAPPING_TRACE:
       # Key: group
       # Value: 2-tuple
@@ -441,8 +482,28 @@ class ColumnMap(object):
   def getCatalogTableAlias(self, group=DEFAULT_GROUP_ID):
     return self.table_alias_dict[(group, self.catalog_table_name)]
 
+  def _isBackwardCompatibilityRequired(self):
+    return bool(
+      # if they explicitly ask for implicit
+      self.implicit_join or
+      # if they don't pass a catalog alias, we cannot do explicit joins
+      not self._setMinimalTableDefinition() or
+      # If one or more RelatedKey methods weren't converted, we'll get
+      # queries for an implicit inner join, so we have to do all joins
+      # as implicit.
+      self.join_query_list or
+      # for now, work in BW compat mode if a table_override
+      # is passed.  It only works for simple subselect
+      # definitions anyway, and it's being used primarily
+      # for writing left-joins manually.
+      self.table_override_map)
+
   def getTableAliasDict(self):
+    if self._isBackwardCompatibilityRequired():
+      # BBB: Using implicit joins or explicit from_expression
       return self.table_map.copy()
+    else:
+      return None
 
   @profiler_decorator
   def resolveColumn(self, column, table_name, group=DEFAULT_GROUP_ID):
@@ -472,16 +533,30 @@ class ColumnMap(object):
   def getTableAlias(self, table_name, group=DEFAULT_GROUP_ID):
     return self.table_alias_dict[(group, table_name)]
 
-  def _addJoinQuery(self, query):
+  def _addJoinQueryForColumn(self, column, query):
+    # BBB: This is a backward compatibility method that will be
+    # removed in the future, when all related key methods have been adapted
+    # to provide all Join conditions separately
+    if column in self.left_join_list:
+      raise RuntimeError('Left Join requested for column: %r, but rendered '
+                         'join query is not compatible and would result in an '
+                         'Implicit Inner Join:\n%s' % 
+                         (column, query,))
     self.join_query_list.append(query)
 
   def iterJoinQueryList(self):
-    return iter(self.join_query_list)
+    if self._isBackwardCompatibilityRequired():
+      # Return all join queries for implicit join, and all the other
+      # queries we were using to build explicit joins, but won't be able to.
+      return itertools.chain(self.join_query_list,
+                             self.table_definition.getJoinConditionQueryList())
+    return []
+    
 
   @profiler_decorator
-  def _addJoinTable(self, table_name, group=DEFAULT_GROUP_ID):
+  def _addJoinTableForColumn(self, table_name, column, group=DEFAULT_GROUP_ID):
     """
-      Declare given table as requiring to be joined with catalog table.
+      Declare given table as requiring to be joined with catalog table on uid.
 
       table_name (string)
         Table name.
@@ -497,17 +572,137 @@ class ColumnMap(object):
         # Register uid column if it is not already
         self.registerColumn('uid')
         self.resolveColumn('uid', catalog_table)
-      self.join_table_set.add((group, table_name))
+      self.join_table_map.setdefault((group, table_name), set()).add(column)
 
   def getJoinTableAliasList(self):
     return [self.getTableAlias(table_name, group=group)
-            for (group, table_name) in self.join_table_set]
-
-  def getStraightJoinTableList(self):
-    return self.straight_join_table_list[:]
-
-  def getLeftJoinTableList(self):
-    return self.left_join_table_list[:]
+            for (group, table_name) in self.join_table_map.keys()]
+
+  def _getTableOverride(self, table_name):
+    # self.table_override_map is a dictionary mapping table names to
+    # strings containing aliases of arbitrary table definitions
+    # (including subselects). So we split the alias and discard it
+    # since we do our own aliasing.
+    table_override_w_alias = self.table_override_map.get(table_name)
+    if table_override_w_alias is None:
+      return table_name
+    # XXX move the cleanup of table alias overrides to EntireQuery
+    # class or ZSQLCatalog, so we don't need SQL syntax knowledge in
+    # ColumnMap. 
+    #
+    # Normalise the AS sql keyword to remove the last
+    # aliasing in the string if present. E.g.:
+    # 
+    # '(SELECT sub_catalog.* 
+    #   FROM catalog AS sub_catalog
+    #   WHERE sub_catalog.parent_uid=183) AS catalog'
+    #
+    # becomes:
+    #
+    # '(SELECT sub_catalog.* 
+    #   FROM catalog AS sub_catalog
+    #   WHERE sub_catalog.parent_uid=183)'
+    table_override, removed = re_sql_as.subn('', table_override_w_alias)
+    assert removed < 2, ('More than one table aliasing was removed from %r' %
+                        table_override_w_alias)
+    if removed:
+      LOG('ColumnMap', WARNING,
+          'Table overrides should not contain aliasing: %r' % table_override)
+    return table_override
+
+  def makeTableAliasDefinition(self, table_name, table_alias):
+    """Make a table alias, giving a change to ColumnMap to override
+    the original table definition with another expression"""
+    table_name = self._getTableOverride(table_name)
+    assert table_name and table_alias, ("table_name (%r) and table_alias (%r) "
+                                        "must both be defined" %
+                                        (table_name, table_alias))
+    return TableAlias(table_name, table_alias)
+
+  def _setMinimalTableDefinition(self):
+    """ Set a minimal table definition: the main catalog alias
+
+    We don't do this at __init__ because we have neither the catalog
+    table name nor its intended alias at that point.
+    """
+    inner_def = self._inner_table_definition
+    if inner_def.table_definition is None:
+      try:
+        catalog_table_alias = self.getCatalogTableAlias()
+      except KeyError:
+        LOG('ColumnMap', WARNING,
+            '_setMinimalTableDefinition called but the main catalog has not '
+            'yet received an alias!')
+        return False
+      inner_def.replace(self.makeTableAliasDefinition(self.catalog_table_name,
+                                                      catalog_table_alias))
+    return True
+
+  def getTableDefinition(self):
+    if self._isBackwardCompatibilityRequired():
+      # BBB: One of the RelatedKeys registered an implicit join, do
+      # not return a table definition, self.getTableAliasDict() should
+      # be used instead
+      return None
+    self.table_definition.checkTableAliases()
+    return self.table_definition
+
+  def addRelatedKeyJoin(self, column, right_side, condition):
+    """ Wraps the current table_definition in the left-side of a new
+    join.  Use an InnerJoin or a LeftJoin depending on whether the
+    column is in the left_join_list or not.
+    """
+    # XXX: to fix TestERP5Catalog.test_52_QueryAndTableAlias, create
+    # here a list of joins and try to merge each new entry into one of
+    # the pre-existing entries by comparing their right-sides.
+    # 
+    # XXX 2: This is the place were we could do ordering of inner and left
+    # joins so as to get better performance. For instance, a quick win is to
+    # add all inner-joins first, and all left-joins later. We could also decide
+    # on the order of left-joins based on the order of self.left_join_list or
+    # even a catalog property/configuration/script.
+    #
+    # XXX 3: This is also the place where we could check if explicit
+    # table aliases should cause some of these table definitions to be
+    # collapsed into others.
+    assert self._setMinimalTableDefinition()
+    Join = (column in self.left_join_list) and LeftJoin or InnerJoin
+    join_definition = Join(self.table_definition, right_side,
+                           condition=condition)
+    self.table_definition = join_definition
+
+  # def getFinalTableDefinition(self):
+  #   self._calculateMissingJoins()
+  #   return self.getTableDefinition()
+
+  def _calculateMissingJoins(self):
+    left_join_set = set(self.left_join_list)
+    self._setMinimalTableDefinition()
+    catalog_table_alias = self.getCatalogTableAlias()
+    for (group, table_name), column_set in self.join_table_map.items():
+      # if any of the columns for this implicit join was requested as a
+      # left-join, then all columns will be subject to a left-join.
+      # XXX What if one of the columns was an actual query, as opposed to a
+      # sort column or select_dict? This would cause results in the main
+      # catalog that don't match the query to be present as well. We expect
+      # the user which passes a left_join_list to know what he is doing.
+      if column_set.intersection(left_join_set):
+        Join = LeftJoin
+      else:
+        Join = InnerJoin
+      table_alias = self.getTableAlias(table_name, group=group)
+      table_alias_def = self.makeTableAliasDefinition(table_name, table_alias)
+      # XXX: perhaps refactor some of the code below to do:
+      # self._inner_table_definition.addInnerJoin(TableAlias(...),
+      #                                           condition=(...))
+      self._inner_table_definition.replace(
+        Join(self._inner_table_definition.table_definition,
+                  table_alias_def,
+                  # XXX ColumnMap shouldn't have SQL knowledge
+                  condition=('`%s`.`uid` = `%s`.`uid`' %
+                             (table_alias, catalog_table_alias)),
+                  )
+        )
 
 verifyClass(IColumnMap, ColumnMap)
 

product/ZSQLCatalog/Query/EntireQuery.py

@@ -28,6 +28,7 @@
 #
 ##############################################################################
 
+import warnings
 from Products.ZSQLCatalog.SQLExpression import SQLExpression
 from Products.ZSQLCatalog.ColumnMap import ColumnMap
 from zLOG import LOG
@@ -35,6 +36,7 @@ from Products.ZSQLCatalog.interfaces.entire_query import IEntireQuery
 from zope.interface.verify import verifyClass
 from zope.interface import implements
 from Products.ZSQLCatalog.SQLCatalog import profiler_decorator
+from Products.ZSQLCatalog.TableDefinition import LegacyTableDefinition
 
 def defaultDict(value):
   if value is None:
@@ -54,19 +56,28 @@ class EntireQuery(object):
   column_map = None
 
   @profiler_decorator
-  def __init__(self, query, order_by_list=(), group_by_list=(),
-               select_dict=None, limit=None, catalog_table_name=None,
-               extra_column_list=(), from_expression=None,
-               order_by_override_list=None):
+  def __init__(self, query,
+               order_by_list=(),
+               group_by_list=(),
+               select_dict=None,
+               left_join_list=(),
+               limit=None,
+               catalog_table_name=None,
+               extra_column_list=(),
+               from_expression=None,
+               order_by_override_list=None,
+               implicit_join=False):
     self.query = query
     self.order_by_list = list(order_by_list)
     self.order_by_override_set = frozenset(order_by_override_list)
     self.group_by_list = list(group_by_list)
     self.select_dict = defaultDict(select_dict)
+    self.left_join_list = left_join_list
     self.limit = limit
     self.catalog_table_name = catalog_table_name
     self.extra_column_list = list(extra_column_list)
     self.from_expression = from_expression
+    self.implicit_join = implicit_join
 
   def asSearchTextExpression(self, sql_catalog):
     return self.query.asSearchTextExpression(sql_catalog)
@@ -78,7 +89,12 @@ class EntireQuery(object):
       # XXX: should we provide a way to register column map as a separate 
       # method or do it here ?
       # Column Map was not built yet, do it.
-      self.column_map = column_map = ColumnMap(catalog_table_name=self.catalog_table_name)
+      column_map = ColumnMap(catalog_table_name=self.catalog_table_name,
+                             table_override_map=self.from_expression,
+                             left_join_list=self.left_join_list,
+                             implicit_join=self.implicit_join,
+                            )
+      self.column_map = column_map
       for extra_column in self.extra_column_list:
         table, column = extra_column.replace('`', '').split('.')
         if table != self.catalog_table_name:
@@ -145,30 +161,55 @@ class EntireQuery(object):
             None, ) * (3 - len(order_by)))
       self.order_by_list = new_order_by_list
       # generate SQLExpression from query
-      sql_expression_list = [self.query.asSQLExpression(sql_catalog, column_map, only_group_columns)]
-      # generate join expression based on column_map.getJoinTableAliasList
+      sql_expression_list = [self.query.asSQLExpression(sql_catalog,
+                                                        column_map,
+                                                        only_group_columns)]
       append = sql_expression_list.append
       for join_query in column_map.iterJoinQueryList():
-        append(join_query.asSQLExpression(sql_catalog, column_map, only_group_columns))
-      join_table_list = column_map.getJoinTableAliasList()
-      if len(join_table_list):
-        # XXX: Is there any special rule to observe when joining tables ?
-        # Maybe we could check which column is a primary key instead of
-        # hardcoding "uid".
-        where_pattern = '`%s`.`uid` = `%%s`.`uid`' % \
-          (column_map.getCatalogTableAlias(), )
-        # XXX: It would cleaner from completeness point of view to use column
-        # mapper to render column, but makes code much more complex to just do
-        # a simple text rendering. If there is any reason why we should have
-        # those column in the mapper, then we should use the clean way.
-        append(SQLExpression(self, where_expression=' AND '.join(
-          where_pattern % (x, ) for x in join_table_list
-        )))
+        append(join_query.asSQLExpression(sql_catalog,
+                                          column_map, 
+                                          only_group_columns))
+      # generate join expression based on column_map.getJoinTableAliasList
+      # XXX: This is now done by ColumnMap to its table_definition,
+      # during build()
+      #
+      # join_table_list = column_map.getJoinTableAliasList()
+      # if len(join_table_list):
+      #   # XXX: Is there any special rule to observe when joining tables ?
+      #   # Maybe we could check which column is a primary key instead of
+      #   # hardcoding "uid".
+      #   where_pattern = '`%s`.`uid` = `%%s`.`uid`' % \
+      #     (column_map.getCatalogTableAlias(), )
+      #   # XXX: It would cleaner from completeness point of view to use column
+      #   # mapper to render column, but makes code much more complex to just do
+      #   # a simple text rendering. If there is any reason why we should have
+      #   # those column in the mapper, then we should use the clean way.
+      #   append(SQLExpression(self, where_expression=' AND '.join(
+      #     where_pattern % (x, ) for x in join_table_list
+      #   )))
+
+      # BBB self.from_expression forces use of implicit inner join
+      table_alias_dict = column_map.getTableAliasDict()
+      if self.from_expression:
+        warnings.warn("Providing a 'from_expression' is deprecated.",
+                      DeprecationWarning)
+        # XXX: perhaps move this code to ColumnMap?
+        legacy_from_expression = self.from_expression
+        from_expression = LegacyTableDefinition(legacy_from_expression,
+                                                table_alias_dict)
+        table_alias_dict = None
+      else:
+        from_expression = column_map.getTableDefinition()
+        assert ((from_expression is None) !=
+                (table_alias_dict is None)), ("Got both a from_expression "
+                                              "and a table_alias_dict")
       self.sql_expression_list = sql_expression_list
+      # TODO: wrap the table_alias_dict above into a TableDefinition as well,
+      # even without a legacy_table_definition.
     return SQLExpression(
       self,
-      table_alias_dict=column_map.getTableAliasDict(),
-      from_expression=self.from_expression,
+      table_alias_dict=table_alias_dict,
+      from_expression=from_expression,
       order_by_list=self.order_by_list,
       group_by_list=self.group_by_list,
       select_dict=self.final_select_dict,

product/ZSQLCatalog/SQLCatalog.py

@@ -2291,6 +2291,13 @@ class Catalog(Folder,
         select_dict = None
     elif isinstance(select_dict, (list, tuple)):
       select_dict = dict([(x, None) for x in select_dict])
+    # Handle left_join_list
+    left_join_list = kw.pop('left_join_list', ())
+    # Handle implicit_join. It's True by default, as there's a lot of code
+    # in BT5s and elsewhere that calls buildSQLQuery() expecting implicit
+    # join. self._queryResults() defaults it to False for those using
+    # catalog.searchResults(...) or catalog(...) directly.
+    implicit_join = kw.pop('implicit_join', True)
     # Handle order_by_list
     order_by_list = kw.pop('order_by_list', None)
     sort_on = kw.pop('sort_on', None)
@@ -2328,6 +2335,8 @@ class Catalog(Folder,
       order_by_override_list=order_by_override_list,
       group_by_list=group_by_list,
       select_dict=select_dict,
+      left_join_list=left_join_list,
+      implicit_join=implicit_join,
       limit=limit,
       catalog_table_name=query_table,
       extra_column_list=extra_column_list,
@@ -2413,6 +2422,7 @@ class Catalog(Folder,
     """ Returns a list of brains from a set of constraints on variables """
     if build_sql_query_method is None:
       build_sql_query_method = self.buildSQLQuery
+    kw.setdefault('implicit_join', False)
     query = build_sql_query_method(REQUEST=REQUEST, **kw)
     # XXX: decide if this should be made normal
     ENFORCE_SEPARATION = True

product/ZSQLCatalog/SQLExpression.py

@@ -166,7 +166,7 @@ class SQLExpression(object):
   @profiler_decorator
   def getFromExpression(self):
     """
-      Returns a string.
+      Returns a TableDefinition stored in one of the from_expressions or None
 
       If there are nested SQLExpression, it checks that they either don't
       define any from_expression or the exact same from_expression. Otherwise,
@@ -175,7 +175,7 @@ class SQLExpression(object):
     result = self.from_expression
     for sql_expression in self.sql_expression_list:
       from_expression = sql_expression.getFromExpression()
-      if None not in (result, from_expression):
+      if from_expression not in (result, None):
         message = 'I don\'t know how to merge from_expressions'
         if DEBUG:
           message = message + '. I was created by %r, and I am working on %r (%r) out of [%s]' % (
@@ -385,20 +385,25 @@ class SQLExpression(object):
       SQL_SELECT_ALIAS_FORMAT % (column, alias)
       for alias, column in self.getSelectDict().iteritems())
 
-  @profiler_decorator
-  def asSQLExpressionDict(self):
+  def getFromTableList(self):
     table_alias_dict = self.getTableAliasDict()
+    if not table_alias_dict:
+      return None
     from_table_list = []
     append = from_table_list.append
     for alias, table in table_alias_dict.iteritems():
       append((SQL_TABLE_FORMAT % (alias, ), SQL_TABLE_FORMAT % (table, )))
-    from_expression_dict = self.getFromExpression()
-    if from_expression_dict is not None:
-      from_expression = SQL_LIST_SEPARATOR.join(
-        from_expression_dict.get(alias, '`%s` AS `%s`' % (table, alias))
-        for alias, table in table_alias_dict.iteritems())
-    else:
-      from_expression = None
+    return from_table_list
+
+  @profiler_decorator
+  def asSQLExpressionDict(self):
+    from_expression = self.getFromExpression()
+    from_table_list = self.getFromTableList()
+    assert None in (from_expression,
+                    from_table_list), ("Cannot return both a from_expression "
+                                       "and a from_table_list")
+    if from_expression is not None:
+      from_expression = from_expression.render()
     return {
       'where_expression': self.getWhereExpression(),
       'order_by_expression': self.getOrderByExpression(),

product/ZSQLCatalog/SearchKey/RelatedKey.py

@@ -37,12 +37,29 @@ from Products.ZSQLCatalog.interfaces.search_key import IRelatedKey
 from zope.interface.verify import verifyClass
 from zope.interface import implements
 from Products.ZSQLCatalog.SQLCatalog import profiler_decorator
+from Products.ZSQLCatalog.TableDefinition import TableAlias, InnerJoin, LeftJoin
+
+from logging import getLogger
+log = getLogger(__name__)
 
 BACKWARD_COMPATIBILITY = True
 
+RELATED_QUERY_SEPARATOR = "\nAND -- related query separator\n"
+
+RELATED_KEY_MISMATCH_MESSAGE = "\
+A rendered related key must contain the same number of querying \
+conditions as the tables it relates, properly separated by \
+RELATED_QUERY_SEPARATOR. \n\
+Offending related key: %r, for column %r, table_alias_list: %r, \
+rendered_related_key: \n%s"
+
+RELATED_KEY_ALIASED_MESSAGE = "\
+Support for explicit joins of aliased related keys is not yet implemented. \
+Offending related key: %r, for column %r, table_alias_list: %r"
+
 class RelatedKey(SearchKey):
   """
-    This SearchKey handles searched on virtual columns of RelatedKey type.
+    This SearchKey handles searches on virtual columns of RelatedKey type.
     It generates joins required by the virtual column to reach the actual
     column to compare, plus a regular query on that column if needed.
   """
@@ -115,13 +132,23 @@ class RelatedKey(SearchKey):
   def registerColumnMap(self, column_map, table_alias_list=None):
     related_column = self.getColumn()
     group = column_map.registerRelatedKey(related_column, self.real_column)
-    # Each table except last one must be registered to their own group, so that
-    # the same table can be used multiple time (and aliased multiple times)
-    # in the same related key. The last one must be register to related key
-    # "main" group (ie, the value of the "group" variable) to be the same as
-    # the ta ble used in join_condition.
+    # Each table except last one must be registered to their own
+    # group, so that the same table can be used multiple times (and
+    # aliased multiple times) in the same related key. The last one
+    # must be registered to the related key "main" group (ie, the
+    # value of the "group" variable) to be the same as the table used
+    # in join_condition.
     if table_alias_list is not None:
       assert len(self.table_list) == len(table_alias_list)
+      # XXX-Leo: remove the rest of this 'if' branch after making sure
+      # that ColumnMap.addRelatedKeyJoin() can handle collapsing
+      # chains of inner-joins that are subsets of one another based on
+      # having the same aliases:
+      msg = RELATED_KEY_ALIASED_MESSAGE % (self.related_key_id,
+                                           self.column,
+                                           table_alias_list,)
+      log.warning(msg + "\n\nForcing implicit join...")
+      column_map.implicit_join = True
     for table_position in xrange(len(self.table_list) - 1):
       table_name = self.table_list[table_position]
       local_group = column_map.registerRelatedKeyColumn(related_column, table_position, group)
@@ -145,6 +172,23 @@ class RelatedKey(SearchKey):
     column_map.registerCatalog()
     return group
 
+  def stitchJoinDefinition(self, table_alias_list, join_query_list, column_map):
+    alias, table = table_alias_list[-1]
+    right = column_map.makeTableAliasDefinition(table, alias)
+    if not join_query_list:
+      # nothing to do, just return the table alias
+      assert len(table_alias_list) == 1
+      return right
+    else:
+      # create an InnerJoin of the last element of the alias list with
+      # a chain of InnerJoins of the rest of the list conditioned on
+      # the the last element of the join_query_list
+      left = self.stitchJoinDefinition(table_alias_list[:-1],
+                                       join_query_list[:-1],
+                                       column_map)
+      condition = join_query_list[-1]
+      return InnerJoin(left, right, condition)
+
   @profiler_decorator
   def buildSQLExpression(self, sql_catalog, column_map, only_group_columns, group):
     """
@@ -170,20 +214,26 @@ class RelatedKey(SearchKey):
     table_alias_list = [(getTableAlias(related_table, group=getRelatedKeyGroup(index, group)), related_table)
                         for (index, related_table) in enumerate(related_table_list)]
     # table alias for destination table
-    table_alias_list.append((getTableAlias(destination_table, group=group), destination_table))
+    table_alias_list.append((getTableAlias(destination_table, group=group),
+                             destination_table))
 
     # map aliases to use in ZSQLMethod.
-    table_alias_dict = dict(('table_%s' % (index, ), table_alias[0])
-                            for (index, table_alias) in enumerate(table_alias_list))
+    table_alias_dict = dict(('table_%s' % (index, ), table_alias)
+                            for (index, (table_alias, table_name))
+                            in enumerate(table_alias_list))
 
     assert len(table_alias_list) == len(table_alias_dict)
 
+    query_table=column_map.getCatalogTableAlias()
     rendered_related_key = related_key(
-      query_table=column_map.getCatalogTableAlias(),
+      query_table=query_table,
+      RELATED_QUERY_SEPARATOR=RELATED_QUERY_SEPARATOR,
       src__=1,
       **table_alias_dict)
+    join_condition_list = rendered_related_key.split(RELATED_QUERY_SEPARATOR)
+
     # Important:
-    # Former catalog separated join condition from related query.
+    # Previously the catalog separated join condition from the related query.
     # Example:
     #   ComplexQuery(Query(title="foo"),
     #                Query(subordination_title="bar")
@@ -199,19 +249,53 @@ class RelatedKey(SearchKey):
     # This was done on purpose, because doing otherwise gives very poor
     # performances (on a simple data set, similar query can take *minutes* to
     # execute - as of MySQL 5.x).
-    # Doing the same way as the former catalog is required for backward
-    # compatibility, until a decent alternative is found (like spliting the
-    # "OR" expression into ensemblist operations at query level).
-    # Note that doing this has a side effect on result list, as objects
-    # lacking a relation will never appear in the result.
+    #
+    # Because of this, we never return an SQLExpression here, as it
+    # would mix join definition with column condition in the body of
+    # the WHERE clause. Instead we explicitly define a Join to the
+    # catalog. The ColumnMap defines whether this is an Inner Join or
+    # a Left Outer Join. Notice that if an Inner Join is decided,
+    # objects lacking a relationship will never appear in the result.
+
+    if len(join_condition_list) == len(table_alias_list):
+      # Good! we got a compatible method that splits the join
+      # conditions according to the related tables.
+      #
+      # Add a join on this related key, based on the chain of
+      # inner-joins of the related key tables.
+      query_table_join_condition = join_condition_list.pop()
+      right_side = self.stitchJoinDefinition(table_alias_list,
+                                             join_condition_list,
+                                             column_map)
+      column_map.addRelatedKeyJoin(self.column,
+                                   right_side=right_side,
+                                   condition=query_table_join_condition)
+    else:
+      # Method did not render the related key condition with the
+      # appropriate separators so we could split it
+
+      # XXX: Can we try to parse rendered_related_key to select which
+      # conditions go with each table? Maybe we could still use
+      # explicit joins this way...
+
+      msg = RELATED_KEY_MISMATCH_MESSAGE % (self.related_key_id,
+                                            self.column,
+                                            table_alias_list,
+                                            rendered_related_key)
       if BACKWARD_COMPATIBILITY:
-      # XXX: Calling a private-ish method on column_map.
-      # This should never happen. It should be removed as soon as an
-      # alternative exists.
-      column_map._addJoinQuery(SQLQuery(rendered_related_key))
-      return None
+        # BBB: remove this branch of the condition, and the above
+        # constant, when all zsql_methods have been adapted to return
+        # the join queries properly separated by the
+        # RELATED_QUERY_SEPARATOR.
+
+        # The rendered related key doesn't have the separators for each
+        # joined table, so we revert to doing implicit inner joins:
+        log.warning(msg + "\n\nAdding an Implicit Join Condition...")
+        column_map._addJoinQueryForColumn(self.column,
+                                          SQLQuery(rendered_related_key))
       else:
-      return SQLExpression(self, where_expression=rendered_related_key)
+        raise RuntimeError(msg)
+    return None
 
 verifyClass(IRelatedKey, RelatedKey)
 

product/ZSQLCatalog/TableDefinition.py

+##############################################################################
+#
+# Copyright (c) 2010 Nexedi SA and Contributors. All Rights Reserved.
+#                    Leonardo Rochael Almeida <leonardo@nexedi.com>
+#
+# WARNING: This program as such is intended to be used by professional
+# programmers who take the whole responsability of assessing all potential
+# consequences resulting from its eventual inadequacies and bugs
+# End users who are looking for a ready-to-use solution with commercial
+# garantees and support are strongly adviced to contract a Free Software
+# Service Company
+#
+# This program is Free Software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
+#
+##############################################################################
+
+# TODO:
+# * collapse of parentheses around chains of inner-joins
+# * indentation on rendering
+
+SQL_LIST_SEPARATOR = ', '
+SQL_SELECT_ALIAS_FORMAT = '%s AS `%s`'
+
+from Products.ZSQLCatalog.Query.SQLQuery import SQLQuery
+
+def escapeTable(table):
+  return "`%s`" % table.replace('`', r'\`')
+
+class TableDefinition(object):
+  """Base class for all TableDefinition objects. Used for
+  typechecking (which should become interface checking later) and
+  for dumping common code (if there is any).
+
+  TableDefinition objects describe the table aliasing and joining on
+  the "FROM" expression of an SQL query. It is supposed to be
+  decoded by an SQLExpressionObject into a string.
+  """
+
+  def checkTableAliases(self, current_aliases=None):
+    """Check that aliases defined in this table definition don't try
+    to alias different tables to the same name.
+
+    Add all aliases defined to the current_aliases mapping if it is passed in.
+    """
+    if current_aliases is None:
+      current_aliases = {}
+    self._checkTableAliases(current_aliases)
+
+  def _checkTableAliases(self, current_aliases):
+    raise NotImplementedError('should be implemented by subclasses')
+
+  def render(self):
+    raise NotImplementedError('should be implemented by subclasses')
+
+  def getJoinConditionQueryList(self):
+    """Return a list of SQLQuery objects containing all conditions
+    used in this table definition.
+
+    This is a deprecated method that is here only to accomodate the
+    fact that not all RelatedKey methods have been migrated.
+    """
+    query_list = []
+    self._extendJoinConditionQueryList(query_list)
+    return query_list
+
+  def _extendJoinConditionQueryList(self, query_list):
+    raise NotImplementedError('should be implemented by subclasses')
+
+  def getSuperSet(self, other):
+    """Checks if this TableDefinition is a subset of the other table
+    definition or vice-versa. Return which one is the superset.
+
+    Returns whichever is the superset of the other or None
+    """
+    raise NotImplementedError('should be implemented by subclasses')
+
+class PlaceHolderTableDefinition(TableDefinition):
+  """Table Definition that simply holds an inner table definition and
+  delegates to it the rendering.
+
+  This object can be used when you need to change a table definition
+  in the future, (like replacing a simple table aliasing with an inner
+  join) but don't know who is going to be holding a reference to the
+  original table definition to replace it.
+  """
+
+  def __init__(self, table_definition=None):
+    self.table_definition = table_definition
+
+  replace = __init__
+
+  def __repr__(self):
+    return '<%s for %r>' % (self.__class__.__name__, self.table_definition)
+
+  def _checkTableAliases(self, current_aliases):
+    assert self.table_definition is not None, "table definition wasn't set"
+    return self.table_definition._checkTableAliases(current_aliases)
+
+  def render(self):
+    assert self.table_definition is not None, "table definition wasn't set"
+    return self.table_definition.render()
+
+  def _extendJoinConditionQueryList(self, query_list):
+    # XXX _extendJoinConditionQueryList
+    #assert self.table_definition is not None, "table definition wasn't set"
+    if self.table_definition is not None:
+      return self.table_definition._extendJoinConditionQueryList(query_list)
+
+  def getSuperSet(self, other):
+    assert self.table_definition is not None, "table definition wasn't set"
+    return self.table_definition.getSuperSet(other)
+
+class TableAlias(TableDefinition):
+  """Definition of a table alias as a FROM expression"""
+
+  def __init__(self, table, alias=None):
+    self.table = table
+    self.alias = alias or table
+
+  def _checkTableAliases(self, current_aliases):
+    #table_name = current_aliases.setdefault(self.alias, self.table)
+    table_name = current_aliases.get(self.alias)
+    if table_name is None:
+      current_aliases[self.alias] = self.table
+      return
+    if table_name != self.table:
+      message = ("Attempted to alias both %r and %r to %r" %
+                 (table_name, self.table, self.alias,))
+    else:
+      message = ("Attempted to  alias %r to %r more than once" %
+                 (self.table, self.alias,))
+    raise ValueError(message)
+
+  def render(self):
+    """Render this table definition into an actual FROM expression"""
+    return SQL_SELECT_ALIAS_FORMAT % (self.table, self.alias)
+
+  def __repr__(self):
+    return '<%s %r AS %r>' % (self.__class__.__name__, self.table, self.alias)
+
+  def _extendJoinConditionQueryList(self, query_list):
+    pass
+
+  def __eq__(self, other):
+    return (isinstance(other, TableAlias) and
+            self.table == other.table and 
+            self.alias == other.alias)
+
+  def getSuperSet(self, other):
+    """A TableAlias is a subset of another table Alias if either:
+     - the other is an equivalent TableAlias
+     - the other is an InnerJoin where the left-side is an equivalent TableAlias
+    """
+    if isinstance(other, TableAlias) and self == other:
+      # we're just like the other guy, we could return self or other
+      return self
+    # delegate the rest of the job to InnerJoin
+    return other.getSuperSet(self)
+
+JOIN_FORMAT = """
+  (
+    %(left)s 
+  %(join)s
+    %(right)s
+  ON 
+    %(condition)s
+  )
+""".strip()
+
+class Join(TableDefinition):
+
+  JOIN_TYPE = None
+
+  def __init__(self, left_tabledef, right_tabledef, condition):
+    assert self.JOIN_TYPE, ('Join must be subclassed and self.JOIN_TYPE '
+                            'must be defined.')
+    assert isinstance(left_tabledef, (TableDefinition, None.__class__))
+    assert isinstance(right_tabledef, (TableDefinition, None.__class__))
+    self.left_tabledef = left_tabledef
+    self.right_tabledef = right_tabledef
+    # perhaps assert condition is an SQLExpression?
+    self.condition = condition
+
+  def render(self):
+    """Render the join as an actual FROM expression, delegating
+    the rendering of each table to its own object.
+    """
+    assert None not in (self.left_tabledef, self.right_tabledef, self.condition)
+    return JOIN_FORMAT % dict(left=self.left_tabledef.render(),
+                              right=self.right_tabledef.render(),
+                              join=self.JOIN_TYPE,
+                              condition=self.condition)
+
+  def _checkTableAliases(self, current_aliases):
+    self.left_tabledef._checkTableAliases(current_aliases)
+    self.right_tabledef._checkTableAliases(current_aliases)
+
+  def __repr__(self):
+    return '<%s of %r and %r on %r>' % (self.__class__.__name__,
+                                        self.left_tabledef,
+                                        self.right_tabledef,
+                                        self.condition)
+
+  def _extendJoinConditionQueryList(self, query_list):
+    self.left_tabledef._extendJoinConditionQueryList(query_list)
+    self.right_tabledef._extendJoinConditionQueryList(query_list)
+    query_list.append(SQLQuery(self.condition))
+
+  def getSuperSet(self, other):
+    return None
+
+class InnerJoin(Join):
+  """Definition of an inner-join as a FROM expression"""
+
+  JOIN_TYPE = "INNER JOIN"
+
+  def getSuperSet(self, other):
+    """This InnerJoin is a superset of another TableDefinition if either:
+
+     - other is a TableAlias (or None) equal to our
+       left_side. I.e. "other" is at the end of it's inner-join chain.
+
+     - other is an InnerJoin, and it's left-side is equal to our
+       left-side (both TableAliases or None), and our right-side is a
+       super-set of it's right-side.
+    """
+    if self.left_tabledef == other:
+      # other and left-side are both None or matching TableAliases
+      return self
+    if (isinstance(other, InnerJoin) and
+        self.left_tabledef == other.left_tabledef):
+      # our left-sides match. If one of our right sides is a superset of the
+      # other right side, then we found the superset
+      sub_superset = self.right_tabledef.getSuperSet(other.right_tabledef)
+      if sub_superset is self.right_tabledef:
+        return self
+      elif sub_superset is other.right_tabledef:
+        return other
+      return None
+    return None
+
+class LeftJoin(InnerJoin):
+  """Definition of a left-join as a FROM expression"""
+  
+  JOIN_TYPE = "LEFT JOIN"
+
+  def _extendJoinConditionQueryList(self, query_list):
+    """ The condition from a left-join cannot be meaningfully
+    extracted to be used in an implicit Inner Join, as is done when a
+    query contains a related key that is not formatted to separate the
+    join conditions for each related table."""
+    raise RuntimeError("Attempted to collapse table definition for implicit "
+                       "inner join, but this table definition contains a Left "
+                       "Join: %r" % self)
+
+class LegacyTableDefinition(TableDefinition):
+  """Table Definition used when a from_expression is passed explicitly.
+  Mostly used for manual left-join definitions. Deprecated
+  """
+
+  def __init__(self, from_expression, table_alias_map):
+    self.from_expression = from_expression
+    self.table_alias_map = table_alias_map
+
+  def render(self):
+    from_expression_dict = self.from_expression
+    table_alias_map = self.table_alias_map
+    from_expression = SQL_LIST_SEPARATOR.join(
+      from_expression_dict.get(alias, '`%s` AS `%s`' % (table, alias))
+      for alias, table in table_alias_map.iteritems())
+    return from_expression

product/ZSQLCatalog/interfaces/column_map.py

@@ -284,18 +284,3 @@ class IColumnMap(Interface):
       Return a copy of the table alias list for tables requiring a join with
       catalog table.
     """
-
-  def getStraightJoinTableList():
-    """
-      Returns the list of tables used this search and which
-      need to be joined with the main table using explicit
-      indices.
-    """
-
-  def getLeftJoinTableList():
-    """
-      Returns the list of tables used this search and which
-      need to be LEFT joined with the main table using explicit
-      indices.
-    """
-

product/ZSQLCatalog/tests/testSQLCatalog.py

@@ -202,6 +202,7 @@ class DummyCatalog(SQLCatalog):
     assert 'query_table' in kw
     assert 'table_0' in kw
     assert 'table_1' in kw
+    assert 'AND' in kw.pop('RELATED_QUERY_SEPARATOR')
     assert len(kw) == 4
     return '%(table_0)s.uid = %(query_table)s.uid AND %(table_0)s.other_uid = %(table_1)s' % kw
 
@@ -629,7 +630,7 @@ class TestSQLCatalog(unittest.TestCase):
     select_dict = sql_expression.getSelectDict()
     self.assertTrue('ambiguous_mapping' in select_dict, select_dict)
     self.assertTrue('bar' in select_dict['ambiguous_mapping'], select_dict['ambiguous_mapping'])
-    # Doted alias: table name must get stripped. This is required to have an
+    # Dotted alias: table name must get stripped. This is required to have an
     # upgrade path from old ZSQLCatalog versions where pre-mapped columns were
     # used in their select_expression. This must only happen in the
     # "{column: None}" form, as otherwise it's the user explicitely asking for