On 07/18/2012 12:47 AM, John Dennis wrote:
On 07/10/2012 04:23 AM, Petr Viktorin wrote:
I've read your summary (which you should summarize into a commit message
before this is pushed), and gone through the patch.
Here is what I found doing that; I didn't get to actual testing yet.
I also didn't do a detailed review of ldap2.py changes yet.
Thank you for your careful review Petr, you had many good suggestions
and comments. It was an enormous amount of material to wade through,
thank you for your diligence. I'm in agreement with most of your
suggestions.
Great :)
I don't have much to reply, I mostly agree too.
[...]
> Originally I was against automatic type promotion on the belief if
you were comparing a string to a DN it was a failure of program logic,
but there were some circumstances where it was evil necessity.
Would you care to elaborate? I also think that's a logic failure; such
circumstances should at least be marked by a TODO.
I should have taken better notes because I don't remember the exact
place this came up, it was late in the testing and I believe the problem
showed up in the ldap updater, possibly some other places.
But here is the bottom line: I encountered a handful of places in the
code where a string was being compared to a DN object for equality (I
seem to recall the equality comparison was indirect by virtue of the
"in" operator). What I do recall is it was logically impossible to know
a prori the string being tested was supposed to be a DN. If it were
possible to know the string represented a DN it would have been easy to
promote it to a DN object. But there were a handful of situations where
there simply was not enough information available to ascertain if the
string was logically a DN or not. This was never an issue when dn's were
simple strings. If it's getting compared to a DN that's a pretty strong
hint it's supposed to be a DN (fwiw the coercion must occur in the
equality operator).
So I pondered the problem for a while and decided there was legitimate
value in allowing an equality comparison between a string and DN. I
could see no downside to it other than it *might* mask an error in
program logic. I didn't take the decision lightly. I looked through the
Python internals to see where type coercion occurs. While not often it's
certainly done where "it makes sense" (e.g. between string types,
between numerical types, between buffer types, etc.). I thought one
could make the argument that a DN object is akin to a string type and
hence falls into the same category of coercion as occurs between string
types. I'm not saying I'm in love with it, rather it's a pragmatic
necessity and can be justified if you look at it a certain way.
We could add a debug message with a backtrace whenever the coercion
occurs if we wanted to completely eliminate all places in the code where
a string-to-DN comparison occurs. But as I said I'm not sure it's
possible to absolutely eliminate this comparison. We've eliminated 99.9%
of those cases already, I'm not sure the other 0.1% can be eliminated or
that it's worth the work involved. I'm happy with 99.9% :-)
Fair enough.
> The find() and rfind() methods (modeled after their string cousins)
to locate a RDN or DN in an existing DN.
I would much rather see the index() and rindex() methods, which do the
Pythonic thing of raising an exception rather than silently returning -1
when the thing isn't found.
Compare:
something[something.find(thing)] => something[-1] if not found
something[something.index(thing)] => raises exception if not found
You're right exceptions are more Pythoninc than return codes. I can add
index and rindex and convert the code to catch an exception.
However, we use find and rfind extensively in the IPA code base
(DN.find() and DN.rfind() simply replaced their string equivalents
during the conversion). Perhaps you should open a ticket to purge all
use of find() and rfind() from our code base and add a prohibition
against their use in our Python coding standard document.
I don't think it's that urgent.
ipapython/dn.py:448:
Each argument must be scalar convertable to unicode or a callable
object whose result is convertable to unicode.
Is it really necessary to allow callables here? If the user has a
callable, he/she should call it before making an AVA out of it. Same
with converting to Unicode.
As you point out in dn_summary, automatic coercion is evil.
There are two issues here, type coercion and string encoding.
Not all coercion is evil, there is value in making it easy to do the
right thing and minimizing inline coercion that otherwise obfuscates the
logical intent of the code. We shouldn't require programmers to be
perfect coders who never forget to perform some operation in the
hundreds of places it might be required. Rather the tools should quietly
and consistently enforce the rules while leaving the code readable,
concise and with clear intent.
There is a well established precedent in Python for coercion to string
in a string context. Coercion to string is so fundamental to Python it
has it's own low-level operator embedded in every class type definition
utilized in both Python and the CPython implementations.
1) We know all DN components must be strings. Let's say someone passes
an integer object; is there really any harm to converting this to a
string representation of an integer? That makes the DN API much
friendlier and it's use more succinct. In fact Python does automatic
coercion to string in many places where it knows something must be a
string, thus it's a well accepted and established practice. We do
automatic coercion to string in many places in IPA (a good example is
the ldap encode/decode operations).
I wish other Python API's also coerced to string as a service to the
caller. I wish python-ldap would have coerced to string those elements
of it's API which it knows must be strings. If it had we could have just
passed DN objects into the python-ldap API instead of having to wrap the
class to perform the coercion. This wouldn't be violating a guideline,
rather it would be utilizing a inherent feature of the Python language.
There are an astonishing number of CPython modules that barf if you pass
the wrong string type. Instead they should be friendly and provide the
right string coercion. (There is a long and sordid history of this
problem and I have a extensive write-up on it).
2) All strings in IPA are unicode. In python 3.x all strings will be
unicode (the way it should have been in python 2.x alas). When string
comparisons are performed they should be done as unicode objects, not
encoded in some undefined encoding. Likewise when returning a string
belonging to a component in a DN it should be unicode. Traditionally
dn's have been str objects (a python-ldap foible), so the decoding of
str to unicode promotes correctness in Python 2.x.
Accepting a callable is just a further refinement of the discussion in
#1. If you allow for automatic coercion to sting (in a context that
demands a string type) then why not allow anything which yields a
string? It just makes the API friendlier and easier to use without
introducing ugly compromises. Some coercion is evil, but some is darn
nice and useful. The trick is recognizing which is which.
You have a point for the conversion to strings.
However, I'm still not convinced automatic function calling is the good
kind of coercion.
It's different from the string conversions Python does automatically --
str(function) gives you '<function function at 0x7f482f6ad8c0>' -- so
it's not something one would expect. Silently doing unexpected things
will lead to errors.
Plus, it saves all of two parentheses of typing, so it's not even that
much more friendly.
[...]
In several places:
+ # Use a property for <attr> to assure it's always a DN type
+ def _set_<attr>(self, value):
+ self._<attr> = DN(value)
+
+ def _get_<attr>(self):
+ assert isinstance(getattr(self, '_<attr>'), DN)
+ return self._<attr>
+
+ ldap_suffix = property(_get_<attr>, _set_<attr>)
Shouldn't the assert be in the setter? Do we not want people to only put
DNs in these properties, instead of silently converting?
I'm afraid I don't follow you here. When setting the property it should
be coerced to a DN, we want to force the property to always be a DN.
However it's possible for someone to set the _<attr> directly even
though they should never do that, the assert in the getter is just
bullet-proofing against that.
This appears enough times to justify using a helper to construct these.
But if you don't want to generalize, the getattr call is unnecessary.
Once again I don't follow you. How would a helper work?
Something like:
def checked_dn_property(private_name):
def setter(self, value):
setattr(self, private_name, DN(value))
def getter(self):
value = getattr(self, private_name)
assert isinstance(value, DN)
return value
return property(getter, setter)
...
ldap_suffix = checked_dn_property('_ldap_suffix')
I don't know why there is a getattr in the getter, it would seem to be
unnecessary unless I was trying to protect against an uninitialized
attribute. The getattr should probably be replaced with normal class
attribute access. I'll fix that.
tests/util.py:307:
if isinstance(expected, DN):
if isinstance(got, basestring):
got = DN(got)
This may be overly broad, the tests should be able to check if they got
a DN or a string. What about something like:
if isinstance(expected, DNString):
assert isinstance(got, basestring)
assert DN(got) == expected.dn
with a suitable DNString class?
Sorry I don't understand what you're trying to protect against nor what
a DNString class would be or what it's purpose is.
Our tests check types quite rigorously. The changes in the patch make
them ignore the difference between DNs and strings -- the test will pass
regardless of whether it gets a DN or a string. It seems that to keep
the tests in this spirit, it would be better to have a way to explicitly
say we want “a string in the form of a DN”.
For this there could be a DNString class for use in the tests, similar
to the Fuzzy class we already use. Something like
class DNString(object):
def __init__(self, *args):
self.dn = DN(args)
def __eq__(self, other):
assert isinstance(other, basestring)
return self.dn == DN(other)
def __ne__(self, other):
return not self == other
But I guess it's not that necessary.
ipapython/dn.py:541: def _set_locked(self, value):
There should be only one way to do things, either the lock/unlock
methods, or setting an attribute. More ways to do one same thing lead to
subtle errors in one of the ways.
The locking code is duplicated in AVA, RDN and DN; it would be good to
make a Lockable mixin to avoid that. A similar case is with __repr__.
Anyway unlocking is bad (see above) so these are mostly moot points.
A mixin would eliminate code duplication but in this case the
duplication is trivial, not sure it warrants introducing a new class in
this circumstance (judgment call), but the topic is moot because locking
is going to be eliminated.
ipaserver/plugins/ldap2.py:134:
class ServerSchema(object):
Please don't use nested classes without reason.
There is a valid reason, the ServerSchema class is private to the
SchemaCache class.
If it's private, I'd just name it with an underscore. Nested classes are
more trouble than it's worth, for example the string representation will
show the unreachable 'ipaserver.plugins.ldap2.ServerSchema', and tools
like pickle will choke on them.
Flat is better than nested.
[...]
> Python provides the __all__ module export list, we really should be
> making better use of that.
I recommend importing names explicitly, or importing a module and using
qualified names, over __all__ and import *. Star imports make it
unnecessarily hard to see where names come from.
(You do use explicit imports in the code, my gripe is just about
recommending * and __all__).
We should come to a project consensus on this topic and add it to our
Python style guide. This really isn't germane to the dn work.
There's an issue filed in the deferred bucket
(https://fedorahosted.org/freeipa/ticket/2653)
[...]
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
