Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review Patches from Frank Heldt #690

Closed
blakewatters opened this issue Apr 20, 2012 · 2 comments
Closed

Review Patches from Frank Heldt #690

blakewatters opened this issue Apr 20, 2012 · 2 comments
Assignees
Milestone

Comments

@blakewatters
Copy link
Member

These came through via e-mail:


Dear Blake Watters,

first i want to say thanks for this great Tool!  

Below 2 little patches for RestKit, which you might want to consider:

1. Now that we have a CoreData caching mechanism, we actually also want to use it (esp. in relationship mappings). This little patch made the difference between unusable and usable for my little app.   

--- RestKit/Code/CoreData/RKManagedObjectMappingOperation.m
+++ RestKit/Code/CoreData/RKManagedObjectMappingOperation.m
@@ -23,6 +23,7 @@
 #import "NSManagedObject+ActiveRecord.h"
 #import "RKDynamicObjectMappingMatcher.h"
 #import "RKLog.h"
+#import "RKManagedObjectStore.h"

 // Set Logging Component
 #undef RKLogComponent
@@ -75,10 +76,16 @@
             relatedObject = [NSSet setWithArray:objects];
         } else {
             RKLogTrace(@"Connecting has-one relationship at keyPath '%@' to object with primaryKey attribute '%@'", relationshipName, primaryKeyAttributeOfRelatedObject);
-            NSFetchRequest *fetchRequest = [NSManagedObject requestFirstByAttribute:primaryKeyAttributeOfRelatedObject withValue:valueOfLocalPrimaryKeyAttribute inContext:[self.destinationObject managedObjectContext]];
-            fetchRequest.entity = objectMapping.entity;
-            NSArray *objects = [NSManagedObject executeFetchRequest:fetchRequest inContext:[self.destinationObject managedObjectContext]];
-            relatedObject = [objects lastObject];
+            relatedObject = [objectMapping.objectStore.cacheStrategy findInstanceOfEntity:objectMapping.entity
+                                                         withPrimaryKeyAttribute:primaryKeyAttributeOfRelatedObject
+                                                                           value:valueOfLocalPrimaryKeyAttribute
+                                                          inManagedObjectContext:[self.destinationObject managedObjectContext]];
+            if (relatedObject == nil) {
+                NSFetchRequest *fetchRequest = [NSManagedObject requestFirstByAttribute:primaryKeyAttributeOfRelatedObject withValue:valueOfLocalPrimaryKeyAttribute inContext:[self.destinationObject managedObjectContext]];
+                fetchRequest.entity = objectMapping.entity;
+                NSArray *objects = [NSManagedObject executeFetchRequest:fetchRequest inContext:[self.destinationObject managedObjectContext]];
+                relatedObject = [objects lastObject];
+            }
         }
         if (relatedObject) {
             RKLogDebug(@"Connected relationship '%@' to object with primary key value '%@': %@", relationshipName, valueOfLocalPrimaryKeyAttribute, relatedObject);


2. I'm starting several RestKit requests from a background thread and this patch made it actually possible to do so. I know that RKRequestQueue is not totally thread safe, but for me this worked fine.

--- RestKit/Code/Network/RKRequestQueue.m
+++ RestKit/Code/Network/RKRequestQueue.m
@@ -382,7 +382,9 @@
 }

 - (BOOL)containsRequest:(RKRequest*)request {
-    return [_requests containsObject:request];
+    @synchronized(self) {
+        return [_requests containsObject:request];
+    }
 }

 - (void)cancelRequest:(RKRequest*)request loadNext:(BOOL)loadNext {


Feel free to use these patches.

Kind regards,
Frank Heldt
@ghost ghost assigned blakewatters Apr 20, 2012
@elsurudo
Copy link

The second one is actually a potential fix to a race condition I've experienced when launching requests from a background thread as well, so I will let you know if it solves my problem.

@blakewatters
Copy link
Member Author

The change for containsRequest: is merged. A similar update for using the cache strategy to connect requests was merged earlier today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants