Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize getSharedWith to fetch permissions with initial query instead of one per card #6114

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jul 15, 2024

  • fix: Add method to map board to file permissions
  • perf(sharing): Optimize getSharedWith to fetch permissions right away
  • fix: Chunk query for getting labels for cards
  • Resolves: #
  • Target version: main

Summary

--- query_before	2024-07-15 20:34:24
+++ query_after	2024-07-15 20:34:06
@@ -7,7 +7,6 @@
 primary  0 SELECT `uid`, `displayname`, `password` FROM `oc_users` WHERE `uid_lower` = :dcValue1
 primary  0 SELECT `provider_id`, `enabled` FROM `oc_twofactor_providers` WHERE `uid` = :dcValue1
 primary  0 SELECT `appid`, `configkey`, `configvalue` FROM `oc_preferences` WHERE `userid` = :dcValue1
-primary  0 UPDATE `oc_preferences` SET `configvalue` = :dcValue1 WHERE (`userid` = :dcValue2) AND (`appid` = :dcValue3) AND (`configkey` = :dcValue4)
 primary  0 SELECT * FROM `oc_notifications_settings` WHERE `user_id` = :dcValue1
 primary  0 SELECT * FROM `oc_ldap_group_membership` WHERE `userid` = :dcValue1
 primary  0 SELECT `credentials` FROM `oc_storages_credentials` WHERE (`identifier` = :dcValue1) AND (`user` = :dcValue2)
@@ -34,16 +33,13 @@
 primary  0 SELECT `r`.`token` FROM `oc_talk_attendees` `a` LEFT JOIN `oc_talk_rooms` `r` ON `a`.`room_id` = `r`.`id` WHERE (`a`.`actor_id` = :dcValue1) AND (`a`.`actor_type` = :dcValue2)
 primary  0 SELECT `s`.*, `f`.`fileid`, `f`.`path`, `f`.`permissions` as `f_permissions`, `f`.`storage`, `f`.`path_hash`, `f`.`parent` as `f_parent`, `f`.`name`, `f`.`mimetype`, `f`.`mimepart`, `f`.`size`, `f`.`mtime`, `f`.`storage_mtime`, `f`.`encrypted`, `f`.`unencrypted_size`, `f`.`etag`, `f`.`checksum`, `st`.`id` AS `storage_string_id` FROM `oc_share` `s` LEFT JOIN `oc_filecache` `f` ON `s`.`file_source` = `f`.`fileid` LEFT JOIN `oc_storages` `st` ON `f`.`storage` = `st`.`numeric_id` WHERE (`s`.`share_type` = :dcValue1) AND (`s`.`share_with` IN (:dcValue2)) AND ((`s`.`item_type` = :dcValue3) OR (`s`.`item_type` = :dcValue4)) ORDER BY `s`.`id` ASC
 primary  0 SELECT * FROM `oc_share` WHERE (`share_type` = :dcValue1) AND (`share_with` = :dcValue2) AND ((`item_type` = :dcValue3) OR (`item_type` = :dcValue4))
-primary  0 SELECT DISTINCT `b`.`id` FROM `oc_deck_boards` `b` WHERE `owner` = :dcValue1
-primary  0 SELECT DISTINCT `b`.`id` FROM `oc_deck_boards` `b` INNER JOIN `oc_deck_board_acl` `acl` ON `b`.`id` = `acl`.`board_id` WHERE ((`acl`.`type` = :dcValue1) AND (`acl`.`participant` = :dcValue2)) OR ((`acl`.`type` = :dcValue3) AND (`acl`.`participant` IN (:dcValue4))) OR ((`acl`.`type` = :dcValue5) AND (`acl`.`participant` IN (:dcValue6)))
-primary  0 SELECT `s`.*, `f`.`fileid`, `f`.`path`, `f`.`permissions` as `f_permissions`, `f`.`storage`, `f`.`path_hash`, `f`.`parent` as `f_parent`, `f`.`name`, `f`.`mimetype`, `f`.`mimepart`, `f`.`size`, `f`.`mtime`, `f`.`storage_mtime`, `f`.`encrypted`, `f`.`unencrypted_size`, `f`.`etag`, `f`.`checksum`, `st`.`id` AS `storage_string_id` FROM `oc_share` `s` LEFT JOIN `oc_filecache` `f` ON `s`.`file_source` = `f`.`fileid` LEFT JOIN `oc_deck_cards` `dc` ON CAST(`dc`.`id` AS CHAR) = `s`.`share_with` LEFT JOIN `oc_storages` `st` ON `f`.`storage` = `st`.`numeric_id` LEFT JOIN `oc_deck_stacks` `ds` ON `dc`.`stack_id` = `ds`.`id` LEFT JOIN `oc_deck_boards` `db` ON `ds`.`board_id` = `db`.`id` WHERE (`s`.`share_type` = :dcValue1) AND (`db`.`id` IN (:dcValue2)) AND ((`s`.`item_type` = :dcValue3) OR (`s`.`item_type` = :dcValue4)) ORDER BY `s`.`id` ASC
+primary  0 SELECT `id`, `title`, `owner`, `color`, `archived`, `deleted_at`, `last_modified` FROM `oc_deck_boards` `b` WHERE `owner` = :dcValue1 ORDER BY `b`.`id` ASC
+primary  0 SELECT `b`.`id`, `title`, `owner`, `color`, `archived`, `deleted_at`, `last_modified` FROM `oc_deck_boards` `b` INNER JOIN `oc_deck_board_acl` `acl` ON `b`.`id` = `acl`.`board_id` WHERE (`acl`.`participant` = :dcValue2) AND (`acl`.`type` = :dcValue3) AND (`b`.`owner` <> :dcValue4) ORDER BY `b`.`id` ASC
+primary  0 SELECT `b`.`id`, `title`, `owner`, `color`, `archived`, `deleted_at`, `last_modified` FROM `oc_deck_boards` `b` INNER JOIN `oc_deck_board_acl` `acl` ON `b`.`id` = `acl`.`board_id` WHERE (`acl`.`type` = :dcValue1) AND (`b`.`owner` <> :dcValue2) AND ((`acl`.`participant` = :dcValue3) OR (`acl`.`participant` = :dcValue4)) ORDER BY `b`.`id` ASC
+primary  0 SELECT `b`.`id`, `title`, `owner`, `color`, `archived`, `deleted_at`, `last_modified` FROM `oc_deck_boards` `b` INNER JOIN `oc_deck_board_acl` `acl` ON `b`.`id` = `acl`.`board_id` WHERE (`acl`.`type` = :dcValue1) AND (`b`.`owner` <> :dcValue2) AND ((`acl`.`participant` = :dcValue3) OR (`acl`.`participant` = :dcValue4)) ORDER BY `b`.`id` ASC
+primary  0 SELECT `id`, `board_id`, `type`, `participant`, `permission_edit`, `permission_share`, `permission_manage` FROM `oc_deck_board_acl` WHERE `board_id` IN (:boardIds)
+primary  0 SELECT `s`.*, `f`.`fileid`, `f`.`path`, `f`.`permissions` as `f_permissions`, `f`.`storage`, `f`.`path_hash`, `f`.`parent` as `f_parent`, `f`.`name`, `f`.`mimetype`, `f`.`mimepart`, `f`.`size`, `f`.`mtime`, `f`.`storage_mtime`, `f`.`encrypted`, `f`.`unencrypted_size`, `f`.`etag`, `f`.`checksum`, `st`.`id` AS `storage_string_id`, `db`.`id` AS `board_id` FROM `oc_share` `s` LEFT JOIN `oc_filecache` `f` ON `s`.`file_source` = `f`.`fileid` LEFT JOIN `oc_deck_cards` `dc` ON CAST(`dc`.`id` AS CHAR) = `s`.`share_with` LEFT JOIN `oc_storages` `st` ON `f`.`storage` = `st`.`numeric_id` LEFT JOIN `oc_deck_stacks` `ds` ON `dc`.`stack_id` = `ds`.`id` LEFT JOIN `oc_deck_boards` `db` ON `ds`.`board_id` = `db`.`id` WHERE (`dc`.`deleted_at` = :dcValue1) AND (`db`.`deleted_at` = :dcValue2) AND (`s`.`share_type` = :dcValue3) AND (`db`.`id` IN (:dcValue4)) AND ((`s`.`item_type` = :dcValue5) OR (`s`.`item_type` = :dcValue6)) ORDER BY `s`.`id` ASC
 primary  0 SELECT * FROM `oc_share` WHERE (`parent` IN (:dcValue1)) AND (`share_with` = :dcValue2) AND ((`item_type` = :dcValue3) OR (`item_type` = :dcValue4))
-primary  0 SELECT * FROM `oc_deck_boards` WHERE (`id` = :dcValue2) AND (`deleted_at` = :dcValue1) ORDER BY `id` ASC
-primary  0 SELECT `id`, `board_id`, `type`, `participant`, `permission_edit`, `permission_share`, `permission_manage` FROM `oc_deck_board_acl` WHERE `board_id` = :dcValue1
-primary  0 SELECT `id`, `board_id`, `type`, `participant`, `permission_edit`, `permission_share`, `permission_manage` FROM `oc_deck_board_acl` WHERE `board_id` = :dcValue1
-primary  0 SELECT * FROM `oc_deck_cards` WHERE `id` = :dcValue1 ORDER BY `order` ASC, `id` ASC
-primary  0 SELECT * FROM `oc_deck_cards` WHERE `id` = :dcValue1 ORDER BY `order` ASC, `id` ASC
-primary  0 SELECT * FROM `oc_deck_cards` WHERE `id` = :dcValue1 ORDER BY `order` ASC, `id` ASC
 primary  0 SELECT `storage_id`, `root_id`, `user_id`, `mount_point`, `mount_id`, `mount_provider_class` FROM `oc_mounts` `m` WHERE `user_id` = ?
 primary  1 INSERT INTO `oc_mounts` (`storage_id`,`root_id`,`user_id`,`mount_point`,`mount_id`,`mount_provider_class`) SELECT ?,?,?,?,?,? FROM `oc_mounts` WHERE `root_id` = ? AND `user_id` = ? AND `mount_point` = ? HAVING COUNT(*) = 0
 primary  1 INSERT INTO `oc_mounts` (`storage_id`,`root_id`,`user_id`,`mount_point`,`mount_id`,`mount_provider_class`) SELECT ?,?,?,?,?,? FROM `oc_mounts` WHERE `root_id` = ? AND `user_id` = ? AND `mount_point` = ? HAVING COUNT(*) = 0

TODO

  • ...

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

$share->setPermissions($this->permissionService->boardToFilePermission($shareToPermissionMap[$cardId]));
return;
}

try {
$this->permissionService->checkPermission($this->cardMapper, $share->getSharedWith(), Acl::PERMISSION_EDIT, $userId);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the previously expensive part as it needs to:

  • find the board id for the card which we already have in the result of the query that fetches all shares
  • fetches the permissions for each board (N+1) which can be optimized by only fetching once we have all board ids
  • Then a map from card id to the related board permissions is built and passed along to the logic above to actually build the file permissions

@luka-nextcloud
Copy link
Contributor

@juliushaertl I think cypress/integration tests are failing. :)

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

Tests should pass now.

Copy link
Contributor

🐢 Performance warning.
It looks like the query count of the integration tests increased with this PR.
Database query count is now 71634 was 70520 (+1.57%)
Please check your code again. If you added a new test this can be expected and the base value in tests/integration/base-query-count.txt can be increased.

Copy link
Contributor

🐢 Performance warning.
It looks like the query count of the integration tests increased with this PR.
Database query count is now 71634 was 70520 (+1.57%)
Please check your code again. If you added a new test this can be expected and the base value in tests/integration/base-query-count.txt can be increased.

@juliusknorr
Copy link
Member Author

Query number increase is coming from the change of using BoardMapper:findAllForUser instead of BoardMapper::findBoardIds which does one more query then the other method.
Screenshot 2024-12-19 at 18 24 21

Might be worth to double check, however the noticable query saving does not happen in the integration tests as we mostly share one card with an attachment to another user.

Needs checking if we can still optimize the above part

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants