Skip to content

Commit

Permalink
refactor: use entity that inherently doesn't log
Browse files Browse the repository at this point in the history
  • Loading branch information
iamacook committed Mar 3, 2025
1 parent dbcaec9 commit e4360d5
Show file tree
Hide file tree
Showing 11 changed files with 160 additions and 244 deletions.
13 changes: 13 additions & 0 deletions src/domain/common/errors/http-exception-no-log.error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { HttpException } from '@nestjs/common';
import type { HttpStatus } from '@nestjs/common';

/**
* The following errors combine with out {@link GlobalErrorFilter} to prevent
* logging if thrown. This is because {@link GlobalErrorFilter} catches all
* errors and logs them, but we may want to manually log
*/
export class HttpExceptionNoLog extends HttpException {
constructor(message: string, code: HttpStatus) {
super(HttpExceptionNoLog.createBody(message, undefined!, code), code);
}
}
16 changes: 0 additions & 16 deletions src/domain/common/errors/http-exception-with-log.errors.ts

This file was deleted.

11 changes: 5 additions & 6 deletions src/domain/safe/safe.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { TransactionVerifierHelper } from '@/routes/transactions/helpers/transac
import { IContractsRepository } from '@/domain/contracts/contracts.repository.interface';
import { IConfigurationService } from '@/config/configuration.service.interface';
import { Operation } from '@/domain/safe/entities/operation.entity';
import { HttpExceptionWithLog } from '@/domain/common/errors/http-exception-with-log.errors';
import { HttpExceptionNoLog } from '@/domain/common/errors/http-exception-no-log.error';

@Injectable()
export class SafeRepository implements ISafeRepository {
Expand Down Expand Up @@ -639,11 +639,10 @@ export class SafeRepository implements ISafeRepository {
args.chainId,
);

const error = new HttpExceptionWithLog({
message: 'Delegate call is disabled',
code: HttpStatus.UNPROCESSABLE_ENTITY,
log: true,
});
const error = new HttpExceptionNoLog(
'Delegate call is disabled',
HttpStatus.UNPROCESSABLE_ENTITY,
);
if (args.proposeTransactionDto.operation === Operation.DELEGATE) {
if (!this.isTrustedDelegateCallEnabled) {
throw error;
Expand Down
19 changes: 8 additions & 11 deletions src/routes/common/filters/global-error.filter.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { ConfigurationModule } from '@/config/configuration.module';
import configuration from '@/config/entities/__tests__/configuration';
import { GlobalErrorFilter } from '@/routes/common/filters/global-error.filter';
import { Server } from 'net';
import { HttpExceptionWithLog } from '@/domain/common/errors/http-exception-with-log.errors';
import { HttpExceptionNoLog } from '@/domain/common/errors/http-exception-no-log.error';
import { ILoggingService, LoggingService } from '@/logging/logging.interface';

@Controller({})
Expand All @@ -29,11 +29,7 @@ class TestController {

@Post('no-log-http-exception')
noLogHttpException(@Body() body: { code: HttpStatus }): void {
throw new HttpExceptionWithLog({
message: 'Some no log http exception',
code: body.code,
log: false,
});
throw new HttpExceptionNoLog('Some no log http exception', body.code);
}

@Get('non-http-exception')
Expand All @@ -46,9 +42,11 @@ describe('GlobalErrorFilter tests', () => {
let app: INestApplication<Server>;
let loggingService: ILoggingService;

beforeEach(async () => {
beforeEach(() => {
jest.resetAllMocks();
});

beforeAll(async () => {
const moduleFixture: TestingModule = await Test.createTestingModule({
imports: [TestLoggingModule, ConfigurationModule.register(configuration)],
controllers: [TestController],
Expand All @@ -69,7 +67,7 @@ describe('GlobalErrorFilter tests', () => {
await app.init();
});

afterEach(async () => {
afterAll(async () => {
await app.close();
});

Expand All @@ -91,7 +89,7 @@ describe('GlobalErrorFilter tests', () => {
.post('/no-log-http-exception')
.send({ code })
.expect(code)
.expect({ message: 'Some no log http exception' });
.expect({ message: 'Some no log http exception', statusCode: code });
});

it('non http exception returns correct error code and message', async () => {
Expand All @@ -112,8 +110,7 @@ describe('GlobalErrorFilter tests', () => {
await request(app.getHttpServer())
.post('/http-exception')
.send({ code })
.expect(code)
.expect({ message: 'Some http exception' });
.expect(code);

expect(loggingService.info).not.toHaveBeenCalled();
expect(loggingService.error).toHaveBeenCalledTimes(1);
Expand Down
10 changes: 5 additions & 5 deletions src/routes/common/filters/global-error.filter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
} from '@nestjs/common';
import { ILoggingService, LoggingService } from '@/logging/logging.interface';
import { HttpAdapterHost } from '@nestjs/core';
import { HttpExceptionWithLog } from '@/domain/common/errors/http-exception-with-log.errors';
import { HttpExceptionNoLog } from '@/domain/common/errors/http-exception-no-log.error';

@Catch()
export class GlobalErrorFilter implements ExceptionFilter {
Expand All @@ -23,14 +23,14 @@ export class GlobalErrorFilter implements ExceptionFilter {
const ctx = host.switchToHttp();

const isHttpException = exception instanceof HttpException;
const isHttpExceptionWithLog = exception instanceof HttpExceptionWithLog;
const isHttpExceptionNoLog = exception.constructor === HttpExceptionNoLog;

const httpStatus =
isHttpException || isHttpExceptionWithLog
isHttpException || isHttpExceptionNoLog
? exception.getStatus()
: HttpStatus.INTERNAL_SERVER_ERROR;

if (!isHttpExceptionWithLog || exception.log) {
if (!isHttpExceptionNoLog) {
const logMessage = {
name: exception.name,
message: exception.message,
Expand All @@ -44,7 +44,7 @@ export class GlobalErrorFilter implements ExceptionFilter {
}

const responseBody =
isHttpException || isHttpExceptionWithLog
isHttpException || isHttpExceptionNoLog
? exception.getResponse()
: {
code: httpStatus,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import { addConfirmationDtoBuilder } from '@/routes/transactions/__tests__/entit
import type { Server } from 'net';
import { rawify } from '@/validation/entities/raw.entity';
import { generatePrivateKey, privateKeyToAccount } from 'viem/accounts';
import { GlobalErrorFilter } from '@/routes/common/filters/global-error.filter';
import { APP_FILTER } from '@nestjs/core';

describe('Add transaction confirmations - Transactions Controller (Unit)', () => {
let app: INestApplication<Server>;
Expand All @@ -47,6 +49,13 @@ describe('Add transaction confirmations - Transactions Controller (Unit)', () =>
TestLoggingModule,
TestNetworkModule,
],
providers: [
// TODO: Add to all tests to reflect app implementation
{
provide: APP_FILTER,
useClass: GlobalErrorFilter,
},
],
}).compile();

const configurationService = moduleFixture.get<IConfigurationService>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ import { TargetedMessagingDatasourceModule } from '@/datasources/targeted-messag
import { rawify } from '@/validation/entities/raw.entity';
import { generatePrivateKey, privateKeyToAccount } from 'viem/accounts';
import { SignatureType } from '@/domain/common/entities/signature-type.entity';
import { GlobalErrorFilter } from '@/routes/common/filters/global-error.filter';
import { APP_FILTER } from '@nestjs/core';

describe('Get by id - Transactions Controller (Unit)', () => {
let app: INestApplication<Server>;
Expand All @@ -57,6 +59,13 @@ describe('Get by id - Transactions Controller (Unit)', () => {
async function initApp(config: typeof configuration): Promise<void> {
const moduleFixture: TestingModule = await Test.createTestingModule({
imports: [AppModule.register(config)],
providers: [
// TODO: Add to all tests to reflect app implementation
{
provide: APP_FILTER,
useClass: GlobalErrorFilter,
},
],
})
.overrideModule(PostgresDatabaseModule)
.useModule(TestPostgresDatabaseModule)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ import { rawify } from '@/validation/entities/raw.entity';
import { generatePrivateKey, privateKeyToAccount } from 'viem/accounts';
import { SignatureType } from '@/domain/common/entities/signature-type.entity';
import { Operation } from '@/domain/safe/entities/operation.entity';
import { GlobalErrorFilter } from '@/routes/common/filters/global-error.filter';
import { APP_FILTER } from '@nestjs/core';

describe('Propose transaction - Transactions Controller (Unit)', () => {
let app: INestApplication<Server>;
Expand All @@ -49,6 +51,13 @@ describe('Propose transaction - Transactions Controller (Unit)', () => {
async function initApp(config: typeof configuration): Promise<void> {
const moduleFixture: TestingModule = await Test.createTestingModule({
imports: [AppModule.register(config)],
providers: [
// TODO: Add to all tests to reflect app implementation
{
provide: APP_FILTER,
useClass: GlobalErrorFilter,
},
],
})
.overrideModule(PostgresDatabaseModule)
.useModule(TestPostgresDatabaseModule)
Expand Down
Loading

0 comments on commit e4360d5

Please sign in to comment.