Improve error messages for starting jobs on other orgs datasets #8181
Conversation
WalkthroughThe pull request introduces significant enhancements to the WEBKNOSSOS application, primarily focusing on error handling and validation improvements for job processing. Key changes include the implementation of asynchronous reading of image files, enhanced error messages related to job initiation on datasets from other organizations, and the addition of an Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Outside diff range and nitpick comments (7)
CHANGELOG.unreleased.md (1)
30-30: Enhance the changelog entry with more details about the error message improvements.While the entry correctly documents the change, it could be more descriptive to help users understand the improvements made. Consider expanding it to better reflect the scope of changes:
-- Improved error messages for starting jobs on datasets from other organizations. [#8181](https://github.com/scalableminds/webknossos/pull/8181) +- Improved error messages when starting jobs (e.g., render_animation, nuclei inferral) on datasets from other organizations. Users now receive clear feedback about organization access restrictions instead of generic "organization not found" errors. [#8181](https://github.com/scalableminds/webknossos/pull/8181)app/controllers/AiModelController.scala (1)
178-180: Improve code formatting for readabilityThe multi-line Messages call could be more concise:
- dataset <- datasetDAO.findOneByNameAndOrganization(request.body.datasetName, organization._id) ?~> Messages( - "dataset.notFound", - request.body.datasetName) + dataset <- datasetDAO.findOneByNameAndOrganization(request.body.datasetName, organization._id) ?~> + Messages("dataset.notFound", request.body.datasetName)conf/messages (1)
336-336: Consider providing more specific guidance about invalid additional coordinates.The current message
job.additionalCoordinates.invalidcould be more helpful by indicating what makes the coordinates invalid (e.g., format, range, missing required values).Consider using a more specific message like:
-job.additionalCoordinates.invalid = The passed additional coordinates are invalid. +job.additionalCoordinates.invalid = The passed additional coordinates are invalid. Expected format: <expected_format> with values in range <valid_range>.app/controllers/JobController.scala (4)
127-127: Consider refactoring organization validation into a helper functionThe logic for validating the user's organization is duplicated across multiple methods. Extracting this into a helper function would reduce code duplication and improve maintainability.
449-449: Ensure consistent access context when retrieving user organizationThe retrieval of
userOrganizationdoes not useGlobalAccessContext. Verify that access controls are correctly handled and consistent with other organization lookups.
Line range hint
449-456: Consider refactoring pricing plan checks into a helper functionThe pricing plan validations are specific to
PricingPlan.Basic. Refactoring these checks into a dedicated method would improve readability and maintainability.
Line range hint
127-449: Consolidate organization access and validation logicThe repeated pattern of retrieving and validating organization access could be consolidated into a shared helper function to reduce duplication and enhance code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)app/controllers/AiModelController.scala(3 hunks)app/controllers/JobController.scala(6 hunks)conf/messages(4 hunks)
🔇 Additional comments (11)
app/controllers/AiModelController.scala (2)
21-21: LGTM: Messages import for localization
The addition of Messages import enables proper localization of error messages, which aligns with the PR objective of improving error message clarity.
139-139: LGTM: Organization validation for training jobs
The organization validation check ensures users can only train models on datasets from their own organization, with an appropriate FORBIDDEN response and localized error message.
Let's verify the error message is properly defined:
✅ Verification successful
Verified: Organization validation and error message defined properly
The organization validation check is correctly implemented, and the associated error message is properly defined in the messages configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the error message key is defined in the messages file
rg "job\.trainModel\.notAllowed\.organization\s*=" conf/
Length of output: 187
conf/messages (3)
141-141: LGTM! Clear and consistent error message.
The new zarr error message follows the existing pattern and clearly communicates the requirement.
153-154: LGTM! Clear and consistent error messages.
The new NML error messages follow the existing pattern and use placeholders consistently.
Line range hint 324-335: LGTM! Clear and consistent organization restriction messages.
The new error messages for job operations follow a consistent pattern and clearly communicate that these operations are restricted to datasets within the user's organization. This directly addresses the PR objectives by replacing vague error messages with clear, actionable feedback.
app/controllers/JobController.scala (6)
266-268: Duplicate comment on GlobalAccessContext usage
Refer to the earlier comment about verifying the use of GlobalAccessContext in organization lookups.
300-302: Duplicate comment on GlobalAccessContext usage
As noted previously, please verify that using GlobalAccessContext here is secure and appropriate.
421-423: Duplicate comment on GlobalAccessContext usage
Please see the prior comments regarding the verification of GlobalAccessContext usage.
445-447: Duplicate comment on GlobalAccessContext usage
Again, ensure that the use of GlobalAccessContext does not introduce security risks.
Line range hint 266-272: Ensure inferMitochondria job restriction aligns with permissions model
Confirm that limiting the inferMitochondria job to superusers is consistent with business requirements. Ensure that appropriate error messages are provided to users who are not superusers.
233-235:
Verify the use of GlobalAccessContext in organization lookup
Using GlobalAccessContext in organizationDAO.findOne(organizationId)(GlobalAccessContext) bypasses default access controls. Ensure this is intentional and does not expose organization data to unauthorized users. If necessary, consider using a more restrictive context or adding access checks.
| dataset <- datasetDAO.findOneByNameAndOrganization(request.body.datasetName, organization._id) ?~> Messages( | ||
| "dataset.notFound", | ||
| request.body.datasetName) |
There was a problem hiding this comment.
Add explicit organization validation for inference jobs
While the code validates the dataset belongs to the organization during lookup, it should have an explicit organization check similar to runTraining for consistency. This aligns with the PR objective of standardizing organization access checks.
dataset <- datasetDAO.findOneByNameAndOrganization(request.body.datasetName, organization._id) ?~>
Messages("dataset.notFound", request.body.datasetName)
+ _ <- bool2Fox(request.identity._organization == dataset._organization) ?~> "job.inferWithModel.notAllowed.organization" ~> FORBIDDEN
dataStore <- dataStoreDAO.findOneByName(dataset._dataStore) ?~> "dataStore.notFound"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dataset <- datasetDAO.findOneByNameAndOrganization(request.body.datasetName, organization._id) ?~> Messages( | |
| "dataset.notFound", | |
| request.body.datasetName) | |
| dataset <- datasetDAO.findOneByNameAndOrganization(request.body.datasetName, organization._id) ?~> Messages( | |
| "dataset.notFound", | |
| request.body.datasetName) | |
| _ <- bool2Fox(request.identity._organization == dataset._organization) ?~> "job.inferWithModel.notAllowed.organization" ~> FORBIDDEN |
There was a problem hiding this comment.
Looking good!
As I understand the runInference Job, it is not necessary there because the organization is taken from the user's identity anyway.
Yes, not sure why, to be honest. I think it would be better to change the RunInferenceParameters to include the organizationId. Could you maybe do this here? Not sure how simple adapting the frontend is, but I would hope it can be done the same way as for runTraining?
Also, could you write an issue detailing that the renderAnimation modal does not show up, as you mentioned in person? I think that there, too, we should have either an explanation or the menu item shouldn’t be there in the first place.
conf/messages
Outdated
| job.additionalCoordinates.invalid = "The passed additional coordinates are invalid." | ||
| job.renderAnimation.notAllowed.organization = Rendering animations is only allowed for datasets of your own organization. | ||
| job.alignSections.notAllowed.organization = Aligning sections is only allowed for datasets of your own organization. | ||
| job.alignSections.notAllowed.onlySuperUsers = For now, aligning sections is only allowed for super users. |
There was a problem hiding this comment.
I think this one can go, we lifted that restriction
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
frontend/javascripts/admin/api/jobs.ts (1)
Line range hint
357-357: Remove commented-out parameterThe commented-out
maskAnnotationLayerNameparameter should be removed. If this is a planned feature, it should be tracked in the issue tracker instead.frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (1)
Line range hint
574-583: Enhance error messages for organization-related failures.The current error message is generic and doesn't specifically indicate when a job fails due to organization access issues. Consider making the error message more informative.
Toast.error( - `The ${jobName} job could not be started. Please contact an administrator or look in the console for more details.`, + `The ${jobName} job could not be started. ${ + error.message?.includes("organization") + ? "You don't have permission to start jobs on datasets from other organizations." + : "Please contact an administrator or look in the console for more details." + }`, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app/controllers/AiModelController.scala(5 hunks)conf/messages(4 hunks)frontend/javascripts/admin/api/jobs.ts(1 hunks)frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx(1 hunks)
🔇 Additional comments (10)
frontend/javascripts/admin/api/jobs.ts (2)
Line range hint 359-363: LGTM: Proper implementation of runInferenceJob
The function correctly handles the new organizationId parameter and follows the established patterns for job functions in the codebase.
351-351: LGTM: Addition of organizationId parameter aligns with PR objectives
The addition of the required organizationId field to RunInferenceParameters is consistent with other job functions in the file and supports the PR's goal of improving organization-related error handling.
Let's verify that all job-related functions consistently handle organization IDs:
app/controllers/AiModelController.scala (3)
22-22: LGTM: Messages import for i18n support
The addition of Messages import enables localized error messages, which aligns with the PR objective of improving error message clarity.
141-141: LGTM: Added organization validation for training jobs
The added check ensures users can only train models on datasets from their own organization, with a clear FORBIDDEN response when validation fails.
179-182: LGTM: Enhanced organization validation for inference jobs
The changes implement proper organization validation with clear error messages:
- Explicit organization lookup with user-friendly error message
- Organization access check with FORBIDDEN response
conf/messages (3)
141-141: LGTM: Clear and consistent error message
The new zarr error message follows the established pattern and clearly communicates the validation requirement.
153-154: LGTM: Well-structured NML validation messages
The new NML error messages maintain consistency with existing messages and provide clear validation feedback.
Line range hint 324-337: LGTM: Comprehensive and consistent job permission messages
The new error messages successfully address the PR objectives by:
- Providing clear, standardized messaging across all job types
- Explicitly stating the organization ownership requirement
- Maintaining consistent wording throughout
This implementation effectively resolves the confusion mentioned in issue #8165 regarding misleading error messages when starting jobs for datasets from other organizations.
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (2)
832-834: LGTM: Consistent implementation across all job API calls.
The implementation of organizationId is consistent with other job API calls in the file, which use dataset.owningOrganization. This ensures uniform error handling for jobs started on datasets from other organizations.
832-832: Good addition of organizationId parameter.
The addition of organizationId parameter to runInferenceJob aligns with the PR objectives to improve error handling for jobs started on datasets from other organizations.
Let's verify if other job API calls in the file also include the organizationId parameter:
✅ Verification successful
All job API calls include the organizationId parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other job API calls include organizationId parameter
# Test: Search for job API calls. Expect: All job API calls should include organizationId.
rg -A 5 "start.*Job|runInference" --type ts
Length of output: 23824
| @@ -41,6 +43,7 @@ object RunTrainingParameters { | |||
| case class RunInferenceParameters(annotationId: Option[ObjectId], | |||
| aiModelId: ObjectId, | |||
| datasetName: String, | |||
| organizationId: String, | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue: organizationId parameter not passed in runInferenceJob calls
organizationIdis not included in anyrunInferenceJobfunction calls in the frontend code, potentially leading to missing organization validation.
🔗 Analysis chain
LGTM: Added organizationId parameter for explicit organization validation
The addition of organizationId parameter enables proper organization validation before running inference jobs. This aligns with the PR objective of improving error handling for cross-organization access attempts.
Let's verify that the frontend is updated to pass this new parameter:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RunInferenceParameters usage in frontend code
rg -t ts -t js "RunInferenceParameters|runInference.*organizationId"
Length of output: 246
Script:
#!/bin/bash
# Search for runInferenceJob calls with organizationId parameter
rg -t ts -t js 'runInferenceJob\s*\([^)]*organizationId'
Length of output: 58
Script:
#!/bin/bash
# List all runInferenceJob calls with their arguments for manual inspection
rg -t ts -t js 'runInferenceJob\s*\(([^)]*)\)' -A 2
Length of output: 336
URL of deployed dev instance (used for testing):
Steps to test:
As I understand the runInference Job, it is not necessary there because the organization is taken from the user's identity anyway.Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Release Notes
New Features
organizationIdto job parameters, enhancing context in job processing.Bug Fixes
Improvements