Make AI models cross orga shareable #8418
Conversation
📝 WalkthroughWalkthroughThe changes implement support for AI model sharing across organizations. Several migration and SQL schema updates adjust how organizations are referenced by renaming fields and adding new tables for many-to-many relationships. Updates in controllers and models enforce that the owning organization is automatically included in updates, and corresponding frontend components, API endpoints, and type definitions have been enhanced to support shared organizations. New configuration flags and error messages reflect these updates, and changelog entries document the feature. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
| q"_organization IN (SELECT _organization FROM webknossos.users_ WHERE _id = $requestingUserId)" | ||
| q"""_id IN ( | ||
| SELECT a._aiModel | ||
| FROM webknossos.aiModel_organizations AS a | ||
| INNER JOIN webknossos.organizations AS o | ||
| ON a._organization = o._id | ||
| WHERE | ||
| (o._id IN ( | ||
| SELECT _organization | ||
| FROM webknossos.users_ | ||
| WHERE _multiUser = ( | ||
| SELECT _multiUser | ||
| FROM webknossos.users_ | ||
| WHERE _id = $requestingUserId | ||
| ) | ||
| )) | ||
| OR TRUE IN ( | ||
| SELECT isSuperUser | ||
| FROM webknossos.multiUsers_ | ||
| WHERE _id IN ( | ||
| SELECT _multiUser | ||
| FROM webknossos.users_ | ||
| WHERE _id = $requestingUserId | ||
| ) | ||
| ) | ||
| ) | ||
| """ |
There was a problem hiding this comment.
I adjusted the accessor query so that all models are shown to the user which the user has potentially access to -> all organization in which the user is part of or allowed to join because of being a super user.
While testing I realized that this might be unwanted as e.g. super users can see all models all the time therefore potentially cluttering the view / list of models.
@fm3 Do you think it makes sense to only show models of the organization in which the user is currently logged in?
There was a problem hiding this comment.
Yes I agree, only the organization the user is currently logged in to should be considered.
There was a problem hiding this comment.
Should be better now 🏖️
|
@fm3 and @philippotto do you have time to review the backend / frontend. I'll provide a testing instance with a model and multiple orgas :) |
| hideNavbarLogin = false | ||
| isWkorgInstance = false | ||
| isWkorgInstance = true | ||
| recommendWkorgInstance = true | ||
| taskReopenAllowedInSeconds = 30 | ||
| allowDeleteDatasets = true | ||
| # to enable jobs for local development, use "yarn enable-jobs" to also activate it in the database | ||
| jobsEnabled = false | ||
| voxelyticsEnabled = false | ||
| jobsEnabled = true | ||
| voxelyticsEnabled = true |
![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
🧹 Nitpick comments (5)
app/controllers/AiModelController.scala (1)
114-114: Parallel vs. sequential transformations.
UsingFox.serialCombinedis fine for smaller sets. For large lists, consider a more concurrent approach if performance becomes an issue.conf/evolutions/127-allow-ai-model-sharing.sql (1)
9-13: Create the many-to-many association table.
aiModel_organizationseffectively separates ownership from shared org references. Consider adding an index on_organizationif you expect frequent queries by organization.frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx (1)
207-267: Ensure consistent usage of onClose vs onCancel in the Modal.Line 249 passes both
onCancel={onClose}andonClose={onClose}, but Ant Design's default property isonCancel. IfonCloseis not a custom property, consider removing it to avoid confusion.app/models/aimodels/AiModel.scala (2)
114-140: UpdatedreadAccessQfor multi-organization and superuser logic.
The query correctly grants read access to superusers via theOR TRUE IN ( ... )portion.Consider a UI or query option to reduce clutter for superusers who might see every model by default.
195-203: Potentially excessive debug prints.
Theseprintlncalls will clutter logs in production. Consider eliminating them or placing them behind a debug flag.- println("--------------------------------------------------------------------------------") - println(s"insertOrganizationQuery($aiModelId, ${organizationIds.mkString(",")})") - println("--------------------------------------------------------------------------------")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/misc.e2e.js.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
MIGRATIONS.unreleased.md(1 hunks)app/controllers/AiModelController.scala(7 hunks)app/controllers/InitialDataController.scala(1 hunks)app/models/aimodels/AiInference.scala(1 hunks)app/models/aimodels/AiModel.scala(7 hunks)conf/application.conf(1 hunks)conf/evolutions/127-allow-ai-model-sharing.sql(1 hunks)conf/evolutions/reversions/127-allow-ai-model-sharing.sql(1 hunks)conf/messages(1 hunks)frontend/javascripts/admin/api/jobs.ts(1 hunks)frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx(6 hunks)frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/misc.e2e.js.md(1 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)tools/postgres/schema.sql(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (48)
frontend/javascripts/admin/api/jobs.ts (1)
379-388:✅ Verification successful
Well-implemented API function for updating AI models with sharing capabilities.
This new function aligns with the PR objective of enabling AI model sharing across organizations by providing a clean API to update models with shared organization IDs.
🌐 Web query:
What are the best practices for TypeScript PUT request error handling?Error during web search
Verified: API Function Implementation Approved
The function at
frontend/javascripts/admin/api/jobs.ts(lines 379-388) is well implemented and aligns with the PR objective of enabling AI model sharing. The design appropriately delegates error handling to the underlyingRequest.sendJSONReceiveJSONmethod. While best practices for TypeScript PUT request error handling typically involve explicit error management (e.g., try-catch blocks, logging, or error propagation), assuming that theRequesthelper encapsulates these concerns, no further changes are required at this stage.MIGRATIONS.unreleased.md (1)
14-14:✅ Verification successful
Migration correctly added for AI model sharing functionality.
The migration entry for allowing AI model sharing is properly added to the unreleased migrations list. This is consistent with the PR's objective of enabling cross-organization AI model sharing.
🏁 Script executed:
#!/bin/bash # Description: Check the structure of the migration file to ensure it includes necessary schema changes # Check if the migration file exists and examine its content cat conf/evolutions/127-allow-ai-model-sharing.sql 2>/dev/null || echo "Migration file not found"Length of output: 1388
Migration file validated and confirmed as correct.
The migration entry in
MIGRATIONS.unreleased.mdproperly points to the newly added migration file (conf/evolutions/127-allow-ai-model-sharing.sql), and a review of this file confirms that it contains the necessary schema modifications to support cross-organization AI model sharing. No removed or replaced code is expected here, as this is a new migration entry.frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/misc.e2e.js.md (1)
79-80:✅ Verification successful
Update feature toggles to enable necessary functionality.
These feature toggle changes enable
isWkorgInstance,jobsEnabled, andvoxelyticsEnabledfunctionalities, which are likely required for the AI model sharing feature to work properly in tests.Also applies to: 87-87
🏁 Script executed:
#!/bin/bash # Description: Verify these feature toggles are also updated in the application configuration # Check the application configuration for corresponding changes grep -A 10 "isWkorgInstance\|jobsEnabled\|voxelyticsEnabled" conf/application.conf 2>/dev/null || echo "Configuration not found"Length of output: 1410
Action: Feature toggles configuration verified
The updated code now correctly reflects the necessary toggles. The configuration file confirms that:
isWkorgInstanceis set totruejobsEnabledis set totruevoxelyticsEnabledis set totrueThis ensures that the AI model sharing feature is properly enabled in tests. No additional changes are required.
app/controllers/InitialDataController.scala (1)
148-158:✅ Verification successful
Sample AI model correctly updated with shared organizations list.
The default AI model initialization now includes a list of shared organization IDs, which initially contains only the owning organization ID. This change supports the new model sharing capability while maintaining backward compatibility.
🏁 Script executed:
#!/bin/bash # Description: Verify that the AiModel class has been updated to support shared organizations # Check if AiModel class has been updated to include shared organizations cat app/models/aimodels/AiModel.scala 2>/dev/null | grep -A 10 "class AiModel"Length of output: 1574
Verified: AI Model correctly supports shared organizations.
The sample AI model initialization now properly includes the shared organization IDs list (with the owning organization ID as its initial element). This setup aligns with the updated case class definition in
app/models/aimodels/AiModel.scala, where the_organizations: List[String]field is now explicitly included, supporting the new model sharing capability while preserving backward compatibility.
- Confirmed that the shared organizations list is present in the AiModel case class.
- The initialization in
app/controllers/InitialDataController.scalacorrectly uses a list containing the default organization ID.conf/messages (1)
387-389: Good addition of clear and specific error messages.These new error messages support the cross-organization AI model sharing feature by providing clear feedback for different failure scenarios: unauthorized access to models, failed model updates, and failed sharing permission updates.
app/models/aimodels/AiInference.scala (1)
51-51: Updated method signature to include requesting user context.The method call has been updated to pass the requesting user to
aiModelService.publicWrites(), which is necessary to determine the ownership status of AI models relative to the user's organization.conf/application.conf (2)
149-149:❓ Verification inconclusive
Verify this change is intended for production.
Setting
isWkorgInstancetotrueindicates this is a WEBKNOSSOS.org instance rather than a self-hosted instance. This affects behaviors such as redirects and UI elements as noted in comments around line 158. Ensure this is intentionally committed.
🏁 Script executed:
#!/bin/bash # Description: Check if this configuration change is consistent with other deployed environments # Look for other environment-specific configurations in the repository echo "Checking for environment-related configuration files..." find . -type f -name "*.conf" | grep -v "application.conf" | xargs grep -l "isWkorgInstance" || echo "No other environment configs found with isWkorgInstance" echo "Checking for deployment scripts that might set this value..." find . -type f -name "*.sh" -o -name "*.yml" | xargs grep -l "isWkorgInstance" || echo "No deployment scripts found setting isWkorgInstance"Length of output: 655
Double-check Production Intent for
isWkorgInstanceFlagThe configuration now sets:
isWkorgInstance = trueA repository search confirms that no other environment configuration files or deployment scripts reference or override
isWkorgInstance. Since this setting directly influences production behavior (redirects, UI elements, etc.), please confirm that having it set to true is indeed intentional for production.
154-155:❓ Verification inconclusive
Verify these feature flags are intentionally enabled.
Enabling
jobsEnabledandvoxelyticsEnabledmakes long-running job functionality and voxelytics features available in the application. Ensure these changes are intentional and not just for local development (the comment on line 154 suggests this might be a development-only change).
🏁 Script executed:
#!/bin/bash # Description: Check if these feature flags are conditionally set in deployment configurations echo "Checking for conditional setting of feature flags in deployment configurations..." find . -type f -name "*.conf" -o -name "*.yml" -o -name "*.sh" | xargs grep -l "jobsEnabled\|voxelyticsEnabled" || echo "No deployment configurations found setting these flags" echo "Checking PR description to see if this is mentioned as a dev-only change..." grep -i "dev-only" "$PR_DESCRIPTION_FILE" 2>/dev/null || echo "PR description file not available"Length of output: 570
Action Required: Confirm the Intentional Use of Feature Flags
The configuration still permanently enables both
jobsEnabledandvoxelyticsEnabledinconf/application.confwithout any conditional logic or environment-specific overrides. Additionally, there’s no pointer in the PR documentation (or elsewhere) suggesting this is intended for development only.
- The only occurrence of these flags was found in
conf/application.conf.- No deployment configurations (e.g., in other
.conf,.yml, or.shfiles) conditionally set these flags.- The PR description does not indicate that these settings are meant for a development-only environment.
Please verify whether these features should be active in your production environment. If they were meant to be enabled only for local development, consider updating the configuration to conditionally enable them and document this behavior accordingly.
frontend/javascripts/types/api_flow_types.ts (2)
769-771: Good addition of shared organization fields to the AiModel type.These new fields support the cross-organization sharing functionality:
isOwnedByUsersOrganizationprovides context about whether the current user can modify the modelsharedOrganizationIdstracks which organizations have access to the modelThis type definition aligns well with the backend changes for AI model sharing.
772-772: Appropriate nullable user field.Making the
userfield nullable (APIUser | null | undefined) is the right approach, as there may be cases where user information is not available, particularly for shared models from other organizations.conf/evolutions/reversions/127-allow-ai-model-sharing.sql (6)
1-3: Ensure consistency with forward migration.
This reversion script correctly starts a transaction and asserts that the schema is at version 127 before rolling back. However, please verify that the forward migration cleanly re-applies after this transition to maintain data integrity.
5-6: Validate table/view drop order.
DroppingaiModel_organizationsand thenaiModels_aligns with removing child dependencies first. Confirm the forward script's creation order matches this removal sequence for a seamless rollback.
9-10: Confirm column and constraint rename matches original schema.
Renaming_owningOrganizationback to_organizationandowningOrganization_refback toorganization_refis correct for reverting. Please ensure references in other code or constraints are updated accordingly.
12-12: Recreate previous view definition.
RecreatingaiModels_as a non-deleted filter matches its prior logic. Confirm that any changes introduced elsewhere (e.g., columns or constraints) do not break queries using this view.
14-14: Schema version alignment check.
Downgrading to 126 is consistent with reverting from 127. Ensure future migrations also correctly handle any intermediate increments.
16-16: Transaction scope clarity.
Explicitly committing the transaction ensures changes are fully reverted without leaving partial schemas. This finalizes the rollback process as intended.app/controllers/AiModelController.scala (4)
57-57: Parameter addition to handle shared organizations.
DefiningsharedOrganizationIdshere is necessary for multi-org sharing. Verify that downstream code can handle cases where this field isNoneor an empty list without errors.
94-94: Ensure consistent serialization.
Passingrequest.identityintopublicWriteslooks appropriate for context-based serialization. Ensure that any nested logic requiring user context is handled equivalently.
159-160: Owning organization & organizations list duplication.
You are storing both_owningOrganizationand_organizations = List(_owningOrganization). This correctly seeds sharing data. Confirm that_organizationsis never empty and always contains_owningOrganization, to avoid misalignment.
170-170: Include requesting user context in published data.
Supplyingrequest.identityintopublicWritesensures consistent filtering of sensitive fields. This is a good addition to maintain correct scoping.conf/evolutions/127-allow-ai-model-sharing.sql (9)
1-3: Ensure forward migration matches prior version.
Starting a transaction and asserting that the schema is 126 is standard. Confirm that the previous evolution leaves the schema at exactly 126 to avoid partial or corrupt states.
5-6: Drop preceding views safely.
Dropping theaiModels_view first helps avoid dependency conflicts with the new table or constraints. Confirm no underlying logic referencesaiModels_before re-creating it.
7-7: Renaming column for clarity.
Renaming_organizationto_owningOrganizationclarifies its primary ownership role. This naming aligns with domain-driven clarity.
14-16: Adjust constraints for new uniqueness rules.
Switching the unique constraint from_organization+nameto_owningOrganization+nameis consistent with the rename. Also renamingorganization_reftoowningOrganization_refensures consistent naming.
17-19: Foreign key constraints withDEFERRABLE.
UsingDEFERRABLEfor cascades is helpful in complex migrations. Double-check that your database config allows deferrable constraints to ensure no contradictory constraints surface.
21-22: Initial population of new table.
Copying_owningOrganizationvalues intoaiModel_organizationsseeds the many-to-many link. This preserves existing relationships. Test thoroughly to ensure data integrity.
24-24: Recreate theaiModels_view.
Filtering onNOT isDeletedremains consistent. This coverage ensures the old references remain valid in code that queries this view.
26-26: Bump schema version to 127.
This final step cements the new structure. Confirm version synchronization inreleaseInformationis fully tested to prevent accidental rollback.
28-28: Commit changes.
Completing the transaction finalizes the migration. Good practice.frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx (6)
1-2: No issues with these import statements.Also applies to: 4-4, 7-7, 8-8
28-28: Storing the entire AiModel object in state.
This approach looks fine given that the object does not appear to be overly large.
112-113: Logic to open the shared organization editor.
The integration with the editor callback is concise and correct.
182-187: Conditional rendering for shared organizations.
Good use of a dedicated button to edit shared organizations if the user owns the model.
189-189: Return logic for models without a training job.
Properly returning the button whenmodel.trainingJob == null.
195-195: Rendering the sharing button in the training job scenario.
Ensures consistent display of the shared-organization editor link.tools/postgres/schema.sql (5)
23-23: Schema version bumped to 127.
This increment aligns with the new migration.
556-556: New_owningOrganizationcolumn and uniqueness constraint.
Adding a unique constraint on(_owningOrganization, name)helps prevent duplicate model names within the same org.Also applies to: 566-567
569-573: Many-to-many relationship table.
aiModel_organizationsproperly sets up a model-to-multiple-organizations mapping.
870-870: Added foreign key for_owningOrganization.
Ensures that the owning organization must exist and reinforces referential integrity.
883-885: Foreign keys foraiModel_organizations.
Linking_aiModeland_organizationto their respective tables is correct.app/models/aimodels/AiModel.scala (8)
10-10: New imports introduced.
These additions look necessary and correct.Also applies to: 17-18
25-26: Case class fields for_owningOrganizationand_organizations.
This aligns with the updated DB schema to differentiate the primary owner from other shared organizations.
44-70: EnhancedpublicWritesto account for the requesting user's organization.
Conditionally exposingsharedOrganizationIdsonly if the user is part of the owning organization is a sound privacy measure.
94-97: Parsing_organizationsfrom the DB.
Matches the newaiModel_organizationsjoin table logic.
158-174: Inserting AI model and organizations.
Using a transaction to insert both the model and its associated organizations is a robust approach.
205-208: Insert query for a single organization.
Implementation is straightforward.
209-215: Retrieving organizations for a givenaiModel.
Simple and clear approach to gather all associated organization IDs.
229-235:updateSharedOrganizationstransactional approach.
Dropping and re-inserting rows within a transaction is valid for syncing updated organization lists.
There was a problem hiding this comment.
Backend looking good! First test worked for me as well :)
I think “shared organizations” is a bit of a weird term for the UI, since it’s not the organizations that are shared , but the model is shared with them. Maybe the frontend reviewers have good suggestions there.
On the other hand, in the code, I’d stick to the term sharedOrganizations, as analogous to annotation’s sharedTeams, and would in fact suggest to use it more consistently, see my comments.
I added a couple more comments.
| jsResult <- aiModelService.publicWrites(updatedAiModel) | ||
| } yield Ok(jsResult) | ||
| { | ||
| // Automatically add the owning organization to the shared organizations if it is not already there. |
There was a problem hiding this comment.
Maybe we should avoid this redundant saving and just consider the owning organization too when checking the shared orgas? I’m not sure what’s best, but the current variant means that we have to ensure that the owning orga is always in the shared orgas list too. But according to the comment, it could indeed be not already there. What happens in this case?
There was a problem hiding this comment.
Maybe we should avoid this redundant saving and just consider the owning organization too when checking the shared orgas?
Yeah, that's also a possible solution.
but the current variant means that we have to ensure that the owning orga is always in the shared orgas list too
Correct 👍
But according to the comment, it could indeed be not already there. What happens in this case?
It should always already be there. But this manual adding of the shared organization is done to avoid that a user can send an update which makes the model inaccessible to the owning organization.
I adjusted the comment a little to reflect this better.
app/models/aimodels/AiModel.scala
Outdated
| organizationIds: List[String]): List[SqlAction[Int, NoStream, Effect]] = { | ||
| println("--------------------------------------------------------------------------------") | ||
| println(s"insertOrganizationQuery($aiModelId, ${organizationIds.mkString(",")})") | ||
| println("--------------------------------------------------------------------------------") |
app/models/aimodels/AiModel.scala
Outdated
| q"""INSERT INTO webknossos.aiModel_organizations(_aiModel, _organization) | ||
| VALUES($aiModelId, $organizationId)""".asUpdate | ||
|
|
||
| private def findOrganizationsFor(aiModelId: ObjectId): Fox[List[String]] = |
There was a problem hiding this comment.
please use the term sharedOrganizations everywhere
tools/postgres/schema.sql
Outdated
| CREATE TABLE webknossos.aiModels( | ||
| _id CHAR(24) PRIMARY KEY, | ||
| _organization VARCHAR(256) NOT NULL, | ||
| _owningOrganization VARCHAR(256) NOT NULL, |
There was a problem hiding this comment.
It might be more consistent with the other code to leave this _organization and rather rename the others to sharedOrganizations. That would be like what we have with annotations and teams. What do you think?
There was a problem hiding this comment.
🤔 Hmm I see the following arguments regarding leaving the name at _organization:
Pro
- More consistent with the code base / rest of the schema
Con
- required domain / schema knowledge or guess work to figure out that the
_organizationis the organization actually owning the model while naming it_owningOrganizationmakes this obvious leading maybe to less potential confusion.
So IMO I prefer "obvious" naming over short names, but this would also make it a little inconsistent with the other parts of the schema. Still I have a slight tendency to go for _owningOrganization bc. it is more straight forward. But I am open to revert the renaming back to _organization.
There was a problem hiding this comment.
I see your point, and ultimately it’s not that important. But I still think that if the organizations the model is shared with were called webknossos.aiModel_sharedOrganizations (which would be consistent with sharedTeams and more descriptive than just organizations), the semantics of _organization would be fairly clear. This is the organization the aiModel lives in / belongs to, just like with users, datasets, teams etc..
There was a problem hiding this comment.
Ok I switched back to _organization ♻️
| segmentAnythingEnabled: false, | ||
| taskReopenAllowedInSeconds: 30, | ||
| voxelyticsEnabled: false, | ||
| voxelyticsEnabled: true, |
There was a problem hiding this comment.
when the application.conf is reset again, these changes also need to be undone
| q"_organization IN (SELECT _organization FROM webknossos.users_ WHERE _id = $requestingUserId)" | ||
| q"""_id IN ( | ||
| SELECT a._aiModel | ||
| FROM webknossos.aiModel_organizations AS a | ||
| INNER JOIN webknossos.organizations AS o | ||
| ON a._organization = o._id | ||
| WHERE | ||
| (o._id IN ( | ||
| SELECT _organization | ||
| FROM webknossos.users_ | ||
| WHERE _multiUser = ( | ||
| SELECT _multiUser | ||
| FROM webknossos.users_ | ||
| WHERE _id = $requestingUserId | ||
| ) | ||
| )) | ||
| OR TRUE IN ( | ||
| SELECT isSuperUser | ||
| FROM webknossos.multiUsers_ | ||
| WHERE _id IN ( | ||
| SELECT _multiUser | ||
| FROM webknossos.users_ | ||
| WHERE _id = $requestingUserId | ||
| ) | ||
| ) | ||
| ) | ||
| """ |
There was a problem hiding this comment.
Yes I agree, only the organization the user is currently logged in to should be considered.
There was a problem hiding this comment.
frontend looks good mostly 👍 cool stuff! left some comments.
I also have one question regarding the permissions: Who exactly may update the shared orgas? from the FE code it looks like I have to be member of the owning orga. but what happens in this scenario:
Orga 1 contains User A and user B
Orga 2 contains User B and user C
if user B creates the model in orga 2, user B and user C can edit the organizations, right? however, if B sets the orgas to 1 and 2, user C won't be able to see/change orga 1. when saving, they will likely remove orga 1 automatically because the FE only fetches getUsersOrganizations.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🔭 Outside diff range comments (5)
app/models/aimodels/AiModel.scala (5)
40-43:⚠️ Potential issueRemove duplicate
userServiceinjection.There's a duplicate injection of
userServicein the constructor parameters.class AiModelService @Inject()(dataStoreDAO: DataStoreDAO, dataStoreService: DataStoreService, userDAO: UserDAO, userService: UserService, - userService: UserService, jobDAO: JobDAO, jobService: JobService) {
76-82:⚠️ Potential issueRemove duplicate code.
These lines appear to be a duplicate of the Json.obj from lines 63-73. They should be removed.
- "user" -> userJs, - "comment" -> aiModel.comment, - "trainingJob" -> trainingJobJsOpt, - "created" -> aiModel.created, - "sharedOrganizationIds" -> sharedOrganizationIds - ) -}
159-166:⚠️ Potential issueRemove duplicate INSERT query.
These lines duplicate the insert query from lines 151-158 and should be removed.
- q"""INSERT INTO webknossos.aiModels ( - _id, _owningOrganization, _dataStore, _user, _trainingJob, name, - comment, category, created, modified, isDeleted - ) VALUES ( - ${a._id}, ${a._owningOrganization}, ${a._dataStore}, ${a._user}, ${a._trainingJob}, ${a.name}, - ${a.comment}, ${a.category}, ${a.created}, ${a.modified}, ${a.isDeleted} - ) - """.asUpdate
201-203:⚠️ Potential issueRemove duplicate insertOrganizationQuery method.
This appears to be a duplicate of the method defined in lines 197-199 and should be removed.
- private def insertOrganizationQuery(aiModelId: ObjectId, organizationId: String): SqlAction[Int, NoStream, Effect] = - q"""INSERT INTO webknossos.aiModel_organizations (_aiModel, _organization) - VALUES ($aiModelId, $organizationId)""".asUpdate
229-233:⚠️ Potential issueRemove duplicate code.
These lines duplicate the end of the
updateSharedOrganizationsmethod and should be removed.- run(DBIO.sequence(deleteQuery +: insertQueries).transactionally).map(_ => ()) - } - -}
♻️ Duplicate comments (1)
app/models/aimodels/AiModel.scala (1)
189-191:⚠️ Potential issueRemove debug println statements.
These debug logging statements should be removed before merging to production.
private def insertOrganizationQuery(aiModelId: ObjectId, organizationIds: List[String]): List[SqlAction[Int, NoStream, Effect]] = { - println("--------------------------------------------------------------------------------") - println(s"insertOrganizationQuery($aiModelId, ${organizationIds.mkString(",")})") - println("--------------------------------------------------------------------------------") organizationIds.map { organizationId => insertOrganizationQuery(aiModelId, organizationId) } }
🧹 Nitpick comments (1)
app/models/aimodels/AiModel.scala (1)
60-61: Use consistent terminology for shared organizations.For consistency, consider using
sharedOrganizationsinstead of_organizationsin the model's field list. This aligns with the comment from a previous review to "use the term sharedOrganizations everywhere".isOwnedByUsersOrganization = aiModel._owningOrganization == requestingUser._organization - sharedOrganizationIds = if (isOwnedByUsersOrganization) Some(aiModel._organizations) else None + sharedOrganizationIds = if (isOwnedByUsersOrganization) Some(aiModel._organizations) else None
🛑 Comments failed to post (2)
util/src/main/scala/com/scalableminds/util/requestlogging/RequestLogging.scala (1)
32-32: 💡 Verification agent
🧩 Analysis chain
Increased duration threshold for request logging.
The default duration threshold for logging request times has been increased from 30 seconds to 2 minutes. This change will reduce log volume by not logging requests that take between 30 seconds and 2 minutes, but it also reduces visibility into moderately slow requests.
Consider whether this change is appropriate for your observability needs, especially when troubleshooting performance issues. If the change is intentional to accommodate new cross-organization AI model operations that typically take longer, please document this reasoning in a comment.
🏁 Script executed:
#!/bin/bash # Check for any related logging configuration or comments that might explain this threshold change echo "Searching for related logging configuration..." rg -A 3 -B 3 "durationThreshold" --type scala echo "Checking for existing logs with durations between 30s and 2min..." # This will help understand the impact of the change on log volume rg "Request .* took .* and was" --type log | grep -E "took [0-9]+m[0-9]+\.[0-9]+s|took [3-9][0-9]\.[0-9]+s|took 1m[0-5][0-9]\.[0-9]+s" | wc -lLength of output: 2970
Action Required: Document the Rationale Behind the Increased Logging Threshold
The default duration threshold in
logTime(located inutil/src/main/scala/com/scalableminds/util/requestlogging/RequestLogging.scalaat line 32) has been increased from 30 seconds to 2 minutes. While this update will indeed lower log volume by ignoring requests that take between 30 seconds and 2 minutes, it also reduces the visibility into moderately slow requests that might reveal performance issues.
- Impact: Moderately slow requests (30s–2min) will no longer be logged by this method, which could make troubleshooting performance anomalies more challenging.
- Recommendation: If this change is intentional—perhaps to better support the new cross-organization AI model operations that typically run longer—please add an inline comment or documentation that explains the reasoning behind choosing a 2-minute threshold.
- Next Steps: Re-assess if this new threshold aligns with overall observability requirements or if adjustments/documentation are needed for clarity.
app/controllers/WKRemoteWorkerController.scala (1)
83-83: 💡 Verification agent
🧩 Analysis chain
Added error handling for failed cleanup.
Added proper error handling with the message "job.cleanup.failed" to better report issues during the cleanup process for failed jobs.
🏁 Script executed:
#!/bin/bash # Verify that error messages are defined in the messages file rg "job\.notFound|job\.cleanup\.failed" conf/messages || echo "Some error message keys may not be defined in the messages file"Length of output: 192
Action Required: Define Missing Error Message Key in conf/messages
- The error message key
"job.cleanup.failed"referenced inapp/controllers/WKRemoteWorkerController.scala(line 83) is not defined in theconf/messagesfile.- Please add an appropriate message for
"job.cleanup.failed"to ensure the error handling reports issues correctly.- Additionally, verify if
"job.notFound"(if used elsewhere) also needs to be defined.
Currently, only super users can change the sharing settings of a model. The issue states that this feature is needed to make trained models from us available to customers. As we are all super user this should work. Additionally, the active user (not mulit user) needs to be in the owning organization (the orga in which the training was started in).
Oh damn. Very good catch. That could actually become a problem when we allow this for non-super users 🤔. TODO:
|
…ut potential other orgas that can access the model and more consistent naming
…ccess settings model
… model are updated.
…red organizations list
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/models/aimodels/AiModel.scala (1)
145-146:⚠️ Potential issueOutdated reference to
_organizationcolumn.This query still references the old column name
_organizationinstead of the renamed_owningOrganization. This should be updated to maintain consistency with the schema changes.- q"SELECT COUNT(*) FROM webknossos.aiModels WHERE name = $aiModelName AND _organization = $organizationId" + q"SELECT COUNT(*) FROM webknossos.aiModels WHERE name = $aiModelName AND _owningOrganization = $organizationId"
🧹 Nitpick comments (2)
frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx (1)
251-252: RedundantonClosepropThe Modal component has both
onCancelandonCloseprops set to the same function. According to Ant Design's documentation,onCloseis not a standard prop for Modal - you can remove it.onOk={submitNewSharedOrganizations} onCancel={onClose} - onClose={onClose} maskClosable={false}app/models/aimodels/AiModel.scala (1)
80-87: Output JSON updated with ownership and sharing information.The JSON output now includes an
isOwnedByUsersOrganizationflag andsharedOrganizationIdsfield to expose the sharing information to the frontend.Consider using a consistent naming scheme between the model field (
_sharedOrganizations) and the JSON field (sharedOrganizationIds). While functionally correct, consistency can improve code readability and maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.unreleased.md(1 hunks)MIGRATIONS.unreleased.md(1 hunks)app/controllers/AiModelController.scala(7 hunks)app/models/aimodels/AiModel.scala(8 hunks)conf/evolutions/128-allow-ai-model-sharing.sql(1 hunks)conf/evolutions/reversions/128-allow-ai-model-sharing.sql(1 hunks)conf/messages(1 hunks)frontend/javascripts/admin/api/jobs.ts(1 hunks)frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx(6 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)tools/postgres/schema.sql(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/javascripts/admin/api/jobs.ts
- MIGRATIONS.unreleased.md
- conf/messages
- frontend/javascripts/types/api_flow_types.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~14-~14: This is normally spelled as one word.
Context: ...os/compare/25.02.1...HEAD) ### Added - Super users can now share the trained AI models wit...
(SUPER_COMPOUNDS)
🔇 Additional comments (32)
CHANGELOG.unreleased.md (1)
14-14: Entry looks good!This changelog entry clearly describes the new feature, allowing super users to share trained AI models across organizations, and correctly links to the pull request.
🧰 Tools
🪛 LanguageTool
[grammar] ~14-~14: This is normally spelled as one word.
Context: ...os/compare/25.02.1...HEAD) ### Added - Super users can now share the trained AI models wit...(SUPER_COMPOUNDS)
conf/evolutions/reversions/128-allow-ai-model-sharing.sql (1)
1-16: SQL reversion script is well-structuredThe reversion script correctly rolls back the database changes by:
- Dropping the new junction table for the many-to-many relationship
- Renaming columns back to their original names
- Recreating the necessary view
- Properly updating the schema version
This follows good migration practices by ensuring proper version checking and transaction handling.
app/controllers/AiModelController.scala (5)
57-57: Parameter definition aligns with feature requirementsThe addition of the optional
sharedOrganizationIdsfield toUpdateAiModelParametersproperly supports the new model sharing functionality.
94-94: Identity parameter consistently added to publicWrites callsYou've systematically updated all calls to
aiModelService.publicWritesto include the requesting user's identity, which is necessary for permission-based rendering of AI models.Also applies to: 114-114, 170-170, 248-248
159-161: Proper initialization of organization fieldsUpdated the model creation to use
_owningOrganizationand added initialization of_sharedOrganizationswith the owning organization, ensuring consistent access permissions from creation.
217-250: Robust shared organization handling logicThe shared organization update logic correctly:
- Ensures the owning organization is always included in shared organizations
- Verifies ownership before allowing updates
- Preserves access for organizations that the current user might not have access to
However, consider wrapping both model updates (line 228 and line 246) in a single transaction to prevent partial updates, as suggested in a previous review.
- _ <- aiModelDAO.updateOne(aiModel.copy(name = request.body.name, - comment = request.body.comment, - modified = Instant.now)) ?~> "aiModel.updatingFailed" - areSharedOrganizationsUpdated = sharedOrganizationIdsOpt - .getOrElse(List()) - .toSet != aiModel._sharedOrganizations.toSet - sharedAndPreservedOrganizationIdsOpt <- if (areSharedOrganizationsUpdated) - Fox.runOptional(sharedOrganizationIdsOpt) { newlySharedOrganizationIds => - // Keep organizations with access to the aiModel which the current user has no access to. - for { - organizationIdsUserCanAccess <- organizationDAO.findAll.flatMap(os => Fox.successful(os.map(_._id))) - organizationsToPreserveAccessTo = aiModel._sharedOrganizations.filter( - organizationIdsUserCanAccess.contains) - } yield newlySharedOrganizationIds ++ organizationsToPreserveAccessTo - } else Fox.successful(None) - _ <- Fox.runOptional(sharedAndPreservedOrganizationIdsOpt)( - orgas => - if (orgas.toSet == aiModel._sharedOrganizations.toSet) Fox.successful(()) - else aiModelDAO.updateSharedOrganizations(aiModel._id, orgas)) ?~> "aiModel.updatingSharedFailed" + _ <- Fox.serialCombined(List( + aiModelDAO.updateOne(aiModel.copy(name = request.body.name, + comment = request.body.comment, + modified = Instant.now)), + { + val areSharedOrganizationsUpdated = sharedOrganizationIdsOpt + .getOrElse(List()) + .toSet != aiModel._sharedOrganizations.toSet + if (!areSharedOrganizationsUpdated) { + Fox.successful(()) + } else { + Fox.runOptional(sharedOrganizationIdsOpt) { newlySharedOrganizationIds => + // Keep organizations with access to the aiModel which the current user has no access to. + for { + organizationIdsUserCanAccess <- organizationDAO.findAll.flatMap(os => Fox.successful(os.map(_._id))) + organizationsToPreserveAccessTo = aiModel._sharedOrganizations.filter( + organizationIdsUserCanAccess.contains) + orgas = newlySharedOrganizationIds ++ organizationsToPreserveAccessTo + _ <- if (orgas.toSet == aiModel._sharedOrganizations.toSet) Fox.successful(()) + else aiModelDAO.updateSharedOrganizations(aiModel._id, orgas) + } yield () + } getOrElse Fox.successful(()) + } + } + )) ?~> "aiModel.updatingFailed"
263-264: Consistent initialization in registerAiModelThe
registerAiModelmethod correctly initializes both the_owningOrganizationand_sharedOrganizationsfields, ensuring even manually registered models will properly support the sharing functionality.frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx (7)
49-51: Fixed refresh counter issueThe refresh counter state update is now properly called directly within the callback, which will correctly trigger a refresh after the modal is closed.
182-190: Conditional UI for organization sharing buttonThe organization sharing button is correctly shown only for models owned by the user's organization, preventing users from attempting to manage access to models they don't own.
185-185: Consider consistent terminologyFor better UI consistency, consider using "Manage Access" in both the button and modal title.
225-229: Ensure owning organization is always includedGood defensive programming by ensuring the owning organization is always in the selected list, even when dealing with UI selections that might try to remove it.
231-243: Immutable state handling in submission functionThe code correctly creates a new model object using spread syntax before updating, avoiding direct mutations of the state. This is a good practice for React components.
259-264: Clear user guidance for permission limitationsThe explanatory text clearly communicates the constraints and expected behavior regarding organization access management, helping users understand the permission model.
266-276: Well-implemented organization selectionThe Select component is well configured with appropriate properties for a multiple selection interface. The disabled state for the owning organization and explanatory title are good UX touches.
tools/postgres/schema.sql (5)
23-23: Schema version updated to 128.The schema version has been incremented to 128, which matches the migration script number (128-allow-ai-model-sharing.sql). This is correct and follows the established versioning pattern.
557-557: Column rename for clarity.The column
_organizationhas been renamed to_owningOrganizationto better express its purpose as the owner of the AI model, distinguishing it from organizations that the model is shared with.It might be worth noting that an alternative approach would have been to leave this as
_organizationand usesharedOrganizationsfor the many-to-many relationship, as was suggested in a previous review comment. This would be consistent with the annotation and team patterns elsewhere in the codebase. However, the current approach also clearly communicates the ownership relationship.
567-567: Unique constraint updated to use the new column name.The unique constraint has been properly updated to reference the new column name
_owningOrganization.
570-574: New many-to-many relationship table added.This new table
aiModel_organizationscreates a many-to-many relationship between AI models and organizations, allowing models to be shared across multiple organizations. The primary key is appropriately defined as a composite of both foreign keys.
884-886: Foreign key constraints added for the new relationship table.The foreign key constraints are properly defined with appropriate cascade rules for deletion and updates, ensuring referential integrity between the
aiModel_organizationstable and both theaiModelsandorganizationstables.conf/evolutions/128-allow-ai-model-sharing.sql (5)
1-3: Transaction and version check properly implemented.The migration script correctly begins with a transaction and verifies that the current schema version is 127 before proceeding, which prevents applying migrations in the wrong order.
5-7: View dropped and column renamed.The script properly drops the dependent view before renaming the column, which is necessary since views can't automatically adapt to schema changes.
9-19: New relationship table created with proper constraints.The script creates the new many-to-many relationship table with appropriate primary key and foreign key constraints, ensuring referential integrity between models and organizations.
21-22: Migration preserves existing relationships.The script correctly populates the new junction table with existing relationships, ensuring that all models remain accessible to their owning organizations after the migration.
24-26: View recreated and schema version updated.The script recreates the view that was dropped earlier and updates the schema version to 128, completing the migration process.
app/models/aimodels/AiModel.scala (8)
26-28: Model class updated to support organization sharing.The
AiModelcase class has been updated to rename_organizationto_owningOrganizationand add a new_sharedOrganizationsfield, reflecting the database schema changes and supporting the new sharing functionality.
46-58: Method signature updated and null-handling improved.The
publicWritesmethod now takes arequestingUserparameter to allow contextual formatting based on the requesting user. The null-handling has been improved usingfutureBoxand pattern matching, which is more robust than the previous approach.
60-69: Improved error handling for training job lookup.The code now properly handles cases where a training job might not exist (such as if it was deleted), ensuring the application doesn't crash when displaying AI model details.
70-76: Organization ownership check and shared organization filtering.This logic correctly determines if the model is owned by the requesting user's organization and only retrieves shared organization IDs that the user can access. This ensures users can only see shared organizations for models owned by their organization.
121-133: Read access query updated to support shared models.The
readAccessQquery has been updated to allow users to access models that are shared with their organization, correctly implementing the cross-organization sharing functionality.
152-167: Insert operation updated to include shared organizations.The
insertOnemethod now properly inserts both the model and its shared organization relationships, with all operations wrapped in a transaction to ensure data consistency.
188-205: Helper methods added for shared organization management.These new private methods handle the insertion and retrieval of shared organization relationships, providing clean separation of concerns and reusable functionality.
219-225: Method added for updating shared organizations.The
updateSharedOrganizationsmethod efficiently updates shared organizations by deleting all existing relationships and inserting the new ones in a single transaction, ensuring atomicity of the operation.Consider adding a check to ensure the owning organization is always included in the shared organizations list, similar to what appears to be done in the controller (based on the PR summary). This would provide an extra safety measure to prevent the owning organization from losing access to its own model.
There was a problem hiding this comment.
Thanks for all the valuable feedback 📿
I also tackled the permissions problem now, by preserving organizations that have access to the Ai model which the active use cannot access.So
make sharing resilient if users from different organizations can edit sharing settings
should be ticked off now.
Additionally, I noticed that a user of the owning organization was able to see the ids of all organizations which have access to the model, even those the user cannot access. This should be fixed now.
Moreover, the AI models now only shows the models the active orga has access to.
Else I did some retesting and it looks like it was still working 😸
| jsResult <- aiModelService.publicWrites(updatedAiModel) | ||
| } yield Ok(jsResult) | ||
| { | ||
| // Automatically add the owning organization to the shared organizations if it is not already there. |
There was a problem hiding this comment.
Maybe we should avoid this redundant saving and just consider the owning organization too when checking the shared orgas?
Yeah, that's also a possible solution.
but the current variant means that we have to ensure that the owning orga is always in the shared orgas list too
Correct 👍
But according to the comment, it could indeed be not already there. What happens in this case?
It should always already be there. But this manual adding of the shared organization is done to avoid that a user can send an update which makes the model inaccessible to the owning organization.
I adjusted the comment a little to reflect this better.
| q"_organization IN (SELECT _organization FROM webknossos.users_ WHERE _id = $requestingUserId)" | ||
| q"""_id IN ( | ||
| SELECT a._aiModel | ||
| FROM webknossos.aiModel_organizations AS a | ||
| INNER JOIN webknossos.organizations AS o | ||
| ON a._organization = o._id | ||
| WHERE | ||
| (o._id IN ( | ||
| SELECT _organization | ||
| FROM webknossos.users_ | ||
| WHERE _multiUser = ( | ||
| SELECT _multiUser | ||
| FROM webknossos.users_ | ||
| WHERE _id = $requestingUserId | ||
| ) | ||
| )) | ||
| OR TRUE IN ( | ||
| SELECT isSuperUser | ||
| FROM webknossos.multiUsers_ | ||
| WHERE _id IN ( | ||
| SELECT _multiUser | ||
| FROM webknossos.users_ | ||
| WHERE _id = $requestingUserId | ||
| ) | ||
| ) | ||
| ) | ||
| """ |
There was a problem hiding this comment.
Should be better now 🏖️
tools/postgres/schema.sql
Outdated
| CREATE TABLE webknossos.aiModels( | ||
| _id CHAR(24) PRIMARY KEY, | ||
| _organization VARCHAR(256) NOT NULL, | ||
| _owningOrganization VARCHAR(256) NOT NULL, |
There was a problem hiding this comment.
🤔 Hmm I see the following arguments regarding leaving the name at _organization:
Pro
- More consistent with the code base / rest of the schema
Con
- required domain / schema knowledge or guess work to figure out that the
_organizationis the organization actually owning the model while naming it_owningOrganizationmakes this obvious leading maybe to less potential confusion.
So IMO I prefer "obvious" naming over short names, but this would also make it a little inconsistent with the other parts of the schema. Still I have a slight tendency to go for _owningOrganization bc. it is more straight forward. But I am open to revert the renaming back to _organization.
| sharedAndPreservedOrganizationIdsOpt <- if (areSharedOrganizationsUpdated) | ||
| Fox.runOptional(sharedOrganizationIdsOpt) { newlySharedOrganizationIds => | ||
| // Keep organizations with access to the aiModel which the current user has no access to. | ||
| for { | ||
| organizationIdsUserCanAccess <- organizationDAO.findAll.flatMap(os => Fox.successful(os.map(_._id))) | ||
| organizationsToPreserveAccessTo = aiModel._sharedOrganizations.filter( | ||
| organizationIdsUserCanAccess.contains) | ||
| } yield newlySharedOrganizationIds ++ organizationsToPreserveAccessTo | ||
| } else Fox.successful(None) |
There was a problem hiding this comment.
Here is the block I implemented to keep the organizations with already access to the model which the current active user cannot access
There was a problem hiding this comment.
Backend is getting there :) I added a few more minor comments.
If you prefer the approach of re-adding the user orga to the shared organizations on every update over adding the owning organization to the access query, well fine by me, it makes no big difference ;-)
app/models/aimodels/AiModel.scala
Outdated
| dataStore <- dataStoreDAO.findOneByName(aiModel._dataStore) | ||
| user <- userDAO.findOne(aiModel._user) | ||
| userJs <- userService.compactWrites(user) | ||
| user <- userDAO |
There was a problem hiding this comment.
maybe userOpt? Also, maybe we have a box2Option conversion somewhere that could be used Here? Or if not, maybe create it. Ideally, I would see something like Fox.runOptional(userBox.toOption)(userService.compactWrites) but I don’t know if we have everything to make that happen.
There was a problem hiding this comment.
userDAO
.findOne(aiModel._user)
.futureBox.toFutureOption.toFoxseems to work
tools/postgres/schema.sql
Outdated
| CREATE TABLE webknossos.aiModels( | ||
| _id CHAR(24) PRIMARY KEY, | ||
| _organization VARCHAR(256) NOT NULL, | ||
| _owningOrganization VARCHAR(256) NOT NULL, |
There was a problem hiding this comment.
I see your point, and ultimately it’s not that important. But I still think that if the organizations the model is shared with were called webknossos.aiModel_sharedOrganizations (which would be consistent with sharedTeams and more descriptive than just organizations), the semantics of _organization would be fairly clear. This is the organization the aiModel lives in / belongs to, just like with users, datasets, teams etc..
I adjusted the code to implicitly add the owning orga to the shared orgas. This was your initial intended solution, correct? Please have another look. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
CHANGELOG.unreleased.md (1)
15-15: Ensure proper spelling/formatting for “superusers”.
Static analysis indicates “Super users” is typically written as a single word (e.g., “superusers”) or hyphenated (“super-users”).Consider applying this diff to fix the style:
- - Super users can now share the trained AI models... + - Superusers can now share the trained AI models...🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: This is normally spelled as one word.
Context: ...//pull/8447/) - Super users can now share the trained AI models wit...(SUPER_COMPOUNDS)
app/models/aimodels/AiModel.scala (2)
46-68: Check performance implications of fetching all organizations.
While this logic works for superusers, consider if retrieving the full list of organizations (organizationDAO.findAll) might be a bottleneck or security concern in larger deployments. Also ensure that superusers indeed require access to all organizations.
208-214: Updates in a single transaction look good; consider concurrency checks.
Deleting and reinserting associations in one transaction avoids partial writes. However, consider whether concurrency checks or versioning might be needed if multiple users update sharing simultaneously.app/controllers/AiModelController.scala (1)
57-57: OptionalsharedOrganizationIdsis reasonable.
Consider whether a default empty list might simplify usage if no shared organizations are specified. Otherwise, this is fine as is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.unreleased.md(1 hunks)app/controllers/AiModelController.scala(7 hunks)app/controllers/InitialDataController.scala(1 hunks)app/models/aimodels/AiModel.scala(8 hunks)conf/evolutions/128-allow-ai-model-sharing.sql(1 hunks)conf/evolutions/reversions/128-allow-ai-model-sharing.sql(1 hunks)tools/postgres/schema.sql(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- conf/evolutions/reversions/128-allow-ai-model-sharing.sql
- app/controllers/InitialDataController.scala
- conf/evolutions/128-allow-ai-model-sharing.sql
- tools/postgres/schema.sql
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~15-~15: This is normally spelled as one word.
Context: ...//pull/8447/) - Super users can now share the trained AI models wit...
(SUPER_COMPOUNDS)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (6)
app/models/aimodels/AiModel.scala (6)
10-10: Import changes look good.
No issues detected with the newly introduced import ofUserService.
17-19: Imports forFoxandOrganizationDAOare fine.
These imports support the new logic for optional job retrieval and organization access.
43-43: Injection oforganizationDAOis appropriate.
No concerns regarding constructor injection; this is necessary to retrieve organization data.
73-73: Use ofisOwnedByUsersOrganizationis clear.
This boolean helps clarify whether the requesting user fully owns the model.
79-79: No issues with exposingcreated.
Exposing creation timestamp is typically safe and useful.
185-194: Good inclusion of owning organization if missing.
This logic prevents the owning organization from accidentally losing access.
| def updateAiModelInfo(aiModelId: ObjectId): Action[UpdateAiModelParameters] = | ||
| sil.SecuredAction.async(validateJson[UpdateAiModelParameters]) { implicit request => | ||
| for { | ||
| _ <- userService.assertIsSuperUser(request.identity) | ||
| aiModel <- aiModelDAO.findOne(aiModelId) ?~> "aiModel.notFound" ~> NOT_FOUND | ||
| _ <- aiModelDAO.updateOne( | ||
| aiModel.copy(name = request.body.name, comment = request.body.comment, modified = Instant.now)) | ||
| updatedAiModel <- aiModelDAO.findOne(aiModelId) ?~> "aiModel.notFound" ~> NOT_FOUND | ||
| jsResult <- aiModelService.publicWrites(updatedAiModel) | ||
| } yield Ok(jsResult) | ||
| { | ||
| // Automatically add the owning organization to the shared organizations to ensure it is impossible for the owning organization to loose access.. | ||
| val sharedOrganizationIdsOpt = request.body.sharedOrganizationIds.map { sharedOrganizationIds => | ||
| if (!sharedOrganizationIds.contains(request.identity._organization)) { | ||
| sharedOrganizationIds :+ request.identity._organization | ||
| } else sharedOrganizationIds | ||
| } | ||
| for { | ||
| _ <- userService.assertIsSuperUser(request.identity) | ||
| aiModel <- aiModelDAO.findOne(aiModelId) ?~> "aiModel.notFound" ~> NOT_FOUND | ||
| _ <- bool2Fox(aiModel._organization == request.identity._organization) ?~> "aiModel.notOwned" | ||
| _ <- aiModelDAO.updateOne(aiModel.copy(name = request.body.name, | ||
| comment = request.body.comment, | ||
| modified = Instant.now)) ?~> "aiModel.updatingFailed" | ||
| areSharedOrganizationsUpdated = sharedOrganizationIdsOpt | ||
| .getOrElse(List()) | ||
| .toSet != aiModel._sharedOrganizations.toSet | ||
| sharedAndPreservedOrganizationIdsOpt <- if (areSharedOrganizationsUpdated) | ||
| Fox.runOptional(sharedOrganizationIdsOpt) { newlySharedOrganizationIds => | ||
| // Keep organizations with access to the aiModel which the current user has no access to. | ||
| for { | ||
| organizationIdsUserCanAccess <- organizationDAO.findAll.map(os => os.map(_._id)) | ||
| organizationsToPreserveAccessTo = aiModel._sharedOrganizations.filter( | ||
| !organizationIdsUserCanAccess.contains(_)) | ||
| } yield newlySharedOrganizationIds ++ organizationsToPreserveAccessTo | ||
| } else Fox.successful(None) | ||
| _ <- Fox.runOptional(sharedAndPreservedOrganizationIdsOpt)( | ||
| orgas => | ||
| if (orgas.toSet == aiModel._sharedOrganizations.toSet) Fox.successful(()) | ||
| else aiModelDAO.updateSharedOrganizations(aiModel._id, orgas)) ?~> "aiModel.updatingSharedFailed" | ||
| updatedAiModel <- aiModelDAO.findOne(aiModelId) ?~> "aiModel.notFound" ~> NOT_FOUND | ||
| js <- aiModelService.publicWrites(updatedAiModel, request.identity) | ||
| } yield Ok(js) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify model and shared-org updates in a single transaction.
Currently, _updateOne and updateSharedOrganizations are separate DB operations. If the second operation fails, the name/comment change still proceeds. Consider wrapping both in a single transaction to avoid partial updates.
|
frontend also looks good 👍 by the way, I "resolved" the remaining comments that showed up as "outdated". probably makes more sense for you to do this in the future, @MichaelBuessemeyer, because you have a better overview as the PR author :) |
Nope, everything is alrighty 😸 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
16-16: Improve Consistency: Spell “superuser” as a single word.Static analysis suggests that “super users” is normally written as “Superusers” in technical documentation. For consistency with related entries (like line 17) and common industry terminology, please update line 16 accordingly.
- - Super users can now share the trained AI models with other organizations. [#8418](https://github.com/scalableminds/webknossos/pull/8418) + - Superusers can now share the trained AI models with other organizations. [#8418](https://github.com/scalableminds/webknossos/pull/8418)🧰 Tools
🪛 LanguageTool
[grammar] ~16-~16: This is normally spelled as one word.
Context: ...//pull/8447/) - Super users can now share the trained AI models wit...(SUPER_COMPOUNDS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)app/controllers/AiModelController.scala(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/AiModelController.scala
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~16-~16: This is normally spelled as one word.
Context: ...//pull/8447/) - Super users can now share the trained AI models wit...
(SUPER_COMPOUNDS)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)