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

Rename db file #110

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Rename db file #110

wants to merge 9 commits into from

Conversation

djih
Copy link
Member

@djih djih commented Aug 12, 2016

This mirrors the db file renaming work done on Android: https://github.com/amplitude/Amplitude-Android/pull/112/files

Tested the renaming on demo app on both simulator and real test device. Also tested that subsequent re-opening of app skips the renaming. Tested that all the database data was still intact (previous sessionId, last event time, deviceId, event id, etc). Added two unit tests as well.

A lot of the test updates is just fixing the call to getDatabaseHelper. I added a new extra method getDatabaseHelperWithInstanceName just for testing, explicitly with a new name so that any uncaught calls to the old getDatabaseHelper:instanceName would not erroneously get mapped to the test method. The purpose of the test method is to re-generate the old database file, sidestepping the static _instances which I cannot access directly.

We already have a helper method to move files around, so I just factored it out into AMPUtils.

Note: the new database filename is in the form com.amplitude.database_instancename_apiKey. I was a little worried that the filename might become too long. iOS limits the full file path to 1024 chars, and the actual filename to 255 chars. On the actual test device, the full path looks like this: /var/mobile/Containers/Data/Application/85A9D80F-FA07-456F-A1CB-BD4C54145649/Library/com.amplitude.database_app2_c6fa6b27f515fb3f0e5ece6caf86027b, which is only 145 characters. So I think it's safe to use that format for the new name.

This builds on top of #109. Comments from that PR:
We need to migrate any database interaction logic in the init method into the initializeApiKey method since we will need the apiKey to interact with the database. I also run the init logic on the backgroundQueue, removing the need for a separate initializerQueue.

Few things to note:

  • _dbHelper is initialized in initializeApiKey now. Any public methods that interact with the database such as setUserId, setDeviceId, identify, etc need to be guarded with an apiKey check to ensure [self.dbHelper] is available
  • I moved [self addObservers] to initializeApiKey since enterBackground interacts with the DB by saving the current timestamp.
  • Most tests still pass. Just need to flush the background queue instead of the initializerQueue.
  • Reran all tests on iPhone 4, iPhone 5, iPhone 6S Plus, tested demo app on test device

NSError *error;
if (![fileManager fileExistsAtPath:to] &&
[fileManager fileExistsAtPath:from]) {
if ([fileManager copyItemAtPath:from toPath:to error:&error]) {
Copy link
Member

Choose a reason for hiding this comment

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

can you use moveItemAtPath?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

@curtisliu
Copy link
Member

LGTM, update readme

@djih
Copy link
Member Author

djih commented Aug 22, 2016

RECAP of this PR's changes

  • AMPDatabaseHelper requires apiKey when getting the instance
  • AMPDatabaseHelper attempts to migrate DB file from old scope to new scope during init
  • remove initializeApiKey, replace with setApiKey and initialize (also removed any deprecated initializeApiKey methods)
  • Removed initializerQueue - we can re-use backgroundQueue
  • Removed initialized bool, we can just check if apiKey is nil to see if we've already called setApiKey
  • Migrate any DB logic from init to setApiKey
  • Updated a lot of methods to return Amplitude instance to allow chaining of multiple methods.
  • Removed optOut boolean property. We already override it with custom setter and getters that read/write directly from database
  • Updated tests - removed all of the local databaseHelper instances created in each test, can re-use the global one created by BaseTestCase
  • Updated documentation in Amplitude.h
  • Updated documentation in README.mh

PTAL @curtisliu

@haoliu-amp
Copy link
Contributor

@djih close this?

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.

3 participants