-
Notifications
You must be signed in to change notification settings - Fork 105
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
feat: support Accept
header in @helia/verified-fetch
#438
Conversation
Let users get raw data back from CIDs that would otherwise trigger decoding as JSON or CBOR etc by specifying an `Accept` header. ```typescript const res = await verifiedFetch(cid, { headers: { accept: 'application/octet-stream' } }) console.info(res.headers.get('accept')) // application/octet-stream ``` Make sure the content-type matches the accept header: ```typescript const res = await verifiedFetch(cid, { headers: { accept: 'application/vnd.ipld.raw' } }) console.info(res.headers.get('accept')) // application/vnd.ipld.raw ``` Support multiple values, match the first one: ```typescript const res = await verifiedFetch(cid, { headers: { accept: 'application/vnd.ipld.raw, application/octet-stream, */*' } }) console.info(res.headers.get('accept')) // application/vnd.ipld.raw ``` If they specify an Accept header we can't honor, return a [406](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/406): ```typescript const res = await verifiedFetch(cid, { headers: { accept: 'application/what-even-is-this' } }) console.info(res.status) // 406 ```
|
||
const resp = await verifiedFetch.fetch(cid, { | ||
headers: { | ||
accept: 'application/what-even-is-this, */*' |
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.
nit: we were not testing with ;q=
, below adds what Firefox sends, should be enough as a regression test:
accept: 'application/what-even-is-this, */*' | |
accept: 'application/what-even-is-this,text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8' |
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.
Looks good.
Nice to have the challenge I brought up addressed by this. 💯
@lidel @2color please take another look - I've added a lot more tests, support for wildcards and q-factor and also the IPLD serialized type conversions from https://specs.ipfs.tech/http-gateways/path-gateway/#accept-request-header |
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.
a few comments, but we could fix those later if necessary. this is a huge net win.
@@ -0,0 +1,117 @@ | |||
import { code as dagCborCode } from '@ipld/dag-cbor' | |||
import { code as dagJsonCode } from '@ipld/dag-json' | |||
import type { RequestFormatShorthand } from '../types.js' |
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.
We could move requestFormatShorthand in here as well, or merge with FORMATS
expect(output).to.deep.equal(obj) | ||
}) | ||
|
||
it('should transform DAG-JSON to DAG-CBOR', async () => { |
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.
baller
function notSupportedResponse (body?: BodyInit | null): Response { | ||
return new Response(body, { | ||
const response = new Response(body, { | ||
status: 501, | ||
statusText: 'Not Implemented' | ||
}) | ||
response.headers.set('X-Content-Type-Options', 'nosniff') // see https://specs.ipfs.tech/http-gateways/path-gateway/#x-content-type-options-response-header | ||
return response | ||
} | ||
|
||
function notAcceptableResponse (body?: BodyInit | null): Response { | ||
return new Response(body, { | ||
status: 406, | ||
statusText: '406 Not Acceptable' | ||
}) | ||
} |
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.
we might want to move these into their own file soon. this file is getting excessive.
return response | ||
} | ||
|
||
private async handleDagCbor ({ cid, path, options }: FetchHandlerFunctionArg): Promise<Response> { | ||
private async handleDagCbor ({ cid, path, accept, options }: FetchHandlerFunctionArg): Promise<Response> { |
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.
we might need to break these functions out at some point as well.
if (ipfsRoots != null) { | ||
response.headers.set('X-Ipfs-Roots', ipfsRoots.map(cid => cid.toV1().toString()).join(',')) // https://specs.ipfs.tech/http-gateways/path-gateway/#x-ipfs-roots-response-header | ||
} |
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.
we will likely need to pull this out of handleDagPb
so we can set roots for others as well
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.
a few comments, but we could fix those later if necessary. this is a huge net win.
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
} | ||
}) | ||
expect(resp.status).to.equal(200) | ||
expect(resp.headers.get('content-type')).to.equal('application/json') |
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.
This is pretty counter intuitive, but my understanding is that the order matters and that when processing */*
the default mime-type is returned, which according to the map is application/json
Let users get raw data back from CIDs that would otherwise trigger decoding as JSON or CBOR etc by specifying an
Accept
header.Make sure the content-type matches the accept header:
Support multiple values, match the first one:
If they specify an Accept header we can't honour, one that doesn't match the eventual content type, or one returned by the content type parser return a 406:
Change checklist