Conversation
...eorm/core/migrations/common/1769200000000-addUniversalIdentifierAndApplicationIdToWebhook.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/webhook/webhook.service.ts
Show resolved
Hide resolved
Greptile OverviewGreptile SummaryThis PR refactors the webhook system to use the metadata-modules architecture pattern (webhook v2), introducing Key changes:
Issue found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant GraphQL as GraphQL Resolver
participant WebhookService as Webhook Service
participant AppService as Application Service
participant Migration as Migration Service
participant Validator as Webhook Validator
participant Cache as Flat Entity Cache
participant DB as Database
Note over Client,DB: Create Webhook Flow
Client->>GraphQL: createWebhook(input)
GraphQL->>WebhookService: create(input, workspaceId)
WebhookService->>WebhookService: normalizeTargetUrl(targetUrl)
WebhookService->>WebhookService: validateTargetUrl(targetUrl)
WebhookService->>AppService: findWorkspaceTwentyStandardAndCustomApplicationOrThrow()
AppService->>Cache: getOrRecompute(flatApplicationMaps)
Cache-->>AppService: return application maps
AppService-->>WebhookService: return workspaceCustomFlatApplication
WebhookService->>WebhookService: fromCreateWebhookInputToFlatWebhookToCreate()
WebhookService->>Migration: validateBuildAndRunWorkspaceMigration()
Migration->>Validator: validateFlatWebhookCreation()
Validator->>Validator: validateTargetUrl()
Validator->>Validator: check secret is non-empty
Validator-->>Migration: validation result
Migration->>DB: INSERT webhook entity
DB-->>Migration: success
Migration->>Cache: invalidate flatWebhookMaps
Migration-->>WebhookService: migration complete
WebhookService->>Cache: getOrRecomputeManyOrAllFlatEntityMaps(flatWebhookMaps)
Cache->>DB: SELECT webhooks
DB-->>Cache: webhook entities
Cache-->>WebhookService: updated flatWebhookMaps
WebhookService->>WebhookService: fromFlatWebhookToWebhookDto()
WebhookService-->>GraphQL: return WebhookDTO
GraphQL-->>Client: return webhook
Note over Client,DB: Webhook Event Triggering
participant EventEmitter as Event Emitter
participant JobQueue as Webhook Job Queue
participant WebhookJob as Call Webhook Job
participant QueryService as WebhookQueryService
EventEmitter->>JobQueue: emit workspace event batch
JobQueue->>WebhookJob: process event
WebhookJob->>QueryService: findByOperations(workspaceId, operations)
QueryService->>DB: SELECT webhooks WHERE operations CONTAINS
DB-->>QueryService: matching webhooks
QueryService-->>WebhookJob: webhook entities
WebhookJob->>WebhookJob: transformEventBatchToWebhookEvents()
WebhookJob->>JobQueue: queue individual webhook calls
JobQueue-->>WebhookJob: jobs queued
|
...eorm/core/migrations/common/1769200000000-addUniversalIdentifierAndApplicationIdToWebhook.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
2 issues found across 56 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/core-modules/webhook/webhook.module.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/webhook/webhook.module.ts:19">
P1: CoreWebhookModule no longer imports PermissionsModule, but WebhookController still uses SettingsPermissionGuard which injects PermissionsService. Since WebhookModule doesn’t export PermissionsService, this module will fail to resolve the guard’s dependency at runtime. Re-add PermissionsModule to the imports.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/webhook/controllers/webhook.controller.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/webhook/controllers/webhook.controller.ts:67">
P2: The update body no longer uses a DTO class, so validation decorators on `UpdateWebhookInputUpdates` are skipped. This allows invalid payload types to reach the service and can cause runtime errors or inconsistent data. Use the DTO class for the `@Body()` type so validation runs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/twenty-server/src/engine/core-modules/webhook/webhook.module.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Patch(':id') | ||
| async update( | ||
| @Param('id') id: string, | ||
| @Body() updateWebhookDto: UpdateWebhookInput, | ||
| @Body() | ||
| updateWebhookDto: { |
There was a problem hiding this comment.
P2: The update body no longer uses a DTO class, so validation decorators on UpdateWebhookInputUpdates are skipped. This allows invalid payload types to reach the service and can cause runtime errors or inconsistent data. Use the DTO class for the @Body() type so validation runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/webhook/controllers/webhook.controller.ts, line 67:
<comment>The update body no longer uses a DTO class, so validation decorators on `UpdateWebhookInputUpdates` are skipped. This allows invalid payload types to reach the service and can cause runtime errors or inconsistent data. Use the DTO class for the `@Body()` type so validation runs.</comment>
<file context>
@@ -40,48 +40,53 @@ export class WebhookController {
@Param('id') id: string,
- @Body() updateWebhookDto: UpdateWebhookInput,
+ @Body()
+ updateWebhookDto: {
+ targetUrl?: string;
+ operations?: string[];
</file context>
packages/twenty-server/src/engine/metadata-modules/webhook/controllers/webhook.controller.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/webhook/controllers/webhook.controller.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/webhook/controllers/webhook.controller.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/webhook/webhook.resolver.ts
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:6251 This environment will automatically shut down when the PR is closed or after 5 hours. |
There was a problem hiding this comment.
Hey there ! Made a quick review found two main points that need to be clarified before merging
Could you please also add integration test coverage that covers all validation exceptions ?
Please let me know !
- perf concerns on find all web hook service handler
- invalid migration -> we should already introduce the save point pattern here ( backfill and migration upgrade command might be added later though )
.../engine/metadata-modules/flat-webhook/constants/flat-webhook-editable-properties.constant.ts
Show resolved
Hide resolved
...ty-server/src/database/typeorm/core/migrations/common/1769517102605-addUniversalToWebhook.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/webhook/webhook.service.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/webhook/webhook.service.ts
Outdated
Show resolved
Hide resolved
| new Date(a.createdAt).getTime() - new Date(b.createdAt).getTime(), | ||
| ) | ||
| return webhooks | ||
| .map(fromWebhookEntityToFlatWebhook) |
There was a problem hiding this comment.
Nitpick: Could introduce a from entity to flat too
|
|
||
| if (!isDefined(flatWebhook)) { | ||
| if (!isDefined(webhook)) { | ||
| return null; |
There was a problem hiding this comment.
Question: Should we throw ?
| `ALTER TABLE "core"."webhook" ALTER COLUMN "universalIdentifier" SET NOT NULL`, | ||
| ); | ||
| await queryRunner.query( | ||
| `ALTER TABLE "core"."webhook" ALTER COLUMN "applicationId" SET NOT NULL`, |
📊 API Changes ReportGraphQL Schema ChangesGraphQL Schema Changes[log] [log] ✖ Type DeleteWebhookInput was removed GraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[log] [log] ✖ Type DeleteWebhookInput was removed
|
| const flatWebhookToCreate = fromCreateWebhookInputToFlatWebhookToCreate({ | ||
| createWebhookInput: { | ||
| ...input, | ||
| targetUrl: normalizedTargetUrl, | ||
| }, | ||
| workspaceId, | ||
| applicationId: workspaceCustomFlatApplication.id, | ||
| }); |
There was a problem hiding this comment.
Bug: The CreateWebhookInput DTO allows an empty operations array, which creates a webhook that will never be triggered because it cannot match any events.
Severity: MEDIUM
Suggested Fix
Add the @ArrayNotEmpty() decorator from class-validator to the operations field in the CreateWebhookInput DTO. This will ensure that any attempt to create or update a webhook with an empty operations array is rejected with a validation error.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-server/src/engine/metadata-modules/webhook/webhook.service.ts#L76-L83
Potential issue: The `operations` field in the `CreateWebhookInput` DTO is validated
with `@IsArray()`, which permits empty arrays. No other validation, such as
`@ArrayNotEmpty()`, is present. When a user creates a webhook with an empty `operations`
array, this value is persisted to the database. The job that triggers webhooks uses an
`ArrayContains` query to find relevant webhooks based on event operations. An empty
`operations` array will never match any event, causing the webhook to be silently
non-functional without any error feedback to the user.
Did we get this right? 👍 / 👎 to inform future reviews.
|
Hey @charlesBochet! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
1 similar comment
|
Hey @charlesBochet! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
Migrate webhook to v2 entity