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

fix(d1): ensure that migrations support compound statements #2495

Conversation

petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Jan 4, 2023

What this PR solves / how to test:

This fix updates the SQL statement splitting so that it does not split in the middle of compound statements.
Previously we were using a third party splitting library, but this needed fixing and was actually unnecessary for our purposes.
So a new splitter has been implemented and the library dependency removed.
Also the error handling in d1 migrations apply has been improved to handle a wider range of error types.

Fixes #2463

To test manually, create a simple migration script migrations/0000_initial.sql that contains a compound statement:

CREATE TABLE IF NOT EXISTS actors (
  id TEXT PRIMARY KEY,
  type TEXT NOT NULL,
  properties TEXT NOT NULL DEFAULT (json_object())
);

CREATE VIRTUAL TABLE IF NOT EXISTS search_fts USING fts5 (
    type,
    name,
);

CREATE TRIGGER IF NOT EXISTS actors_search_fts_update AFTER UPDATE ON actors
BEGIN
    DELETE FROM search_fts WHERE rowid=old.rowid;
    INSERT INTO search_fts (rowid, type, name, preferredUsername)
    VALUES (new.rowid,
            new.type,
            json_extract(new.properties, '$.name'),
            json_extract(new.properties, '$.preferredUsername'));
END;

Then update the wrangler to have a pseudo D1 binding:

[[d1_databases]]
binding = "DATABASE"
database_name = "database"
database_id = "xxxx-xxxx-xxxx-xxxx"

Finally try to apply the migrations locally:

npx wrangler d1 migrations apply --local database

Associated docs issues/PR:

  • N/A

Author has included the following, where applicable:

  • Tests
  • Changeset

Reviewer has performed the following, where applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

Fixes # [insert issue number].

@petebacondarwin petebacondarwin requested a review from a team as a code owner January 4, 2023 14:15
@changeset-bot
Copy link

changeset-bot bot commented Jan 4, 2023

🦋 Changeset detected

Latest commit: 0331160

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/runs/3855177256/npm-package-wrangler-2495

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/prs/2495/npm-package-wrangler-2495

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/runs/3855177256/npm-package-wrangler-2495 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/runs/3855177256/npm-package-cloudflare-pages-shared-2495

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #2495 (58ab7ae) into main (58ab7ae) will not change coverage.
The diff coverage is n/a.

❗ Current head 58ab7ae differs from pull request most recent head 0331160. Consider uploading reports for the commit 0331160 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2495   +/-   ##
=======================================
  Coverage   72.24%   72.24%           
=======================================
  Files         156      156           
  Lines        9685     9685           
  Branches     2547     2547           
=======================================
  Hits         6997     6997           
  Misses       2688     2688           

@petebacondarwin
Copy link
Contributor Author

The new splitter.ts file has 100% code coverage in unit tests 💯

@petebacondarwin petebacondarwin force-pushed the d1-fix-migration-statement-parsing branch from be2654a to 0475d66 Compare January 4, 2023 22:53
@petebacondarwin petebacondarwin force-pushed the d1-fix-migration-statement-parsing branch 2 times, most recently from 4a38e64 to 8f66793 Compare January 6, 2023 10:34
This fix updates the SQL statement splitting so that it does not split in the middle of compound statements.
Previously we were using a third party splitting library, but this needed fixing and was actually unnecessary for our purposes.
So a new splitter has been implemented and the library dependency removed.
Also the error handling in `d1 migrations apply` has been improved to handle a wider range of error types.

Fixes cloudflare#2463
@petebacondarwin petebacondarwin force-pushed the d1-fix-migration-statement-parsing branch from 8f66793 to 0331160 Compare January 6, 2023 12:23
@petebacondarwin petebacondarwin merged commit e93063e into cloudflare:main Jan 6, 2023
@petebacondarwin petebacondarwin deleted the d1-fix-migration-statement-parsing branch January 6, 2023 13:19
@github-actions github-actions bot mentioned this pull request Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: Incorrect SQL statement splitting
4 participants