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

feat(DataStore): database recreation based on new schema #551

Merged
merged 10 commits into from
Jun 19, 2020

Conversation

ruiguoamz
Copy link
Contributor

@ruiguoamz ruiguoamz commented Jun 16, 2020

Description of changes:

  • When user updates schema, the database should be recreated
  • Store schema version in the UserDefaults database
  • Use ModelRegistration.version to verify the right one is being used when starting the StorageAdapter
  • Have log warning that the database will be re-created
  • Added 4 unit tests
  • Added 2 integration tests
  • Need to update DataStore documentation describing the behavior

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ruiguoamz ruiguoamz added the datastore Issues related to the DataStore category label Jun 16, 2020
@ruiguoamz ruiguoamz marked this pull request as ready for review June 17, 2020 01:07
@ruiguoamz ruiguoamz requested a review from lawmicha June 17, 2020 01:08
@@ -16,12 +16,11 @@ final class SQLiteStorageEngineAdapter: StorageEngineAdapter {

internal var connection: Connection!
private var dbFilePath: URL?
static let dbVersionKey = "com.awsamazon.dataStore.dbVersion"
Copy link
Contributor

Choose a reason for hiding this comment

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

a search for "com." shows a few examples like com.amazonaws.DataStore.AWSIncomingEventReconciliationQueue so let's keep it the same like com.amazonaws.DataStore.dbVersion

Copy link
Contributor Author

@ruiguoamz ruiguoamz Jun 17, 2020

Choose a reason for hiding this comment

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

Changed, but in PictureThis the bundle identifier is "com.awsamazon.PictureThis" instead of "com.amazonaws.PictureThis". I guess "awsamazon" or "amazonaws" doesn't matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh good point, not sure which is the right one, more important for the codebase to be consistent regardless of PictureThis's bundle since it's external sample app

_ = UserDefaults.removeObject(userDefaults)
}

func testVersionInUserDefaultsRemainsSameAndFileExists() {
Copy link
Contributor

Choose a reason for hiding this comment

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

test name like testClearIfNewVersionWithVersionSameAsPrevious

let previousVersion = "previousVersion"
userDefaults.set(previousVersion ..

try SQLiteStorageEngineAdaapter.clearIfNewVersion(previousVersion,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, here is having an additional var necessary?
userDefaults.set("previousVersion", forKey: SQLiteStorageEngineAdapter.dbVersionKey) is OK? Because "previousVersion" is used only once.

userDefaults: userDefaults,
fileManager: mockFileManager)
} catch {
XCTAssertEqual(error as? MockFileManagerError, MockFileManagerError.removeItemError)
Copy link
Contributor

Choose a reason for hiding this comment

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

assert is DataStoreError.invalidDatabase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

try fileManager.removeItem(at: dbFilePath)
} catch {
log.error("\(#function) Failed to delete database file located at: \(dbFilePath), error: \(error)")
throw error
Copy link
Contributor

Choose a reason for hiding this comment

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

wrap in DataStoreError.invalidDatabase(path: dbFilePath.absoluteString, error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ruiguoamz ruiguoamz merged commit eb5b7ff into master Jun 19, 2020
@ruiguoamz ruiguoamz deleted the datastore/dbrecreation branch June 19, 2020 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datastore Issues related to the DataStore category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants