[Zope-Coders] SimpleItem.getId busted

[Casey Duncan]

On Thu, 20 Nov 2003 16:42:11 +1100
Stuart Bishop <stuart at stuartbishop.net> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> I'd like to change:
> 
>      def getId(self):
>          """Return the id of the object as a string. This method
>             should be used in preference to accessing an id attribute
>             of an object directly. The getId method is public."""
>          name=getattr(self, 'id', None)
>          if callable(name):
>              return name()
>          if name is not None:
>              return name
>          if hasattr(self, '__name__'):
>              return self.__name__
>          raise AttributeError, 'This object has no id'
> 
> to:
> 
> 	def getId(self):
>          """Return the id of the object as a string. This method
>             should be used in preference to accessing an id attribute
>             of an object directly. The getId method is public."""
> 		name = getattr(aq_base(self), 'id', None)
> 		if callable(name):
> 			return name()
> 		if name:
> 			return name
>         try:
>             return self.__name__
>         except AttributeError:
>             raise AttributeError, 'This object has no id'

Your version changes the semantics if id happens to be false. I think getId should return the value of id if it is not None, regardless of what it is.

The following might be safer (due to vagarities of the "callable" function with ExtensionClass instances). Also getattr(self, name, None) can be dangerous because it can mask read conflict errors. They would be very unlikely here, but probably not impossible. I'm not sure for example if it would be possible for self to be ghostified when getId is entered. It is definitely possible for a persistent object stored as "id" to be a ghost upon entry (however unlikely that senario may seem).

def getId(self):
    try:
        name = aq_base(self).id
    except AttributeError:
        try:
            return self.__name__
        except AttributeError:
            raise AttributeError, 'This object has no name'
    else:
        try:
            return name()
        except TypeError:
            return name

The complexity of this code is mostly due to the fact that id is sometimes a callable, which only worked because of DTML's implicit call semantics. Why they didn't just use a ComputedAttribute, I don't know 8^/

I'm not especially fond of nested try's, but is does avoid doing a bunch of tests that might always do the right thing. There is a subtle semantic difference between this code and the original, though. This code will return None if id itself is None, whereas the original could only return None if name() returned None or __name__ was None.

A potential weakness to my version is if calling name raises a TypeError during the call (rather than because it is not callable), that exception will be masked. If that is objectionable, the safe_callable function could be used instead of the try; except TypeError:
    
> This makes the getId() method actually work for PageTemplateFile's,
> and all the methods that rely on it like absolute_url().
> It is sort-of backward compatible, as people doing
> string:${template/absolute_url}/myid to work around this bug will
> still get valid links thanks to aquisition, but it aint pretty.

Can you explain why this works for PTFs because they define __name__ and not id? Also, why are you using PTFs and not skins? ;^)

> Any objections to me fixing the HEAD?
> 
> What branches should this be back ported to? This goes back to at least
> 2.6 and possibly further to when __name__ was first introduced.

I would suggest a smaller delta on previous release branches. Perhaps just changing the first line to name=getattr(aq_base(self), 'id', None) would be sufficient?

Actually __name__ is quite old (although perhaps not on PTFs). getId is more recent, but it definitely goes back further than 2.6. Fixing versions earlier than 2.6 though is probably not very important.

Before this gets checked in I would strongly suggest adding unittests that exercise all of the desired behavior. Otherwise its bound to get unwittingly broken at some future date.

-Casey