Make annotation layer name check deferrable, fixing false positives (#8896)

fm3 · web-flow · commit 13f3f2ac24dd · 2025-09-15T12:57:05.000+02:00
- removes unused route editAnnotationLayer - change to transactionally update annotation layers in postgres, after it was reported by tracingstore side - make the name unique check deferrable, so it is only checked on the transaction commit. - This fixes the following error scenario: The user renames layer 1 from A to B and layer 2 from C to A. This is legitimate, as no duplicate names exist if the order is correct. However, since the postgres updates are not guaranteed to come in the same order, this could previously lead to false positive duplicate checks. - Note that running this evolution on large annotation_layers tables may be slow, as the constraint is completely rebuilt. ### Steps to test: - Create annotation with multiple volume annotation layers - rename multiple ones of them such that the name that was previously taken by one layer is then immediately taken by another. Should not fail. Save and reload should work and show the correct names. ### Issues: - fixes #8647 ------ - [x] Added changelog entry (create a `$PR_NUMBER.md` file in `unreleased_changes` or use `./tools/create-changelog-entry.py`) - [x] Considered [common edge cases](../blob/master/.github/common_edge_cases.md)
diff --git a/app/controllers/AnnotationController.scala b/app/controllers/AnnotationController.scala
@@ -276,17 +276,6 @@ class AnnotationController @Inject()(
       } yield JsonOk(Messages("annotation.edit.success"))
   }
 
-  def editAnnotationLayer(typ: String, id: ObjectId, tracingId: String): Action[JsValue] =
-    sil.SecuredAction.async(parse.json) { implicit request =>
-      for {
-        annotation <- provider.provideAnnotation(typ, id, request.identity) ~> NOT_FOUND
-        restrictions <- provider.restrictionsFor(typ, id) ?~> "restrictions.notFound" ~> NOT_FOUND
-        _ <- restrictions.allowUpdate(request.identity) ?~> "notAllowed" ~> FORBIDDEN
-        newLayerName = (request.body \ "name").as[String]
-        _ <- annotationLayerDAO.updateName(annotation._id, tracingId, newLayerName) ?~> "annotation.edit.failed"
-      } yield JsonOk(Messages("annotation.edit.success"))
-    }
-
   def annotationsForTask(taskId: ObjectId): Action[AnyContent] =
     sil.SecuredAction.async { implicit request =>
       for {
diff --git a/app/controllers/WKRemoteTracingStoreController.scala b/app/controllers/WKRemoteTracingStoreController.scala
@@ -61,11 +61,11 @@ class WKRemoteTracingStoreController @Inject()(tracingStoreService: TracingStore
           layerIdsToUpdate = existingLayerIds.intersect(newLayerIds)
           layerIdsToInsert = newLayerIds.diff(existingLayerIds)
           _ <- Fox.serialCombined(layerIdsToDelete.toList)(annotationLayerDAO.deleteOneByTracingId(annotationId, _))
-          _ <- Fox.serialCombined(newLayersProto.filter(l => layerIdsToInsert.contains(l.tracingId))) { layerProto =>
-            annotationLayerDAO.insertOne(annotationId, AnnotationLayer.fromProto(layerProto))
-          }
-          _ <- Fox.serialCombined(newLayersProto.filter(l => layerIdsToUpdate.contains(l.tracingId)))(l =>
-            annotationLayerDAO.updateName(annotationId, l.tracingId, l.name))
+          _ <- annotationLayerDAO.updateLayers(
+            annotationId,
+            newLayersProto.filter(l => layerIdsToInsert.contains(l.tracingId)).map(AnnotationLayer.fromProto),
+            newLayersProto.filter(l => layerIdsToUpdate.contains(l.tracingId)).map(AnnotationLayer.fromProto)
+          )
           // Layer stats are ignored here, they are sent eagerly when saving updates
           _ <- annotationDAO.updateDescription(annotationId, request.body.description)
         } yield Ok
diff --git a/app/models/annotation/Annotation.scala b/app/models/annotation/Annotation.scala
@@ -16,6 +16,7 @@ import slick.jdbc.TransactionIsolation.Serializable
 import slick.lifted.Rep
 import slick.sql.SqlAction
 import com.scalableminds.util.objectid.ObjectId
+import slick.dbio.DBIO
 import utils.sql.{SQLDAO, SimpleSQLDAO, SqlClient, SqlToken}
 
 import javax.inject.Inject
@@ -131,8 +132,7 @@ class AnnotationLayerDAO @Inject()(SQLClient: SqlClient)(implicit ec: ExecutionC
       _ <- run(insertOneQuery(annotationId, annotationLayer))
     } yield ()
 
-  def insertLayerQueries(annotationId: ObjectId,
-                         layers: List[AnnotationLayer]): List[SqlAction[Int, NoStream, Effect]] =
+  def insertLayerQueries(annotationId: ObjectId, layers: Seq[AnnotationLayer]): Seq[SqlAction[Int, NoStream, Effect]] =
     layers.map { annotationLayer =>
       insertOneQuery(annotationId, annotationLayer)
     }
@@ -161,13 +161,20 @@ class AnnotationLayerDAO @Inject()(SQLClient: SqlClient)(implicit ec: ExecutionC
       head <- rList.headOption.toFox
     } yield head
 
-  def updateName(annotationId: ObjectId, tracingId: String, newName: String): Fox[Unit] =
+  def updateLayers(annotationId: ObjectId,
+                   newLayers: Seq[AnnotationLayer],
+                   updatedLayers: Seq[AnnotationLayer]): Fox[Unit] = {
+    val insertQueries = insertLayerQueries(annotationId, newLayers)
+    val updateQueries = updatedLayers.map { updatedLayer =>
+      q"""UPDATE webknossos.annotation_layers
+                         SET name = ${updatedLayer.name}
+                         WHERE _annotation = $annotationId
+                         AND tracingId = ${updatedLayer.tracingId}""".asUpdate
+    }
     for {
-      _ <- run(q"""UPDATE webknossos.annotation_layers
-                   SET name = $newName
-                   WHERE _annotation = $annotationId
-                   AND tracingId = $tracingId""".asUpdate)
+      _ <- run(DBIO.sequence(insertQueries ++ updateQueries).transactionally)
     } yield ()
+  }
 
   def deleteAllForAnnotationQuery(annotationId: ObjectId): SqlAction[Int, NoStream, Effect] =
     q"DELETE FROM webknossos.annotation_layers WHERE _annotation = $annotationId".asUpdate
diff --git a/conf/evolutions/140-annotation-layer-name-check-deferrable.sql b/conf/evolutions/140-annotation-layer-name-check-deferrable.sql
@@ -0,0 +1,10 @@
+START TRANSACTION;
+
+do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 139, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql;
+
+ALTER TABLE webknossos.annotation_layers DROP CONSTRAINT annotation_layers_name__annotation_key;
+ALTER TABLE webknossos.annotation_layers ADD CONSTRAINT annotation_layers_name__annotation_key UNIQUE (name, _annotation) DEFERRABLE INITIALLY DEFERRED;
+
+UPDATE webknossos.releaseInformation SET schemaVersion = 140;
+
+COMMIT TRANSACTION;
diff --git a/conf/evolutions/reversions/140-annotation-layer-name-check-deferrable.sql b/conf/evolutions/reversions/140-annotation-layer-name-check-deferrable.sql
@@ -0,0 +1,10 @@
+START TRANSACTION;
+
+do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 140, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql;
+
+ALTER TABLE webknossos.annotation_layers DROP CONSTRAINT annotation_layers_name__annotation_key;
+ALTER TABLE webknossos.annotation_layers ADD CONSTRAINT annotation_layers_name__annotation_key UNIQUE (name, _annotation);
+
+UPDATE webknossos.releaseInformation SET schemaVersion = 139;
+
+COMMIT TRANSACTION;
diff --git a/conf/webknossos.latest.routes b/conf/webknossos.latest.routes
@@ -156,7 +156,6 @@ POST          /userToken/generate
 POST          /annotations/upload                                                               controllers.AnnotationIOController.upload()
 POST          /annotations/:typ/:id/duplicate                                                   controllers.AnnotationController.duplicate(typ: String, id: ObjectId)
 PATCH         /annotations/:typ/:id/edit                                                        controllers.AnnotationController.editAnnotation(typ: String, id: ObjectId)
-PATCH         /annotations/:typ/:id/editLayer/:tracingId                                        controllers.AnnotationController.editAnnotationLayer(typ: String, id: ObjectId, tracingId: String)
 
 PATCH         /annotations/:typ/:id/finish                                                      controllers.AnnotationController.finish(typ: String, id: ObjectId, timestamp: Long)
 PATCH         /annotations/:typ/finish                                                          controllers.AnnotationController.finishAll(typ: String, timestamp: Long)
diff --git a/tools/postgres/schema.sql b/tools/postgres/schema.sql
@@ -21,7 +21,7 @@ CREATE TABLE webknossos.releaseInformation (
   schemaVersion BIGINT NOT NULL
 );
 
-INSERT INTO webknossos.releaseInformation(schemaVersion) values(139);
+INSERT INTO webknossos.releaseInformation(schemaVersion) values(140);
 COMMIT TRANSACTION;
 
 
@@ -59,7 +59,7 @@ CREATE TABLE webknossos.annotation_layers(
   typ webknossos.ANNOTATION_LAYER_TYPE NOT NULL,
   name TEXT NOT NULL CHECK (name ~* '^[A-Za-z0-9\-_\.\$]+$'),
   statistics JSONB NOT NULL,
-  UNIQUE (name, _annotation),
+  UNIQUE (name, _annotation) DEFERRABLE INITIALLY DEFERRED,
   PRIMARY KEY (_annotation, tracingId),
   CONSTRAINT statisticsIsJsonObject CHECK(jsonb_typeof(statistics) = 'object')
 );
diff --git a/unreleased_changes/8896.md b/unreleased_changes/8896.md
@@ -0,0 +1,5 @@
+### Fixed
+- Fixed a bug where annotation updates would fail if multiple annotation layers are renamed such that a previously-taken name is then used by another layer.
+
+### Postgres Evolutions
+- [140-annotation-layer-name-check-deferrable.sql](conf/evolutions/140-annotation-layer-name-check-deferrable.sql)
