Skip to content

Make extraction of ETag header independent of capitalisation #7720

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

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

PatrickJosh
Copy link
Contributor

@PatrickJosh PatrickJosh commented Jan 4, 2025

Fixes #7703.

Without this pull request, the extraction of the ETag header in a reply from the server is dependent on the capitalisation of the name of the field being ETag because rawHeaderList().contains() is not case-insensitive. However, according to the standard, header field names are case-insensitive. This pull request makes the extraction of the ETag header case-insensitive by using the header function instead of rawHeaderList().contains(). Internally, Qt performs a case-insensitive comparison of the field names, as can be seen here.

I have tested this with my server and can confirm that I see notifications again.

septatrix

This comment was marked as outdated.

@PatrickJosh
Copy link
Contributor Author

PatrickJosh commented Jan 4, 2025

Actually, when first proposing this pull request, I thought that raw headers in Qt were not case-insensitive at all; this is not the case. Raw headers in Qt are case-insensitive, but using rawHeaderList().contains() is not because rawHeaderList() only gives you a list of the header names in their original capitalisation and does not make them all lowercase, for example. So, a smaller fix would have been the following:

--- a/src/libsync/networkjobs.cpp
+++ b/src/libsync/networkjobs.cpp
@@ -999,7 +999,7 @@ bool JsonApiJob::finished()
     }
 
     // save new ETag value
-    if(reply()->rawHeaderList().contains("ETag"))
+    if(reply()->hasRawHeader("ETag"))
         emit etagResponseHeaderReceived(reply()->rawHeader("ETag"), statusCode);
 
     QJsonParseError error{};

Still, my original patch works and does, in my opinion, not have any downsides compared to this one.

@PatrickJosh
Copy link
Contributor Author

I have now also fixed the same code in gui/ocsjob.cpp although that did not cause any issues regarding #7703 for me.

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

A couple of nitpicks but LGTM

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@claucambra claucambra added this to the 3.15.3 milestone Jan 7, 2025
@mgallien mgallien merged commit dd888a1 into nextcloud:master Jan 7, 2025
10 of 14 checks passed
@mgallien mgallien modified the milestones: 3.15.3, 3.16.0 Jan 7, 2025
@PatrickJosh
Copy link
Contributor Author

Hi, thanks for merging! My I quickly ask, why is this not included in the 3.15.3 milestone? It is a rather annoying bug in my opinion. If there's nothing that keeps this from being included in 3.15.3, I'd like to kindly ask you to reconsider this for 3.15.3. Thanks!

Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

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

Successfully merging this pull request may close these issues.

[Bug]: No notifications although notifications are enabled
4 participants