Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ObjectId directly as parameter in routes #8285

Merged
merged 11 commits into from
Feb 5, 2025
Merged

Use ObjectId directly as parameter in routes #8285

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bb1e871
Use ObjectId directly as parameter in routes
frcroth Dec 16, 2024
4785ae0
Use ObjectId for annotations
frcroth Dec 16, 2024
fee2261
Replace some more occurences
frcroth Dec 16, 2024
33647b1
Remove option query binder
frcroth Dec 18, 2024
4d0d0c4
Merge branch 'master' into objectid-as-parameter
frcroth Dec 18, 2024
ec14d7c
Merge branch 'master' into objectid-as-parameter
frcroth Jan 8, 2025
3c09994
Merge branch 'master' into objectid-as-parameter
frcroth Jan 29, 2025
6211c66
Merge branch 'master' into objectid-as-parameter
frcroth Feb 3, 2025
a048a10
Fix type
frcroth Feb 3, 2025
5654e25
Merge branch 'master' into objectid-as-parameter
frcroth Feb 3, 2025
67aebae
Merge branch 'master' into objectid-as-parameter
fm3 Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 11 additions & 15 deletions app/controllers/AiModelController.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,23 +86,21 @@ class AiModelController @Inject()(
extends Controller
with FoxImplicits {

def readAiModelInfo(aiModelId: String): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
def readAiModelInfo(aiModelId: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
{
for {
_ <- userService.assertIsSuperUser(request.identity)
aiModelIdValidated <- ObjectId.fromString(aiModelId)
aiModel <- aiModelDAO.findOne(aiModelIdValidated) ?~> "aiModel.notFound" ~> NOT_FOUND
aiModel <- aiModelDAO.findOne(aiModelId) ?~> "aiModel.notFound" ~> NOT_FOUND
jsResult <- aiModelService.publicWrites(aiModel)
} yield Ok(jsResult)
}
}

def readAiInferenceInfo(aiInferenceId: String): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
def readAiInferenceInfo(aiInferenceId: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
{
for {
_ <- userService.assertIsSuperUser(request.identity)
aiInferenceIdValidated <- ObjectId.fromString(aiInferenceId)
aiInference <- aiInferenceDAO.findOne(aiInferenceIdValidated) ?~> "aiInference.notFound" ~> NOT_FOUND
aiInference <- aiInferenceDAO.findOne(aiInferenceId) ?~> "aiInference.notFound" ~> NOT_FOUND
jsResult <- aiInferenceService.publicWrites(aiInference, request.identity)
} yield Ok(jsResult)
}
Expand Down Expand Up @@ -213,15 +211,14 @@ class AiModelController @Inject()(
} yield Ok(newAiModelJs)
}

def updateAiModelInfo(aiModelId: String): Action[UpdateAiModelParameters] =
def updateAiModelInfo(aiModelId: ObjectId): Action[UpdateAiModelParameters] =
sil.SecuredAction.async(validateJson[UpdateAiModelParameters]) { implicit request =>
for {
_ <- userService.assertIsSuperUser(request.identity)
aiModelIdValidated <- ObjectId.fromString(aiModelId)
aiModel <- aiModelDAO.findOne(aiModelIdValidated) ?~> "aiModel.notFound" ~> NOT_FOUND
aiModel <- aiModelDAO.findOne(aiModelId) ?~> "aiModel.notFound" ~> NOT_FOUND
_ <- aiModelDAO.updateOne(
aiModel.copy(name = request.body.name, comment = request.body.comment, modified = Instant.now))
updatedAiModel <- aiModelDAO.findOne(aiModelIdValidated) ?~> "aiModel.notFound" ~> NOT_FOUND
updatedAiModel <- aiModelDAO.findOne(aiModelId) ?~> "aiModel.notFound" ~> NOT_FOUND
jsResult <- aiModelService.publicWrites(updatedAiModel)
} yield Ok(jsResult)
}
Expand All @@ -248,15 +245,14 @@ class AiModelController @Inject()(
} yield Ok
}

def deleteAiModel(aiModelId: String): Action[AnyContent] =
def deleteAiModel(aiModelId: ObjectId): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
_ <- userService.assertIsSuperUser(request.identity)
aiModelIdValidated <- ObjectId.fromString(aiModelId)
referencesCount <- aiInferenceDAO.countForModel(aiModelIdValidated)
referencesCount <- aiInferenceDAO.countForModel(aiModelId)
_ <- bool2Fox(referencesCount == 0) ?~> "aiModel.delete.referencedByInferences"
_ <- aiModelDAO.findOne(aiModelIdValidated) ?~> "aiModel.notFound" ~> NOT_FOUND
_ <- aiModelDAO.deleteOne(aiModelIdValidated)
_ <- aiModelDAO.findOne(aiModelId) ?~> "aiModel.notFound" ~> NOT_FOUND
_ <- aiModelDAO.deleteOne(aiModelId)
} yield Ok
}

Expand Down
65 changes: 32 additions & 33 deletions app/controllers/AnnotationController.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class AnnotationController @Inject()(
def info( // Type of the annotation, one of Task, Explorational, CompoundTask, CompoundProject, CompoundTaskType
typ: String,
// For Task and Explorational annotations, id is an annotation id. For CompoundTask, id is a task id. For CompoundProject, id is a project id. For CompoundTaskType, id is a task type id
id: String,
id: ObjectId,
// Timestamp in milliseconds (time at which the request is sent)
timestamp: Option[Long]): Action[AnyContent] = sil.UserAwareAction.async { implicit request =>
log() {
Expand Down Expand Up @@ -99,7 +99,7 @@ class AnnotationController @Inject()(
}
}

def infoWithoutType(id: String,
def infoWithoutType(id: ObjectId,
// Timestamp in milliseconds (time at which the request is sent)
timestamp: Option[Long]): Action[AnyContent] = sil.UserAwareAction.async { implicit request =>
log() {
Expand All @@ -111,7 +111,7 @@ class AnnotationController @Inject()(
}
}

def merge(typ: String, id: String, mergedTyp: String, mergedId: String): Action[AnyContent] =
def merge(typ: String, id: ObjectId, mergedTyp: String, mergedId: ObjectId): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
annotationA <- provider.provideAnnotation(typ, id, request.identity) ?~> "annotation.notFound" ~> NOT_FOUND
Expand All @@ -124,15 +124,15 @@ class AnnotationController @Inject()(
} yield JsonOk(js, Messages("annotation.merge.success"))
}

def mergeWithoutType(id: String, mergedTyp: String, mergedId: String): Action[AnyContent] =
def mergeWithoutType(id: ObjectId, mergedTyp: String, mergedId: ObjectId): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
annotation <- provider.provideAnnotation(id, request.identity) ?~> "annotation.notFound" ~> NOT_FOUND
result <- merge(annotation.typ.toString, id, mergedTyp, mergedId)(request)
} yield result
}

def reset(typ: String, id: String): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
def reset(typ: String, id: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
for {
annotation <- provider.provideAnnotation(typ, id, request.identity) ?~> "annotation.notFound" ~> NOT_FOUND
_ <- Fox.assertTrue(userService.isTeamManagerOrAdminOf(request.identity, annotation._team))
Expand All @@ -142,7 +142,7 @@ class AnnotationController @Inject()(
} yield JsonOk(json, Messages("annotation.reset.success"))
}

def reopen(typ: String, id: String): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
def reopen(typ: String, id: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
def isReopenAllowed(user: User, annotation: Annotation) =
for {
isAdminOrTeamManager <- userService.isTeamManagerOrAdminOf(user, annotation._team)
Expand All @@ -162,8 +162,8 @@ class AnnotationController @Inject()(
} yield JsonOk(json, Messages("annotation.reopened"))
}

def editLockedState(typ: String, id: String, isLockedByOwner: Boolean): Action[AnyContent] = sil.SecuredAction.async {
implicit request =>
def editLockedState(typ: String, id: ObjectId, isLockedByOwner: Boolean): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
annotation <- provider.provideAnnotation(typ, id, request.identity)
_ <- bool2Fox(annotation._user == request.identity._id) ?~> "annotation.isLockedByOwner.notAllowed"
Expand All @@ -174,13 +174,12 @@ class AnnotationController @Inject()(
updatedAnnotation <- provider.provideAnnotation(typ, id, request.identity) ~> NOT_FOUND
json <- annotationService.publicWrites(updatedAnnotation, Some(request.identity)) ?~> "annotation.write.failed"
} yield JsonOk(json, Messages("annotation.isLockedByOwner.success"))
}
}

def createExplorational(datasetId: String): Action[List[AnnotationLayerParameters]] =
def createExplorational(datasetId: ObjectId): Action[List[AnnotationLayerParameters]] =
sil.SecuredAction.async(validateJson[List[AnnotationLayerParameters]]) { implicit request =>
for {
datasetIdValidated <- ObjectId.fromString(datasetId)
dataset <- datasetDAO.findOne(datasetIdValidated) ?~> Messages("dataset.notFound", datasetIdValidated) ~> NOT_FOUND
dataset <- datasetDAO.findOne(datasetId) ?~> Messages("dataset.notFound", datasetId) ~> NOT_FOUND
annotation <- annotationService.createExplorationalFor(
request.identity,
dataset._id,
Expand All @@ -192,12 +191,11 @@ class AnnotationController @Inject()(
} yield JsonOk(json)
}

def getSandbox(datasetId: String, typ: String, sharingToken: Option[String]): Action[AnyContent] =
def getSandbox(datasetId: ObjectId, typ: String, sharingToken: Option[String]): Action[AnyContent] =
sil.UserAwareAction.async { implicit request =>
val ctx = URLSharing.fallbackTokenAccessContext(sharingToken) // users with dataset sharing token may also get a sandbox annotation
for {
datasetIdValidated <- ObjectId.fromString(datasetId)
dataset <- datasetDAO.findOne(datasetIdValidated)(ctx) ?~> Messages("dataset.notFound", datasetIdValidated) ~> NOT_FOUND
dataset <- datasetDAO.findOne(datasetId)(ctx) ?~> Messages("dataset.notFound", datasetId) ~> NOT_FOUND
tracingType <- TracingType.fromString(typ).toFox
_ <- bool2Fox(tracingType == TracingType.skeleton) ?~> "annotation.sandbox.skeletonOnly"
annotation = Annotation(
Expand All @@ -216,7 +214,7 @@ class AnnotationController @Inject()(
} yield JsonOk(json)
}

private def finishAnnotation(typ: String, id: String, issuingUser: User, timestamp: Instant)(
private def finishAnnotation(typ: String, id: ObjectId, issuingUser: User, timestamp: Instant)(
implicit ctx: DBAccessContext): Fox[(Annotation, String)] =
for {
annotation <- provider.provideAnnotation(typ, id, issuingUser) ~> NOT_FOUND
Expand All @@ -226,7 +224,7 @@ class AnnotationController @Inject()(
_ <- timeSpanService.logUserInteractionIfTheyArePotentialContributor(timestamp, issuingUser, annotation) // log time on tracing end
} yield (updated, message)

def finish(typ: String, id: String, timestamp: Long): Action[AnyContent] = sil.SecuredAction.async {
def finish(typ: String, id: ObjectId, timestamp: Long): Action[AnyContent] = sil.SecuredAction.async {
implicit request =>
log() {
for {
Expand All @@ -242,7 +240,10 @@ class AnnotationController @Inject()(
log() {
withJsonAs[JsArray](request.body \ "annotations") { annotationIds =>
val results = Fox.serialSequence(annotationIds.value.toList) { jsValue =>
jsValue.asOpt[String].toFox.flatMap(id => finishAnnotation(typ, id, request.identity, Instant(timestamp)))
jsValue
.asOpt[String]
.toFox
.flatMap(id => finishAnnotation(typ, ObjectId(id), request.identity, Instant(timestamp)))
Comment on lines +243 to +246
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor to use ObjectId directly in JSON parsing

The current implementation still handles IDs as strings and manually converts them to ObjectId. This is inconsistent with the PR's objective of using ObjectId directly.

Consider modifying the JSON parsing to handle ObjectId directly:

-            jsValue
-              .asOpt[String]
-              .toFox
-              .flatMap(id => finishAnnotation(typ, ObjectId(id), request.identity, Instant(timestamp)))
+            jsValue
+              .asOpt[ObjectId]
+              .toFox
+              .flatMap(id => finishAnnotation(typ, id, request.identity, Instant(timestamp)))

Committable suggestion skipped: line range outside the PR's diff.

}

results.map { _ =>
Expand All @@ -252,7 +253,7 @@ class AnnotationController @Inject()(
}
}

def editAnnotation(typ: String, id: String): Action[JsValue] = sil.SecuredAction.async(parse.json) {
def editAnnotation(typ: String, id: ObjectId): Action[JsValue] = sil.SecuredAction.async(parse.json) {
implicit request =>
for {
annotation <- provider.provideAnnotation(typ, id, request.identity) ~> NOT_FOUND
Expand All @@ -276,7 +277,7 @@ class AnnotationController @Inject()(
} yield JsonOk(Messages("annotation.edit.success"))
}

def editAnnotationLayer(typ: String, id: String, tracingId: String): Action[JsValue] =
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
Expand All @@ -287,19 +288,18 @@ class AnnotationController @Inject()(
} yield JsonOk(Messages("annotation.edit.success"))
}

def annotationsForTask(taskId: String): Action[AnyContent] =
def annotationsForTask(taskId: ObjectId): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
taskIdValidated <- ObjectId.fromString(taskId)
task <- taskDAO.findOne(taskIdValidated) ?~> "task.notFound" ~> NOT_FOUND
task <- taskDAO.findOne(taskId) ?~> "task.notFound" ~> NOT_FOUND
project <- projectDAO.findOne(task._project)
_ <- Fox.assertTrue(userService.isTeamManagerOrAdminOf(request.identity, project._team))
annotations <- annotationService.annotationsFor(task._id) ?~> "task.annotation.failed"
jsons <- Fox.serialSequence(annotations)(a => annotationService.publicWrites(a, Some(request.identity)))
} yield Ok(JsArray(jsons.flatten))
}

def cancel(typ: String, id: String): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
def cancel(typ: String, id: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
def tryToCancel(annotation: Annotation) =
annotation match {
case t if t.typ == AnnotationType.Task =>
Expand All @@ -319,14 +319,14 @@ class AnnotationController @Inject()(
} yield result
}

def cancelWithoutType(id: String): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
def cancelWithoutType(id: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
for {
annotation <- provider.provideAnnotation(id, request.identity) ~> NOT_FOUND
result <- cancel(annotation.typ.toString, id)(request)
} yield result
}

def transfer(typ: String, id: String): Action[JsValue] = sil.SecuredAction.async(parse.json) { implicit request =>
def transfer(typ: String, id: ObjectId): Action[JsValue] = sil.SecuredAction.async(parse.json) { implicit request =>
for {
restrictions <- provider.restrictionsFor(typ, id) ?~> "restrictions.notFound" ~> NOT_FOUND
_ <- restrictions.allowFinish(request.identity) ?~> "notAllowed" ~> FORBIDDEN
Expand All @@ -337,7 +337,7 @@ class AnnotationController @Inject()(
} yield JsonOk(json)
}

def duplicate(typ: String, id: String): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
def duplicate(typ: String, id: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
for {
annotation <- provider.provideAnnotation(typ, id, request.identity) ~> NOT_FOUND
newAnnotation <- duplicateAnnotation(annotation, request.identity) ?~> "annotation.duplicate.failed"
Expand Down Expand Up @@ -376,7 +376,7 @@ class AnnotationController @Inject()(

}

def getSharedTeams(typ: String, id: String): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
def getSharedTeams(typ: String, id: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
for {
annotation <- provider.provideAnnotation(typ, id, request.identity)
_ <- bool2Fox(annotation._user == request.identity._id) ?~> "notAllowed" ~> FORBIDDEN
Expand All @@ -385,7 +385,7 @@ class AnnotationController @Inject()(
} yield Ok(Json.toJson(json))
}

def updateSharedTeams(typ: String, id: String): Action[JsValue] = sil.SecuredAction.async(parse.json) {
def updateSharedTeams(typ: String, id: ObjectId): Action[JsValue] = sil.SecuredAction.async(parse.json) {
implicit request =>
withJsonBodyAs[List[String]] { teams =>
for {
Expand All @@ -399,7 +399,7 @@ class AnnotationController @Inject()(
}
}

def updateOthersMayEdit(typ: String, id: String, othersMayEdit: Boolean): Action[AnyContent] =
def updateOthersMayEdit(typ: String, id: ObjectId, othersMayEdit: Boolean): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
annotation <- provider.provideAnnotation(typ, id, request.identity)
Expand Down Expand Up @@ -438,14 +438,13 @@ class AnnotationController @Inject()(
_ <- annotationDAO.insertOne(clonedAnnotation)
} yield clonedAnnotation

def tryAcquiringAnnotationMutex(id: String): Action[AnyContent] =
def tryAcquiringAnnotationMutex(id: ObjectId): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
logTime(slackNotificationService.noticeSlowRequest, durationThreshold = 1 second) {
for {
idValidated <- ObjectId.fromString(id)
annotation <- provider.provideAnnotation(id, request.identity) ~> NOT_FOUND
_ <- bool2Fox(annotation.othersMayEdit) ?~> "notAllowed" ~> FORBIDDEN
restrictions <- provider.restrictionsFor(AnnotationIdentifier(annotation.typ, idValidated)) ?~> "restrictions.notFound" ~> NOT_FOUND
restrictions <- provider.restrictionsFor(AnnotationIdentifier(annotation.typ, id)) ?~> "restrictions.notFound" ~> NOT_FOUND
_ <- restrictions.allowUpdate(request.identity) ?~> "notAllowed" ~> FORBIDDEN
mutexResult <- annotationMutexService.tryAcquiringAnnotationMutex(annotation._id, request.identity._id) ?~> "annotation.mutex.failed"
resultJson <- annotationMutexService.publicWrites(mutexResult)
Expand Down
Loading