Remove incorrect logic in Asset Worker

[jamesopstad]

When I was looking through the Asset Worker code I spotted an error:

workers-sdk/packages/workers-shared/asset-worker/src/index.ts

Lines 132 to 135 in afe9850

	// if asset exists but non GET/HEAD method, 405
	if (intent && ["GET", "HEAD"].includes(method)) {
	return new MethodNotAllowedResponse();
	}

This currently does the opposite of what it should and returns the MethodNotAllowedResponse for GET and HEAD methods. The only reason that the Router Worker is still functioning correctly is because it treats the MethodNotAllowedResponse as truthy.

workers-sdk/packages/workers-shared/router-worker/src/index.ts

Line 60 in afe9850

if (await env.ASSET_WORKER.unstable_canFetch(request)) {

I've removed method handling from unstable_canFetch altogether as I feel that it should only return a boolean. The Asset Worker already returns a MethodNotAllowedResponse for methods other than GET and HEAD.

workers-sdk/packages/workers-shared/asset-worker/src/handler.ts

Lines 32 to 34 in afe9850

	if (!["GET", "HEAD"].includes(method)) {
	return new MethodNotAllowedResponse();
	}

If this also needs to occur earlier then the request method could be checked in the Router Worker.

I haven't added tests as there aren't any others for the Asset Worker beyond the KV store at present.

---

• Tests
  • TODO (before merge)
  • Tests included
  • Tests not necessary because: there are existing fixture tests, just needs to not break those
• E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
  • I don't know
  • Required
  • Not required because:
• Public documentation
  • TODO (before merge)
  • Cloudflare docs PR(s):
  • Documentation not necessary because: not documented

[github-actions]

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12011975582/npm-package-wrangler-7176

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7176/npm-package-wrangler-7176

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12011975582/npm-package-wrangler-7176 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12011975582/npm-package-create-cloudflare-7176 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12011975582/npm-package-cloudflare-kv-asset-handler-7176

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12011975582/npm-package-miniflare-7176

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12011975582/npm-package-cloudflare-pages-shared-7176

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12011975582/npm-package-cloudflare-vitest-pool-workers-7176

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12011975582/npm-package-cloudflare-workers-editor-shared-7176

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12011975582/npm-package-cloudflare-workers-shared-7176

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12011975582/npm-package-cloudflare-workflows-shared-7176

Note that these links will no longer work once the GitHub Actions artifact expires.

---

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	workspace:*	3.20241106.1
workerd	1.20241106.1	1.20241106.1
workerd --version	1.20241106.1	2024-11-06

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

[WillTaylorDev]

We might still want this check in router worker, since we would rather return the user worker instead of a 405. I'm totally in agreance this makes sense as just a bool response!

[GregBrimble]

Can't change that behavior now without a compat flag.

[petebacondarwin]

I agree that this makes sense as a boolean; and also that we need to change Router Worker to handle the 405 case.
Also, we could do with more tests on this worker. Currently there are scant few. But perhaps unfair on @jamesopstad to be the one to write those?

[emily-shen]

@petebacondarwin There are some tests scattered across the fixtures - that should probably be documented somewhere whoops. There actually are some tests for the 405 (in worker-with-assets I think) but that was clearly a bit too e2e… there’s also some unit tests in asset-config that could be built on.

[CarmenPopoviciu]

@jamesopstad can you please include a changeset to your PR 🙏 ?

[changeset-bot]

🦋 Changeset detected

Latest commit: a311397

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@cloudflare/workers-shared	Patch
wrangler	Patch
@cloudflare/vitest-pool-workers	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR