AlexDenisov / iActiveRecord

ActiveRecord for iOS without CoreData, only SQLite
http://alexdenisov.github.com/iActiveRecord/
MIT License
354 stars 50 forks source link

belongs_to id naming conventions #16

Closed pwightman closed 12 years ago

pwightman commented 12 years ago

I've found what appears to be naming inconsistencies in relationships, though I may just be doing something wrong. I have the following code settings up a has_many relationships where BBSongs have_many BBGrooves:

// BBSong.h
@interface BBSong : ActiveRecord
    ...
    has_many_dec(BBGroove, grooves, ARDependencyDestroy)
@end

// BBSong.m
@implementation BBSong
    has_many_imp(BBGroove, grooves, ARDependencyDestroy)
@end

// BBGroove.h
@interface BBGroove : ActiveRecord
    ...
    belongs_to_dec(BBSong, song, ARDependencyDestroy)
@end

// BBGroove.m
@implementation BBGroove
    belongs_to_imp(BBSong, song, ARDependencyDestroy)
@end

// main code
BBSong *song = [BBSong newRecord];
song.title = @"Foobar";
[song save];

BBGroove *groove = [BBGroove newRecord];

[song addBBGroove:groove];

NSLog(@"All grooves: %@", [BBSong first]);

ARLazyFetcher *fetcher = [song grooves];

NSLog(@"Song's groove: %@", [fetcher fetchRecords]); // This is where the error is thrown, in fetchRecords

I get this error:

2012-08-01 21:11:08.035 BeatBuilder[32686:c07] *** Terminating app due to uncaught exception 'NSUnknownKeyException', reason: '[<BBGroove 0x8a70c20> setValue:forUndefinedKey:]: this class is not key value coding-compliant for the key bBsongId.'

So I noticed its doing its own camelcase sort of thing, so I kept looking into it. Inside ARLazyFetch.m in the buildSql method, I printed out the SQL and it looks like this: http://d.pr/i/DCv4/2DbVKVRz

Notice that in the SELECT it's bBSongId, but in the WHERE it's BBSongId, which seems inconsistent.

I kept going, and inside the allRecordsWithName:withSql: method, I stopped it at the association: http://d.pr/i/DU2l/1drZqAQA

Notice that the propertyName is bBsongId, a third variation. I'm looking into it some more, but I just wanted to get my findings up as an issue first. I imagine that this might not have come up before since the examples in your app don't use the two-letter namespace prefix, which might be messing things up.

Thanks!

pwightman commented 12 years ago

Also, this line in allRecordsWithName:withSql: is what ultimately throws the error:

[record setValue:aValue forKey:propertyName];
pwightman commented 12 years ago

Column names in sqlite are not case sensitive, from what I can read online, which explain why the query works just fine. But the field must be created as "bBsongId" but the accessor is "bBSongId."

Sorry, just using this as a log for my thoughts at the moment before I go to bed, I'll work on a fix in the morning.

pwightman commented 12 years ago

So removing the app and reinstalling seems to have fixed it. I noticed that every time I ran the app, it was trying to add bBSongId to the DB, but got an error saying the column already existed (again, likely due to case sensitivity in the Obj-C code but not in sqlite column names). I'm just curious how bBsongId would have gotten in the DB to begin with... I'll think about it more in the morning.

pwightman commented 12 years ago

Perhaps this is an argument for downcasing all column names in all SQL queries and pertinent AR database-related files?

AlexDenisov commented 12 years ago

Thanks for feedback. You're right. I have implemented an applications within prefixes on a models, and found that this is uncomfortable. Right now I'm working on new version (master branch) and thinking about how to do this stuff more flexible. I think that downcasing columns in SQL queries is overhead and not necessary, but I'll consider your suggestions.

AlexDenisov commented 12 years ago

Did you solved your problem? Can I close this issue?

pwightman commented 12 years ago

I think I've realized where the actual bug is, it was that I originally had this as my belongs_to property:

@property (nonatomic, strong) NSNumber *bBsongId;

However, when that threw an error, I switched it to bBSongId (which is what it's supposed to be, I'm assuming), but it didn't properly remove the old column (likely due to case insensitivity) and just kept trying to add the new column. So I think that's the actual bug, which I can create a ticket for if you like, but we can close this one now.

AlexDenisov commented 12 years ago

This is not a bug, this is a feature =) The problem is that sqlite does not provide interface to rename the columns of the table. I can solve it with temporary table, but it will lead to overhead, too. If you have any suggestions about how to solve this problem - you're welcome ;-)