PM-1073 trolley webhook handling #28

vas3a · 2025-04-16T14:56:45Z

https://topcoder.atlassian.net/browse/PM-1073 - Implement (trolley) Webhook handlers in Finance API

NOTE: we need to add TROLLEY_WH_HMAC to env. If it's missing it will throw an early error (meaning server won't start without it, just so it is obvious that we're missing it)

I tried to make event handling as easy as possible, so when we start implementing them it should be quite easy:

create a new "handler" file in webhooks/trolley/handlers
make sure to add it in webhooks/trolley/index as service provider
the service needs to be injectable
create methods and add @webhookevent(<args: list of events to handle>). It can handle one or more events at the same time
see webhooks/trolley/handlers/payment.handler for example

We also record the status of the log entry: processing, processed or error. Error state has an additional "error_message" field to log the error.

Note: events are not automatically retried if an error happens while processing. Events (webhooks) should be retried only when the system (trolley) wasn't able to reach our api. We need to make sure errors are properly handled on our side.

github-actions · 2025-04-16T14:57:00Z

.editorconfig

+end_of_line = lf
+charset = utf-8
+trim_trailing_whitespace = true
+insert_final_newline = true


Ensure there is a newline at the end of the file. This is a common convention to avoid potential issues with some tools and version control systems.

github-actions · 2025-04-16T14:57:06Z

prisma/migrations/20250416100509_add_trolley_webhook_log_table/migration.sql

+CREATE TABLE "trolley_webhook_log" (
+    "id" UUID NOT NULL DEFAULT uuid_generate_v4(),
+    "event_id" TEXT NOT NULL,
+    "event_time" TIMESTAMP(6) NOT NULL DEFAULT CURRENT_TIMESTAMP,


Consider specifying the timezone for the TIMESTAMP fields to ensure consistency across different environments. Use TIMESTAMP WITH TIME ZONE if appropriate.

prisma/migrations/20250416100509_add_trolley_webhook_log_table/migration.sql

src/api/webhooks/trolley/handlers/payment.handler.ts

src/api/webhooks/trolley/trolley.service.ts

src/api/webhooks/webhooks.controller.ts

src/api/webhooks/webhooks.event-handlers.provider.ts

github-actions · 2025-04-16T14:57:42Z

src/main.ts

@@ -7,7 +7,9 @@ import { ApiModule } from './api/api.module';
 import { AppModule } from './app.module';

 async function bootstrap() {
-  const app = await NestFactory.create<NestExpressApplication>(AppModule);
+  const app = await NestFactory.create<NestExpressApplication>(AppModule, {
+    rawBody: true,


The rawBody: true option is added to the NestFactory.create method. Ensure that this change is necessary for handling webhook requests, as it affects how the request body is parsed. If not needed, consider removing it to avoid potential issues with request body parsing.

This is necessary in order to validate the wh signature. otherwise by parsing then stringifying back the payload, will break the signature. We need to work with the exact data we get from the body.

Setting this won't break other JSON body requests, right @vas3a ?

Tested it, so far it works as expected

src/api/webhooks/trolley/trolley.service.ts

src/api/webhooks/webhooks.event-handlers.provider.ts

kkartunov · 2025-04-17T06:16:02Z

prisma/schema.prisma

+  event_payload String
+  event_model   String?
+  event_action  String?
+  status        webhook_status


Not sure yet how we gonna use this and what will be the lifecycle of log status. Will plan this soon...
Please add new enum value in webhook_status as logged and default to it for status field.

@kkartunov "processing" is basically "logged". When we receive the event first thing is we insert it in db and mark it as "processing".
After something handles it (if there's any handler to process it) we mark it as "processed" or "error" depending on the status of the handler.
I'll replace "processing" with "logged" to be more straight forward, sounds good?

kkartunov · 2025-04-17T06:19:58Z

src/api/webhooks/webhooks.controller.ts

+        request.rawBody?.toString('utf-8') ?? '',
+      )
+    ) {
+      throw new BadRequestException('Missing or invalid signature!');


I think this should be 403 Forbidden as it is more logical sounding.

kkartunov · 2025-04-17T07:33:58Z

src/api/webhooks/webhooks.controller.ts

+   * @throws {BadRequestException} If the signature is invalid or the webhook has already been processed.
+   */
+  @Post('trolley')
+  async handleTrolleyWebhook(@Req() request: RawBodyRequest<Request>) {


This is all great with approaching & preparing for event handlers.
But what we want for scope of this is something different:

handleTrolleyWebhook need to always succeed and return 200 if we able to verify signature. Only if unable to verify signature respond with 403.

Make sure the event is logged in trolley_webhook_log with status logged.

In case of a repeat validateUnique just return 200 to Trolley and do nothing on our side.

we will handle events on our own. Whatever we do with them is not Trolley bussiness but Topcoder stuff.

That's true. the comment is deprecated. As mentioned in the PR, we're not throwing "processing" errors. Only missing or invalid signature will be thrown.
I'll supress the "unique" error as well - makes sense, right!

Hmm. let me rephrase that:

yes, we're always returning 200, and 403 for invalid signature (i was also throwing 400 for duplicate calls, but will supress it)

events are already logged as soon as they land

ok

exactly, that's already happening and I tried to mention it in the PR desc as well. we're on the same side 👍

github-actions · 2025-04-17T11:20:21Z

prisma/migrations/20250416100509_add_trolley_webhook_log_table/migration.sql

@@ -0,0 +1,23 @@
+-- CreateEnum
+CREATE TYPE "webhook_status" AS ENUM ('error', 'processed', 'logged');


The enum value 'processing' was changed to 'logged'. Ensure that this change aligns with the intended logic and that all references to 'processing' in the application code are updated accordingly to prevent any inconsistencies or errors.

github-actions · 2025-04-17T11:20:24Z

prisma/schema.prisma

+enum webhook_status {
+  error
+  processed
+  logged


Consider whether changing the enum value from processing to logged accurately reflects the intended state of the webhook log. If processing was meant to indicate an ongoing process, logged might not convey the same meaning. Ensure that this change aligns with the logic and handling of webhook statuses throughout the application.

github-actions · 2025-04-17T11:20:27Z

src/api/webhooks/trolley/trolley.service.ts

+    const requestId = headers[TrolleyHeaders.id];
+
+    try {
+      await this.setEventState(requestId, webhook_status.logged, payload, {


The event state has been changed from processing to logged. Ensure that this change aligns with the intended logic of the application. If the event should be marked as processing before handling, consider revisiting this change.

github-actions · 2025-04-17T11:20:27Z

src/api/webhooks/trolley/trolley.service.ts

+      const { model, action, body } = payload;
+      const handler = this.handlers.get(`${model}.${action}`);
+      // do nothing if there's no handler for the event (event was logged in db)
+      if (!handler) {


The previous implementation threw an error when no handler was found. The new implementation silently returns. Consider logging a warning or informational message to track unhandled events for monitoring purposes.

github-actions · 2025-04-17T11:20:32Z

src/api/webhooks/webhooks.controller.ts

+    }
+
+    // do not proceed any further if event has already been processed
+    if (!(await this.trolleyService.validateUnique(request.headers))) {


Consider adding a log entry or some form of notification when a webhook is identified as already processed. This can help with debugging and monitoring.

github-actions · 2025-04-17T11:20:32Z

src/api/webhooks/webhooks.controller.ts

+      return;
+    }
+
+    return this.trolleyService.handleEvent(request.headers, request.body);


The try-catch block for error handling has been removed. Consider reintroducing error handling to ensure that any exceptions thrown by handleEvent are properly managed and logged. This will help in diagnosing issues and maintaining system stability.

kkartunov

Looks good.

github-actions · 2025-04-18T09:28:41Z

src/api/webhooks/webhooks.controller.ts

+import {
+  Controller,
+  Post,
+  Req,


The BadRequestException import has been removed but not replaced with any other exception handling for bad requests. Ensure that any necessary exception handling for bad requests is implemented elsewhere in the code.

github-actions · 2025-04-18T09:28:43Z

src/api/webhooks/webhooks.controller.ts

+   *
+   * @param request - The incoming webhook request containing headers, raw body, and parsed body.
+   * @returns A success message if the webhook is processed successfully.
+   * @throws {ForbiddenException} If the signature is invalid or the webhook has already been processed.


The exception type has been changed from BadRequestException to ForbiddenException. Ensure that this change aligns with the intended behavior of the application. If the change is intentional, verify that all parts of the application that handle this exception are updated accordingly.

github-actions · 2025-04-18T09:28:46Z

src/api/webhooks/webhooks.controller.ts

+        request.rawBody?.toString('utf-8') ?? '',
+      )
+    ) {
+      throw new ForbiddenException('Missing or invalid signature!');


Changing the exception from BadRequestException to ForbiddenException may alter the intended HTTP response status code. Ensure that ForbiddenException (HTTP 403) is the correct choice for handling missing or invalid signatures, as it indicates that the request was valid but the server is refusing action, whereas BadRequestException (HTTP 400) indicates that the request itself is malformed. Verify that this change aligns with the desired API behavior.

vas3a added 2 commits April 16, 2025 17:33

PM-1073 - trolley webhook handler

2e9532e

PM-1073 - rework structure for webhooks

cf0aadb

vas3a requested a review from kkartunov April 16, 2025 14:56

github-actions bot reviewed Apr 16, 2025

View reviewed changes

src/api/webhooks/webhooks.controller.ts Outdated Show resolved Hide resolved

github-actions bot reviewed Apr 16, 2025

View reviewed changes

src/api/webhooks/webhooks.event-handlers.provider.ts Outdated Show resolved Hide resolved

src/api/webhooks/webhooks.event-handlers.provider.ts Show resolved Hide resolved

src/api/webhooks/webhooks.event-handlers.provider.ts Outdated Show resolved Hide resolved

github-actions bot reviewed Apr 16, 2025

View reviewed changes

PM-1073 PR feedback

3c745aa

github-actions bot reviewed Apr 16, 2025

View reviewed changes

src/api/webhooks/trolley/trolley.service.ts Show resolved Hide resolved

src/api/webhooks/trolley/trolley.service.ts Show resolved Hide resolved

github-actions bot reviewed Apr 16, 2025

View reviewed changes

src/api/webhooks/trolley/trolley.service.ts Show resolved Hide resolved

github-actions bot reviewed Apr 16, 2025

View reviewed changes

src/api/webhooks/webhooks.event-handlers.provider.ts Show resolved Hide resolved

kkartunov requested changes Apr 17, 2025

View reviewed changes

PM-1073 - PR feedback: do not throw error in webhooks if event received

c603a5f

github-actions bot reviewed Apr 17, 2025

View reviewed changes

kkartunov approved these changes Apr 17, 2025

View reviewed changes

PM-1073 - throw forbid exception

abfb9d0

vas3a merged commit 4c9ab94 into dev Apr 18, 2025
1 check passed

github-actions bot reviewed Apr 18, 2025

View reviewed changes

vas3a deleted the PM-1073_trolley-webhooks branch April 18, 2025 09:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PM-1073 trolley webhook handling #28

PM-1073 trolley webhook handling #28

vas3a commented Apr 16, 2025

github-actions bot Apr 16, 2025

github-actions bot Apr 16, 2025

github-actions bot Apr 16, 2025

vas3a Apr 16, 2025

kkartunov Apr 16, 2025

vas3a Apr 17, 2025

kkartunov Apr 17, 2025

vas3a Apr 17, 2025

kkartunov Apr 17, 2025

vas3a Apr 17, 2025

kkartunov Apr 17, 2025 •

edited

Loading

vas3a Apr 17, 2025

vas3a Apr 17, 2025

github-actions bot Apr 17, 2025

github-actions bot Apr 17, 2025

github-actions bot Apr 17, 2025

github-actions bot Apr 17, 2025

github-actions bot Apr 17, 2025

github-actions bot Apr 17, 2025

kkartunov left a comment

github-actions bot Apr 18, 2025

github-actions bot Apr 18, 2025

github-actions bot Apr 18, 2025

		@@ -0,0 +1,23 @@
		-- CreateEnum
		CREATE TYPE "webhook_status" AS ENUM ('error', 'processed', 'logged');

PM-1073 trolley webhook handling #28

PM-1073 trolley webhook handling #28

Conversation

vas3a commented Apr 16, 2025

github-actions bot Apr 16, 2025

Choose a reason for hiding this comment

github-actions bot Apr 16, 2025

Choose a reason for hiding this comment

github-actions bot Apr 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkartunov Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Apr 17, 2025

Choose a reason for hiding this comment

github-actions bot Apr 17, 2025

Choose a reason for hiding this comment

github-actions bot Apr 17, 2025

Choose a reason for hiding this comment

github-actions bot Apr 17, 2025

Choose a reason for hiding this comment

github-actions bot Apr 17, 2025

Choose a reason for hiding this comment

github-actions bot Apr 17, 2025

Choose a reason for hiding this comment

kkartunov left a comment

Choose a reason for hiding this comment

github-actions bot Apr 18, 2025

Choose a reason for hiding this comment

github-actions bot Apr 18, 2025

Choose a reason for hiding this comment

github-actions bot Apr 18, 2025

Choose a reason for hiding this comment

kkartunov Apr 17, 2025 •

edited

Loading