On 11.12.2013 18:28, Petr Viktorin wrote:
On 12/11/2013 05:05 PM, Petr Viktorin wrote:
On 12/10/2013 02:10 PM, Jan Cholasta wrote:
Hi,

the attached patches fix <https://fedorahosted.org/freeipa/ticket/3488>.

Honza

These look great, thanks! Just a couple of questions/nicpicks.

213: ACK
214: ACK
215: ACK
216: ACK
217: ACK
218: ACK

219:
Does the new method guarantee 'attributetypes' are updated before
'objectclasses'?
I can move the logic to schemaupdate if needed.

That won't be necessary, fixed.


Why is OnlyOneValueAllowed not raised any more? It should be, since the
method assumes get_single_value() gives correct information.

Fixed.


Why is MOD_REPLACE no longer used when no old values are retained? Or
(MOD_DELETE, a, None) when the new set is empty?

Fixed.

However, I am not convinced if this guessing is the right approach. I think we can do better, by either not guessing:

    del entry[attr] -> delete attribute
    entry[attr] = value -> replace attribute
    entry[attr][index] = value -> delete old value, add new value

or by guessing better, to minimize the size of the modify request.



220: ACK

221:
An ancient comment in ldapupdate still has a reference to orig_data in,
can you remove the comment as well?

Sure.


222: ACK
223: ACK
224: ACK

I spoke too soon, NACK. This line:

 > -from ipaserver.plugins import ldap2

is important, please put it back. Without it api.Backend.ldap2 will not
be available and upgrades will fail with AttributeError.

(Yes, I know.)

Fixed. (But it's retarded.)



I noticed that IPASimpleLdapObject.convert_result's docstring is
obsolete, could you update/shorten it?

Sure.

Also fixed some more issues I noticed.

Updated patches attached.

--
Jan Cholasta
>From 3065f010e0342a9f4f122a7b4331251002f96aed Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:09:56 +0100
Subject: [PATCH 01/12] Rename LDAPEntry method commit to reset_modlist.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipapython/ipaldap.py            | 2 +-
 ipaserver/install/ldapupdate.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index a48879f..699a53d 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -740,7 +740,7 @@ class LDAPEntry(collections.MutableMapping):
 
         return result
 
-    def commit(self):
+    def reset_modlist(self):
         """
         Make the current state of the entry a new reference point for change
         tracking.
diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index 87bd0c8..0c44a85 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -281,7 +281,7 @@ class LDAPUpdate:
 
     def _entry_to_entity(self, ent):
         entry = ent.copy()
-        entry.commit()
+        entry.reset_modlist()
         return entry
 
     def _combine_updates(self, all_updates, update):
-- 
1.8.4.2

>From 2e3467315dcf1c33b605558713d09ca407345b68 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:13:48 +0100
Subject: [PATCH 02/12] Use old entry state in LDAPClient.update_entry.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipapython/ipaldap.py | 48 +++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 699a53d..bdaca7d 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -412,14 +412,7 @@ class IPASimpleLDAPObject(object):
         associated with the entry. The keys of attrs are strings, and
         the associated values are lists of strings.
 
-        We convert the dn to a DN object.
-
-        We convert every value associated with an attribute according
-        to it's syntax into the desired Python type.
-
-        returns a IPA result tuple of the same form as a python-ldap
-        result tuple except everything inside of the result tuple has
-        been converted to it's preferred IPA python type.
+        We convert the tuple to an LDAPEntry object.
         '''
 
         ipa_result = []
@@ -440,6 +433,7 @@ class IPASimpleLDAPObject(object):
 
             for attr, original_values in original_attrs.items():
                 ipa_entry.raw[attr] = original_values
+            ipa_entry.reset_modlist()
 
             ipa_result.append(ipa_entry)
 
@@ -1530,16 +1524,6 @@ class LDAPClient(object):
             raise errors.LimitsExceeded()
         return entries[0]
 
-    def _get_dn_and_attrs(self, entry_or_dn, entry_attrs):
-        """Helper for legacy calling style for {add,update}_entry
-        """
-        if entry_attrs is None:
-            return entry_or_dn.dn, entry_or_dn
-        else:
-            assert isinstance(entry_or_dn, DN)
-            entry_attrs = self.make_entry(entry_or_dn, entry_attrs)
-            return entry_or_dn, entry_attrs
-
     def add_entry(self, entry, entry_attrs=None):
         """Create a new entry.
 
@@ -1548,13 +1532,17 @@ class LDAPClient(object):
         The legacy two-argument variant is:
             add_entry(dn, entry_attrs)
         """
-        dn, attrs = self._get_dn_and_attrs(entry, entry_attrs)
+        if entry_attrs is not None:
+            entry = self.make_entry(entry, entry_attrs)
 
         # remove all [] values (python-ldap hates 'em)
-        attrs = dict((k, v) for k, v in attrs.raw.iteritems() if v)
+        attrs = dict((k, v) for k, v in entry.raw.iteritems() if v)
 
         with self.error_handler():
-            self.conn.add_s(dn, attrs.items())
+            self.conn.add_s(entry.dn, attrs.items())
+
+        if entry_attrs is None:
+            entry.reset_modlist()
 
     def update_entry_rdn(self, dn, new_rdn, del_old=True):
         """
@@ -1576,20 +1564,17 @@ class LDAPClient(object):
     def _generate_modlist(self, dn, entry_attrs):
         assert isinstance(dn, DN)
 
-        # get original entry
-        entry_attrs_old = self.get_entry(dn, entry_attrs.keys())
-
         # generate modlist
         # for multi value attributes: no MOD_REPLACE to handle simultaneous
         # updates better
         # for single value attribute: always MOD_REPLACE
         modlist = []
         for (k, v) in entry_attrs.raw.iteritems():
-            if not v and k in entry_attrs_old:
+            if not v and k in entry_attrs.orig_data:
                 modlist.append((ldap.MOD_DELETE, k, None))
             else:
                 v = set(v)
-                old_v = set(entry_attrs_old.raw.get(k, []))
+                old_v = set(entry_attrs.orig_data.raw.get(k, []))
 
                 adds = list(v.difference(old_v))
                 rems = list(old_v.difference(v))
@@ -1629,16 +1614,21 @@ class LDAPClient(object):
         The legacy two-argument variant is:
             update_entry(dn, entry_attrs)
         """
-        dn, attrs = self._get_dn_and_attrs(entry, entry_attrs)
+        if entry_attrs is not None:
+            entry = self.get_entry(entry, entry_attrs.keys())
+            entry.update(entry_attrs)
 
         # generate modlist
-        modlist = self._generate_modlist(dn, attrs)
+        modlist = self._generate_modlist(entry.dn, entry)
         if not modlist:
             raise errors.EmptyModlist()
 
         # pass arguments to python-ldap
         with self.error_handler():
-            self.conn.modify_s(dn, modlist)
+            self.conn.modify_s(entry.dn, modlist)
+
+        if entry_attrs is None:
+            entry.reset_modlist()
 
     def delete_entry(self, entry_or_dn):
         """Delete an entry given either the DN or the entry itself"""
-- 
1.8.4.2

>From 3997239b3ebe264e4dd1d2a841d94c177e1986fb Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:38:12 +0100
Subject: [PATCH 03/12] Move LDAPClient method get_single_value to
 IPASimpleLDAPObject.

Refactor IPASimpleLDAPObject methods get_syntax and get_single_value.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipapython/ipaldap.py | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index bdaca7d..8416552 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -323,21 +323,23 @@ class IPASimpleLDAPObject(object):
         self._schema = None
 
     def get_syntax(self, attr):
+        if isinstance(attr, unicode):
+            attr = attr.encode('utf-8')
+
         # Is this a special case attribute?
-        syntax = self._SCHEMA_OVERRIDE.get(attr)
-        if syntax is not None:
-            return syntax
+        if attr in self._SCHEMA_OVERRIDE:
+            return self._SCHEMA_OVERRIDE[attr]
 
         if self.schema is None:
             return None
 
         # Try to lookup the syntax in the schema returned by the server
         obj = self.schema.get_obj(ldap.schema.AttributeType, attr)
-        if obj is not None:
-            return obj.syntax
-        else:
+        if obj is None:
             return None
 
+        return obj.syntax
+
     def has_dn_syntax(self, attr):
         """
         Check the schema to see if the attribute uses DN syntax.
@@ -347,6 +349,27 @@ class IPASimpleLDAPObject(object):
         syntax = self.get_syntax(attr)
         return syntax == DN_SYNTAX_OID
 
+    def get_single_value(self, attr):
+        """
+        Check the schema to see if the attribute is single-valued.
+
+        If the attribute is in the schema then returns True/False
+
+        If there is a problem loading the schema or the attribute is
+        not in the schema return None
+        """
+        if isinstance(attr, unicode):
+            attr = attr.encode('utf-8')
+
+        if self.schema is None:
+            return None
+
+        obj = self.schema.get_obj(ldap.schema.AttributeType, attr)
+        if obj is None:
+            return None
+
+        return obj.single_value
+
 
     def encode(self, val):
         """
@@ -1182,18 +1205,7 @@ class LDAPClient(object):
         return [unicode(a).lower() for a in list(set(allowed_attributes))]
 
     def get_single_value(self, attr):
-        """
-        Check the schema to see if the attribute is single-valued.
-
-        If the attribute is in the schema then returns True/False
-
-        If there is a problem loading the schema or the attribute is
-        not in the schema return None
-        """
-        if self.schema is None:
-            return None
-        obj = self.schema.get_obj(ldap.schema.AttributeType, attr)
-        return obj and obj.single_value
+        return self.conn.get_single_value(attr)
 
     def make_dn_from_attr(self, attr, value, parent_dn=None):
         """
-- 
1.8.4.2

>From a1132686387594f279b4cd504849f6083e408541 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:39:38 +0100
Subject: [PATCH 04/12] Make IPASimpleLDAPObject.get_single_value result
 overridable.

Add some default overrides.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipapython/ipaldap.py | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 8416552..6a167db 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -246,11 +246,18 @@ class IPASimpleLDAPObject(object):
     #
     # FWIW, many entries under cn=config are undefined :-(
 
-    _SCHEMA_OVERRIDE = CIDict({
+    _SYNTAX_OVERRIDE = CIDict({
         'managedtemplate': DN_SYNTAX_OID, # DN
         'managedbase':     DN_SYNTAX_OID, # DN
         'originscope':     DN_SYNTAX_OID, # DN
     })
+    _SINGLE_VALUE_OVERRIDE = CIDict({
+        'nsslapd-ssl-check-hostname': True,
+        'nsslapd-lookthroughlimit': True,
+        'nsslapd-idlistscanlimit': True,
+        'nsslapd-anonlimitsdn': True,
+        'nsslapd-minssf-exclude-rootdse': True,
+    })
 
     def __init__(self, uri, force_schema_updates, no_schema=False,
                  decode_attrs=True):
@@ -327,8 +334,8 @@ class IPASimpleLDAPObject(object):
             attr = attr.encode('utf-8')
 
         # Is this a special case attribute?
-        if attr in self._SCHEMA_OVERRIDE:
-            return self._SCHEMA_OVERRIDE[attr]
+        if attr in self._SYNTAX_OVERRIDE:
+            return self._SYNTAX_OVERRIDE[attr]
 
         if self.schema is None:
             return None
@@ -361,6 +368,9 @@ class IPASimpleLDAPObject(object):
         if isinstance(attr, unicode):
             attr = attr.encode('utf-8')
 
+        if attr in self._SINGLE_VALUE_OVERRIDE:
+            return self._SINGLE_VALUE_OVERRIDE[attr]
+
         if self.schema is None:
             return None
 
-- 
1.8.4.2

>From db2a8351156127d1eaafb695b482c7487a7ffa75 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:41:17 +0100
Subject: [PATCH 05/12] Use LDAPClient.update_entry for LDAP mods in
 ldapupdate.

Remove legacy IPAdmin methods generateModList and updateEntry.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipapython/ipaldap.py            | 60 -----------------------------------------
 ipaserver/install/ldapupdate.py |  4 +--
 2 files changed, 2 insertions(+), 62 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 6a167db..a269581 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -1771,66 +1771,6 @@ class IPAdmin(LDAPClient):
         self.__bind_with_wait(
             self.conn.sasl_interactive_bind_s, timeout, None, auth_tokens)
 
-    def updateEntry(self,dn,oldentry,newentry):
-        # FIXME: for backwards compatibility only
-        """This wraps the mod function. It assumes that the entry is already
-           populated with all of the desired objectclasses and attributes"""
-
-        assert isinstance(dn, DN)
-
-        modlist = self.generateModList(oldentry, newentry)
-
-        if len(modlist) == 0:
-            raise errors.EmptyModlist
-
-        with self.error_handler():
-            self.modify_s(dn, modlist)
-        return True
-
-    def generateModList(self, old_entry, new_entry):
-        # FIXME: for backwards compatibility only
-        """A mod list generator that computes more precise modification lists
-           than the python-ldap version.  For single-value attributes always
-           use a REPLACE operation, otherwise use ADD/DEL.
-        """
-
-        # Some attributes, like those in cn=config, need to be replaced
-        # not deleted/added.
-        FORCE_REPLACE_ON_UPDATE_ATTRS = ('nsslapd-ssl-check-hostname', 'nsslapd-lookthroughlimit', 'nsslapd-idlistscanlimit', 'nsslapd-anonlimitsdn', 'nsslapd-minssf-exclude-rootdse')
-        modlist = []
-
-        keys = set(old_entry.keys())
-        keys.update(new_entry.keys())
-
-        for key in keys:
-            new_values = new_entry.raw.get(key, [])
-            old_values = old_entry.raw.get(key, [])
-
-            # We used to convert to sets and use difference to calculate
-            # the changes but this did not preserve order which is important
-            # particularly for schema
-            adds = [x for x in new_values if x not in old_values]
-            removes = [x for x in old_values if x not in new_values]
-
-            if len(adds) == 0 and len(removes) == 0:
-                continue
-
-            is_single_value = self.get_single_value(key)
-            force_replace = False
-            if key in FORCE_REPLACE_ON_UPDATE_ATTRS or is_single_value:
-                force_replace = True
-
-            if adds:
-                if force_replace:
-                    modlist.append((ldap.MOD_REPLACE, key, adds))
-                else:
-                    modlist.append((ldap.MOD_ADD, key, adds))
-            if removes:
-                if not force_replace or not new_values:
-                    modlist.append((ldap.MOD_DELETE, key, removes))
-
-        return modlist
-
     def modify_s(self, *args, **kwargs):
         # FIXME: for backwards compatibility only
         return self.conn.modify_s(*args, **kwargs)
diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index 0c44a85..97d7a35 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -722,7 +722,7 @@ class LDAPUpdate:
         else:
             # Update LDAP
             try:
-                changes = self.conn.generateModList(entry.orig_data, entry)
+                changes = self.conn._generate_modlist(entry.dn, entry)
                 if len(changes) >= 1:
                     updated = True
                 safe_changes = []
@@ -731,7 +731,7 @@ class LDAPUpdate:
                 self.debug("%s" % safe_changes)
                 self.debug("Live %d, updated %d" % (self.live_run, updated))
                 if self.live_run and updated:
-                    self.conn.updateEntry(entry.dn, entry.orig_data, entry)
+                    self.conn.update_entry(entry)
                 self.info("Done")
             except errors.EmptyModlist:
                 self.info("Entry already up-to-date")
-- 
1.8.4.2

>From 31ca8b069976e2c1b8967198aaca201f217f7a2c Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:42:35 +0100
Subject: [PATCH 06/12] Reduce amount of LDAPEntry.reset_modlist calls in
 ldapupdate.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipaserver/install/ldapupdate.py | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index 97d7a35..087cfcf 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -279,11 +279,6 @@ class LDAPUpdate:
         if fd != sys.stdin: fd.close()
         return text
 
-    def _entry_to_entity(self, ent):
-        entry = ent.copy()
-        entry.reset_modlist()
-        return entry
-
     def _combine_updates(self, all_updates, update):
         'Combine a new update with the list of total updates'
         dn = update.get('dn')
@@ -518,7 +513,7 @@ class LDAPUpdate:
 
         if not default:
             # This means that the entire entry needs to be created with add
-            return self._entry_to_entity(entry)
+            return entry
 
         for item in default:
             # We already do syntax-parsing so this is safe
@@ -531,8 +526,9 @@ class LDAPUpdate:
             else:
                 e = [value]
             entry[attr] = e
+        entry.reset_modlist()
 
-        return self._entry_to_entity(entry)
+        return entry
 
     def _get_entry(self, dn):
         """Retrieve an object from LDAP.
@@ -672,7 +668,7 @@ class LDAPUpdate:
             if len(e) > 1:
                 # we should only ever get back one entry
                 raise BadSyntax, "More than 1 entry returned on a dn search!? %s" % new_entry.dn
-            entry = self._entry_to_entity(e[0])
+            entry = e[0]
             found = True
             self.info("Updating existing entry: %s", entry.dn)
         except errors.NotFound:
-- 
1.8.4.2

>From a490741aaec9d2cc62908e7ace96aded5c98a6e2 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:45:10 +0100
Subject: [PATCH 07/12] Add LDAPEntry method generate_modlist.

Use LDAPEntry.generate_modlist instead of LDAPClient._generate_modlist and
remove LDAPClient._generate_modlist.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipapython/ipaldap.py              | 85 ++++++++++++++++++---------------------
 ipaserver/install/ldapupdate.py   |  2 +-
 ipaserver/install/schemaupdate.py |  2 +-
 3 files changed, 41 insertions(+), 48 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index a269581..66fb98e 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -988,6 +988,44 @@ class LDAPEntry(collections.MutableMapping):
             return NotImplemented
         return other is not self
 
+    def generate_modlist(self):
+        modlist = []
+
+        names = set(self.iterkeys())
+        names.update(self._orig.iterkeys())
+        for name in names:
+            new = self.raw.get(name)
+            old = self._orig.raw.get(name)
+            if old and not new:
+                modlist.append((ldap.MOD_DELETE, name, None))
+                continue
+            if not old:
+                modlist.append((ldap.MOD_REPLACE, name, new))
+                continue
+
+            # We used to convert to sets and use difference to calculate
+            # the changes but this did not preserve order which is important
+            # particularly for schema
+            adds = [value for value in new if value not in old]
+            dels = [value for value in old if value not in new]
+            if adds and self.conn.get_single_value(name):
+                if len(adds) > 1:
+                    raise errors.OnlyOneValueAllowed(attr=name)
+                modlist.append((ldap.MOD_REPLACE, name, adds))
+            else:
+                if adds:
+                    modlist.append((ldap.MOD_ADD, name, adds))
+                if dels:
+                    modlist.append((ldap.MOD_DELETE, name, dels))
+
+        # Usually the modlist order does not matter.
+        # However, for schema updates, we want 'attributetypes' before
+        # 'objectclasses'.
+        # A simple sort will ensure this.
+        modlist.sort(key=lambda m: m[1].lower() != 'attributetypes')
+
+        return modlist
+
     # FIXME: Remove when python-ldap tuple compatibility is dropped
     def __iter__(self):
         yield self._dn
@@ -1583,51 +1621,6 @@ class LDAPClient(object):
             self.conn.rename_s(dn, new_rdn, delold=int(del_old))
             time.sleep(.3)  # Give memberOf plugin a chance to work
 
-    def _generate_modlist(self, dn, entry_attrs):
-        assert isinstance(dn, DN)
-
-        # generate modlist
-        # for multi value attributes: no MOD_REPLACE to handle simultaneous
-        # updates better
-        # for single value attribute: always MOD_REPLACE
-        modlist = []
-        for (k, v) in entry_attrs.raw.iteritems():
-            if not v and k in entry_attrs.orig_data:
-                modlist.append((ldap.MOD_DELETE, k, None))
-            else:
-                v = set(v)
-                old_v = set(entry_attrs.orig_data.raw.get(k, []))
-
-                adds = list(v.difference(old_v))
-                rems = list(old_v.difference(v))
-
-                is_single_value = self.get_single_value(k)
-
-                value_count = len(old_v) + len(adds) - len(rems)
-                if is_single_value and value_count > 1:
-                    raise errors.OnlyOneValueAllowed(attr=k)
-
-                force_replace = False
-                if len(v) > 0 and len(v.intersection(old_v)) == 0:
-                    force_replace = True
-
-                if adds:
-                    if force_replace:
-                        modlist.append((ldap.MOD_REPLACE, k, adds))
-                    else:
-                        modlist.append((ldap.MOD_ADD, k, adds))
-                if rems:
-                    if not force_replace:
-                        modlist.append((ldap.MOD_DELETE, k, rems))
-
-        # Usually the modlist order does not matter.
-        # However, for schema updates, we want 'attributetypes' before
-        # 'objectclasses'.
-        # A simple sort will ensure this.
-        modlist.sort(key=lambda m: m[1].lower())
-
-        return modlist
-
     def update_entry(self, entry, entry_attrs=None):
         """Update entry's attributes.
 
@@ -1641,7 +1634,7 @@ class LDAPClient(object):
             entry.update(entry_attrs)
 
         # generate modlist
-        modlist = self._generate_modlist(entry.dn, entry)
+        modlist = entry.generate_modlist()
         if not modlist:
             raise errors.EmptyModlist()
 
diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index 087cfcf..42964c0 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -718,7 +718,7 @@ class LDAPUpdate:
         else:
             # Update LDAP
             try:
-                changes = self.conn._generate_modlist(entry.dn, entry)
+                changes = entry.generate_modlist()
                 if len(changes) >= 1:
                     updated = True
                 safe_changes = []
diff --git a/ipaserver/install/schemaupdate.py b/ipaserver/install/schemaupdate.py
index f51b29b..bb2f0f1 100644
--- a/ipaserver/install/schemaupdate.py
+++ b/ipaserver/install/schemaupdate.py
@@ -114,7 +114,7 @@ def update_schema(schema_files, ldapi=False, dm_password=None, live_run=True):
 
     # FIXME: We should have a better way to display the modlist,
     # for now display raw output of our internal routine
-    modlist = conn._generate_modlist(schema_entry.dn, schema_entry)
+    modlist = schema_entry.generate_modlist()
     log.debug("Complete schema modlist:\n%s", pprint.pformat(modlist))
 
     if modified and live_run:
-- 
1.8.4.2

>From c95c25bd5416f4b3fccd846c5c9a7453b6aa1820 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:48:23 +0100
Subject: [PATCH 08/12] Remove unused LDAPClient methods get_syntax and
 get_single_value.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipapython/ipaldap.py | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 66fb98e..e4649a2 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -1227,15 +1227,6 @@ class LDAPClient(object):
         """schema associated with this LDAP server"""
         return self.conn.schema
 
-    def get_syntax(self, attr, value):
-        if self.schema is None:
-            return None
-        obj = self.schema.get_obj(ldap.schema.AttributeType, attr)
-        if obj is not None:
-            return obj.syntax
-        else:
-            return None
-
     def has_dn_syntax(self, attr):
         return self.conn.has_dn_syntax(attr)
 
@@ -1252,9 +1243,6 @@ class LDAPClient(object):
                     reason=_('objectclass %s not found') % oc)
         return [unicode(a).lower() for a in list(set(allowed_attributes))]
 
-    def get_single_value(self, attr):
-        return self.conn.get_single_value(attr)
-
     def make_dn_from_attr(self, attr, value, parent_dn=None):
         """
         Make distinguished name from attribute.
-- 
1.8.4.2

>From eedb6ffde2dec3bcb7b300db12515566d50f6dbf Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:52:31 +0100
Subject: [PATCH 09/12] Remove legacy LDAPEntry properties data and orig_data.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipapython/ipaldap.py                        | 11 -----------
 ipaserver/install/ldapupdate.py             |  4 ----
 ipaserver/install/plugins/rename_managed.py |  8 ++++----
 3 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index e4649a2..99c27c4 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -714,7 +714,6 @@ class LDAPEntry(collections.MutableMapping):
     def conn(self):
         return self._conn
 
-    # properties for Entry and Entity compatibility
     @property
     def dn(self):
         return self._dn
@@ -736,16 +735,6 @@ class LDAPEntry(collections.MutableMapping):
             self._single_value_view = SingleValueLDAPEntryView(self)
         return self._single_value_view
 
-    @property
-    def data(self):
-        # FIXME: for backwards compatibility only
-        return self
-
-    @property
-    def orig_data(self):
-        # FIXME: for backwards compatibility only
-        return self._orig
-
     def __repr__(self):
         data = dict(self._raw)
         data.update((k, v) for k, v in self._nice.iteritems() if v is not None)
diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index 42964c0..94a1fed 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -693,10 +693,6 @@ class LDAPUpdate:
         added = False
         updated = False
         if not found:
-            # New entries get their orig_data set to the entry itself. We want to
-            # empty that so that everything appears new when generating the
-            # modlist
-            # entry.orig_data = {}
             try:
                 if self.live_run:
                     if len(entry):
diff --git a/ipaserver/install/plugins/rename_managed.py b/ipaserver/install/plugins/rename_managed.py
index e0fa36b..dad9259 100644
--- a/ipaserver/install/plugins/rename_managed.py
+++ b/ipaserver/install/plugins/rename_managed.py
@@ -74,7 +74,7 @@ class GenerateUpdateMixin(object):
         for entry in definitions_managed_entries:
             assert isinstance(entry.dn, DN)
             if deletes:
-                old_dn = entry.data['managedtemplate'][0]
+                old_dn = entry['managedtemplate'][0]
                 assert isinstance(old_dn, DN)
                 try:
                     (old_dn, entry) = ldap.get_entry(old_dn, ['*'])
@@ -102,14 +102,14 @@ class GenerateUpdateMixin(object):
 
             else:
                 # Update the template dn by replacing the old containter with the new container
-                old_dn = entry.data['managedtemplate'][0]
+                old_dn = entry['managedtemplate'][0]
                 new_dn = EditableDN(old_dn)
                 if new_dn.replace(old_template_container, new_template_container) != 1:
                     self.error("unable to replace '%s' with '%s' in '%s'",
                                old_template_container, new_template_container, old_dn)
                     continue
                 new_dn = DN(new_dn)
-                entry.data['managedtemplate'] = new_dn
+                entry['managedtemplate'] = new_dn
 
                 # Edit the dn, then convert it back to an immutable DN
                 old_dn = entry.dn
@@ -122,7 +122,7 @@ class GenerateUpdateMixin(object):
 
                 # The old attributes become defaults for the new entry
                 new_update = {'dn': new_dn,
-                              'default': entry_to_update(entry.data)}
+                              'default': entry_to_update(entry)}
 
                 # Add the replacement update to the collection of all updates
                 update_list.append({new_dn: new_update})
-- 
1.8.4.2

>From e4a15ee7db2e42a434449d6939d7a5b17e04a980 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:53:32 +0100
Subject: [PATCH 10/12] Store old entry state in dict rather than LDAPEntry.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipapython/ipaldap.py       | 39 ++++++++++++++-------------------------
 ipaserver/plugins/ldap2.py |  4 ++--
 2 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 99c27c4..daa8394 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -690,14 +690,14 @@ class LDAPEntry(collections.MutableMapping):
         self._raw = {}
         self._sync = {}
         self._not_list = set()
-        self._orig = self
+        self._orig = {}
         self._raw_view = None
         self._single_value_view = None
 
         if isinstance(_obj, LDAPEntry):
             #pylint: disable=E1103
             self._not_list = set(_obj._not_list)
-            self._orig = _obj._orig
+            self._orig = dict(_obj._orig)
             if _obj.conn is _conn:
                 self._names = CIDict(_obj._names)
                 self._nice = dict(_obj._nice)
@@ -743,27 +743,6 @@ class LDAPEntry(collections.MutableMapping):
     def copy(self):
         return LDAPEntry(self)
 
-    def clone(self):
-        result = LDAPEntry(self._conn, self._dn)
-
-        result._names = deepcopy(self._names)
-        result._nice = deepcopy(self._nice)
-        result._raw = deepcopy(self._raw)
-        result._sync = deepcopy(self._sync)
-        result._not_list = deepcopy(self._not_list)
-        if self._orig is not self:
-            result._orig = self._orig.clone()
-
-        return result
-
-    def reset_modlist(self):
-        """
-        Make the current state of the entry a new reference point for change
-        tracking.
-        """
-        self._orig = self
-        self._orig = self.clone()
-
     def _sync_attr(self, name):
         nice = self._nice[name]
         assert isinstance(nice, list)
@@ -836,6 +815,8 @@ class LDAPEntry(collections.MutableMapping):
                 if oldname in self._not_list:
                     self._not_list.remove(oldname)
                     self._not_list.add(name)
+                if oldname in self._orig:
+                    self._orig[name] = self._orig.pop(oldname)
         else:
             if self._conn.schema is not None:
                 attrtype = self._conn.schema.get_obj(ldap.schema.AttributeType,
@@ -847,6 +828,11 @@ class LDAPEntry(collections.MutableMapping):
 
             self._names[name] = name
 
+            for oldname in self._orig.keys():
+                if self._names.get(oldname) == name:
+                    self._orig[name] = self._orig.pop(oldname)
+                    break
+
     def _set_nice(self, name, value):
         name = self._attr_name(name)
         self._add_attr_name(name)
@@ -977,14 +963,17 @@ class LDAPEntry(collections.MutableMapping):
             return NotImplemented
         return other is not self
 
+    def reset_modlist(self):
+        self._orig = deepcopy(dict(self.raw))
+
     def generate_modlist(self):
         modlist = []
 
         names = set(self.iterkeys())
-        names.update(self._orig.iterkeys())
+        names.update(self._orig)
         for name in names:
             new = self.raw.get(name)
-            old = self._orig.raw.get(name)
+            old = self._orig.get(name)
             if old and not new:
                 modlist.append((ldap.MOD_DELETE, name, None))
                 continue
diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py
index 97f26ec..f6284dc 100644
--- a/ipaserver/plugins/ldap2.py
+++ b/ipaserver/plugins/ldap2.py
@@ -272,7 +272,7 @@ class ldap2(LDAPClient, CrudBackend):
         try:
             config_entry = getattr(context, 'config_entry')
             if config_entry.conn is self.conn:
-                return config_entry.clone()
+                return config_entry
         except AttributeError:
             # Not in our context yet
             pass
@@ -289,7 +289,7 @@ class ldap2(LDAPClient, CrudBackend):
         for a in self.config_defaults:
             if a not in config_entry:
                 config_entry[a] = self.config_defaults[a]
-        context.config_entry = config_entry.clone()
+        context.config_entry = config_entry
         return config_entry
 
     def has_upg(self):
-- 
1.8.4.2

>From 9492cd7de14a734cb451addc5e7cb754de37692a Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:54:25 +0100
Subject: [PATCH 11/12] Do not crash on bad LDAP data when formatting decode
 error message.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipapython/ipaldap.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index daa8394..75f0b74 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -422,7 +422,7 @@ class IPASimpleLDAPObject(object):
             try:
                 return target_type(val)
             except Exception, e:
-                msg = 'unable to convert the attribute "%s" value "%s" to type %s' % (attr, val, target_type)
+                msg = 'unable to convert the attribute %r value %r to type %s' % (attr, val, target_type)
                 self.log.error(msg)
                 raise ValueError(msg)
         elif isinstance(val, list):
-- 
1.8.4.2

>From 4ba9a29c5de2e9110dab300010dbf280ae76f4e1 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 Dec 2013 11:56:35 +0100
Subject: [PATCH 12/12] Use raw LDAP data in ldapupdate.

https://fedorahosted.org/freeipa/ticket/3488
---
 ipaserver/install/ldapupdate.py | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index 94a1fed..a9167ae 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -50,9 +50,9 @@ from ipaserver.plugins import ldap2
 def connect(ldapi=False, realm=None, fqdn=None, dm_password=None, pw_name=None):
     """Create a connection for updates"""
     if ldapi:
-        conn = ipaldap.IPAdmin(ldapi=True, realm=realm)
+        conn = ipaldap.IPAdmin(ldapi=True, realm=realm, decode_attrs=False)
     else:
-        conn = ipaldap.IPAdmin(fqdn, ldapi=False, realm=realm)
+        conn = ipaldap.IPAdmin(fqdn, ldapi=False, realm=realm, decode_attrs=False)
     try:
         if dm_password:
             conn.do_simple_bind(binddn=DN(('cn', 'directory manager')),
@@ -235,8 +235,7 @@ class LDAPUpdate:
                                 skipinitialspace=True,
                                 **kwargs)
         for row in csv_reader:
-            # decode UTF-8 back to Unicode, cell by cell:
-            yield [unicode(cell, 'utf-8') for cell in row]
+            yield row
 
     def _identify_arch(self):
         """On multi-arch systems some libraries may be in /lib64, /usr/lib64,
@@ -557,19 +556,7 @@ class LDAPUpdate:
             # We already do syntax-parsing so this is safe
             (action, attr, update_values) = update.split(':',2)
             update_values = self._parse_values(update_values)
-
-            # If the attribute is known to be a DN convert it to a DN object.
-            # This has to be done after _parse_values() due to quoting and comma separated lists.
-            if self.conn.has_dn_syntax(attr):
-                update_values = [DN(x) for x in update_values]
-
-            entry_values = entry.get(attr)
-            if not isinstance(entry_values, list):
-                if entry_values is None:
-                    entry_values = []
-                else:
-                    entry_values = [entry_values]
-
+            entry_values = entry.get(attr, [])
             for update_value in update_values:
                 if action == 'remove':
                     self.debug("remove: '%s' from %s, current value %s", safe_output(attr, update_value), attr, safe_output(attr,entry_values))
@@ -646,12 +633,9 @@ class LDAPUpdate:
             self.debug("%s", message)
         self.debug("dn: %s", e.dn)
         for a, value in e.items():
-            if isinstance(value, (list, tuple)):
-                self.debug('%s:', a)
-                for l in value:
-                    self.debug("\t%s", safe_output(a, l))
-            else:
-                self.debug('%s: %s', a, safe_output(a, value))
+            self.debug('%s:', a)
+            for l in value:
+                self.debug("\t%s", safe_output(a, l))
 
     def _update_record(self, update):
         found = False
-- 
1.8.4.2

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to