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

Core Data adapter #87

Merged
merged 27 commits into from May 21, 2013

Conversation

@jspahrsummers
Copy link
Member

jspahrsummers commented Mar 29, 2013

A MTLManagedObjectAdapter, for transforming MTLModel objects to and from NSManagedObjects.

To do:

  • Serialize collections to NSManagedObject relationships
  • Deserialize relationships to MTLModel collections
  • Unit tests for serialization
  • Unit tests for deserialization
  • Check +classForDeserializingManagedObject: before deserialization
  • Track objects serialized/deserialized so far to avoid cycles
  • Document private MTLManagedObjectAdapter methods
@jspahrsummers

This comment has been minimized.

Copy link
Member Author

jspahrsummers commented Mar 29, 2013

I'm especially interested for @alanjrogers and @dannygreg to weigh in on this one, as Core Data users.

@alanjrogers

This comment has been minimized.

Copy link
Contributor

alanjrogers commented Mar 30, 2013

The interface looks good. I'm curious how you were planning on handling faults?

@dannygreg

This comment has been minimized.

Copy link

dannygreg commented Mar 30, 2013

👍 on the API.

I'm assuming all fetching etc. would be carried out at the CoreData level and then transformed into Mantle model objects to bubble up to the UI etc.?

@jspahrsummers

This comment has been minimized.

Copy link
Member Author

jspahrsummers commented Mar 30, 2013

I'm curious how you were planning on handling faults?

What do you mean? The only time faulting would occur from this API is when deserializing an NSManagedObject into MTLModel. The implementation should be thread-safe, so any faulting can happen in the background if you're careful with the contexts you use.

I'm assuming all fetching etc. would be carried out at the CoreData level and then transformed into Mantle model objects to bubble up to the UI etc.?

I see two major use cases here:

  1. Hit a web service, parse its JSON into MTLModels, then trivially save those into Core Data.
  2. Read objects out of Core Data as MTLModels, then use those in your app (exactly what you described). This nicely hides a lot of the complexity of Core Data. It could result in worse performance, but it's an easy way to persist objects sensibly. Reading objects like this would be similar to an NSDictionaryResultType fetch, but more powerful.

They go hand-in-hand conceptually, but you could also easily do one without the other. For example, maybe you just use Mantle as an intermediary for JSON parsing, but don't actually reference MTLModel anywhere outside your model layer.

@dannygreg

This comment has been minimized.

Copy link

dannygreg commented Mar 30, 2013

You could also just use Core Data directly if performance becomes a serious concern.

@alanjrogers

This comment has been minimized.

Copy link
Contributor

alanjrogers commented Mar 31, 2013

What do you mean? The only time faulting would occur from this API is when deserializing an NSManagedObject > into MTLModel. The implementation should be thread-safe, so any faulting can happen in the background if you're > careful with the contexts you use.

Actually now that I've thought about it some more, any MOs would have their faults fire when you serialized them into MTL objects, so there is nothing we need to worry about.

jspahrsummers added 20 commits Apr 1, 2013
@jspahrsummers

This comment has been minimized.

Copy link
Member Author

jspahrsummers commented May 18, 2013

@github/mac @mdiep @joshvera This is ready for review now. 💥

CC @mattt

//
// Subclasses overriding this method should combine their values with those of
// `super`.
+ (NSDictionary *)relationshipModelClassesByPropertyKey;

This comment has been minimized.

Copy link
@alanjrogers

alanjrogers May 20, 2013

Contributor

This could do with a "Returns a dictionary mapping property keys (as an NSString) to model classes (as Class)." statement as the last paragraph. While the rest of the documentation already explains this and then method name also spells it out, I still find the 'Returns blah' bit of the docs to be a very useful quick reference.

This comment has been minimized.

Copy link
@jspahrsummers

jspahrsummers May 20, 2013

Author Member

👍

#import "MTLModel.h"
#import "MTLReflection.h"

NSString * const MTLManagedObjectAdapterErrorDomain = @"MTLManagedObjectAdapterErrorDomain";

This comment has been minimized.

Copy link
@alanjrogers

alanjrogers May 20, 2013

Contributor

Could use NSSTRING_CONST here. Do we have it defined in Mantle?

This comment has been minimized.

Copy link
@jspahrsummers

jspahrsummers May 20, 2013

Author Member

/nope

I'm not the biggest fan of that macro, at least for our OSS. It doesn't save that much typing, and its purpose is not super intuitive when seeing it.

This comment has been minimized.

Copy link
@alanjrogers

alanjrogers May 20, 2013

Contributor

TBH Neither am I, I'd be fine with getting rid of it :trollface:

}

__block id result = nil;
[context performBlockAndWait:^{

This comment has been minimized.

Copy link
@alanjrogers

alanjrogers May 20, 2013

Contributor

This will deadlock if called on the main thread and the context has a concurrencyType of NSMainQueueConcurrencyType.

This comment has been minimized.

Copy link
@jspahrsummers

jspahrsummers May 20, 2013

Author Member

It's poorly documented, but performBlockAndWait: is specifically meant to support that kind of recursive locking. I'll add a test verifying it.

This comment has been minimized.

Copy link
@alanjrogers

alanjrogers May 20, 2013

Contributor

Oh I didn't know that. Do you have link for the docs?

This comment has been minimized.

Copy link
@jspahrsummers

jspahrsummers May 20, 2013

Author Member

This was ridiculously hard to track down. It's only mentioned in the high-level iOS 5 Release Notes (not OS X), and no specific Core Data document. Specifically:

The performBlockAndWait: method supports API reentrancy.

This comment has been minimized.

Copy link
@alanjrogers

alanjrogers May 21, 2013

Contributor

👍 WOW Awesome documentation there Apple.


// Invoked from +modelOfClass:fromManagedObject:processedObjects:error: after
// the receiver's properties have been initialized.
- (id)modelFromManagedObject:(NSManagedObject *)managedObject processedObjects:(CFMutableDictionaryRef)processedObjects error:(NSError **)error;

This comment has been minimized.

Copy link
@alanjrogers

alanjrogers May 20, 2013

Contributor

I'm curious, Why the CFMutableDictionaryRef over NSMutableDictionary?

This comment has been minimized.

Copy link
@jspahrsummers

jspahrsummers May 20, 2013

Author Member

Need to use pointer equality instead of -isEqual: (which is unnecessarily expensive, and might create false positives). It's documented in a comment in the implementation below.

This comment has been minimized.

Copy link
@alanjrogers

alanjrogers May 20, 2013

Contributor

I can't find that comment, could you link it?

This comment has been minimized.

Copy link
@alanjrogers

alanjrogers May 21, 2013

Contributor

// Mark this as being autoreleased, because validateValue may return
// a new object to be stored in this variable (and we don't want ARC to
// double-free or leak the old or new values).
__autoreleasing id transformedValue = value;

This comment has been minimized.

Copy link
@alanjrogers

alanjrogers May 20, 2013

Contributor

May be worth putting this comment in the setValueForKey block inside modelFromManagedObject... too.

This comment has been minimized.

Copy link
@jspahrsummers

jspahrsummers May 20, 2013

Author Member

👍

@ghost ghost assigned alanjrogers May 20, 2013
@alanjrogers

This comment has been minimized.

Copy link
Contributor

alanjrogers commented May 20, 2013

I've noticed that the implementation of modelFromManagedObject and managedObjectFromModel are very similar, with the only apparent difference being the direction of serialization. Do you think it would be possible to factor out the common stuff into another method?

@alanjrogers

This comment has been minimized.

Copy link
Contributor

alanjrogers commented May 20, 2013

📎

@jspahrsummers

This comment has been minimized.

Copy link
Member Author

jspahrsummers commented May 20, 2013

Do you think it would be possible to factor out the common stuff into another method?

I don't think so. The direction has a pretty big impact, because the logic for deserializing is very different from that of serializing (in terms of methods invoked, objects created, etc.), even though the general structure is similar.

@jspahrsummers

This comment has been minimized.

Copy link
Member Author

jspahrsummers commented May 20, 2013

📋

}

// Jump through some hoops to avoid referencing classes directly.
NSString *propertyClassName = NSStringFromClass(propertyDescription.class);

This comment has been minimized.

Copy link
@joshvera

joshvera May 20, 2013

Member

I don't understand why we're doing this. We don't want the block to retain these classes during execution?

This comment has been minimized.

Copy link
@joshvera

joshvera May 20, 2013

Member

Oh never mind. Missed the conditional linking.

This comment has been minimized.

Copy link
@jspahrsummers

jspahrsummers May 20, 2013

Author Member

Yeah, the goal is that Mantle shouldn't require Core Data to be linked unless you're using this adapter.

@alanjrogers

This comment has been minimized.

Copy link
Contributor

alanjrogers commented May 21, 2013

📚

alanjrogers added a commit that referenced this pull request May 21, 2013
Core Data adapter
@alanjrogers alanjrogers merged commit b6dc288 into master May 21, 2013
1 check passed
1 check passed
default Build #410187 succeeded in 28s
Details
@alanjrogers alanjrogers deleted the core-data-adapter branch May 21, 2013
@jspahrsummers

This comment has been minimized.

Copy link
Member Author

jspahrsummers commented May 21, 2013

🤘

@bachand

This comment has been minimized.

Copy link

bachand commented Jul 19, 2013

@jspahrsummers It's very possible I'm missing something, but I couldn't find a way to easily update an existing NSManagedObject using MTLManagedObjectAdapter. Is there any facility for that workflow?

@jspahrsummers

This comment has been minimized.

Copy link
Member Author

jspahrsummers commented Jul 19, 2013

@bachand There's currently no way to do that, but I can see how it'd be useful when going from JSON > Mantle > Core Data. I'd be happy to review a pull request.

@bachand

This comment has been minimized.

Copy link

bachand commented Jul 19, 2013

@jspahrsummers Thanks for the quick response! I'll let you know if I decide to go down that route.

@robb

This comment has been minimized.

Copy link
Member

robb commented Sep 30, 2013

@bachand did you ever go down that route?

@bachand

This comment has been minimized.

Copy link

bachand commented Oct 1, 2013

@robb I was able to get https://github.com/RestKit/RestKit to fit my needs, so I didn't end up going down that route. What may not be totally apparent about RestKit at first glance is that it is very easy to only use a handful of its classes without adopting its whole request/response mapping framework.

@alvaromb

This comment has been minimized.

Copy link

alvaromb commented Mar 3, 2014

@bachand AFAIK, what do you want to achieve can be done with propertyKeysForManagedObjectUniquing. Here you can find what this method does:

// The managed object returned by the fetch request will then be set with all
// values from the MTLModel that the managed object is being converted from.
@bachand

This comment has been minimized.

Copy link

bachand commented Mar 14, 2014

@alvaromb, thanks for pointing that out – you're absolutely right. Unfortunately as far as I can tell this wasn't released when I was looking into using Mantle, but great to know moving forward!

@onmyway133

This comment has been minimized.

Copy link

onmyway133 commented Jul 26, 2014

It would be great if we just convert from Model to ManagedObject, WITHOUT saving to context. What if we want to use MagicalRecord do the context saving part

@nickynick

This comment has been minimized.

Copy link
Member

nickynick commented Jul 26, 2014

@onmyway133 But there's no saving. MTLManagedObjectAdapter just creates or updates a managed object, that's it.

@alvaromb

This comment has been minimized.

Copy link

alvaromb commented Jul 26, 2014

@onmyway133 Overcoat 2.0 uses Mantle and AFNetworking, and provides automatic context save for your responses, because it uses a private queue to serialise from JSON to MTLModel and then to NSManagedObject. And, as @nickynick just said, there is no saving using Mantle's MTLManagedObjectAdapter, it just generates the NSManagedObject's.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.