Re: CAY-1378, CAY-1009...
On Wed, Feb 10, 2010 at 4:52 PM, Andrus Adamchik wrote: > The test suite is not ideal. In fact it barely touches runtime > relationships, working with bi-directional relationship graphs in most > cases. We need to improve it, no question. > >> I still really don't feel like I mapped my STI relationship incorrectly. >> It's the most natural way to do it. > > The way you described it in this thread, it is indeed. And the thing is - in > my tests this type of mapping works without CAY-1009 patch. So I wonder if > the the actual failing mapping has some details not mentioned here. I'd > appreciate if you dig up that failing mapping, so that we could inspect it. I just updated CAY-1009 with the DataMap that was causing me problems. I haven't had an opportunity to rebuild that project with the latest cayenne to see if the problem reappears. I hope to be able to do that shortly, but I haven't built that project in over a year. -- Kevin
Re: CAY-1378, CAY-1009...
On Feb 10, 2010, at 11:35 PM, Kevin Menard wrote: If your analysis is correct, the patch at the very least should have caused a failure somewhere in the test suite. The test suite is not ideal. In fact it barely touches runtime relationships, working with bi-directional relationship graphs in most cases. We need to improve it, no question. I still really don't feel like I mapped my STI relationship incorrectly. It's the most natural way to do it. The way you described it in this thread, it is indeed. And the thing is - in my tests this type of mapping works without CAY-1009 patch. So I wonder if the the actual failing mapping has some details not mentioned here. I'd appreciate if you dig up that failing mapping, so that we could inspect it. For now I am going to commit my fix, reverting CAY-1009 + some minor refactoring + CAY-1378 test case. Since the controversial piece is just 3 lines of code in ObjRelationship, it should be easy to redo it one more time if we find a reason to do so. Andrus
Re: CAY-1378, CAY-1009...
On Wed, Feb 10, 2010 at 4:33 PM, Andrus Adamchik wrote: > If we correctly identified the scenario, the fix for the users would be to > map their relationships correctly. I was going to add a note to the docs to > that extent. I still really don't feel like I mapped my STI relationship incorrectly. It's the most natural way to do it. I haven't evaluated the other case you outlined yet though. My brain has been too fried to work out the abstract relationships you were talking about. -- Kevin
Re: CAY-1378, CAY-1009...
On Feb 10, 2010, at 11:20 PM, Kevin Menard wrote: 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. If we correctly identified the scenario, the fix for the users would be to map their relationships correctly. I was going to add a note to the docs to that extent. Andrus
Re: CAY-1378, CAY-1009...
On Wed, Feb 10, 2010 at 3:50 PM, Andrus Adamchik 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
Re: CAY-1378, CAY-1009...
On Feb 10, 2010, at 10:50 PM, Andrus Adamchik wrote: and also commit CAY-1378 tests Also CAY-1378 demonstrates the failures caused by self-referential relationships, for which valid reverse relationships are incorrectly discarded. Andrus
Re: CAY-1378, CAY-1009...
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.
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).
So based on that conclusion I am going to revert the CAY-1009 patch
(and also commit CAY-1378 tests).
Andrus
Re: CAY-1378, CAY-1009...
On Feb 10, 2010, at 2:44 PM, Kevin Menard wrote: I mapped the relationship for the one subclass that needed it because it was the only one that needed it. While I could have mapped it at the superclass level, all other siblings would then have the method, which would be logically invalid. This all sounds correct and this works as far as I can tell. The only way I can reproduce the problem is if there is a user-mapped, not runtime, reverse relationship connected to a superclass, while the forward relationship is connected to a subclass (or vice versa). I.e. (A -> C ; C -> B) is a bad combination, but just (C -> B) without an explicit (A -> C) works ok. I.e. runtime relationships help you avoid reverse relationships, unless an incorrect cross-hierarchy mapping is present. Andrus
Re: CAY-1378, CAY-1009...
On Wed, Feb 10, 2010 at 3:53 AM, Andrus Adamchik wrote: > > Now back to 3.0... Could you explain why there is a mismatch in the mapping? > I.e. why can't you remap (A -> C ; C -> B) as either (A -> C ; C -> A) or (B > -> C ; C -> B) from the application design perspective? I don't think this was my case, but the reason I mapped the way I did is that 11 out of 12 columns were in common between several classes, so I used STI. I mapped the relationship for the one subclass that needed it because it was the only one that needed it. While I could have mapped it at the superclass level, all other siblings would then have the method, which would be logically invalid. Additionally, I couldn't reasonably enforce a mandatory constraint. At the time I also looked into having Cayenne not create runtime relationships that it didn't need to . . . after all, this one is mapped. But I ran into much larger obstacles when doing that, so I decided to try to figure out how to work with them. -- Kevin
Re: CAY-1378, CAY-1009...
On Feb 10, 2010, at 11:27 AM, Andrey Razumovsky wrote: However, I think I could have called addToRelated() programmically without knowing name of relationship... Not sure though I just tried with a one-way to-many relationship, and that works, as runtime relationships are added correctly if there's no explicit sub/ superclass relationship mismatch. Anyways, I'm ok with reverting CAY-1009 but reopening it for 3.1 I will do that. I think there is a clear workaround for all affected users (and I still don't understand why people use the type of mapping that is causing the problem. It doesn't look logically correct to me). Andrus
Re: CAY-1378, CAY-1009...
Anyways, I'm ok with reverting CAY-1009 but reopening it for 3.1 2010/2/10 Andrey Razumovsky > However, I think I could have called addToRelated() programmically without > knowing name of relationship... Not sure though > > 2010/2/10 Andrey Razumovsky > > >>> >>> Now back to 3.0... Could you explain why there is a mismatch in the >>> mapping? I.e. why can't you remap (A -> C ; C -> B) as either (A -> C ; C -> >>> A) or (B -> C ; C -> B) from the application design perspective? >>> >>> Andrus >>> >> >> In my case I don't have obj relationships A->C or B->C at all because I >> don't need them. Only C->B, others are runtime > > > > > -- > Andrey > -- Andrey
Re: CAY-1378, CAY-1009...
However, I think I could have called addToRelated() programmically without knowing name of relationship... Not sure though 2010/2/10 Andrey Razumovsky > >> >> Now back to 3.0... Could you explain why there is a mismatch in the >> mapping? I.e. why can't you remap (A -> C ; C -> B) as either (A -> C ; C -> >> A) or (B -> C ; C -> B) from the application design perspective? >> >> Andrus >> > > In my case I don't have obj relationships A->C or B->C at all because I > don't need them. Only C->B, others are runtime -- Andrey
Re: CAY-1378, CAY-1009...
> > > > Now back to 3.0... Could you explain why there is a mismatch in the > mapping? I.e. why can't you remap (A -> C ; C -> B) as either (A -> C ; C -> > A) or (B -> C ; C -> B) from the application design perspective? > > Andrus > In my case I don't have obj relationships A->C or B->C at all because I don't need them. Only C->B, others are runtime
Re: CAY-1378, CAY-1009...
On Feb 9, 2010, at 2:11 PM, Andrey Razumovsky wrote: Correcting my case, there's relationship between B and C, not A and C. If I set B -> C and C -> B relationships, it works with CAY-1009 reverted. It fails if there is a mismatch between forward and reverse relationships like this: A -> C ; C -> B. This is indeed same as Kevin's case, and I just uploaded another patch demonstrating it. The validation error happens if the object is added for to-many: b.addToRelated(c); And this comes down to my earlier comment - if we are to handle multiple permutations of object relationships over the same db path, we need to rewrite the algorithm for reverse lookup (and maybe even add other matching forward relationships to the mix during auto-update). I.e. this is not something we can do in 3.0 at this point, but definitely something to consider in 3.1. Now back to 3.0... Could you explain why there is a mismatch in the mapping? I.e. why can't you remap (A -> C ; C -> B) as either (A -> C ; C -> A) or (B -> C ; C -> B) from the application design perspective? Andrus
Re: CAY-1378, CAY-1009...
On Feb 9, 2010, at 2:11 PM, Andrey Razumovsky wrote: I've reverted CAY-1009 and reproduced problem in one of my projects. It's exactly as in Kevin's scenario. Correcting my case, there's relationship between B and C, not A and C. I'll try to take a look at it tonight. Could you upload your patch in svn format? I'm afraid I don't have git AFAIK it should still be compatible with patch command and hopefully Eclipse (??). It has some extra headers that are simply ignored when patching. Andrus
Re: CAY-1378, CAY-1009...
I've reverted CAY-1009 and reproduced problem in one of my projects. It's exactly as in Kevin's scenario. Correcting my case, there's relationship between B and C, not A and C. Could you upload your patch in svn format? I'm afraid I don't have git 2010/2/7 Andrus Adamchik > > On Feb 7, 2010, at 5:36 PM, Andrey Razumovsky wrote: > > As far as i remember, the problem is if we have a mapping: >> entities: >> A >> B extends A - mapped in same db table >> C >> >> dbRel: >> toA, cArray (from C to A and vice versa) - Is Mandatory >> >> objRel - toA, cArray (from C to A and vice versa) >> >> So, there's isn't any ObjRel from B to C and therefore Cayenne adds >> runtime >> relationship. >> Problems come when we're setting relationship via C.setToA(..) and commit. >> Cayenne thinks runtime rel from C to B is mandatory (after all, it's >> mapped >> to mandatory dbRel) and fails to commit. >> > > Created a patch from this description against 3.0 branch: > > > https://issues.apache.org/jira/secure/attachment/12435122/CAY-1009-Andreys-case.patch > > A == Sti2Inheritance > B == Sti2InheritanceSub > C == Sti2Related > > I still can't reproduce the failure, as there's no runtime relationship > created between B and C, either with or without CAY-1009 commit. > > Andrus > -- Andrey
Re: CAY-1378, CAY-1009...
On Sun, Feb 7, 2010 at 11:23 AM, Andrus Adamchik wrote: > On Feb 7, 2010, at 5:39 PM, Andrey Razumovsky wrote: > > Also we need to check Victor's comment: >> >> https://issues.apache.org/jira/browse/CAY-1009?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12804515 >> #action_12804515 >> > > I did and I agree with him about the semantics of the fix. > 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. > > Also, answering my own earlier observation: > > > DirectToSubEntity.subEntities is NOT a reverse relationship of >> BaseEntity.toDirectToSubEntity, >> > > Looks like a simple model of relationship/reverse relationship breaks when > inheritance is involved. If we define "reverse" as relationship over the > same set of joins, just going in the opposite direction, then we can have > multiple reverse relationships for any single relationship and will need to > do much more e.g. when connecting related objects... > > I'll have to take a look at this test case again. I believe my patch fixed a very specific instance of what I thought at the time was a much larger problem. The core issue I was seeing at the time was something along the lines of: Customer is a subclass of Account Customer requires an Address There is no explicit mapping from Account to Address. There is a mapping between Customer and Address and is marked as mandatory. Cayenne added a runtime relationship from Account to Address and used that when doing something like customer.setAddress(), bypassing the mapped relationship, which caused a validation failure. IIRC, the patch I supplied allowed the relationship search to compare against a broader range of src-target pairings, allowing it to find the relationship I explicitly mapped. I may have some of the details wrong there since I haven't dealt with this is a while. But I should be able to pull up the mapping that wasn't working for me. -- Kevin
Re: CAY-1378, CAY-1009...
I'll check an old project of mine to see what mapping causes the error. BTW I've found another issue (CAY-1386) related to reverse rels. Do we have reverses for flattened rels? 2010/2/7 Andrus Adamchik > > On Feb 7, 2010, at 5:36 PM, Andrey Razumovsky wrote: > > As far as i remember, the problem is if we have a mapping: >> entities: >> A >> B extends A - mapped in same db table >> C >> >> dbRel: >> toA, cArray (from C to A and vice versa) - Is Mandatory >> >> objRel - toA, cArray (from C to A and vice versa) >> >> So, there's isn't any ObjRel from B to C and therefore Cayenne adds >> runtime >> relationship. >> Problems come when we're setting relationship via C.setToA(..) and commit. >> Cayenne thinks runtime rel from C to B is mandatory (after all, it's >> mapped >> to mandatory dbRel) and fails to commit. >> > > Created a patch from this description against 3.0 branch: > > > https://issues.apache.org/jira/secure/attachment/12435122/CAY-1009-Andreys-case.patch > > A == Sti2Inheritance > B == Sti2InheritanceSub > C == Sti2Related > > I still can't reproduce the failure, as there's no runtime relationship > created between B and C, either with or without CAY-1009 commit. > > Andrus > -- Andrey
Re: CAY-1378, CAY-1009...
On Feb 7, 2010, at 5:36 PM, Andrey Razumovsky wrote: As far as i remember, the problem is if we have a mapping: entities: A B extends A - mapped in same db table C dbRel: toA, cArray (from C to A and vice versa) - Is Mandatory objRel - toA, cArray (from C to A and vice versa) So, there's isn't any ObjRel from B to C and therefore Cayenne adds runtime relationship. Problems come when we're setting relationship via C.setToA(..) and commit. Cayenne thinks runtime rel from C to B is mandatory (after all, it's mapped to mandatory dbRel) and fails to commit. Created a patch from this description against 3.0 branch: https://issues.apache.org/jira/secure/attachment/12435122/CAY-1009-Andreys-case.patch A == Sti2Inheritance B == Sti2InheritanceSub C == Sti2Related I still can't reproduce the failure, as there's no runtime relationship created between B and C, either with or without CAY-1009 commit. Andrus
Re: CAY-1378, CAY-1009...
On Feb 7, 2010, at 5:39 PM, Andrey Razumovsky wrote: Also we need to check Victor's comment: https://issues.apache.org/jira/browse/CAY-1009?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12804515 #action_12804515 I did and I agree with him about the semantics of the fix. Also, answering my own earlier observation: DirectToSubEntity.subEntities is NOT a reverse relationship of BaseEntity.toDirectToSubEntity, Looks like a simple model of relationship/reverse relationship breaks when inheritance is involved. If we define "reverse" as relationship over the same set of joins, just going in the opposite direction, then we can have multiple reverse relationships for any single relationship and will need to do much more e.g. when connecting related objects... Andrus
Re: CAY-1378, CAY-1009...
Also we need to check Victor's comment: https://issues.apache.org/jira/browse/CAY-1009?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12804515#action_12804515 2010/2/7 Andrey Razumovsky > As far as i remember, the problem is if we have a mapping: > entities: > A > B extends A - mapped in same db table > C > > dbRel: > toA, cArray (from C to A and vice versa) - Is Mandatory > > objRel - toA, cArray (from C to A and vice versa) > > So, there's isn't any ObjRel from B to C and therefore Cayenne adds runtime > relationship. > Problems come when we're setting relationship via C.setToA(..) and commit. > Cayenne thinks runtime rel from C to B is mandatory (after all, it's mapped > to mandatory dbRel) and fails to commit. > Not sure why I haven't added test maps when committing, maybe I haven't > managed to create one. > We need to investigate this futher, so please hold on reverting the patch > > 2010/2/7 Andrus Adamchik > > Actually Bryan's case seems to be pretty clear. I will try to create a test >> case for it now: >> >> We have a source entity with two to-one relationships to target entities >>> that both inherit from the same table. When we try to insert a new instance >>> of the source entity, we get: >>> >>> org.apache.cayenne.CayenneRuntimeException: >>>Error resolving to-one fault. More than one object found. Source Id: >>>, relationship: runtimeRelationship0 >>> org.apache.cayenne.access.ToOneFault.doResolveFault(ToOneFault.java:90) >>> org.apache.cayenne.access.ToOneFault.resolveFault(ToOneFault.java:54) >>> >>> Cayenne found two objects in the table and thought it was an error. >>> >> >> >> >> >> On Feb 7, 2010, at 5:18 PM, Andrus Adamchik wrote: >> >> Guys, we need to do something about CAY-1009. The fix doesn't look >>> logical to me, and now it turns out that it breaks other things per >>> CAY-1378. >>> >>> I am looking at commented out test case InheritanceTest.testCAY1009() >>> that Kevin created, and from what I can tell, Cayenne does the right thing >>> here (without the CAY-1009 fix). I.e. DirectToSubEntity.subEntities is NOT a >>> reverse relationship of BaseEntity.toDirectToSubEntity, so we should not >>> expect it to behave as one. >>> >>> Bryan and Andrey also had problems with "runtime" relationships. So could >>> you create test cases for those so we can maybe try looking for the another >>> cause is elsewhere? Or maybe you could provide a failing mapping and >>> describe the problem? >>> >>> Andrus >>> >>> >>> >>> Begin forwarded message: >>> Andrus Adamchik updated CAY-1378: - Attachment: 0001-CAY-1378-no-reverse-for-inheritance.patch I tend to agree with Victor. Here is my patch reverting CAY-1009 commit (plus some minor loop refactoring). This fails uncommented InheritanceTest, but I think the test is wrong. I won't commit this yet, and will take further discussion to the dev list. >>> >>> >>> >> > > > -- > Andrey > -- Andrey
Re: CAY-1378, CAY-1009...
As far as i remember, the problem is if we have a mapping: entities: A B extends A - mapped in same db table C dbRel: toA, cArray (from C to A and vice versa) - Is Mandatory objRel - toA, cArray (from C to A and vice versa) So, there's isn't any ObjRel from B to C and therefore Cayenne adds runtime relationship. Problems come when we're setting relationship via C.setToA(..) and commit. Cayenne thinks runtime rel from C to B is mandatory (after all, it's mapped to mandatory dbRel) and fails to commit. Not sure why I haven't added test maps when committing, maybe I haven't managed to create one. We need to investigate this futher, so please hold on reverting the patch 2010/2/7 Andrus Adamchik > Actually Bryan's case seems to be pretty clear. I will try to create a test > case for it now: > > We have a source entity with two to-one relationships to target entities >> that both inherit from the same table. When we try to insert a new instance >> of the source entity, we get: >> >> org.apache.cayenne.CayenneRuntimeException: >>Error resolving to-one fault. More than one object found. Source Id: >>, relationship: runtimeRelationship0 >> org.apache.cayenne.access.ToOneFault.doResolveFault(ToOneFault.java:90) >> org.apache.cayenne.access.ToOneFault.resolveFault(ToOneFault.java:54) >> >> Cayenne found two objects in the table and thought it was an error. >> > > > > > On Feb 7, 2010, at 5:18 PM, Andrus Adamchik wrote: > > Guys, we need to do something about CAY-1009. The fix doesn't look logical >> to me, and now it turns out that it breaks other things per CAY-1378. >> >> I am looking at commented out test case InheritanceTest.testCAY1009() that >> Kevin created, and from what I can tell, Cayenne does the right thing here >> (without the CAY-1009 fix). I.e. DirectToSubEntity.subEntities is NOT a >> reverse relationship of BaseEntity.toDirectToSubEntity, so we should not >> expect it to behave as one. >> >> Bryan and Andrey also had problems with "runtime" relationships. So could >> you create test cases for those so we can maybe try looking for the another >> cause is elsewhere? Or maybe you could provide a failing mapping and >> describe the problem? >> >> Andrus >> >> >> >> Begin forwarded message: >> >>> Andrus Adamchik updated CAY-1378: >>> - >>> >>> Attachment: 0001-CAY-1378-no-reverse-for-inheritance.patch >>> >>> I tend to agree with Victor. Here is my patch reverting CAY-1009 commit >>> (plus some minor loop refactoring). This fails uncommented InheritanceTest, >>> but I think the test is wrong. I won't commit this yet, and will take >>> further discussion to the dev list. >>> >> >> >> > -- Andrey
Re: CAY-1378, CAY-1009...
Actually Bryan's case seems to be pretty clear. I will try to create a test case for it now: We have a source entity with two to-one relationships to target entities that both inherit from the same table. When we try to insert a new instance of the source entity, we get: org.apache.cayenne.CayenneRuntimeException: Error resolving to-one fault. More than one object found. Source Id: , relationship: runtimeRelationship0 org.apache.cayenne.access.ToOneFault.doResolveFault(ToOneFault.java: 90) org.apache.cayenne.access.ToOneFault.resolveFault(ToOneFault.java: 54) Cayenne found two objects in the table and thought it was an error. On Feb 7, 2010, at 5:18 PM, Andrus Adamchik wrote: Guys, we need to do something about CAY-1009. The fix doesn't look logical to me, and now it turns out that it breaks other things per CAY-1378. I am looking at commented out test case InheritanceTest.testCAY1009() that Kevin created, and from what I can tell, Cayenne does the right thing here (without the CAY-1009 fix). I.e. DirectToSubEntity.subEntities is NOT a reverse relationship of BaseEntity.toDirectToSubEntity, so we should not expect it to behave as one. Bryan and Andrey also had problems with "runtime" relationships. So could you create test cases for those so we can maybe try looking for the another cause is elsewhere? Or maybe you could provide a failing mapping and describe the problem? Andrus Begin forwarded message: Andrus Adamchik updated CAY-1378: - Attachment: 0001-CAY-1378-no-reverse-for-inheritance.patch I tend to agree with Victor. Here is my patch reverting CAY-1009 commit (plus some minor loop refactoring). This fails uncommented InheritanceTest, but I think the test is wrong. I won't commit this yet, and will take further discussion to the dev list.
