On 07/07/2012 08:45 PM, John Dennis wrote:
The DN work I was doing on master is ready for review and testing. It's
been a long haul and I've been working relentlessly to get this work
completed. I am on PTO for a week starting today (I know bad timing) but
I spent yesterday and my first day of PTO today writing up extensive
documentation for the work so others can start taking a look at it while
I'm gone. The documentation as well as where to find the code can be
found here:

http://jdennis.fedorapeople.org/dn_summary.html

The document is long but I felt it was better to provide explanations
for as much as possible.

I may check in during the week but I'm going to try and discipline
myself not to and take an actual much needed break.

John


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.



I agree with Simo that it would have been better to put at least your automated changes in a separate patch. Without knowing what was automated and what wasn't, I have to eyeball each change at least to figure that out. Not complaining, it's a reviewer's work after all, and with an intra-line diff tool it's not that hard, but I can't really honor your suggestion for a faster review :)




==== Blockers ====

Mutable objects that support __hash__ are a *very* bad thing to do. It violates a fundamental assumption in Python, as you outlined. Mutable objects *must not* be allowed to be dictionary keys. I understand the patch is not perfect, but this is a big issue that invites very hard-to-debug errors. You say you're already working on a patch to fix this. Please post that ASAP; I can't ack the DN work without it.

In the long run, it would be good to use immutable DNs only: I think modeling them after Python strings in this way would be beneficial.
You seem to forged locked=True for some global DNs, e.g.
ipalib/plugins/pwpolicy.py:172: global_policy_dn
ipalib/plugins/migration.py:117: _compat_dn
Locked by default would prevent these omissions.



Please fix a lint failure; ipaserver.ipautil moved to ipapython.ipautil.



==== Design decision criticism ====

> 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.



> 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



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.



==== Major issues ====

ipalib/plugins/aci.py:340:
+        groupdn = DN(groupdn)
+        try:
+            if groupdn[0].attr == 'cn':
+                dn = DN()
+                entry_attrs = {}
+                try:
+                    (dn, entry_attrs) = ldap.get_entry(groupdn, ['cn'])
+                except errors.NotFound, e:
+                    # FIXME, use real name here
+                    if test:
+ dn = DN(('cn', 'test'), api.env.container_permission)
+                        entry_attrs = {'cn': [u'test']}
+                if api.env.container_permission in dn:
+                    kw['permission'] = entry_attrs['cn'][0]
+                else:
+                    if 'cn' in entry_attrs:
+                        kw['group'] = entry_attrs['cn'][0]
+        except IndexError:
+            pass

Why is any IndexError ignored here? That try block is much too long, it could catch unintended errors. Please only `try` the smallest piece of code that could raise the exception you want to catch.

There are enough `DN(xyz.replace('ldap:///',''))` calls in aci.py to warrant a strip_ldap_prefix helper. (It could also only remove the prefix, not all the occurrences -- unlikely to matter but it's better to do things correctly.)

ipalib/plugins/baseldap.py:1028:
+            try:
+                if dn[0].attr != self.obj.primary_key.name:
+                    self.obj.handle_duplicate_entry(*keys)
+            except (IndexError, KeyError):
+                pass

Again, there's too much in the try block. You don't want to catch errors from handle_duplicate_entry. Do this instead:
    try:
        dn_attr = dn[0].attr
    except (IndexError, KeyError):
        pass
    else:
        if dn_attr != self.obj.primary_key.name:
            self.obj.handle_duplicate_entry(*keys)



==== Minor issues ====

In ipalib/plugins/aci.py:
@@ -343,10 +341,12 @@ def _aci_to_kw(ldap, a, test=False, pkey_only=False):
             else:
                 # See if the target is a group. If so we set the
                 # targetgroup attr, otherwise we consider it a subtree
-                if api.env.container_group in target:
-                    targetdn = unicode(target.replace('ldap:///',''))
-                    target = DN(targetdn)
-                    kw['targetgroup'] = target['cn']
+                targetdn = DN(target.replace('ldap:///',''))
+                if api.env.container_group in targetdn:
+                    try:
+                        kw['targetgroup'] = targetdn[0]['cn']
+                    except (IndexError, KeyError):
+                        pass
                 else:
                     kw['subtree'] = unicode(target)

Why is (IndexError, KeyError) ignored?
Wouldn't be more correct to use endswith(DN(container_group, suffix)) instead of the `in` operator here, and in other places like this.



ipapython/dn.py:26
    LOCKED_ERROR_MSG = 'Object is locked, it cannot be modified: %s'
and then:
    raise TypeError(LOCKED_ERROR_MSG % (repr(self)))

You should define a TypeError subclass for this.
Using a string instead of a specialized object is exactly what this patch tries so hard to get rid of... :)



ipapython/entity.py:49:
    elif isinstance(entrydata, DN):
        self.dn = entrydata
        self.data = ipautil.CIDict()
    elif isinstance(entrydata, basestring):
        self.dn = DN(entrydata)
        self.data = ipautil.CIDict()

Should we not use DNs exclusively here? At least a TODO is warranted.

ipaserver/ipaldap.py:109:
    elif isinstance(entrydata,DN):
        self.dn = entrydata
        self.data = ipautil.CIDict()
    elif isinstance(entrydata,str) or isinstance(entrydata,unicode):
        self.dn = DN(entrydata)
        self.data = ipautil.CIDict()

Again, why not use DNs exclusively? At least put in a TODO.
For now at least do isinstance(entrydata, basestring) instead of the two isinstances.



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?

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.



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?



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.



ipaserver/plugins/ldap2.py:134:
    class ServerSchema(object):

Please don't use nested classes without reason.



==== Nitpicks/opinions ====

In ipa-replica-manage:369
>    sys.exit("winsync agreement already exists on subtree %s" %
please remove the trailing space



ipapython/dn.py:
    in docstring:  DN(arg0, ..., locked=False, first_key_match=True)
    followed by:  def __init__(self, *args, **kwds):
    and:  kwds.get('first_key_match', True)

I don't see the reason for this. Just write `def __init__(self, *args, locked=False, first_key_match=True)` and put a proper summary in the first line of the docstring. Same in AVA & RDN.



ipapython/ipautil.py:201:
-    """Convert a kerberos realm into the IPA suffix."""

Why are you removing a docstring?



ipaserver/plugins/ldap2.py:79:
    _debug_log_ldap = True
    if _debug_log_ldap:
        import pprint

Just import it unconditionally if you need it.



ipalib/plugins/baseldap.py:1874:
- sortfn=lambda x,y: cmp(x[1][self.obj.primary_key.name][0].lower(), y[1][self.obj.primary_key.name][0].lower())
+    sortfn=lambda x,y: cmp(x[1][self.obj.primary_key.name][0],
+        y[1][self.obj.primary_key.name][0])
    entries.sort(sortfn)

Since you're touching this: it's better (easier, faster, more readable) to use a key function. (Also, defining a named function is better with `def foo` than `foo=lambda...`):
    def sortkey(x):
        return x[1][self.obj.primary_key.name][0]
    entries.sort(key=sortkey)



ipapython/entity.py:95:
    # Python "internal" class attributes are prefixed and suffixed
      with double underscores.
    # Make sure we continue to return things like __str__ from
      this class.

I think a better solution would be making Entity a new-style class.
Or even better, killing Entity off altogether. Which is the plan, so this is a moot point.
Same with Entry in ipaserver/ipaldap.py.



ipaserver/install/ldapupdate.py:132:
+        assert isinstance(suffix, (None.__class__, DN))

type(x) is preferable to x.__class__ (the same way e.g. len(x) is preferable to x.__len__). However, I think it would be more readable to use:
    if suffix is not None:
        assert isinstance(suffix, DN)



ipaserver/plugins/ldap2.py:109:
def value_to_utf8(val):
    '''
    If val is not a string we need to convert it to a string
    (specifically a unicode string). Naively we might think we need to
    call str(val) to convert to a string. This is incorrect because if
    val is already a unicode object then str() will call
    encode(default_encoding) returning a str object encoded with
    default_encoding. But we don't want to apply the default_encoding!
    Rather we want to guarantee the val object has been converted to a
    unicode string because from a unicode string we want to explicitly
    encode to a str using our desired encoding (utf-8 in this case).

    Note: calling unicode on a unicode object simply returns the exact
    same object (with it's ref count incremented). This means calling
    unicode on a unicode object is effectively a no-op, thus it's not
    inefficient.
    '''
That is not an explanation of *what* the function does, but *how* it does it. Thus it should be in a comment, not in a docstring.

Also, if something needs such an explanation, it also need unit tests to ensure it actually works correctly.

IPASimpleLDAPObject has the same issue -- lengthy descriptions of design decisions should go in a comment (if not in a commit message or on the mailing list).



> Require that every place a dn is used it must be a DN object. This
> translates into lot of `assert isinstance(dn, DN)` sprinkled through
> out the code.

That seems like a crutch to get your DN work done. We should remove these once the dust settles.



> 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__).



> I equate Python decorators with C preprocessor hell when CPP macros
> are abused.

I do not share that view. It is possible to abuse decorators, but not all of their use is necessarily evil.
It's true that encode.py wasn't a good use of them.



> Removed all usage of get_schema() which was being called prior to
> accessing the .schema attribute of an object. If a class is using
> internal lazy loading as an optimization it's not right to require
> users of the interface to be aware of internal optimization's.
> Instead the class should handle it's lazy loading completely
> internally by loading the data on first access.
> This is very easy to do with class properties. schema is now a
> property of the class instead of an attribute. When the schema
> property is accessed it actually calls a private internal method to
> perform the lazy loading.

In other words, you're reinventing the reify decorator:
https://github.com/Pylons/pyramid/blob/master/pyramid/decorator.py#L1



Being a grammar nazi, I must point out the it's/its confusion and plurals formed with apostrophes.



====

Thanks for the work! This will really make IPA better when it's polished a bit. Hopefully all the criticism doesn't sound too negative :)

--
PetrĀ³

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

Reply via email to