Re: Cleaning up inheritance tests

From: Kevin Menard (kmenar..ervprise.com)
Date: Fri Mar 28 2008 - 08:01:30 EDT

  • Next message: Thaminda Karunanayake: "Re: More Information about the"

    On 3/28/08 5:23 AM, "Andrus Adamchik" <andru..bjectstyle.org> wrote:

    > On Mar 28, 2008, at 4:45 AM, Kevin Menard wrote:
    >> I've also added a patch to CAY-1009 that I think fixes the problem.
    >> If you
    >> get an opportunity, could you please review it?
    >
    >
    >> if (target.isSubentityOf((ObjEntity) rel.getTargetEntity()))
    >> continue;
    >
    > I am confused about this... 'rel.getTargetEntity()' should be compared
    > with "src", not "target"? But see below, I am unsure about the patch
    > in general.

    It's just a short-circuit test. It says that I the current target entity is
    a super-class of the mapped target entity, then bail out. There's no reason
    to add the relationship because we've already dealt with the explicitly
    mapped subclass relationship.

    >
    >> assertEquals(1,
    >> context
    >> .getEntityResolver
    >> ().getObjEntity("DirectToSubEntity").getRelationships().size());
    >
    >
    > I think the assertion above is wrong. "Runtime" relationship from
    > DirectToSubEntity to BaseEntity is entirely correct, as it completes
    > the mapping graph created by the user.

    I don't know about that one. While the overall mapping structure is what
    the user designed, in this case, the user explicitly removed (or never
    added) the relationship to the base class. In that case, I'd argue that the
    use didn't want the relationship mapped. That was certainly the case for
    me.
     
    > So my take on that is that Cayenne correctly detects reverse
    > relationships, however it is not designed to handle "redundant"
    > relationships. The criteria of "redundant relationships" are these
    > (somewhat similar to your patch idea, as you may see):
    >
    > 1. Two or more relationships that are mapped over the same
    > DbRelationship
    >
    > 2. Doesn't matter whether the relationships are explicit (mapped by
    > the user) or implicit (added in runtime). Your mapping of
    > RelatedEntity vs. DirectToSubEntity demonstrates that there is no
    > difference whether the user or framework added a redundant
    > relationship, things are still messed up.
    >
    > 3. Sources and/or targets overlap in the inheritance hierarchy (i.e.
    > if we target two inheritance leaves, I expect things to work ok; if we
    > target subclass and superclass, these are redundant).

    I agree.

    > So essentially avoiding runtime relationship creation does not solve
    > the problem of redundant relationships support (as user can map those
    > explicitly just as easy). I suggest a validation message instead of
    > changing 'getReverseRelationship'... or something deeper than that,
    > like a special redundant relationship handler that is aware of this
    > scenario.

    Right. I think CAY-1008 and CAY-1009 need to be treated together. They're
    separate issues, but closely related. I further concede that fixing
    CAY-1008 would address the symptoms of CAY-1009. I still think CAY-1009 is
    an issue though. If as a user I never intend to use a relationship and
    Cayenne can detect as much, why bother adding runtime relationships that are
    just going to incur extra, unnecessary overhead?

    Taking it a step further, I don't think we actually had agreed how to fix
    CAY-1008. As you've probably seen, it's a tricky one because changing
    getReverseRelationship is not all that feasible.

    One such "fix" is to disallow explicit mappings to base and sub classes and
    replace it with some other mechanism. Mapped queries with criteria could be
    one such solution. In this case, I would argue that the test case for
    CAY-1008 has such an invalid mapping, but that the mapping for the CAY-1009
    test case does not match the criteria.

    Thoughts?

    -- 
    Kevin
    



    This archive was generated by hypermail 2.0.0 : Fri Mar 28 2008 - 08:02:05 EDT