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

🐛 BUG: .raw() does not work locally #2238

Closed
AlexBlokh opened this issue Nov 18, 2022 · 9 comments · Fixed by cloudflare/miniflare#474, cloudflare/miniflare#480 or #2717
Closed

🐛 BUG: .raw() does not work locally #2238

AlexBlokh opened this issue Nov 18, 2022 · 9 comments · Fixed by cloudflare/miniflare#474, cloudflare/miniflare#480 or #2717
Labels
bug Something that isn't working d1 Relating to D1 miniflare Relating to Miniflare

Comments

@AlexBlokh
Copy link

What version of Wrangler are you using?

2.4.2

What operating system are you using?

Mac

Describe the Bug

const rows = await env.DB.prepare('select id, email from users').raw();
console.log(rows);

prints locally:

Statement {
  busy: false,
  reader: true,
  readonly: true,
  source: 'select id, email from users',
  database: Database {
    name: '/Users/alexblokh/Development/drizzle-orm-new/cloudflare/.wrangler/state/d1/DB.sqlite3',
    open: true,
    inTransaction: false,
    readonly: false,
    memory: false
  }
}

prints in cloud:

[
  [
    "e@ma.il",
    6
  ],
  [
    "e@ma.il",
    7
  ]
]
@AlexBlokh AlexBlokh added the bug Something that isn't working label Nov 18, 2022
@rozenmd rozenmd added the d1 Relating to D1 label Nov 21, 2022
@lrapoport-cf lrapoport-cf moved this to Untriaged in workers-sdk Nov 22, 2022
@lrapoport-cf lrapoport-cf moved this from Untriaged to Other in workers-sdk Nov 22, 2022
@rozenmd rozenmd removed this from workers-sdk Nov 29, 2022
@maurerbot
Copy link

Plus 1 on this if possible

@rozenmd rozenmd added the miniflare Relating to Miniflare label Jan 3, 2023
@rozenmd
Copy link
Contributor

rozenmd commented Jan 3, 2023

FYI @mrbbot, looks like miniflare's behaviour doesn't match the d1 facade:

// packages/wrangler/templates/d1-beta-facade.js
async raw() {
		const s = firstIfArray(
			await this.database._send("/query", this.statement, this.params)
		);
		const raw = [];
		for (var r in s.results) {
			const entry = Object.keys(s.results[r]).map((k) => {
				return s.results[r][k];
			});
			raw.push(entry);
		}
		return raw;
	}

vs https://github.com/cloudflare/miniflare/blob/master/packages/d1/src/statement.ts#L127-L130

I'll make a PR updating miniflare to make its behaviour match the shim

@maurerbot
Copy link

thanks @rozenmd

@maurerbot
Copy link

any ETA on this?

@rozenmd
Copy link
Contributor

rozenmd commented Jan 10, 2023

@maurerbot we don't give ETAs, but PRs are also welcome if you'd like the fix ASAP.

@maurerbot
Copy link

I might do it. But only for the cred! 😄

@maurerbot
Copy link

maurerbot commented Jan 10, 2023

@rozenmd @mrbbot not sure if this is right? cloudflare/miniflare#471

Tests pass but could use guidance on how the abstraction works?

@repository
Copy link

repository commented Jan 15, 2023

Hi, please forgive me if I am missing something obvious, but I think we are overcomplicating this.

I looked at the docs for better-sqlite3 to figure out what the .raw() method does. As it turns out, it only enables the raw mode for that statement (and returns the statement), not actually executing it. In miniflare, it is only calling that .raw() from bsqlite3 and nothing further, so that's why we're getting a statement back as illustrated by AlexBlokh.

I simply called .all() after raw in miniflare and it seems to work now for me, but there might some case that I'm not aware of.

This PR implements that change and hopefully resolves this issue.
cloudflare/miniflare#474

@AlexBlokh
Copy link
Author

AlexBlokh commented Jan 15, 2023

@repository hey
.raw() does return a result of T[] due to your types and does it as stated when deployed to the worker

image

Latest worker types shows the same
image

mrbbot added a commit to cloudflare/miniflare that referenced this issue 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:
- 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 added a commit to cloudflare/miniflare that referenced this issue 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:
- 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 added a commit to cloudflare/miniflare that referenced this issue Jan 24, 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:
- 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
  cloudflare/workers-sdk#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}()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working d1 Relating to D1 miniflare Relating to Miniflare
Projects
None yet
4 participants