From c001d7737d5fae1e5be016ff4a06bcda8a5831d3 Mon Sep 17 00:00:00 2001 From: james simone Date: Sun, 6 Jun 2021 14:16:47 -0600 Subject: [PATCH 1/3] potential fix for #166 by introducing limits-minded deletion mechanism in LogBatchPurger --- .../log-management/classes/LogBatchPurger.cls | 64 ++++++++++++++++--- 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/nebula-logger/main/log-management/classes/LogBatchPurger.cls b/nebula-logger/main/log-management/classes/LogBatchPurger.cls index 648df6fe6..2baceacfb 100644 --- a/nebula-logger/main/log-management/classes/LogBatchPurger.cls +++ b/nebula-logger/main/log-management/classes/LogBatchPurger.cls @@ -9,12 +9,60 @@ * @see LogBatchPurgeScheduler */ global with sharing class LogBatchPurger implements Database.Batchable, Database.Stateful { + private final Boolean isSystemDebuggingEnabled; + private String originalTransactionId; private Integer totalProcessedRecords = 0; + // Database.emptyRecycleBin counts as a DML statement per record + private static Integer MAX_RECORDS_DELETED = (Limits.getLimitDmlRows() / 2) - 1; + private static Integer DELETED_COUNT = 0; + private class LogBatchPurgerException extends Exception { } + global LogBatchPurger() { + this.isSystemDebuggingEnabled = Logger.getUserSettings()?.EnableSystemMessages__c == true; + } + + private class LogDeleter implements System.Queueable { + private final List recordsToDelete; + public LogDeleter(List recordsToDelete) { + this.recordsToDelete = recordsToDelete; + } + + public void process() { + if (DELETED_COUNT + this.recordsToDelete.size() < MAX_RECORDS_DELETED) { + this.hardDelete(this.recordsToDelete); + this.recordsToDelete.clear(); + } else { + List safeToDeleteRecords = new List(); + while (this.recordsToDelete.size() > MAX_RECORDS_DELETED && !this.recordsToDelete.isEmpty()) { + for (Integer index = this.recordsToDelete.size() - 1; index >= 0; index--) { + safeToDeleteRecords.add(this.recordsToDelete[index]); + this.recordsToDelete.remove(index); + } + } + this.hardDelete(safeToDeleteRecords); + } + + + if (!this.recordsToDelete.isEmpty() && Limits.getLimitQueueableJobs() > Limits.getQueueableJobs()) { + System.enqueueJob(this); + } + } + + public void execute(QueueableContext queueableContext) { + this.process(); + } + + private void hardDelete(List records) { + DELETED_COUNT += records.size(); + delete records; + Database.emptyRecycleBin(records); + } + } + global Database.QueryLocator start(Database.BatchableContext batchableContext) { if (!Schema.Log__c.SObjectType.getDescribe().isDeletable()) { throw new LogBatchPurgerException('User does not have access to delete logs'); @@ -24,7 +72,7 @@ global with sharing class LogBatchPurger implements Database.Batchable, // ...so store the first transaction ID to later relate the other transactions this.originalTransactionId = Logger.getTransactionId(); - if (Logger.getUserSettings().EnableSystemMessages__c == true) { + if (this.isSystemDebuggingEnabled) { Logger.info('Starting LogBatchPurger job'); Logger.saveLog(); } @@ -39,24 +87,22 @@ global with sharing class LogBatchPurger implements Database.Batchable, throw new LogBatchPurgerException('User does not have access to delete logs'); } - this.totalProcessedRecords += logsToDelete.size(); try { - if (Logger.getUserSettings().EnableSystemMessages__c == true) { + if (this.isSystemDebuggingEnabled) { Logger.setParentLogTransactionId(this.originalTransactionId); Logger.info(new LogMessage('Starting deletion of {0} records', logsToDelete.size())); } // Delete the child log entries first List logEntriesToDelete = [SELECT Id FROM LogEntry__c WHERE Log__c IN :logsToDelete]; - delete logEntriesToDelete; - Database.emptyRecycleBin(logEntriesToDelete); + new LogDeleter(logEntriesToDelete).process(); // Now delete the parent logs - delete logsToDelete; - Database.emptyRecycleBin(logsToDelete); + new LogDeleter(logsToDelete).process(); + this.totalProcessedRecords += DELETED_COUNT; } catch (Exception apexException) { - if (Logger.getUserSettings().EnableSystemMessages__c == true) { + if (this.isSystemDebuggingEnabled) { Logger.error('Error deleting logs', apexException); } } finally { @@ -65,7 +111,7 @@ global with sharing class LogBatchPurger implements Database.Batchable, } global void finish(Database.BatchableContext batchableContext) { - if (Logger.getUserSettings().EnableSystemMessages__c == true) { + if (this.isSystemDebuggingEnabled) { Logger.setParentLogTransactionId(this.originalTransactionId); Logger.info(new LogMessage('Finished LogBatchPurger job, {0} total log records processed', this.totalProcessedRecords)); Logger.saveLog(); From 8051d55391bb87a34421c731e5f090d8aced1f02 Mon Sep 17 00:00:00 2001 From: James Simone Date: Mon, 7 Jun 2021 07:57:23 -0600 Subject: [PATCH 2/3] Re-added code coverage for while loop --- .../log-management/classes/LogBatchPurger.cls | 1 + .../classes/LogBatchPurger_Tests.cls | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/nebula-logger/main/log-management/classes/LogBatchPurger.cls b/nebula-logger/main/log-management/classes/LogBatchPurger.cls index 2baceacfb..310a128b2 100644 --- a/nebula-logger/main/log-management/classes/LogBatchPurger.cls +++ b/nebula-logger/main/log-management/classes/LogBatchPurger.cls @@ -14,6 +14,7 @@ global with sharing class LogBatchPurger implements Database.Batchable, private String originalTransactionId; private Integer totalProcessedRecords = 0; + @testVisible // Database.emptyRecycleBin counts as a DML statement per record private static Integer MAX_RECORDS_DELETED = (Limits.getLimitDmlRows() / 2) - 1; private static Integer DELETED_COUNT = 0; diff --git a/nebula-logger/tests/log-management/classes/LogBatchPurger_Tests.cls b/nebula-logger/tests/log-management/classes/LogBatchPurger_Tests.cls index f78b589f3..a86f64bb7 100644 --- a/nebula-logger/tests/log-management/classes/LogBatchPurger_Tests.cls +++ b/nebula-logger/tests/log-management/classes/LogBatchPurger_Tests.cls @@ -92,6 +92,21 @@ private class LogBatchPurger_Tests { System.assertEquals(0, logEntries.size(), logEntries); } + @isTest + static void it_should_continue_deleting_logs_even_when_current_count_greater_than_limits() { + List logsToDelete = [SELECT Id FROM Log__c]; + LogBatchPurger.MAX_RECORDS_DELETED = logsToDelete.size() / 2; + + Test.startTest(); + new LogBatchPurger().execute(null, logsToDelete); + Test.stopTest(); + + logsToDelete = [SELECT Id FROM Log__c]; + List logEntries = [SELECT Id FROM LogEntry__c]; + System.assertEquals(0, logsToDelete.size(), logsToDelete); + System.assertEquals(0, logEntries.size(), logEntries); + } + @isTest static void it_should_not_delete_a_log_before_scheduled_deletion_date() { List logs = [SELECT Id, LogRetentionDate__c FROM Log__c]; From 44c79241c9125d235b46643e363ba7cc2bc857d4 Mon Sep 17 00:00:00 2001 From: James Simone Date: Mon, 7 Jun 2021 13:09:20 -0600 Subject: [PATCH 3/3] Prettier formatting, extra safety --- .../log-management/classes/LogBatchPurger.cls | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/nebula-logger/main/log-management/classes/LogBatchPurger.cls b/nebula-logger/main/log-management/classes/LogBatchPurger.cls index 310a128b2..5e2de0686 100644 --- a/nebula-logger/main/log-management/classes/LogBatchPurger.cls +++ b/nebula-logger/main/log-management/classes/LogBatchPurger.cls @@ -47,7 +47,6 @@ global with sharing class LogBatchPurger implements Database.Batchable, this.hardDelete(safeToDeleteRecords); } - if (!this.recordsToDelete.isEmpty() && Limits.getLimitQueueableJobs() > Limits.getQueueableJobs()) { System.enqueueJob(this); } @@ -58,9 +57,14 @@ global with sharing class LogBatchPurger implements Database.Batchable, } private void hardDelete(List records) { - DELETED_COUNT += records.size(); - delete records; - Database.emptyRecycleBin(records); + // normally this would be an anti-pattern since most DML operations + // are a no-op with an empty list - but emptyRecycleBin throws + // for empty lists! + if (!records.isEmpty()) { + DELETED_COUNT += records.size(); + delete records; + Database.emptyRecycleBin(records); + } } } @@ -88,15 +92,13 @@ global with sharing class LogBatchPurger implements Database.Batchable, throw new LogBatchPurgerException('User does not have access to delete logs'); } - try { + // Delete the child log entries first + List logEntriesToDelete = [SELECT Id FROM LogEntry__c WHERE Log__c IN :logsToDelete]; if (this.isSystemDebuggingEnabled) { Logger.setParentLogTransactionId(this.originalTransactionId); - Logger.info(new LogMessage('Starting deletion of {0} records', logsToDelete.size())); + Logger.info(new LogMessage('Starting deletion of {0} logs and {1} log entries', logsToDelete.size(), logEntriesToDelete.size())); } - - // Delete the child log entries first - List logEntriesToDelete = [SELECT Id FROM LogEntry__c WHERE Log__c IN :logsToDelete]; new LogDeleter(logEntriesToDelete).process(); // Now delete the parent logs