Repository navigation
Conversation
skjnldsv
added
enhancement
2. developing
skjnldsv
added
3. to review
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
- This must go through proper planning, it looks very AI generated, which is not bad but tend to not meet our stability requirements (this API needs to be stable for 5+ years)
- This is missing proper handling of duplicates, what if two apps have the same attribute? How to override one if you need to? How to handle clashes? Maybe keep namespaces (we only do not use them in the frontend because the webdav js library does not use it, but its likely something we need with further growth of Nextcloud)
- This is potential changing API, it must not go straight into OCP but first NCU
I’d forbid it on purpose in v1. I think this is still not something we actually use and for now I don't think we should go for it. If we wanna improve in the future, why not
If you feel more at ease with that, no problem for me :)
This is AI assisted, but not fully generated no. The change is really not that big, stability is not about how it's been coded but about proper reviews, which I've yet to actually see 😉 |
Apps register INodeAttributeProvider with the attributes they serve, in Clark notation. INodeSerializer returns nodes with those attributes in the shape @nextcloud/files uses, so PHP code gets the same file data as a PROPFIND. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: John Molakvoæ <14975046+skjnldsv@users.noreply.github.com>
…providers A Sabre plugin serves the registered attributes on remote.php, /webdav and the public endpoints, preloading collections and search results at once. has-preview and favorite move from FilesPlugin and TagsPlugin to providers, so WebDAV and the serializer share one implementation. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: John Molakvoæ <14975046+skjnldsv@users.noreply.github.com>
346f738 to
defcdf7
Compare
|
|
||
| $fileIds = array_values(array_filter( | ||
| array_map(static fn (Node $node): int => $node->getId(), $nodes), | ||
| fn (int $fileId): bool => !isset($this->favorites[$userId][$fileId]), |
There was a problem hiding this comment.
How can $this->favorites[$userId][$fileId] be set at this point, the only set I see is below on line 47?
Can preload be called several times?
Each provider has to handle its own preloading? Is there no way to upstream this logic?
There was a problem hiding this comment.
Yes it can be called several times in the same request. The serializer always calls it, so two serialize() on the same nodes would reload them, and getAttributes() falls back to preload() if a node wasnt preloaded. Over DAV the plugin already skips them so it never matches there.
The provider has to keep its results anyway between preload() and getAttributes(). The "skip what's already loaded" bit could move up to the serializer/plugin though, I can do that if you prefer 👍
|
It’s hard for me to grasp what this is about. The second commit is an example of migrating an app to the new api or is it more than this? It feels like it’s mixed with implementing the API. It’s an issue that NodeAttributeRegistry is used in OCA while defined in OC, it needs a public API or it should be moved to apps/dav if it’s only used there, not clear to me. |
It's clearer when you read the RFC linked with it :) |
The serializer and the DAV plugin both preload through the registry, which only passes the nodes a provider has not preloaded yet for the same context and attributes. Providers no longer need to skip known nodes or preload lazily from getAttributes(). Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: John Molakvoæ <14975046+skjnldsv@users.noreply.github.com>
Updated the main description. That way it's a bit clearer without reading the rfc 👍 |
Summary
Today only a PROPFIND gives you a complete file:
favorite,has-preview,share-typesetc all come from Sabre plugins. Apps that return files from their own endpoints (Deck, Recommendations, Talk, OpenProject...) each rebuild a partial version, with their own keys.This adds a registry for attribute providers. A generic DAV plugin and a PHP serializer (
INodeSerializer) both read from it, so you get the same data from the same code either way. The new code is the registry, serializer and plugin, the dedup comes as existing plugins move over.First commit is the API, the second moves
has-previewandfavoriteas the first two. Details in the RFC.Worth a look:
favoriteanymore, public endpoints use the share as contextFilesPluginandServerFactoryno longer takeIPreviewNot in here: share attributes (#65400) and
dataToNode()in@nextcloud/files. Psalm not checked locally yet.TODO
Checklist
3. to review, feature component)stable32)AI (if applicable)
👾 This pull request was assisted by Claude Code, commits carry an
Assisted-bytrailer.