You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cayenne.apache.org by Kevin Menard <km...@servprise.com> on 2008/03/28 03:45:43 UTC
Cleaning up inheritance tests
Hi Andrus,
I just reverted the data map to the old structure and added the new entities
necessary for testing. I agree that this is much cleaner now.
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?
Thanks,
Kevin
On 3/16/08 6:32 PM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
>
> On Mar 17, 2008, at 12:06 AM, Kevin Menard wrote:
>
>> I was also trying to avoid entity overload for the testing.
>
> But I think that's what sort of happened in the quoted commits?
>
>> The test cases
>> are very much structured around being able to load a particular
>> DataMap, so
>> coming up with a new one means a new test case as well. Not
>> necessarily a
>> bad thing, just explaining a bit more.
>
> I was thinking of adding new entities to the existing people DataMap.
> This should be good enough to separate the test cases. Also we don't
> have to keep using meaningful entity names (Person, Address), just
> something like Entity1, Subentity1 should be ok. The only
> consideration in naming is to avoid using the word Test in the names,
> as this confuses Maven surefire sometimes.
>
> Andrus
>
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Mar 30, 2008, at 12:25 AM, Kevin Menard wrote:
> That's fine with me. I've just always been of the mindset that if I
> want
> something, I should step up to the plate. Clearly, though, you
> understand
> the architecture better than anyone else. Realizing that this
> likely wasn't
> a priority for you but is for me, do you want to come up with some
> sort of
> mini-roadmap? I was thinking of soliciting one from the list anyway
> so that
> we have an idea of when to release 3.0 M4.
For M4 I think we are ready to go with what we have on trunk now.
Andrus
Re: Cleaning up inheritance tests
Posted by Kevin Menard <km...@servprise.com>.
On 3/29/08 4:22 PM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
>
> On Mar 29, 2008, at 9:25 PM, Kevin Menard wrote:
>> Likewise, there may be a reason a user is mapping multiple
>> relationships,
>> and in that case, I'd call the term "redundant" pejorative.
>
> Ok, let's call them "overlapping" or something else. The important
> thing is what that means: there are two or more overlapping
> collections based on the same join condition. And this is a curse for
> object graph consistency. Users should create their own filter on top
> of a single most inclusive collection and stop mapping the overlapping
> ones ... or risk messed up object graph right away. I'd say this
> should be a warning in the modeler.
Sounds good. The modeler probably shouldn't auto-generate overlapping
relationships either.
> Ok, maybe to reduce the number of cases we need to analyze, why don't
> we stop this discussion, and work on reducing the number of runtime
> relationships created (hmm... I only see a single case: a to-many part
> of a 1..N), then see what harm is caused by the remaining ones.
Largely, that's what I was shooting for by CAY-1008 and CAY-1009. In the
process though, I think I've confounded the situation a bit. I do like the
approach though.
>
>> If we want to go down the path of allowing multiple reverse
>> relationships, I
>> can lead the work up. I don't want you to think I'm trying to shell
>> this
>> off on you. I just don't want to be making large architectural
>> changes
>> without someone else keeping me in check.
>
> Yes, in order for us to keep consistent architecture, I feel like I'd
> have to be involved anyways. I don't think it is good for anybody if
> we have a set of diverging architectural visions in one product.
> Sounding like a control freak, but I don't see any other way around.
That's fine with me. I've just always been of the mindset that if I want
something, I should step up to the plate. Clearly, though, you understand
the architecture better than anyone else. Realizing that this likely wasn't
a priority for you but is for me, do you want to come up with some sort of
mini-roadmap? I was thinking of soliciting one from the list anyway so that
we have an idea of when to release 3.0 M4.
--
Kevin
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Mar 29, 2008, at 9:25 PM, Kevin Menard wrote:
> Likewise, there may be a reason a user is mapping multiple
> relationships,
> and in that case, I'd call the term "redundant" pejorative.
Ok, let's call them "overlapping" or something else. The important
thing is what that means: there are two or more overlapping
collections based on the same join condition. And this is a curse for
object graph consistency. Users should create their own filter on top
of a single most inclusive collection and stop mapping the overlapping
ones ... or risk messed up object graph right away. I'd say this
should be a warning in the modeler.
> If Cayenne does not need the relationships that it is creating,
> however, those would be
> redundant. As near as I can tell, that is the case.
Ok, maybe to reduce the number of cases we need to analyze, why don't
we stop this discussion, and work on reducing the number of runtime
relationships created (hmm... I only see a single case: a to-many part
of a 1..N), then see what harm is caused by the remaining ones.
> If we want to go down the path of allowing multiple reverse
> relationships, I
> can lead the work up. I don't want you to think I'm trying to shell
> this
> off on you. I just don't want to be making large architectural
> changes
> without someone else keeping me in check.
Yes, in order for us to keep consistent architecture, I feel like I'd
have to be involved anyways. I don't think it is good for anybody if
we have a set of diverging architectural visions in one product.
Sounding like a control freak, but I don't see any other way around.
Andrus
Re: Cleaning up inheritance tests
Posted by Kevin Menard <km...@servprise.com>.
On 3/29/08 3:07 PM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
>
> On Mar 29, 2008, at 8:39 PM, Kevin Menard wrote:
>
>> I think we may have had a different understanding of what redundant
>> means.
>> I was thinking internal to Cayenne, not considering relationships
>> mapped by
>> the user as "redundant", even if they may in fact be.
>
> There's no difference in the runtime... Redundant is redundant, and we
> either know how to handle it or not.
Well, there is a difference. If you disallow redundant, fine, but if the
user never maps redundant relationships, but Cayenne creates them, that's a
problem.
Likewise, there may be a reason a user is mapping multiple relationships,
and in that case, I'd call the term "redundant" pejorative. If Cayenne does
not need the relationships that it is creating, however, those would be
redundant. As near as I can tell, that is the case.
Note a huge distinction, but I think there should be one from the user's
perspective and from the perspective of the runtime.
> Hmm... Bailing on runtime relationships (at least on the required
> variety) would result in us losing support for one-way to-many (and
> IIRC also one-way 1..1) which would be a very serious downgrade. I
> don't see any *simple* alternative to handle that functionality. But i
> we develop one, I'd say drop runtime ObjRelationships and only keep
> the Db.
I'll have to chew on this one. I don't think the relationships I'm looking
to cut out are useful to Cayenne at all though. Without them, my test cases
still work. The created relationships are never used by the user and I
don't think they're even useful to Cayenne.
Once again, their presence or lack thereof have no real bearing on the
larger issue of CAY-1008. Other than that if we disallow redundant
relationships as a fix for CAY-1008, then we shouldn't introduce them at
runtime.
> Until then that I'd rather change my earlier statement about
> "BaseEntity -> DirectToSubEntity -> SubEntity" relationships chain,
> and call it "implicitly redundant". This would be equivalent to the
> following rule:
>
> "All Cayenne ObjRelationships implicitly or explicitly require a
> single reverse relationship. If the mapping does not contain a reverse
> relationship, Cayenne creates one (invisible to the user) during
> runtime. If the mapping results in more than one relationship matching
> reverse relationship criteria, this will result in a runtime exception
> thrown".
>
> That's how I see the "lazy" option implementation.
Then my vote is a -1 on the lazy option. The rules here are overly complex.
As a user, I wouldn't expect a relationship that I either: a) explicitly
deleted, or b) never added, to be added by Cayenne and invalidate my graph.
Additionally, it would basically eliminate any relationships to subclasses,
which I think would call the utility of the inheritance system into question
altogether. Although, maybe not. I'm rather surprised I'm the first to run
into such issues that seem basic to me. Either I'm abusing inheritance by
using relationships to the subclasses, or others have gotten lucky, because
the presence of a problem depends on the iteration order of the
relationships.
If we want to go down the path of allowing multiple reverse relationships, I
can lead the work up. I don't want you to think I'm trying to shell this
off on you. I just don't want to be making large architectural changes
without someone else keeping me in check.
--
Kevin
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Mar 29, 2008, at 8:39 PM, Kevin Menard wrote:
> I think we may have had a different understanding of what redundant
> means.
> I was thinking internal to Cayenne, not considering relationships
> mapped by
> the user as "redundant", even if they may in fact be.
There's no difference in the runtime... Redundant is redundant, and we
either know how to handle it or not.
>> A lazy person in me votes for the later option. Or maybe not that
>> lazy? I always liked my object graph structure to avoid any
>> inconsistency (e.g. assume relationship cardinality that DB does not
>> enforce) and redundancy (e.g. RelatedEntity double relationships over
>> the same set of joins). So mentally I never allowed myself to map
>> anything like that, and never felt the need to do so. So maybe we
>> shouldn't enable this scenario, complicating the framework
>> significantly, just because we can?
>
> Yeap, this is the core of what I was getting at. One problem I see
> is that
> the autocreated runtime relationships affects the relationship
> cardinality
> and can be a real pain in the neck for the user to debug. Any error
> message
> would not match what the user has mapped and their non-intuitive
> naming
> would not lend to easy debugging. Please note I'm talking about
> runtime
> relationships created for base classes, as demonstrated in the test
> for
> CAY-1009.
Hmm... Bailing on runtime relationships (at least on the required
variety) would result in us losing support for one-way to-many (and
IIRC also one-way 1..1) which would be a very serious downgrade. I
don't see any *simple* alternative to handle that functionality. But i
we develop one, I'd say drop runtime ObjRelationships and only keep
the Db.
Until then that I'd rather change my earlier statement about
"BaseEntity -> DirectToSubEntity -> SubEntity" relationships chain,
and call it "implicitly redundant". This would be equivalent to the
following rule:
"All Cayenne ObjRelationships implicitly or explicitly require a
single reverse relationship. If the mapping does not contain a reverse
relationship, Cayenne creates one (invisible to the user) during
runtime. If the mapping results in more than one relationship matching
reverse relationship criteria, this will result in a runtime exception
thrown".
That's how I see the "lazy" option implementation.
Andrus
Re: Cleaning up inheritance tests
Posted by Kevin Menard <km...@servprise.com>.
On 3/29/08 1:33 PM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
>
> On Mar 29, 2008, at 3:16 PM, Kevin Menard wrote:
>
>> Essentially, what I'm seeing is that there is not a 1:1 correlation
>> between
>> relationships and their reverses.
>
> True. A very good point. From what I can tell this may be another way
> to tell what "redundant relationships" are.
I think we may have had a different understanding of what redundant means.
I was thinking internal to Cayenne, not considering relationships mapped by
the user as "redundant", even if they may in fact be.
> In the case described both relationships are equally valid reverse
> relationships. I wouldn't even call the situation ambiguous - it is
> just two clearly defined relationships instead of one like we assumed
> in the past.
>
> So where do we go from here? There are two ways: we either change the
> API to reflect 1:N natire of reverse relationship (and deal with it
> everywhere throught the framework, e.g by setting multiple reverse
> relationships inside CayenneDataObject, etc.) or disallow redundant
> mapping explicitly. Either way it is a good idea to acknowledge this
> scenario explicitly in some way, so that users know what they are
> dealing with.
Right. This was what I've been trying to convey since opening the two
issues. I'm glad we're both on the same page now.
> A lazy person in me votes for the later option. Or maybe not that
> lazy? I always liked my object graph structure to avoid any
> inconsistency (e.g. assume relationship cardinality that DB does not
> enforce) and redundancy (e.g. RelatedEntity double relationships over
> the same set of joins). So mentally I never allowed myself to map
> anything like that, and never felt the need to do so. So maybe we
> shouldn't enable this scenario, complicating the framework
> significantly, just because we can?
Yeap, this is the core of what I was getting at. One problem I see is that
the autocreated runtime relationships affects the relationship cardinality
and can be a real pain in the neck for the user to debug. Any error message
would not match what the user has mapped and their non-intuitive naming
would not lend to easy debugging. Please note I'm talking about runtime
relationships created for base classes, as demonstrated in the test for
CAY-1009.
Another way to consider this is how it would impact the work for vertical
and horizontal inheritance. Would we need to be able to support 1:N
relationships in that case? If so, we may as well add
getReverseRelationships() : List<Relationship> now to make that work
simpler.
--
Kevin
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Mar 29, 2008, at 3:16 PM, Kevin Menard wrote:
> Essentially, what I'm seeing is that there is not a 1:1 correlation
> between
> relationships and their reverses.
True. A very good point. From what I can tell this may be another way
to tell what "redundant relationships" are.
In the case described both relationships are equally valid reverse
relationships. I wouldn't even call the situation ambiguous - it is
just two clearly defined relationships instead of one like we assumed
in the past.
So where do we go from here? There are two ways: we either change the
API to reflect 1:N natire of reverse relationship (and deal with it
everywhere throught the framework, e.g by setting multiple reverse
relationships inside CayenneDataObject, etc.) or disallow redundant
mapping explicitly. Either way it is a good idea to acknowledge this
scenario explicitly in some way, so that users know what they are
dealing with.
A lazy person in me votes for the later option. Or maybe not that
lazy? I always liked my object graph structure to avoid any
inconsistency (e.g. assume relationship cardinality that DB does not
enforce) and redundancy (e.g. RelatedEntity double relationships over
the same set of joins). So mentally I never allowed myself to map
anything like that, and never felt the need to do so. So maybe we
shouldn't enable this scenario, complicating the framework
significantly, just because we can?
Andrus
Re: Cleaning up inheritance tests
Posted by Kevin Menard <km...@servprise.com>.
On 3/29/08 6:15 AM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
>> I think that part of the issue is that Cayenne seems to be
>> inconsistent in
>> how it handles this.
>>
>> A runtime relationship is not created if a subclass is missing a
>> relationship. ObjEntity will try to use the base class's
>> relationship if it
>> can.
>
> Why is that inconsistent? ObjEntity is a model of a Java class... All
> superclass fields and methods are first-class citizens (no pun
> intended) in a subclass... So a relationship owned by a superentity,
> is a relationship of every subentity. This logic of course applies to
> the *source* of the relationship, not the *target* (there is no
> relationship target inheritance). Or are you talking about something
> else?
I may very well be getting confused, but this is what I'm seeing:
sub.setToRelatedEntity(related);
This retrieves the relationship from BaseEntity to RelatedEntity. Some key
fields are:
dbRelationshipPath: relatedEntities
name: toRelatedEntity
sourceEntity: BaseEntity
targetEntityName: RelatedEntity
Now, there really are two candidate reverse relationships, depending on what
class it's being called from:
RelatedEntity -> BaseEntity:
dbRelationshipPath: entities
name: baseEntities
sourceEntity: RelatedEntity
targetEntityName: BaseEntity
and
RelatedEntity -> SubEntity:
dbRelationshipPath: entities
name: subEntities
sourceEntity: RelatedEntity
targetEntityName: SubEntity
What I'm seeing is one relationship being the reverse for two others. This
isn't readily apparent because the following line in
ObjRelationship#getReverseRelationship() stops any sort of polymorphism from
occurring:
if (rel.getTargetEntity() != src)
continue;
In this case, the src is always "BaseEntity" so the RelatedEntity ->
BaseEntity relationship is the one that's found and used. If that check is
removed, then both candidate relationships are found, but now we have an
issue of ambiguity.
What my previous notes indicate is that if instead of using the inherited
relationship with the base type set as the sourceEntity, we create a new
relationship for the subEntity using it as the sourceEntity value, the
previous check can stick. In this way, we'll always find the most specific
rather than the most general relationship. It does not serve to address the
fact of updating all parents, but I contend that it is easier to work bottom
up than top down, due to the unambiguous path that must be taken from a
subclass through all of its ancestors.
Essentially, what I'm seeing is that there is not a 1:1 correlation between
relationships and their reverses. Moreover, the calculation of reverse
relationship makes the distinction between source and target entities murky,
since the roles can swap quite easily, so relying on Java inheritance in one
case and independent relationships in another can be problematic.
Even if I'm still wrong, I hope this clarifies more, especially with regards
to why I feel like that conditional check is problematic. I've voiced that
in the past while looking at this issue and it just feels wrong to me.
Regards,
Kevin
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Mar 29, 2008, at 3:39 AM, Kevin Menard wrote:
>>
>> * As we discussed, relationships targeting subclass and superclass
>> are
>> two different things
>
> I think that part of the issue is that Cayenne seems to be
> inconsistent in
> how it handles this.
>
> A runtime relationship is not created if a subclass is missing a
> relationship. ObjEntity will try to use the base class's
> relationship if it
> can.
Why is that inconsistent? ObjEntity is a model of a Java class... All
superclass fields and methods are first-class citizens (no pun
intended) in a subclass... So a relationship owned by a superentity,
is a relationship of every subentity. This logic of course applies to
the *source* of the relationship, not the *target* (there is no
relationship target inheritance). Or are you talking about something
else?
Andrus
Re: Cleaning up inheritance tests
Posted by Kevin Menard <km...@servprise.com>.
I'm just jotting some notes down as I go in case someone else wants to dig
in or can offer some insight. Apologies if it just seems like rambling.
On 3/28/08 10:10 AM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
>
> On Mar 28, 2008, at 4:03 PM, Kevin Menard wrote:
>
>> If I understand correctly, you'de rather see the null returned and let
>> EntityResolver investigate more itself at that point?
>
> Yes. Here is why:
>
> * As we discussed, relationships targeting subclass and superclass are
> two different things
I think that part of the issue is that Cayenne seems to be inconsistent in
how it handles this.
A runtime relationship is not created if a subclass is missing a
relationship. ObjEntity will try to use the base class's relationship if it
can. Locally I've tested creating a new relationship for the subclass on
the fly [1], using the subclass as the source entity, rather than the base
class.
While this doesn't fix the issue of CAY-1008, it gets a little closer I
think. Now there is a direct match found for the type of entity being
called. I.e., I have the following results:
RelatedEntity related = context.newObject(RelatedEntity.class);
BaseEntity base = context.newObject(BaseEntity.class);
base.setToRelatedEntity(related);
assertEquals(1, related.getBaseEntities().size()); // 1. PASS
assertEquals(0, related.getSubEntities().size()); // 2. PASS
SubEntity sub = context.newObject(SubEntity.class);
sub.setToRelatedEntity(related);
assertEquals(2, related.getBaseEntities().size()); // 3. FAIL
assertEquals(1, related.getSubEntities().size()); // 4. PASS
Prior to these modifications, tests #1, #2, #3 would pass, while #4 would
fail. So, the mod allows updating the subclass list, whereas previously
only the base class list would ever be modified. If we're able to propagate
the change back up the inheritance hierarchy, we may have something here.
--
Kevin
[1] The really messy mod is:
ObjEntity:
@Override
public Relationship getRelationship(String name) {
ObjRelationship relationship = (ObjRelationship)
super.getRelationship(name);
if (relationship != null) {
return relationship;
}
if (superEntityName == null) {
return null;
}
ObjEntity superEntity = getSuperEntity();
ObjRelationship superRelationship = (ObjRelationship)
superEntity.getRelationship(name);
if (superRelationship != null) {
ObjRelationship childRelationship = new ObjRelationship(name);
childRelationship.setCollectionType(superRelationship.getCollectionType());
childRelationship.setDbRelationshipPath(superRelationship.getDbRelationshipP
ath());
childRelationship.setDeleteRule(superRelationship.getDeleteRule());
childRelationship.setMapKey(superRelationship.getMapKey());
childRelationship.setParent(superRelationship.getParent());
childRelationship.setRuntime(superRelationship.isRuntime());
childRelationship.setSourceEntity(this);
childRelationship.setTargetEntity(superRelationship.getTargetEntity());
childRelationship.setTargetEntityName(superRelationship.getTargetEntityName(
));
childRelationship.setUsedForLocking(superRelationship.isUsedForLocking());
addRelationship(childRelationship);
return childRelationship;
}
return null;
}
Re: Cleaning up inheritance tests
Posted by Kevin Menard <km...@servprise.com>.
Gotcha. Thanks for the insights. Since this thing has been dogging me for
two weeks, I'll see if I can figure something out that can work with this
over the weekend.
Thanks,
Kevin
On 3/28/08 10:10 AM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
>
> On Mar 28, 2008, at 4:03 PM, Kevin Menard wrote:
>
>> If I understand correctly, you'de rather see the null returned and let
>> EntityResolver investigate more itself at that point?
>
> Yes. Here is why:
>
> * As we discussed, relationships targeting subclass and superclass are
> two different things
> * Aside from that, what we are interested in here is to figure whether
> creating reverse ObjRelationship is needed to maintain object graph
> consistency, which is entirely different logic.
>
> Andrus
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Mar 28, 2008, at 4:03 PM, Kevin Menard wrote:
> If I understand correctly, you'de rather see the null returned and let
> EntityResolver investigate more itself at that point?
Yes. Here is why:
* As we discussed, relationships targeting subclass and superclass are
two different things
* Aside from that, what we are interested in here is to figure whether
creating reverse ObjRelationship is needed to maintain object graph
consistency, which is entirely different logic.
Andrus
Re: Cleaning up inheritance tests
Posted by Kevin Menard <km...@servprise.com>.
On 3/28/08 9:43 AM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
> It changed 'getReverseRelationship' which is a generic method not
> specifically related to runtime relationships logic (and I must admit
> I still don't fully understand the logic in the fix..) Anyways, what
> I'm saying is that the caller of this method should be responsible for
> making a decision whether a runtime relationship makes sense or not,
> not the method itself.
The execution may have been off (removing the condition altogether seems to
accomplish the same thing), but the general idea is to not do direct type
checking. The old check was the following:
if (rel.getTargetEntity() != src)
continue;
But, elsewhere entity matching is done based on a deep attribute matching.
BaseEntity and SubEntity essentially have the same structure and the latter
may be used in place of the former. The aforementioned check, however,
would explicitly disallow this.
The result is that the method would return null, causing EntityResolver to
determine that a runtime relationship should be created.
What I intended is that if an exact match could not be made, don't bail out
yet, because a subclass might be able to fulfill the obligation. In that
case, a non-null value would be returned and EntityResolver would at that
point decide not to create a runtime relationship.
If I understand correctly, you'de rather see the null returned and let
EntityResolver investigate more itself at that point?
--
Kevin
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Mar 28, 2008, at 3:36 PM, Kevin Menard wrote:
>>
>> Question... Once we do that, would that mean that we only solved 50%
>> of the broken cases? Mapping cases showing remaining problems are
>> welcomed.
>
> I know that the patch I provided did fix a subset of cases for me.
> It did
> allow the test case for CAY-1009 to pass. It also allowed tests in
> my own
> app that's been blocked on the issue to pass. The test case for
> CAY-1008
> still fails though, so it clearly wouldn't address the problem
> globally.
It changed 'getReverseRelationship' which is a generic method not
specifically related to runtime relationships logic (and I must admit
I still don't fully understand the logic in the fix..) Anyways, what
I'm saying is that the caller of this method should be responsible for
making a decision whether a runtime relationship makes sense or not,
not the method itself.
Andrus
Re: Cleaning up inheritance tests
Posted by Kevin Menard <km...@servprise.com>.
On 3/28/08 8:47 AM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
>> One such "fix" is to disallow explicit mappings to base and sub
>> classes and
>> replace it with some other mechanism.
>
> *simultaneous* mapping that is
Correct. The conjunction there was ambiguous.
>
>> 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.
>
> Without runtime relationships in the picture this relationship loop is
> "non-redundant" I guess:
>
> BaseEntity -> DirectToSubEntity -> SubEntity
>
> This brings me to a potential runtime relationships optimization -
> avoid creation of runtime relationships that are not needed to set a
> correct FK (e.g. in a Artist -> Paintings case, toArtist is a required
> runtime relationship to set PAINTING.ARTIST_ID, while "paintingArray"
> is not). So we can probably narrow down the field to just the
> necessary runtime rels.
I think this is consistent with what I was trying to convey. Apologies if
not.
> Question... Once we do that, would that mean that we only solved 50%
> of the broken cases? Mapping cases showing remaining problems are
> welcomed.
I know that the patch I provided did fix a subset of cases for me. It did
allow the test case for CAY-1009 to pass. It also allowed tests in my own
app that's been blocked on the issue to pass. The test case for CAY-1008
still fails though, so it clearly wouldn't address the problem globally.
I think, however, that such an optimization fixes all cases except those
where there is an explicit simultaneous mapping to both base and subclasses.
My tests have mappings to two sibling classes and trimming out the runtime
parent relationship allows them to pass, so your hunch seems to be on with
regard to siblings.
At that point, if we've managed to address all cases except explicitly
mapped relationships to both the base and subclasses, is it something worth
addressing in the framework? Should we just say use the base class and cast
as appropriate? Or, if you don't want to cast, use a qualified query? Or,
should we try to fix it in Cayenne internally? I guess the last one would
be the least controversial, but I'm having a hard time thinking of a case
where it would be valid to map to both the base and subclasses at the same
time. My suspicion is that this happens more accidentally than anything
else. But, I am aware that the modeler populates such relationships itself,
so there is a notion of it being valid . . .
I'll have to chew on other mapping cases. I've only run across the problems
I have in a real development environment. So, I hadn't put much academic
thought into it yet.
--
Kevin
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Mar 28, 2008, at 2:01 PM, Kevin Menard wrote:
> 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.
*simultaneous* mapping that is
> 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.
Without runtime relationships in the picture this relationship loop is
"non-redundant" I guess:
BaseEntity -> DirectToSubEntity -> SubEntity
This brings me to a potential runtime relationships optimization -
avoid creation of runtime relationships that are not needed to set a
correct FK (e.g. in a Artist -> Paintings case, toArtist is a required
runtime relationship to set PAINTING.ARTIST_ID, while "paintingArray"
is not). So we can probably narrow down the field to just the
necessary runtime rels.
Question... Once we do that, would that mean that we only solved 50%
of the broken cases? Mapping cases showing remaining problems are
welcomed.
Andrus
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Mar 28, 2008, at 3:25 PM, Kevin Menard wrote:
> Okay. I've always viewed them as just a way to not have to specify
> the
> other half of a mapped relationship. The fact that new ones were
> being created for relationships I never intended on using was news
> to me
It's sort of all of the above:
1. For DbRelationships (which themselves are really an artificial
construct, not exposed in the code in 99.9% of cases) it is a
convenience shortcut.
2. For ObjRelationships it is a way to allow users to choose the
object model that makes sense to them, instead of forcing mandatory bi-
directional relationships.
Andrus
Re: Cleaning up inheritance tests
Posted by Kevin Menard <km...@servprise.com>.
On 3/28/08 8:30 AM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
> That's the main area of disagreement. Essentially you are saying that
> runtime relationships are harmful and not needed in Cayenne at all. My
> point is that they were introduced exactly to allow users to remove
> explicit relationships whenever they please (the original motivation
> for runtime relationships was to enable one-way to-many). In other
> words runtime relationships are there for a reason and should be
> considered an internal artifact of Cayenne and users shouldn't be
> bothered about their presence (as long as everything works as
> advertised).
Okay. I've always viewed them as just a way to not have to specify the
other half of a mapped relationship. The fact that new ones were being
created for relationships I never intended on using was news to me. Having
said that, if I never ran into a problem, it was likely something I was
never going to discover anyway.
> So we should separate cases where runtime relationships are the cause
> of the problem vs. cases where the problem is elsewhere.
Agreed.
> So regarding runtime relationships... Maybe we should write targeted
> unit tests to demonstrate delete rules and/or validation problems they
> may cause?
I've not come across a case where delete rules weren't followed for both
based and subclasses. That was actually what I thought was going wrong
initially. Then I discovered it was just two different relationships where
one was ignored and the delete rules were followed on the other.
I suppose tests that verify that behavior wouldn't be a terrible idea
though.
--
Kevin
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
On Mar 28, 2008, at 2:01 PM, Kevin Menard wrote:
> 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.
That's the main area of disagreement. Essentially you are saying that
runtime relationships are harmful and not needed in Cayenne at all. My
point is that they were introduced exactly to allow users to remove
explicit relationships whenever they please (the original motivation
for runtime relationships was to enable one-way to-many). In other
words runtime relationships are there for a reason and should be
considered an internal artifact of Cayenne and users shouldn't be
bothered about their presence (as long as everything works as
advertised).
So we should separate cases where runtime relationships are the cause
of the problem vs. cases where the problem is elsewhere.
So regarding runtime relationships... Maybe we should write targeted
unit tests to demonstrate delete rules and/or validation problems they
may cause?
Andrus
Re: Cleaning up inheritance tests
Posted by Kevin Menard <km...@servprise.com>.
On 3/28/08 5:23 AM, "Andrus Adamchik" <an...@objectstyle.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
Re: Cleaning up inheritance tests
Posted by Andrus Adamchik <an...@objectstyle.org>.
Cool. With the new model, it is much more clear what mapping we are
talking about... Here it is for other's reference:
http://svn.apache.org/repos/asf/cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/src/test/resources/people.map.xml
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.
> 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.
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).
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.
Andrus