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

Add MR_findOrCreateByID:attribute:inContext: method #171

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants

brunow commented May 30, 2012

Hi,

I've created a method MR_findOrCreateByID:attribute:inContext: just like activerecord.
If you want to add it to MagicalRecord I'll be happy to write some test.

@ghost

ghost commented on 66094a2 May 30, 2012

Couple of thoughts:
Please continue to use the coding style that is prevalent throughout the MagicalRecord source code. Curly braces should always line up so that scope can be easily determined at a glance. Thanks

Also, you have a good start. I would name the method MR_findOrCreateByAttribute:value:inContext: for the simple reason that you are not looking things up by Object IDs or IDs specifically. You will always do searches based on attributes given public APIs.

And, while you do a search and create an object if it's nil, you don't set your newly created object with your given information. That is, you expect the object to have at least the value of the attribute parameter included, and you haven't set it. It's as simple as adding on line 211 [object setValue:value forKey:attribute]; Remember, NSManagedObjects, like all objects, respond to KeyValue coding, and you definitely have enough info to fill in some blanks before you return the object.

And, one more thing, please add the shorthand header for these two methods in the MagicalRecordShorthand.h file

Please resubmit with these changes, Thanks!

brunow commented May 30, 2012

I've changed the method name you've right it's better now.

I think it's good.

brunow commented Jun 4, 2012

Did it look good to you ?

I'd love to merge this. Can you write a unit test that validates that this works? There are decent examples in the project already.

Owner

brunow replied Sep 14, 2012

No problem I can do that, but I'm not sure in which file I need to add the test. Shall I add it to NSManagedObjectHelperTests.m ?

Member

blackgold9 commented Sep 28, 2012

Yup. Sounds great.

smic commented Jan 24, 2013

As a note, the findOrCreate approach is slow for single IDs in my experience. It would be better to fetch an array of objects at once if possible.
See "Implementing Find-or-Create Efficiently" https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/CoreData/Articles/cdImporting.html

Contributor

tonyarnold commented Apr 21, 2013

Hi @brunow — no pressure, but when you get time it would be great to get some tests for this pull request so that we can merge the changes. We'll also need this pull request submitted against develop, rather than master — if you're too busy let me know and I'll recreate your changes manually.

@smic, so you'd just fire off a request to load all of the potential matches and use fast enumeration to handle this? It'd be great to get this as fast as possible before releasing it to general use!

@tonyarnold tonyarnold closed this Apr 21, 2013

smic commented Apr 24, 2013

@tonyarnold Yes, exactly. I don't know if it exists a faster way doing this, but I use more or less the following code:

// get all entities matching the IDs
    NSArray *personIds = ...;
    NSMutableDictionary *existingObjects = [NSMutableDictionary dictionaryWithCapacity:[personIds count]];
    for (MyPerson *person in [MyPerson findAllWithPredicate:[NSPredicate predicateWithFormat: @"(id IN %@)", personIds] inContext:context]) {
        NSAssert1(![existingObjects objectForKey:person.id], @"Unexpected object for ID %@", person.id);
        [existingObjects setObject:person forKey:person.id];
    }

[...]

            NSString *personId = ...;
            MyPerson *person = [existingObjects objectForKey:personId];
            if (!person) {
                person =  [MyPerson createInContext:context];
                [existingObjects setObject:person forKey:personId];
            }
Contributor

tonyarnold commented Apr 24, 2013

I'd imagine you could use an NSExpression to get the SQL query down to something really tight and fast (assuming you're using the SQLite backend). The approach you've suggested is fine for very small data sets, but it won't scale (assuming you're not running the import in batches). I'm sure @casademora has a lot more to add here — I've actually rarely had need to import data into Core Data.

smic commented Apr 24, 2013

It works fine for a webservice call with around 2000 entities. In this context you can't call 2000x findOrCreate. "(id IN %@)" is just one fetch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment