Enable short URLs in the playground #6383

charliermarsh · 2023-08-07T04:45:44Z

Summary

This PR adds a Workers KV-based database to the playground, which enables us to associate shared snippets with a stable ID, which in turn allows us to generate short URLs, rather than our existing extremely-long URLs.

For now, the URLs are based on UUID, so they look like https://play.ruff.rs/a1c40d58-f643-4a3e-bc23-15021e16acef. (This URL isn't expected to work, as the playground isn't deployed; it's only included as an example.)

There are no visible changes in the UI here -- you still click the "Share" button, which copies the link to your URL. There's no user-visible latency either -- KV is very fast.

For context, with Workers KV, we provision a Workers KV store in our Cloudflare account (wrangler kv:namespace create "PLAYGROUND"), and then create a Cloudflare Worker that's bound to the KV store via the wrangler.toml:

name = "db"
main = "src/index.ts"
compatibility_date = "2023-08-07"

kv_namespaces = [
  { binding = "PLAYGROUND", id = "672e16c4fb5e4887845973bf0e9f6021", preview_id = "0a96477e116540e5a6e1eab6d6e7523e" }
]

The KV store exists in perpetuity, while the Worker can be updated, deployed, removed, etc. independently of the KV store. The Worker itself has unfettered access to the KV store. The Worker is exposed publicly, and just does some basic verification against the request host.

charliermarsh · 2023-08-07T04:46:03Z

playground/db/tsconfig.json

@@ -0,0 +1,105 @@
+{


(This is generated by wrangler init.)

charliermarsh · 2023-08-07T04:48:45Z

playground/src/Editor/settings.ts

+  settingsSource: string,
+  pythonSource: string,
+): Promise<void> {
+  const id = uuidv4();


Perhaps this should instead be a truncated hex string of the hash, so that you get a stable URL for a given snippet?

What about hash collisions?

I guess it depends on the desired UX:

Should users be able to keep making changes to the linked version even after sharing (and support different versions)

or do we want unique URLs for each possible playgorund.

I'd expect the second one for now.

I worry about truncated hash collisions though?

Yeah I think the second one is the correct model. It would perhaps be nice if hitting Share multiple times on the same snippet gave you the same URL though, that's what I meant by "Stable URL" (same content leads to same URL).

What I like about astexplorer.net is that the workflow is explicit about when it creates a new gist vs updating the existing one.

The Rust playground creates a new gist whenever you hit save. (Gists everywhere)

MichaReiser

Neat. A few follow up questions

Do all core contributors have the necessary permissions to log-in to our KV storage?
Does this break existing playgrounds? If so, would it be difficult to continue supporting the old URL format to not break links in issues?
How does the frontend authenticate against the key value storage?

playground/db/README.md

MichaReiser · 2023-08-07T06:31:56Z

playground/db/src/index.ts

+      "Access-Control-Allow-Origin": "*",
+    };
+
+    switch (request.method) {


Could you add some documentation about the supported URL schema? Or / and refactor out the common path (getting the PLAYGROUND env variable and doing some sort of slicing on the pathname (this is where an inline comment would be useful, what's the thing we remove?

Could we use url.hash? (assuming this is what you're slicing off)

playground/src/Editor/Editor.tsx

MichaReiser · 2023-08-07T06:33:08Z

playground/src/Editor/db.ts

Nit: Move outside of Editor and/or rename to api?

playground/src/Editor/db.ts

MichaReiser · 2023-08-07T06:39:42Z

playground/src/Editor/settings.ts

+  settingsSource: string,
+  pythonSource: string,
+): Promise<void> {
+  const id = uuidv4();


What about hash collisions?

I guess it depends on the desired UX:

Should users be able to keep making changes to the linked version even after sharing (and support different versions)

or do we want unique URLs for each possible playgorund.

MichaReiser · 2023-08-07T06:40:36Z

playground/src/Editor/db.ts

+  if (!response.ok) {
+    throw new Error("Failed to save.");
+  }


Can we include some details from the response to ease debugging?

charliermarsh · 2023-08-07T13:37:03Z

Do all core contributors have the necessary permissions to log-in to our KV storage?

They will, once we add folks to the Cloudflare team. I intentionally created this in the team Cloudflare account rather than my personal :)

Does this break existing playgrounds? If so, would it be difficult to continue supporting the old URL format to not break links in issues?

It does break existing playgrounds but I can fix it.

How does the frontend authenticate against the key value storage?

So because the site is public facing, I don't know if there's anything we can do that will be completely unspoofable. My plan is to just guard based on the request host in the Worker, so there's at least something. But even an API key would be spoofable. (We should definitely not expose deletions on the public API though.)

MichaReiser · 2023-08-07T13:55:29Z

They will, once we add folks to the Cloudflare team. I intentionally created this in the team Cloudflare account rather than my personal :)

I would prefer holding off with this PR until this happened. It otherwise prevents other people from contributing to the playground. Do we have a setup for external contributors?

So because the site is public facing, I don't know if there's anything we can do that will be completely unspoofable. My plan is to just guard based on the request host in the Worker, so there's at least something. But even an API key would be spoofable. (We should definitely not expose deletions on the public API though.)

Okay. It would help me if you can add some more context to the PR how this looks deployed (I'm not familiar with CF workers). Do we deploy our own backend and the backend uses a KV store? The question then is how we authenticate against the frontend?

Have you considered using a github gist instead? The nice thing about a gist is that it could also be shareable in issues because it stores separate files for the source and configuraiton.

charliermarsh · 2023-08-07T13:56:39Z

I would prefer holding off with this PR until this happened. It otherwise prevents other people from contributing to the playground. Do we have a setup for external contributors?

Can you describe how this prevents others from contributing to the playground? Why is access to this functionality required to contribute to the playground?

MichaReiser · 2023-08-07T13:58:59Z

Can you describe how this prevents others from contributing to the playground? Why is access to this functionality required to contribute to the playground?

I probably don't understand the architecture. So I don't know. Maybe the question should be different. Can external contributors make changes to the playground without having to set up their own backend (including share functionality)?

charliermarsh · 2023-08-07T20:34:49Z

I probably don't understand the architecture. So I don't know. Maybe the question should be different. Can external contributors make changes to the playground without having to set up their own backend (including share functionality)?

The setup no longer requires logging in to Cloudflare for any development. wrangler dev --local runs a local key-value store and persists the data locally.

charliermarsh · 2023-08-07T20:45:49Z

Have you considered using a github gist instead? The nice thing about a gist is that it could also be shareable in issues because it stores separate files for the source and configuraiton.

This is interesting but aren't the local development and auth challenges here much harder, since you'd need a GitHub API key in order to develop locally? I suppose we'd also still need a Worker so that we could deploy the API key to a server rather than making requests from the client directly. Workers and KV are also fast enough that there is no discernible latency vs. using local storage, which is a nice property.

charliermarsh · 2023-08-07T20:51:00Z

Does this break existing playgrounds? If so, would it be difficult to continue supporting the old URL format to not break links in issues?

Existing URLs are now supported.

zanieb · 2023-08-07T22:54:35Z

Nothing is persisted until the share button is pressed, correct?
Can we easily egress data from the KV store if we want to switch providers?

charliermarsh · 2023-08-08T00:28:44Z

Nothing is persisted until the share button is pressed, correct?

Correct.

Can we easily egress data from the KV store if we want to switch providers?

Yes, we can paginate over the KV store (list keys) and export the key-value pairs, but it would admittedly require writing some kind of script to do the export itself (e.g. https://github.com/stevenpack/cloudflare-workers-kv-export).

MichaReiser

The code changes look good to me. I would find a short note saying that we now have a backend service (maybe call it api rather than db because the db is the key value store) useful.

We should also look into how we want to protect the API endpoint. At least deploy what cloudflare gives us to avoid trivial attacks

charliermarsh · 2023-08-09T02:40:00Z

I would find a short note saying that we now have a backend service (maybe call it api rather than db because the db is the key value store) useful.

I expanded in the README, but let me know if I'm misinterpreting the request.

MichaReiser

Looks good. Please take a look at the error handling in db.ts and grant the team permission to cloudflare.

playground/README.md

playground/api/README.md

playground/api/src/index.ts

playground/src/Editor/db.ts

MichaReiser · 2023-08-09T11:45:23Z

I thought about the hash collision problem a bit more and think it would be good to make the API less generic, so that it can act more intelligently. We don't need to do this as part of this PR but I think it is a worthwhile improvement:

Change the POST (should probably be PUT) API to accept the playground state as JSON payload
The API generates a key based on the payload and tries to get an entry with the key
- If an existing entry is present
  - If the payload is identical, return the key
  - Otherwise add a -{i} or similar to the input before re-hashing the data. Retry
- If no entry exists, create it
Return the key

This still leaves the risk of a race, but this is extremely unlikely (needs a hash collision + race).

charliermarsh · 2023-08-09T13:22:36Z

I’m happy to make the IDs content-addressed as described, but I don’t know that I feel the need to design around hash collisions. We can just use the full SHA if we want, right?

MichaReiser · 2023-08-09T17:45:56Z

I’m happy to make the IDs content-addressed as described, but I don’t know that I feel the need to design around hash collisions. We can just use the full SHA if we want, right?

Thank's for pushing back. I'm sometimes too fast with my ideas. I forgot that we're using uuid keys, and the likelihood of a uuid collision is extremely low. Moving the uuid generation to the server (and returning it rather than having the client provide it) could be a nice change because it allows us to drop the uuid dependency from the frontend code. Adding a loop to testing for uuid collisions is straightforward but probably not worth the effort. So we could keep the API as is (client provides a content string), except that the server computes the id

charliermarsh · 2023-08-09T23:28:27Z

Sounds good. Do you have a preference between (1) server generates a UUID, or (2) server hashes the contents and returns the full SHA as the ID? (The latter would mean that we return the same URL given the same playground contents, which is a nice property, but isn't fully required.)

MichaReiser · 2023-08-10T06:08:18Z

Sounds good. Do you have a preference between (1) server generates a UUID, or (2) server hashes the contents and returns the full SHA as the ID? (The latter would mean that we return the same URL given the same playground contents, which is a nice property, but isn't fully required.)

UUID seems simpler but I don't have a strong preference. And changing it later is easy enough.

## Summary This PR adds a [Workers KV](https://developers.cloudflare.com/workers/runtime-apis/kv/)-based database to the playground, which enables us to associate shared snippets with a stable ID, which in turn allows us to generate short URLs, rather than our existing extremely-long URLs. For now, the URLs are based on UUID, so they look like https://play.ruff.rs/a1c40d58-f643-4a3e-bc23-15021e16acef. (This URL isn't expected to work, as the playground isn't deployed; it's only included as an example.) There are no visible changes in the UI here -- you still click the "Share" button, which copies the link to your URL. There's no user-visible latency either -- KV is very fast. For context, with Workers KV, we provision a Workers KV store in our Cloudflare account (`wrangler kv:namespace create "PLAYGROUND"`), and then create a Cloudflare Worker that's bound to the KV store via the `wrangler.toml`: ```toml name = "db" main = "src/index.ts" compatibility_date = "2023-08-07" kv_namespaces = [ { binding = "PLAYGROUND", id = "672e16c4fb5e4887845973bf0e9f6021", preview_id = "0a96477e116540e5a6e1eab6d6e7523e" } ] ``` The KV store exists in perpetuity, while the Worker can be updated, deployed, removed, etc. independently of the KV store. The Worker itself has unfettered access to the KV store. The Worker is exposed publicly, and just does some basic verification against the request host.

charliermarsh added the playground A playground-specific issue label Aug 7, 2023

charliermarsh commented Aug 7, 2023

View reviewed changes

playground/db/tsconfig.json Outdated

@@ -0,0 +1,105 @@

{

Copy link

Member Author

charliermarsh Aug 7, 2023

Choose a reason for hiding this comment

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

(This is generated by wrangler init.)

charliermarsh force-pushed the charlie/urls branch 3 times, most recently from 29561f7 to d9ad973 Compare August 7, 2023 04:48

charliermarsh commented Aug 7, 2023

View reviewed changes

MichaReiser reviewed Aug 7, 2023

View reviewed changes

charliermarsh force-pushed the charlie/urls branch from d9ad973 to 08f803f Compare August 7, 2023 20:32

charliermarsh force-pushed the charlie/urls branch from 08f803f to 76288cf Compare August 7, 2023 20:42

charliermarsh force-pushed the charlie/urls branch 2 times, most recently from c4805f1 to 7e81b89 Compare August 7, 2023 21:09

MichaReiser requested changes Aug 8, 2023

View reviewed changes

charliermarsh force-pushed the charlie/urls branch from 7e81b89 to ce057f4 Compare August 9, 2023 02:37

MichaReiser approved these changes Aug 9, 2023

View reviewed changes

playground/README.md Show resolved Hide resolved

playground/api/README.md Outdated Show resolved Hide resolved

playground/api/src/index.ts Show resolved Hide resolved

playground/src/Editor/db.ts Outdated Show resolved Hide resolved

playground/src/Editor/db.ts Outdated Show resolved Hide resolved

Enable short URLs in the playground

27fc94e

charliermarsh added 4 commits August 10, 2023 21:49

Rename need for Cloudflare login

e6d2732

Respect existing URLs

4b7836d

Rename to API; add architecture to README

a2e3857

Restrict API; create IDs on the server

133e700

charliermarsh force-pushed the charlie/urls branch from ce057f4 to 133e700 Compare August 11, 2023 02:20

charliermarsh enabled auto-merge (squash) August 11, 2023 02:27

charliermarsh merged commit 5633745 into main Aug 11, 2023

charliermarsh deleted the charlie/urls branch August 11, 2023 02:31

chenrui333 mentioned this pull request Aug 17, 2023

ruff 0.0.285 Homebrew/homebrew-core#139823

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable short URLs in the playground #6383

Enable short URLs in the playground #6383

charliermarsh commented Aug 7, 2023 •

edited

Loading

charliermarsh Aug 7, 2023

charliermarsh Aug 7, 2023

MichaReiser Aug 7, 2023

zanieb Aug 7, 2023

zanieb Aug 7, 2023

charliermarsh Aug 7, 2023

MichaReiser Aug 8, 2023

MichaReiser left a comment

MichaReiser Aug 7, 2023

MichaReiser Aug 7, 2023 •

edited

Loading

MichaReiser Aug 7, 2023

MichaReiser Aug 7, 2023

charliermarsh commented Aug 7, 2023

MichaReiser commented Aug 7, 2023 •

edited

Loading

charliermarsh commented Aug 7, 2023

MichaReiser commented Aug 7, 2023

charliermarsh commented Aug 7, 2023

charliermarsh commented Aug 7, 2023

charliermarsh commented Aug 7, 2023

zanieb commented Aug 7, 2023

charliermarsh commented Aug 8, 2023

MichaReiser left a comment

charliermarsh commented Aug 9, 2023

MichaReiser left a comment

MichaReiser commented Aug 9, 2023

charliermarsh commented Aug 9, 2023

MichaReiser commented Aug 9, 2023

charliermarsh commented Aug 9, 2023

MichaReiser commented Aug 10, 2023

Enable short URLs in the playground #6383

Enable short URLs in the playground #6383

Conversation

charliermarsh commented Aug 7, 2023 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Aug 7, 2023

MichaReiser commented Aug 7, 2023 • edited Loading

charliermarsh commented Aug 7, 2023

MichaReiser commented Aug 7, 2023

charliermarsh commented Aug 7, 2023

charliermarsh commented Aug 7, 2023

charliermarsh commented Aug 7, 2023

zanieb commented Aug 7, 2023

charliermarsh commented Aug 8, 2023

MichaReiser left a comment

Choose a reason for hiding this comment

charliermarsh commented Aug 9, 2023

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser commented Aug 9, 2023

charliermarsh commented Aug 9, 2023

MichaReiser commented Aug 9, 2023

charliermarsh commented Aug 9, 2023

MichaReiser commented Aug 10, 2023

charliermarsh commented Aug 7, 2023 •

edited

Loading

MichaReiser Aug 7, 2023 •

edited

Loading

MichaReiser commented Aug 7, 2023 •

edited

Loading