Fix nml upload with only ds name in nml by using user's orga as fallback #8277
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing the handling of organization-specific data within the file upload processes in the application. Key modifications include the introduction of a new parameter, Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (4)
app/controllers/TaskController.scala (1)
92-94: Consider adding validation for userOrganizationIdWhile the implementation is correct, consider adding a validation check to ensure the organization ID is not empty.
userOrganizationId = request.identity._organization +_ <- Fox.assertTrue(userOrganizationId.nonEmpty) ?~> "organization.id.missing" extractedFiles <- nmlService.extractFromFiles( inputFiles.map(f => (f.ref.path.toFile, f.filename)), SharedParsingParameters(useZipName = false, isTaskUpload = true, userOrganizationId = userOrganizationId))app/models/annotation/nml/NmlParser.scala (1)
383-386: LGTM: Proper fallback implementation for organization IDThe implementation correctly uses the user's organization as a fallback when the organization is not specified in the NML file.
However, consider adding logging to track when fallback is used:
private def parseOrganizationId(nodes: NodeSeq, fallbackOrganization: String): String = { - nodes.headOption - .map(node => getSingleAttributeOpt(node, "organization").getOrElse(fallbackOrganization)) - .getOrElse(fallbackOrganization) + val parsedOrg = nodes.headOption + .flatMap(node => getSingleAttributeOpt(node, "organization")) + if (parsedOrg.isEmpty) { + logger.debug(s"No organization found in NML, using fallback organization: $fallbackOrganization") + } + parsedOrg.getOrElse(fallbackOrganization) +}app/controllers/AnnotationIOController.scala (2)
115-116: Consider enhancing error message specificityWhile the error handling is robust, consider making the error message more specific about why the parsing failed (e.g., "No valid files were found in the upload" instead of a generic failure message).
- _ <- bool2Fox(parseResultsFiltered.nonEmpty) orElse returnError(parsedFiles) + _ <- bool2Fox(parseResultsFiltered.nonEmpty) orElse { + Fox.failure("No valid files were found in the upload. Please ensure the files are in the correct format.") + } orElse returnError(parsedFiles)
111-114: Consider parallel processing for multiple filesThe current implementation processes files sequentially. For better performance with multiple file uploads, consider using parallel processing where possible.
- parsedFiles <- annotationUploadService.extractFromFiles( + parsedFiles <- Future.traverse(attachedFiles) { file => + annotationUploadService.extractFromFiles( + List(file), + SharedParsingParameters(useZipName = true, overwritingDatasetId, userOrganizationId)) + }.map(_.flatten)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/AnnotationIOController.scala(1 hunks)app/controllers/TaskController.scala(1 hunks)app/models/annotation/AnnotationUploadService.scala(2 hunks)app/models/annotation/nml/NmlParser.scala(5 hunks)
🔇 Additional comments (7)
app/models/annotation/AnnotationUploadService.scala (2)
29-31: LGTM: Addition of userOrganizationId parameter
The addition of the mandatory userOrganizationId field to SharedParsingParameters is well-aligned with the PR objective of providing a fallback organization ID when parsing NML files.
56-58: LGTM: Improved method signature
Refactoring to use the SharedParsingParameters object instead of individual parameters improves maintainability and makes the code more extensible.
app/controllers/TaskController.scala (1)
85-85: LGTM: Correctly extracting user's organization ID
The organization ID is properly extracted from the request identity.
app/models/annotation/nml/NmlParser.scala (2)
51-58: LGTM: Updated parse method signature
The method signature has been updated to use SharedParsingParameters, maintaining consistency with the changes in AnnotationUploadService.
145-148: LGTM: Correct precedence handling for datasetId
The implementation correctly prioritizes the overwriting dataset ID over the one parsed from the NML file.
app/controllers/AnnotationIOController.scala (2)
108-108: LGTM: User organization fallback implementation
The addition of userOrganizationId and its integration with extractFromFiles aligns well with the PR objective of using the user's organization as a fallback mechanism.
Also applies to: 111-113
109-110: Verify upload size limits
While the access control is robust, consider adding explicit file size validation to prevent potential DoS attacks through large file uploads.
| sharedParsingParameters.overwritingDatasetId, | ||
| sharedParsingParameters.isTaskUpload, |
There was a problem hiding this comment.
all members of sharedParsingParameters are passed to nmlParser.parse anyway. So just pass the whole sharedParsingParameters object
| .map(node => getSingleAttributeOpt(node, "organization").getOrElse(fallbackOrganization)) | ||
| .getOrElse(fallbackOrganization) |
There was a problem hiding this comment.
in both cases
- the "experiment" tag does not exist
- in case the experiment tag has no "organization" field
the fallback organization (user orga) is used.
| nodes.headOption.map(node => getSingleAttribute(node, "organization")).getOrElse("") | ||
| private def parseOrganizationId(nodes: NodeSeq, fallbackOrganization: String): String = | ||
| nodes.headOption | ||
| .map(node => getSingleAttributeOpt(node, "organization").getOrElse(fallbackOrganization)) |
There was a problem hiding this comment.
| .map(node => getSingleAttributeOpt(node, "organization").getOrElse(fallbackOrganization)) | |
| .flatMap(node => getSingleAttributeOpt(node, "organization")) |
would a flstMap work here?
There was a problem hiding this comment.
Yeah, good suggestion. Should work 🎉
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/models/annotation/nml/NmlParser.scala (1)
156-163: Consider improving readability of the bounding box handling.While the logic is correct, consider these improvements:
- Use a more descriptive variable name than
asUserBoundingBox- Simplify the nested condition structure
Consider this refactoring:
- if (!sharedParsingParameters.isTaskUpload) { - parseTaskBoundingBoxAsUserBoundingBox(parameters \ "taskBoundingBox", userBoundingBoxes) - .map(asUserBoundingBox => userBoundingBoxes :+ asUserBoundingBox) - } + val convertedBoundingBox = if (!sharedParsingParameters.isTaskUpload) { + parseTaskBoundingBoxAsUserBoundingBox(parameters \ "taskBoundingBox", userBoundingBoxes) + .map(convertedBox => userBoundingBoxes :+ convertedBox) + } else userBoundingBoxes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/annotation/nml/NmlParser.scala(5 hunks)
🔇 Additional comments (3)
app/models/annotation/nml/NmlParser.scala (3)
51-53: LGTM! Good refactoring of method parameters.
The consolidation of parameters into SharedParsingParameters improves code maintainability and readability.
383-384: LGTM! Correct implementation of organization fallback.
The implementation properly handles the fallback to user's organization when no organization is specified in the NML file, which directly addresses the PR objective.
123-126: LGTM! Well-structured error handling and parameter parsing.
The changes maintain the robust error handling pattern while integrating the new SharedParsingParameters structure seamlessly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/backend/NMLUnitTestSuite.scala(2 hunks)
🔇 Additional comments (1)
test/backend/NMLUnitTestSuite.scala (1)
11-11: LGTM!
The import statement is correctly placed and necessary for the updated parameter handling.
| SharedParsingParameters(useZipName = false, | ||
| overwritingDatasetId = None, | ||
| userOrganizationId = "testOrganization", | ||
| isTaskUpload = true), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for organization fallback behavior.
While the parameter changes look good, considering the PR's objective to "Fix nml upload with only ds name in nml by using user's orga as fallback", we should add test cases to verify this specific behavior.
Consider adding test cases that:
- Verify successful parsing when only dataset name is provided (without datasetId)
- Confirm the fallback to user's organization works as expected
- Test edge cases like missing or invalid organization values
Example test case structure:
"NML Parser" should {
"use user's organization as fallback when only dataset name is provided" in {
// Setup tracing without datasetId
val tracingWithoutDatasetId = dummyTracing.copy(/* setup without datasetId */)
writeAndParseTracing(tracingWithoutDatasetId) match {
case Full(NmlParseSuccessWithoutFile(Some(tracing), _, _, _, _)) =>
// Verify organization fallback worked
assert(/* verify organization fallback */)
case _ => fail("Should succeed with organization fallback")
}
}
}There was a problem hiding this comment.
@fm3 Writing such a test is rather difficult as the nmls for the tests are generated with NmlWriter().toNmlStream which requires an organization id. This would mean to rewrite toNmlStream which is not preferable as one should always supply the toNmlStream with an organization. But writing a copy of toNmlSteam that supports an optional organizationId is also not preferable :/
=> I skipped this for now but it should be useful for the future to avoid this regression bug introduced by #8075
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
what are your thoughts on this?
There was a problem hiding this comment.
I’d say this is part of the broader problem of testing the libs and webknossos together. That’s a topic we’ll definitely discuss next year.
This PR fixes uploading an nml with only setting a dataset name in the nml and not passing the optional
datasetIdto the route. Wklibs currently behaves like this. And therefore this should be fixed.URL of deployed dev instance (used for testing):
Steps to test:
datasetId. The upload should succeedtest_annotation.zip
Issues:
(Please delete unneeded items, merge only when none are left open)