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