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

Support endpoints that don't support range requests in asyncBufferFromUrl #57

Merged
merged 3 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,18 +86,42 @@ export async function asyncBufferFromUrl({ url, byteLength, requestInit }) {
if (!url) throw new Error('missing url')
// byte length from HEAD request
byteLength ||= await byteLengthFromUrl(url, requestInit)

/**
* A promise for the whole buffer, if range requests are not supported.
* @type {Promise<ArrayBuffer>|undefined}
*/
let buffer = undefined
const init = requestInit || {}

return {
byteLength,
async slice(start, end) {
// fetch byte range from url
if (buffer) {
return buffer.then(buffer => buffer.slice(start, end));
}

const headers = new Headers(init.headers)
const endStr = end === undefined ? '' : end - 1
headers.set('Range', `bytes=${start}-${endStr}`)

const res = await fetch(url, { ...init, headers })
if (!res.ok || !res.body) throw new Error(`fetch failed ${res.status}`)
return res.arrayBuffer()
},

switch (res.status) {
// Endpoint does not support range requests and returned the whole object
case 200:
buffer = res.arrayBuffer();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lint: should be no semi-colons at end of line. Please run npm run lint and fix the errors.

(I'm not sure why lint didn't run on this PR? It should be in the github actions?)

Copy link
Contributor

@severo severo Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the CI requires approval to run: https://github.com/hyparam/hyparquet/actions/runs/12801142297/attempts/1

This workflow is awaiting approval from a maintainer in #57

Maybe a repo setting to change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just launched the CI

return buffer.then(buffer => buffer.slice(start, end));

// The endpoint supports range requests and sent us the requested range
case 206:
return res.arrayBuffer();

default:
throw new Error(`fetch received unexpected status code ${res.status}`)
}
}
}
}

Expand Down
42 changes: 42 additions & 0 deletions test/utils.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { describe, expect, it, vi } from 'vitest'
import { asyncBufferFromUrl, byteLengthFromUrl, toJson } from '../src/utils.js'
import { arrayBuffer } from 'stream/consumers'

describe('toJson', () => {
it('convert undefined to null', () => {
Expand Down Expand Up @@ -114,6 +115,7 @@ describe('asyncBufferFromUrl', () => {
global.fetch = vi.fn().mockResolvedValue({
ok: true,
body: {},
status: 206,
arrayBuffer: () => Promise.resolve(mockArrayBuffer),
})

Expand All @@ -131,6 +133,7 @@ describe('asyncBufferFromUrl', () => {
global.fetch = vi.fn().mockResolvedValue({
ok: true,
body: {},
status: 206,
arrayBuffer: () => Promise.resolve(mockArrayBuffer),
})

Expand Down Expand Up @@ -191,6 +194,7 @@ describe('asyncBufferFromUrl', () => {
return Promise.resolve({
ok: true,
body: {},
status: 206,
arrayBuffer: () => Promise.resolve(mockArrayBuffer),
})
})
Expand All @@ -203,4 +207,42 @@ describe('asyncBufferFromUrl', () => {

await expect(withHeaders.slice(0, 10)).rejects.toThrow('fetch failed 404')
})

describe("when range requests are unsupported", () => {
it('creates an AsyncBuffer with the correct byte length', async () => {
const mockArrayBuffer = new ArrayBuffer(1024)
global.fetch = vi.fn().mockResolvedValue({
ok: true,
status: 200,
body: {},
arrayBuffer: () => Promise.resolve(mockArrayBuffer),
});

const buffer = await asyncBufferFromUrl({ url: 'https://example.com', byteLength: 1024 })
const chunk = await buffer.slice(0, 100);

expect(fetch).toHaveBeenCalledWith('https://example.com', {
headers: new Headers({ Range: 'bytes=0-99' })
});

expect(chunk.byteLength).toBe(100);
})

it('does not make multiple requests for multiple slices', async () => {
const mockArrayBuffer = new ArrayBuffer(1024)
global.fetch = vi.fn().mockResolvedValue({
ok: true,
status: 200,
body: {},
arrayBuffer: () => Promise.resolve(mockArrayBuffer)
});

const buffer = await asyncBufferFromUrl({ url: 'https://example.com', byteLength: 1024 })

await buffer.slice(0, 100)
await buffer.slice(550, 600)

expect(fetch).toBeCalledTimes(1)
})
})
})
Loading