✨ Adds consistent redirects handling for cf workers. #3895

danyx23 · 2024-08-22T15:52:52Z

Cloudflare has support for redirects but only up to a limited number (I think 2000?). We have a lot of chart redirects in the chart_redirects table (about 1600 or so). Because everything under the /grapher/ route on our website has to run through a CF function anyways (to update the thumbnail image to include the correct query string), we decided to bake these redirects into a json file and do custom redirects handling in the CF function.

We did this for html pages already but not for thumbnails, but now that we also have .config.json requests to worry about and would like to have consistent redirects handling that doesn't overly complicate the logic of our handlers.

This PR makes it so that both requests for the html pages at /grapher/[slug] and requests for /grapher/[slug].config.json will be redirected according to the _grapherRedirects.json file using a catch error handler. This is done as a catch handler that checks for 404 pages so that the common, happy path does not have to fetch the redirects file.

danyx23 · 2024-08-22T15:53:19Z

✨ Use grapher config from API instead of from HTML in multiembedder #3884 : 2 dependent PRs (#3613 , #3922 )
✨ Render thumbnails for graphers by uuid #3880
✨ add fetching grapher configs by UUID #3879
✨ Adds consistent redirects handling for cf workers. #3895 👈
✨ add CF function for getting SLUG.config.json #3870 : 1 other dependent PR (#3871 )
🔨 change CF thumbnail function to use chart configs from R2 #3867 : 1 other dependent PR (#3865 )
🔨 unify r2 config dirs #3945
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @danyx23 and the rest of your teammates on Graphite

owidbot · 2024-08-26T20:56:25Z

Quick links (staging server):

Site	Admin	Wizard

Login: ssh owid@staging-site-cf-function-consistent-redirects

Site-screenshots: https://github.com/owid/site-screenshots/compare/cf-function-consistent-redirects
SVG tester: https://github.com/owid/owid-grapher-svgs/compare/cf-function-consistent-redirects

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2024-09-10 08:28:01 UTC
Execution time: 1.19 seconds

marcelgerber

Nicely done!

I did find a bunch more cases that we need to cover, or may want to cover in a nicer way.

marcelgerber · 2024-09-02T10:32:04Z

functions/grapher/[slug].ts

+            "Access-Control-Allow-Origin": "*",
+        },
+    })
+}
+
 export const onRequestGet: PagesFunction = async (context) => {
-    // Makes it so that if there's an error, we will just deliver the original page before the HTML rewrite.
-    // Only caveat is that redirects will not be taken into account for some reason; but on the other hand the worker is so simple that it's unlikely to fail.
-    context.passThroughOnException()


We should really try to keep this in as the first line of business.

It allows us that if any unhandled exception happens in this handler, we're still failing open and are sending over the unmodified /grapher/... response as-is from the origin server. Which is a good fallback (and will work for graphers unless there's a redirect - but never for .json, .png and .svg, obviously.)

See https://developers.cloudflare.com/workers/runtime-apis/context/#passthroughonexception.

marcelgerber · 2024-09-02T10:40:01Z

functions/grapher/[slug].ts

+                const allExtensions = Object.values(extensions)
+                    .map((ext) => ext.replace(".", "\\.")) // for the regex make sure we match only a single dot, not any character
+                    .join("|")
+                const regexForKnownExtensions = new RegExp(
+                    `^(?<slug>.*?)(?<extension>${allExtensions})?$`
+                )


I think this code will be simpler by assuming that a slug can't contain a dot, i.e. something like /^(?<slug>[^\s\.]+)(?<extension>\.\S+)?$/.

In theory we could then check after the fact that the extension is valid (and throw a fitting error), but it's also fine if we're not doing that.

Yeah true but I think I like the idea of using a list of known extensions as a communication device that is added to further up in the stack. I'm inclined to leave it as is for now.

marcelgerber · 2024-09-02T10:46:06Z

functions/grapher/[slug].ts

+                const matchResult = fullslug.match(regexForKnownExtensions)
+                const slug = matchResult?.groups?.slug ?? fullslug
+                const extension = matchResult?.groups?.extension ?? ""


So, what this code doesn't currently seem to handle is non-lowercase slugs.
Our rule is that a slug needs to be all-lowercase, and if it isn't we want to redirect it.

In the old code, I tried to be clever about it so we never need to run through two redirects, avoiding the scenario REDIR-SOURCE -> redir-source -> redir-target. I don't think we need to take care of that again, which means we would just do:

if (slug.toLowerCase() !== slug) return redir(302, slug.toLowerCase())

marcelgerber · 2024-09-02T10:47:17Z

functions/_common/grapherRenderer.ts

@@ -215,6 +217,13 @@ export async function fetchGrapherConfig(
 ): Promise<FetchGrapherConfigResult> {
    const fetchResponse = await fetchUnparsedGrapherConfig(slug, env, etag)

+    if (fetchResponse.status === 404) {
+        // we throw 404 errors instad of returning a 404 response so that the router


Suggested change

// we throw 404 errors instad of returning a 404 response so that the router

// we throw 404 errors instead of returning a 404 response so that the router

This means that both request for the html pages at /grapher/[slug] and requests for /grapher/[slug].config.json will be redirected according to the _grapherRedirects.json file. This is done as a catch handler that checks for 404 pages so that the common, happy path does not have to fetch the redirects file.

danyx23 · 2024-09-10T11:13:53Z

Merge activity

Sep 10, 7:13 AM EDT: @danyx23 started a stack merge that includes this pull request via Graphite.
Sep 10, 7:14 AM EDT: @danyx23 merged this pull request with Graphite.

This was referenced Aug 22, 2024

✨ add fetching grapher configs by UUID #3879

Merged

✨ Render thumbnails for graphers by uuid #3880

Merged

github-actions bot assigned danyx23 Aug 22, 2024

This was referenced Aug 22, 2024

✨ add CF function for getting SLUG.config.json #3870

Merged

🔨 add fallback to prod for grapher config retrieval #3871

Closed

danyx23 mentioned this pull request Aug 22, 2024

✨ Use grapher config from API instead of from HTML in multiembedder #3884

Merged

danyx23 marked this pull request as ready for review August 22, 2024 16:01

danyx23 requested a review from marcelgerber August 22, 2024 16:01

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from 484bd0d to 6aa79a3 Compare August 23, 2024 11:21

danyx23 force-pushed the cf-function-consistent-redirects branch from afa702d to 9b7ba3d Compare August 23, 2024 11:21

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from 6aa79a3 to 7a9ac89 Compare August 26, 2024 20:45

danyx23 force-pushed the cf-function-consistent-redirects branch from 9b7ba3d to af2f857 Compare August 26, 2024 20:45

This was referenced Aug 26, 2024

Pyodide notebooks support #3603

Draft

✨ generate csv and zip file server side #3613

Merged

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from 7a9ac89 to 797db83 Compare August 28, 2024 15:50

danyx23 force-pushed the cf-function-consistent-redirects branch from af2f857 to 08a9354 Compare August 28, 2024 15:50

marcelgerber reviewed Sep 2, 2024

View reviewed changes

sophiamersmann force-pushed the fetch-grapher-config-from-r2 branch from 797db83 to 7b3ec48 Compare September 2, 2024 16:01

sophiamersmann force-pushed the cf-function-consistent-redirects branch from 08a9354 to f080612 Compare September 2, 2024 16:01

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from 7b3ec48 to 2b3e22c Compare September 3, 2024 07:59

danyx23 force-pushed the cf-function-consistent-redirects branch from f080612 to 61f8b29 Compare September 3, 2024 07:59

sophiamersmann force-pushed the fetch-grapher-config-from-r2 branch from 2b3e22c to 2c10fea Compare September 3, 2024 09:52

sophiamersmann force-pushed the cf-function-consistent-redirects branch from 61f8b29 to e6c9b7c Compare September 3, 2024 09:52

sophiamersmann force-pushed the fetch-grapher-config-from-r2 branch from 2c10fea to 1d38e5c Compare September 3, 2024 10:24

sophiamersmann force-pushed the cf-function-consistent-redirects branch from e6c9b7c to d5326c5 Compare September 3, 2024 10:24

sophiamersmann force-pushed the fetch-grapher-config-from-r2 branch from 1d38e5c to d07caed Compare September 3, 2024 13:29

sophiamersmann force-pushed the cf-function-consistent-redirects branch from d5326c5 to 9b79ac8 Compare September 3, 2024 13:30

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from d07caed to 38489b5 Compare September 3, 2024 14:18

danyx23 force-pushed the cf-function-consistent-redirects branch from 546f675 to ecffac1 Compare September 5, 2024 12:31

danyx23 mentioned this pull request Sep 5, 2024

mdd: fetch variable-level config by uuid #3922

Merged

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from bc65d56 to ba14771 Compare September 5, 2024 18:36

danyx23 force-pushed the cf-function-consistent-redirects branch from ecffac1 to 2b761ca Compare September 5, 2024 18:37

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from ba14771 to ff124a9 Compare September 9, 2024 09:33

danyx23 force-pushed the cf-function-consistent-redirects branch from 2b761ca to ade6496 Compare September 9, 2024 09:33

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from ff124a9 to 909e78d Compare September 9, 2024 11:53

danyx23 force-pushed the cf-function-consistent-redirects branch from ade6496 to a8553c5 Compare September 9, 2024 11:53

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from 909e78d to fc689f5 Compare September 9, 2024 14:43

danyx23 force-pushed the cf-function-consistent-redirects branch from a8553c5 to 563e74b Compare September 9, 2024 14:44

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from fc689f5 to 6d2c969 Compare September 10, 2024 07:59

danyx23 force-pushed the cf-function-consistent-redirects branch from 563e74b to 275fa56 Compare September 10, 2024 07:59

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from 6d2c969 to 8abfd52 Compare September 10, 2024 08:06

danyx23 force-pushed the cf-function-consistent-redirects branch from 275fa56 to c902103 Compare September 10, 2024 08:06

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from 8abfd52 to 48bd302 Compare September 10, 2024 09:55

danyx23 force-pushed the cf-function-consistent-redirects branch from c902103 to b0cd9d5 Compare September 10, 2024 09:55

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from 48bd302 to 29ecc72 Compare September 10, 2024 10:23

danyx23 force-pushed the cf-function-consistent-redirects branch from b0cd9d5 to 2f44d84 Compare September 10, 2024 10:24

danyx23 force-pushed the fetch-grapher-config-from-r2 branch from 29ecc72 to 5fdd975 Compare September 10, 2024 10:34

danyx23 force-pushed the cf-function-consistent-redirects branch from 2f44d84 to 3589dce Compare September 10, 2024 10:35

danyx23 changed the base branch from fetch-grapher-config-from-r2 to graphite-base/3895 September 10, 2024 10:54

danyx23 force-pushed the cf-function-consistent-redirects branch from 3589dce to d25b1ef Compare September 10, 2024 10:54

danyx23 changed the base branch from graphite-base/3895 to master September 10, 2024 10:55

danyx23 merged commit 3a153b9 into master Sep 10, 2024
21 checks passed

danyx23 deleted the cf-function-consistent-redirects branch September 10, 2024 11:14

This was referenced Oct 4, 2024

🚧 Draft of grapher download UI that uses the new server side CSV generation. #4014

Closed

Marcel's enhancements to server-side csv generation #4051

Merged

🧹 Restructure CF functions code into smaller files #4054

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Adds consistent redirects handling for cf workers. #3895

✨ Adds consistent redirects handling for cf workers. #3895

danyx23 commented Aug 22, 2024 •

edited

Loading

danyx23 commented Aug 22, 2024 •

edited

Loading

owidbot commented Aug 26, 2024 •

edited

Loading

marcelgerber left a comment

marcelgerber Sep 2, 2024

marcelgerber Sep 2, 2024

danyx23 Sep 3, 2024

marcelgerber Sep 2, 2024

marcelgerber Sep 2, 2024

danyx23 commented Sep 10, 2024 •

edited

Loading

	// we throw 404 errors instad of returning a 404 response so that the router
	// we throw 404 errors instead of returning a 404 response so that the router

✨ Adds consistent redirects handling for cf workers. #3895

✨ Adds consistent redirects handling for cf workers. #3895

Conversation

danyx23 commented Aug 22, 2024 • edited Loading

danyx23 commented Aug 22, 2024 • edited Loading

owidbot commented Aug 26, 2024 • edited Loading

marcelgerber left a comment

Choose a reason for hiding this comment

marcelgerber Sep 2, 2024

Choose a reason for hiding this comment

marcelgerber Sep 2, 2024

Choose a reason for hiding this comment

danyx23 Sep 3, 2024

Choose a reason for hiding this comment

marcelgerber Sep 2, 2024

Choose a reason for hiding this comment

marcelgerber Sep 2, 2024

Choose a reason for hiding this comment

danyx23 commented Sep 10, 2024 • edited Loading

Merge activity

danyx23 commented Aug 22, 2024 •

edited

Loading

danyx23 commented Aug 22, 2024 •

edited

Loading

owidbot commented Aug 26, 2024 •

edited

Loading

danyx23 commented Sep 10, 2024 •

edited

Loading