-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(gateway): X-Ipfs-Path, Etag, Cache-Control, Suborigin #1537
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -120,6 +121,15 @@ module.exports = { | |||
|
|||
let response = reply(stream2).hold() | |||
|
|||
// Etag maps directly to an identifier for a specific version of a resource | |||
// TODO: change to .cid.toBaseEncodedString() after switch to new js-ipfs-http-response | |||
response.header('Etag', `"${data.multihash}"`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is data.multihash
already a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is a string already. Etag
spec requires wrapping it with ""
Note: the source of this value may change after ipfs/js-ipfs-http-response#9 lands – it will add CID object as data.cid
. It just needs to be a unique string, so we can use cid or a raw multihash – both are fine for this.
response.header('X-Ipfs-Path', ref) | ||
if (ref.startsWith('/ipfs/')) { | ||
const rootCid = ref.split('/')[2] | ||
const CID = request.server.app.ipfs.types.CID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just require
this instead please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
d21693b
to
34d3f44
Compare
@alanshaw if it is okay to add http gateway tests to the interop repo, I will PR tests there. Should I PR before or after this one lands? |
IMHO, yes I it is appropriate to add HTTP interop tests there. I am unaware of a more appropriate place. Although I'm happy for @diasdavid or anyone else to veto :D If you are going to submit interop tests then I'm happy to merge this and then have it fixed if the interop tests throw something up. That said, I'd rather not merge this just yet until after 0.32 is released (hopefully tomorrow/Friday). |
Sounds good. I've added interop for this to my TODO list. (We will need HTTP API interop tests for #1540, so this one will not be the only case of looking at HTTP headers) |
34d3f44
to
a06b5b2
Compare
@alanshaw I noticed there is a lot of code duplication between protocol handler in companion, service worker gateway and a gateway exposed by js-ipfs. Perhaps instead of this PR, we should go in the direction described in ipfs/js-ipfs-http-response#6? Would be great to read your thoughts there 🙌 |
Return the same headers as HTTP Gateway exposed by go-ipfs: - X-Ipfs-Path: requested IPFS Path of returned resource - Etag: multihash of returned payload - Cache-Control: disable cache for directory listings and errors, enable heavy caching for immutable assets from /ipfs/ namespace - Suborigin: use root CID in base32 and literal prefix to conform to current suborigin spec License: MIT Signed-off-by: Marcin Rataj <[email protected]>
a06b5b2
to
e8d8d67
Compare
This PR makes js-ipfs Gateway return the same headers as HTTP Gateway exposed by go-ipfs, namely:
X-Ipfs-Path
: requested IPFS Path of returned resourceEtag
: multihash of returned payloadCache-Control
: disable cache for directory listings and errors,enable heavy caching for immutable assets from /ipfs/ namespace
Suborigin
: use root CID in base32 and literal prefix to conformto current suborigin spec (Suborigins in-web-browsers#66)
cc ipfs/js-ipfs-http-response#10