Skip to content
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

Jump to bottom

[tests-only] Upload post processing #2963

Merged
merged 12 commits into from
Jul 1, 2022
Merged

[tests-only] Upload post processing #2963

merged 12 commits into from
Jul 1, 2022

Conversation

kobergj
Copy link
Contributor

@kobergj kobergj commented Jun 14, 2022

Introduces post processing of items after upload

@kobergj kobergj force-pushed the UploadPostProcessing branch 12 times, most recently from c2b518c to b2d145e Compare June 27, 2022 08:27
@kobergj kobergj marked this pull request as ready for review June 27, 2022 08:27
@kobergj kobergj requested review from a team, labkode and ishank011 as code owners June 27, 2022 08:27
@kobergj kobergj requested a review from glpatcern as a code owner June 27, 2022 08:31
@kobergj kobergj changed the title [tests-only] Upload post processing Upload post processing Jun 27, 2022
@kobergj kobergj requested a review from C0rby June 27, 2022 13:29
@butonic
Copy link
Contributor

butonic commented Jun 28, 2022

related: owncloud/ocis#214 (comment)

@kobergj
Copy link
Contributor Author

kobergj commented Jun 28, 2022

@butonic should we rename the steps? Not sure if that was a call to action or just an extra info 😄

butonic
butonic reviewed Jun 28, 2022
View reviewed changes
internal/http/services/owncloud/ocdav/propfind/propfind.go Outdated
@@ -708,6 +708,14 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p
Propstat: []PropstatXML{},
}

if status := utils.ReadPlainFromOpaque(md.Opaque, "status"); status == "processing" {
response.Propstat = append(response.Propstat, PropstatXML{
Status: "HTTP/1.1 425 TOO EARLY", // TODO: use proper status code
Copy link
Contributor

@butonic butonic Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, 425 is about replay attacks and the Early-Data header.

IMO [202 Accepted] would be the correct Status for a resource that is still being uploaded:

The 202 (Accepted) status code indicates that the request has been accepted for processing, but the processing has not been completed. The request might or might not eventually be acted upon, as it might be disallowed when processing actually takes place. There is no facility in HTTP for re-sending a status code from an asynchronous operation.

But existing desktop clients might try to download files with a 200 status ... @TheOneRing what would desktop clients do with status 202? ignore the file? would be great for now...

I rember discussing with @dragotin that pre/postprocessing effectively locks the file and 423 Locked is the better Status ... hm although Locked would mean the user can unlock the file, which is not true ... Maybe 202 still is the right response after all? Together with a Location header to a websocket or Long polling endpoint ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure to guess what a 423 will cause.
For everything indicating ok I'd expect a download to be triggered. If you need more info I'd need to dig deeper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in cross platform meeting we will stick with the 425 status code for now. We can revisit that decision later.

@kobergj kobergj changed the base branch from edge to experimental June 28, 2022 09:05
@kobergj
add postprocessing package
b0e1413 
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj
add upload package
837e656 
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj
add require dependency
b8c78be 
Signed-off-by: jkoberg <jkoberg@owncloud.com>

add changelog

Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj
allow async postprocessing of files
56440d5 
Signed-off-by: jkoberg <jkoberg@owncloud.com>

make postprocessing configurable

Signed-off-by: jkoberg <jkoberg@owncloud.com>

set node status only through node pkg

Signed-off-by: jkoberg <jkoberg@owncloud.com>
butonic
butonic requested changes Jun 28, 2022
View reviewed changes
pkg/storage/utils/decomposedfs/decomposedfs.go Outdated Show resolved Hide resolved
pkg/storage/utils/decomposedfs/node/node.go Outdated Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload/upload.go Outdated Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload/upload.go
)

// Tree is used to manage a tree hierarchy
type Tree interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't you use the Tree interface from decomposedfs.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to. But decomposedfs imports upload pkg. So if I import decomposedfs in upload it won't build any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use the tree interface from here in decomposedfs. But I don't see the benefit in that...

@butonic
Copy link
Contributor

butonic commented Jun 28, 2022

@dragotin @kobergj I was trying to find an issue where we decide how to implement this, eg the status to return to clients for uploads that are in progress. If and how to return the current progress in the propfind (or only via graph) etc. It is not an ADR either ... did you discuss and design something off the record?

My find-related-issue-fu only came up with owncloud/ocis#214 (comment), which is why I linked it here. Maybe we can clarify there what clients can expect from the server and how we want to handle this?

@kobergj kobergj changed the title Upload post processing [tests-only] Upload post processing Jun 28, 2022
@kobergj
proper naming
eb447f5 
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj
allow retrieving metadata while processing
3ddae70 
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj
check permissions also for processing nodes
45fde95 
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj
use checksum to calculate etag
0a090e1 
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj
Revert "use checksum to calculate etag"
c6ae303 
This reverts commit 0a090e1.
@kobergj
update modtime only in async case
52745b0 
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj
dont clean processing flag early when async
c3c6070 
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj
copy xattrs to binpath
cec3c2d 
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj kobergj requested a review from butonic June 30, 2022 13:20
@butonic
Copy link
Contributor

butonic commented Jul 1, 2022

cool!

@kobergj kobergj merged commit b001142 into cs3org:experimental Jul 1, 2022
@kobergj kobergj deleted the UploadPostProcessing branch July 1, 2022 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheOneRing TheOneRing TheOneRing left review comments

@butonic butonic butonic approved these changes

@C0rby C0rby C0rby approved these changes

@labkode labkode Awaiting requested review from labkode labkode is a code owner

@ishank011 ishank011 Awaiting requested review from ishank011

@glpatcern glpatcern Awaiting requested review from glpatcern glpatcern is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kobergj @butonic @TheOneRing @C0rby