Wrap included files with include comment

[astephensen]

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]

Thanks for taking a stab at this, @astephensen 🙂

[benjie]

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:

1. Must refuse to run/commit/process user-authored migrations that contain --! Included or --! EndIncluded comments. Save people from copy/paste issues.
2. 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]

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]

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]

Why? Suggest we revert this:

Suggested change

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

[astephensen]

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]

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]

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

[zarybnicky]

This is also necessary for a commit rollback - if there is a consistency error and commit fails, the ABORTED AND ROLLED BACK reverts current/ files to a form with includes already expanded, same as the standalone uncommit command does.