-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: allow D1 local commands to work without database_id #11804
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
fix: allow D1 local commands to work without database_id #11804
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: 509a79b The changes in this PR will be included in the next version bump. 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 |
|
Claude finished @emily-shen's task —— View job Changeset Review✅ All changesets look good Validation Summary:
The changeset properly describes the bug fix with appropriate detail. The title is clear and the use of locally (with italics) appropriately emphasizes the scope of the fix. |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
d60f443 to
2497179
Compare
|
I've not dug into the code yet, but from some local testing with 1: If you have 2 bindings pointing at the same database name, it will do the migration separately for each bindingto reproduce: wrangler.jsonc: migrations/0001_test-migration.sql: this can be empty This works as expected: I would expect migrating the second binding to also say 2: If you start without a database id, and then add it in later, it also attempts to migrate the db from scratchTo reproduce: After doing the migration above, change wrangler.toml to: and then run I think these are both symptoms of it not knowing how to map things to the appropriate .wrangler/state/v3/d1/miniflare-D1DatabaseObject/ sqlite db.
There are going to be bunch of horrible edge cases here, where old wrangler versions use one lookup scheme and new ones do something different. We can simulate the upgrade from current wrangler in tests by making bindings with just database_id, and then adding database_name later maybe? |
|
Thanks for the thorough testing! You've identified two real edge cases that stem from the choice of "local persistence identity" when Current behavior (this PR): Uses Your suggestions:
I intentionally matched Options:
What's the preferred approach here? I'm happy to expand the scope of this PR if maintainers want option (B), but wanted to check before making changes that affect Also worth noting: changing the fallback from |
alsuren
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a chat internally and decided to go with (A) Keep parity with wrangler dev for now.
Approved with comments.
| name: string, | ||
| options?: { | ||
| /** Local databases might not have a database id, so we don't require it for local-only operations */ | ||
| requireDatabaseId?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit that you don't necessarily need to do anything about): I am generally not a fan of optional boolean flags that default to true.
It seems that e.g. isLocal({local, remote}, default) defaults to true as well though, so that might be the wrangler code style.
If you're keeping it like this, maybe call out what the default is in the flag's docstring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think that's a losing battle to fight in this codebase 😅
but for my own interest, why? consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember where I picked up the habit. Possibly from writing untyped javascript, when "go to definition" in your editor wasn't so reliable. I have a feeling that being able to provide a default value for js positional function arguments is an ES6 thing, and it was common for people to just pass the resulting undefined into if statements, and relied on it being falsy? (maybe I'm misremembering?)
Probably just me showing my age, and not a thing people care about anymore.
I also have thing about boolean variables that have negative-sounding names. I have a very small brain, and those take me too long to read each time. I think we're okay here though.
| if (name === d1Database.database_name || name === d1Database.binding) { | ||
| if (requireDatabaseId && !d1Database.database_id) { | ||
| throw new UserError( | ||
| `Found a database with name or binding ${name} but it is missing a database_id, which is needed for operations on remote resources. Please create the remote D1 database by deploying your project or running 'wrangler d1 create ${name}'.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that adding a second binding with a matching binding/database_name but without a database_id might or might not poison future invocations of wrangler commands, or might not, depending on which order we iterate over things.
In the face of a config with some bindings having database_id and some not, Options are to make it consistently permissive (by raising a warning and then continuing?), make it consistently strict (by validating all d1_databases before returning), or leave it inconsistent as it is now.
This is a super edge case, and making things consistent might add unnecessary complexity, so use your judgement here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm i think this is enough of an edgecase to ignore - having duplicate bindings names is already forbidden, so that won't be an issue, and i can't really see why you'd end up having two bindings to the same database_name, and also only have one of them with a database_id.
1c9ef8c to
62812f8
Compare
62812f8 to
509a79b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #11802.
Now that resource provisioning means wrangler dev doesn't need database_ids, local versions of d1 commands also shouldn't need the id.
Link to Devin run: https://app.devin.ai/sessions/92cc6dea3f1543ecb6667ad7d1f85ad0
Devin PR requested by @emily-shen
(rewrote a lot of it though)