🐛 BUG: `onlyIf` and `httpMetadata` do not honor Headers object.

[bstopp]

Which Cloudflare product(s) does this pertain to?

Miniflare,R2

What version(s) of the tool(s) are you using?

3.20240725.0

What version of Node are you using?

20.13.1

What operating system and version are you using?

Mac Sonoma 14.6

Describe the Bug

Observed behavior

Using a literal Headers object in onlyIf or httpMetadata property for PUT options fails with a parse error.

Expected behavior

Both onlyIf and httpMetadata support literal Headers object as per the documentation.

Steps to reproduce

Please provide the following: See Gist link

Please provide a link to a minimal reproduction

https://gist.github.com/bstopp/6792216fb1ede3466e182b5cc77a9014

Please provide any relevant error logs

No response

[bstopp]

This is related to #6411 as onlyIf should honor If-Match: * and should not save a file if no file exists.

[harshal317]

There seems to be an issue with the onlyIf and ifMatch parameter when passing in an R2Conditional as pointed out by your other issue.

Here though if you construct a Headers object you'll see the behavior you're expecting. For example your test

onlyIf should support generic object? but does not save when 'If-Match: *' is false

will pass when written as

  it("onlyIf should support generic object? but does not save when `If-Match: *` is false", async () => {
    const key = "index.html";
    // No initial object
    const current = await env.BUCKET.get(key);
    assert.ifError(current);

    const httpHeaders = new Headers({});
    httpHeaders.set("If-Match", "*");

    const update = await env.BUCKET.put(key, "Overwrite Content", {
      onlyIf: httpHeaders,
    }); // Should not save anything as per Http Condition Requests
    assert.ifError(update);
  });

[bstopp]

The problem is that there's a parse error on passing any Headers object to either onlyIf or httpHeaders.

The suggested test case you provided already exists in the referenced gist under the test case: "onlyIf should support 'Headers' object with If-Match === '*'"

[bstopp]

This issue wasn't intended to show the success/failure of the If/Match headers and wildcards. It's specifically to indicate that these two options which, IIUC, should accept Headers objects, do not.

[harshal317]

I think part of your issues might be you're not using the Miniflare Headers which is why you may be why you're seeing some of your issues. Do you get any relevant error message?

If I update your import to

import { Miniflare, Headers } from "miniflare";

I still see some tests fail

> test
> mocha --spec=test/**/*.test.js

  put conditional
    ✔ onlyIf should support generic object? (43ms)
    1) onlyIf should support generic object? but does not save when `If-Match: *` is false
workerd/api/r2-rpc.c++:245: warning: R2 error response does not contain the CF-R2-Error header.; response.statusCode = 500
    2) onlyIf should support 'Headers' object
    3) onlyIf should support 'Headers' object with If-Match === '*'
    ✔ httpMetadata should support generic object?
    ✔ httpMetadata should support 'Headers' object

  3 passing (1s)
  3 failing

  4) "after each" hook in "put conditional"

Looking into the tests these are the issues I see

1. onlyIf should support generic object?

This test isn't passing the headers in appropriately. It'd pass even if you changed the If-Match to some garbage value. Should be constructing the headers object.

2. onlyIf should support generic object? but does not save whenIf-Match: *is false

Same issue with not constructing headers appropriately.

3. onlyIf should support 'Headers' object

This one you want to be quoting your etag (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag#etag_value)

const httpHeaders = new Headers({ "If-Match":"${current.etag}"});

We could/should be raising a more appropriate error here so apologies there.

4.onlyIf should support 'Headers' object with If-Match === '*'

This one works as expected but I believe your assertion should be assert.ifError since we don't expect the write to happen and it doesn't

[bstopp]

One can't use the Header type from the Miniflare module. That module is not part of the Workers Runtime, and thus that type won't be available. The use of Miniflare is an implementation detail of these test cases, abstracted from the worker itself. I only incorporated all the logic into the unit test for the sake of brevity, rather than build a full runtime example.

Additionally - there does not appear to be a Header type in the Workers API Reference, as such the assumption has to be that references to Header is the language provided type.

I think that this also makes the most sense, we didn't once consider that there might be a different type reference, given the property being populated is httpMetadata and the context of execution.

You point about the structure of the onlyIf being junk is taken, I was making assumptions of the structure based on the documentation, and my understanding was incorrect. Also, the lack of quotes on the etag for the header - that would likely have been identified if the test got past the header parse error.

[harshal317]

Yeah within your worker it'd suffice to simply do new Headers(). I thought you were seeing this issues specifically w/ your local testing w/ Miniflare.

Let me sanity check the behavior here within a deployed worker.

[bstopp]

I thought you were seeing this issues specifically w/ your local testing w/ Miniflare.

I am seeing this with my local testing and Miniflare. I can't get my actual worker test to pass using the Miniflare text context. and without that, I can't deploy to a runtime.

If we're seeing that runtime is working fine but new Headers() aren't parsed correctly in Miniflare, then we just need to move this ticket?

[harshal317]

Pretty trivial worker here but I do believe the header's are working just fine within the runtime.

    async fetch(request, env, ctx): Promise<Response> {
        await env.BUCKET.delete(['1']);

        // Put unconditionally
        let result = await env.BUCKET.put('1', '1');
        if (result === null) {
            return new Response(null, { status: 500 });
        }

        // Overwrite key 1 since etagMatches wildcard
        result = await env.BUCKET.put('1', '1', {
            onlyIf: new Headers({
                'If-Match': '*',
            }),
        });
        if (result === null) {
            return new Response(null, { status: 500 });
        }

        // Don't overwrite key 1 since etag doesn't match
        result = await env.BUCKET.put('1', '1', {
            onlyIf: new Headers({
                'If-Match': '"foo"',
            }),
        });
        if (result !== null) {
            return new Response(null, { status: 500 });
        }

        // Overwrite key 1 since ifNoneMatch bad etag will be true
        result = await env.BUCKET.put('1', '1', {
            onlyIf: new Headers({
                'If-None-Match': '"foo"',
            }),
        });
        if (result === null) {
            return new Response(null, { status: 500 });
        }

        return new Response(null, { status: 204 });
    }

If you're going to be pulling the binding from Miniflare and writing tests directly interacting with the binding then you'll need to use the Miniflare Headers. However, you should be able to write tests that fetch into your worker where your worker is interacting with the R2 binding and can will do so with the runtime Headers implementation.

That is a much better experience with the vitest integration

https://www.npmjs.com/package/@cloudflare/vitest-pool-workers

Your tests can run inside the workers runtime where you'd be able to construct the headers as you are and not run into these issues

[bstopp]

If you're going to be pulling the binding from Miniflare and writing tests directly interacting with the binding then you'll need to use the Miniflare Headers.

I don't necessarily agree with this, but I can understand the position.

I can't use ViTest due to this bug, so I'll have to figure something out.