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

Set proper headers in upload requests #935

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Oct 4, 2023

Currently, when you upload a file, the last modified time is not forwarded to the server, so it becomes the current time.

See https://docs.nextcloud.com/server/latest/developer_manual/client_apis/WebDAV/basic.html#request-headers

Can we merge the destinationFile and headers params?

@artonge artonge self-assigned this Oct 4, 2023
@artonge artonge requested a review from skjnldsv October 4, 2023 09:44
@artonge artonge added enhancement New feature or request 3. to review Waiting for reviews labels Oct 4, 2023
@skjnldsv
Copy link
Contributor

skjnldsv commented Oct 5, 2023

@juliusknorr
Copy link
Contributor

  • 'X-OC-Mtime': file.lastModified,
    • Should still be relevant as this is the only way a client can set the mtime
  • 'OC-Chunked': true,
    • I don't think this way of chunked uploading is still used/supported, we inteded to drop this as part of nextcloud/server@5d4efb4 but was revered for other reasons
  • 'OC-Total-Length': file.size
    • I'm also unsure about this one, might be no longer relevant as from the previous one but could still be used in some sanity checks in the quota checking as far as my code search goes

Maybe @icewind1991 has some more input

Copy link
Contributor

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Small comment, keeping OC-Total-Length doesn't hurt to have just in case but could probably also be removed separately

lib/uploader.ts Outdated Show resolved Hide resolved
lib/uploader.ts Show resolved Hide resolved
@artonge artonge force-pushed the artonge/feat/allow_custom_headers branch 2 times, most recently from 9717d30 to a1df8fe Compare October 11, 2023 08:16
@skjnldsv skjnldsv requested a review from juliusknorr October 11, 2023 08:23
@juliusknorr
Copy link
Contributor

Tests need some adjustment :)

@artonge artonge force-pushed the artonge/feat/allow_custom_headers branch from a1df8fe to 5900f74 Compare October 11, 2023 13:02
@artonge
Copy link
Contributor Author

artonge commented Oct 11, 2023

Tests need some adjustment :)

Adapted on the API side.

@artonge artonge enabled auto-merge October 11, 2023 13:04
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #935 (5900f74) into master (f3fd109) will increase coverage by 0.20%.
Report is 3 commits behind head on master.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #935      +/-   ##
==========================================
+ Coverage   92.10%   92.30%   +0.20%     
==========================================
  Files           3        3              
  Lines          76       78       +2     
  Branches       14       14              
==========================================
+ Hits           70       72       +2     
  Misses          3        3              
  Partials        3        3              
Files Coverage Δ
lib/utils/upload.ts 92.59% <75.00%> (+0.59%) ⬆️

@artonge artonge merged commit abab125 into master Oct 11, 2023
14 of 15 checks passed
@artonge artonge deleted the artonge/feat/allow_custom_headers branch October 11, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants