Handle Cross-Datastore Duplicates, introducing scratch datastores (#3394

) * [WIP] handle cross-datastore dataset duplicates * introduce isScratch property for datastores * reset config files * fix views in evolutions * Update dataStores.csv * new snapshots (new datastore field isScratch)
scalableminds · Oct 29, 2018 · 9406db7 · 9406db7
1 parent 2aa8415
commit 9406db7
Show file tree

Hide file tree

Showing 15 changed files with 89 additions and 25 deletions.
diff --git a/.../snapshots/public/test-bundle/test/backend-snapshot-tests/annotations.e2e.js.md b/.../snapshots/public/test-bundle/test/backend-snapshot-tests/annotations.e2e.js.md
@@ -805,6 +805,7 @@ Generated by [AVA](https://ava.li).
       dataSetName: 'confocal-multi_knossos',
       dataStore: {
         isForeign: false,
+        isScratch: false,
         name: 'localhost',
         url: 'http://localhost:9000',
       },
@@ -879,6 +880,7 @@ Generated by [AVA](https://ava.li).
       dataSetName: '2012-06-28_Cortex',
       dataStore: {
         isForeign: false,
+        isScratch: false,
         name: 'localhost',
         url: 'http://localhost:9000',
       },
@@ -958,6 +960,7 @@ Generated by [AVA](https://ava.li).
       dataSetName: '2012-06-28_Cortex',
       dataStore: {
         isForeign: false,
+        isScratch: false,
         name: 'localhost',
         url: 'http://localhost:9000',
       },
@@ -1083,6 +1086,7 @@ Generated by [AVA](https://ava.li).
       dataSetName: '2012-06-28_Cortex',
       dataStore: {
         isForeign: false,
+        isScratch: false,
         name: 'localhost',
         url: 'http://localhost:9000',
       },
@@ -1167,6 +1171,7 @@ Generated by [AVA](https://ava.li).
       dataSetName: '2012-06-28_Cortex',
       dataStore: {
         isForeign: false,
+        isScratch: false,
         name: 'localhost',
         url: 'http://localhost:9000',
       },
@@ -1287,6 +1292,7 @@ Generated by [AVA](https://ava.li).
       dataSetName: '2012-06-28_Cortex',
       dataStore: {
         isForeign: false,
+        isScratch: false,
         name: 'localhost',
         url: 'http://localhost:9000',
       },
@@ -1412,6 +1418,7 @@ Generated by [AVA](https://ava.li).
       dataSetName: '2012-06-28_Cortex',
       dataStore: {
         isForeign: false,
+        isScratch: false,
         name: 'localhost',
         url: 'http://localhost:9000',
       },

diff --git a/...pts/test/snapshots/public/test-bundle/test/backend-snapshot-tests/annotations.e2e.js.snap b/...pts/test/snapshots/public/test-bundle/test/backend-snapshot-tests/annotations.e2e.js.snap
diff --git a/...est/snapshots/public/test-bundle/test/backend-snapshot-tests/datasets.e2e.js.md b/...est/snapshots/public/test-bundle/test/backend-snapshot-tests/datasets.e2e.js.md
@@ -223,6 +223,7 @@ Generated by [AVA](https://ava.li).
         },
         dataStore: {
           isForeign: false,
+          isScratch: false,
           name: 'localhost',
           url: 'http://localhost:9000',
         },
@@ -343,6 +344,7 @@ Generated by [AVA](https://ava.li).
         },
         dataStore: {
           isForeign: false,
+          isScratch: false,
           name: 'localhost',
           url: 'http://localhost:9000',
         },
@@ -382,6 +384,7 @@ Generated by [AVA](https://ava.li).
         },
         dataStore: {
           isForeign: false,
+          isScratch: false,
           name: 'localhost',
           url: 'http://localhost:9000',
         },
@@ -416,6 +419,7 @@ Generated by [AVA](https://ava.li).
         },
         dataStore: {
           isForeign: false,
+          isScratch: false,
           name: 'localhost',
           url: 'http://localhost:9000',
         },
@@ -450,6 +454,7 @@ Generated by [AVA](https://ava.li).
         },
         dataStore: {
           isForeign: false,
+          isScratch: false,
           name: 'localhost',
           url: 'http://localhost:9000',
         },
@@ -615,6 +620,7 @@ Generated by [AVA](https://ava.li).
         },
         dataStore: {
           isForeign: false,
+          isScratch: false,
           name: 'localhost',
           url: 'http://localhost:9000',
         },
@@ -735,6 +741,7 @@ Generated by [AVA](https://ava.li).
         },
         dataStore: {
           isForeign: false,
+          isScratch: false,
           name: 'localhost',
           url: 'http://localhost:9000',
         },
@@ -769,6 +776,7 @@ Generated by [AVA](https://ava.li).
         },
         dataStore: {
           isForeign: false,
+          isScratch: false,
           name: 'localhost',
           url: 'http://localhost:9000',
         },

diff --git a/...cripts/test/snapshots/public/test-bundle/test/backend-snapshot-tests/datasets.e2e.js.snap b/...cripts/test/snapshots/public/test-bundle/test/backend-snapshot-tests/datasets.e2e.js.snap
diff --git a/...ts/test/snapshots/public/test-bundle/test/backend-snapshot-tests/misc.e2e.js.md b/...ts/test/snapshots/public/test-bundle/test/backend-snapshot-tests/misc.e2e.js.md
@@ -45,6 +45,7 @@ Generated by [AVA](https://ava.li).
     [
       {
         isForeign: false,
+        isScratch: false,
         name: 'localhost',
         url: 'http://localhost:9000',
       },

diff --git a/...avascripts/test/snapshots/public/test-bundle/test/backend-snapshot-tests/misc.e2e.js.snap b/...avascripts/test/snapshots/public/test-bundle/test/backend-snapshot-tests/misc.e2e.js.snap
diff --git a/...s/test/snapshots/public/test-bundle/test/backend-snapshot-tests/tasks.e2e.js.md b/...s/test/snapshots/public/test-bundle/test/backend-snapshot-tests/tasks.e2e.js.md
@@ -60,6 +60,7 @@ Generated by [AVA](https://ava.li).
       dataSetName: 'confocal-multi_knossos',
       dataStore: {
         isForeign: false,
+        isScratch: false,
         name: 'localhost',
         url: 'http://localhost:9000',
       },
@@ -186,6 +187,7 @@ Generated by [AVA](https://ava.li).
         dataSetName: '2012-06-28_Cortex',
         dataStore: {
           isForeign: false,
+          isScratch: false,
           name: 'localhost',
           url: 'http://localhost:9000',
         },

diff --git a/...vascripts/test/snapshots/public/test-bundle/test/backend-snapshot-tests/tasks.e2e.js.snap b/...vascripts/test/snapshots/public/test-bundle/test/backend-snapshot-tests/tasks.e2e.js.snap
diff --git a/app/models/binary/DataSet.scala b/app/models/binary/DataSet.scala
@@ -194,7 +194,7 @@ class DataSetDAO @Inject()(sqlClient: SQLClient, dataSetDataLayerDAO: DataSetDat
     } yield ()
   }
 
-  def updateDataSourceByName(name: String, dataStoreName: String, source: InboxDataSource, isUsable: Boolean)(implicit ctx: DBAccessContext): Fox[Unit] = {
+  def updateDataSourceByNameAndOrganizationName(name: String, dataStoreName: String, source: InboxDataSource, isUsable: Boolean)(implicit ctx: DBAccessContext): Fox[Unit] = {
 
     for {
       old <- findOneByNameAndOrganizationName(name, source.id.team)
@@ -212,20 +212,20 @@ class DataSetDAO @Inject()(sqlClient: SQLClient, dataSetDataLayerDAO: DataSetDat
     } yield ()
   }
 
-  def deactivateUnreported(names: List[String], dataStoreName: String): Fox[Unit] = {
+  def deactivateUnreported(names: List[String], organizationId: ObjectId, dataStoreName: String): Fox[Unit] = {
     val inclusionPredicate = if (names.isEmpty) "true" else s"name not in ${writeStructTupleWithQuotes(names.map(sanitize))}"
     val deleteResolutionsQuery =
       sqlu"""delete from webknossos.dataSet_resolutions where _dataSet in
-            (select _id from webknossos.dataSets where _dataStore = ${dataStoreName}
+            (select _id from webknossos.dataSets where _dataStore = ${dataStoreName} and _organization = ${organizationId}
              and #${inclusionPredicate})"""
     val deleteLayersQuery =
       sqlu"""delete from webknossos.dataSet_layers where _dataSet in
-            (select _id from webknossos.dataSets where _dataStore = ${dataStoreName}
+            (select _id from webknossos.dataSets where _dataStore = ${dataStoreName} and _organization = ${organizationId}
              and #${inclusionPredicate})"""
     val setToUnusableQuery =
       sqlu"""update webknossos.datasets
              set isUsable = false, status = 'No longer available on datastore.', scale = NULL
-             where _dataStore = ${dataStoreName}
+             where _dataStore = ${dataStoreName} and _organization = ${organizationId}
              and #${inclusionPredicate}"""
     for {
       _ <- run(DBIO.sequence(List(deleteResolutionsQuery, deleteLayersQuery, setToUnusableQuery)).transactionally)

diff --git a/app/models/binary/DataSetService.scala b/app/models/binary/DataSetService.scala
@@ -98,16 +98,28 @@ class DataSetService @Inject()(organizationDAO: OrganizationDAO,
                       )(implicit ctx: DBAccessContext): Fox[Unit] = {
     dataSetDAO.findOneByNameAndOrganizationName(dataSource.id.name, dataSource.id.team)(GlobalAccessContext).futureBox.flatMap {
       case Full(dataSet) if dataSet._dataStore == dataStore.name =>
-        dataSetDAO.updateDataSourceByName(
+        dataSetDAO.updateDataSourceByNameAndOrganizationName(
           dataSource.id.name,
           dataStore.name,
           dataSource,
           dataSource.isUsable)(GlobalAccessContext).futureBox
-      case Full(_) =>
-        // TODO: There is a problem: The dataset name is already in use by some (potentially different) team.
-        // We are not going to update that datasource.
-        // this should be somehow populated to the user to inform him that he needs to rename the datasource
-        Fox.failure("dataset.name.alreadyInUse").futureBox
+      case Full(foundDataSet) =>
+        // The dataSet is already present (belonging to the same organization), but reported from a different datastore
+        (for {
+          originalDataStore <- dataStoreDAO.findOneByName(foundDataSet._dataStore)
+        } yield {
+          if (originalDataStore.isScratch && !dataStore.isScratch) {
+            logger.info(s"Replacing dataset ${foundDataSet.name} from scratch datastore ${originalDataStore.name} by the one from ${dataStore.name}")
+            dataSetDAO.updateDataSourceByNameAndOrganizationName(
+              dataSource.id.name,
+              dataStore.name,
+              dataSource,
+              dataSource.isUsable)(GlobalAccessContext)
+          } else {
+            logger.info(s"Dataset ${foundDataSet.name}, as reported from ${dataStore.name} is already present from datastore ${originalDataStore.name} and will not be replaced.")
+            Fox.failure("dataset.name.alreadyInUse")
+          }
+        }).flatten.futureBox
       case _ =>
         createDataSet(
           dataSource.id.name,
@@ -118,8 +130,15 @@ class DataSetService @Inject()(organizationDAO: OrganizationDAO,
     }
   }
 
-  def deactivateUnreportedDataSources(dataStoreName: String, dataSources: List[InboxDataSource])(implicit ctx: DBAccessContext) =
-    dataSetDAO.deactivateUnreported(dataSources.map(_.id.name), dataStoreName)
+  def deactivateUnreportedDataSources(dataStoreName: String, dataSources: List[InboxDataSource])(implicit ctx: DBAccessContext) = {
+    val dataSourcesByOrganizationName: Map[String, List[InboxDataSource]] = dataSources.groupBy(_.id.team)
+    Fox.serialCombined(dataSourcesByOrganizationName.keys.toList) { organizationName =>
+      for {
+        organization <- organizationDAO.findOneByName(organizationName)
+        _ <- dataSetDAO.deactivateUnreported(dataSourcesByOrganizationName(organizationName).map(_.id.name), organization._id, dataStoreName)
+      } yield ()
+    }
+  }
 
   def updateDataSources(dataStore: DataStore, dataSources: List[InboxDataSource])(implicit ctx: DBAccessContext) = {
     logger.info(s"[${dataStore.name}] Available datasets: " +

diff --git a/app/models/binary/DataStore.scala b/app/models/binary/DataStore.scala
@@ -18,6 +18,7 @@ case class DataStore(
                        name: String,
                        url: String,
                        key: String,
+                       isScratch: Boolean = false,
                        isDeleted: Boolean = false,
                        isForeign: Boolean = false
                        )
@@ -28,18 +29,21 @@ class DataStoreService @Inject()(dataStoreDAO: DataStoreDAO)(implicit ec: Execut
     Fox.successful(Json.obj(
       "name" -> dataStore.name,
       "url" -> dataStore.url,
-      "isForeign" -> dataStore.isForeign
+      "isForeign" -> dataStore.isForeign,
+      "isScratch" -> dataStore.isScratch
     ))
   }
 
   def validateAccess[A](name: String)(block: (DataStore) => Future[Result])
                        (implicit request: Request[A], m: MessagesProvider): Fox[Result] = {
-    request.getQueryString("key")
-      .toFox
-      .flatMap(key => dataStoreDAO.findOneByKey(key)(GlobalAccessContext)) // Check if key is valid
-      .flatMap(dataStore => block(dataStore)) // Run underlying action
-      .getOrElse(Forbidden(Messages("dataStore.notFound"))) // Default error
-    }
+    (for {
+      dataStore <- dataStoreDAO.findOneByName(name)(GlobalAccessContext)
+      key <- request.getQueryString("key").toFox
+      _ <- bool2Fox(key == dataStore.key)
+      result <- block(dataStore)
+    } yield result).getOrElse(Forbidden(Messages("dataStore.notFound")))
+  }
+
 }
 
 class DataStoreDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext) extends SQLDAO[DataStore, DatastoresRow, Datastores](sqlClient) {
@@ -53,6 +57,7 @@ class DataStoreDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext
       r.name,
       r.url,
       r.key,
+      r.isscratch,
       r.isdeleted,
       r.isforeign
     ))
@@ -82,8 +87,8 @@ class DataStoreDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext
 
   def insertOne(d: DataStore): Fox[Unit] = {
     for {
-      _ <- run(sqlu"""insert into webknossos.dataStores(name, url, key, isDeleted, isForeign)
-                         values(${d.name}, ${d.url}, ${d.key}, ${d.isDeleted}, ${d.isForeign})""")
+      _ <- run(sqlu"""insert into webknossos.dataStores(name, url, key, isScratch, isDeleted, isForeign)
+                             values(${d.name}, ${d.url}, ${d.key}, ${d.isScratch}, ${d.isDeleted}, ${d.isForeign})""")
     } yield ()
   }
 

diff --git a/conf/evolutions/032-scratch-datastores.sql b/conf/evolutions/032-scratch-datastores.sql
@@ -0,0 +1,12 @@
+-- https://github.com/scalableminds/webknossos/pull/TODO
+
+START TRANSACTION;
+
+DROP VIEW webknossos.dataStores_;
+ALTER TABLE webknossos.dataStores ADD COLUMN isScratch BOOLEAN NOT NULL DEFAULT false;
+
+CREATE VIEW webknossos.dataStores_ AS SELECT * FROM webknossos.dataStores WHERE NOT isDeleted;
+
+UPDATE webknossos.releaseInformation SET schemaVersion = 32;
+
+COMMIT TRANSACTION;
diff --git a/conf/evolutions/reversions/32-scratch-datastores.sql b/conf/evolutions/reversions/32-scratch-datastores.sql
@@ -0,0 +1,9 @@
+START TRANSACTION;
+
+DROP VIEW webknossos.dataStores_;
+ALTER TABLE webknossos.dataStores DROP COLUMN isScratch;
+CREATE VIEW webknossos.dataStores_ AS SELECT * FROM webknossos.dataStores WHERE NOT isDeleted;
+
+UPDATE webknossos.releaseInformation SET schemaVersion = 31;
+
+COMMIT TRANSACTION;
diff --git a/test/db/dataStores.csv b/test/db/dataStores.csv
@@ -1,2 +1,2 @@
-name,url,key,isDeleted,isForeign
-'localhost','http://localhost:9000','something-secure',f,f
+name,url,key,isScratch,isDeleted,isForeign
+'localhost','http://localhost:9000','something-secure',f,f,f
diff --git a/tools/postgres/schema.sql b/tools/postgres/schema.sql
@@ -21,7 +21,7 @@ START TRANSACTION;
 CREATE TABLE webknossos.releaseInformation (
   schemaVersion BIGINT NOT NULL
 );
-INSERT INTO webknossos.releaseInformation(schemaVersion) values(31);
+INSERT INTO webknossos.releaseInformation(schemaVersion) values(32);
 COMMIT TRANSACTION;
 
 CREATE TABLE webknossos.analytics(
@@ -115,6 +115,7 @@ CREATE TABLE webknossos.dataStores(
   name VARCHAR(256) PRIMARY KEY NOT NULL CHECK (name ~* '^[A-Za-z0-9\-_\.]+$'),
   url VARCHAR(512) UNIQUE NOT NULL CHECK (url ~* '^https?://[a-z0-9\.]+.*$'),
   key VARCHAR(1024) NOT NULL,
+  isScratch BOOLEAN NOT NULL DEFAULT false,
   isDeleted BOOLEAN NOT NULL DEFAULT false,
   isForeign BOOLEAN NOT NULL DEFAULT false
 );