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

Better caching #794

Merged
merged 3 commits into from
Dec 6, 2020
Merged

Better caching #794

merged 3 commits into from
Dec 6, 2020

Conversation

elegaanz
Copy link
Member

Based on @trinity-1686a's patch.

I'm not sure it is a fix for #786 but it is better than nothing.

@elegaanz elegaanz added C: Enhancement New feature or request S: Ready for review This PR is ready to be reviewed A: Backend Code running on the server labels Jun 25, 2020
@elegaanz
Copy link
Member Author

OK, my git history is broken, I need to fix that.

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

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

🙋️

@@ -63,7 +63,7 @@ impl<'r> Responder<'r> for Ructe {
let etag = format!("{:x}", hasher.finish());
if r.headers()
.get("If-None-Match")
.any(|s| s[1..s.len() - 1] == etag)
.any(|s| s[1..s.len() - 1] == etag || s[3..s.len() - 1] == etag)
Copy link
Contributor

Choose a reason for hiding this comment

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

can one of you two explain what this does?

Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably get it's comment actually.
We send Etags as 'some_tag', the old match does not verify the ', but check that whatever in-between is the right tag. Nginx apparently change the Etag to W/'some_tag', W/ indicating it's a weak Etag, the content may not be byte-equal, but is semantically identical (which is imo a shitty but correct interpretation of the RFC, as Nginx send compressed data)

The new match ignore that W/ saying it's a weak etag and so match if Nginx modified the header we sent previously

Comment might looks like : // if a strong or weak etag matches

@igalic
Copy link
Contributor

igalic commented Jun 25, 2020

don't we still have to to change the subscribe/follow/blah button from a form… to… well… a button?

@trinity-1686a
Copy link
Contributor

well there is that too, but these buttons are not here on the front page or a blog or user page. Anyway for post a better caching solution that does not require to query the database should be added

@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

Merging #794 (719fec0) into main (0bec13e) will increase coverage by 0.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #794      +/-   ##
==========================================
+ Coverage   38.98%   39.07%   +0.08%     
==========================================
  Files          73       73              
  Lines        9777     9756      -21     
  Branches     2240     2233       -7     
==========================================
  Hits         3812     3812              
+ Misses       4906     4886      -20     
+ Partials     1059     1058       -1     

@elegaanz elegaanz changed the base branch from master to main July 5, 2020 16:19
Copy link

@FreyjaWildes FreyjaWildes left a comment

Choose a reason for hiding this comment

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

I'm not totally sure, being new to the software and Rust, but this seems fair enough and a small change. Probably can be approved, right ?

@elegaanz elegaanz merged commit 5099a00 into main Dec 6, 2020
@KitaitiMakoto
Copy link
Contributor

@FreyjaWildes You're right. I merged this pull request as https://git.joinplu.me/Plume/Plume/pulls/840

Thank you for keeping interest on Plume!

@igalic igalic deleted the better-caching branch December 6, 2020 19:28
@elegaanz elegaanz restored the better-caching branch December 7, 2020 02:03
@elegaanz elegaanz deleted the better-caching branch September 5, 2021 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Backend Code running on the server C: Enhancement New feature or request S: Ready for review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants