Re: [Freeipa-devel] [PATCHES] 106-113 Access raw LDAP values directly from LDAPEntry
On 10/29/2013 04:17 PM, Petr Viktorin wrote: On 10/29/2013 01:34 PM, Jan Cholasta wrote: On 16.10.2013 18:13, Petr Viktorin wrote: On 10/14/2013 10:59 AM, Jan Cholasta wrote: On 10.10.2013 09:45, Jan Cholasta wrote: On 9.10.2013 13:57, Petr Viktorin wrote: [...] 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. That's what I was afraid of. We could live with `isinstance(value, list)`; hopefully we could get rid of `type(value) == list` that is the real problem. With `entry[attr] = list()` we could convert automatically. But OK, let's settle for a worse solution in the meantime. To be frank I don't particularly like the LDAPEntryView. While the dict-like interface is great, there isn't a case for storing a Raw view long-term, i.e. you'd always want to do something like values = entry.raw[x] ... entry.raw[x] = new_values rather than raw = entry.raw values = raw[x] ... raw[x] = new_values The latter is confusing because LDAPEntryView and RawLDAPEntryView are two classes that have exactly the same interface, but do something different. In a duck-typed world that's a recipe for disaster. I think it would be better if the view implemented just the dict protocol, and not `conn`, `dn`, `nice`, `raw`, etc. The code would also be much simpler without the elaborate view class hierarchy. If you don't agree then at least don't make `raw` available on raw views and `nice` on nice views; the programmer *always* needs to know which version they're working with so these aren't necessary. I agree. Most of the attributes are leftovers from a previous implementation, which didn't work very well. I should have removed them a long time ago. Thanks for pointing this out! Updated the views to provide only the dict interface, removed nice and multi_value properties and also removed single_value from the raw view. Looks much better now. Hopefully _sync_attr can dissappear one day. 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 Updated patches attached. 110. It can't hurt to have this in for now. 111 - 121 look great! 106 - 121: ACK 169. For reasons I said before I'd prefer if single_value stayed a simple function. IMO a view better matches its semantics, plus I changed the code, so I would like to keep it a view, if you don't mind. OK, ACK to that one as well, but I'd rather wait a few weeks (until 3.3 churn dies out) before pushing it, since it could complicate backporting patches. Pushed 169 to master: df5f4ee81d1aff1122dd92ab1b56eb335294c3a7 -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 106-113 Access raw LDAP values directly from LDAPEntry
On 10/29/2013 04:17 PM, Petr Viktorin wrote: On 10/29/2013 01:34 PM, Jan Cholasta wrote: On 16.10.2013 18:13, Petr Viktorin wrote: On 10/14/2013 10:59 AM, Jan Cholasta wrote: On 10.10.2013 09:45, Jan Cholasta wrote: On 9.10.2013 13:57, Petr Viktorin wrote: [...] 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. That's what I was afraid of. We could live with `isinstance(value, list)`; hopefully we could get rid of `type(value) == list` that is the real problem. With `entry[attr] = list()` we could convert automatically. But OK, let's settle for a worse solution in the meantime. To be frank I don't particularly like the LDAPEntryView. While the dict-like interface is great, there isn't a case for storing a Raw view long-term, i.e. you'd always want to do something like values = entry.raw[x] ... entry.raw[x] = new_values rather than raw = entry.raw values = raw[x] ... raw[x] = new_values The latter is confusing because LDAPEntryView and RawLDAPEntryView are two classes that have exactly the same interface, but do something different. In a duck-typed world that's a recipe for disaster. I think it would be better if the view implemented just the dict protocol, and not `conn`, `dn`, `nice`, `raw`, etc. The code would also be much simpler without the elaborate view class hierarchy. If you don't agree then at least don't make `raw` available on raw views and `nice` on nice views; the programmer *always* needs to know which version they're working with so these aren't necessary. I agree. Most of the attributes are leftovers from a previous implementation, which didn't work very well. I should have removed them a long time ago. Thanks for pointing this out! Updated the views to provide only the dict interface, removed nice and multi_value properties and also removed single_value from the raw view. Looks much better now. Hopefully _sync_attr can dissappear one day. 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 Updated patches attached. 110. It can't hurt to have this in for now. 111 - 121 look great! 106 - 121: ACK 106-121 pushed to master. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 106-113 Access raw LDAP values directly from LDAPEntry
On 10/29/2013 01:34 PM, Jan Cholasta wrote: On 16.10.2013 18:13, Petr Viktorin wrote: On 10/14/2013 10:59 AM, Jan Cholasta wrote: On 10.10.2013 09:45, Jan Cholasta wrote: On 9.10.2013 13:57, Petr Viktorin wrote: [...] 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. That's what I was afraid of. We could live with `isinstance(value, list)`; hopefully we could get rid of `type(value) == list` that is the real problem. With `entry[attr] = list()` we could convert automatically. But OK, let's settle for a worse solution in the meantime. To be frank I don't particularly like the LDAPEntryView. While the dict-like interface is great, there isn't a case for storing a Raw view long-term, i.e. you'd always want to do something like values = entry.raw[x] ... entry.raw[x] = new_values rather than raw = entry.raw values = raw[x] ... raw[x] = new_values The latter is confusing because LDAPEntryView and RawLDAPEntryView are two classes that have exactly the same interface, but do something different. In a duck-typed world that's a recipe for disaster. I think it would be better if the view implemented just the dict protocol, and not `conn`, `dn`, `nice`, `raw`, etc. The code would also be much simpler without the elaborate view class hierarchy. If you don't agree then at least don't make `raw` available on raw views and `nice` on nice views; the programmer *always* needs to know which version they're working with so these aren't necessary. I agree. Most of the attributes are leftovers from a previous implementation, which didn't work very well. I should have removed them a long time ago. Thanks for pointing this out! Updated the views to provide only the dict interface, removed nice and multi_value properties and also removed single_value from the raw view. Looks much better now. Hopefully _sync_attr can dissappear one day. 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 Updated patches attached. 110. It can't hurt to have this in for now. 111 - 121 look great! 106 - 121: ACK 169. For reasons I said before I'd prefer if single_value stayed a simple function. IMO a view better matches its semantics, plus I changed the code, so I would like to keep it a view, if you don't mind. OK, ACK to that one as well, but I'd rather wait a few weeks (until 3.3 churn dies out) before pushing it, since it could complicate backporting patches. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 106-113 Access raw LDAP values directly from LDAPEntry
On 10/14/2013 10:59 AM, Jan Cholasta wrote: On 10.10.2013 09:45, Jan Cholasta wrote: 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. Git rants about one more whitespace error. [...] +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 ==... Thanks. Please also implement __ne__() when reimplementing __eq__(). 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. Right, IMO it's not immediately obvious that these are reimplemented because they depend on __iter__. [...] 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. That's what I was afraid of. We could live with `isinstance(value, list)`; hopefully we could get rid of `type(value) == list` that is the real problem. With `entry[attr] = list()` we could convert automatically. But OK, let's settle for a worse solution in the meantime. To be frank I don't particularly like the LDAPEntryView. While the dict-like interface is great, there isn't a case for storing a Raw view long-term, i.e. you'd always want to do something like values = entry.raw[x] ... entry.raw[x] = new_values rather than raw = entry.raw values = raw[x] ... raw[x] = new_values The latter is confusing because LDAPEntryView and RawLDAPEntryView are two classes that have exactly the same interface, but do something different. In a duck-typed world that's a recipe for disaster. I think it would be better if the view implemented just the dict protocol, and not `conn`, `dn`, `nice`, `raw`, etc. The code would also be much simpler without the elaborate view class hierarchy. If you don't agree then at least don't make `raw` available on raw views and `nice` on nice views; the programmer *always* needs to know which version they're working with so these aren't
Re: [Freeipa-devel] [PATCHES] 106-113 Access raw LDAP values directly from LDAPEntry
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
Re: [Freeipa-devel] [PATCHES] 106-113 Access raw LDAP values directly from LDAPEntry
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. 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. +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`). 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. I'd welcome FIXME comments on whatever is reimplemented for this reason. 107. Introduce IPASimpleLDAPObject.decode method for decoding LDAP values. Can you put in a docstring? 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. 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 think it would also help (in the future?) to make the value lists more set-like, since the order doesn't matter. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
