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

Cache end to end encrypted paths #316

Merged
merged 3 commits into from
Aug 23, 2022
Merged

Cache end to end encrypted paths #316

merged 3 commits into from
Aug 23, 2022

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Aug 16, 2022

On perftesting this changes the DB requests to determine if a path is end to encrypted from ~900 to 72 with nextcloud/server#33602. Code is inspired by the filesystem tag checks in the workflowengine app.

Todos:

  • Fix unit tests

@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Aug 16, 2022
@CarlSchwan CarlSchwan requested a review from a team August 16, 2022 08:43
@CarlSchwan CarlSchwan self-assigned this Aug 16, 2022
@CarlSchwan CarlSchwan requested review from ArtificialOwl, icewind1991 and skjnldsv and removed request for a team August 16, 2022 08:43
lib/Connector/Sabre/APlugin.php Outdated Show resolved Hide resolved
lib/Connector/Sabre/RedirectRequestPlugin.php Outdated Show resolved Hide resolved
lib/E2EEnabledPathCache.php Outdated Show resolved Hide resolved
lib/E2EEnabledPathCache.php Outdated Show resolved Hide resolved
lib/E2EEnabledPathCache.php Outdated Show resolved Hide resolved
lib/E2EEnabledPathCache.php Outdated Show resolved Hide resolved
lib/E2EEnabledPathCache.php Outdated Show resolved Hide resolved
lib/E2EEnabledPathCache.php Outdated Show resolved Hide resolved
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

code looks better, much more understandable, thanks for updating it

see further comments

lib/E2EEnabledPathCache.php Outdated Show resolved Hide resolved
lib/E2EEnabledPathCache.php Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan force-pushed the cache-is-encrypted branch 4 times, most recently from 51808b2 to 0e5f688 Compare August 18, 2022 13:26
@CarlSchwan CarlSchwan requested a review from juliusknorr August 18, 2022 13:29
@CarlSchwan CarlSchwan force-pushed the cache-is-encrypted branch 3 times, most recently from ca81323 to 3d1f58c Compare August 23, 2022 11:02
composer.json Show resolved Hide resolved
tests/Unit/Connector/Sabre/LockPluginTest.php Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan force-pushed the cache-is-encrypted branch 2 times, most recently from d1c1897 to 0ebdcad Compare August 23, 2022 12:27
- Don't fetch OCP\Files nodes each time but instead use the one in
  $davNode->getNode()

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan CarlSchwan merged commit ef4edd2 into master Aug 23, 2022
@delete-merged-branch delete-merged-branch bot deleted the cache-is-encrypted branch August 23, 2022 13:46
artonge added a commit that referenced this pull request Jan 16, 2025
This is causing performance issues because for non encrypted nodes, we were checking all upper folders in the hierarchy.

Also, this seem unnecessary as top and sub E2EE folders are marked as encrypted, so checking for the current node or its parent folder is enough to have the answer.

Partially introduced by #316 which was an improvement at the time by introducing caching.

Signed-off-by: Louis Chemineau <[email protected]>
artonge added a commit that referenced this pull request Jan 16, 2025
This is causing performance issues because for non encrypted nodes, we were checking all upper folders in the hierarchy.

Also, this seems unnecessary as top and sub E2EE folders are marked as encrypted, so checking for the current node or its parent folder is enough to have the answer.

Partially introduced by #316 which was an improvement at the time by introducing caching.

Signed-off-by: Louis Chemineau <[email protected]>
artonge added a commit that referenced this pull request Jan 16, 2025
This is causing performance issues because for non encrypted nodes, we were checking all upper folders in the hierarchy.

Also, this seems unnecessary as top and sub E2EE folders are marked as encrypted, so checking for the current node or its parent folder is enough to have the answer.

Partially introduced by #316 which was an improvement at the time by introducing caching.

Signed-off-by: Louis Chemineau <[email protected]>
artonge added a commit that referenced this pull request Jan 20, 2025
This is causing performance issues because for non encrypted nodes, we were checking all upper folders in the hierarchy.

Also, this seems unnecessary as top and sub E2EE folders are marked as encrypted, so checking for the current node or its parent folder is enough to have the answer.

Partially introduced by #316 which was an improvement at the time by introducing caching.

Signed-off-by: Louis Chemineau <[email protected]>
artonge added a commit that referenced this pull request Jan 20, 2025
This is causing performance issues because for non encrypted nodes, we were checking all upper folders in the hierarchy.

Also, this seems unnecessary as top and sub E2EE folders are marked as encrypted, so checking for the current node or its parent folder is enough to have the answer.

Partially introduced by #316 which was an improvement at the time by introducing caching.

Signed-off-by: Louis Chemineau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants