From 74f68627661e76ce2315ac1f0ab459469be84583 Mon Sep 17 00:00:00 2001 From: "Grigorii K. Shartsev" Date: Thu, 27 Jun 2024 15:55:04 +0200 Subject: [PATCH 1/3] perf(rich_workspace): only add property for parent Signed-off-by: Grigorii K. Shartsev --- lib/DAV/WorkspacePlugin.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/DAV/WorkspacePlugin.php b/lib/DAV/WorkspacePlugin.php index b7cd454bf1b..c115ab83c61 100644 --- a/lib/DAV/WorkspacePlugin.php +++ b/lib/DAV/WorkspacePlugin.php @@ -78,6 +78,12 @@ public function propFind(PropFind $propFind, INode $node) { return; } + // Only return the property for the parent node and ignore it for further in depth nodes + // Otherwise requesting parent with description files for all the children makes a huge performance impact for external storages children + if ($propFind->getDepth() !== $this->server->getHTTPDepth()) { + return; + } + $node = $node->getNode(); try { $file = $this->workspaceService->getFile($node); @@ -85,7 +91,6 @@ public function propFind(PropFind $propFind, INode $node) { $file = null; } - // Only return the property for the parent node and ignore it for further in depth nodes $propFind->handle(self::WORKSPACE_PROPERTY, function () use ($file) { $cachedContent = ''; if ($file instanceof File) { From 0517913fd3b3e44f9c7cb8dd587bf189cc270b0e Mon Sep 17 00:00:00 2001 From: "Grigorii K. Shartsev" Date: Fri, 28 Jun 2024 14:21:22 +0200 Subject: [PATCH 2/3] test(rich_workspace): never add rich workspace property to nesteds Signed-off-by: Grigorii K. Shartsev --- cypress/e2e/propfind.spec.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cypress/e2e/propfind.spec.js b/cypress/e2e/propfind.spec.js index 7b53b9308bf..04a83d6a303 100644 --- a/cypress/e2e/propfind.spec.js +++ b/cypress/e2e/propfind.spec.js @@ -40,8 +40,7 @@ describe('Text PROPFIND extension ', function() { .should('have.property', richWorkspace, '') }) - // Android app relies on this when navigating nested folders - it('adds rich workspace property to nested folders', function() { + it('never adds rich workspace property to nested folders', function() { cy.createFolder('/workspace') // FIXME: Ideally we do not need a page context for those tests at all // For now the dashboard avoids that we have failing requests due to conflicts when updating the file @@ -52,9 +51,8 @@ describe('Text PROPFIND extension ', function() { cy.uploadFile('test.md', 'text/markdown', '/workspace/Readme.md') cy.propfindFolder('/', 1) .then(results => results.pop().propStat[0].properties) - .should('have.property', richWorkspace, '## Hello world\n') + .should('not.have.property', richWorkspace) }) - }) describe('with workspaces disabled', function() { From be1604f8a14af49fe3d6e7aa76e78994c1d57fd3 Mon Sep 17 00:00:00 2001 From: Julius Knorr Date: Wed, 9 Apr 2025 22:51:02 +0200 Subject: [PATCH 3/3] fix: Add extra property to fetch only the top most workspace Signed-off-by: Julius Knorr --- cypress/e2e/propfind.spec.js | 60 +++++++++++++++++++++++++----------- cypress/support/commands.js | 16 +++------- lib/DAV/WorkspacePlugin.php | 41 ++++++++++++++++-------- src/helpers/files.js | 10 +++--- src/init.js | 4 +-- 5 files changed, 83 insertions(+), 48 deletions(-) diff --git a/cypress/e2e/propfind.spec.js b/cypress/e2e/propfind.spec.js index 04a83d6a303..65139bacb0e 100644 --- a/cypress/e2e/propfind.spec.js +++ b/cypress/e2e/propfind.spec.js @@ -8,7 +8,10 @@ import { randUser } from '../utils/index.js' const user = randUser() describe('Text PROPFIND extension ', function() { - const richWorkspace = '{http://nextcloud.org/ns}rich-workspace' + const PROPERTY_WORKSPACE = '{http://nextcloud.org/ns}rich-workspace' + const PROPERTY_WORKSPACE_FILE = '{http://nextcloud.org/ns}rich-workspace-file' + const PROPERTY_WORKSPACE_FLAT = '{http://nextcloud.org/ns}rich-workspace-flat' + const PROPERTY_WORKSPACE_FILE_FLAT = '{http://nextcloud.org/ns}rich-workspace-file-flat' before(function() { cy.createUser(user) @@ -24,34 +27,55 @@ describe('Text PROPFIND extension ', function() { cy.configureText('workspace_enabled', 1) }) - // Android app relies on this to detect rich workspace availability it('always adds rich workspace property', function() { + const properties = [PROPERTY_WORKSPACE_FLAT, PROPERTY_WORKSPACE_FILE_FLAT] cy.uploadFile('empty.md', 'text/markdown', '/Readme.md') // FIXME: Ideally we do not need a page context for those tests at all // For now the dashboard avoids that we have failing requests due to conflicts when updating the file cy.visit('/apps/dashboard') - cy.propfindFolder('/') - .should('have.property', richWorkspace, '') + cy.propfindFolder('/', 0, properties) + .should('have.property', PROPERTY_WORKSPACE_FLAT, '') cy.uploadFile('test.md', 'text/markdown', '/Readme.md') - cy.propfindFolder('/') - .should('have.property', richWorkspace, '## Hello world\n') + cy.propfindFolder('/', 0, properties) + .should('have.property', PROPERTY_WORKSPACE_FLAT, '## Hello world\n') cy.deleteFile('/Readme.md') - cy.propfindFolder('/') - .should('have.property', richWorkspace, '') + cy.propfindFolder('/', 0, properties) + .should('have.property', PROPERTY_WORKSPACE_FLAT, '') }) - it('never adds rich workspace property to nested folders', function() { + it('never adds rich workspace property to nested folders for flat properties', function() { + const properties = [PROPERTY_WORKSPACE_FLAT, PROPERTY_WORKSPACE_FILE_FLAT] + // FIXME: Ideally we do not need a page context for those tests at all + // For now the dashboard avoids that we have failing requests due to conflicts when updating the file + cy.visit('/apps/dashboard') + cy.createFolder('/workspace-flat') + cy.propfindFolder('/', 1, properties) + .then(results => results.pop().propStat[0].properties) + .should('have.property', PROPERTY_WORKSPACE_FLAT, '') + cy.uploadFile('test.md', 'text/markdown', '/workspace-flat/Readme.md') + cy.propfindFolder('/', 1, properties) + .then(results => results.pop().propStat[0].properties) + .should('not.have.property', PROPERTY_WORKSPACE_FLAT) + cy.deleteFile('/workspace-flat/Readme.md') + cy.propfindFolder('/', 1, properties) + .then(results => results.pop().propStat[0].properties) + .should('have.property', PROPERTY_WORKSPACE_FLAT, '') + }) + + // Android app relies on this to detect rich workspace availability in subfolders properly + it('adds rich workspace property to nested folders for the default properties', function() { + const properties = [PROPERTY_WORKSPACE, PROPERTY_WORKSPACE_FILE] cy.createFolder('/workspace') // FIXME: Ideally we do not need a page context for those tests at all // For now the dashboard avoids that we have failing requests due to conflicts when updating the file cy.visit('/apps/dashboard') - cy.propfindFolder('/', 1) + cy.propfindFolder('/', 1, properties) .then(results => results.pop().propStat[0].properties) - .should('have.property', richWorkspace, '') + .should('have.property', PROPERTY_WORKSPACE, '') cy.uploadFile('test.md', 'text/markdown', '/workspace/Readme.md') - cy.propfindFolder('/', 1) + cy.propfindFolder('/', 1, properties) .then(results => results.pop().propStat[0].properties) - .should('not.have.property', richWorkspace) + .should('not.property', PROPERTY_WORKSPACE, 'Hello world\n') }) }) @@ -65,15 +89,15 @@ describe('Text PROPFIND extension ', function() { // FIXME: Ideally we do not need a page context for those tests at all // For now the dashboard avoids that we have failing requests due to conflicts when updating the file cy.visit('/apps/dashboard') - cy.propfindFolder('/') - .should('not.have.property', richWorkspace) + cy.propfindFolder('/', 1, [PROPERTY_WORKSPACE_FLAT, PROPERTY_WORKSPACE_FILE_FLAT]) + .should('not.have.property', PROPERTY_WORKSPACE_FLAT) cy.uploadFile('test.md', 'text/markdown', '/Readme.md') - cy.propfindFolder('/') - .should('not.have.property', richWorkspace) + cy.propfindFolder('/', 1, [PROPERTY_WORKSPACE_FLAT, PROPERTY_WORKSPACE_FILE_FLAT]) + .should('not.have.property', PROPERTY_WORKSPACE_FLAT) cy.createFolder('/without-workspace') cy.propfindFolder('/', 1) .then(results => results.pop().propStat[0].properties) - .should('not.have.property', richWorkspace) + .should('not.have.property', PROPERTY_WORKSPACE_FLAT) }) }) diff --git a/cypress/support/commands.js b/cypress/support/commands.js index 2b3df5d4a29..e986edbbaab 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -225,21 +225,15 @@ Cypress.Commands.add('getFileContent', (path) => { .then(response => response.data) }) -Cypress.Commands.add('propfindFolder', (path, depth = 0) => { +Cypress.Commands.add('propfindFolder', (path, depth = 0, properties = []) => { return cy.window(silent) .then(win => { const files = win.OC.Files - const PROPERTY_WORKSPACE_FILE - = `{${files.Client.NS_NEXTCLOUD}}rich-workspace-file` - const PROPERTY_WORKSPACE - = `{${files.Client.NS_NEXTCLOUD}}rich-workspace` - const properties = [ - ...files.getClient().getPropfindProperties(), - PROPERTY_WORKSPACE_FILE, - PROPERTY_WORKSPACE, - ] const client = files.getClient().getClient() - return client.propFind(client.baseUrl + path, properties, depth) + return client.propFind(client.baseUrl + path, [ + ...properties, + ...files.getClient().getPropfindProperties(), + ], depth) .then((results) => { cy.log(`Propfind returned ${results.status}`) if (depth) { diff --git a/lib/DAV/WorkspacePlugin.php b/lib/DAV/WorkspacePlugin.php index c115ab83c61..a89c2d78087 100644 --- a/lib/DAV/WorkspacePlugin.php +++ b/lib/DAV/WorkspacePlugin.php @@ -29,9 +29,10 @@ class WorkspacePlugin extends ServerPlugin { public const WORKSPACE_PROPERTY = '{http://nextcloud.org/ns}rich-workspace'; public const WORKSPACE_FILE_PROPERTY = '{http://nextcloud.org/ns}rich-workspace-file'; + public const WORKSPACE_PROPERTY_FLAT = '{http://nextcloud.org/ns}rich-workspace-flat'; + public const WORKSPACE_FILE_PROPERTY_FLAT = '{http://nextcloud.org/ns}rich-workspace-file-flat'; - /** @var Server */ - private $server; + private Server $server; public function __construct( private WorkspaceService $workspaceService, @@ -62,8 +63,12 @@ public function initialize(Server $server) { public function propFind(PropFind $propFind, INode $node) { - if (!in_array(self::WORKSPACE_PROPERTY, $propFind->getRequestedProperties()) - && !in_array(self::WORKSPACE_FILE_PROPERTY, $propFind->getRequestedProperties())) { + if (!array_intersect([ + self::WORKSPACE_PROPERTY, + self::WORKSPACE_FILE_PROPERTY, + self::WORKSPACE_PROPERTY_FLAT, + self::WORKSPACE_FILE_PROPERTY_FLAT + ], $propFind->getRequestedProperties())) { return; } @@ -78,9 +83,16 @@ public function propFind(PropFind $propFind, INode $node) { return; } - // Only return the property for the parent node and ignore it for further in depth nodes - // Otherwise requesting parent with description files for all the children makes a huge performance impact for external storages children - if ($propFind->getDepth() !== $this->server->getHTTPDepth()) { + $shouldFetchChildren = array_intersect([ + self::WORKSPACE_PROPERTY, + self::WORKSPACE_FILE_PROPERTY, + ], $propFind->getRequestedProperties()); + + // In most cases we only need the workspace property for the root node + // So we can skip the propFind for further nodes for performance reasons + // Fetching the workspace property for all children is still required for mobile apps + + if ($propFind->getDepth() !== $this->server->getHTTPDepth() && !$shouldFetchChildren) { return; } @@ -90,8 +102,7 @@ public function propFind(PropFind $propFind, INode $node) { } catch (\Exception $e) { $file = null; } - - $propFind->handle(self::WORKSPACE_PROPERTY, function () use ($file) { + $workspaceContentCallback = function () use ($file) { $cachedContent = ''; if ($file instanceof File) { $cache = $this->cacheFactory->createDistributed('text_workspace'); @@ -115,12 +126,18 @@ public function propFind(PropFind $propFind, INode $node) { } } return $cachedContent; - }); - $propFind->handle(self::WORKSPACE_FILE_PROPERTY, function () use ($file) { + }; + + $workspaceFileCallback = function () use ($file) { if ($file instanceof File) { return $file->getFileInfo()->getId(); } return ''; - }); + }; + + $propFind->handle(self::WORKSPACE_PROPERTY, $workspaceContentCallback); + $propFind->handle(self::WORKSPACE_PROPERTY_FLAT, $workspaceContentCallback); + $propFind->handle(self::WORKSPACE_FILE_PROPERTY, $workspaceFileCallback); + $propFind->handle(self::WORKSPACE_FILE_PROPERTY_FLAT, $workspaceFileCallback); } } diff --git a/src/helpers/files.js b/src/helpers/files.js index 141df4cfa4a..06c637792f0 100644 --- a/src/helpers/files.js +++ b/src/helpers/files.js @@ -119,7 +119,7 @@ export const addMenuRichWorkspace = () => { displayName: t('text', 'Add folder description'), category: NewMenuEntryCategory.Other, enabled(context) { - if (Number(context.attributes['rich-workspace-file'])) { + if (Number(context.attributes['rich-workspace-file-flat'])) { return false } return (context.permissions & Permission.CREATE) !== 0 @@ -180,9 +180,9 @@ export const FilesWorkspaceHeader = new Header({ vm.$destroy() vm = null } - const hasRichWorkspace = !!folder.attributes['rich-workspace-file'] || !!newWorkspaceCreated + const hasRichWorkspace = !!folder.attributes['rich-workspace-file-flat'] || !!newWorkspaceCreated const path = newWorkspaceCreated ? dirname(newWorkspaceCreated.path) : folder.path - const content = newWorkspaceCreated ? '' : folder.attributes['rich-workspace'] + const content = newWorkspaceCreated ? '' : folder.attributes['rich-workspace-flat'] newWorkspaceCreated = false @@ -220,10 +220,10 @@ export const FilesWorkspaceHeader = new Header({ // removing the rendered element from the DOM // This is only relevant if switching to a folder that has no content as then the render function is not called - const hasRichWorkspace = !!folder.attributes['rich-workspace-file'] + const hasRichWorkspace = !!folder.attributes['rich-workspace-file-flat'] vm.path = folder.path vm.hasRichWorkspace = hasRichWorkspace - vm.content = folder.attributes['rich-workspace'] + vm.content = folder.attributes['rich-workspace-flat'] }, }) diff --git a/src/init.js b/src/init.js index 087df50b2b4..acbb1f53361 100644 --- a/src/init.js +++ b/src/init.js @@ -11,8 +11,8 @@ import 'vite/modulepreload-polyfill' const workspaceAvailable = loadState('text', 'workspace_available') -registerDavProperty('nc:rich-workspace', { nc: 'http://nextcloud.org/ns' }) -registerDavProperty('nc:rich-workspace-file', { nc: 'http://nextcloud.org/ns' }) +registerDavProperty('nc:rich-workspace-flat', { nc: 'http://nextcloud.org/ns' }) +registerDavProperty('nc:rich-workspace-file-flat', { nc: 'http://nextcloud.org/ns' }) if (workspaceAvailable) { addMenuRichWorkspace()