Make sure to call each invariant only once when validating invariants.

[janjaapdriessen]

When an invariant is defined in an interface, it's found by validateInvariants in all interfaces inheriting from that interface.

[jamadden]

Thanks for this!

[jamadden]

Thanks for considering that! I see your new implementation and I agree it looks nice and clean!

While reviewing it, I had another thought though: What if the body was changed from self.queryTaggedValue() to self.queryDirectTaggedValue()? That would be only change to the entirety of the original function.

queryTaggedValue returns the first value seen when walking up the __iro__. That could result in the same value being seen multiple times if the tag is defined somewhere up the __iro__ from the current object: the problem this PR is preventing.

queryDirectTaggedValue, OTOH, looks only and exactly for a value defined in this exact instance. That would eliminate the entire possibility of finding duplicates through the inheritance hierarchy.

As this function proceeds to walk up __bases__ it should ultimately always find everything included in __iro__….

But here's the problem with that: it might actually find them multiple times:

>>> from zope.interface import Interface
>>> class I1(Interface):
...     pass
...
>>> class I2(I1):
...     pass
...
>>> class I11(I1): 
…     pass
…
>>> class I3(I11, I2):
...     pass
...
>>> I3.__iro__
(<InterfaceClass __main__.I3>,
 <InterfaceClass __main__.I11>,
 <InterfaceClass __main__.I2>,
 <InterfaceClass __main__.I1>,
 <InterfaceClass zope.interface.Interface>)
>>> def visit(s, pfx=''):
...     print(pfx, s)
...     for b in s.__bases__:
...         visit(b, pfx + ' ')
...
>>> visit(I3)
 <InterfaceClass __main__.I3>
  <InterfaceClass __main__.I11>
   <InterfaceClass __main__.I1>
    <InterfaceClass zope.interface.Interface>
  <InterfaceClass __main__.I2>
   <InterfaceClass __main__.I1>
    <InterfaceClass zope.interface.Interface>

Actually, thinking more about this, I find this method very confusing as it currently exists. It uses __iro__ to find invariants, but it walks up __bases__. Since this is InterfaceClass, __bases__ should only consist of other interfaces (not specifications or anything like that), like __iro__. One consequence of this oddity is that the method is O(n^2) due to the hidden loop in queryTaggedValue; also, recursion isn't especially fast in Python.

What if the method was made non-recursive using __iro__ and queryDirectTaggedValue directly?

def validateInvariants(self, obj, errors=None):
    # pseudo-code
    for iface in self.__iro__: # Iterate starting with self
        for invariant in iface.queryDirectTaggedValue('invariants', ()):
            try:
                invariant(obj)
            except Invalid:
                 if errors is not None: errors.append(e)
                 else: raise
    if errors: raise Invalid(errors)

That seems a lot simpler and more direct. It eliminates the hashing of the PR. It still has the same issue with people that have tried to override validateInvariants as the PR, but in reality that's probably a non-issue.

queryDirectTaggedValue is relatively new, so that might explain why it wasn't written this way to start with. It was added specifically because using __bases__ can result in inconsistencies.

Thoughts? What have I missed?

[janjaapdriessen]

The implementation using queryDirectTaggedValue looks smooth, all tests pass, and the issue in my codebase is fixed. I am happy with this fix, thanks for you help @jamadden!

[jamadden]

LGTM! Thanks for being willing to iterate!

[janjaapdriessen]

Would you mind making a pypi release so I can tie up loose ends at my end? Thanks!

[jamadden]

Done! And thank you! I haven't benchmarked but for deep interface trees I expect this also results in a small performance boost too.