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

(fix) Warn for duplicated inverseBy relationships #15217

Merged
merged 11 commits into from
Jan 23, 2023
7 changes: 6 additions & 1 deletion packages/core/database/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const errors = require('./errors');

// TODO: move back into strapi
const { transformContentTypes } = require('./utils/content-types');
const { validateDatabase } = require('./validations');

class Database {
constructor(config) {
Expand Down Expand Up @@ -76,7 +77,11 @@ class Database {

// TODO: move into strapi
Database.transformContentTypes = transformContentTypes;
Database.init = async (config) => new Database(config);
Database.init = async (config) => {
const db = new Database(config);
await validateDatabase(db);
return db;
};

module.exports = {
Database,
Expand Down
5 changes: 4 additions & 1 deletion packages/core/database/lib/metadata/relations.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ const isAnyToMany = (attribute) => ['oneToMany', 'manyToMany'].includes(attribut
const isBidirectional = (attribute) => hasInversedBy(attribute) || hasMappedBy(attribute);
const isOwner = (attribute) => !isBidirectional(attribute) || hasInversedBy(attribute);
const shouldUseJoinTable = (attribute) => attribute.useJoinTable !== false;
const getJoinTableName = (tableName, attributeName) =>
_.snakeCase(`${tableName}_${attributeName}_links`);

/**
* Creates a oneToOne relation metadata
Expand Down Expand Up @@ -397,7 +399,7 @@ const createJoinTable = (metadata, { attributeName, attribute, meta }) => {
throw new Error(`Unknown target ${attribute.target}`);
}

const joinTableName = _.snakeCase(`${meta.tableName}_${attributeName}_links`);
const joinTableName = getJoinTableName(meta.tableName, attributeName);

const joinColumnName = _.snakeCase(`${meta.singularName}_id`);
let inverseJoinColumnName = _.snakeCase(`${targetMeta.singularName}_id`);
Expand Down Expand Up @@ -560,4 +562,5 @@ module.exports = {
isAnyToMany,
hasOrderColumn,
hasInverseOrderColumn,
getJoinTableName,
};
20 changes: 20 additions & 0 deletions packages/core/database/lib/validations/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict';

const { validateRelations } = require('./relations');

/**
* Validate if the database is in a valid state before starting the server.
*
* @param {*} db - Database instance
*/
async function validateDatabase(db) {
const relationErrors = await validateRelations(db);
const errorList = [...relationErrors];

if (errorList.length > 0) {
errorList.forEach((error) => strapi.log.error(error));
throw new Error('There are errors in some of your models. Please check the logs above.');
}
}

module.exports = { validateDatabase };
89 changes: 89 additions & 0 deletions packages/core/database/lib/validations/relations/bidirectional.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
'use strict';

const types = require('../../types');
const { getJoinTableName } = require('../../metadata/relations');

const getLinksWithoutMappedBy = (db) => {
const relationsToUpdate = {};

db.metadata.forEach((contentType) => {
const attributes = contentType.attributes;

// For each relation attribute, add the joinTable name to tablesToUpdate
Object.values(attributes).forEach((attribute) => {
if (!types.isRelation(attribute.type)) return;

if (attribute.inversedBy) {
const invRelation = db.metadata.get(attribute.target).attributes[attribute.inversedBy];

// Both relations use inversedBy.
if (invRelation.inversedBy) {
relationsToUpdate[attribute.joinTable.name] = {
relation: attribute,
invRelation,
};
}
}
});
});

return Object.values(relationsToUpdate);
};

const isLinkTableEmpty = async (db, linkTableName) => {
// If the table doesn't exist, it's empty
const exists = await db.getConnection().schema.hasTable(linkTableName);
if (!exists) return true;

const result = await db.getConnection().count('* as count').from(linkTableName);
return Number(result[0].count) === 0;
};

/**
* Validates bidirectional relations before starting the server.
* - If both sides use inversedBy, one of the sides must switch to mappedBy.
* When this happens, two join tables exist in the database.
* This makes sure you switch the side which does not delete any data.
*
* @param {*} db
* @return {*}
*/
const validateBidirectionalRelations = async (db) => {
const invalidLinks = getLinksWithoutMappedBy(db);
const errorList = [];

for (const { relation, invRelation } of invalidLinks) {
const contentType = db.metadata.get(invRelation.target);
const invContentType = db.metadata.get(relation.target);
Marc-Roig marked this conversation as resolved.
Show resolved Hide resolved

// Generate the join table name based on the relation target table and attribute name.
const joinTableName = getJoinTableName(contentType.tableName, invRelation.inversedBy);
const inverseJoinTableName = getJoinTableName(invContentType.tableName, relation.inversedBy);

const joinTableEmpty = await isLinkTableEmpty(db, joinTableName);
const inverseJoinTableEmpty = await isLinkTableEmpty(db, inverseJoinTableName);

if (joinTableEmpty) {
process.emitWarning(
`Error on attribute "${invRelation.inversedBy}" in model "${contentType.singularName}" (${contentType.uid}).` +
` Please modify your ${contentType.singularName} schema by renaming the key "inversedBy" to "mappedBy".` +
` Ex: { "inversedBy": "${relation.inversedBy}" } -> { "mappedBy": "${relation.inversedBy}" }`
);
} else if (inverseJoinTableEmpty) {
// Its safe to delete the inverse join table
process.emitWarning(
`Error on attribute "${relation.inversedBy}" in model "${invContentType.singularName}" (${invContentType.uid}).` +
` Please modify your ${invContentType.singularName} schema by renaming the key "inversedBy" to "mappedBy".` +
` Ex: { "inversedBy": "${invRelation.inversedBy}" } -> { "mappedBy": "${invRelation.inversedBy}" }`
);
} else {
// Both sides have data in the join table
}
}

return errorList;
};

module.exports = {
validateBidirectionalRelations,
};
14 changes: 14 additions & 0 deletions packages/core/database/lib/validations/relations/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
'use strict';

const { validateBidirectionalRelations } = require('./bidirectional');

/**
* Validates if relations data and tables are in a valid state before
* starting the server.
*/
const validateRelations = async (db) => {
const bidirectionalRelationsErrors = await validateBidirectionalRelations(db);
return [...bidirectionalRelationsErrors];
};

module.exports = { validateRelations };