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(changelog): should exclude file contents, search indexes and mg_ fields to keep storage use down #4305

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
f8eca9a
fix(catalogue): catalogue about page should show network page but nav…
mswertz Oct 1, 2024
edc77ce
remove unused page
mswertz Oct 1, 2024
ef869a4
remove unused change
mswertz Oct 1, 2024
814125d
fix(sql): changelog should exclude file contents and search indexes
mswertz Oct 2, 2024
f78a8da
fix(sql): changelog should exclude file contents and search indexes;
mswertz Oct 2, 2024
8b58e37
remove unused changes
mswertz Oct 2, 2024
b48c395
fix %_content to %_contents
mswertz Oct 2, 2024
0731f45
Merge branch 'master' of github.com:molgenis/molgenis-emx2 into fix/c…
mswertz Oct 2, 2024
6d84a7e
try to fix
mswertz Oct 2, 2024
c0e3985
simplified, fixed
mswertz Oct 2, 2024
9e6cf3e
simplified, fixed
mswertz Oct 2, 2024
ec99d2f
simplified, fixed
mswertz Oct 2, 2024
770d226
in 'old' only show changed columns
mswertz Oct 3, 2024
bbcf55e
fix this test (what is the point of this test??)
mswertz Oct 3, 2024
60106f2
Merge branch 'master' into fix/change_log_skip_file_contents
connoratrug Oct 3, 2024
2f2ad3a
Merge branch 'master' into fix/change_log_skip_file_contents
connoratrug Oct 4, 2024
22bfb72
Merge branch 'master' of github.com:molgenis/molgenis-emx2 into fix/c…
mswertz Oct 8, 2024
d5c56fe
refactor to use specific columns; requires updates on each metadata c…
mswertz Oct 8, 2024
5a7eab7
remove empty line
mswertz Oct 8, 2024
0f20a7e
Merge branch 'master' into fix/change_log_skip_file_contents
mswertz Oct 14, 2024
b444a46
merged
mswertz Oct 14, 2024
4ccd0a7
merged
mswertz Oct 14, 2024
f29481c
add migration to test
mswertz Oct 14, 2024
c7ac73d
Merge branch 'master' of github.com:molgenis/molgenis-emx2 into fix/c…
mswertz Oct 16, 2024
4cccd30
extract method to make clear that we filter the column names
mswertz Oct 16, 2024
5b4f634
rename tm to tableMetadata
mswertz Oct 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.molgenis.emx2.Column.column;
import static org.molgenis.emx2.TableMetadata.table;
import static org.molgenis.emx2.sql.Migrations.executeMigrationFile;
import static org.molgenis.emx2.sql.Migrations.migration5addMgTableclassUpdateTrigger;
import static org.molgenis.emx2.sql.Migrations.*;

import java.util.Collections;
import java.util.List;
Expand All @@ -27,7 +26,7 @@ public static void setup() {
@Test
@Tag("slow")
@Tag("windowsFail")
void testMigration2() {
void testMigrations() {
SqlDatabase database = (SqlDatabase) TestDatabaseFactory.getTestDatabase();
database.dropCreateSchema("TestMigrations");

Expand Down Expand Up @@ -103,5 +102,7 @@ void testMigration2() {
migration5addMgTableclassUpdateTrigger(database);

executeMigrationFile(database, "migration22.sql", "test migration for deletion of refback");

executeMigration24(database);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,17 +58,20 @@ static void enableChangeLog(SqlDatabase db, SchemaMetadata schema) {

// Setup trigger for each table in schema
for (TableMetadata table : schema.getTables()) {
updateChangeLogTrigger(table);
}
}

static void updateChangeLogTrigger(TableMetadata table) {
if (ChangeLogUtils.isChangeSchema(table.getSchema().getDatabase(), table.getSchemaName())) {
DSLContext jooq = ((SqlTableMetadata) table).getJooq();
// drop previous version
db.getJooq()
.execute(ChangeLogUtils.buildAuditTriggerRemove(schema.getName(), table.getTableName()));
jooq.execute(
ChangeLogUtils.buildAuditTriggerRemove(table.getSchemaName(), table.getTableName()));
// setup trigger processing function
db.getJooq()
.execute(
ChangeLogUtils.buildProcessAuditFunction(schema.getName(), table.getTableName()));

jooq.execute(ChangeLogUtils.buildProcessAuditFunction(table));
// set audit trigger, logs insert, update and delete actions on table
db.getJooq()
.execute(ChangeLogUtils.buildAuditTrigger(schema.getName(), table.getTableName()));
jooq.execute(ChangeLogUtils.buildAuditTrigger(table.getSchemaName(), table.getTableName()));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,13 @@

import static java.lang.Boolean.TRUE;

import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;
import org.molgenis.emx2.ColumnType;
import org.molgenis.emx2.Constants;
import org.molgenis.emx2.Database;
import org.molgenis.emx2.TableMetadata;
import org.molgenis.emx2.utils.TypeUtils;

public class ChangeLogUtils {
Expand All @@ -12,33 +17,96 @@ private ChangeLogUtils() {
throw new IllegalStateException("Utility class");
}

public static String buildProcessAuditFunction(String schemaName, String tableName) {
public static String buildProcessAuditFunction(TableMetadata tableMetadata) {
List<String> columnNames = new ArrayList<>();
tableMetadata
.getColumns()
.forEach(
column -> {
ColumnType type = column.getColumnType();
if (type.isRef() || type.isRefArray()) {
column
.getReferences()
.forEach(
ref -> {
columnNames.add(ref.getName());
});
} else if (type.isFile()) {
columnNames.add(column.getName() + "_filename");
columnNames.add(column.getName() + "_size");
} else if (!type.isHeading()) {
columnNames.add(column.getName());
}
});

String columnNameArray =
columnNames.stream()
mswertz marked this conversation as resolved.
Show resolved Hide resolved
.filter(name -> !name.startsWith("mg_"))
.map(name -> "'" + name + "'")
.collect(Collectors.joining(","));

return """
CREATE OR REPLACE FUNCTION "%1$s"."process_%3$s_audit"() RETURNS TRIGGER AS $%3$s_audit$
BEGIN
IF (TG_OP = 'DELETE') THEN
INSERT INTO "%1$s".mg_changelog
SELECT 'D', now(), user, TG_TABLE_NAME, row_to_json(OLD.*), row_to_json(NEW.*);
ELSIF (TG_OP = 'UPDATE') THEN
INSERT INTO "%1$s".mg_changelog
SELECT 'U', now(), user, TG_TABLE_NAME, row_to_json(OLD.*), row_to_json(NEW.*);
ELSIF (TG_OP = 'INSERT') THEN
INSERT INTO "%1$s".mg_changelog
SELECT 'I', now(), user, TG_TABLE_NAME, row_to_json(OLD.*), row_to_json(NEW.*);
END IF;
RETURN NULL; -- result is ignored since this is an AFTER trigger
END;
$%3$s_audit$ LANGUAGE plpgsql;
"""
CREATE OR REPLACE FUNCTION "%1$s"."process_%3$s_audit"()
RETURNS TRIGGER AS $%3$s_audit$
DECLARE
column_names varchar[] := ARRAY[%4$s];
old_row JSONB;
new_row JSONB;
col_name TEXT;
old_value TEXT;
new_value TEXT;
BEGIN
-- Initialize empty JSONB objects
old_row := '{}'::JSONB;
new_row := '{}'::JSONB;

-- Loop through each column in the OLD record
FOREACH col_name IN ARRAY column_names
LOOP
-- Skip columns that end with '_contents' or '_TEXT_SEARCH_COLUMN'
mswertz marked this conversation as resolved.
Show resolved Hide resolved
IF col_name LIKE '%%_contents' OR col_name LIKE '%%_TEXT_SEARCH_COLUMN' OR col_name LIKE 'mg_%%' THEN
CONTINUE;
END IF;
IF TG_OP != 'INSERT' THEN
EXECUTE 'SELECT ($1).' || quote_ident(col_name) INTO old_value USING OLD;
IF old_value IS NOT NULL THEN
old_row := jsonb_set(old_row, ARRAY[col_name], to_jsonb(old_value::TEXT)::JSONB);
END IF;
END IF;
IF TG_OP != 'DELETE' THEN
EXECUTE 'SELECT ($1).' || quote_ident(col_name) INTO new_value USING NEW;
IF new_value IS NOT NULL THEN
new_row := jsonb_set(new_row, ARRAY[col_name], to_jsonb(new_value::TEXT)::JSONB);
END IF;
END IF;
END LOOP;

-- Log the change based on the operation
IF TG_OP = 'DELETE' THEN
INSERT INTO "%1$s".mg_changelog
SELECT 'D', now(), user, TG_TABLE_NAME, old_row, new_row;
ELSIF TG_OP = 'UPDATE' THEN
INSERT INTO "%1$s".mg_changelog
SELECT 'U', now(), user, TG_TABLE_NAME, old_row, new_row;
ELSIF TG_OP = 'INSERT' THEN
INSERT INTO "%1$s".mg_changelog
SELECT 'I', now(), user, TG_TABLE_NAME, old_row, new_row;
END IF;

RETURN NULL; -- result is ignored since this is an AFTER trigger
END;
$%3$s_audit$ LANGUAGE plpgsql;
"""
.formatted(
schemaName,
ChangeLogUtils.buildFunctionName(schemaName),
ChangeLogUtils.buildFunctionName(tableName));
tableMetadata.getSchemaName(),
ChangeLogUtils.buildFunctionName(tableMetadata.getSchemaName()),
ChangeLogUtils.buildFunctionName(tableMetadata.getTableName()),
columnNameArray);
}

public static String buildAuditTrigger(String schemaName, String tableName) {
return """
CREATE TRIGGER %3$s_audit
CREATE OR REPLACE TRIGGER %3$s_audit
Copy link
Contributor

Choose a reason for hiding this comment

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

these should not be a trigger when the trigger is add , should fail fast

Copy link
Member Author

Choose a reason for hiding this comment

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

???

Copy link
Contributor

Choose a reason for hiding this comment

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

the code assumes there is no trigger with this name , if there is a trigger with this name, something has gone (very) wrong and we should know , not suppress the error

AFTER INSERT OR UPDATE OR DELETE ON "%1$s"."%2$s"
FOR EACH ROW EXECUTE FUNCTION "%1$s"."process_%3$s_audit"();
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.jooq.impl.DSL.*;
import static org.jooq.impl.SQLDataType.VARCHAR;
import static org.molgenis.emx2.Constants.MG_TABLECLASS;
import static org.molgenis.emx2.sql.ChangeLogExecutor.updateChangeLogTrigger;
import static org.molgenis.emx2.sql.MetadataUtils.*;
import static org.molgenis.emx2.sql.SqlDatabase.TEN_SECONDS;
import static org.molgenis.emx2.sql.SqlTableMetadataExecutor.MG_TABLECLASS_UPDATE;
Expand All @@ -22,7 +23,7 @@

public class Migrations {
// version the current software needs to work
private static final int SOFTWARE_DATABASE_VERSION = 23;
private static final int SOFTWARE_DATABASE_VERSION = 24;
public static final int THREE_MINUTES = 180;
private static Logger logger = LoggerFactory.getLogger(Migrations.class);

Expand Down Expand Up @@ -152,11 +153,26 @@ public static synchronized void initOrMigrate(SqlDatabase db) {
executeMigrationFile(tdb, "migration23.sql", "add enable state to user metadata");
}

if (version < 24) {
executeMigration24(tdb);
}

// if success, update version to SOFTWARE_DATABASE_VERSION
updateDatabaseVersion((SqlDatabase) tdb, SOFTWARE_DATABASE_VERSION);
});
}

static void executeMigration24(Database tdb) {
for (String schemaName : tdb.getSchemaNames()) {
Schema schema = tdb.getSchema(schemaName);
for (TableMetadata tm : schema.getMetadata().getTables()) {
updateChangeLogTrigger(tm);
}
}
logger.debug(
"executed migration24: changelog triggers should skip system columns and file contents column");
}

private static void executeMigration7(SqlDatabase tdb, String message) {
DSLContext jooq = tdb.getJooq();
// rename table
Expand Down Expand Up @@ -220,7 +236,7 @@ static void executeMigrationFile(Database db, String sqlFile, String message) {
jooq.settings().setQueryTimeout(THREE_MINUTES);
String sql = new String(Migrations.class.getResourceAsStream(sqlFile).readAllBytes());
jooq.execute(sql);
logger.debug(message + "(file = " + sqlFile);
logger.info(message + "(file = " + sqlFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

info ?

Copy link
Member Author

Choose a reason for hiding this comment

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

currently we cannot see that migrations are executed. I think this is important enough to be 'info'

} catch (IOException e) {
throw new MolgenisException("Execute migration failed", e);
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import static org.molgenis.emx2.Constants.MG_EDIT_ROLE;
import static org.molgenis.emx2.Constants.MG_TABLECLASS;
import static org.molgenis.emx2.Privileges.EDITOR;
import static org.molgenis.emx2.sql.ChangeLogExecutor.updateChangeLogTrigger;
import static org.molgenis.emx2.sql.MetadataUtils.deleteColumn;
import static org.molgenis.emx2.sql.MetadataUtils.saveColumnMetadata;
import static org.molgenis.emx2.sql.SqlColumnExecutor.*;
Expand Down Expand Up @@ -81,6 +82,7 @@ private static SqlTableMetadata addTransaction(
log(tm, start, "added column '" + newColumn.getName() + "' to table " + tm.getTableName());
}
}
updateChangeLogTrigger(tm);
mswertz marked this conversation as resolved.
Show resolved Hide resolved
return tm;
}

Expand Down Expand Up @@ -133,7 +135,7 @@ private static SqlTableMetadata alterNameTransaction(
for (Column column : tm.getStoredColumns()) {
SqlColumnExecutor.executeCreateRefConstraints(tm.getJooq(), column);
}

updateChangeLogTrigger(tm);
return tm;
}

Expand Down Expand Up @@ -269,6 +271,7 @@ private static SqlTableMetadata alterColumnTransaction(
if (!oldColumn.getName().equals(newColumn.getName())) deleteColumn(tm.getJooq(), oldColumn);
saveColumnMetadata(tm.getJooq(), newColumn);

updateChangeLogTrigger(tm);
return tm;
}

Expand Down Expand Up @@ -312,6 +315,7 @@ private static SqlTableMetadata dropColumnTransaction(
DSLContext jooq = ((SqlDatabase) db).getJooq();
SqlColumnExecutor.executeRemoveColumn(jooq, tm.getColumn(columnName));
tm.columns.remove(columnName);
updateChangeLogTrigger(tm);
return tm;
}

Expand Down Expand Up @@ -387,6 +391,7 @@ private static SqlTableMetadata setInheritTransaction(
TableMetadata om = db.getSchema(inheritSchema).getTable(inheritedName).getMetadata();
executeSetInherit(jooq, tm, om);
tm.inheritName = inheritedName;
updateChangeLogTrigger(tm);
MetadataUtils.saveTableMetadata(jooq, tm);
return tm;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import static org.molgenis.emx2.ColumnType.*;
import static org.molgenis.emx2.Constants.*;
import static org.molgenis.emx2.sql.ChangeLogExecutor.disableChangeLog;
import static org.molgenis.emx2.sql.ChangeLogExecutor.updateChangeLogTrigger;
import static org.molgenis.emx2.sql.MetadataUtils.saveColumnMetadata;
import static org.molgenis.emx2.sql.SqlColumnExecutor.*;
import static org.molgenis.emx2.utils.ColumnSort.sortColumnsByDependency;
Expand Down Expand Up @@ -102,14 +103,7 @@ static void executeCreateTable(DSLContext jooq, SqlTableMetadata table) {
executeAddMetaColumns(table);
}

if (ChangeLogUtils.isChangeSchema(table.getSchema().getDatabase(), table.getSchemaName())) {
// setup trigger processing function
jooq.execute(
ChangeLogUtils.buildProcessAuditFunction(table.getSchemaName(), table.getTableName()));

// set audit trigger, logs insert, update and delete actions on table
jooq.execute(ChangeLogUtils.buildAuditTrigger(table.getSchemaName(), table.getTableName()));
}
updateChangeLogTrigger(table);
}

static void executeAlterName(DSLContext jooq, TableMetadata table, String newName) {
Expand Down
Loading