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

issue #25 HTTP last modified header #27

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@voila
Contributor

voila commented Apr 21, 2016

I have added an Etag header in the HTTP response.

The Etag is a hash of :

  • the unikernel startup time () for static files
  • the article date for a specific article
  • the most recent article date, for the tags or for a group of articles
  • the store latest commit for the Atom feed

If the Etag form the request match the one we are calculating for the response, we return a 304
Otherwise we respond with a new Etag header.

Feedback welcome, I am sure there are plenty of things to improve :)

@hannesm

This comment has been minimized.

Collaborator

hannesm commented Apr 21, 2016

I'll look at this later today... @voila awesome :D

@Engil

This comment has been minimized.

Owner

Engil commented Apr 21, 2016

Will try to take a look this evening, looks amazing indeed. :) Thank you @voila

@hannesm

This comment has been minimized.

Collaborator

hannesm commented Apr 22, 2016

I like the code and approach, thanks! :)

but I've some nitpicky comments:

  • you have some trailing whitespaces in some lines, please don't (if you're using emacs, M-x whitespace-cleanup)
  • you have tabs in your code, please don't (M-x untabify)
  • the updated (you pass to dispatcher) should imho be a Ptime.t, not an option... In order to get there:
    • initial call (in Canopy_main) should error hard (invalid_arg) if Ptime.of_float_s (CLOCK.time ()) returns None
    • if there's no post with the requested tag, instead of providing a listing, return a Not found (404)
    • same for listings of posts, if there's none, no need to return anything apart from a 404
  • hash_time: the ptime_to_pretty_date is granular on a day only, thus our Etags will only be refreshed on a daily basis (which is bad if you change a page more than once a day), I'd instead use the output of Ptime.to_rfc3339.
  • the hashing step is not necessary (it expands atm from 16 bytes to 42 bytes [b64 encoded sha256]), if you will use the rfc3339 encoding it will expand from 36 to 44 bytes
  • specification conformance: RFC 2616 says * in If-none-match should have special treatment (never respond with the resource), and if-none-match contains a list of etags (not sure if anybody sends a list)

I'd ignore the last item (if it works good enough in practise, there's no need to add complexity of requests containing a list of etags; also the * seems to be there for interaction with if-none-match and if-modified-since); but if you could work on the minor previous nitpicks (and rebase to current master), that'd be hugely appreciated! :)

@voila

This comment has been minimized.

Contributor

voila commented Apr 22, 2016

Thanks for the review @hannes. And sorry about the tabs... I did suspect something was amiss.
I have made the changes you suggested except the RFC 2616 conformance.

I'll test tomorrow (it's nightime here...) and rebase :)

@voila voila force-pushed the voila:if-modified branch from c302eeb to 678bae0 Apr 22, 2016

@voila

This comment has been minimized.

Contributor

voila commented Apr 22, 2016

Voila :) Hopefully I did not miss anything...

Unrelated, I am not sure why assets/assets_generated.tar.gz was added to the repo, it is a big file...

let open Canopy_utils in
let respond_html ~headers ~status ~content ~title =
let latest = function

This comment has been minimized.

@hannesm

hannesm Apr 22, 2016

Collaborator

this binding is no longer needed

match sorted with
| [] -> respond_not_found ()
| a::_ ->
let updated = latest sorted in

This comment has been minimized.

@hannesm

hannesm Apr 22, 2016

Collaborator

here let updated = Canopy_content.date a in

| [] -> respond_not_found ()
| a::_ -> (
let sorted = List.sort Canopy_content.compare articles in
let updated = latest sorted in

This comment has been minimized.

@hannesm

hannesm Apr 22, 2016

Collaborator

here also let updated = Canopy_content.date a in

@@ -29,6 +29,10 @@ module Main (C: CONSOLE) (S: STACKV4) (RES: Resolver_lwt.S) (CON: Conduit_mirag
Tls.Config.server ~certificates:(`Single cert) ()
let start console stack resolver conduit disk _clock keys _ _ =
let started = match Ptime.of_float_s (CLOCK.time ()) with
| None -> invalid_arg ("Ptime.of_float_s")
| Some(t) -> t

This comment has been minimized.

@hannesm

hannesm Apr 22, 2016

Collaborator

| Some t -> t

@hannesm

This comment has been minimized.

Collaborator

hannesm commented Apr 22, 2016

I approve (apart from minor nitpicks) 👍

@voila

This comment has been minimized.

Contributor

voila commented Apr 23, 2016

agreed and fixed :)

@hannesm

This comment has been minimized.

Collaborator

hannesm commented Apr 24, 2016

Unrelated, I am not sure why assets/assets_generated.tar.gz was added to the repo, it is a big file...:
I agree with @voila.

@Engil that file is slightly messy.. could you maybe remove it (and maybe massage git in a way that a clone won't need to download it; yes it'll mess up hash ids, but imho it would be good to do)

I think an assets tarball could be distributed out of band for those not having npm, browserify, lessc. The config.ml should use curl or wget to receive that tarball..

@hannesm

This comment has been minimized.

Collaborator

hannesm commented Apr 24, 2016

@voila for some reason your first commit 678bae0 changes assets/assets_generated.tar.gz into an unbearable size.. could you please squash your commits and remove the change to the assets tarball? thanks!

@voila voila force-pushed the voila:if-modified branch from d540ca0 to 712ca57 Apr 25, 2016

@voila

This comment has been minimized.

Contributor

voila commented Apr 25, 2016

I have rebased with the original (small) assets tarball... I think that this massive file got created when I ran mirage config after rebasing to master. I happened to have MathJax (170MB) and other Bower dependencies in the folder disk

@hannesm

This comment has been minimized.

Collaborator

hannesm commented Apr 25, 2016

thx, but the files changed tab on top https://github.com/Engil/Canopy/pull/27/files still includes the assets tarball...

@Engil

This comment has been minimized.

Owner

Engil commented Apr 25, 2016

I assumed bundling the tar file inside the repo would be alright, sorry about that.

So how should we distribute them ? If I recall correctly, Github allows uploading static assets when doing a release so it should be the rightful place to store the assets, but I'm not sure.
I am not a huge fan of multiplying the distribution channels, I already find that bundling a tarball introduce security concerns, but uploading them somewhere else is kind of weird…

Once this branch is cleaned of the tarball (really sorry about that @voila, this is my fault for not being patient enough when merging PRs…) I will clean the main branch from all tarball bits and we'll find a way to distribute assets correctly. :)

@voila voila force-pushed the voila:if-modified branch from 712ca57 to 0ad3179 Apr 25, 2016

@voila voila closed this Apr 25, 2016

@voila voila force-pushed the voila:if-modified branch from 0ad3179 to d5e0899 Apr 25, 2016

@hannesm

This comment has been minimized.

Collaborator

hannesm commented Apr 25, 2016

since the original tarball is only 1.3MB, I think having it in the git repo is fine after all -- I was a bit tired and had the 175MB tarball in mind... which we should avoid ;)

@hannesm

This comment has been minimized.

Collaborator

hannesm commented Apr 25, 2016

@voila somehow your commits now disappeared here... one bug I noticed is that the etag is the date of the first commit, rather than the latest commit for an article.. which is slightly inconvenient (might be related to (or fixed by) 70cef0f in Engil's new-last-updated branch...

@hannesm

This comment has been minimized.

Collaborator

hannesm commented Apr 25, 2016

(now using 70cef0f, the problem persists... but I have a fix hannesm@387604c)

@hannesm

This comment has been minimized.

Collaborator

hannesm commented Apr 25, 2016

which is still not working properly... I'd guess that the last_updated and created are sometimes not correct after a pull somehow.. @Engil, any idea? :)

@Engil

This comment has been minimized.

Owner

Engil commented Apr 25, 2016

Will investigate further, I maybe forgot to try after pulling new content… :)

@voila

This comment has been minimized.

Contributor

voila commented Apr 25, 2016

Do you still want the code related to the Etag ? The pull request has been merged but without my code...

@Engil

This comment has been minimized.

Owner

Engil commented Apr 25, 2016

It is still pretty cool so indeed I want it merged! :)
Looks like the commit disappeared for some reasons… the PR says it want to merge 0 commit into master… 🤔
Maybe open a new one ? Sorry again about the merging mess. 😕

@voila

This comment has been minimized.

Contributor

voila commented Apr 25, 2016

no worries :) I can open a new PR

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