feat(tus): return etag and permissions after the last TUS chunk#666
feat(tus): return etag and permissions after the last TUS chunk#666michaelstingl wants to merge 1 commit into
Conversation
The finalizing PATCH of a chunked TUS upload returned only the file id, so clients had to issue a follow-up PROPFIND to learn the new etag. Register tusd's PreFinishResponseCallback to stat the finalized resource and attach its etag and WebDAV permissions, the same header set the ocdav gateway already produces for the creation-with-upload path. The data path is authenticated by the reva transfer token, which carries no user identity, so the stat restores the upload's executant into the context first, the same way the decomposedfs upload session does. Fixes: opencloud-eu/opencloud#2409
Would you mind sharing what you have in mind here? How would that "small storage interface" look like? |
There was a problem hiding this comment.
If you would prefer the etag be surfaced through a small storage interface instead of reconstructing the executant in the datatx layer, I am happy to reshape it.
Somehow this sounds a bit cleaner. The current implementation relies on some keys being present in the uploads Fileinfo.MetaData and Fileinfo.Storage which AFAICT might be specific to the storagedriver. Even if we currently only have the decomposedfs/posixfs driver, I'd rather have the datatx layer not depend on that.
Also, please rebase on lastest main
| // derive the WebDAV permissions the same way the ocdav gateway does for the creation-with-upload path | ||
| isPublic := false | ||
| if o := ri.GetOpaque(); o != nil && o.Map != nil { | ||
| if e := o.Map["link-share"]; e != nil && e.Decoder == "json" { | ||
| ls := &link.PublicShare{} | ||
| _ = json.Unmarshal(e.Value, ls) | ||
| isPublic = ls != nil | ||
| } | ||
| } | ||
| isShared := !net.IsCurrentUserOwnerOrManager(ctx, ri.GetOwner(), ri) | ||
| role := conversions.RoleFromResourcePermissions(ri.GetPermissionSet(), isPublic) | ||
| resp.Header[net.HeaderOCPermissions] = role.WebDAVPermissions( | ||
| ri.GetType() == provider.ResourceType_RESOURCE_TYPE_CONTAINER, | ||
| isShared, | ||
| false, | ||
| isPublic, | ||
| ) |
There was a problem hiding this comment.
This is a verbatim copy of the code from ocdav/tus.go. Please avoid duplication and create a shared helper for this.
|
Reshaped along the lines you suggested, as a separate PR so both approaches stay visible side by side: It moves the executant handling into the storage driver: a small This PR keeps the self-contained version for comparison. Happy to close whichever you prefer. 🤖 drafted with Claude Code, reviewed before submitting. |
2 participants
Description
The response to the finalizing PATCH of a chunked TUS upload carried only the file id, so a client had to issue a follow-up PROPFIND to learn the new etag, an extra round-trip per upload. The creation-with-upload path returns the etag and permissions directly: the ocdav gateway stats the resource and sets
OC-ETag/ETag/OC-Perm(next toOC-FileID,Last-ModifiedandContent-Type) in itsfinishUploadbranch. The chunked path finalizes with a PATCH at the dataprovider, wheresetHeadersset onlyOC-FileID.This registers tusd's
PreFinishResponseCallbackin the dataprovider tus handler (pkg/rhttp/datatx/manager/tus/tus.go) to stat the finalized resource and attachOC-ETag/ETagandOC-Perm. The chunked finalize now returns the same etag and permissions as the creation-with-upload path, so the two upload paths behave consistently. The alignment is deliberately limited to those headers:Last-ModifiedandContent-Typeare not mirrored (clients do not need them at finalize, andContent-Typeon the204finalize would carry no body), and the finalize stays a204, not the gateway's201.The data path is authenticated by the reva transfer token, which carries no user identity, so a plain
GetMDis permission-gated toNotFound. The callback restores the upload's executant into the context before the stat, the same way the decomposedfs upload session does inOcisSession.Context. If you would prefer the etag be surfaced through a small storage interface instead of reconstructing the executant in the datatx layer, I am happy to reshape it.The permission derivation mirrors the gateway's creation-with-upload block verbatim, so the two stay in lockstep. The lookup is best-effort: on a stat error or a nil resource it logs at warn and sets no headers, leaving the PROPFIND fallback intact rather than failing a completed upload.
Related Issue
Motivation and Context
The desktop and Android clients already read the etag from the finalize response when present and PROPFIND otherwise, so the change is backward compatible: older servers keep working, updated ones drop the extra request. Both headers are set because the desktop fast-path needs the etag and the permissions together.
Under async postprocessing the etag at finalize is provisional, exactly as it already is for the creation-with-upload path. The existing
UploadReady-> SSEpostprocessing-finishedflow and tree etag propagation communicate any later change to the client.How Has This Been Tested?
main, built into a local opencloud single binary (7.1.0+dev) via ago.modreplace, Go 1.26go build,gofmt, and the pinnedgolangci-lintv2.10.1 on the changed package: 0 issuespkg/rhttp/datatx/manager/tus/tus_internal_test.go(7 specs) coversexecutantFromUploadInfo(session-key mapping) and the callback with a fakestorage.FSthat records the reference and context user it was called with, asserting the reference is rebuilt from the session keys, the stat runs as the executant (whose identity flips theisSharedmarker), etag mirrored into both headers, best-effort no-op on stat error and on a nil resource, and etag omitted when absentOC-FileID. After:OC-ETag/ETag(equal to the etag a follow-up PROPFIND returns) andOC-Perm, withOC-FileIDunchangedOC_ASYNC_UPLOADS=true; the creation-with-upload path is unchangedScreenshots (if appropriate):
n/a. Server-side response-header change.
Types of changes
Checklist:
storage.FS; covers the callback success/best-effort/no-etag paths and the executant mapping)Type:*label via ready-release-go; no fragment needed)🤖 drafted with Claude Code, reviewed before submitting.