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

inline SQL scripts for bundlers #345

Merged
merged 6 commits into from
Oct 3, 2023
Merged

Conversation

timelf123
Copy link
Contributor

@timelf123 timelf123 commented Sep 20, 2023

Description

Removing fs dependency and inlining SQL files into a TS file at build time before tsc is run allows this to be run in serverless environments. For example, this nextjs config in vercel worked:

  webpack: (config) => {
    config.module.rules.push(
      {
        test: [
          path.resolve("node_modules/pg/lib/native"),
          path.resolve("node_modules/import-fresh"),
          path.resolve("node_modules/graphile-worker/dist/getTasks"),
        ],
        loader: "null-loader",
      },
// unsure if this is needed after the inlining changes
      {
        test: path.resolve("node_modules/graphile-worker/dist/migrate"),
        loader: "@vercel/webpack-asset-relocator-loader",
        options: {
          outputAssetBase: "graphile-worker",
        },
      }
    );

    // Return the modified config object
    return config;
  },

Fixes #143

Performance impact

Front loaded at build time

Security impact

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. - current e2e tests work
  • 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.

build script compiles sql files into strings
@timelf123 timelf123 marked this pull request as draft September 20, 2023 21:30
@timelf123 timelf123 marked this pull request as ready for review September 25, 2023 19:21
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Excellent start; here's some suggested changes 🙌

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
scripts/buildSqlModule.js Outdated Show resolved Hide resolved
scripts/buildSqlModule.js Outdated Show resolved Hide resolved
Comment on lines 7 to 11
const sqlModule = sqlFiles.reduce((acc, file) => {
const sql = fs.readFileSync(path.join(sqlDir, file), "utf8");
const varName = "sql_" + file.replace(".sql", "").replace(/-/g, "_");
return acc + `export const ${varName} = \`${sql}\`;\n`;
}, "");
Copy link
Member

@benjie benjie Sep 27, 2023

Choose a reason for hiding this comment

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

The escaping of the SQL wasn't safe in general, let's protect against future migrations. If you can think of any UTF-8 encoded string sql for which sql !== eval(escape(sql)) please let me know.

Suggested change
const sqlModule = sqlFiles.reduce((acc, file) => {
const sql = fs.readFileSync(path.join(sqlDir, file), "utf8");
const varName = "sql_" + file.replace(".sql", "").replace(/-/g, "_");
return acc + `export const ${varName} = \`${sql}\`;\n`;
}, "");
/**
* Turns `str` into a String.raw expression, safely escaping backticks and
* placeholder strings.
*/
function escape(str) {
return (
"String.raw`" +
str
.replace(/`/g, '` + "`" + String.raw`')
.replace(/\$\{/g, "$$` + String.raw`{") +
"`"
);
}
const sqlModule =
"/* This file is auto-generated by `npm run build:sql`; DO NOT EDIT */\n" +
"// prettier-ignore\n" +
"const migrations = {\n" +
sqlFiles
.map((file) => {
const sql = fs.readFileSync(path.join(sqlDir, file), "utf8");
return ` ${JSON.stringify(file)}: ${escape(sql)},`;
})
.join("\n") + "\n};\n";

Copy link
Contributor Author

@timelf123 timelf123 Sep 27, 2023

Choose a reason for hiding this comment

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

Unless I am misunderstanding, I believe this may break out, but isn't an SQL escape. I can do a bit of fuzzing
}; console.log('Hacked!'); //

Copy link
Member

Choose a reason for hiding this comment

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

Nice try, but that looks safe enough to me:

$ node
Welcome to Node.js v18.17.1.
Type ".help" for more information.
> function escape(str) {
...   return (
...     "String.raw`" +
...     str
...       .replace(/`/g, '` + "`" + String.raw`')
...       .replace(/\$\{/g, "$$` + String.raw`{") +
...     "`"
...   );
... }
undefined
> sql = "}; console.log('Hacked!'); //"
"}; console.log('Hacked!'); //"
> sql === eval(escape(sql))
true
> sql = "`}; console.log('Hacked!'); //`"
"`}; console.log('Hacked!'); //`"
> sql === eval(escape(sql))
true

scripts/buildSqlModule.js Outdated Show resolved Hide resolved
src/migrate.ts Outdated Show resolved Hide resolved
src/migrate.ts Outdated Show resolved Hide resolved
src/migrate.ts Outdated Show resolved Hide resolved
timelf123 and others added 2 commits September 27, 2023 15:27
Co-authored-by: Benjie <benjie@jemjie.com>
Co-authored-by: Benjie <benjie@jemjie.com>
@benjie
Copy link
Member

benjie commented Sep 28, 2023

When you get a chance, please regenerate the generated file (and remove the old version). Cheers!

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

This looks almost perfect. One last tiny change and regenerate and then we're good I think!

scripts/buildSqlModule.js Outdated Show resolved Hide resolved
Co-authored-by: Benjie <benjie@jemjie.com>
@timelf123
Copy link
Contributor Author

This looks almost perfect. One last tiny change and regenerate and then we're good I think!

Thanks, changed!

@benjie
Copy link
Member

benjie commented Sep 29, 2023

Please regenerate the generated file 🙏

@timelf123 timelf123 requested a review from benjie October 1, 2023 15:02
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@benjie benjie merged commit 53e0739 into graphile:main Oct 3, 2023
0 of 12 checks passed
@benjie
Copy link
Member

benjie commented Oct 3, 2023

Released as graphile-worker@0.15.0 🙌

@timelf123
Copy link
Contributor Author

@benjie thank you for the PRR and quick release/tag!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve webpack-ability of worker
2 participants