address code review: fix inclusive to-date filter, add retention error handling

- LoggingService.findLogs(): the `to` date filter compared a date-only
  string (e.g. from a date picker) against a timestamp column, which
  parses to midnight and silently excludes the entire last day. Widen
  it to end-of-day so the range is genuinely inclusive.
- LogRetentionScheduler.cleanupOldLogs(): wrap the delete in try/catch
  and log failures via logger.error, matching the existing convention
  in CashboxExportScheduler/RecurringTransactionsScheduler. Without
  this, a failed nightly cleanup would fail silently - exactly what
  this feature exists to prevent.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Bastian Wagner
2026-08-04 16:34:34 +02:00
parent f1b4f7e5b4
commit 020b390953
5 changed files with 41 additions and 10 deletions

View File

@@ -4,7 +4,7 @@ import { LogRetentionScheduler } from './log-retention.scheduler';
describe('LogRetentionScheduler', () => { describe('LogRetentionScheduler', () => {
const repository = { delete: jest.fn() }; const repository = { delete: jest.fn() };
const configService = { get: jest.fn() }; const configService = { get: jest.fn() };
const logger = { info: jest.fn() }; const logger = { info: jest.fn(), error: jest.fn() };
let scheduler: LogRetentionScheduler; let scheduler: LogRetentionScheduler;
beforeEach(() => { beforeEach(() => {
@@ -49,4 +49,17 @@ describe('LogRetentionScheduler', () => {
userId: -1, userId: -1,
}); });
}); });
it('logs and does not rethrow when the delete fails', async () => {
repository.delete.mockRejectedValue(new Error('connection reset'));
await expect(scheduler.cleanupOldLogs()).resolves.toBeUndefined();
expect(logger.error).toHaveBeenCalledWith({
event: 'log_retention_cleanup_run_fail',
details: 'connection reset',
userId: -1,
});
expect(logger.info).not.toHaveBeenCalled();
});
}); });

View File

@@ -21,6 +21,7 @@ export class LogRetentionScheduler {
const cutoff = new Date(); const cutoff = new Date();
cutoff.setUTCDate(cutoff.getUTCDate() - retentionDays); cutoff.setUTCDate(cutoff.getUTCDate() - retentionDays);
try {
const result = await this.repository.delete({ createdAt: LessThan(cutoff) }); const result = await this.repository.delete({ createdAt: LessThan(cutoff) });
await this.logger.info({ await this.logger.info({
@@ -28,5 +29,13 @@ export class LogRetentionScheduler {
details: `deletedCount=${result.affected ?? 0} retentionDays=${retentionDays}`, details: `deletedCount=${result.affected ?? 0} retentionDays=${retentionDays}`,
userId: -1, userId: -1,
}); });
} catch (error) {
const errorMessage = error instanceof Error ? error.message : String(error);
await this.logger.error({
event: 'log_retention_cleanup_run_fail',
details: errorMessage,
userId: -1,
});
}
} }
} }

View File

@@ -85,7 +85,12 @@ describe('LoggingService.findLogs', () => {
await service.findLogs({ page: 1, limit: 20, from: '2026-01-01', to: '2026-01-31' }); await service.findLogs({ page: 1, limit: 20, from: '2026-01-01', to: '2026-01-31' });
expect(query.andWhere).toHaveBeenCalledWith('log.createdAt >= :from', { from: '2026-01-01' }); expect(query.andWhere).toHaveBeenCalledWith('log.createdAt >= :from', { from: '2026-01-01' });
expect(query.andWhere).toHaveBeenCalledWith('log.createdAt <= :to', { to: '2026-01-31' }); // `to` is a plain date (e.g. from a <input type="date">); comparing it
// as-is would parse to midnight and exclude the whole last day, so it
// must be widened to the end of that day to be genuinely inclusive.
expect(query.andWhere).toHaveBeenCalledWith('log.createdAt <= :to', {
to: new Date('2026-01-31T23:59:59.999Z'),
});
}); });
it('does not add level/event/date filters when omitted', async () => { it('does not add level/event/date filters when omitted', async () => {

View File

@@ -115,7 +115,9 @@ export class LoggingService {
if (query.level) builder.andWhere('log.level = :level', { level: query.level }); if (query.level) builder.andWhere('log.level = :level', { level: query.level });
if (query.event) builder.andWhere('log.event = :event', { event: query.event }); if (query.event) builder.andWhere('log.event = :event', { event: query.event });
if (query.from) builder.andWhere('log.createdAt >= :from', { from: query.from }); if (query.from) builder.andWhere('log.createdAt >= :from', { from: query.from });
if (query.to) builder.andWhere('log.createdAt <= :to', { to: query.to }); if (query.to) {
builder.andWhere('log.createdAt <= :to', { to: new Date(`${query.to}T23:59:59.999Z`) });
}
const term = query.search?.trim().toLocaleLowerCase(); const term = query.search?.trim().toLocaleLowerCase();
if (term) { if (term) {
builder.andWhere('LOWER(log.details) LIKE :search', { search: `%${term}%` }); builder.andWhere('LOWER(log.details) LIKE :search', { search: `%${term}%` });

View File

@@ -37,7 +37,8 @@ export type LOGEVENT =
| 'cashbox_export_subscription_update' | 'cashbox_export_subscription_update'
| 'cashbox_export_subscription_run' | 'cashbox_export_subscription_run'
| 'cashbox_export_subscription_run_fail' | 'cashbox_export_subscription_run_fail'
| 'log_retention_cleanup_run'; | 'log_retention_cleanup_run'
| 'log_retention_cleanup_run_fail';
export const LOGEVENT_VALUES: LOGEVENT[] = [ export const LOGEVENT_VALUES: LOGEVENT[] = [
'user_create', 'user_create',
@@ -78,6 +79,7 @@ export const LOGEVENT_VALUES: LOGEVENT[] = [
'cashbox_export_subscription_run', 'cashbox_export_subscription_run',
'cashbox_export_subscription_run_fail', 'cashbox_export_subscription_run_fail',
'log_retention_cleanup_run', 'log_retention_cleanup_run',
'log_retention_cleanup_run_fail',
]; ];
export type LOGLEVEL = 'FATAL' | 'ERROR' | 'WARN' | 'INFO' | 'DEBUG' | 'TRACE'; export type LOGLEVEL = 'FATAL' | 'ERROR' | 'WARN' | 'INFO' | 'DEBUG' | 'TRACE';