fix: error handling for intent api endpoints#1560
Conversation
📝 WalkthroughWalkthroughAdds input trimming and UUID validation to API Gateway ecosystem endpoints, tightens DTO validation, changes some public user parameter types, adds existence and duplicate checks with explicit error responses in ecosystem service, relaxes repository not-found behavior to return null, and adds new ecosystem error messages. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
end
rect rgba(200,255,200,0.5)
participant API_Gateway as API Gateway
end
rect rgba(255,200,200,0.5)
participant Ecosystem_MS as Ecosystem Service
end
participant Repo as Repository/DB
Client->>API_Gateway: HTTP request (path/query with IDs)
Note right of API_Gateway: TrimStringParamPipe\nParseUUIDPipe validate & trim IDs
API_Gateway->>Ecosystem_MS: call with trimmed UUIDs and user.id
Ecosystem_MS->>Repo: existence checks (ecosystem / intent / template)
alt found
Repo-->>Ecosystem_MS: record(s)
Ecosystem_MS-->>API_Gateway: success payload
API_Gateway-->>Client: 200 OK
else missing or duplicate
Repo-->>Ecosystem_MS: null or conflict
Ecosystem_MS-->>API_Gateway: NotFound / Conflict (RpcException)
API_Gateway-->>Client: 4xx error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api-gateway/src/ecosystem/ecosystem.controller.ts (1)
659-659:⚠️ Potential issue | 🟡 MinorMissing
TrimStringParamPipeandParseUUIDPipeonidparam fordeleteIntentTemplate.Same inconsistency as
updateIntentTemplate— theidparam should be validated.Proposed fix
- async deleteIntentTemplate(`@Param`('id') id: string, `@Res`() res: Response): Promise<Response> { + async deleteIntentTemplate( + `@Param`( + 'id', + TrimStringParamPipe, + new ParseUUIDPipe({ + exceptionFactory: (): Error => { + throw new BadRequestException(ResponseMessages.oid4vpIntentToTemplate.error.invalidId); + } + }) + ) + id: string, + `@Res`() res: Response + ): Promise<Response> {
🤖 Fix all issues with AI agents
In `@apps/api-gateway/src/ecosystem/ecosystem.controller.ts`:
- Around line 632-633: The id parameter in updateIntentTemplate needs the same
validation as other endpoints—apply TrimStringParamPipe and ParseUUIDPipe to the
`@Param`('id') decorator so whitespace-trimmed values and invalid UUIDs are
rejected; locate the updateIntentTemplate method and change its `@Param`('id')
usage to include TrimStringParamPipe and ParseUUIDPipe (matching the pattern
used in getIntentTemplateById/deleteIntent) so the id argument is validated and
trimmed before use.
- Around line 746-748: The optional query param intentId is only trimmed but not
validated as a UUID; update the controller method (the parameter and call to
ecosystemService.getIntents) to validate intentId with Nest's ParseUUIDPipe when
present: import ParseUUIDPipe and, before calling
this.ecosystemService.getIntents, if (intentId) run new ParseUUIDPipe({ version:
'4' }).transform(intentId.trim(), { type: 'query', metatype: String }) (or apply
an equivalent parameter pipe that only runs when a value exists), then pass the
validated value to getIntents to prevent invalid DB queries.
In `@apps/api-gateway/src/utilities/dtos/intent-template.dto.ts`:
- Around line 28-31: Update UpdateIntentTemplateDto.intentId to be required to
match service validation: remove the `@IsOptional`() decorator from the intentId
property on the UpdateIntentTemplateDto class so the DTO enforces presence of
intentId (and ensure the TypeScript property is not optional — e.g., no trailing
? — to match the decorator change); this keeps DTO validation consistent with
the required check in ecosystem.service (the code path that currently expects a
required intentId).
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 687-689: The error message thrown when userId is missing is
grammatically incorrect; update the BadRequestException call that checks if
(!userId) to use a correct message such as "userId is required" (or "userId is
required."). Locate the conditional that throws new BadRequestException('userId
are required') and replace the string with the corrected message so the check in
the function that references userId and the BadRequestException reflects proper
grammar.
- Around line 575-582: The method in ecosystem.service.ts that accepts
templateData (typed so intentId is required) must be updated to match
UpdateIntentTemplateDto where intentId is optional: change the method signature
so templateData.intentId is typed as string | undefined (e.g., accept
templateData: UpdateIntentTemplateDto or declare intentId?: string in the
parameter destructure), update any internal uses to handle undefined
accordingly, and adjust callers if necessary so TypeScript enforces the
optionality instead of relying on the runtime guard; reference
UpdateIntentTemplateDto and the templateData.intentId usage to locate the
change.
🧹 Nitpick comments (3)
apps/ecosystem/src/ecosystem.service.ts (3)
629-633: Good: validates ecosystem existence before creating an intent.This aligns with the design that intent records always require a valid
ecosystemId.One minor inconsistency: this throws
NotFoundException(NestJS HTTP exception) while the similar check ingetIntentTemplatesByIntentId(line 496) throwsRpcException. In a microservice context,RpcExceptionis the canonical way to propagate errors back to the API gateway. Consider usingRpcExceptionhere for consistency.Proposed fix for consistency
if (!ecosystem) { - throw new NotFoundException(ResponseMessages.ecosystem.error.ecosystemNotFound); + throw new RpcException({ + statusCode: HttpStatus.NOT_FOUND, + message: ResponseMessages.ecosystem.error.ecosystemNotFound + }); }Based on learnings: "ecosystemId is NOT NULL because intent records are never created without an ecosystemId in the application workflow."
691-696: Consider validating that the intent exists before updating.
updateIntentTemplate(line 585) validates the record exists before updating;updateIntentdoes not. If the intent is missing, Prisma will throw a genericP2025error that gets wrapped as "Failed to update intent" — a proper 404 would be more helpful to callers. This is especially relevant given the PR's stated goal of improving error handling for intent API endpoints.Proposed fix
if (!userId) { throw new BadRequestException('userId is required'); } + const existingIntent = await this.ecosystemRepository.findIntentById(updateIntentDto.intentId); + if (!existingIntent) { + throw new RpcException({ + statusCode: HttpStatus.NOT_FOUND, + message: ResponseMessages.ecosystem.error.intentNotFound + }); + } + const intent = await this.ecosystemRepository.updateIntent({
713-721:deleteIntentlacks error handling, inconsistent with the rest of the PR.This method has no try/catch, no existence validation, and no
ErrorHandler.categorize— unlike every other intent method in this file (includingdeleteIntentTemplateat line 609). Given the PR's goal of improving error handling for intent endpoints, this seems like an oversight.Proposed fix
async deleteIntent(data: { ecosystemId: string; intentId: string; userId: string }): Promise<object> { const { ecosystemId, intentId, userId } = data; - - return this.ecosystemRepository.deleteIntent({ - ecosystemId, - intentId, - userId - }); + try { + const intent = await this.ecosystemRepository.findIntentById(intentId); + if (!intent) { + throw new RpcException({ + statusCode: HttpStatus.NOT_FOUND, + message: ResponseMessages.ecosystem.error.intentNotFound + }); + } + + return await this.ecosystemRepository.deleteIntent({ + ecosystemId, + intentId, + userId + }); + } catch (error) { + const errorResponse = ErrorHandler.categorize(error, 'Failed to delete intent'); + this.logger.error( + `[deleteIntent] - ${errorResponse.statusCode}: ${errorResponse.message}`, + ErrorHandler.format(error) + ); + throw new RpcException(errorResponse); + } }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/api-gateway/src/ecosystem/ecosystem.controller.ts`:
- Around line 755-763: The ParseUUIDPipe on the optional query param intentId
will error when intentId is omitted; update the ParseUUIDPipe instantiation used
on the intentId parameter so it allows undefined by passing optional: true into
its options (i.e., new ParseUUIDPipe({ optional: true, exceptionFactory: ...
})), ensuring the BadRequestException behavior remains for invalid UUIDs but
does not reject requests that omit intentId in the ecosystem.controller.ts
endpoint.
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 642-646: Replace the HTTP NotFoundException thrown after the call
to this.ecosystemRepository.getEcosystemById(ecosystemId) with an RpcException
that carries the same message and HttpStatus.NOT_FOUND (mirror the pattern
already used earlier), and do the same change in the getIntents flow at the
corresponding check; ensure RpcException and HttpStatus are imported/available
and use the same ResponseMessages.ecosystem.error.ecosystemNotFound message so
transport-aware microservice clients receive the statusCode correctly.
🧹 Nitpick comments (1)
apps/ecosystem/src/ecosystem.service.ts (1)
696-709:updateIntentdoes not verify the intent exists before updating.Other mutating methods (
updateIntentTemplateat line 582,createIntentat line 642) now validate entity existence before proceeding.updateIntentskips this — if theintentIdrefers to a non-existent record, the repository call will either silently no-op or throw an opaque Prisma error. Consider adding afindIntentByIdcheck before the update, consistent with the pattern established elsewhere in this PR.Proposed addition before line 704
+ const existingIntent = await this.ecosystemRepository.findIntentById(updateIntentDto.intentId); + if (!existingIntent) { + throw new RpcException({ + statusCode: HttpStatus.NOT_FOUND, + message: ResponseMessages.ecosystem.error.intentNotFound + }); + } + const intent = await this.ecosystemRepository.updateIntent({
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
pranalidhanavade
force-pushed
the
fix/error-handling-for-intent-api-endpoints
branch
from
dac75b5 to
55d2688
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 731-735: The null-checks in ecosystem.service.ts are dead because
ecosystemRepository.getEcosystemById and getIntents already throw
NotFoundException on missing records; remove the unreachable guards (the if
(!ecosystem) throw ... block around the call to getEcosystemById and the
analogous if (!intents) check around getIntents) so the service relies on the
repository to raise NotFoundException, or alternatively change the repository
methods (getEcosystemById/getIntents) to return null and move the
NotFoundException into the service—pick one consistent approach (prefer removing
the redundant checks in the service to match current repository behavior).
In `@libs/common/src/response-messages/index.ts`:
- Line 215: The response-messages export includes an unused constant
intentIdIsRequired; remove the intentIdIsRequired entry from the messages object
in libs/common/src/response-messages/index.ts (leaving intentNotFound intact,
which is used by ecosystem.service.ts) and run a quick search for
intentIdIsRequired to confirm it has no references before committing; if you
intended to validate/messaging in ecosystem logic instead, add usage of
intentIdIsRequired in the appropriate validation path (e.g., the method in
ecosystem.service that checks for a missing intent id) rather than keeping an
unused constant.
🧹 Nitpick comments (1)
apps/ecosystem/repositories/ecosystem.repository.ts (1)
643-660: Log message is misleading when the template is not found.Line 654 logs
fetched id ${id}regardless of whether the record was actually found. This can be confusing when debugging null-return cases.Proposed fix
- this.logger.log(`[getIntentTemplateById] fetched id ${id}`); + this.logger.log(`[getIntentTemplateById] id=${id} found=${!!intentTemplate}`);
| const ecosystem = await this.ecosystemRepository.getEcosystemById(ecosystemId); | ||
|
|
||
| if (!name || !ecosystemId || !userId) { | ||
| throw new BadRequestException('name, ecosystemId and userId are required'); | ||
| if (!ecosystem) { | ||
| throw new NotFoundException(ResponseMessages.ecosystem.error.ecosystemNotFound); | ||
| } |
There was a problem hiding this comment.
Dead code: getEcosystemById never returns null.
The repository method getEcosystemById (Line 614–627 of ecosystem.repository.ts) throws NotFoundException when the ecosystem is not found, so it never returns a falsy value. The if (!ecosystem) check here is unreachable. The same applies to getIntents at Lines 761–765.
Either remove the dead checks or change the repository to return null (like getIntentTemplateById was changed in this PR) and handle the not-found case here.
🤖 Prompt for AI Agents
In `@apps/ecosystem/src/ecosystem.service.ts` around lines 731 - 735, The
null-checks in ecosystem.service.ts are dead because
ecosystemRepository.getEcosystemById and getIntents already throw
NotFoundException on missing records; remove the unreachable guards (the if
(!ecosystem) throw ... block around the call to getEcosystemById and the
analogous if (!intents) check around getIntents) so the service relies on the
repository to raise NotFoundException, or alternatively change the repository
methods (getEcosystemById/getIntents) to return null and move the
NotFoundException into the service—pick one consistent approach (prefer removing
the redundant checks in the service to match current repository behavior).
| failInvitationUpdate: 'Failed to update Ecosystem Invitation', | ||
| ecosystemIdIsRequired: 'ecosystemId is required', | ||
| notCreated: 'Error while creating ecosystem', | ||
| intentNotFound: 'Ecosystem intent not found', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "intentIdIsRequired" --type tsRepository: credebl/platform
Length of output: 156
🏁 Script executed:
rg -n "intentNotFound" --type tsRepository: credebl/platform
Length of output: 269
Remove unused error message intentIdIsRequired.
intentNotFound (line 215) is properly consumed in ecosystem.service.ts:587. However, intentIdIsRequired (line 275) is defined but never referenced anywhere in the codebase. Either use it in the corresponding service/controller logic or remove it to keep the message constants clean.
🤖 Prompt for AI Agents
In `@libs/common/src/response-messages/index.ts` at line 215, The
response-messages export includes an unused constant intentIdIsRequired; remove
the intentIdIsRequired entry from the messages object in
libs/common/src/response-messages/index.ts (leaving intentNotFound intact, which
is used by ecosystem.service.ts) and run a quick search for intentIdIsRequired
to confirm it has no references before committing; if you intended to
validate/messaging in ecosystem logic instead, add usage of intentIdIsRequired
in the appropriate validation path (e.g., the method in ecosystem.service that
checks for a missing intent id) rather than keeping an unused constant.
2 participants
What
Summary by CodeRabbit
New Features
Bug Fixes