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(workers-shared): Add Asset Server Worker behaviour #6539

Merged
merged 6 commits into from
Aug 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
11 changes: 11 additions & 0 deletions .changeset/dull-ants-check.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
"@cloudflare/workers-shared": minor
---

feat: Add basic Asset Worker behaviour

This commit implements a basic Asset Worker behaviour, including:

- headers handling
- `200`/`404`/`500` response handling
- fetching data from KV
57 changes: 57 additions & 0 deletions fixtures/workers-with-assets/public/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# Workers with Assets

Welcome to Workers + Assets YAY!

Please proceed with much excitement ^.^

.................
.......-++**********:.
..-+****++**********#:
...-+***+++====+********##=
....:=****+======+***********##+
....-****+======+*************###+
.:==+*+======+***************####=
....-===--=====++****************#####:
....:====-:::-==+******************#####+.
..-====:::::::+*******************#####*..
..-===-:::::::-==+*****************######=..
..-===-::::::-=======***************######*.
...-===-::::::-==========+************######*:.
...-===-::::::-==----========+*********#######:..
.-===-::::::--::::::::::-======+******#######-. .
..:====::::::-=:::-*####*-:::-=======+**#######-.
.:====-:::::-==:::*#########-::========+**#####-
..====-:::::-===-::###########*:::======*******+..
..=+====:::::=====::-############:::====+*******=...
....:=++====:::-======-::###########*:::===+*******-..
...........:-==+**+=++++=============-:::*#########-::===********:..
....--=+************====+++===============:::=*####*=:::-=+*******=..
.:*************###*=====++++================::::::::::-==********:..
..*****#############=======++++==========================+*******+...
...+****#############*==:::-===++++========================********-.
...+****###############+=::::=====+++++====================********+...
.=****################*=-:::-======+++++=================+********:..
..-****##################===:-=========++++++=============+********:..
.+***####################===========+***#++++++=========+********=..
.*###################+-.-=======+****##*==+++++++====+********=...
...-+#############*=...-++=====+****####+=====++++++*********+...
....-*######+:.....++++===******####==========+*######**+..
...+=.....:::-+++++******#####=========+***######*..
.......::::::=++******#####+========***********:...
....::::::...+*****######=======+**********##*.
..:::::....-*****######+=====+**********#####*.
...:::::...:******#####*+++++*********#########+.
..:::::...+*****######+++++++=.=###############:.
.:::::..-*****######-..-=++=..-##########***###..
....::::..=****######=....::::..:########******##*..
...::::..+***######-.....::::...*##***********###=..
..::::...-**####*:......::::...###************###-..
..:::......=##*:......:::::...+##*************###:
...::::...............:::::....+##*************####.
...::::::::::::::..:::::::......**************###-..
.::::::::::::::::::::::..... ...:***********###+....
..::::.......::::::...... ..:********###*:...
........ ...=******###=..
.=***###+..
..*####:...
........
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 12 additions & 0 deletions fixtures/workers-with-assets/public/yay.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@

.----------------. .----------------. .----------------.
| .--------------. || .--------------. || .--------------. |
| | ____ ____ | || | __ | || | ____ ____ | |
| | |_ _||_ _| | || | / \ | || | |_ _||_ _| | |
| | \ \ / / | || | / /\ \ | || | \ \ / / | |
| | \ \/ / | || | / ____ \ | || | \ \/ / | |
| | _| |_ | || | _/ / \ \_ | || | _| |_ | |
| | |______| | || ||____| |____|| || | |______| | |
| | | || | | || | | |
| '--------------' || '--------------' || '--------------' |
'----------------' '----------------' '----------------'
82 changes: 80 additions & 2 deletions fixtures/workers-with-assets/tests/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,86 @@ describe("[Workers + Assets] `wrangler dev`", () => {

it("should not resolve '/' to '/index.html' ", async ({ expect }) => {
let response = await fetch(`http://${ip}:${port}/`);
let text = await response.text();
expect(response.status).toBe(404);
expect(text).toContain("Not Found");
});

it("should 404 if asset is not found in the asset manifest", async ({
expect,
}) => {
let response = await fetch(`http://${ip}:${port}/hello.html`);
expect(response.status).toBe(404);

response = await fetch(`http://${ip}:${port}/hello.txt`);
expect(response.status).toBe(404);
});

it("should handle content types correctly", async ({ expect }) => {
let response = await fetch(`http://${ip}:${port}/index.html`);
let text = await response.text();
expect(response.status).toBe(200);
expect(response.headers.get("Content-Type")).toBe(
"text/html; charset=utf-8"
);

response = await fetch(`http://${ip}:${port}/README.md`);
text = await response.text();
expect(response.status).toBe(200);
expect(response.headers.get("Content-Type")).toBe(
"text/markdown; charset=utf-8"
);
expect(text).toContain(`Welcome to Workers + Assets YAY!`);

response = await fetch(`http://${ip}:${port}/yay.txt`);
text = await response.text();
expect(response.status).toBe(200);
expect(response.headers.get("Content-Type")).toBe(
"text/plain; charset=utf-8"
);
expect(text).toContain(`.----------------.`);

response = await fetch(`http://${ip}:${port}/lava-lamps.jpg`);
expect(response.status).toBe(200);
expect(response.headers.get("Content-Type")).toBe("image/jpeg");
});

it("should only ever handle GET requests", async ({ expect }) => {
// as per https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods
// excl. TRACE and CONNECT which are not supported

let response = await fetch(`http://${ip}:${port}/hello.html`, {
method: "HEAD",
});
expect(response.status).toBe(405);
expect(response.statusText).toBe("Method Not Allowed");

response = await fetch(`http://${ip}:${port}/hello.html`, {
method: "POST",
});
expect(response.status).toBe(405);
expect(response.statusText).toBe("Method Not Allowed");

response = await fetch(`http://${ip}:${port}/hello.html`, {
method: "PUT",
});
expect(response.status).toBe(405);
expect(response.statusText).toBe("Method Not Allowed");

response = await fetch(`http://${ip}:${port}/hello.html`, {
method: "DELETE",
});
expect(response.status).toBe(405);
expect(response.statusText).toBe("Method Not Allowed");

response = await fetch(`http://${ip}:${port}/hello.html`, {
method: "OPTIONS",
});
expect(response.status).toBe(405);
expect(response.statusText).toBe("Method Not Allowed");

response = await fetch(`http://${ip}:${port}/hello.html`, {
method: "PATCH",
});
expect(response.status).toBe(405);
expect(response.statusText).toBe("Method Not Allowed");
});
});
11 changes: 10 additions & 1 deletion packages/miniflare/src/workers/kv/assets.worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@ export default <ExportedHandler<Env>>{
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { filePath, contentType } = entry;
const blobsService = env[SharedBindings.MAYBE_SERVICE_BLOBS];
return blobsService.fetch(new URL(filePath, "http://placeholder"));
const response = await blobsService.fetch(
new URL(filePath, "http://placeholder")
);
const newResponse = new Response(response.body, response);
// ensure the runtime will return the metadata we need
newResponse.headers.append(
"cf-kv-metadata",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

`{"contentType": "${contentType}"}`
);
return newResponse;
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ const CONTENT_HASH_SIZE = 16;
const TAIL_SIZE = 8;
const ENTRY_SIZE = PATH_HASH_SIZE + CONTENT_HASH_SIZE + TAIL_SIZE;

export type AssetEntry = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this ends up being used anywhere

Copy link
Contributor Author

@CarmenPopoviciu CarmenPopoviciu Aug 23, 2024

Choose a reason for hiding this comment

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

oh right. must be some leftover from all the iterations 😅 . will remove as part of WC-2597

path: string;
contentHash: string;
};

export class AssetsManifest {
private data: ArrayBuffer;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// have the browser check in with the server to make sure its local cache is valid before using it
export const CACHE_CONTROL_BROWSER = "public, max-age=0, must-revalidate";
9 changes: 9 additions & 0 deletions packages/workers-shared/asset-server-worker/src/global.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
type Env = {
// ASSETS_MANIFEST is a pipeline binding to an ArrayBuffer containing the
// binary-encoded site manifest
ASSETS_MANIFEST: ArrayBuffer;

// ASSETS_KV_NAMESPACE is a pipeline binding to the KV namespace that the
// assets are in.
ASSETS_KV_NAMESPACE: KVNamespace;
};
80 changes: 52 additions & 28 deletions packages/workers-shared/asset-server-worker/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,38 +1,62 @@
import { WorkerEntrypoint } from "cloudflare:workers";
import { AssetsManifest } from "./assets-manifest";
import {
InternalServerErrorResponse,
MethodNotAllowedResponse,
NotFoundResponse,
OkResponse,
} from "./responses";
import { getAdditionalHeaders, getMergedHeaders } from "./utils/headers";
import { getAssetWithMetadataFromKV } from "./utils/kv";

interface Env {
/**
* ASSETS_MANIFEST is a pipeline binding to an ArrayBuffer containing the
* binary-encoded site manifest
*/
ASSETS_MANIFEST: ArrayBuffer;
/**
* ASSETS_KV_NAMESPACE is a pipeline binding to the KV namespace that the
* assets are in.
*/
ASSETS_KV_NAMESPACE: KVNamespace;
}

export default {
async fetch(request: Request, env: Env) {
const { ASSETS_MANIFEST, ASSETS_KV_NAMESPACE } = env;
export default class extends WorkerEntrypoint<Env> {
async fetch(request: Request) {
if (request.method.toLowerCase() !== "get") {
return new MethodNotAllowedResponse();
}

const url = new URL(request.url);
const { pathname } = url;
try {
return this.handleRequest(request);
} catch (err) {
return new InternalServerErrorResponse(err);
}
}

const assetsManifest = new AssetsManifest(ASSETS_MANIFEST);
const assetKey = await assetsManifest.get(pathname);
if (!assetKey) {
return new Response("Not Found", { status: 404 });
async handleRequest(request: Request) {
const assetEntry = await this.getAssetEntry(request);
if (!assetEntry) {
return new NotFoundResponse();
}

const content = await ASSETS_KV_NAMESPACE.get(assetKey);
if (!content) {
const assetResponse = await getAssetWithMetadataFromKV(
this.env.ASSETS_KV_NAMESPACE,
assetEntry
);

if (!assetResponse || !assetResponse.value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like assetResponse.value being falsy is a programming error on our part?

Copy link
Contributor Author

@CarmenPopoviciu CarmenPopoviciu Aug 23, 2024

Choose a reason for hiding this comment

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

technically this should be just a sanity check, but felt like we should check nonetheless. I'll ask the EWC folks if we want to throw a different error here. IMO a 500 sounds like the right thing to return

throw new Error(
`Requested asset ${assetKey} exists in the asset manifest but not in the KV namespace.`
`Requested asset ${assetEntry} exists in the asset manifest but not in the KV namespace.`
);
}

return new Response(content);
},
};
const { value: assetContent, metadata: assetMetadata } = assetResponse;
const additionalHeaders = getAdditionalHeaders(
assetEntry,
assetMetadata,
request
);
const headers = getMergedHeaders(request.headers, additionalHeaders);

return new OkResponse(assetContent, { headers });
}

private async getAssetEntry(request: Request) {
const url = new URL(request.url);
let { pathname } = url;

const assetsManifest = new AssetsManifest(this.env.ASSETS_MANIFEST);
pathname = globalThis.decodeURIComponent(pathname);
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this should be decoded because the assetManifest uses encoded pathnames?
Might be worth adding a test for url encoding with a funky pathname. Not high priority though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, so funky path names def don't work, with or without this decoding. Let's have a look together, and I'll add the proper fix/tests in a follow-up PR. Tracking in WC-2597


return await assetsManifest.get(pathname);
}
}
46 changes: 46 additions & 0 deletions packages/workers-shared/asset-server-worker/src/responses.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
export class OkResponse extends Response {
constructor(body: BodyInit | null, init?: ResponseInit) {
super(body, {
...init,
status: 200,
});
}
}

export class NotFoundResponse extends Response {
constructor(...[body, init]: ConstructorParameters<typeof Response>) {
super(body, {
...init,
status: 404,
statusText: "Not Found",
});
}
}

export class MethodNotAllowedResponse extends Response {
constructor(...[body, init]: ConstructorParameters<typeof Response>) {
super(body, {
...init,
status: 405,
statusText: "Method Not Allowed",
});
}
}

export class InternalServerErrorResponse extends Response {
constructor(err: Error, init?: ResponseInit) {
super(undefined, {
...init,
status: 500,
});
}
}

export class NotModifiedResponse extends Response {
constructor(...[_body, _init]: ConstructorParameters<typeof Response>) {
super(undefined, {
status: 304,
statusText: "Not Modified",
});
}
}
Loading
Loading