feat(history): migrate C7 audit log history to C8#851
Conversation
|
@copilot, the audit log transformer is missing. |
Added AuditLogTransformer in commit 45a590a. The transformer implements EntityInterceptor and handles conversion from UserOperationLogEntry to AuditLogDbModel, mapping all operation details, timestamps, user information, and entity references. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
yanavasileva
force-pushed
the
copilot/migrate-audit-log-entities
branch
3 times, most recently
from
166fd46 to
9fce750
Compare
yanavasileva
force-pushed
the
copilot/migrate-audit-log-entities
branch
3 times, most recently
from
30cd043 to
26e9e89
Compare
|
Commit for |
There was a problem hiding this comment.
Pull request overview
This PR implements migration of Camunda 7 audit log history (UserOperationLogEntry) to Camunda 8 (AuditLogDbModel), enabling preservation of a complete audit trail during migration. The implementation adds a new entity type to the migration framework, introduces database schema changes to support composite string keys, and includes comprehensive test coverage for various audit log scenarios.
Changes:
- Added
HISTORY_AUDIT_LOGentity type with migration support viaAuditLogMigratorandAuditLogTransformer - Modified
C8_KEYcolumn fromBIGINTtoVARCHAR(255)to support composite keys for audit logs (format: "4095-123456") - Integrated audit log migration as the last step in
HistoryMigratorto ensure all referenced entities exist before migration - Added comprehensive test coverage for user task operations, process instance operations, admin operations, and decision operations
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
db.0.3.0.xml |
Liquibase changelog to modify C8_KEY column from BIGINT to VARCHAR(255) |
IdKey.xml |
Updated MyBatis mapper for VARCHAR C8_KEY column |
IdKeyDbModel.java |
Changed C8_KEY property type from Long to String |
IdKeyMapper.java |
Added HISTORY_AUDIT_LOG enum type |
AuditLogMigrator.java |
Core migrator class for audit log entities with dependency validation |
AuditLogTransformer.java |
Entity transformer converting C7 UserOperationLogEntry to C8 AuditLogDbModel |
HistoryMigrator.java |
Integrated audit log migration as final step |
BaseMigrator.java |
Added String c8Key overload for markMigrated, moved helper methods from ProcessInstanceMigrator |
DbClient.java |
Added String overloads for insert/update methods to support both Long and String keys |
C7Client.java |
Added methods to fetch user operation log entries with pagination |
C8Client.java |
Added insertAuditLog and searchUserTasks methods |
HistoryAuditLogTest.java |
Comprehensive tests for process instance and decision-related audit logs |
HistoryAuditLogUserTaskTest.java |
Tests for user task operation audit logs (assign, claim, complete, etc.) |
HistoryAuditLogAdminTest.java |
Tests for administrative operation audit logs (user, group, tenant, authorization) |
HistoryMigrationAbstractTest.java |
Added helper methods to search audit logs in tests |
MigratorAutoConfiguration.java |
Registered AuditLogMigrator bean |
InterceptorConfiguration.java |
Registered AuditLogTransformer bean |
| Various log constants | Added logging messages for audit log migration |
| Test fixes | Updated tests for type compatibility with String C8_KEY |
| @@ -184,7 +184,7 @@ protected static void insertTestData(IdKeyDbModel idKeyDbModel) { | |||
| if (idKeyDbModel.getC8Key() == null) { | |||
| stmt.setNull(2, java.sql.Types.BIGINT); | |||
There was a problem hiding this comment.
The code sets null using java.sql.Types.BIGINT, but the C8_KEY column has been changed to VARCHAR in the database schema. For consistency with the schema change, this should be updated to java.sql.Types.VARCHAR. While setting NULL with the wrong type often works in practice, it's better to use the correct type for clarity and potential database compatibility.
| stmt.setNull(2, java.sql.Types.BIGINT); | |
| stmt.setNull(2, java.sql.Types.VARCHAR); |
...main/java/io/camunda/migration/data/impl/interceptor/history/entity/AuditLogTransformer.java
Outdated
Show resolved
Hide resolved
...n-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryMigrationAbstractTest.java
Outdated
Show resolved
Hide resolved
| builder.userTaskKey(taskKey) | ||
| .elementInstanceKey(userTaskDbModel.elementInstanceKey()); |
There was a problem hiding this comment.
Potential null pointer exception: If searchUserTasksByKey returns null (when the user task is not found), the code on line 178 will attempt to call elementInstanceKey() on a null userTaskDbModel. This should be checked before accessing the elementInstanceKey() method.
| builder.userTaskKey(taskKey) | |
| .elementInstanceKey(userTaskDbModel.elementInstanceKey()); | |
| builder.userTaskKey(taskKey); | |
| if (userTaskDbModel != null && userTaskDbModel.elementInstanceKey() != null) { | |
| builder.elementInstanceKey(userTaskDbModel.elementInstanceKey()); | |
| } |
| assertThat(logs).hasSize(1); // result is 0 since task is not linked to a process instance and can't be migrated | ||
| assertAuditLogProperties(logs, AuditLogEntity.AuditLogOperationType.DELETE); |
There was a problem hiding this comment.
The comment on line 259 states "result is 0 since task is not linked to a process instance and can't be migrated", but the assertion on line 259 expects hasSize(1). This is inconsistent - either the comment should be updated to reflect that the test expects 1 result, or the assertion should be changed to hasSize(0) to match the comment. Since the test is disabled, this needs to be resolved before enabling it.
| assertThat(logs).hasSize(1); // result is 0 since task is not linked to a process instance and can't be migrated | |
| assertAuditLogProperties(logs, AuditLogEntity.AuditLogOperationType.DELETE); | |
| assertThat(logs).hasSize(0); // result is 0 since task is not linked to a process instance and can't be migrated |
| .processInstanceId(processInstance.getId()) | ||
| .operationType(UserOperationLogEntry.OPERATION_TYPE_CLAIM) | ||
| .count(); | ||
| assertThat(auditLogCount).isEqualTo(1); // there are two UPDATE logs: for each property |
There was a problem hiding this comment.
The comment on line 195 states "there are two UPDATE logs: for each property", but the assertion checks for exactly 1 audit log entry. This comment appears to be incorrect or misleading - it should either be removed or corrected to match the actual behavior being tested (which expects 1 log entry).
| assertThat(auditLogCount).isEqualTo(1); // there are two UPDATE logs: for each property | |
| assertThat(auditLogCount).isEqualTo(1); // expect exactly one CLAIM operation log entry |
| case UserOperationLogEntry.OPERATION_TYPE_RESOLVE -> { | ||
| if (EntityTypes.PROCESS_INSTANCE.equals(userOperationLog.getEntityType())) { | ||
| builder.entityType(AuditLogEntity.AuditLogEntityType.INCIDENT); | ||
| yield AuditLogEntity.AuditLogOperationType.RESOLVE; | ||
| } else { | ||
| yield AuditLogEntity.AuditLogOperationType.UPDATE; | ||
| } | ||
| } | ||
|
|
||
| default -> | ||
| throw new EntityInterceptorException(HistoryMigratorLogs.UNSUPPORTED_AUDIT_LOG_OPERATION_TYPE + operationType); | ||
| }; | ||
| } | ||
|
|
||
| protected void updateEntityTypesThatDontMatchBetweenC7andC8(UserOperationLogEntry userOperationLog, AuditLogDbModel.Builder builder) { | ||
| if (UserOperationLogEntry.OPERATION_TYPE_RESOLVE.equals(userOperationLog.getOperationType()) | ||
| && EntityTypes.PROCESS_INSTANCE.equals(userOperationLog.getEntityType())) { | ||
| builder.entityType(AuditLogEntity.AuditLogEntityType.INCIDENT); | ||
| } else if (UserOperationLogEntry.OPERATION_TYPE_SET_VARIABLE.equals(userOperationLog.getOperationType()) | ||
| || UserOperationLogEntry.OPERATION_TYPE_SET_VARIABLES.equals(userOperationLog.getOperationType())) { | ||
| builder.entityType(AuditLogEntity.AuditLogEntityType.VARIABLE); | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplication: The entityType for RESOLVE operations on process instances is being set in two places - within the convertOperationType method (line 163) and again in updateEntityTypesThatDontMatchBetweenC7andC8 (lines 176-178). This is redundant and could lead to confusion. Consider consolidating this logic into one location, preferably in the updateEntityTypesThatDontMatchBetweenC7andC8 method since that's its explicit purpose.
| assertThat(migratedInstance.getC7Id()).isEqualTo(processInstanceId); | ||
| assertThat(migratedInstance.getCreateTime()).isNotNull().isBeforeOrEqualTo(beforeMigration); // Should be before we started the test | ||
| assertThat(migratedInstance.getC8Key()).isNotNull().isPositive(); | ||
| assertThat(Long.valueOf(migratedInstance.getC8Key())).isNotNull().isPositive(); |
There was a problem hiding this comment.
Potential uncaught 'java.lang.NumberFormatException'.
| assertThat(Long.valueOf(migratedInstance.getC8Key())).isNotNull().isPositive(); | |
| assertThat(migratedInstance.getC8Key()).isNotNull().matches("^[1-9][0-9]*$"); |
HeleneW-dot
left a comment
HeleneW-dot
left a comment
There was a problem hiding this comment.
🚀
I mainly have questions for my understanding, and I have not yet tested this locally due to the current issue with the snapshot dependency. Will comment once I was able to test locally.
Other than questions/nitpicks I have one comment in AuditLogTransformer where I am unsure the correct operation type is being set. Other than that just minor typos and one helper thats using a hardcoded value that needs adjusted.
| UserOperationLogQueryImpl query = (UserOperationLogQueryImpl) historyService.createUserOperationLogQuery() | ||
| .orderByTimestamp() | ||
| .asc(); |
There was a problem hiding this comment.
With the other entities we're also ordering by ID in case of simultaneous timestamps, is this possible here?
| } | ||
|
|
||
| /** | ||
| * Inserts a new process instance record into the mapping table. |
There was a problem hiding this comment.
| * Inserts a new process instance record into the mapping table. | |
| * Inserts a new record of the given type into the mapping table. |
or just record
| builder.processInstanceKey(processInstance.processInstanceKey()); | ||
| if (c7RootProcessInstanceId != null && isMigrated(c7RootProcessInstanceId, HISTORY_PROCESS_INSTANCE)) { | ||
| ProcessInstanceEntity rootProcessInstance = findProcessInstanceByC7Id(c7RootProcessInstanceId); | ||
| if (rootProcessInstance != null && rootProcessInstance.processInstanceKey() != null) { |
There was a problem hiding this comment.
For my understanding, under which circumstances would the process instance key of the root process instance be null?
| HISTORY_USER_TASK); | ||
| UserTaskDbModel userTaskDbModel = searchUserTasksByKey(taskKey); | ||
| builder.userTaskKey(taskKey) | ||
| .elementInstanceKey(userTaskDbModel.elementInstanceKey()); |
There was a problem hiding this comment.
Is the element instance key only relevant for userTasks or could we also set this to the flownode key for other operations?
There was a problem hiding this comment.
The key is set for variable operations too but we don't store the information in C7. I will document the limitation
| protected UserTaskDbModel searchUserTasksByKey(Long taskKey) { | ||
| return c8Client.searchUserTasks(UserTaskDbQuery.of(b -> b.filter(f -> f.userTaskKeys(taskKey)))) | ||
| .stream() | ||
| .findFirst() | ||
| .orElse(null); | ||
| } |
There was a problem hiding this comment.
Nit
could consider moving this to C8 client (there is a similar method in there for process instances)
...-migrator/core/src/main/java/io/camunda/migration/data/impl/logging/HistoryMigratorLogs.java
Outdated
Show resolved
Hide resolved
...ion-tests/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryAuditLogTest.java
Show resolved
Hide resolved
...ion-tests/src/test/java/io/camunda/migration/data/qa/history/entity/HistoryAuditLogTest.java
Show resolved
Hide resolved
...n-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryMigrationAbstractTest.java
Outdated
Show resolved
Hide resolved
...n-tests/src/test/java/io/camunda/migration/data/qa/history/HistoryMigrationAbstractTest.java
Outdated
Show resolved
Hide resolved
|
WIP I am half way throu the review hints |
yanavasileva
force-pushed
the
copilot/migrate-audit-log-entities
branch
from
6d0c119 to
2317a89
Compare
|
Implemented review hints in separate commit. I had to add entityKey impl for the entities that we can retrieve info from |
HeleneW-dot
left a comment
HeleneW-dot
left a comment
There was a problem hiding this comment.
Lgtm! Thank you for implementing review hints. I also smoke tested locally and works fine for me.
I rebased locally to do the smoke test, do you want me to push that or are you already on that?
|
I will rebase in a minute |
yanavasileva
force-pushed
the
copilot/migrate-audit-log-entities
branch
from
2317a89 to
bb16e40
Compare
yanavasileva
force-pushed
the
copilot/migrate-audit-log-entities
branch
from
bb16e40 to
d0acfa6
Compare
Description
This PR implements the migration path for Camunda 7 audit log entities (
UserOperationLogEntry) to Camunda 8, enabling a complete audit trail to be preserved during migration.Key Components
HISTORY_AUDIT_LOGtoIdKeyMapper.TYPEenum for migration trackingfetchAndHandleUserOperationLogEntries()and single entry retrieval viagetUserOperationLogEntry()insertAuditLog()method usingAuditLogDbModelandAuditLogMapperAuditLogMigratorextendingBaseMigratorwith support for both MIGRATE and RETRY_SKIPPED modesAuditLogTransformerthat converts C7UserOperationLogEntryto C8AuditLogDbModel, mapping all operation details, timestamps, user information, property changes, and entity referencesHistoryMigratorto migrate audit logs last (after all other entities)Migration Order
Audit logs are migrated after all other history entities to ensure all referenced entities exist:
Process Definitions → Process Instances → Flow Nodes → User Tasks → Variables → Incidents → Decision Requirements → Decision Definitions → Decision Instances → Audit Logs
What Gets Migrated
Camunda 7's
UserOperationLogEntrycaptures:Dependencies
This implementation depends on Camunda 8.9.0-SNAPSHOT which provides:
io.camunda.db.rdbms.write.domain.AuditLogDbModelio.camunda.db.rdbms.sql.AuditLogMapperType of Change
Testing Checklist
Black-Box Testing Requirements
DbClient,IdKeyMapper,..impl..packages except logging constants)ArchitectureTestvalidates these rules)Test Coverage
Architecture Compliance
Run architecture tests to ensure compliance:
mvn test -Dtest=ArchitectureTestNote: Tests require Camunda 8.9.0 dependencies with audit log RDBMS schema support.
Documentation
Checklist
Related Issues
Related to camunda/camunda#41126
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.