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

Do not remove first char in etag #3274

Merged
merged 1 commit into from Mar 27, 2020
Merged

Do not remove first char in etag #3274

merged 1 commit into from Mar 27, 2020

Conversation

LukasHirt
Copy link
Contributor

Description

Follow up on #3187. The first char of etag was being removed when stripping away double quotes.

@LukasHirt LukasHirt added the Status:Needs-Review Needs review from a maintainer label Mar 27, 2020
@LukasHirt LukasHirt self-assigned this Mar 27, 2020
@update-docs
Copy link

update-docs bot commented Mar 27, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@LukasHirt LukasHirt marked this pull request as ready for review March 27, 2020 10:16
@LukasHirt LukasHirt requested a review from C0rby March 27, 2020 10:16
Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

Why stripping anything at all? the double quotes are an integral part of the etag.
They should never be removed!

Also note that an etag does not always start with a double quote - see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag

@C0rby
Copy link
Contributor

C0rby commented Mar 27, 2020

Why stripping anything at all? the double quotes are an integral part of the etag.
They should never be removed!

The etags in the oc10 requests do not contain quotes. This request should be compatible to the oc10 request.

Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

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

👍

@DeepDiver1975
Copy link
Member

The etags in the oc10 requests do not contain quotes. This request should be compatible to the oc10 request.

They do for sure - unless anybody did break anything in oc10 the past months 🙈

@C0rby
Copy link
Contributor

C0rby commented Mar 27, 2020

The etags in the oc10 requests do not contain quotes. This request should be compatible to the oc10 request.

They do for sure - unless anybody did break anything in oc10 the past months

Here is a preview request from oc10.
http://localhost/owncloud/remote.php/dav/files/admin/Photos/Paris.jpg?c=00356a07fe238448791f9c2526d546a1&x=32&y=32&forceIcon=0&preview=1

@DeepDiver1975
Copy link
Member

Here is a preview request from oc10.
http://localhost/owncloud/remote.php/dav/files/admin/Photos/Paris.jpg?c=00356a07fe238448791f9c2526d546a1&x=32&y=32&forceIcon=0&preview=1

Wow - this is so wrong .... anyway since you are rebuilding oc10 apis 👍

@DeepDiver1975
Copy link
Member

FYI: on the PROPFIND response there are double quotes ...

@LukasHirt
Copy link
Contributor Author

the double quotes are an integral part of the etag.

Why is it like that? @DeepDiver1975

@LukasHirt
Copy link
Contributor Author

@C0rby @DeepDiver1975 pls let me know about decision if keep/remove. I'd like to have this in today's release so we don't release previews with a bug 🙂

@DeepDiver1975
Copy link
Member

Why is it like that? @DeepDiver1975

the rfc says so 🤷‍♂️

anyway - this is 'only' on the preview url parameter so I don't care much

@DeepDiver1975 DeepDiver1975 merged commit 9113798 into master Mar 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the bugfix/etag branch March 27, 2020 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants