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

Implement D1Database using D1JS and D1 API #480

Merged
merged 1 commit into from
Jan 24, 2023
Merged

Implement D1Database using D1JS and D1 API #480

merged 1 commit into from
Jan 24, 2023

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Jan 23, 2023

Previously, Miniflare had its own implementations of BetaDatabase and Statement. These were subtly different to the implementations in the D1 Wrangler shim D1JS, causing behaviour mismatches.

This change switches the implementation to use the shim, with Miniflare implementing the underlying D1 HTTP API instead. We'll need to do this anyway when adding D1 support to Miniflare 3 using workerd.

Specific changes:

Previously, Miniflare had its own implementations of `BetaDatabase`
and `Statement`. These were subtly different to the implementations
in the D1 Wrangler shim D1JS, causing behaviour mismatches.

This change switches the implementation to use the shim, with
Miniflare implementing the underlying D1 HTTP API instead. We'll
need to do this anyway when adding D1 support to Miniflare 3 using
`workerd`.

Specific changes:
- Throw when calling `D1PreparedStatement#run()` with statements that
  return data, closes #441
- Fix response envelope format, closes #442 and
  cloudflare/workers-sdk#2504
- Fix binding/return of `BLOB`-typed values, closes wrangler2#2527
- Fix `D1Database#raw()` return, closes cloudflare/workers-sdk#2238
  (already fixed in #474)
- Add support for `D1Database#dump()`
- Run `D1Database#{batch,exec}()` statements in implicit transaction
- Only run first statement when calling
  `D1PreparedStatement#{first,run,all,raw}()`
@mrbbot mrbbot requested review from geelen, rozenmd and penalosa January 23, 2023 23:38
@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2023

⚠️ No Changeset found

Latest commit: a54e8b7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link
Contributor

@geelen geelen left a comment

Choose a reason for hiding this comment

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

Ahh, so you're still expecting Wrangler not to shim the binding (see https://github.com/cloudflare/wrangler2/blob/main/packages/wrangler/templates/d1-beta-facade.js#L217-L219) but then you're having Miniflare use the shim code to generate the right API?

That should work, but you might like to consider changing Wrangler's behaviour to always inject the shim, and then Miniflare's D1 only needs to export the D1DatabaseAPI with its .fetch method.

I'm not sure if your testing story would be worse in that case, though, so maybe it's better to keep it as it is... In fact, we do want the Wrangler shim to disappear eventually (which is why I built the Miniflare stuff the way I did), so this is probably the right approach.

Approved.

@mrbbot
Copy link
Contributor Author

mrbbot commented Jan 24, 2023

I'm not sure if your testing story would be worse in that case

Yeah, the reason I did it this way is so users can continue to run Miniflare standalone (with Miniflare's CLI or in the testing environments) without having to bundle with Wrangler first. This would be a breaking change otherwise.

For Miniflare 3, we'll probably switch to Wrangler doing the shimming, or if you're planning to remove that, none at all. 👍

@mrbbot mrbbot merged commit 2d0fbc6 into master Jan 24, 2023
@mrbbot mrbbot deleted the bcoll/d1-fixes branch January 24, 2023 11:39
@ruslantalpa
Copy link

@mrbbot
I think this PR broke queries like select ?1 + ?2

statements.ts used to have this code

      // For statements using ?1 ?2, etc, we want to pass them as varargs but
      // "better" sqlite3 wants them as an object of {1: params[0], 2: params[1], ...}
      if (this.#bindings.length > 0 && typeof this.#bindings[0] !== "object") {
        return prepared.bind(
          Object.fromEntries(this.#bindings.map((v, i) => [i + 1, v]))
        );
      } else {
        throw e;
      }

@ruslantalpa
Copy link

i added these lines to one of the tests (to make sure it's not my code):

const sum = await db.prepare("SELECT ?1 + ?2 as sum").bind(1, 2).first("sum");
t.is(sum, 3);

and the test fails

d1 › test › d1js.ts › D1PreparedStatement: first

  packages/d1/test/d1js.spec.ts:212

   211:
   212:   const sum = await db.prepare("SELECT ?1 + ?2 as sum").bind(1, 2).first("sum");
   213:   t.is(sum, 3);

  Rejected promise returned by test. Reason:

  Error {
    message: 'D1_ERROR',
  }

  › D1Database._send (packages/d1/src/d1js.ts:148:13)
  › D1PreparedStatement.first (packages/d1/src/d1js.ts:215:7)
  › packages/d1/test/d1js.spec.ts:212:15

  ─

mrbbot added a commit that referenced this pull request Mar 22, 2023
`better-sqlite3` expects parameters of the form `?1, ?2, ...` to be
bound as an object of the form `{ 1: params[0], 2: params[1], ...}`.
In #480, we accidentally removed the code that handled this case.
This PR adds it back, and lifts out some common functionality into a
`#prepareAndBind()` function. :)

Thanks @ruslantalpa for spotting the removed code.

Closes #526
Closes cloudflare/workers-sdk#2811
Closes cloudflare/workers-sdk#2887
mrbbot added a commit that referenced this pull request Mar 23, 2023
…544)

`better-sqlite3` expects parameters of the form `?1, ?2, ...` to be
bound as an object of the form `{ 1: params[0], 2: params[1], ...}`.
In #480, we accidentally removed the code that handled this case.
This PR adds it back, and lifts out some common functionality into a
`#prepareAndBind()` function. :)

Thanks @ruslantalpa for spotting the removed code.

Closes #526
Closes cloudflare/workers-sdk#2811
Closes cloudflare/workers-sdk#2887
mrbbot added a commit that referenced this pull request May 4, 2023
In #480, we updated the D1 implementation to use the same D1 shim
injected by Wrangler. This caused the `@miniflare/d1` API to change
a little, but we didn't update the `README`. :(
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants