Re: CAY-1378, CAY-1009...

From: Kevin Menard (nirvdru..mail.com)
Date: Wed Feb 10 2010 - 16:20:23 EST

  • Next message: Andrus Adamchik: "Re: CAY-1378, CAY-1009..."

    On Wed, Feb 10, 2010 at 3:50 PM, Andrus Adamchik <andru..bjectstyle.org> wrote:
    >
    > On Feb 8, 2010, at 3:39 PM, Kevin Menard wrote:
    >
    >>
    >> I'm reaching here a bit since it's been a while since I looked at this
    >> problem, but I think the semantics were preserved because isSubentityOf
    >> wasn't a proper subentity relationship.  I.e., an entity could be a
    >> subentity of itself.  So, while the code looks strange, it worked.
    >
    >
    > -            if (rel.getTargetEntity() != src)
    > +            if (target.isSubentityOf((ObjEntity) rel.getTargetEntity())) {
    >                 continue;
    > +            }
    >
    > In the past we'd give up on a given relationship as a potential candidate
    > for a reverse relationship based on the fact that its *target* entity is not
    > the same as forward relationship *source*.
    >
    > The new code compares inheritance hierarchy of the forward relationship
    > *target*, and reverse candidate relationship *target*. Apples and oranges.
    > Seems like the "if" check would *always* fail here except for
    > self-referential relationships? So the check itself is not doing anything,
    > and simply lets the following code to execute unconditionally.

    I'll have to dig up the project this was a problem in and run through
    it again. I recall spending about two days in a debugger ensuring the
    semantics were upheld. But that was a year and a half ago and I'm
    hazy on how it all works. If your analysis is correct, the patch at
    the very least should have caused a failure somewhere in the test
    suite.

    > And the result is that further analysis is done based on the DbRelationship
    > path, and relationships starting/terminating at different subentities of the
    > same inheritance hierarchy as picked as a forward/reverse pair (which IMO is
    > incorrect until we implement a more sophisticated matching algorithm as
    > discussed).

    Sure, but we could address the most common case without having to do a
    rewrite for the more general case, IMHO. I don't think the STI case I
    outlined is really that esoteric a mapping.

    > So based on that conclusion I am going to revert the CAY-1009 patch (and
    > also commit CAY-1378 tests).

    I'd caution that the change is going to break things for existing
    users. I left the patch as is for peer review and then users reported
    on the list that it worked in their scenarios. Reverting that is
    undoubtedly going to break code they already have working and it's
    going to do so in an RC. So it seems to me like we're just trading
    off solving one problem for another.

    -- 
    Kevin
    



    This archive was generated by hypermail 2.0.0 : Wed Feb 10 2010 - 16:21:14 EST