issue-163: added ability to include external files in current.sql.

[jnbarlow]

Description

fixes #163
This PR adds the ability to include external files into current.sql, allowing for better
integration with source control. Functions, computed columns, etc can live in separate
diffable files for pull requests, but still take advantage of all the goodness of graphile-migrate
and the current.sql.

Performance impact

If someone decides to include a ton of files in a current.sql, the I/O to read these in will make
things not super pleasant.

Security impact

unknown... shouldn't be any more risky than using current.sql

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).
• If this is a breaking change I've explained why.

[jnbarlow]

@benjie Greetings! If I understood the flow of how sql gets compiled, I think I shimmed my new compile step in so that it should work for everything. Let me know if I missed something :)

[benjie]

Amazing! I’ve not reviewed the code yet, but we need it to integrate with watch mode: https://github.com/graphile/migrate/blob/main/src/commands/watch.ts#L217

easiest would be to require that these files all live within one folder (migrations/includes or migrations/library or whatever) and watch that folder, however another option is to dynamically add files to chokidar when they are imported and remove them when they are no longer so. Chokidar has APIs for this.

[jnbarlow]

@benjie So you're thinking of having a base folder that all the includes live (with Chokidar watching it) so that any time one of them changes it reprocesses the current.sql?

Is the path something we should make configurable with a default or hard code it as part of the framework? What about placeholders? Right now the includes are brought in after the placeholder replacement. We could swap that, but I'm not sure of the use cases would warrant that. To me, it feels like the includes probably shouldn't have placeholders, but you've got more experience with how people use them.

[benjie]

Hardcoded is fine to start with; if someone really wants it configurable they can send a PR. Still not sure of the name; perhaps migrations/lib…

Yes, I’m thinking any change to that folder (or descendents) triggers current to run again; we already checksum the result so if it wasn’t relevant we’ll skip it.

It’s very common for roles to be placeholders, particularly on shared hosting (where you grant to a different role on dev versus staging versus production), and in fact that’s what we do in starter via :VISITOR or :VISITOR_ROLE or whatever (I forget the specifics). So it’s essential that the included files are also processed via the placeholder system. I think there’s a handy method you can call on the strings to apply the placeholders, you don’t have to concatenate first if you don’t want to.

[benjie]

It is essential that these substitutions are made before the SQL is written to migrations/committed/, so putting the code in run is incorrect - instead it should go in readCurrentMigration:

migrate/src/current.ts

Line 98 in 757194c

export async function readCurrentMigration(

Importantly, each file that is read should be subjected to these transforms, even the included files themselves. (Be sure to use a stack to prevent infinite loops.)

For security, please ensure that the file to be imported is actually within the migrations/fixtures/ folder, e.g. by using fs.realpath (on both the resolved path and the fixtures folder) or similar.

Really excited to add this feature finally; it's been on my TODO list for a very long time!

[jnbarlow]

@benjie Ahh ok, thanks for pointing me to where this should go. I was trying to figure out the flow of things and shim it into a spot where it would always be called. Run seemed to be the logical spot that called the placeholder replacement to do that. Let me work on the updates then

[benjie]

Yeah calling it in readCurrentMigration means you don't even have to think about placeholders because they're handled for you later. (Placeholders can change at runtime, that's deliberate.)

[jnbarlow]

@benjie alright, I think I've address everything and pushed.

• hardened the compileInclude function to work if the fixtures directory doesn't exist
• moved the compileInclude call to readCurrentMigration
• added fsrealpath support
• fixed all relevant tests after including this step
• updated the watch to include the fixtures directory

Give this a look whenever you get a chance. The only thing I've not done yet is actually run the code (been relying on tests to check everything). I'm not super sure how to go about compiling it down to something I can execute.

[benjie]

Awesome; will take a look when I'm back from the winter break.

I'm not super sure how to go about compiling it down to something I can execute.

You should be able to run yarn prepack which will build everything. Then you can run yarn link to create a global version of it, and in a clean folder somewhere (or in your project that uses migrate) you can do yarn link graphile-worker to install the version. I'm not 100% sure this will work, it sometimes gets confused due to modules coming from multiple places, but migrate is simple enough that I think it will.

Alternatively you can run yarn pack which will create a tgz containing the module, and then you can install that tgz into a project somewhere via yarn add file:/path/to/local/tarball.tgz or equivalent in whatever package manager you use.

[jnbarlow]

@benjie another round pushed up

• updated watch to proper **/* to get everything in the fixtures directory
• had to add regex escaping to imported files... files with $ was breaking in the replacement
  • Can you think of any other SQL allowed characters that would need to be escaped?
• added tests

I was able to get it to run locally too, but ran into a problem where yarn add kept importing old versions. I manually extracted the built file and it worked.

[benjie]

Thanks so much for taking this on, sorry it's taken me a while to get around to, January has been hectic!

I'm going to take on getting this over the finish line, thanks again!

[benjie]

@jnbarlow I think this is good to merge now; would you care to give it a test and make sure it still works well (in particular, ensure that watch mode functions correctly)?

[jnbarlow]

@benjie Sure!. I'll give that a test tonight and let you know

[jnbarlow]

@benjie everything works :) ... Ignore my last message if you saw it, I was doing something boneheaded when I tested it :)