Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Can no longer use NSNumber as a primary key as of 0.10.1? #754

Closed
RobWilco opened this Issue May 22, 2012 · 12 comments

Comments

Projects
None yet
2 participants

Ha! No sooner did I figure out #692 than I updated to 0.10.1 (adjusting the Xcode project settings - it compiles just fine) ... and now I'm running into a new, albeit hopefully easier, problem.

My primary keys are NSNumbers (Integer 16 to be exact). With a fresh build/install (no prior Core Data store), we get this (using Trace mode):

 2012-05-22 13:28:12.691 MyApp[26444:18303] I restkit.core_data:RKInMemoryManagedObjectCache.m:34 Creating thread-local entity cache for managed object context: <NSManagedObjectContext: 0x8a74830>
 2012-05-22 13:28:12.691 MyApp[26444:18303] I restkit.core_data:RKInMemoryManagedObjectCache.m:50 Caching instances of Entity 'MyClass' by primary key attribute 'identifier'
 2012-05-22 13:28:12.692 MyApp[26444:18303] I restkit.core_data.cache:RKEntityByAttributeCache.m:102 Loading entity cache for Entity 'MyClass' by attribute 'identifier'

Once I reach this line in RKManagedObjectMapping.m:

 [object setValue:primaryKeyValue forKey:primaryKeyAttribute];

https://github.com/RestKit/RestKit/blob/release/0.10.1/Code/CoreData/RKManagedObjectMapping.m#L185

primaryKeyValue is seen to be a NSString, not a NSNumber (per the debugger, at any rate) ... and NSManagedObject throws an exception.

 Catchpoint 4 (exception thrown).2012-05-22 13:54:43.537 MyApp[27249:18403]
 *** Terminating app due to uncaught exception 'NSInvalidArgumentException',
 reason: 'Unacceptable type of value for attribute: property = "identifier";
 desired type = NSNumber; given type = __NSCFString; value = 19001.'

Are NSNumber objects no longer allowed as primary keys in 0.10.1?

Sigh. Pilot error again. Was using the release/0.10.1 branch. Switched to master branch and now all is well.

@RobWilco RobWilco closed this May 22, 2012

@blakewatters blakewatters reopened this May 22, 2012

Owner

blakewatters commented May 22, 2012

Hmm this may actually be a legitimate concern for you. As part of the new cacheing implementation, the primaryKey value is set on newly created object instances post mapping and then they are added to the cache (so subsequent lookups find it). It sounds me like you are sending down your primary key as a string, but then storing it on the model as an number. RK is not performing any type conversions for you at that moment in the mapping process.

Is this analysis correct?

I think you got it! The JSON (and XML) look like this (a contrived simplification, but accurate -- note the "id" attribute in both). I'm using JSON in practice, but the API can send either/or. The content starts out as XML nodes (from a PHP-based API), is converted to JSON (and/or cached server-side) if necessary, and then it just comes on down the wire like this.

 "widgets": {
    "widget": [{
        "m_attributes": {
            "id": "24894"
        },
        "category": {
            "m_attributes": {
                "id": "23",
                "name": "Category Name"
            }
        },
        "name": "Name Goes Here",
        "summary": "Summary Here"
    },
    ...
    ]
 }

 <widgets>
    <widget id="24894">
       <category id="23" name="Category Name" />
       <name>Name Goes Here</name>
       <summary>Summary Here</summary>
    </widget>
    ...
 </widgets>

Thoughts?

Owner

blakewatters commented May 22, 2012

Cool. I will throw a test case in place and update the logic to accommodate that case and merge it this evening. Will make it in for 0.10.1

Grazie!

(I suppose the server side XML-to-JSON conversion isn't smart enough to spot numbers, so it ends up as "24894" in quotes on the JSON side. If it would be better/proper for me to use strings on the app side, by all means say the word.)

Owner

blakewatters commented May 22, 2012

Nah, this is actually a regression compared with 0.10.0 behavior -- it just wasn't covered by a test so it slipped past me. Its leakage from under the covers caching changes that should not change behavior. I'll fix it in RK.

@RobWilco RobWilco closed this May 23, 2012

@blakewatters blakewatters reopened this May 23, 2012

(Whoops - sorry, closed by mistake.)

In the meantime, I switched to the main branch, and now I can use NSNumber as a primary key (yay!) ... but now I get duplicate categories (d'oh!). Perhaps I need to reinforce uniqueness using validatePrimaryKey:error: per this thread?

Only in my case I'm not fetching anything in parallel. I'm just doing one fetch of 25 widgets at a time, and in this initial foray I'm only grabbing 25 widgets overall just to try things out. Of course, each widget may be related to multiple categories, but overall the categories should not be duplicated in the database. Hmm ...

Reading this now: Implementing Find-or-Create Efficiently. It sounds like that's almost what I want to do (more like "create or re-use what's already there"), but maybe RestKit can do this and I'm just missing it? (Also re-reading the Object Mapping guide.)

Ahh, I may be stuck. Looks like the duplicate entries issue was fixed in 0.10.1.

Owner

blakewatters commented May 24, 2012

@RobWilco You can fix your issue preventing 0.10.1 adoption on your sources until I get the fix into the tree. Open up RKManagedObjectMapping.m and look at line 184:

if (primaryKeyAttribute && primaryKeyValue && ![primaryKeyValue isEqual:[NSNull null]]) {
            [object setValue:primaryKeyValue forKey:primaryKeyAttribute];
        }

        if ([self.objectStore.cacheStrategy respondsToSelector:@selector(didCreateObject:)]) {
            [self.objectStore.cacheStrategy didCreateObject:object];
        }

That call to setValue right there is where your primary key is being set to the string value instead of the number. Coerce the value to an NSNumber there and you should be all good. I am going to add more intelligent type checking this evening.

Stupendous - thank you so much! I also just caught this branch merge of feature/635-accelerate-entity-cache and was going to give that a spin, but if it's also in 0.10.1 and not just development, I should be fine.

Owner

blakewatters commented May 24, 2012

@RobWilco I just pushed a fix to the 0.10.1 release branch. Can you verify its working for you?

It works! Not only that, I got to see a nice example of adding a relevant unit test in the bargain. Thanks tons - very much appreciated.

signalsandstuff added a commit to signalsandstuff/RestKit that referenced this issue May 31, 2012

Merge branch 'master' of git://github.com/RestKit/RestKit
* 'master' of git://github.com/RestKit/RestKit: (360 commits)
  Fix issue where grouping objects by section in RKTableController and loading an empty collection would fail to refresh table view.
  Fix type mismatch assertion in cases where JSON/XML payload contains a string value for an integer primary key. fixes #754
  Fix static analyzer warning
  Cleanups to and test coverage expansion of RKTableController API's. refs #765
  Update RKTestFactory to silence logging output during setup/tear down operations. closes #764
  Reworked table controller state into a bit mask to coalesce state change into a single observation. fixes #753
  Implemented support for type coercions in primaryKeyAttribute API's. closes #758
  Cancel current object loader when loading a table view
  Clean up inconsistencies with 'cancelled' and 'cancel' to match Cocoa (isCancelled)
  Ensure managed object connections are established across appropriate managed object contexts
  Add RKLog helper for displaying detailed key-value validation failure information. closes #750
  Bump version to 0.10.1
  Fix for RestKit#709 - [RKManagedObjectThreadSafeInvocation serializeManagedObjectsForArgument:withKeyPaths:] is not properly serializing managed objects
  Ensure creation of Application Data directory when initializing Managed Object Store. fixes #657
  Add tests verifying expected behavior for availability of RKRequest response property. closes #527
  Eliminate duplication of request method to string logic in [RKRequest HTTPMethod]
  Migrate RKResponse property from RKObjectLoader to RKRequest. refs #527
  Fix static analyzer issues
  Replace v0.9.4 references with 0.10.0
  Rework intermittently failing tests
  ...

Conflicts:
	Code/Network/RKRequest.m

smclelland pushed a commit to smclelland/RestKit that referenced this issue Jun 7, 2012

Merge branch 'master' of git://github.com/RestKit/RestKit
* 'master' of git://github.com/RestKit/RestKit: (109 commits)
  Fix issue where grouping objects by section in RKTableController and loading an empty collection would fail to refresh table view.
  Fix type mismatch assertion in cases where JSON/XML payload contains a string value for an integer primary key. fixes #754
  Fix static analyzer warning
  Cleanups to and test coverage expansion of RKTableController API's. refs #765
  Update RKTestFactory to silence logging output during setup/tear down operations. closes #764
  Reworked table controller state into a bit mask to coalesce state change into a single observation. fixes #753
  Implemented support for type coercions in primaryKeyAttribute API's. closes #758
  Cancel current object loader when loading a table view
  Clean up inconsistencies with 'cancelled' and 'cancel' to match Cocoa (isCancelled)
  Ensure managed object connections are established across appropriate managed object contexts
  Add RKLog helper for displaying detailed key-value validation failure information. closes #750
  Bump version to 0.10.1
  Fix for RestKit#709 - [RKManagedObjectThreadSafeInvocation serializeManagedObjectsForArgument:withKeyPaths:] is not properly serializing managed objects
  Ensure creation of Application Data directory when initializing Managed Object Store. fixes #657
  Add tests verifying expected behavior for availability of RKRequest response property. closes #527
  Eliminate duplication of request method to string logic in [RKRequest HTTPMethod]
  Migrate RKResponse property from RKObjectLoader to RKRequest. refs #527
  Fix static analyzer issues
  Replace v0.9.4 references with 0.10.0
  Rework intermittently failing tests
  ...

Conflicts:
	Code/Support/Parsers/JSON/RKJSONParserJSONKit.m
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment