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

chore: add document-id migration #19514

Merged
merged 12 commits into from
Apr 3, 2024
8 changes: 4 additions & 4 deletions examples/getstarted/config/database.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ const sqlite = {
const postgres = {
client: 'postgres',
connection: {
database: 'strapi',
user: 'strapi',
password: 'strapi',
port: 5432,
database: 'postgres',
user: 'postgres',
password: 'test',
port: 54321,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to keep the old credentials, as all databases sahre the same ones! is there any reason to change these ones?

host: 'localhost',
},
};
Expand Down
1 change: 1 addition & 0 deletions packages/core/database/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"watch": "pack-up watch"
},
"dependencies": {
"@paralleldrive/cuid2": "2.2.2",
"@strapi/utils": "4.20.0",
"date-fns": "2.30.0",
"debug": "4.3.4",
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about naming it something like 5.0.0-01-document-id.ts so that we have an easy way to see the version it was introduced in and the order in which the migrations are run? It's not (currently) enforced by the internal migrations loader because we're just using an array, but it could be added later, and in the meantime it would still be helpful to keep things organized as we add more migrations

Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
import { createId } from '@paralleldrive/cuid2';
import type { Knex } from 'knex';

import type { Migration } from '../common';
import type { Database } from '../..';
import type { Meta } from '../../metadata';

interface Params {
joinColumn: string;
inverseJoinColumn: string;
tableName: string;
joinTableName: string;
}

const QUERIES = {
async postgres(knex: Knex, params: Params) {
const res = await knex.raw(
`
SELECT :tableName:.id as id, string_agg(DISTINCT :inverseJoinColumn:::character varying, ',') as other_ids
alexandrebodin marked this conversation as resolved.
Show resolved Hide resolved
FROM :tableName:
LEFT JOIN :joinTableName: ON :tableName:.id = :joinTableName:.:joinColumn:
WHERE document_id IS NULL
GROUP BY :tableName:.id, :joinColumn:
LIMIT 1;
`,
params
);

return res.rows;
},
async mysql(knex: Knex, params: Params) {
const [res] = await knex.raw(
`
SELECT :tableName:.id as id, group_concat(DISTINCT :inverseJoinColumn:) as other_ids
FROM :tableName:
LEFT JOIN :joinTableName: ON :tableName:.id = :joinTableName:.:joinColumn:
WHERE document_id IS NULL
GROUP BY :tableName:.id, :joinColumn:
LIMIT 1;
`,
params
);

return res;
},
async sqlite(knex: Knex, params: Params) {
return knex.raw(
`
SELECT :tableName:.id as id, group_concat(DISTINCT :inverseJoinColumn:) as other_ids
FROM :tableName:
LEFT JOIN :joinTableName: ON :tableName:.id = :joinTableName:.:joinColumn:
WHERE document_id IS NULL
GROUP BY :joinColumn:
LIMIT 1;
`,
params
);
},
};

const getNextIdsToCreateDocumentId = async (
db: Database,
knex: Knex,
{
joinColumn,
inverseJoinColumn,
tableName,
joinTableName,
}: {
joinColumn: string;
inverseJoinColumn: string;
tableName: string;
joinTableName: string;
}
): Promise<number[]> => {
const res = await QUERIES[db.dialect.client as keyof typeof QUERIES](knex, {
joinColumn,
inverseJoinColumn,
tableName,
joinTableName,
});

if (res.length > 0) {
const row = res[0];
const otherIds = row.other_ids
? row.other_ids.split(',').map((v: string) => parseInt(v, 10))
: [];

return [row.id, ...otherIds];
}

return [];
};

// Migrate document ids for tables that have localizations
const migrateDocumentIdsWithLocalizations = async (db: Database, knex: Knex, meta: Meta) => {
const joinColumn = `${meta.singularName.toLowerCase()}_id`;
const inverseJoinColumn = `inv_${meta.singularName.toLowerCase()}_id`;

let ids: number[];

do {
ids = await getNextIdsToCreateDocumentId(db, knex, {
joinColumn,
inverseJoinColumn,
tableName: meta.tableName,
joinTableName: `${meta.tableName}_localizations_links`,
});

if (ids.length > 0) {
await knex(meta.tableName).update({ document_id: createId() }).whereIn('id', ids);
}
} while (ids.length > 0);
};

// Migrate document ids for tables that don't have localizations
const migrationDocumentIds = async (db: Database, knex: Knex, meta: Meta) => {
let res;
do {
res = await knex.select('id').from(meta.tableName).whereNull('document_id').limit(1).first();

if (res) {
await knex(meta.tableName).update({ document_id: createId() }).whereIn('id', res.id);
}
} while (res !== null);
alexandrebodin marked this conversation as resolved.
Show resolved Hide resolved
};

const createDocumentIdColumn = async (knex: Knex, tableName: string) => {
await knex.schema.alterTable(tableName, (table) => {
table.string('document_id');
});
};

const hasLocalizationsJoinTable = async (knex: Knex, tableName: string) => {
return knex.schema.hasTable(`${tableName}_localizations_links`);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how @innerdvations, about shortening table names, is going to affect this bit.

After the first migration to v5, I imagine the table names will have not been shortened in knex schema right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will have to update it later, but we can merge this first and I'll take it into account for my feature and fix it.

I actually suspect we will find a lot of little places throughout the codebase with hardcoded names that I'm not currently aware of and can't easily discover until I start testing the shortening and see what breaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, I wanted to take a look when reviewing your PR!


export const createdDocumentId: Migration = {
name: 'created-document-id',
async up(knex, db) {
// do sth
for (const meta of db.metadata.values()) {
const hasTable = await knex.schema.hasTable(meta.tableName);

if (!hasTable) {
continue;
}

if ('documentId' in meta.attributes) {
// add column if doesn't exist
const hasDocumentIdColumn = await knex.schema.hasColumn(meta.tableName, 'document_id');

if (hasDocumentIdColumn) {
continue;
}

await createDocumentIdColumn(knex, meta.tableName);

if (await hasLocalizationsJoinTable(knex, meta.tableName)) {
await migrateDocumentIdsWithLocalizations(db, knex, meta);
} else {
await migrationDocumentIds(db, knex, meta);
}
}
}
},
async down() {
throw new Error('not implemented');
},
};
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { Migration } from '../common';
import { createdDocumentId } from './document-id';

/**
* List of all the internal migrations. The array order will be the order in which they are executed.
Expand All @@ -9,4 +10,4 @@ import type { Migration } from '../common';
* async down(knex: Knex, db: Database) {},
* },
*/
export const internalMigrations: Migration[] = [];
export const internalMigrations: Migration[] = [createdDocumentId];
1 change: 1 addition & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -9480,6 +9480,7 @@ __metadata:
version: 0.0.0-use.local
resolution: "@strapi/database@workspace:packages/core/database"
dependencies:
"@paralleldrive/cuid2": "npm:2.2.2"
"@strapi/pack-up": "npm:4.20.0"
"@strapi/utils": "npm:4.20.0"
date-fns: "npm:2.30.0"
Expand Down