On 9.10.2013 13:57, Petr Viktorin wrote:
On 09/26/2013 02:22 PM, Jan Cholasta wrote:
On 24.9.2013 15:35, Jan Cholasta wrote:
On 27.2.2013 16:31, Jan Cholasta wrote:
Hi,

these patches add the ability to access and manipulate raw attribute
values as they are returned from python-ldap to the LDAPEntry class.
This is useful for comparing entries, computing modlists for the modify
operation, deleting values that don't match the syntax of an attribute,
etc., because we don't need to care what syntax does a particular
attribute use, or what Python type is used for it in the framework (raw
values are always list of str). It should also make interaction with
non-389 DS LDAP servers easier in the framework.

(It might be too late for this kind of changes to get into 3.2 now, I'm
posting these patches mainly so that you are aware that they exist.)

Honza


This is now planned for 3.4:
<https://fedorahosted.org/freeipa/ticket/3521>

I fixed some issues in these patches and refined the API. Updated
patches attached.

Also added a patch to use raw values when adding new entries and a patch
which refines LDAPEntry.single_value, so that it is consistent with the
rest of the changes introduced in the patches.

Patch 110 will probably be dropped in favor of Petr Viktorin's schema
update patches, but I included it anyway.

Incidentally, this also fixes
<https://fedorahosted.org/freeipa/ticket/3927> and possibly also
<https://fedorahosted.org/freeipa/ticket/2131>.

Honza


Noticed a couple more issues and fixed them. Updated patches attached.

Honza

Thanks for the patches!


106. Make LDAPEntry a wrapper around dict rather than a dict subclass.

ipapython/ipaldap.py:847:
  warning: 1 line adds whitespace errors.

          if isinstance(_obj, LDAPEntry):
+            data = dict(_obj._data)
              orig = _obj._orig

Is this necessary? `self.update(_obj)` is done later.

Probably not. But it's removed in patch 109.



      def __contains__(self, name):
-        return self._names.has_key(self._attr_name(name))
+        return self._names.has_key(name)

has_key() is deprecated for dict, it would make sense to prefer `name in
self._names` for CIDict too.

Sure, this line is from before CIDict got __contains__.


+    def __eq__(self, other):
+        if not isinstance(other, LDAPEntry):
+            return NotImplemented
+        if self._dn != other._dn:
+            return False
+        return super(LDAPEntry, self).__eq__(other)

I don't think equality checking makes sense on a LDAPEntry, where you
might have different capitalizations/variants of attribute names,
different _orig, or a different set of attributes loaded on the same
entry. It's not obvious which of those differences should make the
entries inequal.
I'd just base it on identity (`self is other`).

Right, I'm not sure why I even did it this way. But I remember seeing some code that did comparison of entries with ==...


      def __iter__(self):
          yield self._dn
          yield self

This makes the whole thing sort of hackish -- we need to reimplement
everything in MutableMapping that uses iter() internally :(
Hopefully we can get rid of it soon.

Yes, it's a shame MutableMapping uses iter() instead of iterkeys().

I'd welcome FIXME comments on whatever is reimplemented for this reason.

I thought the comment above __iter__ would be enough. Apparently I was wrong.



107. Introduce IPASimpleLDAPObject.decode method for decoding LDAP values.

Can you put in a docstring?

OK.




108. Always use lists for values in LDAPEntry internally.

@@ -698,6 +701,7 @@ class LDAPEntry(collections.MutableMapping):

          result._names = deepcopy(self._names)
          result._data = deepcopy(self._data)
+        result._not_list = deepcopy(self._not_list)
          if self._orig is not self:
              result._orig = self._orig.clone()

It's better to use set() than deepcopy() for a set of strings.

Right.



109. Decode and encode attribute values in LDAPEntry on demand.

The syncing looks rather over-engineered to me.
Did you consider a custom MutableSequence for the values?
I think it would be much cleaner in the end than merging two sets of
changes together.

I'm not entirely happy about it either, but it works. I did consider a custom sequence type, but I didn't feel like it was the right time to this kind of change in this patchset. Unlike the (DN, dict) -> LDAPEntry transition, this change won't be backward compatible and there is a lot of isinstance(value, list) and entry[attr] = list() kind of things in the framework code.


I think it would also help (in the future?) to make the value lists more
set-like, since the order doesn't matter.

+1

Honza

--
Jan Cholasta

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

Reply via email to