Refactor/api cleanup#2289
Conversation
…le to RESTful resources
… parameterizable guards using route metadata
Greptile SummaryThis PR consolidates and renames backend API endpoints to follow REST conventions — plural resource nouns, path parameters for resource IDs, and unified auth guards that use reflector metadata instead of separate per-permission guard classes.
Confidence Score: 3/5The auth guard consolidation and endpoint renames are well-executed, but merging OldProjectController into ProjectController places static routes (recent, default-rights) after a parameterized @get(':uuid') route, which will silently break those two endpoints in the Express router. The GET /projects/recent and GET /projects/default-rights routes are now unreachable because @get(':uuid') is declared first and Express matches in registration order. The developer's own comment acknowledges the conflict. The frontend has been migrated to call those new paths, so the regression is user-visible. Additionally, the POST /missions/:uuid/metadata body is untyped, meaning non-object values pass through to Object.entries in the service layer. backend/src/endpoints/project/project.controller.ts needs the recent and default-rights handler methods moved above @get(':uuid'); backend/src/endpoints/mission/mission.controller.ts needs a validated DTO for the addTags body.
|
| Filename | Overview |
|---|---|
| backend/src/endpoints/project/project.controller.ts | Merges OldProjectController into ProjectController, but @get(':uuid') declared before @get('recent') and @get('default-rights'), risking route shadowing for those two GET endpoints. |
| backend/src/endpoints/mission/mission.controller.ts | Route restructured to REST conventions. Static routes (filteredMinimal, filtered) declared before :uuid, which is correct. Body for addTags is untyped (no DTO class-validator). |
| backend/src/endpoints/auth/guards/project.guards.ts | Six separate guard classes collapsed into single ProjectAccessGuard; API-key branches preserved correctly per access level. |
| backend/src/endpoints/auth/guards/mission.guards.ts | Multiple guards consolidated into MissionAccessGuard with Reflector; logic looks correct. DeleteTagGuard intentionally downgrades DELETE right to WRITE for canTagMission. |
| backend/src/endpoints/auth/guards/file.guards.ts | DeleteFileGuard, ReadFileGuard, WriteFileGuard merged into FileAccessGuard using Reflector; clean consolidation with correct fallback to READ. |
| backend/src/endpoints/auth/roles.decorator.ts | Decorator factory functions updated to use the new unified guards; logic is correct and consistent. |
| backend/src/services/tag.service.ts | addTags now awaits Promise.all before returning { success: true }, fixing the prior incorrect return type mismatch. |
| packages/api-dto/src/types/tags/add-tags.dto.ts | AddTagsDto and AddTagDto now have proper swagger/validator decorators; replaces empty stub classes. |
| frontend/src/services/mutations/mission.ts | All mutation calls updated to new REST endpoint paths; consistent with backend changes. |
| cli/kleinkram/api/routes.py | CLI route constants and _update_mission function updated to new endpoint paths. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request] --> B{HTTP Method + Path}
B -->|GET /projects/:uuid| C[ProjectAccessGuard - READ right]
B -->|GET /projects/recent| D{Declared after :uuid?}
B -->|GET /projects/default-rights| E{Declared after :uuid?}
D -->|Yes - Express matches :uuid first| C
E -->|Yes - Express matches :uuid first| C
C --> F{Project found by UUID?}
F -->|uuid=recent - DB fail| G[401/404 error]
F -->|valid UUID| H[getProjectById]
B -->|PATCH /missions/:uuid/name| J[MissionAccessGuard - WRITE right]
B -->|POST /missions/:uuid/metadata| K[MissionAccessGuard - WRITE right]
B -->|DELETE /missions/:uuid| L[MissionAccessGuard - DELETE right]
B -->|DELETE /metadata/:uuid| M[DeleteTagGuard - DELETE to WRITE downgrade]
J --> O[MissionService.updateName]
K --> P[TagService.addTags]
L --> Q[MissionService.deleteMission]
M --> R[TagService.deleteTag]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Incoming Request] --> B{HTTP Method + Path}
B -->|GET /projects/:uuid| C[ProjectAccessGuard - READ right]
B -->|GET /projects/recent| D{Declared after :uuid?}
B -->|GET /projects/default-rights| E{Declared after :uuid?}
D -->|Yes - Express matches :uuid first| C
E -->|Yes - Express matches :uuid first| C
C --> F{Project found by UUID?}
F -->|uuid=recent - DB fail| G[401/404 error]
F -->|valid UUID| H[getProjectById]
B -->|PATCH /missions/:uuid/name| J[MissionAccessGuard - WRITE right]
B -->|POST /missions/:uuid/metadata| K[MissionAccessGuard - WRITE right]
B -->|DELETE /missions/:uuid| L[MissionAccessGuard - DELETE right]
B -->|DELETE /metadata/:uuid| M[DeleteTagGuard - DELETE to WRITE downgrade]
J --> O[MissionService.updateName]
K --> P[TagService.addTags]
L --> Q[MissionService.deleteMission]
M --> R[TagService.deleteTag]
Comments Outside Diff (1)
-
backend/src/endpoints/project/project.controller.ts, line 65-76 (link)Route shadowing:
GET /projects/recentandGET /projects/default-rightsare unreachableNestJS with the Express adapter registers routes in declaration order; Express then matches them in that order. Because
@Get(':uuid')is declared at line 66, requests toGET /projects/recentandGET /projects/default-rightsare intercepted by the parameterized route first (withuuid = 'recent'/uuid = 'default-rights'). The@CanReadProject()guard runs and tries a DB lookup for a project with UUID'recent', which fails, returning a 4xx rather than delegating togetRecentProjects()orgetDefaultRights(). The comment on line 65 acknowledges this but leaves it unresolved. Both the frontend (/projects/recent) and (/projects/default-rights) will silently break after this migration fromOldProjectController.The fix is to move
@Get('recent')and@Get('default-rights')above@Get(':uuid')in the class.Prompt To Fix With AI
This is a comment left during a code review. Path: backend/src/endpoints/project/project.controller.ts Line: 65-76 Comment: **Route shadowing: `GET /projects/recent` and `GET /projects/default-rights` are unreachable** NestJS with the Express adapter registers routes in **declaration order**; Express then matches them in that order. Because `@Get(':uuid')` is declared at line 66, requests to `GET /projects/recent` and `GET /projects/default-rights` are intercepted by the parameterized route first (with `uuid = 'recent'` / `uuid = 'default-rights'`). The `@CanReadProject()` guard runs and tries a DB lookup for a project with UUID `'recent'`, which fails, returning a 4xx rather than delegating to `getRecentProjects()` or `getDefaultRights()`. The comment on line 65 acknowledges this but leaves it unresolved. Both the frontend (`/projects/recent`) and (`/projects/default-rights`) will silently break after this migration from `OldProjectController`. The fix is to move `@Get('recent')` and `@Get('default-rights')` above `@Get(':uuid')` in the class. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
backend/src/endpoints/project/project.controller.ts:65-76
**Route shadowing: `GET /projects/recent` and `GET /projects/default-rights` are unreachable**
NestJS with the Express adapter registers routes in **declaration order**; Express then matches them in that order. Because `@Get(':uuid')` is declared at line 66, requests to `GET /projects/recent` and `GET /projects/default-rights` are intercepted by the parameterized route first (with `uuid = 'recent'` / `uuid = 'default-rights'`). The `@CanReadProject()` guard runs and tries a DB lookup for a project with UUID `'recent'`, which fails, returning a 4xx rather than delegating to `getRecentProjects()` or `getDefaultRights()`. The comment on line 65 acknowledges this but leaves it unresolved. Both the frontend (`/projects/recent`) and (`/projects/default-rights`) will silently break after this migration from `OldProjectController`.
The fix is to move `@Get('recent')` and `@Get('default-rights')` above `@Get(':uuid')` in the class.
### Issue 2 of 3
backend/src/endpoints/mission/mission.controller.ts:221-242
**Unvalidated body shape allows non-object input to reach service layer**
The `body` parameter is typed as `{ metadata?: Record<string, string>; tags?: Record<string, string> }` but uses a plain `@Body()` without a DTO class, so class-validator never runs. If a caller sends `{ "metadata": "not-an-object" }`, `body.metadata` resolves to the string `"not-an-object"` and `Object.entries("not-an-object")` in `tag.service.ts` will iterate over character indices instead of throwing. Creating a validated DTO class (or at minimum adding a runtime `typeof` check before calling `tagService.addTags`) would prevent this.
### Issue 3 of 3
backend/src/endpoints/project/project.controller.ts:161-173
**UUID format validation removed from `addTagType` and `updateTagTypes`**
The old `@QueryUUID('tagTypeUUID', ...)` and `@BodyUUIDArray('tagTypeUUIDs', ...)` decorators validated that the supplied values were well-formed UUIDs before reaching the service. The new `@Query()` and `@Body()` calls perform no such validation, so arbitrary strings will be passed to the DB query, returning misleading errors instead of a clean 400. The same pattern appears in `updateTagTypes` (lines 195-209). Restoring UUID validation via the original query/body decorators or by introducing a DTO with `@IsUUID()` would keep the behaviour consistent with the rest of the codebase.
Reviews (1): Last reviewed commit: "refactor(backend): consolidate project, ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @Post(':uuid/metadata') | ||
| @CanAddTag() | ||
| @ApiOkResponse({ | ||
| description: 'Metadata added to mission', | ||
| type: AddTagsDto, | ||
| }) | ||
| async addTags( | ||
| @ParameterUID('uuid') uuid: string, | ||
| @Body() | ||
| body: { | ||
| metadata?: Record<string, string>; | ||
| tags?: Record<string, string>; | ||
| }, | ||
| ): Promise<AddTagsDto> { | ||
| const metadata = body.metadata ?? body.tags; | ||
| if (!metadata) { | ||
| throw new BadRequestException( | ||
| 'metadata or tags object is required', | ||
| ); | ||
| } | ||
| return this.tagService.addTags(uuid, metadata); | ||
| } |
There was a problem hiding this comment.
The body parameter is typed as { metadata?: Record<string, string>; tags?: Record<string, string> } but uses a plain @Body() without a DTO class, so class-validator never runs. If a caller sends { "metadata": "not-an-object" }, body.metadata resolves to the string "not-an-object" and Object.entries("not-an-object") in tag.service.ts will iterate over character indices instead of throwing. Creating a validated DTO class (or at minimum adding a runtime typeof check before calling tagService.addTags) would prevent this.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/endpoints/mission/mission.controller.ts
Line: 221-242
Comment:
**Unvalidated body shape allows non-object input to reach service layer**
The `body` parameter is typed as `{ metadata?: Record<string, string>; tags?: Record<string, string> }` but uses a plain `@Body()` without a DTO class, so class-validator never runs. If a caller sends `{ "metadata": "not-an-object" }`, `body.metadata` resolves to the string `"not-an-object"` and `Object.entries("not-an-object")` in `tag.service.ts` will iterate over character indices instead of throwing. Creating a validated DTO class (or at minimum adding a runtime `typeof` check before calling `tagService.addTags`) would prevent this.
How can I resolve this? If you propose a fix, please make it concise.| async addTagType( | ||
| @ParameterUID('uuid') uuid: string, | ||
| @QueryUUID('tagTypeUUID', 'TagType UUID') tagTypeUUID: string, | ||
| @Query() query: { tagTypeUUID?: string; metadataTypeUUID?: string }, | ||
| ): Promise<AddTagTypeDto> { | ||
| await this.projectService.addTagType(uuid, tagTypeUUID); | ||
| const typeUuid = query.metadataTypeUUID ?? query.tagTypeUUID; | ||
| if (!typeUuid) { | ||
| throw new BadRequestException( | ||
| 'metadataTypeUUID or tagTypeUUID query param is required', | ||
| ); | ||
| } | ||
| await this.projectService.addTagType(uuid, typeUuid); | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
addTagType and updateTagTypes
The old @QueryUUID('tagTypeUUID', ...) and @BodyUUIDArray('tagTypeUUIDs', ...) decorators validated that the supplied values were well-formed UUIDs before reaching the service. The new @Query() and @Body() calls perform no such validation, so arbitrary strings will be passed to the DB query, returning misleading errors instead of a clean 400. The same pattern appears in updateTagTypes (lines 195-209). Restoring UUID validation via the original query/body decorators or by introducing a DTO with @IsUUID() would keep the behaviour consistent with the rest of the codebase.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/endpoints/project/project.controller.ts
Line: 161-173
Comment:
**UUID format validation removed from `addTagType` and `updateTagTypes`**
The old `@QueryUUID('tagTypeUUID', ...)` and `@BodyUUIDArray('tagTypeUUIDs', ...)` decorators validated that the supplied values were well-formed UUIDs before reaching the service. The new `@Query()` and `@Body()` calls perform no such validation, so arbitrary strings will be passed to the DB query, returning misleading errors instead of a clean 400. The same pattern appears in `updateTagTypes` (lines 195-209). Restoring UUID validation via the original query/body decorators or by introducing a DTO with `@IsUUID()` would keep the behaviour consistent with the rest of the codebase.
How can I resolve this? If you propose a fix, please make it concise.…order bug, SQL wildcards, serialization typo, and tests
Split FileService into FileQueryService, FileStorageService, and FileLifecycleService. Split AccessService into AccessQueryService and AccessModificationService. Retained FileService and AccessService as facades for backward compatibility.
|
Too many files changed for review. ( |
Updated FileController, ActionsController, ProjectController, and AccessController to directly inject and call the specialized sub-services (FileQueryService, FileStorageService, FileLifecycleService, AccessQueryService, AccessModificationService) rather than delegating through the monolithic FileService and AccessService. Deleted FileService and AccessService facade classes.
…d S3 package production crash, and enforce group access security
…dpoints/ directory and cwd
1 participant
No description provided.