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

For #1511 ETag support #1652

Merged
merged 5 commits into from Feb 15, 2018
Merged

For #1511 ETag support #1652

merged 5 commits into from Feb 15, 2018

Conversation

@eklavya
Copy link
Contributor

@eklavya eklavya commented Feb 8, 2018

No description provided.

@@ -74,17 +74,28 @@ object StaticFile {
}
})

def fromFile[F[_]: Sync](f: File, req: Option[Request[F]] = None): OptionT[F, Response[F]] =
fromFile(f, DefaultBufferSize, req)
def calcETag: File => String = _.length().toString

This comment has been minimized.

@aeons

aeons Feb 8, 2018
Member

File.length() can throw and should probably be wrapped in F.

This comment has been minimized.

@eklavya

eklavya Feb 9, 2018
Author Contributor

It's only ever called from inside F.delay

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Feb 9, 2018
Member

Not referentially transparent is not referentially transparent.

This comment has been minimized.

@eklavya

eklavya Feb 9, 2018
Author Contributor

I didn't get you, all the file calls aren't, they are all inside one F.delay block.

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Feb 9, 2018
Member

This function is not referentially transparent. It is a top level public function that does not respect its own function signature. If it was used safely everywhere or private that would not change the fact that the function itself does not respect its own signature.

req: Option[Request[F]],
etagCalculator: File => F[String])(implicit F: Sync[F]): OptionT[F, Response[F]] =
OptionT(for {
etagCalc <- etagCalculator(f).map(et => ETag(et))

This comment has been minimized.

@rossabaker

rossabaker Feb 9, 2018
Member

Do we need to f.isFile check before passing off to the calculator?

@@ -74,59 +74,91 @@ object StaticFile {
}
})

def calcETag[F[_]: Sync]: File => F[String] = f => Sync[F].delay(f.length().toString)

This comment has been minimized.

@rossabaker

rossabaker Feb 9, 2018
Member

Is this a sensible default? I tend to think of hashes of the content for accuracy, or maybe a combination of the timestamp and length for speed.

Sync[F]
.delay(f.isFile)
.flatMap { isFile =>
if (isFile) {

This comment has been minimized.

@rossabaker

rossabaker Feb 12, 2018
Member

We could probably micro-optimize this into a single delay rather than a flatMap. It's all local.

This comment has been minimized.

@eklavya

eklavya Feb 13, 2018
Author Contributor

Single flatMap as in call unsafeRun on calcETag or not have F on it? It was without F before because it was only ever called from inside a delay block.

This comment has been minimized.

@aeons

aeons Feb 13, 2018
Member

No. Instead of .delay(...).flatMap(...) just do .delay(...)

This comment has been minimized.

@eklavya

eklavya Feb 13, 2018
Author Contributor

oops, wrong place 👅

.delay(f.isFile)
.flatMap { isFile =>
if (isFile) {
Sync[F].delay(s"${f.lastModified()}${f.length()}")

This comment has been minimized.

@rossabaker

rossabaker Feb 12, 2018
Member

nginx uses these two components, so we're in good company here. They render as hex, which I'm not sure is significant. They also have a - delimiter, which is pedantically safer. (lastModified shouldn't ever vary by an order of magnitude, but if it did, a collision could happen.)

This comment has been minimized.

@eklavya

eklavya Feb 13, 2018
Author Contributor

As I understand, the only think that matters is that there is some change, as long as that is maintained do we need to polish it more?

if (isFile) {
Sync[F].delay(s"${f.lastModified()}${f.length()}")
} else {
Sync[F].pure("")

This comment has been minimized.

@rossabaker

rossabaker Feb 12, 2018
Member

Is this what we want, or do we want to omit the header? I don't think an empty Etag makes any sense. Maybe the calculator should return an Option.

This comment has been minimized.

@eklavya

eklavya Feb 13, 2018
Author Contributor

It doesn't matter, if it's not a file the whole this is never called anyway. Nothing is added.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Feb 15, 2018

Any other objections to this? Will merge later today if not.

@rossabaker rossabaker merged commit 70870ae into http4s:master Feb 15, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rossabaker rossabaker mentioned this pull request Apr 20, 2018
19 of 31 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.