Wrap included files with include comment #211

astephensen · 2024-04-16T10:09:38Z

Description

This PR will wrap included files with comments, as first described in #208.

This also facilitates removing the included content when performing an uncommit.

Have opened as a draft as I'm open to any and all feedback!

Pre-Commit

current.sql

select 'foo';
--!include functions/hello.sql

fixtures/functions/hello.sql

create or replace function public.hello() returns text as $$
  select 'Hello, world!';
$$ language sql;

Committed

migrations/000001-world.sql

--! Previous: -
--! Hash: sha1:2d5fea0f4300a51ebf5c9d2416def4f2d22563d3
--! Message: World

select 'foo';
--! Included functions/hello.sql
create or replace function public.hello() returns text as $$
  select 'Hello, world!';
$$ language sql;
--! EndIncluded functions/hello.sql

Uncommitted

current.sql

--! Message: World

select 'foo';
--!include functions/hello.sql

Implementation

The method I implemented is similar to what @jcgsville described in the original issue, with a few little changes.

The colon was dropped between the header and the filename — I was encountering issues with --! Include: being treated as an actual header if the first statement was an include. Figured dropping the colon helps distinguish this as a file include, but I'm open to suggestions on how to fix this!
The filename is also added after the --! EndIncluded — This allows a simple regex back reference to be used to delete the included file (if the filename wasn't added, nested includes could be trickier to remove).

The main downside of this approach is the original formatting of the --!include is not preseved, i.e. if the include is written as --! include ... the migration will not restore the additional whitespace when uncommitting.

Notes

I had to run prettier across a lot of files that I didn't touch — not sure if this was missed in a previous PR or if I didn't have it configured correctly locally, but it was causing the tests to fail.

Performance impact

unknown - likely minimal.

Security impact

unknown - likely none.

Checklist

My code matches the project's code style and yarn lint:fix passes.
I've added tests for the new feature, and yarn test passes.
I have detailed the new feature in the relevant documentation.
~~I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).~~ - Does not exist
~~If this is a breaking change I've explained why.~~ - Not a breaking change

jcgsville · 2024-04-21T16:45:23Z

Thanks for taking a stab at this, @astephensen 🙂

benjie · 2024-05-14T09:21:20Z

I had to run prettier across a lot of files that I didn't touch — not sure if this was missed in a previous PR or if I didn't have it configured correctly locally, but it was causing the tests to fail.

I think you must have it misconfigured (older prettier version?) - use yarn && yarn lint:fix to fix formatting issues.

benjie

This looks really good to me. The main things I'd like to see to get this merged are:

Must refuse to run/commit/process user-authored migrations that contain --! Included or --! EndIncluded comments. Save people from copy/paste issues.
Extend the tests to cover multi-file migrations - want to ensure that the --! Included/--! EndIncluded interact fine with file splits. (Not anticipating any issues, but always good to have tests.) Both for commit and uncommit.

benjie · 2024-05-14T09:34:01Z

src/commands/uncommit.ts

@@ -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;


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;

benjie · 2024-05-14T09:41:10Z

src/commands/uncommit.ts

+  const decompiledBody = body.replace(includeRegex, (match) => {
+    return match.split("\n")[0].replace(" Included", "include");
+  });


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}`,

);

benjie · 2024-05-14T09:45:28Z

src/migration.ts

@@ -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;


Why? Suggest we revert this:

Suggested change

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

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

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.

benjie · 2024-05-14T09:46:04Z

src/migration.ts

      const sqlPath = sqlPathByRawSqlPath[rawSqlPath];
      const content = contentBySqlPath[sqlPath];
-      return content;
+      const included = match.replace(/^--![ \t]*include/, "--! Included");


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.

astephensen · 2024-05-14T22:37:25Z

Thanks for the review @benjie, I will address those comments!

Wrap included file with include comment

62a57f6

astephensen marked this pull request as draft April 16, 2024 10:10

benjie added 2 commits May 14, 2024 10:20

Merge branch 'main' into add-include-comment

297a91c

yarn lint:fix

82b2dda

Revert formatting changes

df01359

benjie requested changes May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap included files with include comment #211

Wrap included files with include comment #211

astephensen commented Apr 16, 2024 •

edited

jcgsville commented Apr 21, 2024

benjie commented May 14, 2024

benjie left a comment

benjie May 14, 2024

benjie May 14, 2024

benjie May 14, 2024

astephensen May 14, 2024

benjie May 14, 2024

astephensen commented May 14, 2024

	/^--![ \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 regex = /^--![ \t][iI]nclude[ \t]+(.\.sql)[ \t]*$/gm;
	const regex = /^--![ \t]include[ \t]+(.\.sql)[ \t]*$/gm;

Wrap included files with include comment #211

Are you sure you want to change the base?

Wrap included files with include comment #211

Conversation

astephensen commented Apr 16, 2024 • edited

Description

Pre-Commit

Committed

Uncommitted

Implementation

Notes

Performance impact

Security impact

Checklist

jcgsville commented Apr 21, 2024

benjie commented May 14, 2024

benjie left a comment

Choose a reason for hiding this comment

benjie May 14, 2024

Choose a reason for hiding this comment

benjie May 14, 2024

Choose a reason for hiding this comment

benjie May 14, 2024

Choose a reason for hiding this comment

astephensen May 14, 2024

Choose a reason for hiding this comment

benjie May 14, 2024

Choose a reason for hiding this comment

astephensen commented May 14, 2024

astephensen commented Apr 16, 2024 •

edited