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

#278 migration support for database delivered in framework #286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

#278 migration support for database delivered in framework #286

wants to merge 1 commit into from

Conversation

tomasz-czyzak
Copy link
Contributor

No description provided.

@tomasz-czyzak
Copy link
Contributor Author

Feel free to close it if #268 fixes it


if (modelBundle == nil)
{
modelBundle = [NSBundle mainBundle];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the main bundle is fetched and then later added to an array on line 983. However according to Apple documentation on the mainBundle method the method can return nil. Therefore an error will occur when attempting to add nil to an array.

This method may return a valid bundle object even for unbundled apps. It may also return nil if the bundle object could not be created, so always check the return value.

You could add a additional check and error code for this case if you wanted to make this code as resilient as possible.

typedef NS_ENUM(NSInteger, EncryptedStoreError)
{
    EncryptedStoreErrorIncorrectPasscode = 6000,
    EncryptedStoreErrorMigrationFailed,
    EncryptedStoreErrorModelLoadFailed
};
if (modelBundle == nil)
{
    if (error)
    {
        NSDictionary * userInfo = @{EncryptedStoreErrorMessageKey : @"Main Bundle not found, cannot load model"};
        *error = [NSError errorWithDomain: EncryptedStoreErrorDomain 
                                     code: EncryptedStoreErrorModelLoadFailed 
                                 userInfo: userInfo];
    }
    return NO;
}

Copy link
Contributor Author

@tomasz-czyzak tomasz-czyzak Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree with you about handling mainBundle method however it is kind of overkill since if mainBundle is nil then we are in more serious troubles than handling error correctly,
Also getting modelBundle from mainBundle assumes that model is delivered inside app while issue which has been fixed by this PR is to migrate correctly model delivered in framework

Anyway feel free to raise separate PR for fixing mainBundle potential nil return.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, perhaps it would be an extreme edge case.

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

Successfully merging this pull request may close these issues.

None yet

3 participants