Use ObjectId in TracingStore where applicable (#8674)

Using our ObjectId class + validation in the tracingstore for
annotationId and userId fields, to match the data model on the wk side.

For some reason putting the sbt setting that enables using ObjectId in
routes files in commonSettings would not work, I had to repeat it for
all three projects 🤷

### Steps to test:
- Trace some, should still work
- Annotation user state should still work
- Re-test availableBySwitching workflow for annotations (not sure why
there was String there previously instead of ObjectId despite this
living on the wk side)

------
- [x] Removed dev-only changes like prints and application.conf edits
- [x] Considered [common edge
cases](../blob/master/.github/common_edge_cases.md)
- [x] Needs datastore update after deployment

Author: fm3

app/controllers/AuthenticationController.scala

@@ -215,7 +215,7 @@ class AuthenticationController @Inject()(
     } yield result
 
   def accessibleBySwitching(datasetId: Option[ObjectId],
-                            annotationId: Option[String],
+                            annotationId: Option[ObjectId],
                             workflowHash: Option[String]): Action[AnyContent] = sil.SecuredAction.async {
     implicit request =>
       for {

app/controllers/WKRemoteTracingStoreController.scala

@@ -82,8 +82,7 @@ class WKRemoteTracingStoreController @Inject()(tracingStoreService: TracingStore
       tracingStoreService.validateAccess(name, key) { _ =>
         val report = request.body
         for {
-          annotationId <- ObjectId.fromString(report.annotationId)
-          annotation <- annotationDAO.findOne(annotationId)
+          annotation <- annotationDAO.findOne(report.annotationId)
           _ <- ensureAnnotationNotFinished(annotation)
           _ <- annotationDAO.updateModified(annotation._id, Instant.now)
           _ = report.statistics.map(statistics => annotationService.updateStatistics(annotation._id, statistics))

app/models/annotation/AnnotationLayerPrecedence.scala

@@ -29,7 +29,7 @@ case class RedundantTracingProperties(
     zoomLevel: Double,
     userBoundingBoxes: Seq[NamedBoundingBoxProto],
     editPositionAdditionalCoordinates: Seq[AdditionalCoordinateProto],
-    userStateBoundingBoxVisibilities: Map[String, Seq[Id32WithBool]] // UserId → Seq(bboxId, bboxIsVisible)
+    userStateBoundingBoxVisibilities: Map[ObjectId, Seq[Id32WithBool]] // UserId → Seq(bboxId, bboxIsVisible)
 )
 
 trait AnnotationLayerPrecedence extends FoxImplicits {
@@ -79,7 +79,7 @@ trait AnnotationLayerPrecedence extends FoxImplicits {
       userStates: Seq[SkeletonUserStateProto],
       oldPrecedenceLayerProperties: RedundantTracingProperties): Seq[SkeletonUserStateProto] = {
     val adaptedExistingUserStates = userStates.map { userState =>
-      val userId = userState.userId
+      val userId = ObjectId(userState.userId)
       oldPrecedenceLayerProperties.userStateBoundingBoxVisibilities.get(userId) match {
         case None => userState
         case Some(precedenceBboxVisibilities) =>
@@ -88,9 +88,9 @@ trait AnnotationLayerPrecedence extends FoxImplicits {
     }
     // We also have to create new user states for the users the old precedence layer has, but the new precedence layer is missing.
     val newUserPrecedenceProperties = oldPrecedenceLayerProperties.userStateBoundingBoxVisibilities.filter(tuple =>
-      !userStates.exists(_.userId == tuple._1))
+      !userStates.exists(_.userId == tuple._1.toString))
     val newUserStates = newUserPrecedenceProperties.map {
-      case (userId: String, boundingBoxVisibilities: Seq[Id32WithBool]) =>
+      case (userId: ObjectId, boundingBoxVisibilities: Seq[Id32WithBool]) =>
         SkeletonTracingDefaults
           .emptyUserState(userId)
           .copy(
@@ -104,7 +104,7 @@ trait AnnotationLayerPrecedence extends FoxImplicits {
       userStates: Seq[VolumeUserStateProto],
       oldPrecedenceLayerProperties: RedundantTracingProperties): Seq[VolumeUserStateProto] = {
     val adaptedExistingUserStates = userStates.map { userState =>
-      val userId = userState.userId
+      val userId = ObjectId(userState.userId)
       oldPrecedenceLayerProperties.userStateBoundingBoxVisibilities.get(userId) match {
         case None => userState
         case Some(precedenceBboxVisibilities) =>
@@ -113,9 +113,9 @@ trait AnnotationLayerPrecedence extends FoxImplicits {
     }
     // We also have to create new user states for the users the old precedence layer has, but the new precedence layer is missing.
     val newUserPrecedenceProperties = oldPrecedenceLayerProperties.userStateBoundingBoxVisibilities.filter(tuple =>
-      !userStates.exists(_.userId == tuple._1))
+      !userStates.exists(_.userId == tuple._1.toString))
     val newUserStates = newUserPrecedenceProperties.map {
-      case (userId: String, boundingBoxVisibilities: Seq[Id32WithBool]) =>
+      case (userId: ObjectId, boundingBoxVisibilities: Seq[Id32WithBool]) =>
         VolumeTracingDefaults
           .emptyUserState(userId)
           .copy(
@@ -194,7 +194,7 @@ trait AnnotationLayerPrecedence extends FoxImplicits {
           s.userBoundingBoxes ++ s.userBoundingBox.map(
             com.scalableminds.webknossos.datastore.geometry.NamedBoundingBoxProto(0, None, None, None, _)),
           s.editPositionAdditionalCoordinates,
-          s.userStates.map(userState => (userState.userId, userState.boundingBoxVisibilities)).toMap
+          s.userStates.map(userState => (ObjectId(userState.userId), userState.boundingBoxVisibilities)).toMap
         )
       case Right(v) =>
         RedundantTracingProperties(
@@ -204,7 +204,7 @@ trait AnnotationLayerPrecedence extends FoxImplicits {
           v.userBoundingBoxes ++ v.userBoundingBox.map(
             com.scalableminds.webknossos.datastore.geometry.NamedBoundingBoxProto(0, None, None, None, _)),
           v.editPositionAdditionalCoordinates,
-          v.userStates.map(userState => (userState.userId, userState.boundingBoxVisibilities)).toMap
+          v.userStates.map(userState => (ObjectId(userState.userId), userState.boundingBoxVisibilities)).toMap
         )
     }
 }

app/models/annotation/AnnotationMerger.scala

@@ -74,7 +74,7 @@ class AnnotationMerger @Inject()(datasetDAO: DatasetDAO, tracingStoreService: Tr
     for {
       dataset <- datasetDAO.findOne(datasetId)
       tracingStoreClient: WKRemoteTracingStoreClient <- tracingStoreService.clientFor(dataset)
-      mergedAnnotationProto <- tracingStoreClient.mergeAnnotationsByIds(annotations.map(_.id),
+      mergedAnnotationProto <- tracingStoreClient.mergeAnnotationsByIds(annotations.map(_._id),
                                                                         annotations.map(_._user),
                                                                         newAnnotationId,
                                                                         toTemporaryStore,

app/models/annotation/AnnotationService.scala

@@ -801,7 +801,7 @@ class AnnotationService @Inject()(
       dataStore <- dataStoreDAO.findOneByName(dataset._dataStore.trim) ?~> "datastore.notFound"
       tracingStore <- tracingStoreDAO.findFirst
       annotationSource = AnnotationSource(
-        id = annotation.id,
+        id = annotation._id,
         annotationLayers = annotation.annotationLayers,
         datasetDirectoryName = dataset.directoryName,
         organizationId = organization._id,

app/models/annotation/WKRemoteTracingStoreClient.scala

@@ -172,7 +172,7 @@ class WKRemoteTracingStoreClient(
       .postEmpty()
   }
 
-  def mergeAnnotationsByIds(annotationIds: List[String],
+  def mergeAnnotationsByIds(annotationIds: List[ObjectId],
                             ownerIds: List[ObjectId],
                             newAnnotationId: ObjectId,
                             toTemporaryStore: Boolean,
@@ -183,8 +183,8 @@ class WKRemoteTracingStoreClient(
       .addQueryString("toTemporaryStore" -> toTemporaryStore.toString)
       .addQueryString("newAnnotationId" -> newAnnotationId.toString)
       .addQueryString("requestingUserId" -> requestingUserId.toString)
-      .postJsonWithProtoResponse[MergedFromIdsRequest, AnnotationProto](
-        MergedFromIdsRequest(annotationIds, ownerIds.map(_.toString)))(AnnotationProto)
+      .postJsonWithProtoResponse[MergedFromIdsRequest, AnnotationProto](MergedFromIdsRequest(annotationIds, ownerIds))(
+        AnnotationProto)
   }
 
   def mergeSkeletonTracingsByContents(newTracingId: String, tracings: SkeletonTracings): Fox[Unit] = {

app/security/AccessibleBySwitchingService.scala

@@ -30,7 +30,7 @@ class AccessibleBySwitchingService @Inject()(
 
   def getOrganizationToSwitchTo(user: User,
                                 datasetId: Option[ObjectId],
-                                annotationId: Option[String],
+                                annotationId: Option[ObjectId],
                                 workflowHash: Option[String])(implicit ctx: DBAccessContext): Fox[Organization] =
     for {
       isSuperUser <- multiUserDAO.findOne(user._multiUser).map(_.isSuperUser)
@@ -42,7 +42,7 @@ class AccessibleBySwitchingService @Inject()(
     } yield selectedOrganization
 
   private def accessibleBySwitchingForSuperUser(datasetIdOpt: Option[ObjectId],
-                                                annotationIdOpt: Option[String],
+                                                annotationIdOpt: Option[ObjectId],
                                                 workflowHashOpt: Option[String]): Fox[Organization] = {
     implicit val ctx: DBAccessContext = GlobalAccessContext
     (datasetIdOpt, annotationIdOpt, workflowHashOpt) match {
@@ -53,8 +53,7 @@ class AccessibleBySwitchingService @Inject()(
         } yield organization
       case (None, Some(annotationId), None) =>
         for {
-          annotationObjectId <- ObjectId.fromString(annotationId)
-          annotation <- annotationDAO.findOne(annotationObjectId) // Note: this does not work for compound annotations.
+          annotation <- annotationDAO.findOne(annotationId) // Note: this does not work for compound annotations.
           user <- userDAO.findOne(annotation._user)
           organization <- organizationDAO.findOne(user._organization)
         } yield organization
@@ -69,7 +68,7 @@ class AccessibleBySwitchingService @Inject()(
 
   private def accessibleBySwitchingForMultiUser(multiUserId: ObjectId,
                                                 datasetIdOpt: Option[ObjectId],
-                                                annotationIdOpt: Option[String],
+                                                annotationIdOpt: Option[ObjectId],
                                                 workflowHashOpt: Option[String]): Fox[Organization] =
     for {
       identities <- userDAO.findAllByMultiUser(multiUserId)
@@ -80,7 +79,7 @@ class AccessibleBySwitchingService @Inject()(
 
   private def canAccessDatasetOrAnnotationOrWorkflow(user: User,
                                                      datasetIdOpt: Option[ObjectId],
-                                                     annotationIdOpt: Option[String],
+                                                     annotationIdOpt: Option[ObjectId],
                                                      workflowHashOpt: Option[String]): Fox[Boolean] = {
     val ctx = AuthorizedAccessContext(user)
     (datasetIdOpt, annotationIdOpt, workflowHashOpt) match {
@@ -99,12 +98,11 @@ class AccessibleBySwitchingService @Inject()(
     foundFox.shiftBox.map(_.isDefined)
   }
 
-  private def canAccessAnnotation(user: User, ctx: DBAccessContext, annotationId: String): Fox[Boolean] = {
+  private def canAccessAnnotation(user: User, ctx: DBAccessContext, annotationId: ObjectId): Fox[Boolean] = {
     val foundFox = for {
-      annotationIdParsed <- ObjectId.fromString(annotationId)
-      annotation <- annotationDAO.findOne(annotationIdParsed)(GlobalAccessContext)
+      annotation <- annotationDAO.findOne(annotationId)(GlobalAccessContext)
       _ <- Fox.fromBool(annotation.state != Cancelled)
-      restrictions <- annotationProvider.restrictionsFor(AnnotationIdentifier(annotation.typ, annotationIdParsed))(ctx)
+      restrictions <- annotationProvider.restrictionsFor(AnnotationIdentifier(annotation.typ, annotationId))(ctx)
       _ <- restrictions.allowAccess(user)
     } yield ()
     foundFox.shiftBox.map(_.isDefined)

build.sbt

@@ -34,13 +34,10 @@ Compile / console / scalacOptions -= "-Xlint:unused"
 scapegoatIgnoredFiles := Seq(".*/Tables.scala", ".*/Routes.scala", ".*/.*mail.*template\\.scala")
 scapegoatDisabledInspections := Seq("FinalModifierOnCaseClass", "UnusedMethodParameter", "UnsafeTraversableMethods")
 
-// Allow path binding for ObjectId
-routesImport += "com.scalableminds.util.objectid.ObjectId"
-
 lazy val commonSettings = Seq(
   resolvers ++= DependencyResolvers.dependencyResolvers,
   Compile / doc / sources := Seq.empty,
-  Compile / packageDoc / publishArtifact := false
+  Compile / packageDoc / publishArtifact := false,
 )
 
 lazy val protocolBufferSettings = Seq(
@@ -92,6 +89,7 @@ lazy val webknossosDatastore = (project in file("webknossos-datastore"))
       }
       ((libs +++ subs +++ targets) ** "*.jar").classpath
     },
+    routesImport += "com.scalableminds.util.objectid.ObjectId",
     copyMessagesFilesSetting
   )
 
@@ -102,11 +100,12 @@ lazy val webknossosTracingstore = (project in file("webknossos-tracingstore"))
   .settings(
     name := "webknossos-tracingstore",
     commonSettings,
+    routesImport += "com.scalableminds.util.objectid.ObjectId",
     generateReverseRouter := false,
     BuildInfoSettings.webknossosTracingstoreBuildInfoSettings,
     libraryDependencies ++= Dependencies.webknossosTracingstoreDependencies,
     dependencyOverrides ++= Dependencies.dependencyOverrides,
-    copyMessagesFilesSetting
+    copyMessagesFilesSetting,
   )
 
 lazy val webknossos = (project in file("."))
@@ -116,6 +115,7 @@ lazy val webknossos = (project in file("."))
   .settings(
     name := "webknossos",
     commonSettings,
+    routesImport += "com.scalableminds.util.objectid.ObjectId",
     generateReverseRouter := false,
     AssetCompilation.settings,
     BuildInfoSettings.webknossosBuildInfoSettings,

conf/webknossos.latest.routes

@@ -28,7 +28,7 @@ GET           /auth/token
 DELETE        /auth/token                                                                       controllers.AuthenticationController.deleteToken()
 GET           /auth/switch                                                                      controllers.AuthenticationController.switchMultiUser(to: String)
 POST          /auth/switchOrganization/:organizationId                                          controllers.AuthenticationController.switchOrganization(organizationId: String)
-GET           /auth/accessibleBySwitching                                                       controllers.AuthenticationController.accessibleBySwitching(datasetId: Option[ObjectId], annotationId: Option[String], workflowHash: Option[String])
+GET           /auth/accessibleBySwitching                                                       controllers.AuthenticationController.accessibleBySwitching(datasetId: Option[ObjectId], annotationId: Option[ObjectId], workflowHash: Option[String])
 POST          /auth/sendInvites                                                                 controllers.AuthenticationController.sendInvites()
 POST          /auth/startResetPassword                                                          controllers.AuthenticationController.handleStartResetPassword()
 POST          /auth/changePassword                                                              controllers.AuthenticationController.changePassword()

frontend/javascripts/test/backend-snapshot-tests/annotations.e2e.ts

@@ -199,6 +199,8 @@ describe("Annotation API (E2E)", () => {
           [UpdateActions.updateCameraAnnotation([2, 3, 4], null, [1, 2, 3], 2)],
         ],
         123456789,
+        null,
+        true,
       ),
       0,
     );
@@ -246,6 +248,8 @@ describe("Annotation API (E2E)", () => {
       createSaveQueueFromUpdateActions(
         [createTreesUpdateActions, [updateTreeGroupsUpdateAction]],
         123456789,
+        null,
+        true,
       ),
       0,
     );
@@ -284,7 +288,12 @@ describe("Annotation API (E2E)", () => {
 
     const updateTreeAction = UpdateActions.updateTree(trees.getOrThrow(1), tracingId);
     const [saveQueue] = addVersionNumbers(
-      createSaveQueueFromUpdateActions([createTreesUpdateActions, [updateTreeAction]], 123456789),
+      createSaveQueueFromUpdateActions(
+        [createTreesUpdateActions, [updateTreeAction]],
+        123456789,
+        null,
+        true,
+      ),
       0,
     );
 
@@ -301,6 +310,8 @@ describe("Annotation API (E2E)", () => {
       createSaveQueueFromUpdateActions(
         [[UpdateActions.updateMetadataOfAnnotation(newDescription)]],
         123456789,
+        null,
+        true,
       ),
       0,
     );

frontend/javascripts/test/e2e-setup.ts

@@ -34,6 +34,8 @@ const tokenUserFInOrgaX =
 
 let currentUserAuthToken = tokenUserA;
 
+const idUserA = "570b9f4d2a7c0e4d008da6ef";
+
 function setUserAuthToken(token: string) {
   currentUserAuthToken = token;
 }
@@ -163,4 +165,5 @@ export {
   tokenUserFInOrgaX,
   tokenUserFInOrgaY,
   setUserAuthToken,
+  idUserA,
 };

frontend/javascripts/test/helpers/saveHelpers.ts

@@ -1,12 +1,14 @@
 import type { TracingStats } from "viewer/model/accessors/annotation_accessor";
 import type { UpdateActionWithoutIsolationRequirement } from "viewer/model/sagas/update_actions";
 import type { SaveQueueEntry } from "viewer/store";
+import { idUserA } from "test/e2e-setup";
 import dummyUser from "test/fixtures/dummy_user";
 
 export function createSaveQueueFromUpdateActions(
   updateActions: UpdateActionWithoutIsolationRequirement[][],
   timestamp: number,
   stats: TracingStats | null = null,
+  useE2eAuthorId: boolean = false,
 ): SaveQueueEntry[] {
   return updateActions.map((ua) => ({
     version: -1,
@@ -15,7 +17,7 @@ export function createSaveQueueFromUpdateActions(
     actions: ua,
     info: "[]",
     transactionGroupCount: 1,
-    authorId: dummyUser.id,
+    authorId: useE2eAuthorId ? idUserA : dummyUser.id,
     transactionGroupIndex: 0,
     transactionId: "dummyRequestId",
   }));