Set cache header in creator node CID route #959
Conversation
creator-node/src/routes/files.js
Outdated
| @@ -246,6 +254,8 @@ const getDirCID = async (req, res) => { | |||
| await findCIDInNetwork(queryResults.storagePath, CID, req.logger, libs) | |||
| return await streamFromFileSystem(req, res, queryResults.storagePath) | |||
| } catch (e) { | |||
| // Unset the cache-control header so that a bad response is not cached | |||
| res.removeHeader('cache-control') | |||
There was a problem hiding this comment.
not sure if i understand this correctly, but i dont think this is the right place for this? cause after it fails in this catch block, it continues to the ipfs fetch try/catch. only if it fails there then we would not want to not cache right? cause this catch just logs, doesn't return anything to the client
There was a problem hiding this comment.
yeah i think dheeraj has a point, we should move this to the catch down below if we failed the IPFS cat
There was a problem hiding this comment.
a potential alternative would be to explicitly set when we know the write is successful. like in streamFromFileSystem or ipfsCat on the end event we can add the header like
.on('end', () => { res.setHeader('cache-control'...); res.end(); resolve() })
and that would prevent unintentional caches
There was a problem hiding this comment.
@dmanjunath looks like your solution didn't work unf. for the stream case. Headers are already set on the first byte of the piping the stream to the res. It should still be ok here though because if there is an error, it should throw and be caught by the final catch (if it fails IPFS too)
There was a problem hiding this comment.
Want to move this to staging to test a bit more if ok w/ u
Trello Card Link
na
Description
For the creator node
/ipfs/<CID>and/ipfs/<CID>/<File>routes, acache-controlheader was added.This is to improve client performance, also the ipfs.io and cloudflare public gateways have a header of
cache-control: public, max-age=29030400, immutable(Cache of almost a yr)The one added to CN is only for 30 days.
Note: The getCID function is also used in the
/tracks/stream/:encodedIdroute which I presume is fine for now b/c we do not allow a track to change its cid.Note: I did not add it to the fallback of
catReadableStreamb/c in the case that it fails, I'm unsure about the cache headers.Services
Creator Node
Does it touch a critical flow like Discovery indexing, Creator Node track upload, Creator Node gateway, or Creator Node file system?
Delete an option.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.
Include log analysis if applicable.
I ran the system locally and checked the the header was present on both the /ipfs/cid and /ipfs/cid/file routes and that on subsequent requests, the browser pulled it from disk cache.