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

Wrap included files with include comment #211

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -770,10 +770,12 @@ and when the migration is committed or watched, the contents of `myfunction.sql`
will be included in the result, such that the following SQL is executed:

```sql
--! Included functions/myfunction.sql
create or replace function myfunction(a int, b int)
returns int as $$
select a + b;
$$ language sql stable;
--! EndIncluded functions/myfunction.sql
drop policy if exists access_by_numbers on mytable;
create policy access_by_numbers on mytable for update using (myfunction(4, 2) < 42);
```
Expand Down
12 changes: 12 additions & 0 deletions __tests__/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,15 @@ export const makeMigrations = (commitMessage?: string) => {
commitMessage ? `\n--! Message: ${commitMessage}` : ``
}\n\n${MIGRATION_NOTRX_TEXT.trim()}\n`;

const MIGRATION_INCLUDE_TEXT = `--!include foo.sql`;
const MIGRATION_INCLUDE_COMPILED = `${MIGRATION_INCLUDE_TEXT}\n${MIGRATION_1_TEXT}\n${MIGRATION_INCLUDE_TEXT}`;
const MIGRATION_INCLUDE_HASH = createHash("sha1")
.update(`${MIGRATION_INCLUDE_COMPILED.trim()}` + "\n")
.digest("hex");
const MIGRATION_INCLUDE_COMMITTED = `--! Previous: -\n--! Hash: sha1:${MIGRATION_INCLUDE_HASH}${
commitMessage ? `\n--! Message: ${commitMessage}` : ``
}\n\n${MIGRATION_INCLUDE_COMPILED}\n`;

const MIGRATION_MULTIFILE_FILES = {
"migrations/links/two.sql": "select 2;",
"migrations/current": {
Expand Down Expand Up @@ -308,6 +317,9 @@ select 3;
MIGRATION_NOTRX_TEXT,
MIGRATION_NOTRX_HASH,
MIGRATION_NOTRX_COMMITTED,
MIGRATION_INCLUDE_TEXT,
MIGRATION_INCLUDE_HASH,
MIGRATION_INCLUDE_COMMITTED,
MIGRATION_MULTIFILE_TEXT,
MIGRATION_MULTIFILE_HASH,
MIGRATION_MULTIFILE_COMMITTED,
Expand Down
13 changes: 12 additions & 1 deletion __tests__/include.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ it("compiles an included file", async () => {
FAKE_VISITED,
),
).toEqual(`\
--! Include foo.sql
select * from foo;
--! EndInclude foo.sql
`);
});

Expand All @@ -64,9 +66,17 @@ it("compiles multiple included files", async () => {
FAKE_VISITED,
),
).toEqual(`\
--! Include dir1/foo.sql
select * from foo;
--! EndInclude dir1/foo.sql
--! Include dir2/bar.sql
select * from bar;
--! EndInclude dir2/bar.sql
--! Include dir3/baz.sql
--! Include dir4/qux.sql
select * from qux;
--! EndInclude dir4/qux.sql
--! EndInclude dir3/baz.sql
`);
});

Expand Down Expand Up @@ -129,6 +139,7 @@ commit;
FAKE_VISITED,
),
).toEqual(`\
--! Include foo.sql
begin;

create or replace function current_user_id() returns uuid as $$
Expand All @@ -140,6 +151,6 @@ comment on function current_user_id is E'The ID of the current user.';
grant all on function current_user_id to :DATABASE_USER;

commit;

--! EndInclude foo.sql
`);
});
5 changes: 4 additions & 1 deletion __tests__/readCurrentMigration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,5 +111,8 @@ it("reads from current.sql, and processes included files", async () => {

const currentLocation = await getCurrentMigrationLocation(parsedSettings);
const content = await readCurrentMigration(parsedSettings, currentLocation);
expect(content).toEqual("-- TEST from foo");
expect(content).toEqual(`\
--! Included foo_current.sql
-- TEST from foo
--! EndIncluded foo_current.sql`);
});
32 changes: 32 additions & 0 deletions __tests__/uncommit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ describe.each([[undefined], ["My Commit Message"]])(
const {
MIGRATION_1_TEXT,
MIGRATION_1_COMMITTED,
MIGRATION_INCLUDE_TEXT,
MIGRATION_INCLUDE_COMMITTED,
MIGRATION_MULTIFILE_COMMITTED,
MIGRATION_MULTIFILE_FILES,
} = makeMigrations(commitMessage);
Expand Down Expand Up @@ -88,6 +90,36 @@ describe.each([[undefined], ["My Commit Message"]])(
).toEqual(MIGRATION_1_COMMITTED);
});

it("rolls back a migration that has included another file", async () => {
mockFs({
[`migrations/committed/000001${commitMessageSlug}.sql`]:
MIGRATION_INCLUDE_COMMITTED,
"migrations/current.sql": "-- JUST A COMMENT\n",
"migrations/fixtures/foo.sql": MIGRATION_1_TEXT,
});
await migrate(settings);
await uncommit(settings);

await expect(
fsp.stat("migrations/committed/000001.sql"),
).rejects.toMatchObject({
code: "ENOENT",
});
expect(await fsp.readFile("migrations/current.sql", "utf8")).toEqual(
(commitMessage ? `--! Message: ${commitMessage}\n\n` : "") +
MIGRATION_INCLUDE_TEXT.trim() +
"\n",
);

await commit(settings);
expect(
await fsp.readFile(
`migrations/committed/000001${commitMessageSlug}.sql`,
"utf8",
),
).toEqual(MIGRATION_INCLUDE_COMMITTED);
});

it("rolls back multifile migration", async () => {
mockFs({
[`migrations/committed/000001${commitMessageSlug}.sql`]:
Expand Down
9 changes: 8 additions & 1 deletion src/commands/uncommit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,16 @@ export async function _uncommit(parsedSettings: ParsedSettings): Promise<void> {
const contents = await fsp.readFile(lastMigrationFilepath, "utf8");
const { headers, body } = parseMigrationText(lastMigrationFilepath, contents);

// Remove included migrations
const includeRegex =
/^--![ \t]*Included[ \t]+(?<filename>.*?\.sql)[ \t]*$.*?^--![ \t]*EndIncluded[ \t]*\k<filename>[ \t]*$/gms;
Copy link
Member

Choose a reason for hiding this comment

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

We don't want newlines to be included in filenames, so I've removed the /s flag and been more explicit. I'm actually not sure what our validation is on include paths, whether they're allowed to contain spaces/etc? For now, I've decided a filename is anything but whitespace. I don't think we need to require here that it ends in .sql?

Suggested change
/^--![ \t]*Included[ \t]+(?<filename>.*?\.sql)[ \t]*$.*?^--![ \t]*EndIncluded[ \t]*\k<filename>[ \t]*$/gms;
/^--![ \t]*Included[ \t]+(?<filename>\S+)[ \t]*$[\s\S]*?^--![ \t]*EndIncluded[ \t]*\k<filename>[ \t]*$/gm;

const decompiledBody = body.replace(includeRegex, (match) => {
return match.split("\n")[0].replace(" Included", "include");
});
Comment on lines +48 to +50
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather use the capture group:

Suggested change
const decompiledBody = body.replace(includeRegex, (match) => {
return match.split("\n")[0].replace(" Included", "include");
});
const decompiledBody = body.replace(
includeRegex,
(_, filename) => `--! include ${filename}`,
);


// Drop Hash, Previous and AllowInvalidHash from headers; then write out
const { Hash, Previous, AllowInvalidHash, ...otherHeaders } = headers;
const completeBody = serializeMigration(body, otherHeaders);
const completeBody = serializeMigration(decompiledBody, otherHeaders);
await writeCurrentMigration(parsedSettings, currentLocation, completeBody);

// Delete the migration from committed and from the DB
Expand Down
8 changes: 5 additions & 3 deletions src/migration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export async function compileIncludes(
content: string,
processedFiles: ReadonlySet<string>,
): Promise<string> {
const regex = /^--![ \t]*include[ \t]+(.*\.sql)[ \t]*$/gm;
const regex = /^--![ \t]*[iI]nclude[ \t]+(.*\.sql)[ \t]*$/gm;
Copy link
Member

Choose a reason for hiding this comment

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

Why? Suggest we revert this:

Suggested change
const regex = /^--![ \t]*[iI]nclude[ \t]+(.*\.sql)[ \t]*$/gm;
const regex = /^--![ \t]*include[ \t]+(.*\.sql)[ \t]*$/gm;

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I didn't mean to leave this change in. I was toying around with the idea of writing it as --! Include ... before I concluded that amount of flexibility isn't worth it, particularly when uncommitting will clobber the formatting anyway.


// Find all includes in this `content`
const matches = [...content.matchAll(regex)];
Expand Down Expand Up @@ -205,10 +205,12 @@ export async function compileIncludes(
// Simple string replacement for each path matched
const compiledContent = content.replace(
regex,
(_match, rawSqlPath: string) => {
(match, rawSqlPath: string) => {
const sqlPath = sqlPathByRawSqlPath[rawSqlPath];
const content = contentBySqlPath[sqlPath];
return content;
const included = match.replace(/^--![ \t]*include/, "--! Included");
Copy link
Member

Choose a reason for hiding this comment

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

If you make the change above to allow Include as well as include, this will break. I don't think we want the change on line 135. I feel like we should check for no match, but meh.

const endIncluded = included.replace("Included", "EndIncluded");
return `${included}\n${content.trim()}\n${endIncluded}`;
},
);

Expand Down
Loading