Use ObjectId directly as parameter in routes #8285
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of identifier handling across multiple Scala controllers and routes in the WebKnossos application. The primary change involves transitioning from using Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
app/controllers/ProjectController.scala (2)
Line range hint
120-133: Inconsistent ObjectId usage detectedWhile the parameter has been changed to ObjectId, line 123 converts it back to String for the database query. This suggests that
findAllWithTaskTypeinprojectDAOhasn't been updated to use ObjectId directly.Consider updating the DAO layer to use ObjectId consistently:
- projects <- projectDAO.findAllWithTaskType(taskTypeId.toString) + projects <- projectDAO.findAllWithTaskType(taskTypeId)
Line range hint
178-192: Inconsistent ObjectId handling in JSON parsingWhile the method parameter uses ObjectId, the user ID is still parsed as String from JSON and manually converted. Consider using a JSON format that handles ObjectId directly.
Example improvement:
// Add to companion object implicit val objectIdReads: Reads[ObjectId] = Reads.of[String].map(ObjectId.fromString(_).get) // In the method newUserId <- (request.body \ "userId").asOpt[ObjectId].toFox ?~> "user.id.notFound" ~> NOT_FOUND // Remove the manual conversion line
🧹 Nitpick comments (9)
app/controllers/VoxelyticsController.scala (2)
113-127: LGTM! Consider adding parameter validation.The change from
Option[String]toOption[ObjectId]aligns well with the PR objectives. While the type change eliminates string conversion overhead, consider adding validation for the workflowHash parameter since it remains a String.def getWorkflow(workflowHash: String, runIdOpt: Option[ObjectId]): Action[AnyContent] = sil.SecuredAction.async { implicit request => for { _ <- bool2Fox(wkConf.Features.voxelyticsEnabled) ?~> "voxelytics.disabled" + _ <- bool2Fox(workflowHash.nonEmpty) ?~> "voxelytics.invalidWorkflowHash" ~> BAD_REQUEST workflow <- voxelyticsDAO.findWorkflowByHashAndOrganization(...)
Line range hint
268-279: Consider caching runName lookup for performance.While the conversion to
ObjectIdis correct, the additional database query to getrunNamecould impact performance. Consider:
- Caching the runName lookup results
- Including runName in the previous database queries where possible
// Option 1: Add caching private val runNameCache = TrieMap[ObjectId, String]() def getLogs(...) = { ... runName <- runNameCache.get(runId).map(Fox.successful).getOrElse { voxelyticsDAO.getRunNameById(runId, request.identity._organization).map { name => runNameCache.put(runId, name) name } } ... }app/controllers/PublicationController.scala (1)
25-25: LGTM! Good improvement in type safety.The change from
StringtoObjectIdis a solid architectural improvement that:
- Provides compile-time type safety
- Eliminates runtime string-to-ObjectId conversion overhead
- Reduces potential for invalid ID formats reaching the controller
app/controllers/WKRemoteDataStoreController.scala (1)
259-265: Add explicit error handling for credential not foundThe change to use ObjectId directly is good, but consider adding explicit error handling:
def findCredential(name: String, key: String, credentialId: ObjectId): Action[AnyContent] = Action.async { implicit request => dataStoreService.validateAccess(name, key) { _ => for { - credential <- credentialDAO.findOne(credentialId) + credential <- credentialDAO.findOne(credentialId) ?~> Messages("credential.notFound") ~> NOT_FOUND } yield Ok(Json.toJson(credential)) } }app/controllers/ProjectController.scala (1)
89-99: Consider using an atomic update operationWhile the conversion to ObjectId is good, there's a potential race condition between the find and update operations. Consider combining these into an atomic update operation.
Example approach:
def update(id: ObjectId): Action[JsValue] = sil.SecuredAction.async(parse.json) { implicit request => withJsonBodyUsing(Project.projectPublicReads) { updateRequest => for { result <- projectDAO.findAndUpdate( id, updateRequest, request.identity, GlobalAccessContext ) ?~> "project.update.failed" js <- projectService.publicWrites(result) } yield Ok(js) } }app/controllers/TimeController.scala (2)
49-60: Consider enhancing error messages for better debuggingThe type signature change from
StringtoObjectIdis good. However, consider making the error messages more specific by including the actual values that failed validation.- user <- userService.findOneCached(userId) ?~> "user.notFound" ~> NOT_FOUND + user <- userService.findOneCached(userId) ?~> s"user.notFound: $userId" ~> NOT_FOUND
72-83: Consider extracting common validation logicThe type signature change is good, but there's significant code duplication between this method and
timeSummedByAnnotationForUserin terms of parameter validation and authorization checks.Consider extracting the common validation logic into a shared method:
private def validateTimeQueryParams( userId: ObjectId, annotationTypes: String, annotationStates: String, projectIds: Option[String], requestUser: User )(implicit request: Request[AnyContent]): Fox[(User, List[ObjectId], List[AnnotationType], List[AnnotationState])] = { for { projectIdsValidated <- ObjectId.fromCommaSeparated(projectIds) annotationTypesValidated <- AnnotationType.fromCommaSeparated(annotationTypes) ?~> "invalidAnnotationType" annotationStatesValidated <- AnnotationState.fromCommaSeparated(annotationStates) ?~> "invalidAnnotationState" user <- userService.findOneCached(userId) ?~> s"user.notFound: $userId" ~> NOT_FOUND isTeamManagerOrAdmin <- userService.isTeamManagerOrAdminOf(requestUser, user) _ <- bool2Fox(isTeamManagerOrAdmin || user._id == requestUser._id) ?~> "user.notAuthorised" ~> FORBIDDEN } yield (user, projectIdsValidated, annotationTypesValidated, annotationStatesValidated) }app/controllers/ConfigurationController.scala (1)
38-49: Consider adding validation for sharing token when dataset ID is invalidWhile the ObjectId parameter change is correct, consider enhancing error handling when both the sharing token and dataset ID are invalid.
def readDatasetViewConfiguration(datasetId: ObjectId, sharingToken: Option[String]): Action[List[String]] = sil.UserAwareAction.async(validateJson[List[String]]) { implicit request => val ctx = URLSharing.fallbackTokenAccessContext(sharingToken) for { + _ <- datasetDAO.findOne(datasetId)(ctx) ?~> "dataset.notFound" ~> NOT_FOUND configuration <- request.identity.toFox .flatMap(user => datasetConfigurationService.getDatasetViewConfigurationForUserAndDataset(request.body, user, datasetId)( GlobalAccessContext)) .orElse( datasetConfigurationService.getDatasetViewConfigurationForDataset(request.body, datasetId)(ctx) ) .getOrElse(Map.empty) } yield Ok(Json.toJson(configuration)) }app/controllers/AiModelController.scala (1)
248-257: Consider wrapping deletion in a transactionWhile the change to ObjectId is good, the multiple database operations (reference check and deletion) should ideally be wrapped in a transaction to ensure atomicity.
Consider wrapping the operations in a transaction:
def deleteAiModel(aiModelId: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request => for { _ <- userService.assertIsSuperUser(request.identity) - referencesCount <- aiInferenceDAO.countForModel(aiModelId) - _ <- bool2Fox(referencesCount == 0) ?~> "aiModel.delete.referencedByInferences" - _ <- aiModelDAO.findOne(aiModelId) ?~> "aiModel.notFound" ~> NOT_FOUND - _ <- aiModelDAO.deleteOne(aiModelId) + _ <- sqlClient.transactionally { implicit session => + for { + referencesCount <- aiInferenceDAO.countForModel(aiModelId) + _ <- bool2Fox(referencesCount == 0) ?~> "aiModel.delete.referencedByInferences" + _ <- aiModelDAO.findOne(aiModelId) ?~> "aiModel.notFound" ~> NOT_FOUND + _ <- aiModelDAO.deleteOne(aiModelId) + } yield () + } } yield Ok }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
app/controllers/AiModelController.scala(3 hunks)app/controllers/AnnotationController.scala(25 hunks)app/controllers/AnnotationIOController.scala(9 hunks)app/controllers/AnnotationPrivateLinkController.scala(2 hunks)app/controllers/AuthenticationController.scala(1 hunks)app/controllers/ConfigurationController.scala(1 hunks)app/controllers/DatasetController.scala(13 hunks)app/controllers/FolderController.scala(2 hunks)app/controllers/JobController.scala(14 hunks)app/controllers/LegacyApiController.scala(10 hunks)app/controllers/MaintenanceController.scala(1 hunks)app/controllers/MeshController.scala(2 hunks)app/controllers/ProjectController.scala(5 hunks)app/controllers/PublicationController.scala(1 hunks)app/controllers/ReportController.scala(1 hunks)app/controllers/ScriptController.scala(3 hunks)app/controllers/TaskController.scala(3 hunks)app/controllers/TaskTypeController.scala(3 hunks)app/controllers/TeamController.scala(1 hunks)app/controllers/TimeController.scala(3 hunks)app/controllers/UserController.scala(6 hunks)app/controllers/VoxelyticsController.scala(5 hunks)app/controllers/WKRemoteDataStoreController.scala(1 hunks)app/controllers/WKRemoteWorkerController.scala(1 hunks)app/models/annotation/AnnotationIdentifier.scala(1 hunks)app/models/annotation/AnnotationInformationProvider.scala(2 hunks)app/models/annotation/AnnotationService.scala(1 hunks)build.sbt(1 hunks)conf/webknossos.latest.routes(8 hunks)conf/webknossos.versioned.routes(4 hunks)util/src/main/scala/com/scalableminds/util/objectid/ObjectId.scala(2 hunks)
👮 Files not reviewed due to content moderation or server errors (6)
- app/controllers/MeshController.scala
- app/controllers/TaskTypeController.scala
- app/controllers/FolderController.scala
- app/controllers/LegacyApiController.scala
- app/controllers/UserController.scala
- app/controllers/JobController.scala
🔇 Additional comments (84)
app/controllers/VoxelyticsController.scala (2)
223-229: LGTM! Type-safe parameter handling implemented correctly.
The conversion to Option[ObjectId] is implemented consistently with proper error handling and database query adaptation.
240-248: LGTM! Consistent implementation of ObjectId parameter.
The change maintains consistency with other methods and properly handles the ObjectId type in database queries.
app/controllers/PublicationController.scala (1)
25-31: Verify route configuration update.
While the controller change is correct, we should verify that the corresponding route configuration has been updated to handle ObjectId parameters correctly.
app/controllers/WKRemoteDataStoreController.scala (1)
245-265: Consider comprehensive testing of modified endpoints
Since these changes affect route parameters, and you mentioned manual testing of some routes:
- Ensure both positive and negative test cases are covered for these endpoints
- Consider adding integration tests to verify the complete request-response cycle
- Verify that error responses are consistent with other similar endpoints in the codebase
Let's check for existing tests:
app/controllers/ReportController.scala (3)
167-170: Error handling looks good!
The changes maintain robust error handling while improving type safety:
- Appropriate use of Fox for error handling
- Clear error messages with proper HTTP status codes
- Reduced potential for runtime errors by using ObjectId directly
Also applies to: 174-179
167-182: Integration with existing code is seamless.
The changes integrate well with the existing codebase:
- Consistent with TeamDAO, UserDAO, and UserService interfaces
- SQL queries in ReportDAO already handle ObjectId parameters
- No breaking changes introduced in the integration points
174-174: LGTM! Verify route definition update.
The change from String to ObjectId parameter improves type safety by moving validation to compile-time. The implementation correctly:
- Removes the unnecessary
ObjectId.fromStringconversion - Uses
teamIddirectly in theuserService.isTeamManagerOrAdminOfcall
Also applies to: 176-176, 179-179
✅ Verification successful
Route definition is correctly updated
The route definition in conf/webknossos.latest.routes already uses ObjectId type parameter:
GET /teams/:id/availableTasksReport controllers.ReportController.availableTasksReport(id: ObjectId)
This matches the controller method signature change, confirming proper type-safety across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the route definition has been updated to match the new signature
# Expected: The route should use ObjectId parameter type
rg "availableTasksReport" conf/routes
Length of output: 37
Script:
#!/bin/bash
# Let's try to find the routes file in other possible locations
fd "routes$"
# Also search for route definitions in Scala files as they might be defined programmatically
rg -A 2 "availableTasksReport.*=>" --type scala
Length of output: 725
Script:
#!/bin/bash
# Let's check the content of all routes files for the availableTasksReport endpoint
for route_file in $(fd "routes$"); do
echo "=== Checking $route_file ==="
rg "availableTasksReport" "$route_file"
done
Length of output: 2275
app/controllers/ProjectController.scala (3)
52-56: LGTM! Clean conversion to ObjectId parameter
The change simplifies the code by removing the need for ObjectId conversion while maintaining proper error handling.
Line range hint 67-73: LGTM! Secure deletion logic maintained
The change to ObjectId maintains all necessary permission checks and proper error handling for the deletion process.
Line range hint 52-192: Verify API compatibility with existing clients
The change from String to ObjectId parameters might affect API clients. Ensure that:
- API documentation is updated to reflect the new parameter types
- Clients are prepared to handle the new ObjectId format
- Consider adding a migration guide if needed
app/controllers/TimeController.scala (2)
30-33: LGTM: Clean type signature update
The change from String to ObjectId for the userId parameter improves type safety and eliminates the need for explicit ObjectId conversion. The error handling remains robust with proper Fox monadic operations.
30-30: Verify route configurations for updated parameter types
Ensure that the route configurations in conf/routes have been updated to handle ObjectId parameters correctly for these endpoints.
Also applies to: 49-49, 72-72
✅ Verification successful
Based on the route configurations shown in the output, I can see that the time tracking endpoints are properly defined in the routes file:
GET /time/user/:userId/spans controllers.TimeController.timeSpansOfUser(userId: ObjectId, start: Long, end: Long, annotationTypes: String, annotationStates: String, projectIds: Option[String])
GET /time/user/:userId/summedByAnnotation controllers.TimeController.timeSummedByAnnotationForUser(userId: ObjectId, start: Long, end: Long, annotationTypes: String, annotationStates: String, projectIds: Option[String])
GET /users/:id/loggedTime controllers.TimeController.userLoggedTime(id: ObjectId)
All these routes are correctly configured to handle ObjectId parameters for user IDs. The route configurations are consistent with the changes made in the TimeController.
Route configurations are properly updated for ObjectId parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check route configurations for these endpoints
# Search for route definitions related to these endpoints
rg --type scala "userLoggedTime|timeSummedByAnnotationForUser|timeSpansOfUser" conf/routes
Length of output: 92
Script:
#!/bin/bash
# Let's try a different approach to find route configurations
# First, let's check the content of routes file without type filtering
cat conf/routes
# As a backup, let's also search for files with 'routes' in their name
fd routes
Length of output: 736
Script:
#!/bin/bash
# Let's check all the route files for the relevant endpoints
cat conf/webknossos.latest.routes conf/webknossos.versioned.routes conf/noDS.noTS.routes conf/noDS.routes conf/noTS.routes 2>/dev/null
# Let's also search for route definitions in Scala router files
rg "userLoggedTime|timeSummedByAnnotationForUser|timeSpansOfUser" webknossos-datastore/conf/com.scalableminds.webknossos.datastore.routes webknossos-tracingstore/conf/com.scalableminds.webknossos.tracingstore.routes
Length of output: 56077
app/controllers/ScriptController.scala (3)
43-45: LGTM! Verify route parameter binding.
The change from String to ObjectId improves type safety and eliminates the need for explicit ObjectId.fromString conversion. The error handling remains robust with the ?~> operator.
Let's verify the route configuration for this endpoint:
✅ Verification successful
LGTM! Route configuration correctly handles ObjectId parameter binding.
The route configuration in conf/webknossos.latest.routes shows that the script endpoint is properly configured to accept ObjectId:
GET /scripts/:id controllers.ScriptController.get(id: ObjectId)
The parameter type matches the controller method signature, confirming that the change from String to ObjectId is consistently implemented across both the routes and controller.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the route configuration properly handles ObjectId parameter binding
rg -A 3 "GET.*scripts/.*scriptId" conf/
Length of output: 41
Script:
#!/bin/bash
# Let's try searching for routes file with different patterns
fd "routes$" conf/
# If found, let's examine its content for script-related routes
fd "routes$" conf/ --exec cat {}
# As a fallback, let's search for any file containing route definitions
rg "GET.*script" conf/
Length of output: 56578
Line range hint 22-24: Verify ObjectId handling in scriptPublicReads.
The scriptPublicReads JSON reader already handles ObjectId for the owner field, which aligns well with the PR's objective of using ObjectId directly. However, ensure that the client-side code properly sends ObjectId-compatible strings.
Let's check the client-side handling:
✅ Verification successful
The previous search in app/assets failed, so let's first understand the repository structure and locate the client-side code before searching for script operations.
I see the client-side code is in the frontend directory. Let's examine how scripts are created and handled, particularly focusing on ObjectId handling in the frontend code.
The test file shows that script IDs are handled properly in the API layer, but we need to verify how the frontend makes these API calls and how the backend model handles ObjectId. Let's examine these files to complete our verification.
Based on the examination of the frontend code, particularly the admin_rest_api.ts file, I can now generate the final response:
The ObjectId handling in scriptPublicReads is safe and consistent with the codebase.
The client-side code properly handles script IDs through the REST API endpoints. The createScript and updateScript functions in admin_rest_api.ts use type-safe interfaces (APIScriptCreator and APIScriptUpdater) and communicate with the backend through JSON, which means:
- The owner field is properly serialized when sent to the server
- The script ID handling is abstracted away from the client code
- The API endpoints handle the conversion between string and ObjectId on the server side
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for client-side script creation/update handling
rg -A 5 "createScript|updateScript" app/assets/
Length of output: 101
Script:
#!/bin/bash
# Let's first find the client-side code directory structure
fd -t d "assets|public|client|frontend"
# Then search for script-related files
fd -t f "script|Script"
# Finally, let's look for any JavaScript/TypeScript files that might handle script operations
fd -e js -e ts -e jsx -e tsx
Length of output: 36475
Script:
#!/bin/bash
# Let's examine the script creation view and related API calls
ast-grep --pattern 'createScript|updateScript' frontend/javascripts/admin/scripts/script_create_view.tsx
# Also check the script list view for any ObjectId handling
ast-grep --pattern 'ObjectId|_id' frontend/javascripts/admin/scripts/script_list_view.tsx
# Let's also check the API test file for script-related tests
cat frontend/javascripts/test/backend-snapshot-tests/scripts.e2e.ts
Length of output: 1999
Script:
#!/bin/bash
# Let's check the admin_rest_api.ts file to see how script creation is handled
cat frontend/javascripts/admin/admin_rest_api.ts
# Also check the script model to understand the ID handling
cat app/models/task/Script.scala
Length of output: 65622
76-81: Consider adding transaction handling for cascading deletes.
While the ObjectId conversion is correct, the cascading delete operations (script deletion and task updates) should ideally be atomic to maintain data consistency. Consider wrapping these operations in a transaction.
Let's check if transactions are used elsewhere in the codebase:
Example transaction implementation:
def delete(scriptId: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
withTransaction { implicit session =>
for {
oldScript <- scriptDAO.findOne(scriptId) ?~> "script.notFound" ~> NOT_FOUND
_ <- bool2Fox(oldScript._owner == request.identity._id) ?~> "script.notOwner" ~> FORBIDDEN
_ <- scriptDAO.deleteOne(scriptId) ?~> "script.removalFailed"
_ <- taskDAO.removeScriptFromAllTasks(scriptId) ?~> "script.removalFailed"
} yield Ok
}
}util/src/main/scala/com/scalableminds/util/objectid/ObjectId.scala (4)
6-6: Necessary Imports Added
The import statement correctly includes PathBindable and QueryStringBindable to enable custom binding of ObjectId in routes.
38-44: Implemented PathBindable[ObjectId] for Path Parameter Binding
The PathBindable[ObjectId] implicit allows seamless conversion between route path parameters and ObjectId instances. The bind and unbind methods are properly defined, providing informative error messages on parsing failures.
46-54: Implemented QueryStringBindable[ObjectId] for Query Parameter Binding
The QueryStringBindable[ObjectId] implicit enables binding ObjectId from query string parameters. The implementation correctly handles parameter extraction and parsing, with appropriate error handling.
56-65: Implemented QueryStringBindable[Option[ObjectId]] for Optional Query Parameters
The QueryStringBindable[Option[ObjectId]] implicit correctly manages optional ObjectId query parameters, returning None when the parameter is absent or empty. The logic is sound and handles parsing reliably.
conf/webknossos.versioned.routes (9)
25-25: Updated V8 Task Route Parameter to ObjectId
The GET /v8/tasks/:id route now accepts id: ObjectId, enhancing type safety and reducing the need for manual parsing in the controller.
27-27: Updated V8 Task Update Route to Use ObjectId
The PUT /v8/tasks/:id route parameter id is now an ObjectId, aligning with the updated controller method signature.
28-30: Enhanced V8 Project and Annotation Routes with ObjectId Parameters
GET /v8/projects/:id/tasksnow usesid: ObjectId.GET /v8/annotations/:id/infonow usesid: ObjectId.GET /v8/tasks/:id/annotationsnow usesid: ObjectId.
These updates improve type safety across the routes.
39-44: Applied ObjectId Parameter Update to V7 Routes
The V7 routes have been updated similarly to V8:
- Task routes now accept
id: ObjectId. - Project and annotation routes have also been updated to use
ObjectId.
47-47: Changed uploaderId and folderId Parameters to Option[ObjectId] in V7
The GET /v7/datasets route now accepts uploaderId: Option[ObjectId] and folderId: Option[ObjectId], providing consistent identifier types.
56-61: Updated V6 Routes to Use ObjectId for Identifiers
The V6 task and annotation routes now accept id: ObjectId, enhancing consistency with newer API versions.
65-65: Modified uploaderId and folderId Parameters to Option[ObjectId] in V6
The GET /v6/datasets route parameters have been changed to Option[ObjectId], aligning with changes in higher API versions.
76-81: Updated V5 Routes with ObjectId Parameters
The task and annotation routes in V5 now use id: ObjectId, promoting type safety across all API versions.
84-84: Adjusted V5 Dataset List Route Parameters
The GET /v5/datasets route now accepts uploaderId: Option[ObjectId] and folderId: Option[ObjectId], maintaining consistency.
conf/webknossos.latest.routes (26)
16-18: Updated Maintenance Routes to Use ObjectId
The id parameter in maintenance routes is now of type ObjectId:
GET /maintenances/:idPUT /maintenances/:idDELETE /maintenances/:id
This change enhances type safety and consistency.
30-30: Changed datasetId to Option[ObjectId] in Authentication Route
The GET /auth/accessibleBySwitching route now accepts datasetId: Option[ObjectId], improving consistency with other routes.
46-49: Updated Dataset Configuration Routes to Use ObjectId
The datasetId parameter in the following routes is now an ObjectId:
POST /datasetConfigurations/:datasetIdPUT /datasetConfigurations/:datasetIdGET /datasetConfigurations/default/:datasetIdPUT /datasetConfigurations/default/:datasetId
This ensures consistent handling of dataset identifiers.
59-66: Modified User Routes to Use ObjectId
The id parameter is now ObjectId in user-related routes:
GET /users/:idPATCH /users/:idPUT /users/:id/taskTypeIdPUT /users/:id/novelUserExperienceInfosPUT /users/:id/selectedThemeGET /users/:id/tasksGET /users/:id/annotationsGET /users/:id/loggedTime
These changes enhance type safety in user management.
71-73: Updated Team Routes with ObjectId Parameters
The id parameter in team-related routes is now an ObjectId:
DELETE /teams/:idGET /teams/:id/availableTasksReportGET /teams/:id/projectProgressReport
76-77: Changed datasetId Parameter to ObjectId in Annotation Routes
POST /datasets/:datasetId/createExplorationalGET /datasets/:datasetId/sandbox/:typ
These routes now use datasetId: ObjectId, aligning with the dataset identifier changes.
78-95: Updated Dataset Routes to Use ObjectId
The datasetId parameter is now ObjectId across dataset-related routes, including:
- Health checks
- Updates
- Access lists
- Sharing tokens
- Thumbnails
- Reading dataset information
This standardizes dataset handling throughout the API.
100-104: Modified Folder Routes with ObjectId Parameters
Folder-related routes now use id: ObjectId and parentId: ObjectId:
POST /folders/createGET /folders/:idPUT /folders/:idPUT /folders/:id/moveDELETE /folders/:id
This change enhances consistency in folder management.
115-116: Updated Datastore Routes with ObjectId Parameters
GET /datastores/:name/jobExportPropertiesnow acceptsjobId: ObjectId.GET /datastores/:name/findCredentialnow acceptscredentialId: ObjectId.
These updates improve type safety in datastore operations.
136-137: Added ObjectId Parameters to Annotation Routes
POST /annotations/:typ/:id/duplicatenow usesid: ObjectId.PATCH /annotations/:typ/:id/editnow usesid: ObjectId.
140-144: Updated Annotation State Modification Routes with ObjectId
The following routes now accept id: ObjectId:
PATCH /annotations/:typ/:id/finishPATCH /annotations/:typ/:id/reopenPUT /annotations/:typ/:id/resetPATCH /annotations/:typ/:id/transferPATCH /annotations/:typ/:id/editLockedState
147-155: Modified Annotation Routes Without Type
Routes that operate on annotations without specifying the type now use id: ObjectId:
GET /annotations/:id/infoPATCH /annotations/:id/makeHybridPATCH /annotations/:id/downsamplePATCH /annotations/:id/addAnnotationLayerPATCH /annotations/:id/deleteAnnotationLayerDELETE /annotations/:idPOST /annotations/:id/merge/:mergedTyp/:mergedIdGET /annotations/:id/downloadPOST /annotations/:id/acquireMutex
158-165: Updated Typed Annotation Routes with ObjectId
For routes with annotation type specified, id is now ObjectId:
GET /annotations/:typ/:id/infoPATCH /annotations/:typ/:id/makeHybridPATCH /annotations/:typ/:id/downsamplePATCH /annotations/:typ/:id/addAnnotationLayerPATCH /annotations/:typ/:id/deleteAnnotationLayerDELETE /annotations/:typ/:idPOST /annotations/:typ/:id/merge/:mergedTyp/:mergedIdGET /annotations/:typ/:id/download
170-172: Adjusted Annotation Sharing Routes
GET /annotations/:typ/:id/sharedTeamsPATCH /annotations/:typ/:id/sharedTeamsPATCH /annotations/:typ/:id/othersMayEdit
These routes now use id: ObjectId.
176-180: Updated Zarr Private Link Routes with ObjectId
GET /zarrPrivateLinks/byAnnotation/:annotationIdnow usesannotationId: ObjectId.PUT /zarrPrivateLinks/:idDELETE /zarrPrivateLinks/:idGET /zarrPrivateLinks/:id
184-188: Modified Mesh Routes to Use ObjectId
Mesh-related routes now accept id: ObjectId:
PUT /meshes/:idDELETE /meshes/:idGET /meshes/:idPUT /meshes/:id/dataGET /meshes/:id/data
195-199: Updated Task Routes with ObjectId
Task-related routes now use id: ObjectId:
GET /tasks/:idDELETE /tasks/:idPUT /tasks/:idPOST /tasks/:id/assignGET /tasks/:id/annotations
205-209: Adjusted TaskType Routes with ObjectId
Task type routes now accept id: ObjectId:
DELETE /taskTypes/:idGET /taskTypes/:id/tasksGET /taskTypes/:id/projectsGET /taskTypes/:idPUT /taskTypes/:id
214-216: Updated Script Routes with ObjectId
Script management routes now use id: ObjectId:
GET /scripts/:idPUT /scripts/:idDELETE /scripts/:id
223-231: Modified Project Routes to Use ObjectId
Project-related routes now accept id: ObjectId:
GET /projects/:idDELETE /projects/:idPUT /projects/:idGET /projects/:id/tasksPATCH /projects/:id/incrementEachTasksInstancesPATCH /projects/:id/pausePATCH /projects/:id/resumeGET /projects/:id/usersWithActiveTasksPOST /projects/:id/transferActiveTasks
255-256: Updated Time Tracking Routes with ObjectId
GET /time/user/:userId/spansnow usesuserId: ObjectId.GET /time/user/:userId/summedByAnnotationnow usesuserId: ObjectId.
263-279: Modified Job Routes with ObjectId Parameters
Job-related routes now accept datasetId: ObjectId and id: ObjectId where applicable, including:
- Running various jobs (e.g.,
convertToWkw,computeMeshFile) - Retrieving job details
- Canceling jobs
- Updating job status
284-290: Updated AI Model Routes with ObjectId
AI model and inference routes now use id: ObjectId:
GET /aiModels/inferences/:idGET /aiModels/:idPUT /aiModels/:idDELETE /aiModels/:id
294-294: Changed Publication Route to Use ObjectId
GET /publications/:idnow acceptsid: ObjectId.
312-316: Modified Voxelytics Workflow Routes with ObjectId and runId Parameters
GET /voxelytics/workflows/:workflowHashnow acceptsrunId: Option[ObjectId].GET /voxelytics/workflows/:workflowHash/chunkStatisticsnow acceptsrunId: Option[ObjectId].
318-318: Updated Voxelytics Logs Route with ObjectId
GET /voxelytics/logsnow usesrunId: ObjectId.
app/models/annotation/AnnotationIdentifier.scala (1)
18-21: Refactored parse Method to Accept ObjectId
The parse method now accepts id: ObjectId instead of String, eliminating the need for internal parsing and enhancing type safety. All calls to this method should ensure they provide an ObjectId.
app/controllers/TeamController.scala (1)
32-40: LGTM! Verify route configuration for this endpoint
The conversion from String to ObjectId parameter looks correct. The error handling and operation sequence are well-maintained.
Let's verify the route configuration and potential API impacts:
✅ Verification successful
Route configuration is correctly updated to use ObjectId
The route configuration in conf/webknossos.latest.routes matches the controller's signature change, accepting ObjectId instead of String. The only remaining ObjectId.fromString conversion for team IDs is in User.scala which is appropriate for JSON deserialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check routes configuration for team deletion endpoint
rg "DELETE.*teams/.*" conf/ -A 2
# Look for any remaining String to ObjectId conversions related to team IDs
rg "ObjectId\.fromString.*team" --type scala
Length of output: 772
app/models/annotation/AnnotationInformationProvider.scala (2)
22-23: LGTM! Method signature changes are consistent
The conversion to ObjectId parameters improves type safety and simplifies the code by removing string conversion logic.
Also applies to: 25-29
32-39: Verify annotation lookup edge cases
While the ObjectId conversion looks correct, we should ensure proper handling of invalid annotation identifiers.
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information about how invalid annotation identifiers are handled throughout the codebase.
Edge cases for annotation lookup are properly handled
The code has comprehensive error handling for invalid annotation identifiers:
- Non-existent annotations return "annotation.notFound"
- Invalid annotation types are caught with "annotationType.notFound"
- Access control is enforced with "notAllowed" responses
- Cancelled annotations are rejected with "annotation.cancelled"
- Proper handling of authentication context with custom messages for logged-out users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential edge cases in annotation lookup
rg "annotation.*notFound" --type scala -B 2 -A 2
# Verify error handling in annotation routes
rg "GET.*annotations/.*" conf/ -A 2
Length of output: 21964
app/controllers/ConfigurationController.scala (1)
53-61: LGTM! Consistent ObjectId usage across configuration methods
The changes to dataset configuration methods are consistent and maintain proper access control checks.
Let's verify the dataset configuration routes and access controls:
Also applies to: 64-69, 71-78
✅ Verification successful
Let me generate additional verification scripts to check the configuration routes and service implementations:
Based on the verification results, I can now generate the final response:
Access controls and configuration updates are properly implemented
The codebase shows consistent implementation of access controls:
- All dataset configuration updates require
isEditableBypermission checks - The
updateDatasetViewConfigurationmethod is properly integrated with user and dataset services - Access control checks are enforced at both controller and service levels
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check dataset configuration routes
rg "POST.*datasets/.*/configuration" conf/ -A 2
# Verify access control checks
rg "isEditableBy.*dataset" --type scala -B 2 -A 2
Length of output: 4726
Script:
#!/bin/bash
# Check dataset configuration routes in routes file
rg "POST.*configuration" conf/routes
# Check dataset configuration service implementation
ast-grep --pattern 'def updateDatasetViewConfiguration($$$)'
# Check if there are any other configuration update methods
rg "update.*Configuration" --type scala
Length of output: 3007
build.sbt (1)
36-38: LGTM! Verify route compilation after this change.
The addition of ObjectId to routesImport is correct and necessary for enabling direct ObjectId parameters in routes.
✅ Verification successful
LGTM! The ObjectId import is being used correctly across route definitions.
The verification confirms that ObjectId is being used extensively and correctly as a parameter type across various route definitions in both the latest and versioned routes. The import is necessary and properly utilized for ID parameters in endpoints related to:
- Maintenance operations
- User management
- Dataset operations
- Annotation handling
- Task management
- Project management
and many other core functionalities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that routes are using ObjectId parameters correctly
# Look for route definitions using ObjectId parameters
rg "ObjectId" conf/ -A 2 -B 2
Length of output: 47728
app/controllers/AnnotationPrivateLinkController.scala (2)
67-72: LGTM! Parameter type change is clean and consistent.
The change from String to ObjectId for annotationId parameter improves type safety.
114-120: LGTM! Clean conversion to ObjectId parameter.
The delete method's parameter type change is clean and maintains proper error handling.
app/controllers/WKRemoteWorkerController.scala (3)
75-85: LGTM! Clean parameter type change with proper job tracking.
The change to ObjectId parameter is clean and maintains proper job status tracking and cleanup.
104-114: LGTM! Clean parameter type change with proper error handling.
The change maintains proper error handling for both job and AI inference lookups.
87-102: Verify error handling for workflow hash storage.
The change to ObjectId parameter is good, but ensure the workflow hash storage can handle all possible hash formats.
✅ Verification successful
Based on the search results, I can now provide a final response about the workflow hash storage verification:
Workflow hash storage is properly handled with appropriate validation and constraints.
The codebase shows robust handling of workflow hashes:
- Database schema defines workflow hash as VARCHAR(512) with proper foreign key constraints
- Workflow hash storage is validated through
findWorkflowByHashAndOrganizationbefore storage - The hash is properly scoped to organizations through database constraints
- Error handling is implemented for cases where workflow doesn't exist or validation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential workflow hash format variations
# Look for workflow hash handling patterns
ast-grep --pattern 'workflowHash = $value'
Length of output: 44
Script:
#!/bin/bash
# Search for workflow hash handling patterns in a more comprehensive way
rg -A 5 "workflowHash"
# Also check for any validation or sanitization of workflow hashes
rg -A 5 "workflow.*hash|hash.*workflow" -i
# Look for voxelytics DAO implementation to understand hash handling
fd "Voxelytics.*scala|.*voxelytics.*scala" -t f --exec cat {}
Length of output: 65714
app/controllers/MaintenanceController.scala (1)
36-57: LGTM! Type-safe parameter changes
The conversion from String to ObjectId parameters improves type safety and eliminates the need for runtime validation. Error handling is properly maintained.
app/controllers/TaskController.scala (3)
39-44: LGTM! Type-safe task ID parameter
The change to ObjectId improves type safety while maintaining proper error handling for not found cases.
113-125: LGTM! Consistent ObjectId usage
The change to ObjectId is consistently applied across all database operations while maintaining proper error handling and authorization checks.
176-192: LGTM! Verify route parameter updates
The change to ObjectId for both task and user IDs improves type safety. Authorization checks are properly maintained.
✅ Verification successful
Route parameters correctly updated to ObjectId
The route configuration in conf/webknossos.latest.routes correctly matches the method signature, using ObjectId for both id and userId parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify route parameter updates in conf files
rg -A 1 "assignOne\(.*\)" conf/
Length of output: 420
app/controllers/AiModelController.scala (2)
89-97: LGTM! Type-safe AI model ID parameter
The change to ObjectId improves type safety while maintaining proper authorization and error handling.
214-224: LGTM! Consistent ObjectId usage in update operations
The change to ObjectId is consistently applied across find and update operations while maintaining proper error handling.
app/controllers/DatasetController.scala (5)
106-111: LGTM: Proper type safety for dataset ID
The change from String to ObjectId improves type safety and eliminates the need for runtime conversion.
Line range hint 113-125: LGTM: Consistent error handling with ObjectId
The error handling is properly maintained with the new ObjectId type, using proper error messages and status codes.
166-168: LGTM: Consistent parameter type changes
The uploaderId and folderId parameters have been updated to use ObjectId consistently throughout the method.
Also applies to: 190-191, 203-204
243-245: LGTM: Proper error handling in project-related operations
The error messages properly handle ObjectId in string interpolation for not found cases.
Also applies to: 255-258
430-439: Verify error message formatting
The error message uses toString() on ObjectId which might not be the most user-friendly format.
Consider using a more user-friendly format for the error message display.
app/controllers/AnnotationIOController.scala (3)
Line range hint 382-394: LGTM: Proper type handling in download methods
The downloadWithoutType method properly handles ObjectId parameters and maintains consistent error handling.
526-533: LGTM: Proper error handling in project download
The project download functionality correctly handles ObjectId parameters with appropriate error messages.
Line range hint 352-362: Verify analytics event parameter type
The analytics tracking still converts ObjectId to String. Consider if this is necessary or if the analytics service should be updated to handle ObjectId directly.
Consider updating the analytics event to accept ObjectId directly if the analytics service supports it.
✅ Verification successful
Let me check the analytics service implementation to understand if it accepts ObjectId directly.
Converting ObjectId to String is the correct approach here
The toString conversion is necessary and correct because:
- The
DownloadAnnotationEventcase class explicitly requiresannotationIdasStringin its constructor - The analytics event properties are stored as JSON with the annotation ID as a string field:
"annotation_id" -> annotationId - This is a common pattern as analytics systems typically store IDs as strings for better compatibility and readability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar analytics event patterns
rg "DownloadAnnotationEvent.*toString"
Length of output: 203
Script:
#!/bin/bash
# Check analytics service and event implementations
ast-grep --pattern 'case class DownloadAnnotationEvent'
# Also check for any other usages of this event
rg "DownloadAnnotationEvent" -A 2
Length of output: 1794
app/controllers/AuthenticationController.scala (1)
216-224: LGTM: Improved type safety in authentication
The change to use ObjectId for datasetId parameter improves type safety in the authentication flow.
app/controllers/AnnotationController.scala (3)
Line range hint 93-120: LGTM! Type-safe parameter handling
The change from String to ObjectId for the id parameter improves type safety and eliminates the need for string-to-ObjectId conversions, reducing potential runtime errors.
626-635: LGTM! Consistent mutex handling with ObjectId
The implementation correctly uses ObjectId for mutex acquisition while maintaining proper error handling and access restrictions.
Line range hint 831-840: LGTM! Secure annotation transfer implementation
The implementation correctly uses ObjectId while maintaining proper access control checks and error handling.
app/models/annotation/AnnotationService.scala (1)
Line range hint 831-839: LGTM! The signature change aligns with the PR objective.
The change from String to ObjectId for the id parameter improves type safety by eliminating the need for string-to-ObjectId conversion. The method's logic remains sound with proper validation checks.
Let's verify the usage of this method to ensure all callers have been updated:
✅ Verification successful
Found callers still using String-based id parameter
The verification revealed that while the transferAnnotationToUser method signature has been updated to use ObjectId, there are callers that need to be updated:
app/controllers/AnnotationController.scala: Call totransferAnnotationToUser(typ, id, newUserIdValidated, request.identity)app/controllers/ProjectController.scala: Call totransferAnnotationToUser(AnnotationType.Task.toString, id, newUserIdValidated, request.identity)
Both callers appear to be passing the id parameter directly, but we should verify their parameter types to ensure they're using ObjectId.
The method signature change is correctly implemented and used
The verification shows that all callers are properly handling the ObjectId type:
- In
AnnotationController.scala, theidparameter is already of typeObjectId, andnewUserIdValidatedis properly converted from String usingObjectId.fromString - In
ProjectController.scala, theidparameter comes fromannotationDAO.findAllActiveForProjectwhich would return ObjectId types
The change to use ObjectId in the method signature is consistent with the codebase's usage patterns and all callers are properly handling the type conversion where needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any remaining String-based calls to transferAnnotationToUser
# Expect: No matches with String literals or String types as the id parameter
rg -A 2 "transferAnnotationToUser.*String.*id"
# Check the actual calls to this method
ast-grep --pattern 'transferAnnotationToUser($$$)'
Length of output: 657
Script:
#!/bin/bash
# Let's search for calls to transferAnnotationToUser with a broader pattern
rg -A 3 "transferAnnotationToUser\(" --type scala
# Also check for any controller actions that might be calling this method
rg -A 3 "def.*transfer.*annotation" --type scala
Length of output: 1412
Script:
#!/bin/bash
# Check the parameter types in the controller methods
ast-grep --pattern 'def transfer$$$($$$)' app/controllers/AnnotationController.scala
ast-grep --pattern 'def transfer$$$($$$)' app/controllers/ProjectController.scala
Length of output: 170
Script:
#!/bin/bash
# Let's try a more specific pattern to find the controller methods
rg -B 2 -A 5 "transferAnnotationToUser" app/controllers/AnnotationController.scala app/controllers/ProjectController.scala
# Also check the route definitions to understand the parameter types
rg "POST.*transfer.*annotation" conf/routes
Length of output: 1775
| def jobExportProperties(name: String, key: String, jobId: ObjectId): Action[AnyContent] = Action.async { | ||
| implicit request => | ||
| dataStoreService.validateAccess(name, key) { _ => | ||
| for { | ||
| jobIdValidated <- ObjectId.fromString(jobId) | ||
| job <- jobDAO.findOne(jobIdValidated)(GlobalAccessContext) | ||
| job <- jobDAO.findOne(jobId)(GlobalAccessContext) | ||
| jobOwner <- userDAO.findOne(job._owner)(GlobalAccessContext) | ||
| organization <- organizationDAO.findOne(jobOwner._organization)(GlobalAccessContext) | ||
| latestRunId <- job.latestRunId.toFox ?~> "job.notRun" | ||
| exportFileName <- job.exportFileName.toFox ?~> "job.noExportFileName" | ||
| jobExportProperties = JobExportProperties(jobId, latestRunId, organization._id, exportFileName) | ||
| jobExportProperties = JobExportProperties(jobId.toString, latestRunId, organization._id, exportFileName) | ||
| } yield Ok(Json.toJson(jobExportProperties)) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Route definition needs to be synchronized with client usage
The verification revealed an inconsistency:
- The route definition correctly uses
ObjectIdtype forjobIdparameter - However, the client code in
DSRemoteWebknossosClient.scalastill usesStringtype forjobId
This needs to be addressed to maintain type consistency across the codebase:
webknossos-datastore/.../DSRemoteWebknossosClient.scala: UpdategetJobExportPropertiesmethod to useObjectIdinstead ofString
🔗 Analysis chain
LGTM! Verify route definition updates
The change from String to ObjectId parameter type is correct and aligns with the PR objectives. The necessary toString conversion is maintained when creating JobExportProperties.
Let's verify that the corresponding route definition has been updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for route definitions referencing jobExportProperties
rg "jobExportProperties" conf/routes
Length of output: 36
Script:
#!/bin/bash
# Let's try a broader search for routes file and jobExportProperties
fd "routes" --type f
rg -l "jobExportProperties" .
Length of output: 652
Script:
#!/bin/bash
# Let's check the route definition in webknossos.latest.routes
rg "jobExportProperties" conf/webknossos.latest.routes -A 1 -B 1
# Also check the client usage to ensure parameter type consistency
rg "jobExportProperties" webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSRemoteWebknossosClient.scala -A 2 -B 2
Length of output: 1017
| @@ -164,21 +164,19 @@ class ReportController @Inject()(reportDAO: ReportDAO, | |||
| extends Controller | |||
| with FoxImplicits { | |||
|
|
|||
| def projectProgressReport(teamId: String): Action[AnyContent] = sil.SecuredAction.async { implicit request => | |||
| def projectProgressReport(teamId: ObjectId): Action[AnyContent] = sil.SecuredAction.async { implicit request => | |||
There was a problem hiding this comment.
💡 Codebase verification
Route definition is updated but TypeScript interface needs attention
The route definition correctly uses ObjectId type parameter, but the TypeScript interface in admin_rest_api.ts still uses string type for teamId. This mismatch should be addressed:
frontend/javascripts/admin/admin_rest_api.ts: UpdateteamIdparameter type fromstringto match the backend'sObjectIdtype
🔗 Analysis chain
LGTM! Verify route definition update.
The change from String to ObjectId parameter improves type safety by moving validation to compile-time. The implementation correctly removes the unnecessary ObjectId.fromString conversion.
Also applies to: 169-170
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the route definition has been updated to match the new signature
# Expected: The route should use ObjectId parameter type
rg "projectProgressReport" conf/routes
Length of output: 38
Script:
#!/bin/bash
# Let's try to find the routes file with a broader search
fd "routes$"
# Also search for any file containing projectProgressReport to understand the routing
rg -l "projectProgressReport"
Length of output: 577
Script:
#!/bin/bash
# Let's check the route definition in the main routes file
cat conf/webknossos.latest.routes | rg "projectProgressReport"
# Also check if there are any TypeScript interfaces that need to be updated
cat frontend/javascripts/admin/admin_rest_api.ts | rg -A 5 -B 5 "projectProgressReport"
Length of output: 643
| def update(scriptId: ObjectId): Action[JsValue] = sil.SecuredAction.async(parse.json) { implicit request => | ||
| withJsonBodyUsing(scriptPublicReads) { scriptFromForm => | ||
| for { | ||
| scriptIdValidated <- ObjectId.fromString(scriptId) | ||
| oldScript <- scriptDAO.findOne(scriptIdValidated) ?~> "script.notFound" ~> NOT_FOUND | ||
| oldScript <- scriptDAO.findOne(scriptId) ?~> "script.notFound" ~> NOT_FOUND |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for ScriptController update functionality
Based on the codebase analysis, there appears to be no dedicated test suite for the ScriptController, particularly for the update functionality. The type change from String to ObjectId warrants test coverage for:
- Invalid ObjectId formats
- Ownership validation
- Concurrent update scenarios
Locations requiring test coverage:
test/controllers/ScriptControllerSpec.scala(needs to be created)- Test cases should cover the update endpoint in
app/controllers/ScriptController.scala
🔗 Analysis chain
LGTM! Consider adding edge case tests.
The update method's conversion to ObjectId is well-implemented while maintaining all necessary validations and error handling. Since this is a significant type change, consider adding test cases for:
- Invalid ObjectId formats in requests
- Concurrent update scenarios
- Script ownership edge cases
Let's check for existing test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for existing test cases for script updates
rg -A 5 "\"update.*script.*" test/
Length of output: 35
Script:
#!/bin/bash
# Let's try a broader search for test files related to scripts and updates
fd ".*[Ss]cript.*[Tt]est.*" test/
# Also search for any test files containing "update" case-insensitive
rg -i "update.*script" test/ -l
# Look for any spec files that might contain script tests
fd ".*[Ss]cript.*[Ss]pec.*" test/
# Search for test files containing scriptId
rg "scriptId" test/ -l
Length of output: 128
Script:
#!/bin/bash
# Let's try a different approach to find test files
# First, let's find the test directory structure
fd --type d "test"
# Then look for all test files in the project
fd -e scala -e java ".*Test.*"
# Also check for any references to ScriptController in test files
rg "ScriptController" -l
Length of output: 1206
| def update(id: ObjectId): Action[AnnotationPrivateLinkParams] = | ||
| sil.SecuredAction.async(validateJson[AnnotationPrivateLinkParams]) { implicit request => | ||
| val params = request.body | ||
| for { | ||
| annotationId <- ObjectId.fromString(params.annotation) | ||
| idValidated <- ObjectId.fromString(id) | ||
| aPLInfo <- annotationPrivateLinkDAO.findOne(idValidated) ?~> "annotation private link not found" ~> NOT_FOUND | ||
| aPLInfo <- annotationPrivateLinkDAO.findOne(id) ?~> "annotation private link not found" ~> NOT_FOUND | ||
| _ <- annotationDAO.assertUpdateAccess(aPLInfo._annotation) ?~> "notAllowed" ~> FORBIDDEN | ||
| _ <- annotationDAO.assertUpdateAccess(annotationId) ?~> "notAllowed" ~> FORBIDDEN | ||
| _ <- annotationPrivateLinkDAO.updateOne(idValidated, annotationId, params.expirationDateTime) ?~> "update.failed" | ||
| updated <- annotationPrivateLinkDAO.findOne(idValidated) ?~> "not Found" | ||
| _ <- annotationPrivateLinkDAO.updateOne(id, annotationId, params.expirationDateTime) ?~> "update.failed" | ||
| updated <- annotationPrivateLinkDAO.findOne(id) ?~> "not Found" | ||
| js <- annotationPrivateLinkService.publicWrites(updated) ?~> "write failed" | ||
| } yield Ok(js) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider converting annotation parameter to ObjectId in AnnotationPrivateLinkParams.
While the id parameter has been updated to ObjectId, the method still converts params.annotation from String to ObjectId. Consider updating AnnotationPrivateLinkParams to use ObjectId directly for consistency.
- annotationId <- ObjectId.fromString(params.annotation)
+ annotationId = params.annotation // After updating AnnotationPrivateLinkParamsCommittable suggestion skipped: line range outside the PR's diff.
| jsValue | ||
| .asOpt[String] | ||
| .toFox | ||
| .flatMap(id => finishAnnotation(typ, ObjectId(id), request.identity, Instant(timestamp))) |
There was a problem hiding this comment.
🛠️ 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.
|
There seems to be a problem as seen in e2e tests |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
util/src/main/scala/com/scalableminds/util/objectid/ObjectId.scala (3)
38-44: Consider enhancing error messages for better debuggingThe implementation is correct and aligns with the PR objectives, but the error message could be more helpful for debugging.
Consider this enhancement:
override def bind(key: String, value: String): Either[String, ObjectId] = - fromStringSync(value).toRight(s"Cannot parse parameter $key as ObjectId: $value") + fromStringSync(value).toRight( + s"Invalid ObjectId format for parameter '$key'. Value '$value' must be a 24-character hex string." + )
46-54: Consider handling edge cases in query parameter bindingWhile the implementation is functionally correct, there are some edge cases that could be handled more robustly:
Consider these improvements:
override def bind(key: String, params: Map[String, Seq[String]]): Option[Either[String, ObjectId]] = params.get(key).flatMap(_.headOption).map { value => - fromStringSync(value).toRight(s"Cannot parse parameter $key as ObjectId: $value") + if (value.trim.isEmpty) { + Left(s"Empty value provided for ObjectId parameter '$key'") + } else { + fromStringSync(value).toRight( + s"Invalid ObjectId format for parameter '$key'. Value '$value' must be a 24-character hex string." + ) + } }Additionally, consider adding a comment explaining why we only use the first value when multiple values are provided for the same parameter.
38-54: Consider documenting the error handling strategyThis architectural change moves ObjectId validation to the routing layer, which is a good practice for type safety. However, this shift might affect how errors are reported and logged throughout the application.
Consider:
- Documenting the error handling strategy in the README or wiki
- Ensuring consistent error reporting across the application
- Adding logging for failed ObjectId parsing attempts to help with debugging
- Creating integration tests that verify error handling for invalid ObjectIds in both path and query parameters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
util/src/main/scala/com/scalableminds/util/objectid/ObjectId.scala(2 hunks)
🔇 Additional comments (1)
util/src/main/scala/com/scalableminds/util/objectid/ObjectId.scala (1)
6-6: LGTM: Import statement is correctly placed
The import statement for Play Framework's binding traits is appropriately placed and necessary for the new implementations.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
Updated changelog