allow overriding of migration templating #11
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| export type MigrationTemplater = (args: MigrationTemplateArgs) => string; | ||
| export async function defaultMigrationTemplater({ name, upQueries, downQueries }: MigrationTemplateArgs): Promise<string> { | ||
| const templateQuery = ({ query, parameters }: Query): string => | ||
| " await queryRunner.query(`" + query.replace(new RegExp("`", "g"), "\\`") + "`" + (parameters && parameters.length ? `, ${JSON.stringify(parameters)}` : "") + ");"; |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, we must escape both backticks and backslashes in the query string before injecting it into the template literal within the migration file. The best approach is to first escape all backslashes, and then escape backticks: these should be done in this order, so that we don't double-escape backslashes that precede backticks. This can be done by chaining .replace(/\\/g, '\\\\') before .replace(//g, '\'). The fix is limited to the block within the function defaultMigrationTemplater, specifically in the function expression assigned to templateQuery (line 121); no external libraries are required for this basic escaping. No changes are needed outside this function or file.
| @@ -117,8 +117,14 @@ | ||
| } | ||
| export type MigrationTemplater = (args: MigrationTemplateArgs) => string; | ||
| export async function defaultMigrationTemplater({ name, upQueries, downQueries }: MigrationTemplateArgs): Promise<string> { | ||
| const templateQuery = ({ query, parameters }: Query): string => | ||
| " await queryRunner.query(`" + query.replace(new RegExp("`", "g"), "\\`") + "`" + (parameters && parameters.length ? `, ${JSON.stringify(parameters)}` : "") + ");"; | ||
| const templateQuery = ({ query, parameters }: Query): string => | ||
| " await queryRunner.query(`" + | ||
| query | ||
| .replace(/\\/g, '\\\\') | ||
| .replace(/`/g, '\\`') + | ||
| "`" + | ||
| (parameters && parameters.length ? `, ${JSON.stringify(parameters)}` : "") + | ||
| ");"; | ||
| return `import { MigrationInterface, QueryRunner } from "typeorm"; | ||
|
|
||
| export class ${name} implements MigrationInterface { |
There was a problem hiding this comment.
I am keeping this escaping consistent with the previous code
| /** | ||
| * Formats query parameters for migration queries if parameters actually exist | ||
| */ | ||
| protected static queryParams(parameters: any[] | undefined): string { |
There was a problem hiding this comment.
one thing I discovered while doing this is that it is possible for typeorm to generate parameterized queries in migrations. I couldn't actually find any examples of these in our codebase (from looking at the typeorm code it seemed like maybe some DDL involving views might use parameters for some reason?). I opted to preserve this in my downstream change just in case there are scenarios where we need parameterization, as it wasn't too involved
jkillian
left a comment
jkillian
left a comment
There was a problem hiding this comment.
Seems reasonable to me! Curious how much this conflicts with the latest version of typeorm, is this code essentially unchanged or will is make it harder if we ever upgrade? Would also be curious if the latest version has any tools for doing this w/o forking, though I'm guessing not.
Looks like the latest MigrationGenerateCommand.ts is not much different. They've added a few more cli flags but nothing to allow overriding. I don't think it would be difficult to rebase this change on top if we are able to upgrade. |
This PR updates our typeorm fork to allow the migration templating functionality (which takes lists of "up" and "down" queries and produces the migration class definition) to be overriden. The aim of this is to implement a transactionless migration generator which automatically transforms the default typeorm-generated migrations into safe transactionless migrations following the rules in the migration runbook. I structured things this way so that logic can live in our monorepo, and our fork just exposes this seam.
While I was at it I removed a couple of CLI options that we don't use and which impact the templating (
--outputJsand--pretty- removing the latter allows us to drop a dependency as well). I also dropped a branch that handled quoting for MySQL to simplify.