On 09/23/2013 09:18 AM, Jan Cholasta wrote:
On 18.9.2013 14:00, Petr Viktorin wrote:
On 09/17/2013 05:13 PM, Jan Cholasta wrote:
On 20.2.2013 17:37, Petr Viktorin wrote:
On 02/19/2013 01:51 PM, Jan Cholasta wrote:

On 5.2.2013 18:02, Petr Viktorin wrote:
CIDict, our case-insensitive dictionary, inherits from dict but did
reimplement the full dict interface. Calling the missing methods
silently invoked case-sensitive behavior. Our code seems to avoid
but it's a bit of a minefield for new development.

Patch 119 adds the missing dict methods (except
view{items,keys,values}(), which now raise errors), and adds tests.

Can you please also add the (obj, **kwargs) and (**kwargs) variants of
__init__ and update?

Added, thanks for the catch.

Patches 117-118 modernize the testsuite a bit (these have been
in my queue for a while, I think now is a good time to submit them):
The first one moves some old tests from the main code tree to tests/.
(The adtrust_install test wasn't run before, this move makes nose
The second converts CIDict's unittest-based suite to nose.


Whoa, I totally forgot about these patches!

Can you please rebase them?


One more comment:

   Document that CIDict.copy() returns a plain dict.

Why does it return a plain dict? I think it should return a CIDict,
otherwise it is not actually a copy, right?

I wanted to keep the existing (intended) functionality.
But since we don't actually use CIDict.copy() anywhere any more, I've
made the change.


P.S. I recently came across a bug in python-ldap where something like
CIDict({'cn': ['name1', 'name2'], 'cN': ['name3']}) will throw away some
of the values.
This is expected at the CIDict level, but if you're working with dicts
of lists it's something to keep an eye out for.

This is a good point. IIRC when you use such a dict in python-ldap, it
will fail with TYPE_OR_VALUE_EXISTS. I think we should raise an error in
CIDict as well if such a dict is used in __init__() and update(), as
this kind of error is very hard to track otherwise.


Right. Here's a patch that does that.


From 63039a1e28302b2801d2a4722a9e6ee0fe794cc0 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Mon, 23 Sep 2013 10:46:01 +0200
Subject: [PATCH] Raise an error when updating CIDict with duplicate keys

Updating a CIDict with data like {'A': 1, 'a': 2} would lead to data
loss since only one of the items would get to the CIDict.
This can result in non-obvious bugs similar to this one in python-ldap:

Raise an error in this case; any resolution must be done by the caller.
 ipapython/ipautil.py                    | 25 ++++++++++++++++++++++---
 ipatests/test_ipapython/test_ipautil.py | 12 ++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index c5b47f5b44e00014a9627d875b7eeb941d13eafd..13d242792273321cd9efc7c25b4b02b097b0c5ca 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -455,27 +455,46 @@ def __init__(self, default=None, **kwargs):
     def __getitem__(self, key):
         return super(CIDict, self).__getitem__(key.lower())
-    def __setitem__(self, key, value):
+    def __setitem__(self, key, value, seen_keys=None):
+        """cidict[key] = value
+        The ``seen_keys`` argument is used by ``update()`` to keep track of
+        duplicate keys. It should be an initially empty set that is
+        passed to all calls to __setitem__ that should not set duplicate keys.
+        """
         lower_key = key.lower()
+        if seen_keys is not None:
+            if lower_key in seen_keys:
+                raise ValueError('Duplicate key in update: %s' % key)
+            seen_keys.add(lower_key)
         self._keys[lower_key] = key
         return super(CIDict, self).__setitem__(lower_key, value)
     def __delitem__(self, key):
         lower_key = key.lower()
         del self._keys[lower_key]
         return super(CIDict, self).__delitem__(lower_key)
     def update(self, new=None, **kwargs):
+        """Update self from dict/iterable new and kwargs
+        Functions like ``dict.update()``.
+        Neither ``new`` nor ``kwargs`` may contain two keys that only differ in
+        case, as this situation would result in loss of data.
+        """
+        seen = set()
         if new:
                 keys = new.keys
             except AttributeError:
                 for key in keys():
-                    self[key] = new[key]
+                    self.__setitem__(key, new[key], seen)
+        seen = set()
         for key, value in kwargs.iteritems():
-            self[key] = value
+            self.__setitem__(key, value, seen)
     def __contains__(self, key):
         return super(CIDict, self).__contains__(key.lower())
diff --git a/ipatests/test_ipapython/test_ipautil.py b/ipatests/test_ipapython/test_ipautil.py
index a8a73edcdd105c8e7eb36ebe872346a5f443cceb..04a43990ea99564319560de1b59d28d1e4a5bdb6 100644
--- a/ipatests/test_ipapython/test_ipautil.py
+++ b/ipatests/test_ipapython/test_ipautil.py
@@ -243,6 +243,18 @@ def test_update_list_and_kwargs(self):
             'a': 'va', 'b': 'vb',
             'Key1': 'val1', 'key2': 'val2', 'KEY3': 'VAL3'}
+    def test_update_duplicate_values_dict(self):
+        with nose.tools.assert_raises(ValueError):
+            self.cidict.update({'a': 'va', 'A': None, 'b': 3})
+    def test_update_duplicate_values_list(self):
+        with nose.tools.assert_raises(ValueError):
+            self.cidict.update([('a', 'va'), ('A', None), ('b', 3)])
+    def test_update_duplicate_values_kwargs(self):
+        with nose.tools.assert_raises(ValueError):
+            self.cidict.update(a='va', A=None, b=3)
     def test_update_kwargs(self):
         self.cidict.update(b='vb', key2='val2')
         assert dict(self.cidict.items()) == {

Freeipa-devel mailing list

Reply via email to