Merge pull request #10 from guzman-raphael/delete-parsing

Fix error when cascading delete on tables with many, complex keys

Author: dimitri-yatsenko

.gitignore

@@ -24,4 +24,5 @@ notebook
 .vscode
 __main__.py
 jupyter_custom.js
-apk_requirements.txt
+apk_requirements.txt
+.eggs

CHANGELOG.md

@@ -1,6 +1,6 @@
 ## Release notes
 
-### 0.13.0 -- Mar 19, 2021
+### 0.13.0 -- Mar 24, 2021
 * Re-implement query transpilation into SQL, fixing issues (#386, #449, #450, #484). PR #754
 * Re-implement cascading deletes for better performance. PR #839.
 * Add table method `.update1` to update a row in the table with new values PR #763
@@ -11,6 +11,7 @@
 * Default enable_python_native_blobs to True
 * Bugfix - Regression error on joins with same attribute name (#857) PR #878
 * Bugfix - Error when `fetch1('KEY')` when `dj.config['fetch_format']='frame'` set (#876) PR #880, #878
+* Bugfix - Error when cascading deletes in tables with many, complex keys (#883, #886) PR #839
 * Drop support for Python 3.5
 
 ### 0.12.8 -- Jan 12, 2021

LNX-docker-compose.yml

@@ -32,7 +32,7 @@ services:
       interval: 1s
   fakeservices.datajoint.io:
     <<: *net
-    image: datajoint/nginx:v0.0.15
+    image: datajoint/nginx:v0.0.16
     environment:
     - ADD_db_TYPE=DATABASE
     - ADD_db_ENDPOINT=db:3306

datajoint/table.py

@@ -15,16 +15,27 @@
 from . import blob
 from .utils import user_choice
 from .heading import Heading
-from .errors import DuplicateError, AccessError, DataJointError, UnknownAttributeError, IntegrityError
+from .errors import (DuplicateError, AccessError, DataJointError, UnknownAttributeError,
+                     IntegrityError)
 from .version import __version__ as version
 
 logger = logging.getLogger(__name__)
 
-foregn_key_error_regexp = re.compile(
+foreign_key_error_regexp = re.compile(
     r"[\w\s:]*\((?P<child>`[^`]+`.`[^`]+`), "
     r"CONSTRAINT (?P<name>`[^`]+`) "
-    r"FOREIGN KEY \((?P<fk_attrs>[^)]+)\) "
-    r"REFERENCES (?P<parent>`[^`]+`(\.`[^`]+`)?) \((?P<pk_attrs>[^)]+)\)")
+    r"(FOREIGN KEY \((?P<fk_attrs>[^)]+)\) "
+    r"REFERENCES (?P<parent>`[^`]+`(\.`[^`]+`)?) \((?P<pk_attrs>[^)]+)\)[\s\w]+\))?")
+
+constraint_info_query = ' '.join("""
+    SELECT
+        COLUMN_NAME as fk_attrs,
+        CONCAT('`', REFERENCED_TABLE_SCHEMA, '`.`', REFERENCED_TABLE_NAME, '`') as parent,
+        REFERENCED_COLUMN_NAME as pk_attrs
+    FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE
+    WHERE
+        CONSTRAINT_NAME = %s AND TABLE_SCHEMA = %s AND TABLE_NAME = %s;
+    """.split())
 
 
 class _RenameMap(tuple):
@@ -344,23 +355,31 @@ def _delete_cascade(self):
             try:
                 delete_count += self.delete_quick(get_count=True)
             except IntegrityError as error:
-                match = foregn_key_error_regexp.match(error.args[0])
-                assert match is not None, "foreign key parsing error"
+                match = foreign_key_error_regexp.match(error.args[0]).groupdict()
+                if "`.`" not in match['child']:  # if schema name missing, use self
+                    match['child'] = '{}.{}'.format(self.full_table_name.split(".")[0],
+                                                    match['child'])
+                if match['pk_attrs'] is not None:  # fully matched, adjusting the keys
+                    match['fk_attrs'] = [k.strip('`') for k in match['fk_attrs'].split(',')]
+                    match['pk_attrs'] = [k.strip('`') for k in match['pk_attrs'].split(',')]
+                else:  # only partially matched, querying with constraint to determine keys
+                    match['fk_attrs'], match['parent'], match['pk_attrs'] = list(map(
+                        list, zip(*self.connection.query(constraint_info_query, args=(
+                            match['name'].strip('`'),
+                            *[_.strip('`') for _ in match['child'].split('`.`')]
+                            )).fetchall())))
+                    match['parent'] = match['parent'][0]
                 # restrict child by self if
                 # 1. if self's restriction attributes are not in child's primary key
                 # 2. if child renames any attributes
                 # otherwise restrict child by self's restriction.
-                child = match.group('child')
-                if "`.`" not in child:  # if schema name is not included, take it from self
-                    child = self.full_table_name.split("`.")[0] + child
-                child = FreeTable(self.connection, child)
+                child = FreeTable(self.connection, match['child'])
                 if set(self.restriction_attributes) <= set(child.primary_key) and \
-                        match.group('fk_attrs') == match.group('pk_attrs'):
+                        match['fk_attrs'] == match['pk_attrs']:
                     child._restriction = self._restriction
-                elif match.group('fk_attrs') != match.group('pk_attrs'):
-                    fk_attrs = [k.strip('`') for k in match.group('fk_attrs').split(',')]
-                    pk_attrs = [k.strip('`') for k in match.group('pk_attrs').split(',')]
-                    child &= self.proj(**dict(zip(fk_attrs, pk_attrs)))
+                elif match['fk_attrs'] != match['pk_attrs']:
+                    child &= self.proj(**dict(zip(match['fk_attrs'],
+                                                  match['pk_attrs'])))
                 else:
                     child &= self.proj()
                 delete_count += child._delete_cascade()
@@ -375,8 +394,10 @@ def _delete_cascade(self):
     def delete(self, transaction=True, safemode=None):
         """
         Deletes the contents of the table and its dependent tables, recursively.
+
         :param transaction: if True, use the entire delete becomes an atomic transaction.
-        :param safemode: If True, prohibit nested transactions and prompt to confirm. Default is dj.config['safemode'].
+        :param safemode: If True, prohibit nested transactions and prompt to confirm. Default
+            is dj.config['safemode'].
         """
         safemode = config['safemode'] if safemode is None else safemode
 

datajoint/version.py

@@ -1,3 +1,3 @@
-__version__ = "0.13.dev6"
+__version__ = "0.13.dev7"
 
 assert len(__version__) <= 10  # The log table limits version to the 10 characters

docs-parts/intro/Releases_lang1.rst

@@ -1,4 +1,4 @@
-0.13.0 -- Mar 19, 2021
+0.13.0 -- Mar 24, 2021
 ----------------------
 * Re-implement query transpilation into SQL, fixing issues (#386, #449, #450, #484). PR #754
 * Re-implement cascading deletes for better performance. PR #839.
@@ -10,6 +10,7 @@
 * Default enable_python_native_blobs to True
 * Bugfix - Regression error on joins with same attribute name (#857) PR #878
 * Bugfix - Error when `fetch1('KEY')` when `dj.config['fetch_format']='frame'` set (#876) PR #880, #878
+* Bugfix - Error when cascading deletes in tables with many, complex keys (#883, #886) PR #839
 * Drop support for Python 3.5
 
 0.12.8 -- Jan 12, 2021

local-docker-compose.yml

@@ -34,7 +34,7 @@ services:
       interval: 1s
   fakeservices.datajoint.io:
     <<: *net
-    image: datajoint/nginx:v0.0.15
+    image: datajoint/nginx:v0.0.16
     environment:
     - ADD_db_TYPE=DATABASE
     - ADD_db_ENDPOINT=db:3306

tests/schema.py

@@ -366,3 +366,16 @@ class Child(dj.Lookup):
     name: varchar(30)
     """
     contents = [(1, 12, 'Dan')]
+
+# Related to issue #886 (8), #883 (5)
+@schema
+class ComplexParent(dj.Lookup):
+    definition = '\n'.join(['parent_id_{}: int'.format(i+1) for i in range(8)])
+    contents = [tuple(i for i in range(8))]
+
+
+@schema
+class ComplexChild(dj.Lookup):
+    definition = '\n'.join(['-> ComplexParent'] + ['child_id_{}: int'.format(i+1)
+                                                   for i in range(1)])
+    contents = [tuple(i for i in range(9))]

tests/test_cascading_delete.py

@@ -1,6 +1,7 @@
 from nose.tools import assert_false, assert_true, assert_equal
 import datajoint as dj
 from .schema_simple import A, B, D, E, L
+from .schema import ComplexChild, ComplexParent
 
 
 class TestDelete:
@@ -78,3 +79,20 @@ def test_delete_lookup_restricted():
         deleted_count = len(rel)
         rel.delete()
         assert_true(len(L()) == original_count - deleted_count)
+
+    @staticmethod
+    def test_delete_complex_keys():
+        # https://github.com/datajoint/datajoint-python/issues/883
+        # https://github.com/datajoint/datajoint-python/issues/886
+        assert_false(dj.config['safemode'], 'safemode must be off for testing')
+        parent_key_count = 8
+        child_key_count = 1
+        restriction = dict({'parent_id_{}'.format(i+1): i
+                            for i in range(parent_key_count)},
+                           **{'child_id_{}'.format(i+1): (i + parent_key_count)
+                              for i in range(child_key_count)})
+        assert len(ComplexParent & restriction) == 1, 'Parent record missing'
+        assert len(ComplexChild & restriction) == 1, 'Child record missing'
+        (ComplexParent & restriction).delete()
+        assert len(ComplexParent & restriction) == 0, 'Parent record was not deleted'
+        assert len(ComplexChild & restriction) == 0, 'Child record was not deleted'