feat: require toVersion in migrate()

ipfs · Oct 20, 2019 · 1596dfe · 1596dfe
1 parent 1c923e2
commit 1596dfe
Show file tree

Hide file tree

Showing 7 changed files with 65 additions and 59 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -36,6 +36,13 @@ jobs:
         firefox: latest
       script: npx aegir test -t browser -- --browsers FirefoxHeadless
 
+    - stage: test
+      name: sharness
+      os:
+        - linux
+        - osx
+      script: cd ./test/sharness && make
+
 notifications:
   email: false
 
diff --git a/README.md b/README.md
@@ -77,30 +77,30 @@ Example:
 
 ```js
 const migrations = require('ipfs-repo-migrations')
-const getVersion = require('ipfs-repo-migrations/repo/version')
 
 const repoPath = 'some/repo/path'
-const repoVersion = await getVersion(repoPath)
+const currentRepoVersion = 7
+const latestVersion = migrations.getLatestMigrationVersion()
 
-if(repoVersion < migrations.getLatestMigrationVersion()){
+if(currentRepoVersion < latestVersion){
   // Old repo! Lets migrate to latest version!
-  await migrations.migrate(repoPath)
+  await migrations.migrate(repoPath, latestVersion)
 }
 ```
 
 To migrate your repository using the CLI, see the [how to run migrations](./run.md) tutorial. 
 
 ## API
 
-### `.migrate(path, {toVersion, ignoreLock, repoOptions, onProgress, isDryRun}) -> Promise<void>`
+### `.migrate(path, toVersion, {ignoreLock, repoOptions, onProgress, isDryRun}) -> Promise<void>`
 
 Executes a forward migration to a specific version, or to the latest version if a specific version is not specified.
 
 **Arguments:**
 
  * `path` (string, mandatory) - path to the repo to be migrated
+ * `toVersion` (int, mandatory) - version to which the repo should be migrated.
  * `options` (object, optional) - options for the migration
- * `options.toVersion` (int, optional) - version to which the repo should be migrated. Defaults to the latest migration version.
  * `options.ignoreLock` (bool, optional) - if true will not lock the repo when applying migrations. Use with caution.
  * `options.repoOptions` (object, optional) - options that are passed to migrations, that use them to construct the datastore. (options are the same as for IPFSRepo).
  * `options.onProgress` (function, optional) - callback that is called after finishing execution of each migration to report progress.

diff --git a/src/commands.js b/src/commands.js
@@ -36,7 +36,7 @@ async function migrate ({ repoPath, migrations, to, dry, revertOk }) {
   }
 
   if (!to) {
-    to = migrator.getLatestMigrationVersion()
+    to = migrator.getLatestMigrationVersion(migrations)
   }
 
   const version = await repoVersion.getVersion(repoPath)
@@ -63,7 +63,7 @@ async function migrate ({ repoPath, migrations, to, dry, revertOk }) {
   if (version === to) {
     return chalk.yellow('Nothing to migrate! Versions matches')
   } else if (version < to) {
-    await migrator.migrate(repoPath, options)
+    await migrator.migrate(repoPath, to, options)
   } else if (revertOk) {
     await migrator.revert(repoPath, to, options)
   } else {

diff --git a/src/index.js b/src/index.js
@@ -42,17 +42,18 @@ exports.getLatestMigrationVersion = getLatestMigrationVersion
  * Signature of the progress callback is: function(migrationObject: object, currentMigrationNumber: int, totalMigrationsCount: int)
  *
  * @param {string} path - Path to initialized (!) JS-IPFS repo
+ * @param {int} toVersion - Version to which the repo should be migrated.
  * @param {Object} options - Options for migration
- * @param {int?} options.toVersion - Version to which the repo should be migrated, if undefined repo will be migrated to the latest version.
  * @param {boolean?} options.ignoreLock - Won't lock the repo for applying the migrations. Use with caution.
  * @param {object?} options.repoOptions - Options that are passed to migrations, that can use them to correctly construct datastore. Options are same like for IPFSRepo.
  * @param {function?} options.onProgress - Callback which will be called after each executed migration to report progress
  * @param {boolean?} options.isDryRun - Allows to simulate the execution of the migrations without any effect.
  * @param {array?} options.migrations - Array of migrations to migrate. If undefined, the bundled migrations are used. Mainly for testing purpose.
  * @returns {Promise<void>}
  */
-async function migrate (path, { toVersion, ignoreLock = false, repoOptions, onProgress, isDryRun = false, migrations }) {
+async function migrate (path, toVersion, {ignoreLock = false, repoOptions, onProgress, isDryRun = false, migrations }) {
   migrations = migrations || defaultMigrations
+  onProgress = onProgress || (() => {})
 
   if (!path) {
     throw new errors.RequiredParameterError('Path argument is required!')
@@ -62,10 +63,13 @@ async function migrate (path, { toVersion, ignoreLock = false, repoOptions, onPr
     throw new errors.NotInitializedRepoError(`Repo in path ${path} is not initialized!`)
   }
 
-  if (toVersion && (!Number.isInteger(toVersion) || toVersion <= 0)) {
+  if (!toVersion) {
+    throw new errors.RequiredParameterError('toVersion argument is required!')
+  }
+
+  if (!Number.isInteger(toVersion) || toVersion <= 0) {
     throw new errors.InvalidValueError('Version has to be positive integer!')
   }
-  toVersion = toVersion || getLatestMigrationVersion(migrations)
 
   if (toVersion > getLatestMigrationVersion(migrations)) {
     throw new errors.InvalidValueError('The ipfs-repo-migrations package does not have migration for version: ' + toVersion)
@@ -74,13 +78,12 @@ async function migrate (path, { toVersion, ignoreLock = false, repoOptions, onPr
   const currentVersion = await repoVersion.getVersion(path)
 
   if (currentVersion === toVersion) {
-    log('Nothing to migrate, skipping migrations.')
+    log('Nothing to migrate.')
     return
   }
 
   if (currentVersion > toVersion) {
-    log(`Current repo's version (${currentVersion}) is higher then toVersion (${toVersion}), nothing to migrate.`)
-    return
+    throw new errors.InvalidValueError(`Current repo's version (${currentVersion}) is higher then toVersion (${toVersion}), you probably wanted to revert it?`)
   }
 
   let lock
@@ -110,7 +113,7 @@ async function migrate (path, { toVersion, ignoreLock = false, repoOptions, onPr
         throw e
       }
 
-      typeof onProgress === 'function' && onProgress(migration, counter, totalMigrations) // Reports on migration process
+      onProgress(migration, counter, totalMigrations) // Reports on migration process
       log(`Migrating to version ${migration.version} finished`)
     }
 
@@ -141,6 +144,7 @@ exports.migrate = migrate
  */
 async function revert (path, toVersion, { ignoreLock = false, repoOptions, onProgress, isDryRun = false, migrations }) {
   migrations = migrations || defaultMigrations
+  onProgress = onProgress || (() => {})
 
   if (!path) {
     throw new errors.RequiredParameterError('Path argument is required!')
@@ -160,13 +164,12 @@ async function revert (path, toVersion, { ignoreLock = false, repoOptions, onPro
 
   const currentVersion = await repoVersion.getVersion(path)
   if (currentVersion === toVersion) {
-    log('Nothing to revert, skipping reverting.')
+    log('Nothing to revert.')
     return
   }
 
   if (currentVersion < toVersion) {
-    log(`Current repo's version (${currentVersion}) is lower then toVersion (${toVersion}), nothing to revert.`)
-    return
+    throw new errors.InvalidValueError(`Current repo's version (${currentVersion}) is lower then toVersion (${toVersion}), you probably wanted to migrate it?`)
   }
 
   const reversibility = verifyReversibility(migrations, currentVersion, toVersion)
@@ -204,7 +207,7 @@ async function revert (path, toVersion, { ignoreLock = false, repoOptions, onPro
         throw e
       }
 
-      typeof onProgress === 'function' && onProgress(migration, counter, totalMigrations) // Reports on migration process
+      onProgress(migration, counter, totalMigrations) // Reports on migration process
       log(`Reverting to version ${migration.version} finished`)
     }
 

diff --git a/test/index.spec.js b/test/index.spec.js
@@ -38,10 +38,9 @@ function createMigrations () {
   ]
 }
 
-function createOptions (toVersion) {
+function createOptions () {
   return {
     migrations: createMigrations(),
-    toVersion
   }
 }
 
@@ -123,7 +122,7 @@ describe('index.js', () => {
       const options = createOptions()
 
       await expect(migrator.revert('/some/path', 3, options))
-        .to.eventually.be.fulfilled()
+        .to.eventually.be.rejectedWith(errors.InvalidValueError).with.property('code', errors.InvalidValueError.code)
 
       expect(lockStub.called).to.be.false()
     })
@@ -266,61 +265,58 @@ describe('index.js', () => {
     it('should error with out path argument', () => {
       const options = createOptions()
 
-      return expect(migrator.migrate(undefined, options))
+      return expect(migrator.migrate(undefined, undefined, options))
+        .to.eventually.be.rejectedWith(errors.RequiredParameterError).with.property('code', errors.RequiredParameterError.code)
+    })
+
+    it('should error with out toVersion argument', () => {
+      const options = createOptions()
+
+      return expect(migrator.migrate('/some/path', undefined, options))
         .to.eventually.be.rejectedWith(errors.RequiredParameterError).with.property('code', errors.RequiredParameterError.code)
     })
 
     it('should error with invalid toVersion argument', () => {
-      const invalidValues = ['eight', '-1', '1', -1]
+      const invalidValues = ['eight', '-1', '1', -1, {}]
 
       return Promise.all(
-        invalidValues.map((invalidValue) => expect(migrator.migrate('/some/path', createOptions(invalidValue)))
+        invalidValues.map((invalidValue) => expect(migrator.migrate('/some/path', invalidValue, createOptions()))
           .to.eventually.be.rejectedWith(errors.InvalidValueError).with.property('code', errors.InvalidValueError.code))
       )
     })
 
     it('should error if migrations does not exist', () => {
-      const options = createOptions(5)
-
-      return expect(migrator.migrate('/some/path', options))
-        .to.eventually.be.rejectedWith(errors.InvalidValueError).with.property('code', errors.InvalidValueError.code)
-    })
-
-    it('should use latest migration\'s version if no toVersion is provided', async () => {
       const options = createOptions()
-      getVersionStub.returns(2)
-
-      await expect(migrator.migrate('/some/path', options))
-        .to.eventually.be.fulfilled()
 
-      setVersionStub.calledOnceWithExactly('/some/path', 4) // 4 is the latest migration's version
+      return expect(migrator.migrate('/some/path', 5, options))
+        .to.eventually.be.rejectedWith(errors.InvalidValueError).with.property('code', errors.InvalidValueError.code)
     })
 
     it('should not migrate if current repo version and toVersion matches', async () => {
       getVersionStub.returns(2)
-      const options = createOptions(2)
+      const options = createOptions()
 
-      await expect(migrator.migrate('/some/path', options))
+      await expect(migrator.migrate('/some/path', 2, options))
         .to.eventually.be.fulfilled()
 
       expect(lockStub.called).to.be.false()
     })
 
     it('should not migrate if current repo version is higher then toVersion', async () => {
       getVersionStub.returns(3)
-      const options = createOptions(2)
+      const options = createOptions()
 
-      await expect(migrator.migrate('/some/path', options))
-        .to.eventually.be.fulfilled()
+      await expect(migrator.migrate('/some/path', 2, options))
+        .to.eventually.be.rejectedWith(errors.InvalidValueError).with.property('code', errors.InvalidValueError.code)
 
       expect(lockStub.called).to.be.false()
     })
 
     it('should migrate expected migrations', async () => {
-      const options = createOptions(3)
+      const options = createOptions()
       getVersionStub.returns(1)
 
-      await expect(migrator.migrate('/some/path', options))
+      await expect(migrator.migrate('/some/path', 3, options))
         .to.eventually.be.fulfilled()
 
       expect(lockCloseStub.calledOnce).to.be.true()
@@ -335,11 +331,11 @@ describe('index.js', () => {
     })
 
     it('should not have any side-effects when in dry run', async () => {
-      const options = createOptions(4)
+      const options = createOptions()
       options.isDryRun = true
       getVersionStub.returns(2)
 
-      await expect(migrator.migrate('/some/path', options))
+      await expect(migrator.migrate('/some/path', 4, options))
         .to.eventually.be.fulfilled()
 
       expect(lockCloseStub.called).to.be.false()
@@ -354,7 +350,7 @@ describe('index.js', () => {
       options.ignoreLock = true
       getVersionStub.returns(2)
 
-      await expect(migrator.migrate('/some/path', options))
+      await expect(migrator.migrate('/some/path', 4,  options))
         .to.eventually.be.fulfilled()
 
       expect(lockCloseStub.called).to.be.false()
@@ -369,11 +365,11 @@ describe('index.js', () => {
     })
 
     it('should report progress when progress callback is supplied', async () => {
-      const options = createOptions(4)
+      const options = createOptions()
       options.onProgress = sinon.stub()
       getVersionStub.returns(2)
 
-      await expect(migrator.migrate('/some/path', options))
+      await expect(migrator.migrate('/some/path', 4, options))
         .to.eventually.be.fulfilled()
 
       expect(options.onProgress.getCall(0).calledWith(sinon.match.any, 1, 2)).to.be.true()
@@ -382,10 +378,10 @@ describe('index.js', () => {
 
     it('should unlock repo when error is thrown', async () => {
       getVersionStub.returns(2)
-      const options = createOptions(4)
+      const options = createOptions()
       options.migrations[3].migrate = sinon.stub().rejects()
 
-      await expect(migrator.migrate('/some/path', options))
+      await expect(migrator.migrate('/some/path', 4, options))
         .to.eventually.be.rejected()
 
       expect(lockCloseStub.calledOnce).to.be.true()

diff --git a/test/integration-test.js b/test/integration-test.js
@@ -23,7 +23,7 @@ module.exports = (setup, cleanup) => {
   )
 
   it('migrate forward', async () => {
-    await migrator.migrate(dir, { migrations: migrations })
+    await migrator.migrate(dir, migrator.getLatestMigrationVersion(migrations), { migrations: migrations })
 
     const store = new Datastore(dir, { extension: '', createIfMissing: false })
 
@@ -37,7 +37,7 @@ module.exports = (setup, cleanup) => {
   })
 
   it('revert', async () => {
-    await migrator.migrate(dir, { migrations: migrations })
+    await migrator.migrate(dir, migrator.getLatestMigrationVersion(migrations), { migrations: migrations })
 
     await migrator.revert(dir, 1, { migrations: migrations })
 

diff --git a/test/sharness/t0010-basic-commands.sh b/test/sharness/t0010-basic-commands.sh
@@ -8,7 +8,7 @@ test_description="Test installation and some basic commands"
 
 # setup temp repo
 rm -rf ./tmp
-cp -r ../test-repo ./tmp
+cp -r ../fixtures/test-repo ./tmp
 IPFS_PATH=$(echo `pwd`/tmp)
 export IPFS_PATH
 
@@ -29,9 +29,9 @@ test_expect_success "jsipfs-migrations help succeeds" '
 
 test_expect_success "jsipfs-migrations status shows migrations are needed" $'
 	jsipfs-migrations status --migrations ../test/test-migrations > status.txt &&
-	grep "There are migrations to be applied!" status.txt &&
+	grep "Repo is out of date" status.txt &&
 	grep "Current repo version: 1" status.txt &&
-	grep "Last migration\'s version: 2" status.txt
+	grep "Latest migration version: 2" status.txt
 '
 
 test_expect_success "jsipfs-migrations successfully migrate to latest version" $'
@@ -41,9 +41,9 @@ test_expect_success "jsipfs-migrations successfully migrate to latest version" $
 
 test_expect_success "jsipfs-migrations status shows NO migrations are needed" $'
 	jsipfs-migrations status --migrations ../test/test-migrations > status.txt &&
-	grep "Nothing to migrate!" status.txt &&
+	grep "Nothing to migrate" status.txt &&
 	grep "Current repo version: 2" status.txt &&
-	grep "Last migration\'s version: 2" status.txt
+	grep "Latest migration version: 2" status.txt
 '
 
 test_expect_success "jsipfs-migrations successfully reverts" $'