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.

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

feat: add folder id and etag to dav mkcol responses #4767

Draft
wants to merge 1 commit into
base: edge
Choose a base branch
from

Conversation

JammingBen
Copy link

@JammingBen JammingBen commented Jul 17, 2024

Adds the OC-FileId and the OC-ETag headers to dav mkcol responses.

This is necessary for clients that work with ids and want to fetch a folder immediately after creating.

fixes owncloud/ocis#9618

@JammingBen JammingBen self-assigned this Jul 17, 2024
Adds the `OC-FileId` and the `OC-ETag` headers to dav `mkcol` responses.

This is necessary for clients that work with ids and want to fetch a folder immediately after creating.
@JammingBen JammingBen changed the title feat: add folder id to dav mkcol responses feat: add folder id and etag to dav mkcol responses Jul 17, 2024
@JammingBen JammingBen marked this pull request as ready for review July 17, 2024 10:36
@JammingBen JammingBen requested review from labkode, a team and glpatcern as code owners July 17, 2024 10:36
@kobergj
Copy link
Contributor

kobergj commented Jul 22, 2024

I'm not sure this is the correct approach. We would need another stat call, which will impact performance. We should pass the id with the grpc responses so we don't have to call it again. This needs some refactoring, but I still prefer it over the extra stat call.

@JammingBen
Copy link
Author

I'm not sure this is the correct approach. We would need another stat call, which will impact performance. We should pass the id with the grpc responses so we don't have to call it again. This needs some refactoring, but I still prefer it over the extra stat call.

Sounds reasonable, could you give me a hint where to start for this? :) Or will this be a more complicated task...?

@micbar
Copy link
Member

micbar commented Jul 22, 2024

We can use an opaque property for now

@kobergj
Copy link
Contributor

kobergj commented Jul 22, 2024

Unfortunately not so easy 😢

The decomposedfs method CreateDir does not return anything, hence it cannot return the id: https://github.com/cs3org/reva/blob/edge/pkg/storage/utils/decomposedfs/decomposedfs.go#L682

We would need to change the interface and adjust all other implementations.

@JammingBen
Copy link
Author

Argh ok. Then I'm afraid this is a bit too much for me currently, especially since I don't know how to test all these storage drivers.

@kobergj
Copy link
Contributor

kobergj commented Jul 23, 2024

Just for the records: We have the same issue with Move, Delete and TouchFile. All of them do not return anything and therefore need extra calls to get their id (or similar values)

@kobergj
Copy link
Contributor

kobergj commented Jul 25, 2024

@micbar will you do your IDE magic for the refactor? I volunteer to review it. (Though I'll probably regret that 😉 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants