magicalpanda / MagicalRecord

Super Awesome Easy Fetching for Core Data!
Other
10.79k stars 1.79k forks source link

Bug in NSManagedObject methods entityDescription and entityDescriptionInContext: #387

Closed bachand closed 10 years ago

bachand commented 11 years ago

See the below code from NSManagedObject+MagicalRecord.m. Computing entityName using the name of the NSManagedObject (sub)class is not correct since it's possible (and, as far as I know, completely valid) to give a NSManagedObject subclass a name different than the name of the entity to which it corresponds. For example, you could create an entity in Core Data named "Foo" and then link it to a corresponding NSManagedObject subclass called "Bar". In that case, both [Bar entityDescription] and [Bar entityDescriptionInContext:myContext] will return nil assuming the NSManagedObject doesn't respond to entityInManagedObjectContext: (which it didn't in my test case).

+ (NSString *) MR_entityName
{
    return NSStringFromClass(self);
}

+ (NSEntityDescription *) MR_entityDescriptionInContext:(NSManagedObjectContext *)context
{
    if ([self respondsToSelector:@selector(entityInManagedObjectContext:)]) 
    {
        NSEntityDescription *entity = [self performSelector:@selector(entityInManagedObjectContext:) withObject:context];
        return entity;
    }
    else
    {
        NSString *entityName = [self MR_entityName];
        return [NSEntityDescription entityForName:entityName inManagedObjectContext:context];
    }
}
casademora commented 11 years ago

That's why you have the MR_entityName method to override and set your name explicitly. There are 3 cases here:

1) Assume the class and entity names match.
2) Assume you're using mogenerator and use the mapping mogenerator provides for you.
3) Assume you have a custom entity name to class name mapping or you're not using mogenerator

The MR_entityName method gives you the opportunity to customize that name. This works because in your class, Bar, you need to implement your custom MR_entityName method to return what you've set in your Model file...

Saul Mora @casademora saul@casademora.com

On Wednesday, January 16, 2013 at 12:15 PM, Michael Bachand wrote:

See the below code from NSManagedObject+MagicalRecord.m. Computing entityName using the name of the NSManagedObject (sub)class is not correct since it's possible (and, as far as I know, completely valid) to give a NSManagedObject subclass a name different than the name of the entity to which it corresponds. For example, you could create an entity in Core Data named "Foo" and then link it to a corresponding NSManagedObject subclass called "Bar". In that case, both [Bar entityDescription] and [Bar entityDescriptionInContext:myContext] will return nil assuming the NSManagedObject doesn't respond to entityInManagedObjectContext: (which it didn't in my test case).

  • (NSString ) MR_entityName { return NSStringFromClass(self); } + (NSEntityDescription ) MR_entityDescriptionInContext:(NSManagedObjectContext )context { if ([self respondsToSelector:@selector(entityInManagedObjectContext:)]) { NSEntityDescription entity = [self performSelector:@selector(entityInManagedObjectContext:) withObject:context]; return entity; } else { NSString *entityName = [self MR_entityName]; return [NSEntityDescription entityForName:entityName inManagedObjectContext:context]; } }

— Reply to this email directly or view it on GitHub (https://github.com/magicalpanda/MagicalRecord/issues/387).

bachand commented 11 years ago

True, that's reasonable. If that's the expectation, though, would it make more sense to have MR_entityName declared in a .h file then? I realize it works without such a declaration, but it may be more clear if that method is more visible.

casademora commented 11 years ago

That part is an oversight on my end. It should definitely be in a header.

Saul Mora @casademora saul@casademora.com

On Wednesday, January 16, 2013 at 12:34 PM, Michael Bachand wrote:

True, that's reasonable. If that's the expectation, though, would it make more sense to have MR_entityName declared in a .h file then? I realize it works without such a declaration, but it may be more clear if that method is more visible.

— Reply to this email directly or view it on GitHub (https://github.com/magicalpanda/MagicalRecord/issues/387#issuecomment-12335733).

bachand commented 11 years ago

Sounds good, thanks for your quick responses!

bachand commented 11 years ago

Just wanted to follow-up on this @casademora , when do you think this will be resolved? Thanks!

sergiou87 commented 10 years ago

Hi @bachand @casademora !

According to Apple Documentation (https://developer.apple.com/library/mac/documentation/cocoa/conceptual/ProgrammingWithObjectiveC/CustomizingExistingClasses/CustomizingExistingClasses.html#//apple_ref/doc/uid/TP40011210-CH6-SW4, Avoid Category Method Name Clashes) you shouldn't have the same method in a category and in a class because the behaviour is undefined.

Therefore, if I understood correctly, that approach is somehow dangerous and we should opt to add a setter with an associated object or something like that to allow this entity name customization.

Another option: when MR_entityName is invoked, look for another method (for example, MR_customEntityName) and return its value if it exists, or return the class name if not:

+ (NSString *) MR_entityName
{
    if ([self respondsToSelector:@selector(MR_customEntityName)])
        return [self MR_customEntityName];
    else
        return NSStringFromClass(self);
}

In this case, we must somehow make this method public and optional (a MRCustomEntityName protocol conformed by a category on NSObject?)

What do you think?

notxcain commented 10 years ago

@sergiou87 I think it's okay to declare a method in a category on a base class, and override it in a subclass. What Apple says is not to declare the method in a category on a class where this method is already declared.

tonyarnold commented 10 years ago

@denis-mikhaylov overriding anything from a category is risky business. Even in the situation you've described, the subclass implementation of the category method may actually inherit the behaviour of the category, and not the subclass.

We've modified this code in develop to use a private method named MR_bestGuessAtAnEntityName. This new method is used throughout MagicalRecord in place of MR_entityName or entityName.

The behaviour of MR_bestGuessAtAnEntityName is to see if the NSManagedObject subclass has defined the method entityName, and use that if it's available. If not, it will convert the class name into a string and use that.

You can find more detail about this change in commit 8a68ebe94acf.