Amplitude-iOS icon indicating copy to clipboard operation
Amplitude-iOS copied to clipboard

Rename db file

Open djih opened this issue 8 years ago • 3 comments

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 https://github.com/amplitude/Amplitude-iOS/pull/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

djih avatar Aug 12 '16 00:08 djih

LGTM, update readme

curtisliu avatar Aug 19 '16 22:08 curtisliu

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

djih avatar Aug 22 '16 22:08 djih

@djih close this?

haoliu-amp avatar Feb 06 '20 19:02 haoliu-amp