refactor: GET all ecosystems API to support orgId as optional query param.#1556
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded optional orgId propagation and strict UUID/string validation across API Gateway and Ecosystem services/controllers/repos; introduced org-scoped repository query; adjusted interfaces nullability; removed a large static prisma data file; standardized response messages and simplified invite/error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_GW_Controller as API_GW_Controller
participant API_GW_Service as API_GW_Service
participant NATS
participant Eco_Service as Ecosystem_Service
participant Eco_Repo as Ecosystem_Repository
Client->>API_GW_Controller: GET /ecosystem/all-ecosystem?orgId=<orgId>
API_GW_Controller->>API_GW_Service: getEcosystems(userId, orgId)
API_GW_Service->>NATS: sendNatsMessage('get-ecosystems', { userId, orgId })
NATS->>Eco_Service: deliver get-ecosystems payload
alt orgId provided
Eco_Service->>Eco_Repo: getAllEcosystemsByOrgId(orgId)
Eco_Repo-->>Eco_Service: ecosystems[]
else orgId not provided
Eco_Service->>Eco_Repo: fetch user-scoped ecosystems
Eco_Repo-->>Eco_Service: ecosystems[]
end
Eco_Service-->>NATS: response payload
NATS-->>API_GW_Service: response
API_GW_Service-->>API_GW_Controller: ecosystems
API_GW_Controller-->>Client: 200 OK + ecosystems
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
pranalidhanavade
changed the title
pranalidhanavade
force-pushed
the
refactor/api-to-get-all-ecosystem-by-orgid
branch
from
6db9ce6 to
8712d4f
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/user/interfaces/user.interface.ts (1)
3-12:⚠️ Potential issue | 🔴 Critical
The Prisma schema defines
String?(optional/nullable), meaning existing database records can haveemail: null. With this interface change, methods likegetUserById(),getProfile(), andgetPublicProfile()will return data where the TypeScript type declaresemail: stringbut the actual value may benull, breaking type safety. Either make
🤖 Fix all issues with AI agents
In `@apps/api-gateway/src/ecosystem/ecosystem.controller.ts`:
- Around line 200-213: The ParseUUIDPipe on the orgId query in getEcosystems
currently rejects undefined, making the param effectively required; update the
ParseUUIDPipe instantiation to include optional: true and mark the route
parameter as optional (orgId?: string) so missing orgId won't trigger
validation, i.e., change the ParseUUIDPipe options to { optional: true,
exceptionFactory: ... } and update the orgId parameter signature in
getEcosystems to be optional.
In `@apps/ecosystem/repositories/ecosystem.repository.ts`:
- Around line 438-444: The orgId null/undefined check must run before any
database access: move the existing "if (!orgId) { throw new
NotFoundException(ResponseMessages.ecosystem.error.notFound); }" guard to the
top of the repository method (before the Prisma queries that reference orgId) or
remove it entirely if you rely on the service-level getEcosystemDashboard
validation; ensure the repository does not execute the Prisma query with a falsy
orgId and keep the subsequent ecosystem null check (throwing
ResponseMessages.ecosystem.error.ecosystemNotFound) intact.
🧹 Nitpick comments (4)
apps/ecosystem/repositories/ecosystem.repository.ts (1)
1-1: File-leveleslint-disablefor@typescript-eslint/no-explicit-anyis overly broad.This suppresses the rule for the entire file. Prefer inline
// eslint-disable-next-lineon the specific lines whereanyis needed (lines 168, 894, 1295).apps/ecosystem/src/ecosystem.service.ts (3)
329-338:userEmailis derived before theusernull check — reorder for clarity.Line 332 accesses
user?.emailbefore line 334 checksif (!user). While this isn't a runtime bug (the throw prevents further use), the logical flow is misleading. DeriveuserEmailafter the null guard.Proposed fix
const user = await this.ecosystemRepository.getUserById(reqUser); - const userEmail = user?.email || ''; if (!user) { throw new RpcException({ status: HttpStatus.NOT_FOUND, message: 'User not found to send invitation' }); } + const userEmail = user.email || ''; + const existingInvitation = await this.ecosystemRepository.getEcosystemInvitationsByEmail(userEmail, ecosystemId);
356-381: Empty-string fallback foruserIdcan cause a silent incorrect query.Line 358 sets
const userId = result.userId || ''. Line 367 checksif (!result.userId)and throws, but line 381 uses theuserIdvariable (which would be''ifresult.userIdis null) ingetEcosystemOrg. If the throw on line 368 were ever removed or bypassed,getEcosystemOrgwould query with an empty string. Useresult.userIddirectly after the guard instead.Proposed fix
- const userId = result.userId || ''; if (!role) { throw new Error('Error fetching ecosystem role'); } if (!result.invitedOrg) { throw new Error('Organization ID is missing'); } if (!result.userId) { throw new Error('User ID missing'); } - const existingOrg = await this.ecosystemRepository.getEcosystemOrg(result.ecosystemId, userId); + const existingOrg = await this.ecosystemRepository.getEcosystemOrg(result.ecosystemId, result.userId);
194-196: Dead code:userIdhas already been used above.
userIdis destructured and used on lines 181 and 183. By the time line 194 is reached,userIdhas already been passed tofindAcceptedInvitationByUserId. This guard is unreachable in practice (ifuserIdwere falsy, the repository call would have already failed or returned no result).Proposed fix — move the check to the top
async createEcosystem(createEcosystemDto: ICreateEcosystem): Promise<IEcosystem> { try { const { userId } = createEcosystemDto; + if (!userId) { + throw new BadRequestException(ResponseMessages.ecosystem.error.userIdMissing); + } const invitation = await this.ecosystemRepository.findAcceptedInvitationByUserId(userId); ... - if (!userId) { - throw new BadRequestException(ResponseMessages.ecosystem.error.userIdMissing); - }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/api-gateway/src/ecosystem/ecosystem.controller.ts`:
- Around line 91-101: The controller currently uses the wrong exception filter
which maps all RpcException errors to 400; update the controller to preserve
upstream status codes by either replacing the import of CustomExceptionFilter
with the HTTP-level one from apps/api-gateway/common/exception-handler (so the
filter used by ecosystem.controller preserves RpcException status codes) or wrap
the call to this.ecosystemService.inviteMemberToEcosystem(...) in a try/catch
that catches RpcException, extracts the status (or maps RpcException.getError()
metadata) and rethrows an HttpException with the original status (e.g.,
HttpStatus.CONFLICT for duplicate invites) before returning the created
response; reference the RpcException type, the inviteMemberToEcosystem call, and
the CustomExceptionFilter import to locate the change.
🧹 Nitpick comments (3)
apps/ecosystem/repositories/ecosystem.repository.ts (2)
1-1: File-leveleslint-disableforno-explicit-anyis overly broad.This suppresses the rule across the entire file. Prefer inline
// eslint-disable-next-lineon the specific lines that need it (lines 168, 894, 1295) to keep the rest of the file guarded.
1256-1299: NewgetAllEcosystemsByOrgIdlargely duplicates the query shape fromgetEcosystemsForEcosystemLead(lines 1217-1254) andgetAllEcosystems(lines 276-305).All three methods query
ecosystemwithdeletedAt: null, includeecosystemOrgs → ecosystemRole + organisation, and order bycreateDateTime desc. Consider extracting a shared private helper that accepts an optionalwherefilter forecosystemOrgsto reduce duplication across these methods.Also, the
!orgIdguard on line 1258 is a defensive no-op when the controller already guarantees a valid UUID viaParseUUIDPipe({ optional: true }). It won't hurt, but be aware that TypeScript'sstringtype won't be falsy after pipe validation — onlyundefinedwould skip the call at the service layer.#!/bin/bash # Check how getAllEcosystemsByOrgId is called from the service layer rg -n --type=ts -C3 'getAllEcosystemsByOrgId'apps/api-gateway/src/ecosystem/ecosystem.controller.ts (1)
203-211: Inconsistent error message keys fororgIdvalidation across endpoints.
getEcosystems(line 209) usesResponseMessages.ecosystem.error.invalidOrgId, whilegetEcosystemDashboard(line 259) usesResponseMessages.organisation.error.invalidOrgIdfor the same type of parameter. Pick one and use it consistently to avoid confusing API consumers with different error messages for the same kind of invalid input.Also applies to: 254-261
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ecosystem/src/ecosystem.service.ts (1)
442-448:⚠️ Potential issue | 🟠 MajorUpdate return types to reflect nullable repository returns.
getUserByKeycloakId(line 442) declaresPromise<user>andgetEcosystemDetailsByUserId(line 446) declaresPromise<ecosystem>, but the corresponding repository methods returnPromise<user | null>andPromise<ecosystem | null>. While current callers inapps/api-gateway/src/authz/jwt.strategy.tsproperly handle null returns with optional chaining checks (if (user?.id)andif (ecosystem?.id)), the service method signatures should be corrected to match the repository contract for type safety.Proposed fix
- async getUserByKeycloakId(keycloakId: string): Promise<user> { + async getUserByKeycloakId(keycloakId: string): Promise<user | null> { return this.ecosystemRepository.getUserByKeycloakId(keycloakId); } - async getEcosystemDetailsByUserId(userId: string): Promise<ecosystem> { + async getEcosystemDetailsByUserId(userId: string): Promise<ecosystem | null> { return this.ecosystemRepository.getEcosystemDetailsByUserId(userId); }
🤖 Fix all issues with AI agents
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 329-339: The code currently falls back to an empty string for a
missing user.email which can cause incorrect lookups in
ecosystemRepository.getEcosystemInvitationsByEmail and
ecosystemRepository.updateEcosystemInvitationStatusByEmail (also present in
inviteMemberToEcosystem); instead, after retrieving the user via
ecosystemRepository.getUserById, validate that user.email is present and
non-empty and if not throw an RpcException (e.g., HttpStatus.BAD_REQUEST or
NOT_FOUND with a clear message like "user email missing for invitation") before
calling getEcosystemInvitationsByEmail or updateEcosystemInvitationStatusByEmail
so no empty-string email is passed into those repository methods.
- Around line 181-196: The userId presence check is dead because userId is
destructured from createEcosystemDto and used by findAcceptedInvitationByUserId
before the guard; move the BadRequestException guard that checks userIdMissing
to run immediately after destructuring (i.e., right after const { userId } =
createEcosystemDto) or remove the guard entirely if the repository method
already validates/handles missing userId; update the control flow around
createEcosystemDto/userId, findAcceptedInvitationByUserId, and
checkEcosystemNameExist accordingly to ensure no repository calls are made with
a missing userId.
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 203-213: TrimStringParamPipe currently calls value.trim() and will
throw when Nest passes undefined for an optional query, breaking the optional
ParseUUIDPipe for orgId; update TrimStringParamPipe.transform() in libs/common
(cast.helper.ts) to first guard for null/undefined and simply return the
original value (undefined) when absent, or alternatively remove
TrimStringParamPipe from the orgId decorator chain where ParseUUIDPipe is
configured as optional; ensure the transform() signature still accepts undefined
and does not call trim() unless value is a non-empty string.
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 293-300: In getEcosystems, before calling
ecosystemRepository.getAllEcosystemsByOrgId(orgId), verify the user is a member
of that org (unless they have PLATFORM_ADMIN); add a membership check such as
this.organizationService.isUserMember(orgId, userId) or
this.orgRepository.isMember(orgId, userId) and if it returns false throw a
ForbiddenException (or ResponseMessages.ecosystem.error.notMember); ensure the
check runs only when orgId is provided and short-circuit to allow PLATFORM_ADMIN
users to bypass membership validation.
🧹 Nitpick comments (2)
apps/api-gateway/src/ecosystem/ecosystem.controller.ts (1)
678-686: Minor inconsistency:TrimStringParamPipeused on some path params but not others.
getEcosystemDashboard(lines 244-263) appliesTrimStringParamPipebeforeParseUUIDPipeon path params, butcreateIntentand other endpoints (getIntents,updateIntent,deleteIntent) do not. Consider standardizing for consistency.apps/ecosystem/src/ecosystem.service.ts (1)
365-390:userIdfallback to''on line 367 is unnecessary given the guard on lines 376-378.Line 367 assigns
const userId = result.userId || '', but lines 376-378 throw if!result.userId, souserIdcan never be''at line 390. The fallback masks the intent — prefer assigning after the guard or removing the fallback.Proposed fix
- const userId = result.userId || ''; if (!role) { throw new Error('Error fetching ecosystem role'); } if (!result.invitedOrg) { throw new Error(ResponseMessages.ecosystem.error.orgIdMissing); } if (!result.userId) { throw new Error(ResponseMessages.ecosystem.error.userIdMissing); } + const userId = result.userId; + const ecosystemOrgPayload = {
…arams 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>
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>
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
pranalidhanavade
force-pushed
the
refactor/api-to-get-all-ecosystem-by-orgid
branch
from
43cde2f to
44bd506
Compare
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
|
3 participants
What
IS_ECOSYSTEM_ENABLED.env variable dependency from user and organization service.Summary by CodeRabbit
New Features
Bug Fixes / Validation
API Changes
Chores