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

cloudflareR2bindings: Fixed getItemRaw() #517

Closed
wants to merge 1 commit into from
Closed

Conversation

MickL
Copy link

@MickL MickL commented Dec 7, 2024

TLDR: Files saved to Cloudflare R2 were not returned correctly by .getItemRaw(), this PR fixes that.


I tried to save files into Cloudflare R2 from a Cloudflare worker by a binding. Saving was no problem by use of .setItemRaw(), the file was added correctly to R2. But getting the item I had weird results:

  • .getItem() returns a string, e.g. when the file was a pdf, the response was the markup of the pdf as a string. (btw this is because it is calling .text(), not sure if this is usefull at all)
  • .getItemRaw() returns an empty object

I checked the docs and they do it like this:

export default defineEventHandler(async (event) => {
    const object = await globalThis.__env__.BINDING.get('key')`
    return object.data;
});

In this example above, object is of type R2ObjectBody, reference is here

In the current implementation object.arrayBuffer() is returned instead of .body. I am not exactly sure why the author chose to do that, because:

  1. It doesnt work (i tried to return image and pdf)
  2. I would expect a "raw" function not to call a function that obviously does transformations

Therefor I changed getItemRaw() to return .body which now returns files correctly.

@MickL MickL changed the title Fixed getItemRaw() cloudflareR2bindings: Fixed getItemRaw() Dec 7, 2024
@pi0
Copy link
Member

pi0 commented Dec 10, 2024

I have pushed some better tests (#518) to validate getItemRaw behavior against actual platform behavior, and it works as expected.

It is strange that you are getting an empty object. How do you test it? console.log might be misleading (please consider always sharing a reproduction this way we can help faster and better 🙏🏼)

raw methods return a raw buffer (ie: fully resolved binary value) and .body is a stream.

We could return a Uint8Array to be more consistent with other drivers but it will be a behavior change.

From this point:

  • As an enhancement, we could support .getItemRaw(key, { type: 'stream' }} if you like to have stream type like this PR changes + in unstorage v2 we might always return a Buffer/Unit8Array for consistency
  • As a (possible) fix, we need a minimal reproduction that shows currently there is a bug. Addtional tests work as expected so it might not be a bug.

@pi0
Copy link
Member

pi0 commented Dec 10, 2024

~> To allow specify raw type: #519

@MickL
Copy link
Author

MickL commented Dec 10, 2024

What did you test exactly that arrayBuffer was correct? Did you upload a file to R2 using setItem?

I saved an uploaded file like this, and I only tested deployed to Cloudflare worker.

Upload file:

export default defineEventHandler(async (event) => {
  const files = await readMultipartFormData(event);

  await useStorage('files').setItemRaw('some-key', files[0].data);

  return 'ok';
});

Get file:

  1. getItem() returns the uploaded file as text
export default defineEventHandler(async (event) => {
  return useStorage('files').getItem('some-key');
});

Response:

�PNG
�
���
IHDR�������*�������W����biTXtXML:com.adobe.xmp�����
<?xpacket begin="" id="W5M0MpCehiHzreSzNTczkc9d"?>
<x:xmpmeta xmlns:x="adobe:ns:meta/" x:xmptk="XMP Core 6.0.0">
  1. getItemRaw() returns an empty object:
export default defineEventHandler(async (event) => {
  return useStorage('files').getItemRaw('some-key');
});

Response:

{}

Btw. on localhost using memory driver this works with both: getItem() and getItemRaw().

@pi0
Copy link
Member

pi0 commented Dec 10, 2024

Tests: https://github.com/unjs/unstorage/blob/main/test/drivers/cloudflare-r2-binding.test.ts

getItemRaw('some-key') returns an ArrayBuffer and I guess h3 does not support it as direct response that's why you see it like an empty object (it handles Buffer and Buffer-like Uint8Array ) you might want to do something like:

const val = await useStorage('files').getItemRaw('some-key')
return new Uint8Array(val)

(another option is return new Response(val))

Regardless there is no issue with unstorage driver itself and next release allows you to specify what raw type you want.

@MickL
Copy link
Author

MickL commented Dec 10, 2024

Opened here: unjs/h3#931

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.

2 participants