Repository: dedupeio/dedupe

Issue:
CompoundPredicate should escape `:` to avoid wrong block_keys
I've found an issue on `CompoundPredicate` caused by the lack of escaping `:` chars inside fields.

This causes two problems. First, it can generate duplicate `block_key`s for the same `id`. Here's the example:

```
(SimplePredicate: (commonSetElementPredicate, name), SimplePredicate: (commonSetElementPredicate, address))
```

```
id  | name            | address    
---------------------------------------
1   | {'foo:', 'foo'} | {':bar', 'bar'}
```

```
id | block_key
---------------
1  | foo::bar:0
1  | foo::bar:0
```

That's probably not problematic for Dedupe 2, but that breaks [this line](https://github.com/dedupeio/dedupe-examples/blob/83dbf872674a5c3f6a209c367f46c7e6ef78e9a3/pgsql_big_dedupe_example/pgsql_big_dedupe_example.py#L246-L247) of the old `smaller_id`-based big Dedupe I still use.

Second, it can group together under the same `block_key` records that shouldn't be blocked together. See:

```
(SimplePredicate: (wholeFieldPredicate, name), SimplePredicate: (wholeFieldPredicate, address))
```

```
id  | name    | address    
---------------------------
1   | flavio: | one street
2   | flavio  | :one street
```

```
id | blocking_key
-------------------------
1  | flavio::one street:0
2  | flavio::one street:0
```

The solution is to escape `:` right before performing the `join` at this line:

https://github.com/dedupeio/dedupe/blob/5092f2598f1f2bcb4f3b1f0ec85f88b95ce7454b/dedupe/predicates.py#L332

Something like `u':'.join(block_key.replace(u':', u'\\:'))` would work, because the examples above would become this:

```
id | block_key
---------------
1  | foo\::bar:0
1  | foo:\:bar:0
```

```
id | blocking_key
-------------------------
1  | flavio\::one street:0
2  | flavio:\:one street:0
```

I'll open a PR for this.

Discussion:

Patch:
diff --git a/dedupe/predicates.py b/dedupe/predicates.py
index 1934811..e796a6d 100644
--- a/dedupe/predicates.py
+++ b/dedupe/predicates.py
@@ -329,9 +329,14 @@ class CompoundPredicate(tuple):
     def __call__(self, record, **kwargs):
         predicate_keys = [predicate(record, **kwargs)
                           for predicate in self]
-        return [u':'.join(block_key)
-                for block_key
-                in itertools.product(*predicate_keys)]
+        return [
+            u':'.join(
+                # must escape : to avoid confusion with : join separator
+                b.replace(u':', u'\\:') for b in block_key
+            )
+            for block_key
+            in itertools.product(*predicate_keys)
+        ]
 
 
 def wholeFieldPredicate(field: Any) -> Tuple[str]:


Test patch:
diff --git a/tests/test_predicates.py b/tests/test_predicates.py
index deca290..714ac31 100644
--- a/tests/test_predicates.py
+++ b/tests/test_predicates.py
@@ -80,5 +80,47 @@ class TestNumericPredicates(unittest.TestCase):
         assert predicates.roundTo1(-22315) == (u'-20000',)
 
 
+class TestCompoundPredicate(unittest.TestCase):
+    def test_escapes_colon(self):
+        '''
+        Regression test for issue #836
+        '''
+        predicate_1 = predicates.SimplePredicate(
+            predicates.commonSetElementPredicate, 'col_1')
+        predicate_2 = predicates.SimplePredicate(
+            predicates.commonSetElementPredicate, 'col_2')
+        record = {
+            'col_1': ['foo:', 'foo'],
+            'col_2': [':bar', 'bar']
+        }
+
+        block_val = predicates.CompoundPredicate([
+            predicate_1,
+            predicate_2
+        ])(record)
+        assert len(set(block_val)) == 4
+        assert block_val == ['foo\\::\\:bar', 'foo\\::bar', 'foo:\\:bar', 'foo:bar']
+
+    def test_escapes_escaped_colon(self):
+        '''
+        Regression test for issue #836
+        '''
+        predicate_1 = predicates.SimplePredicate(
+            predicates.commonSetElementPredicate, 'col_1')
+        predicate_2 = predicates.SimplePredicate(
+            predicates.commonSetElementPredicate, 'col_2')
+        record = {
+            'col_1': ['foo\\:', 'foo'],
+            'col_2': ['\\:bar', 'bar']
+        }
+
+        block_val = predicates.CompoundPredicate([
+            predicate_1,
+            predicate_2
+        ])(record)
+        assert len(set(block_val)) == 4
+        assert block_val == ['foo\\\\::\\\\:bar', 'foo\\\\::bar', 'foo:\\\\:bar', 'foo:bar']
+
+
 if __name__ == '__main__':
     unittest.main()