We need to be able to use parameterised insert queries to populate a table during migration

[OracPrime]

Description

It is currently quite messy to use parameterised inserts. If you naively use pgm.sql with $1 style parameters you get an error message related to regular expressions which is somewhat impenetrable if your not expecting mustache templating. If you use mustache templating, you are open to injection attacks. The only solution I can find is using pg-format for build the string (and I'm a bit worried if the result of that itself contains, for example $1.

Suggested solution

It would be nice to have access to an equivalent to pgm.sql() which doesn't do the mustaching. psm.sql_raw() for example. Either a new function, or an additional options argument on the existing one. However I prefer the separate function approach as it keeps the meaning simple and the option argument might be confused with an argument for the sql.

Alternative

pg-format is the only one I'm aware of.

Additional context

At minimum the regex error should be decorated with some explanation?

[Shinigami92]

Please note that we don't really need to be careful/sanitize parameters, because you should not use node-pg-migrate at runtime, but only for migration progresses.

That aside, what would you propose as an API?

And how does a current use-case look like right now (rough example).

[OracPrime]

The current use case if for populating a static lookup table. I'd agree in theory you're only using your own data, so you should feel slightly safe, but in an open source environment an innocuous update of a data file could become an injection attack, so I want to be paranoid.

I think my proposed api is to have a function that looks exactly like sql but which doesn't mustache.

Current use case it this (previously insert with $1... $5)

	async up(pgm) {
		pgm.createTable("uk_stations", {
			station_id: { notNull: true, primaryKey: true, type: "bigserial" },
			station_name: { notNull: true, type: "text" },
			sixteen_character_name: { notNull: true, type: "text" },
			longitude: { notNull: true, type: "float8" },
			latitude: { notNull: true, type: "float8" },
			crs_code: { notNull: true, type: "char(3)" },
		});

		const filePath = path.join(__dirname, "../ukStations.csv");
		const fileContent = fs.readFileSync(filePath, "utf8");
		const records = parse(fileContent, {
			columns: true,
			skip_empty_lines: true,
		});

		// Prepare data for bulk insertion
		const values = records.map(
			({
				station_name,
				sixteen_character_name,
				latitude,
				longitude,
				crs_code,
			}) => [
				station_name,
				sixteen_character_name,
				latitude,
				longitude,
				crs_code,
			],
		);

		const query = format(
			`INSERT INTO uk_stations (station_name, sixteen_character_name, latitude, longitude, crs_code) VALUES %L`,
			values,
		);
		pgm.sql(query);
	},

[Shinigami92]

Just me thinking out loud: Not sure if we should provide a insert (and then update and delete) statement, because these are not really anymore migrations but populations 🤔
Currently I would suggest to use node-pg-migration as a migration tool to have your DB versioned and use other tools (potentially knex) for population after the migration was executed.

However, I even know from myself in company work that there is best practice and real world scenario 🤣 ...

So maybe we could add just the absolutely simplest form of such functions that in the end will more or less just to what you already do yourself with the format function.

pgm.insert('uk_station', {
  station_name: '...',
  // data for one station...
});

pgm.insertBulk('uk_station', [
  {
    station_name: '...',
    // data for one station...
  },
  // ...
]);

I have not thought that fully out yet, but I would be okay with it if you explore an initial draft PR on your own with that info and then I can review and see where it is going.
Sounds good?

[firelizzard18]

@Shinigami92 Some kinds of data are part of the schema, or in other words some tables are pointless without data. If I have an entity such as book and another entity to classify the first such as mediaType (e.g. paper, ebook, audiobook, etc), the class entity is effectively part of the schema IMO and should be populated by the migration. pgm.insert would work for my purposes.

[Shinigami92]

@Shinigami92 Some kinds of data are part of the schema, or in other words some tables are pointless without data. If I have an entity such as book and another entity to classify the first such as mediaType (e.g. paper, ebook, audiobook, etc), the class entity is effectively part of the schema IMO and should be populated by the migration. pgm.insert would work for my purposes.

Do you know https://www.postgresql.org/docs/current/datatype-enum.html ?

[firelizzard18]

Do you know https://www.postgresql.org/docs/current/datatype-enum.html ?

I was not aware of that. If I were designing a new schema I might use that, but I've inherited an existing schema and reworking it is not an option at this time. That being said, I often like to have metadata attached to my classes/enumerations.

[Shinigami92]

As I said, I'm not against such an implementation, because I know how real-life and company work is. However, some real-talk... I'm currently in a mental health breakdown (partly because of today...).
So I reduce currently as much as possible to work for others if I do not benefit from it myself.
But what we can do here, is to work in a community and someone of the persons in this issue can start a PR (implementation, documentation and tests), and I can review and help from there. I hope that sounds fair.

[firelizzard18]

That's certainly fair, I am not entitled to your free labor. If this becomes a blocker for me I will open a PR.

[marciodsousa]

Just me thinking out loud: Not sure if we should provide a insert (and then update and delete) statement, because these are not really anymore migrations but populations 🤔
Currently I would suggest to use node-pg-migration as a migration tool to have your DB versioned and use other tools (potentially knex) for population after the migration was executed.

@Shinigami92 While I see where you're coming from, the population of lookup tables should in my opinion be seen as an integral part of the DB modeling, and therefore belong to migrations instead of, for example, seeders. Enums sadly aren't a 1-1 match use-case-wise for lookup tables.

Having the proposed programatic way of doing CRUD operations on records via node-pg-migrate natively would complete the tool further indeed, so this comment serves as a +1 on your suggestion 👍 (not exactly a blocker for me either right now as I built some in-house abstractions to achieve that meanwhile, but i don't feel those can be submitted upstream to this repo)

[Shinigami92]

@marciodsousa are you interested in proposing a draft PR?

Also as you say, it might not only be insert, but also merge, update and delete to fully cover C(R)UD.
And in worst case even select 😅

up and down are able to be used with async-await. So maybe it could be worked around more easily right now to just use the underlying pg-client directly 🤔 (not even sure right now if that is exposed 👀)

However, as far as I can remember, a migration file in it self just creates a big chunk of SQL statements that are then run in a transaction at once. So if you would e.g. do

up(pgm) {
  pgm.createTable('a')
  pgClient.insert('a', ...)
}

it wont work, as the table is not created while running the up, but only after the full up got batched in the transaction by the migration runner.

So yeah, what we would need to do is creating the SQL insert statement and then add it to the "migration.statements.push" if you know what I mean.

Feel free to explore 🫶 I'm here to review.

[firelizzard18]

However, as far as I can remember, a migration file in it self just creates a big chunk of SQL statements that are then run in a transaction at once.

I'm almost certain this is correct, as certain as I can be without going and finding the specific code. I tried the createTable then insert approach and it failed so I stepped through it in the debugger to figure out why.