bugGNUstep - Bugs: bug #22565, EOKeyValueCoding: Support modern...

Group
 
 

bug #22565: EOKeyValueCoding: Support modern Cocoa KVC semantics for NSArray aggregate functions

Submitter:  Tim McIntosh <tmcintos>
Submitted:  Wed 12 Mar 2008 06:22:35 AM UTC
   
 
Category:  gdl2 Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  ayers
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 25 Feb 2009 09:28:15 AM UTC, comment #16: 

I think the only thing that was different was the special handling of @count in WO45.  Once an NSArray received a @count.some.key.path didn't matter and it was the only computational key which didn't expect a parameter.  I'm mostly concerned about the second issue since we have the lazy key paths.

I believe that this bug was fixed for you after I applied your patch and hope I didn't break anything for you with the fix.  Please reopen the report if you have an issues with it.

Cheers,
David

David Ayers <ayers>
Group Member
Tue 18 Mar 2008 02:16:39 AM UTC, comment #15: 

Hmm, could I be seeing different results because of different implementations/behavior of -[NSObject valueForKeyPath:] between Cocoa and -base?  I've been running only on top of Cocoa, not -base.  Maybe that is why it seems like I'm on a different page here?

-Tim


Tim McIntosh <tmcintos>
Tue 18 Mar 2008 01:54:57 AM UTC, comment #14: 

Hi David,

I must be missing something.  For educational purposes, can you clue me in on the specific simple cases that you're referring to?

Also wondering about your recent change to computeCountForKey: and the check-in comment right before that one, which says "This is not WO45 compatible".  Can you clarify specifically what is not WO45 compatible?  I'm also not clear on the @count handling--I thought @count ignored the string following the operator in both EOF and Cocoa. 

Thanks,
Tim

Tim McIntosh <tmcintos>
Mon 17 Mar 2008 07:10:33 AM UTC, comment #13: 

Hello Tim,

The changes wrt replacing valueForKey: with valueForKeyPath: is certainly correct.  The changes wrt key path handling still issues as the break the simple cases.  Hopefully I can find the time today or tomorrow to get this sorted out.

Cheers,
David

David Ayers <ayers>
Group Member
Mon 17 Mar 2008 06:58:23 AM UTC, comment #12: 

Hmm it seems that my mail reply did didn't get attached to the but report... sorry for the duplicate:

Richard Frith-Macdonald schrieb:

> > I added [NSString-decimalValue] to base, as a test shows that it's present in
> > MacOS-X Foundation (though it's not in either the documentation or the header
> > files).


Thanks!

> > Not sure what to do about in aggregate functions.  The documentation
> > specifically says they should use doubles ... so perhaps it would be better
> > for GDL2 to override the base implementations .. or maybe we could write a test
> > to find out if apple actually uses decimal values even though the doc says
> > doubles.  Perhaps we should allow more precision than apple anyway.


We need to override NSArray's implementation anyway so it makes no difference once EOControl is linked.  And I agree that if the
documentation explicitly refers to doubleValue then -base has good reason to stick to it.

But I'm sure people will find it surprising when they start relying on on those aggregate functions outside of GDL2 and notice that round(x.5) will not give them consistent results.  So I would also sympathize with an NSDecimal implementation.

David Ayers <ayers>
Group Member
Sun 16 Mar 2008 07:57:07 PM UTC, comment #11: 

I added [NSString-decimalValue] to base, as a test shows that it's present in MacOS-X Foundation (though it's not in either the documentation or the header files).

Not sure what to do about in aggregate functions.  The documentation specifically says they should use doubles ... so perhaps it would be better for GDL2 to override thebase implementations .. or maybe we could write a test to find out if apple actually uses decimal values even though the doc says doubles.  Perhaps we should allow more precision than apple anyway.

Richard Frith-Macdonald <CaS>
Group Member
Sun 16 Mar 2008 06:59:30 PM UTC, comment #10: 

The string stems from the original property list parsing and is consistent with WO45.

The -[NSString decimalValue] is an undocumented method in WO45.  If it doesn't exist in current Foundation and/or you do not want to add it to -base, then I'll add it to GDL2.  But since I have reports that the tests pass on OS X, I'll assume this it also exists on OS X.

Note that for any financial application doubleValue is not an option and I think you could consider using decimalValue for -base's NSArray aggregate functions also.

David Ayers <ayers>
Group Member
Sun 16 Mar 2008 12:00:00 PM UTC, comment #9: 

To the best of my knowledge -decimalValue has never been implemented by NSString in Foundation or base ... so presumably the question 'why is there an NSString representation involved'  is the important one.

Richard Frith-Macdonald <CaS>
Group Member
Sun 16 Mar 2008 11:36:17 AM UTC, comment #8: 

Thanks a lot folks!.

The only reason I haven't committed the GDL2 patch yet is that I'm still seeing an exception that NSString doesn't respond to decimalValue.

This is strange for two reasons:
1. why is there an NSString representation involved in the first place... these should be NS(Decimal)Numbers.
2. did Foundation stop implementing decimalValue for NSString?

If I add hacked -decimalNumber category into the test the EOKeyValueCoding02.m passes.

PS: I'm AFK for a bit and only have very limited time to investigate currently.

David Ayers <ayers>
Group Member
Sun 16 Mar 2008 07:23:54 AM UTC, comment #7: 

Sounds right to me ... I added valueForKeyPath: in NSArray.m and NSSet.m

Richard Frith-Macdonald <CaS>
Group Member
Sun 16 Mar 2008 01:41:44 AM UTC, comment #6: 

I agree that it looks like that's what Apple did.  It appears that in Cocoa's NSArray, valueForKeyPath: only interprets multi-component key paths starting with "@" as aggregate functions, and valueForKey: only interprets keys starting with "@" to mean that [super valueForKey:] should be invoked with the key following the "@".

For example, this works in Cocoa (10.5):
  [array valueForKeyPath: @"@sum.property"]
  [array valueForKey: @"@lastObject"]
  [array valueForKeyPath: @"@lastObject"]
but this does not:
  [array valueForKey: @"@sum.property"];
  [array valueForKeyPath: @"@lastObject.locations"]

It appears that in EOF's NSArray additions, valueForKeyPath: just treats a path starting with "@" a single key, passing it to valueForKey:, which invokes the appropriate computeXyzForKey: method with the key path following the aggregate function.  The computeXyzForKey: method then uses valueForKeyPath:, as in my original patch.

This appears to be true for EOF 2.1 and WO/EOF 4.0.1, anyway (I just ran the attached sample code on 4.0.1 and got the same results as EOF 2.1 (see KVC.m)).  I suspect that it is also true for WO 4.5.

David, can you verify how it works with WO 4.5?

Thus, it appears that I don't really need the new Cocoa behavior after all--I just need the old EOF-compatible behavior.

I've attached a refinement of my original patch to EOKeyValueCoding.m, which I believe brings GDL2's behavior into alignment with EOF:
- Removed special case for "count" and "@count" keys.
- Changed valueForKeyPath: to treat paths beginning with "@" as a single key.
- Change valueForKey: to valueForKeyPath: in computeXyzForKey: methods.
- Updates comments.

For now, it seems like it would be best to keep the GDL2 and -base implementations separate.  -base can implement the Cocoa behavior and GDL2 can implement the EOF behavior.  GDL2 still needs to override Cocoa's methods in order to provide the WO4.5 behavior when running on Cocoa, anyway.

It seems like GDL2's [NSArray valueForKey:] behavior (after patching) could easily be made compatible with Cocoa's by just falling back to [super valueForKey:] if no suitable computeXyzForKey: method is found for a key starting with "@".  It seems like this would be more elegant than Cocoa's implementation.

What do you guys think?  Am I missing anything here?


(file #15272)

Tim McIntosh <tmcintos>
Sat 15 Mar 2008 03:28:42 PM UTC, comment #5: 

It seems to me that, with the current state of the code, we can/should easily be able to implement the MacOS-X behavior in gnustep-base by implementing
-valueForKeyPath: in the various collection classes.
From stepping through example code in the debugger on MacOS-X, it seems likely that Apple did it that way too.

Richard Frith-Macdonald <CaS>
Group Member
Fri 14 Mar 2008 04:28:21 AM UTC, comment #4: 

I attached a contrived but concrete example in KVC.zip, which I ran on Mac OS 10.5 and OPENSTEP 4.2 + EOF 2.1.  The results are somewhat surprising--EOF 2.1 gives the same results as Cocoa for valueForKeyPath: when a key path is used with an aggregate operator.  I don't have WO4.5 so I can't test that version of EOF (I could test the WO4 version if I could get Mac OS X Server going again).

Note that the behavior of valueForKey: appears to be different between EOF and Cocoa.  In EOF, valueForKey: implemented the aggregate operators, but in Cocoa they appear to be implemented only in valueForKeyPath:.

In Cocoa, @sum.x.y.z appears to mean different things depending on whether it is interpreted as a key or a key path.  The valueForKey: method of NSArray and NSSet appears to work as documented in NSDictionary with respect to keys starting with "@" (though I don't see this documented for NSArray and NSSet):

  "If key does start with “@”, strips the “@” and invokes [super valueForKey:] with the rest of the key."


(file #15262)

Tim McIntosh <tmcintos>
Thu 13 Mar 2008 08:35:39 PM UTC, comment #3: 

Lets have closer look at some key paths...

(I'm not using @count because it's special since the parameter is optional)

 displayGroup.allObjects.@sum.value

Currently implementation starts evaluating from left to right:
 #define vfk valueForKey
 t = [displayGroup vfk: @"allObjects"]
 t = [t vfk: @"@sum.value"]

Now NSArray's implementation would invoke computeSumForKey:@"value" on itself.  That implementation iterates over the receivers objects and asks each item:
 sum += [[currentItem valueForKey:@"value"] decimalValue]
and return the sum. Fine...

Now let's look at:
 displayGroup.allObjects.@sum.detailArray.@avg.value

The current proposed implementation would send:
 t = [displayGroup vfk: @"allObjects"]
 t = [t vfk: @"@sum.detailArray"]
 for each item in the array
   sum += [[currentItem vfk:@"detailArray"] decimalValue]

 this is broken... we do not want to send detailArray's decimalValue.

What we actually want is to send:
 t = [displayGroup vfk: @"allObjects"]
 t = [t vfk: @"@sum.detailArray.@avg.value"]

And then possibly have NSArray's implementation send something like:

 for each item in detailArray
   [item vfk:@"avg.value"]

 collect an interim array of {@"value"=<numbers>} then invoke
   [interimArray vfk:@"sum.value"]
 and return that result.

Given a key path like:
  displayGroup.allObjects.@sum.detailArray.@avg.value.stringValue.length

We would still want to merely group the aggregate keys I believe...

So I guess wrt NSArray's implementation I guess the part I quoted is OK, but the calculation of the range is wrong.  We want to group the aggregate key paths.  And then we need to look into NSArray's valueForKey: implementation to be able to handle arbitrary aggregate key paths...


David Ayers <ayers>
Group Member
Thu 13 Mar 2008 12:49:50 PM UTC, comment #2: 

I've just been reworking all the default keypath methods to operaate recursively the way MacOS-X appears to do.  So I'm worried by the comment:

To the requested feature I believe we need to replace the implementation of:
else
{
NSString *key = [keyPath substringToIndex: r.location];
NSString *path = [keyPath substringFromIndex: NSMaxRange(r)];
result = [[self valueForKey: key] valueForKeyPath: path];
}
to /do the right thing/... I'm still working on that.

As that code fragment is essentially what I thought was doing the right thing, and all my changes follow that pattern.

What is it that you think this is doing wrong?
As far as I can see, this pattern isolates all the handling of the special '@' prefixed keys into the overridden methods in NSArray, NSSet and NSDictionary... which seems a clean solution.

Am I missing something?

Richard Frith-Macdonald <CaS>
Group Member
Wed 12 Mar 2008 05:37:01 PM UTC, comment #1: 

This is an RFC/RFA for a patch to -base to
- implement the simplified version of valueForKeyPath: on NSArray
- get NSObject's valueForKeyPath: to call NSArray's valueForKeyPath: implementation

This will give currently just give -base the ability to handle the "simple" aggregate key value passing... i.e. you still do not get the the correct value for the key path:

-email is unavailable-

but at least the NSArray will receive valueForKey:@"@sum.value"

To the requested feature I believe we need to replace the implementation of:

  else
    {
      NSString *key  = [keyPath substringToIndex: r.location];
      NSString *path = [keyPath substringFromIndex: NSMaxRange(r)];

      result = [[self valueForKey: key] valueForKeyPath: path];
    }

to /do the right thing/... I'm still working on that.

But would it be OK to commit this but to allow GDL2 to at least work with the more limited WO45 KVC semantics?

Cheers,
David

The reason I didn't notice this earlier is because the testsuite only tested the aggregate KVCPaths directly on NSArray and I haven't updated our Apps to current GNUstep SVN for quite a while.


(file #15250)

David Ayers <ayers>
Group Member
Wed 12 Mar 2008 06:22:35 AM UTC, original submission:  

The Cocoa KVC semantics for NSArray aggregate functions (e.g. @sum) differ from the traditional semantics implemented in the EOControl framework when there is a key path following the aggregating operator instead of a simple key.

For example:   displayGroup.allObjects.@sum.itemsArray.@count

With Cocoa's -[NSArray valueForKeyPath:] behavior, this would invoke [object valueForKeyPath: @"itemsArray.@count"] on each object in the display group, then return the sum of the counts.

In order to ease porting of WebObjects 5 applications to GSWeb/GDL2, it is desirable to support this behavior, if possible.

Though not suitable as a final implementation, the attached patch file illustrates the desired behavior.  Note that this patch also removes the handling of "special keys" in NSDictionary (e.g. @"allKeys" vs. Cocoa's @"@allKeys" (also supported in -base)).

Reference:  http://lists.gnu.org/archive/html/gnustep-dev/2008-03/msg00093.html

Tim McIntosh <tmcintos>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #15262:  KVC.zip added by tmcintos (43KiB - application/zip)
file #15250:  kvc.patch added by ayers (3KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by CaS (Posted a comment)
  • -email is unavailable- added by ayers (Updated the item)
  • -email is unavailable- added by tmcintos (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-02-25 ayers StatusNone Fixed
        Open/ClosedOpen Closed
    2008-03-16 tmcintos Attached File- Added EOKeyValueCoding.m-2.patch, #15272
    2008-03-14 tmcintos Attached File- Added KVC.zip, #15262
    2008-03-13 tmcintos Carbon-CopyRemoved tmcintos ayers -
    2008-03-12 ayers Attached File- Added kvc.patch, #15250
        Assigned toNone ayers
    2008-03-12 tmcintos Attached File- Added EOKeyValueCoding.m.diff, #15245
        Carbon-Copy- Added tmcintos ayers

    Back to the top

    Powered by Savane 3.13-bb6a.
    Corresponding source code