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

feat: serve blobs over http #117

Merged
merged 60 commits into from
Jul 28, 2023
Merged

feat: serve blobs over http #117

merged 60 commits into from
Jul 28, 2023

Conversation

gmaclennan
Copy link
Member

@gmaclennan gmaclennan commented Jul 6, 2023

Fixes #115

  • Add BlobServer class
  • Write tests

@gmaclennan gmaclennan changed the title WIP: server blobs over http WIP: serve blobs over http Jul 6, 2023
@gmaclennan
Copy link
Member Author

gmaclennan commented Jul 7, 2023

This initial implementation is done, but I have some questions:

  1. Do we need to provide a mimetype header? If so, what is the best way to do it?
    1. Store the mimetype as Hyperdrive metadata and make two reads on the server (get the entry, then get the stream)
    2. Store the mimetype as metadata, and in the blobStore.createReadstream function, emit the mimetype as an event when read. Would need some magic to start the stream to get the event before piping to the response stream (because headers need to be set before piping the response).
    3. Guess the mimetype from the magic bytes. Can get these bytes with buffer-peek-stream before piping to the response stream.
  2. Maybe just use Fastify? Although relatively simple, there a a few things that will require boilerplate to get working without fastify:
    1. Error handling to return 404 for missing routes, vs 400 bad request, 500 server error etc.
    2. Logging requests
    3. Adding cache headers / etag e.g. https://github.com/fastify/fastify-caching
    4. Promisified server.listen (already in code, copied from Fastify, more complex than first seems).

I have a feeling that over time we will end up adding these ourselves when we would get them from Fastify.
It does create the issue of potentially multiple Fastify versions. I think publishing as Fastify plugins can help this.
@achou11 thoughts?

@gmaclennan
Copy link
Member Author

Discussed with @achou11 in-person.

Do we need to provide a mimetype header? If so, what is the best way to do it?

Yes. Store in hyperdrive metadata. Read metadata as drive.entry(), write mimetype header, then return the stream from (await drive.getBlobs()).createReadStream(entry.value.blob)

Maybe just use Fastify?

Yes, although avoiding another fastify instance might be preferable, the simplicity gains of writing a raw http server are quickly lost as we add error handling and tackle edge-cases. In the future we can convert everything to fastify plugins and set fastify as a peer dependency so that we do not end up with multiple fastify instances.

Copy link
Member Author

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

Implementation all looks good. Just needs some more tests I think:

  • Extract logic to populate the server with fixtures, to make tests easier to read (and clarify what they are testing)
  • Test for valid URL but non-existent blob name and/or variant, should return 404
  • Test for trying to read blob that exists but has not been replicated, should also return 404

@gmaclennan gmaclennan merged commit 8e64abd into main Jul 28, 2023
@gmaclennan gmaclennan deleted the feat/blob-server branch July 28, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide photos in a way that can be consumed by the front-end
2 participants