perf(rich_workspace): only add property for parent

[ShGKme]

📝 Summary

Fixes a performance regression for instances with many external storages.

Steps to reproduce

1. Add 300-500 s3 external storages and mount to the root subfolder
2. Enable text app
3. See 10-15s loading time on /apps/files/files for PROPFIND request for local s3.
   With external s3, like Amazon, it could be 10 times slower.

That was a regression in 28.

s3	28	27
0	0.1s	0.1s
100	3s	0.2s
200	6s	0.5s
300	9s	0.9s
400	12s	1-2s
500	15s	1.1-2.4s

The problem

There is a new <nc:rich_workspace> property in each Node in the response. To add this property (even empty), text checks for a file in every child folder, which could be an external remote storage.

Having a hundred external storages mounted in one parent, it results in a hundred requests to remotes for the data that is not really required to have in advance.

🚧 TODO

• Only add property for parent ignoring deep nodes as it was in the past

💥 Breaking changes

This breaks the Android Files client a bit—it uses a response from a parent to know about description in children.

Broken scenario:

1. Open root
2. Open a child without README
3. Go back to parent
4. Open a child with README
   • Before: README is shown as a description
   • After: README is not shown and a swipe-reload is required

Alternative solution

Only ignore external storages, which has the most performance impact.

🏁 Checklist

• Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
• Sign-off message is added to all commits
• Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
• Documentation (README or documentation) has been updated or is not required

[juliusknorr]

So the test actually has a comment on this that proofs what we discussed in the call:

text/cypress/e2e/propfind.spec.js

Lines 43 to 56 in 3a9c4f6

	// Android app relies on this when navigating nested folders
	it('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
	cy.visit('/apps/dashboard')
	cy.propfindFolder('/', 1)
	.then(results => results.pop().propStat[0].properties)
	.should('have.property', richWorkspace, '')
	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')
	})

@alperozturk96 Can you keep us posted here on if that is possible to address on Android?

[juliusknorr]

Other failure is unrelated.

[marinofaggiana]

Good to know ! @mpivchev

[juliusknorr]

Discussed with @tobiasKaminsky as we couldn't think of a proper way to make the root only result working with android:

• Check if we can just cache the existence of a rich workspace
• alternative would be to use a special property for web

[ShGKme]

• alternative would be to use a special property for web

Then performance regression is not fixed on Android, having ~10-100 times slower root update than it was in 27

[juliusknorr]

Then performance regression is not fixed on Android, having ~10-100 times slower root update than it was in 27

True, but this should already have been the case before 28. Though only considered as an alternative, caching is probably the most reasonable

[ShGKme]

True, but this should already have been the case before 28. Though only considered as an alternative, caching is probably the most reasonable

Why? It was added only in 28, or I got it wrong?

[juliusknorr]

No, the clients already request this property since a long time. 28 only introduced a change where the web frontend now also fetches those, so the performance issue is also there earlier, just was never noticed.

[ShGKme]

• Check if we can just cache the existence of a rich workspace

Working on this solution

[icewind1991]

Can this be finished as it's preventing workspace from being enabled on c.nc.c

[max-nextcloud]

Can you test the clients with this specific commit e5efd64 ? That would help to decide if we actually need the extra one.

@tobiasKaminsky Could you test? (see #5963 (comment))

[icewind1991]

This is still relevant