From 9dcd79826925048a138f472fd5d607c6e2f99359 Mon Sep 17 00:00:00 2001 From: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com> Date: Thu, 7 Mar 2024 15:12:41 -0800 Subject: [PATCH] chore: Apply suggestions from code review --- .../src/utils/byte-range-context.ts | 19 +++++++++---------- .../src/utils/response-headers.ts | 10 +++++----- packages/verified-fetch/src/verified-fetch.ts | 3 ++- .../test/range-requests.spec.ts | 2 -- 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/packages/verified-fetch/src/utils/byte-range-context.ts b/packages/verified-fetch/src/utils/byte-range-context.ts index b4cc58c3..9ad38f84 100644 --- a/packages/verified-fetch/src/utils/byte-range-context.ts +++ b/packages/verified-fetch/src/utils/byte-range-context.ts @@ -48,9 +48,9 @@ export class ByteRangeContext { private _isValidRangeRequest: boolean | null = null private readonly requestRangeStart: number | null private readonly requestRangeEnd: number | null - byteStart: number | undefined - byteEnd: number | undefined - byteSize: number | undefined + private byteStart: number | undefined + private byteEnd: number | undefined + private byteSize: number | undefined constructor (logger: ComponentLogger, private readonly headers?: HeadersInit) { this.log = logger.forComponent('helia:verified-fetch:byte-range-context') @@ -80,7 +80,7 @@ export class ByteRangeContext { public setBody (body: SupportedBodyTypes): void { this._body = body - // if fileSize was set explicitly or already set, don't recalculate it + // if fileSize was already set, don't recalculate it this.fileSize = this.fileSize ?? getBodySizeSync(body) this.log.trace('set request body with fileSize %o', this._fileSize) @@ -111,7 +111,6 @@ export class ByteRangeContext { } else if (body instanceof ReadableStream) { // stream should already be spliced by dagPb/unixfs return body - // return splicingTransformStream(body, offset, length, this.logger) } } // offset and length are not set, so not a range request, return body untouched. @@ -146,7 +145,7 @@ export class ByteRangeContext { public set fileSize (size: number | bigint | null) { this._fileSize = size != null ? Number(size) : null this.log.trace('set _fileSize to %o', this._fileSize) - // if fileSize was set explicitly, we need to recalculate the offset details + // when fileSize changes, we need to recalculate the offset details this.setOffsetDetails() } @@ -220,11 +219,10 @@ export class ByteRangeContext { if (this.byteStart == null || this.byteStart === 0) { return 0 } - // if length is undefined, unixfs.cat and ArrayBuffer.slice will not use an inclusive offset, so we have to subtract by 1 + // if byteStart is undefined, unixfs.cat and ArrayBuffer.slice will not use an inclusive offset, so we have to subtract by 1 if (this.isPrefixLengthRequest || this.isSuffixLengthRequest) { return this.byteStart - 1 } - // this value will be passed to unixfs.cat return this.byteStart } @@ -235,8 +233,9 @@ export class ByteRangeContext { */ public get length (): number | undefined { // this value will be passed to unixfs.cat. - if (this.requestRangeEnd == null) { - return undefined // this is a suffix-length request and unixfs has a bug where it doesn't always respect the length parameter + if (this.isSuffixLengthRequest) { + // this is a suffix-length request and unixfs has a bug where it doesn't always respect the length parameter + return undefined } return this.byteSize ?? undefined } diff --git a/packages/verified-fetch/src/utils/response-headers.ts b/packages/verified-fetch/src/utils/response-headers.ts index 09dd9399..48e1103b 100644 --- a/packages/verified-fetch/src/utils/response-headers.ts +++ b/packages/verified-fetch/src/utils/response-headers.ts @@ -1,17 +1,17 @@ /** * This function returns the value of the `Content-Range` header for a given range. - * If you know the total size of the body, you should pass it in the `options` object. + * If you know the total size of the body, pass it as `byteSize` * * @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Range */ export function getContentRangeHeader ({ byteStart, byteEnd, byteSize }: { byteStart: number | undefined, byteEnd: number | undefined, byteSize: number | undefined }): string { const total = byteSize ?? '*' // if we don't know the total size, we should use * - if (byteStart == null && byteEnd == null) { - return `bytes */${total}` - } if (byteStart != null && byteEnd == null) { - return `bytes ${byteStart}-*/${total}` + if (byteSize == null) { + return `bytes */${total}` + } + return `bytes ${byteStart}-${byteSize}/${byteSize}` } if (byteStart == null && byteEnd != null) { if (byteSize == null) { diff --git a/packages/verified-fetch/src/verified-fetch.ts b/packages/verified-fetch/src/verified-fetch.ts index 1301dbe5..e4efa749 100644 --- a/packages/verified-fetch/src/verified-fetch.ts +++ b/packages/verified-fetch/src/verified-fetch.ts @@ -332,12 +332,13 @@ export class VerifiedFetch { } } + // we have to .stat before .cat so we can get the size and make sure byte offsets are calculated properly if (byteRangeContext.isRangeRequest && byteRangeContext.isValidRangeRequest) { stat = stat ?? await this.unixfs.stat(resolvedCID, { signal: options?.signal, onProgress: options?.onProgress }) - this.log.trace('stat for %c/%s: %o', resolvedCID, path, stat) + this.log.trace('stat.fileSize for rangeRequest %d', stat.fileSize) byteRangeContext.fileSize = stat.fileSize } const offset = byteRangeContext.offset diff --git a/packages/verified-fetch/test/range-requests.spec.ts b/packages/verified-fetch/test/range-requests.spec.ts index 5ac3f420..9df28b5b 100644 --- a/packages/verified-fetch/test/range-requests.spec.ts +++ b/packages/verified-fetch/test/range-requests.spec.ts @@ -29,7 +29,6 @@ describe('range requests', () => { interface SuccessfulTestExpectation { contentRange: string - // byteSize?: number bytes: Uint8Array } async function testRange (cid: CID, headerRange: string, expected: SuccessfulTestExpectation): Promise { @@ -49,7 +48,6 @@ describe('range requests', () => { const responseContent = await response.arrayBuffer() expect(new Uint8Array(responseContent)).to.deep.equal(expected.bytes) - // expect(responseContent.byteLength).to.equal(expected.byteSize) // the length of the data should match the requested range } async function assertFailingRange (response: Promise): Promise {