Skip to content

perf(rich_workspace): only add property for parent #5963

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.

Sign up for GitHub

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

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

perf(rich_workspace): only add property for parent #5963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
74f6862
perf(rich_workspace): only add property for parent
ShGKme Jun 27, 2024
0517913
test(rich_workspace): never add rich workspace property to nesteds
ShGKme Jun 28, 2024
be1604f
fix: Add extra property to fetch only the top most workspace
juliusknorr Apr 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 42 additions & 20 deletions cypress/e2e/propfind.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -24,37 +27,56 @@ 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, '')
})

// 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 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('have.property', richWorkspace, '## Hello world\n')
.should('not.property', PROPERTY_WORKSPACE, 'Hello world\n')
})

})

describe('with workspaces disabled', function() {
Expand All @@ -67,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)
})

})
Expand Down
16 changes: 5 additions & 11 deletions cypress/support/commands.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
42 changes: 32 additions & 10 deletions lib/DAV/WorkspacePlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}

Expand All @@ -78,15 +83,26 @@ public function propFind(PropFind $propFind, INode $node) {
return;
}

$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;
}

$node = $node->getNode();
try {
$file = $this->workspaceService->getFile($node);
} catch (\Exception $e) {
$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) {
$workspaceContentCallback = function () use ($file) {
$cachedContent = '';
if ($file instanceof File) {
$cache = $this->cacheFactory->createDistributed('text_workspace');
Expand All @@ -110,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);
}
}
10 changes: 5 additions & 5 deletions src/helpers/files.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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']
},
})

Expand Down
4 changes: 2 additions & 2 deletions src/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@

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' })

Check warning on line 15 in src/init.js

View check run for this annotation

Codecov / codecov/patch

src/init.js#L14-L15 

Added lines #L14 - L15 were not covered by tests

if (workspaceAvailable) {
addMenuRichWorkspace()
Expand Down
Loading