Add missing isValidNewName legacy route #8252
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several updates to the WEBKNOSSOS application, including a new feature for pasting remote URIs from Neuroglancer without format prefixes. It enhances the asynchronous image file reading process and allows for dataset renaming with potential duplicates. Improvements in error messaging and registration processes are implemented, along with the removal of bounding box restrictions for super users. Additionally, logging has been improved, and multiple bug fixes have been applied. Support for certain HTTP API versions has been removed, and a new method for validating dataset names has been added. Changes
Possibly related PRs
Suggested labels
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: 2
🧹 Outside diff range and nitpick comments (3)
CHANGELOG.unreleased.md (1)
31-31: Consider adding more context to the changelog entry.The entry correctly documents the change, but could be more informative for users migrating between versions.
Consider expanding the entry to provide more context:
-Added missing legacy support for `isValidNewName` route. [#8252](https://github.com/scalableminds/webknossos/pull/8252) +Added missing legacy support for `isValidNewName` route for dataset name validation (supports pre-v8 API calls). [#8252](https://github.com/scalableminds/webknossos/pull/8252)app/controllers/LegacyApiController.scala (2)
62-68: Architectural consistency check neededThe implementation pattern differs across versions v5-v8:
- v5 (
assertValidNewNameV5): Direct validation with organization check- v8 (
isValidNewNameV8): Delegates todatasetControllerConsider:
- Document why the implementation differs between versions
- Add deprecation notice for v5 if it's being replaced
- Ensure consistent error responses across versions
62-68: Add logging for validation failuresWhile the method logs the request via
logVersioned, it doesn't log validation failures which could be useful for monitoring and debugging.def isValidNewNameV8(datasetName: String, organizationId: String): Action[AnyContent] = sil.SecuredAction.async { implicit request => for { _ <- Fox.successful(logVersioned(request)) - result <- datasetController.isValidNewName(datasetName)(request) + result <- datasetController.isValidNewName(datasetName)(request).recoverWith { + case e => + logger.warn(s"Dataset name validation failed: ${e.getMessage}") + Fox.failure(e) + } } yield result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)app/controllers/LegacyApiController.scala(1 hunks)conf/webknossos.versioned.routes(4 hunks)
🔇 Additional comments (4)
CHANGELOG.unreleased.md (1)
Line range hint 1-42: LGTM! Changelog structure is well-maintained.
The changelog properly documents both the addition of the legacy route and its relationship to the removal of older API versions. The entries are well-organized and provide good traceability through PR references.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: Possible missing comma found.
Context: ... the datasource setting of the advanced tab making the form not submittable. [#8245...
(AI_HYDRA_LEO_MISSING_COMMA)
conf/webknossos.versioned.routes (2)
23-23: LGTM! Consistent implementation of isValidNewName routes.
The new routes for versions v6-v8 are consistently implemented with the same path pattern and controller method.
Let's verify that the controller methods exist:
Also applies to: 37-37, 55-55, 75-75
✅ Verification successful
Both controller methods exist and match the route configuration
The verification confirms that both controller methods are present in LegacyApiController.scala:
isValidNewNameV8(datasetName: String, organizationId: String)- matches routes for v6-v8assertValidNewNameV5(organizationName: String, datasetName: String)- matches v5 route with different parameter naming
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the controller methods exist in the LegacyApiController
# Search for the controller methods
rg "def (isValidNewNameV8|assertValidNewNameV5)" -t scala
Length of output: 355
75-75: Verify the impact of different controller methods in v5.
The v5 routes use different controller methods for the same functionality:
- New route uses
isValidNewNameV8 - Existing route uses
assertValidNewNameV5
This might lead to inconsistent behavior within v5.
Let's verify the differences between these methods:
Also applies to: 89-89
app/controllers/LegacyApiController.scala (1)
62-68:
Verify organization ID validation
The method delegates to datasetController.isValidNewName but doesn't validate the organizationId parameter. This differs from the v5 implementation (assertValidNewNameV5) which explicitly checks organization access.
Consider adding organization validation similar to v5:
def isValidNewNameV8(datasetName: String, organizationId: String): Action[AnyContent] = sil.SecuredAction.async {
implicit request =>
for {
_ <- Fox.successful(logVersioned(request))
+ organization <- organizationDAO.findOne(organizationId)
+ _ <- bool2Fox(organization._id == request.identity._organization) ~> FORBIDDEN
result <- datasetController.isValidNewName(datasetName)(request)
} yield result
}| @@ -69,6 +72,7 @@ GET /v6/datasets/:organizationName/:datasetName co | |||
| PATCH /v5/datasets/:organizationId/:datasetName controllers.LegacyApiController.updateDatasetV8(organizationId: String, datasetName: String) | |||
| GET /v5/datasets/:organizationId/:datasetName/sharingToken controllers.LegacyApiController.getDatasetSharingTokenV8(organizationId: String, datasetName: String) | |||
| PATCH /v5/datasets/:organizationId/:datasetName/teams controllers.LegacyApiController.updateDatasetTeamsV8(organizationId: String, datasetName: String) | |||
| GET /v5/datasets/:organizationId/:datasetName/isValidNewName controllers.LegacyApiController.isValidNewNameV8(datasetName: String, organizationId: String) | |||
There was a problem hiding this comment.
Inconsistent parameter naming between v5 routes.
There's an inconsistency in parameter naming between the new v5 route and the existing v5 route:
- New route (L75): Uses
organizationId - Existing route (L89): Uses
organizationName
This might cause confusion for API consumers using v5.
Consider using consistent parameter naming within the same API version. Either:
- Update the new v5 route to use
organizationNameto match the existing pattern:
-GET /v5/datasets/:organizationId/:datasetName/isValidNewName controllers.LegacyApiController.isValidNewNameV8(datasetName: String, organizationId: String)
+GET /v5/datasets/:organizationName/:datasetName/isValidNewName controllers.LegacyApiController.isValidNewNameV8(datasetName: String, organizationName: String)- Or document the difference in the version changelog if this is intentional.
Also applies to: 89-89
| def isValidNewNameV8(datasetName: String, organizationId: String): Action[AnyContent] = sil.SecuredAction.async { | ||
| implicit request => | ||
| for { | ||
| _ <- Fox.successful(logVersioned(request)) | ||
| result <- datasetController.isValidNewName(datasetName)(request) | ||
| } yield result | ||
| } |
There was a problem hiding this comment.
Missing implementations for v5-v7
The PR objective mentions testing via v7 endpoint (/api/v7/datasets/<some orga>/TestThisName/isValidNewName), but only v8 implementation is present. Implementations for v6 and v7 appear to be missing.
Add implementations for v6 and v7:
def isValidNewNameV7(datasetName: String, organizationId: String): Action[AnyContent] =
isValidNewNameV8(datasetName, organizationId)
def isValidNewNameV6(datasetName: String, organizationId: String): Action[AnyContent] =
isValidNewNameV8(datasetName, organizationId)
MichaelBuessemeyer
deleted the
add-missing-is-valid-new-name-legacy-route
branch
This PR add the missing legacy route for
isValidNewName. This was forgotten in #8075.URL of deployed dev instance (used for testing):
Steps to test:
{ isValid: true }Issues:
(Please delete unneeded items, merge only when none are left open)