Make Annotation Layer Names Unique in DB

MIGRATIONS.released.md

@@ -407,4 +407,4 @@ No migrations necessary.
 
 
 ## [18.07.0](https://github.com/scalableminds/webknossos/releases/tag/18.07.0) - 2018-07-05
-First release
+First release

MIGRATIONS.unreleased.md

@@ -8,4 +8,25 @@ User-facing changes are documented in the [changelog](CHANGELOG.released.md).
 ## Unreleased
 [Commits](https://github.com/scalableminds/webknossos/compare/22.07.0...HEAD)
 
+ - Postgres evolution 83 (see below) introduces unique and url-safe constraints for annotation layer names. If the database contains entries violating those new constraints, they need to be fixed manually, otherwise the evolution will abort:
+    - change null names to the front-end-side defaults:
+        ```
+        update webknossos.annotation_layers set name = 'Volume' where name is null and typ = 'Volume'
+        update webknossos.annotation_layers set name = 'Skeleton' where name is null and typ = 'Skeleton'
+        ```
+
+    - find annotations with multiple layers, make unique manually
+        ```
+        select _annotation, name from webknossos.annotation_layers where _annotation in (select s._annotation from
+        (select _annotation, count(_annotation) from webknossos.annotation_layers where typ = 'Volume' group by _annotation order by count(_annotation) desc limit 1000) as s
+        where count > 1) and typ = 'Volume' order by _annotation
+        ```
+
+   - find layers with interesting names, manually remove spaces and special characters
+        ```
+        select * from webknossos.annotation_layers where not name ~* '^[A-Za-z0-9\-_\.]+$'
+        ```
+
 ### Postgres Evolutions:
+
+- [083-unique-layer-names.sql](conf/evolutions/083-unique-layer-names.sql) Note: Note that this evolution introduces constraints which may not be met by existing data. See above for manual steps

app/controllers/AnnotationController.scala

@@ -5,35 +5,35 @@ import com.mohiva.play.silhouette.api.Silhouette
 import com.scalableminds.util.accesscontext.{DBAccessContext, GlobalAccessContext}
 import com.scalableminds.util.geometry.BoundingBox
 import com.scalableminds.util.tools.{Fox, FoxImplicits}
-import com.scalableminds.webknossos.tracingstore.tracings.{TracingIds, TracingType}
 import com.scalableminds.webknossos.tracingstore.tracings.volume.ResolutionRestrictions
-import io.swagger.annotations.{Api, ApiOperation, ApiParam, ApiResponse, ApiResponses}
+import com.scalableminds.webknossos.tracingstore.tracings.{TracingIds, TracingType}
+import io.swagger.annotations._
+import javax.inject.Inject
+import models.analytics.{AnalyticsService, CreateAnnotationEvent, OpenAnnotationEvent}
+import models.annotation.AnnotationLayerType.AnnotationLayerType
 import models.annotation.AnnotationState.Cancelled
 import models.annotation._
 import models.binary.{DataSetDAO, DataSetService}
+import models.organization.OrganizationDAO
 import models.project.ProjectDAO
 import models.task.TaskDAO
 import models.team.{TeamDAO, TeamService}
 import models.user.time._
 import models.user.{User, UserDAO, UserService}
+import oxalis.mail.{MailchimpClient, MailchimpTag}
 import oxalis.security.{URLSharing, WkEnv}
 import play.api.i18n.{Messages, MessagesProvider}
 import play.api.libs.json.{JsArray, _}
 import play.api.mvc.{Action, AnyContent, PlayBodyParsers}
 import utils.{ObjectId, WkConf}
-import javax.inject.Inject
-import models.analytics.{AnalyticsService, CreateAnnotationEvent, OpenAnnotationEvent}
-import models.annotation.AnnotationLayerType.AnnotationLayerType
-import models.organization.OrganizationDAO
-import oxalis.mail.{MailchimpClient, MailchimpTag}
 
 import scala.concurrent.ExecutionContext
 import scala.concurrent.duration._
 
 case class AnnotationLayerParameters(typ: AnnotationLayerType,
                                      fallbackLayerName: Option[String],
                                      resolutionRestrictions: Option[ResolutionRestrictions],
-                                     name: Option[String])
+                                     name: String)
 object AnnotationLayerParameters {
   implicit val jsonFormat: OFormat[AnnotationLayerParameters] = Json.format[AnnotationLayerParameters]
 }
@@ -262,7 +262,10 @@ class AnnotationController @Inject()(
           None,
           ObjectId.dummyId,
           ObjectId.dummyId,
-          List(AnnotationLayer(TracingIds.dummyTracingId, AnnotationLayerType.Skeleton))
+          List(
+            AnnotationLayer(TracingIds.dummyTracingId,
+                            AnnotationLayerType.Skeleton,
+                            AnnotationLayer.defaultSkeletonLayerName))
         )
         json <- annotationService.publicWrites(annotation, request.identity) ?~> "annotation.write.failed"
       } yield JsonOk(json)
@@ -384,7 +387,7 @@ class AnnotationController @Inject()(
         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").asOpt[String]
+        newLayerName = (request.body \ "name").as[String]
         _ <- annotationLayerDAO.updateName(annotation._id, tracingId, newLayerName) ?~> "annotation.edit.failed"
       } yield JsonOk(Messages("annotation.edit.success"))
     }

app/controllers/AnnotationIOController.scala

@@ -144,15 +144,17 @@ Expects:
     if (volumeLayersGrouped.length > 1 && volumeLayersGrouped.exists(_.length > 1))
       return Fox.failure("Cannot merge multiple annotations that each have multiple volume layers.")
     if (volumeLayersGrouped.length == 1) { // Just one annotation was uploaded, keep its layers separate
-      Fox.serialCombined(volumeLayersGrouped.toList.flatten) { uploadedVolumeLayer =>
+      Fox.serialCombined(volumeLayersGrouped.toList.flatten.zipWithIndex) { volumeLayerWithIndex =>
+        val uploadedVolumeLayer = volumeLayerWithIndex._1
+        val idx = volumeLayerWithIndex._2
         for {
           savedTracingId <- client.saveVolumeTracing(uploadedVolumeLayer.tracing,
                                                      uploadedVolumeLayer.getDataZipFrom(otherFiles))
         } yield
           AnnotationLayer(
             savedTracingId,
             AnnotationLayerType.Volume,
-            uploadedVolumeLayer.name
+            uploadedVolumeLayer.name.getOrElse(AnnotationLayer.defaultVolumeLayerName + idx.toString)
           )
       }
     } else { // Multiple annotations with volume layers (but at most one each) was uploaded merge those volume layers into one
@@ -168,7 +170,7 @@ Expects:
           AnnotationLayer(
             mergedTracingId,
             AnnotationLayerType.Volume,
-            None
+            AnnotationLayer.defaultVolumeLayerName
           ))
     }
   }
@@ -180,7 +182,8 @@ Expects:
       mergedTracingId <- tracingStoreClient.mergeSkeletonTracingsByContents(
         SkeletonTracings(skeletonTracings.map(t => SkeletonTracingOpt(Some(t)))),
         persistTracing = true)
-    } yield List(AnnotationLayer(mergedTracingId, AnnotationLayerType.Skeleton, None))
+    } yield
+      List(AnnotationLayer(mergedTracingId, AnnotationLayerType.Skeleton, AnnotationLayer.defaultSkeletonLayerName))
   }
 
   private def assertNonEmpty(parseSuccesses: List[NmlParseSuccess]) =

app/controllers/LegacyApiController.scala

@@ -429,15 +429,15 @@ class LegacyApiController @Inject()(annotationController: AnnotationController,
           AnnotationLayerParameters(AnnotationLayerType.Skeleton,
                                     request.body.fallbackLayerName,
                                     request.body.resolutionRestrictions,
-                                    name = None))
+                                    name = AnnotationLayer.defaultSkeletonLayerName))
     val volumeParameters =
       if (request.body.typ == "skeleton") None
       else
         Some(
           AnnotationLayerParameters(AnnotationLayerType.Volume,
                                     request.body.fallbackLayerName,
                                     request.body.resolutionRestrictions,
-                                    name = None))
+                                    name = AnnotationLayer.defaultVolumeLayerName))
     List(skeletonParameters, volumeParameters).flatten
   }
 

app/models/annotation/Annotation.scala

@@ -110,7 +110,7 @@ class AnnotationLayerDAO @Inject()(SQLClient: SQLClient)(implicit ec: ExecutionC
 
   def insertOneQuery(annotationId: ObjectId, a: AnnotationLayer): SqlAction[Int, NoStream, Effect] =
     sqlu"""insert into webknossos.annotation_layers(_annotation, tracingId, typ, name)
-            values($annotationId, ${a.tracingId}, '#${a.typ.toString}', ${a.name.map(sanitize)})"""
+            values($annotationId, ${a.tracingId}, '#${a.typ.toString}', ${a.name})"""
 
   def findAnnotationIdByTracingId(tracingId: String): Fox[ObjectId] =
     for {
@@ -125,7 +125,7 @@ class AnnotationLayerDAO @Inject()(SQLClient: SQLClient)(implicit ec: ExecutionC
         sqlu"update webknossos.annotation_layers set tracingId = $newTracingId where _annotation = $annotationId and tracingId = $oldTracingId")
     } yield ()
 
-  def updateName(annotationId: ObjectId, tracingId: String, newName: Option[String]): Fox[Unit] =
+  def updateName(annotationId: ObjectId, tracingId: String, newName: String): Fox[Unit] =
     for {
       _ <- run(
         sqlu"update webknossos.annotation_layers set name = $newName where _annotation = $annotationId and tracingId = $tracingId")

app/models/annotation/AnnotationLayer.scala

@@ -13,34 +13,44 @@ import scala.concurrent.ExecutionContext
 case class AnnotationLayer(
     tracingId: String,
     typ: AnnotationLayerType,
-    name: Option[String] = None
+    name: String
 ) {}
 
 object AnnotationLayer extends FoxImplicits {
   implicit val jsonFormat: OFormat[AnnotationLayer] = Json.format[AnnotationLayer]
 
+  val defaultSkeletonLayerName: String = "Skeleton"
+  val defaultVolumeLayerName: String = "Volume"
+
+  def defaultNameForType(typ: AnnotationLayerType): String =
+    typ match {
+      case AnnotationLayerType.Skeleton => defaultSkeletonLayerName
+      case AnnotationLayerType.Volume   => defaultVolumeLayerName
+    }
+
   def layersFromIds(skeletonTracingIdOpt: Option[String],
                     volumeTracingIdOpt: Option[String],
                     assertNonEmpty: Boolean = true)(implicit ec: ExecutionContext): Fox[List[AnnotationLayer]] = {
     val annotationLayers: List[AnnotationLayer] = List(
-      skeletonTracingIdOpt.map(AnnotationLayer(_, AnnotationLayerType.Skeleton)),
-      volumeTracingIdOpt.map(AnnotationLayer(_, AnnotationLayerType.Volume))).flatten
+      skeletonTracingIdOpt.map(AnnotationLayer(_, AnnotationLayerType.Skeleton, defaultSkeletonLayerName)),
+      volumeTracingIdOpt.map(AnnotationLayer(_, AnnotationLayerType.Volume, defaultVolumeLayerName))
+    ).flatten
     for {
       _ <- bool2Fox(!assertNonEmpty || annotationLayers.nonEmpty) ?~> "annotation.needsEitherSkeletonOrVolume"
     } yield annotationLayers
   }
 }
 
 case class FetchedAnnotationLayer(tracingId: String,
-                                  name: Option[String],
+                                  name: String,
                                   tracing: Either[SkeletonTracing, VolumeTracing],
                                   volumeDataOpt: Option[Array[Byte]] = None) {
   def typ: AnnotationLayerType =
     if (tracing.isLeft) AnnotationLayerType.Skeleton else AnnotationLayerType.Volume
 
   def volumeDataZipName(index: Int, isSingle: Boolean): String = {
     val indexLabel = if (isSingle) "" else s"_$index"
-    val nameLabel = name.map(n => s"_$n").getOrElse("")
+    val nameLabel = s"_$name"
     s"data$indexLabel$nameLabel.zip"
   }
 }
@@ -74,8 +84,10 @@ object FetchedAnnotationLayer {
       _ <- bool2Fox(skeletonTracingIdOpt.isDefined == skeletonTracingOpt.isDefined) ?~> "annotation.mismatchingSkeletonIdsAndTracings"
       _ <- bool2Fox(volumeTracingIdOpt.isDefined == volumeTracingOpt.isDefined) ?~> "annotation.mismatchingVolumeIdsAndTracings"
       annotationLayers: List[FetchedAnnotationLayer] = List(
-        skeletonTracingIdOpt.map(FetchedAnnotationLayer(_, None, Left(skeletonTracingOpt.get))),
-        volumeTracingIdOpt.map(FetchedAnnotationLayer(_, None, Right(volumeTracingOpt.get)))
+        skeletonTracingIdOpt.map(
+          FetchedAnnotationLayer(_, AnnotationLayer.defaultSkeletonLayerName, Left(skeletonTracingOpt.get))),
+        volumeTracingIdOpt.map(
+          FetchedAnnotationLayer(_, AnnotationLayer.defaultVolumeLayerName, Right(volumeTracingOpt.get)))
       ).flatten
       _ <- bool2Fox(!assertNonEmpty || annotationLayers.nonEmpty) ?~> "annotation.needsEitherSkeletonOrVolume"
     } yield annotationLayers

app/models/annotation/AnnotationMerger.scala

@@ -70,12 +70,18 @@ class AnnotationMerger @Inject()(dataSetDAO: DataSetDAO, tracingStoreService: Tr
                                                        skeletonLayers.map(_.tracingId),
                                                        persistTracing)
       mergedVolumeTracingId <- mergeVolumeTracings(tracingStoreClient, volumeLayers.map(_.tracingId), persistTracing)
-      mergedSkeletonName = allEqual(skeletonLayers.flatMap(_.name))
-      mergedVolumeName = allEqual(volumeLayers.flatMap(_.name))
-      mergedSkeletonLayer = mergedSkeletonTracingId.map(id =>
-        AnnotationLayer(id, AnnotationLayerType.Skeleton, mergedSkeletonName))
-      mergedVolumeLayer = mergedVolumeTracingId.map(id =>
-        AnnotationLayer(id, AnnotationLayerType.Volume, mergedVolumeName))
+      mergedSkeletonName = allEqual(skeletonLayers.map(_.name))
+      mergedVolumeName = allEqual(volumeLayers.map(_.name))
+      mergedSkeletonLayer = mergedSkeletonTracingId.map(
+        id =>
+          AnnotationLayer(id,
+                          AnnotationLayerType.Skeleton,
+                          mergedSkeletonName.getOrElse(AnnotationLayer.defaultSkeletonLayerName)))
+      mergedVolumeLayer = mergedVolumeTracingId.map(
+        id =>
+          AnnotationLayer(id,
+                          AnnotationLayerType.Volume,
+                          mergedVolumeName.getOrElse(AnnotationLayer.defaultVolumeLayerName)))
     } yield List(mergedSkeletonLayer, mergedVolumeLayer).flatten
 
   private def allEqual(str: List[String]): Option[String] =

app/models/annotation/AnnotationService.scala

@@ -329,10 +329,11 @@ class AnnotationService @Inject()(
         case _                    => Fox.failure("annotation.makeHybrid.alreadyHybrid")
       }
       usedFallbackLayerName = if (newAnnotationLayerType == AnnotationLayerType.Volume) fallbackLayerName else None
-      newAnnotationLayerParameters = AnnotationLayerParameters(newAnnotationLayerType,
-                                                               usedFallbackLayerName,
-                                                               Some(ResolutionRestrictions.empty),
-                                                               None)
+      newAnnotationLayerParameters = AnnotationLayerParameters(
+        newAnnotationLayerType,
+        usedFallbackLayerName,
+        Some(ResolutionRestrictions.empty),
+        AnnotationLayer.defaultNameForType(newAnnotationLayerType))
       _ <- addAnnotationLayer(annotation, organizationName, newAnnotationLayerParameters) ?~> "makeHybrid.createTracings.failed"
     } yield ()
 

app/models/annotation/nml/NmlWriter.scala

@@ -200,14 +200,13 @@ class NmlWriter @Inject()(implicit ec: ExecutionContext) extends FoxImplicits {
                         volumeFilename: Option[String],
                         skipVolumeData: Boolean)(implicit writer: XMLStreamWriter): Unit =
     if (skipVolumeData) {
-      val nameLabel = volumeLayer.name.map(n => f"named $n ").getOrElse("")
       writer.writeComment(
-        f"A volume layer $nameLabel(id = $index) was omitted here while downloading this annotation without volume data.")
+        f"A volume layer named ${volumeLayer.name} (id = $index) was omitted here while downloading this annotation without volume data.")
     } else {
       Xml.withinElementSync("volume") {
         writer.writeAttribute("id", index.toString)
         writer.writeAttribute("location", volumeFilename.getOrElse(volumeLayer.volumeDataZipName(index, isSingle)))
-        volumeLayer.name.foreach(n => writer.writeAttribute("name", n))
+        writer.writeAttribute("name", volumeLayer.name)
         volumeLayer.tracing match {
           case Right(volumeTracing) => volumeTracing.fallbackLayer.foreach(writer.writeAttribute("fallbackLayer", _))
           case _                    => ()

conf/evolutions/083-unique-layer-names.sql

@@ -0,0 +1,16 @@
+-- Note that this evolution introduces constraints which may not be met by existing data. See migration guide for manual steps
+
+START TRANSACTION;
+
+ALTER TABLE webknossos.annotation_layers
+ALTER COLUMN name SET NOT NULL;
+
+ALTER TABLE webknossos.annotation_layers
+ADD CONSTRAINT annotation_layers_name__annotation_key UNIQUE(name, _annotation);
+
+ALTER TABLE webknossos.annotation_layers
+ADD CONSTRAINT annotation_layers_name_check CHECK (name ~* '^[A-Za-z0-9\-_\.]+$');
+
+UPDATE webknossos.releaseInformation SET schemaVersion = 83;
+
+COMMIT TRANSACTION;

conf/evolutions/reversions/083-unique-layer-names.sql

@@ -0,0 +1,14 @@
+START TRANSACTION;
+
+ALTER TABLE webknossos.annotation_layers
+ALTER COLUMN name DROP NOT NULL;
+
+ALTER TABLE webknossos.annotation_layers
+DROP CONSTRAINT annotation_layers_name__annotation_key;
+
+ALTER TABLE webknossos.annotation_layers
+DROP CONSTRAINT annotation_layers_name_check;
+
+UPDATE webknossos.releaseInformation SET schemaVersion = 82;
+
+COMMIT TRANSACTION;

frontend/javascripts/messages.ts

@@ -173,7 +173,9 @@ instead. Only enable this option if you understand its effect. All layers will n
   "tracing.volume_layer_name_includes_invalid_characters": (disallowedCharacters: string) =>
     `This layer name includes the disallowed character${
       disallowedCharacters.length > 1 ? "s" : ""
-    } "${disallowedCharacters}". Please delete ${disallowedCharacters.length > 1 ? "them" : "it"}.`,
+    } "${disallowedCharacters}". Please remove ${
+      disallowedCharacters.length > 1 ? "them" : "it"
+    } to set the layer name.`,
   "tracing.delete_initial_node": "Do you really want to delete the initial node?",
   "tracing.delete_tree": "Do you really want to delete the whole tree?",
   "tracing.delete_tree_with_initial_node":

frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts

@@ -102,7 +102,7 @@ export function getAllReadableLayerNames(dataset: APIDataset, tracing: Tracing)
       : currentLayer.name,
   );
   if (tracing.skeleton != null) {
-    allReadableLayerNames.push("Skeletons");
+    allReadableLayerNames.push("Skeleton");
   }
   return allReadableLayerNames;
 }

frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx

@@ -781,7 +781,7 @@ class DatasetSettings extends React.PureComponent<DatasetSettingsProps, State> {
     return (
       <React.Fragment>
         <Tooltip
-          title={showSkeletons ? "Hide all Skeletons" : "Show all skeletons"}
+          title={showSkeletons ? "Hide skeleton layer" : "Show skeleton layer"}
           placement="top"
         >
           {/* This div is necessary for the tooltip to be displayed */}
@@ -804,7 +804,7 @@ class DatasetSettings extends React.PureComponent<DatasetSettingsProps, State> {
             wordWrap: "break-word",
           }}
         >
-          Skeletons
+          Skeleton
         </span>
         {showSkeletons ? (
           <div

frontend/javascripts/oxalis/view/left-border-tabs/modals/add_volume_layer_modal.tsx

@@ -36,9 +36,9 @@ export function checkForLayerNameDuplication(
 }
 
 export function checkLayerNameForInvalidCharacters(readableLayerName: string): ValidationResult {
-  const uriSaveCharactersRegex = /[0-9a-zA-Z-._~]+/g;
-  // Removing all URISaveCharacters from readableLayerName. The left over chars are all invalid.
-  const allInvalidChars = readableLayerName.replace(uriSaveCharactersRegex, "");
+  const uriSafeCharactersRegex = /[0-9a-zA-Z-._]+/g;
+  // Removing all URISaveCharacters from readableLayerName. The leftover chars are all invalid.
+  const allInvalidChars = readableLayerName.replace(uriSafeCharactersRegex, "");
   const allUniqueInvalidCharsAsSet = new Set(allInvalidChars);
   const allUniqueInvalidCharsAsString = "".concat(...allUniqueInvalidCharsAsSet.values());
   const isValid = allUniqueInvalidCharsAsString.length === 0;