-
Notifications
You must be signed in to change notification settings - Fork 331
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
Refactor the D1 binding to use D1DatabaseSession for all actions #2624
Conversation
All contributors have signed the CLA ✍️ ✅ |
963937b
to
11a0b48
Compare
11a0b48
to
2382ee0
Compare
I have read the CLA Document and I hereby sign the CLA |
a970407
to
acc1db4
Compare
9e947e4
to
a65f7b5
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.
I won't comment on the details of the code, but the class structure seems reasonable, especially how the statements use the session class.
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.
Looks good! A bit of feedback, but nothing major, so consider this approved.
106f99d
to
629a599
Compare
...values, | ||
}); | ||
|
||
export async function testD1ApiQueriesHappyPath(DB) { |
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 was cut-pasted as-is, without any changes apart from adding the last section of DELETE statements, so that the database is cleaned up.
629a599
to
c74db2b
Compare
c74db2b
to
4b1c31e
Compare
Overview
This change prepares the D1 binding for the upcoming Sessions API (relevant https://blog.cloudflare.com/building-d1-a-global-database). Most of the changes is moving things around, but the highlights are as below:
D1DatabaseSession
class is introduced that maintains acommitTokenOrConstraint
property, and also takes ownership of thefetch
+_send/_sendOrThrow
methods.D1DatabaseSessionAlwaysPrimary
that extendsD1DatabaseSession
and essentially always sends the commit token constraintfirst-primary
.dump, exec
functions that the new Sessions API does not have.withSession()
function is"first-unconstrained"
(to run the first query (which may be a read or a write) with no constraints, and then use causal consistency for subsequent queries).Notes
I decided to design it such that
D1DatabaseSession
proxies all the calls and maintains the commit token so that the other classes don't really care about which session they use, or having to know about it.This way, all the complex logic about the commit token and how the interaction with the D1 DOs is going to be inside our eyeball worker, which we control fully, it's easier to deploy and fix, and is the first shared entrypoint with the REST API too, so once the REST API can pass the commit token to use, the rest of the logic is exactly the same.
I am not sure if using HTTP headers is common in Cloudflare to pass this kind of information to our APIs, so if that's not allowed/suggested, then we will need to pass it as query parameter instead.
Suggestions for naming of the header or the query parameter welcome.
Compatibility flag
There is a new
experimental
compatibility flagenable_d1_with_sessions_api
to expose this newwithSessions()
method. The default binding still uses theD1DatabaseSession
object though, so this PR's changes will actually be running after merge, always sending thefirst-primary
constraint.The
d1-api-test.js
is used by the D1 eyeball worker E2E tests so I added the new tests in a new test suite with the compatibility flag enabled only for that.Tests
The first target runs the default database tests, without using the sessions API, and without the compatibility flag enabled.
The second target runs all the test suites with the compatibility flag enabled.