feat: use walletAddress as the primary public ID#140
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughMigrates User.id from integer to UUID across DB, entities, services, and DTOs; shifts public APIs to use walletAddress as the primary identifier. Adds TypeORM migrations for users and related foreign keys, updates JWT validation, adjusts controllers/services for users, auth, reviews, and buyer-requests, adds tests, and documents the migration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as UsersController
participant USvc as UserService
participant DB
Client->>API: PUT /users/update/:walletAddress
API->>USvc: updateUser(walletAddress, dto)
USvc->>USvc: getUserByWalletAddress(walletAddress)
USvc->>DB: SELECT user WHERE walletAddress=...
DB-->>USvc: User(id: UUID)
USvc->>DB: UPDATE user SET ...
DB-->>USvc: Updated User
USvc-->>API: Updated User
API-->>Client: { walletAddress, name, email, role, updatedAt } (no id)
sequenceDiagram
participant Client
participant Auth as JwtStrategy.validate
participant ASvc as AuthService
participant DB
Client->>Auth: JWT payload
alt payload.walletAddress
Auth->>ASvc: getUserByWalletAddress(walletAddress)
else payload.id
Auth->>ASvc: getUserById(id: UUID)
end
ASvc->>DB: SELECT user + roles
DB-->>ASvc: User
ASvc-->>Auth: User
Auth-->>Client: Authorized (user.id UUID, walletAddress)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (6)
src/modules/auth/controllers/role.controller.ts (1)
22-22: MigrategetMyRolesendpoint to usewalletAddress
The controller is still passingreq.user.idintogetUserRoles, and the service signature only supports a numeric ID. To align with the public-API strategy, switch both controller and service to use the user’swalletAddress.• In src/modules/auth/controllers/role.controller.ts (line 22):
- Replace
const roles = await this.roleService.getUserRoles(req.user.id);- With
const roles = await this.roleService.getUserRoles(req.user.walletAddress);- Ensure
req.user.walletAddressis populated and typed on the request user object.• In src/modules/auth/services/role.service.ts:
- Update the
getUserRolessignature toasync getUserRoles(walletAddress: string): Promise<Role[]> { … }- Change the repository query to filter by wallet address (e.g. join on user → walletAddress), or introduce a new method
getUserRolesByWalletAddress.- Apply the same parameter and query changes to
hasRoleandhasAnyRole.--- a/src/modules/auth/controllers/role.controller.ts +++ b/src/modules/auth/controllers/role.controller.ts @@ -22,7 +22,7 @@ export class RoleController { - const roles = await this.roleService.getUserRoles(req.user.id); + const roles = await this.roleService.getUserRoles(req.user.walletAddress);src/modules/reviews/services/review.service.ts (2)
56-60: Handle unique-constraint race for duplicate reviews.There’s a TOCTOU between the existence check and save. Under concurrency, the DB unique constraint (
@Unique(['userId','productId'])) will be the source of truth. Map Postgres unique violations (23505) to a friendly error.Apply this diff:
- try { - return await this.repository.save(review); - } catch (error) { - throw new BadRequestError(`Failed to create review: ${error.message}`); - } + try { + return await this.repository.save(review); + } catch (error: any) { + // Postgres unique_violation + if (error?.code === '23505') { + throw new BadRequestError('You have already reviewed this product'); + } + throw new BadRequestError(`Failed to create review: ${error.message || 'Unknown error'}`); + }
15-18: Unify AppDataSource import in ReviewService
ReviewService is currently pulling in the default export fromconfig/ormconfig.ts, while most other services (e.g. UserService, RoleService, JwtAuthMiddleware) use the named export fromconfig/database.ts. This mismatch spins up two differentDataSourceinstances and can lead to config drift.Please update:
• File:
src/modules/reviews/services/review.service.ts
• Change the import on line 3:- import AppDataSource from '../../../config/ormconfig'; + import { AppDataSource } from '../../../config/database';After this change, all modules will reference the same
AppDataSourcedefinition.src/modules/auth/services/auth.service.ts (3)
179-192: Avoid duplicating update logic; delegate to UserService.updateUser to retain email-uniqueness validation.Currently this path updates without the uniqueness check present in
UserService.updateUser, risking duplicate emails. Delegate the update here.Apply this diff:
- async updateUser(walletAddress: string, updateData: { name?: string; email?: string }): Promise<User> { - const user = await this.userRepository.findOne({ where: { walletAddress } }); - - if (!user) { - throw new BadRequestError('User not found'); - } - - // Update user data - Object.assign(user, updateData); - await this.userRepository.save(user); - - return this.getUserByWalletAddress(walletAddress); - } + async updateUser(walletAddress: string, updateData: { name?: string; email?: string }): Promise<User> { + // Reuse centralized validation (email uniqueness, etc.) + return this.userService.updateUser(walletAddress, updateData); + }
45-55: Dev-only signature bypass is risky; gate via explicit feature flag.A hardcoded bypass token in development increases the risk of accidental enablement. Gate this behind a dedicated config flag (e.g.,
AUTH_BYPASS_SIGNATURE=true) and log a clear warning, or restrict it to test environments only.Follow-up (outside selected lines):
if (process.env.AUTH_BYPASS_SIGNATURE === 'true' && signature === process.env.TEST_SIGNATURE) { console.warn('AUTH_BYPASS_SIGNATURE is enabled; skipping signature verification'); return true; }
151-158: Consolidate JWT signing via JwtServiceThis service currently mixes direct
jsonwebtoken.signcalls withthis.jwtService.sign. For consistency and easier testing, replace all directsign(…)invocations withthis.jwtService.sign(…)and remove theimport { sign } from 'jsonwebtoken'.Occurrences to update:
- src/modules/auth/services/auth.service.ts
• Lines 91–93 (after updating a user)
• Lines 124–126 (after saving a new user)
• Lines 153–155 (on login)Example replacement:
- const token = sign({ id: user.id, walletAddress: user.walletAddress, role }, config.jwtSecret, { - expiresIn: '1h', - }); + const token = this.jwtService.sign( + { id: user.id, walletAddress: user.walletAddress, role }, + { expiresIn: '1h' } + );Also ensure your
JwtModule(or equivalent) is configured with the secret and default expiration so you only need to pass payload and optional overrides here. Finally, remove the unusedimport { sign } from 'jsonwebtoken'.
🧹 Nitpick comments (19)
src/migrations/1751199237000-MigrateUserIdToUUID.ts (2)
22-22: Use IF NOT EXISTS is unnecessary for UNIQUE INDEXThe
CREATE UNIQUE INDEXstatement usesIF NOT EXISTSbut this is a new migration adding the index for the first time.Simplify the statement:
- await queryRunner.query(`CREATE UNIQUE INDEX IF NOT EXISTS "IDX_users_walletAddress" ON "users" ("walletAddress")`); + await queryRunner.query(`CREATE UNIQUE INDEX "IDX_users_walletAddress" ON "users" ("walletAddress")`);
24-26: Missing foreign key updates in this migrationThe comment indicates that related tables will be updated separately, but this creates a risk of running migrations out of order.
Consider combining this migration with the foreign key updates in a single transaction, or add explicit checks to ensure migrations run in the correct order. You could also use TypeORM's migration transaction support to ensure atomicity.
src/modules/auth/strategies/jwt.strategy.ts (1)
34-35: Consider adding deprecation logging for id-based authenticationSince the fallback to
payload.idis for backward compatibility during migration, consider adding a deprecation warning to track usage and plan for its eventual removal.} else if (payload.id) { // Fallback to id for backward compatibility during migration + console.warn('JWT validation using legacy id field. This will be deprecated in future versions.'); user = await this.authService.getUserById(payload.id);src/modules/reviews/entities/review.entity.ts (1)
19-20: UUID FK change for userId looks good; consider aligning cascade behavior.The switch to
type: 'uuid'on userId is correct and consistent with the migration. To avoid mismatches with DB constraints and to ensure cleanup of dependent rows, consider addingonDelete: 'CASCADE'to the relation.Follow-up change (outside selected lines):
// around Lines 22-24 @ManyToOne(() => User, { onDelete: 'CASCADE' }) @JoinColumn({ name: 'userId' }) user: User;Please verify that the corresponding FK in the migration for
reviews.userIdalso usesON DELETE CASCADEso entity metadata and DB constraints stay in sync.src/modules/reviews/services/review.service.ts (1)
35-39: User existence verification aligns with new ID semantics.Catching and rethrowing a NotFound here keeps the outward contract clean.
You can avoid the try/catch by having
userService.getUserByIditself throw aNotFoundErrorand letting it bubble up unchanged, keeping error mapping centralized.src/modules/auth/services/role.service.ts (2)
67-69: hasRole signature alignment and logic look good.Small follow-up: ensure Role.name is constrained to RoleName at the entity/type level to avoid widening to arbitrary strings.
72-75: hasAnyRole signature alignment looks good.No issues; consider normalizing case if you ever introduce case-insensitive role names.
src/modules/users/services/user.service.ts (1)
46-55: getUserByWalletAddress aligns with wallet-first design; consider 404 semantics.Implementation is correct and loads roles. Consider throwing a 404-specific error type (e.g., NotFoundError) for absent users to match REST semantics; several other modules use NotFoundError.
src/modules/users/tests/user-update-api.spec.ts (3)
79-95: Response shape assertion is good; ensure controller strips internal IDs.Test covers walletAddress as the primary identifier and role derivation. Consider also asserting that the response omits
idhere (as you do later for GET) to enforce consistency across endpoints.
170-195: GET profile test is aligned with wallet-first approach and hides internal id.Good assertion set. Consider also verifying that roles are derived from
userRolesconsistently.
260-271: Brittle test: introspecting method source to assert parameter names.Using
toString()on functions to check parameter names is not robust and may break under minification or transpilation. Prefer validating:
- Route metadata (path template includes ':walletAddress'), or
- Controller behavior by calling the method with a walletAddress and asserting service calls (already covered).
Recommendation: remove this test or replace it with metadata inspection using Nest’s
Reflectoror by spinning up the module and checking the route definitions.src/modules/auth/services/auth.service.ts (3)
193-202: Deduplicate getUserByWalletAddress with UserService.This method duplicates
UserService.getUserByWalletAddress. Prefer delegating to reduce drift and keep relations consistent.Follow-up change (outside selected lines):
// Remove this method and replace callers with: return this.userService.getUserByWalletAddress(walletAddress);
247-271: assignRole by walletAddress is correct and consistent with UUID FKs.
- Good: Lookup user by walletAddress, delete prior roles, assign new one, and return via wallet-based lookup.
- Optional: Wrap delete+insert in a transaction to avoid transient gaps if another read happens in-between.
273-282: removeRole by walletAddress is correct.Consider a transaction if other related operations will be added later, but current usage is fine.
src/modules/users/controllers/user.controller.ts (5)
96-98: Route pattern inconsistency with REST conventionsThe update route pattern
/update/:walletAddressdeviates from standard RESTful conventions. Consider usingPUT /users/:walletAddressinstead.- * PUT /users/update/:walletAddress + * PUT /users/:walletAddress */ - @Put('update/:walletAddress') + @Put(':walletAddress')
139-145: Code duplication: Extract authorization logicThe authorization check logic is duplicated between
updateUserandgetUserByWalletAddress. Consider extracting this into a private method to follow DRY principles.Add a private method to handle authorization:
private checkUserAuthorization( walletAddress: string, currentUserWalletAddress: string | undefined, currentUserRole: string | undefined, errorMessage: string ): void { if (!currentUserWalletAddress) { throw new UnauthorizedError('User not authenticated'); } if (walletAddress.toLowerCase() !== currentUserWalletAddress.toLowerCase() && currentUserRole !== 'admin') { throw new UnauthorizedError(errorMessage); } }Then use it in both methods:
- const currentUserWalletAddress = req.user?.walletAddress; - const currentUserRole = req.user?.role?.[0]; - - // Check if user is accessing their own profile or is admin - if (walletAddress !== currentUserWalletAddress && currentUserRole !== 'admin') { - throw new UnauthorizedError('Access denied'); - } + this.checkUserAuthorization( + walletAddress, + req.user?.walletAddress, + req.user?.role?.[0], + 'Access denied' + );
129-131: Update documentation to reflect actual endpoint behaviorThe comment states "admin only or own profile" but the endpoint actually allows users to access their own profile OR admins to access any profile. The current wording could be misleading.
/** - * Get user by wallet address (admin only or own profile) + * Get user by wallet address (own profile or admin access to any profile) * GET /users/:walletAddress */
87-87: Consider extracting role resolution logicThe pattern
user.userRoles?.[0]?.role?.name || 'buyer'is repeated multiple times throughout the controller. Consider extracting this into a utility method to improve maintainability.Add a private method:
private getUserRole(user: any): string { return user.userRoles?.[0]?.role?.name || 'buyer'; }Then use it throughout:
- role: result.user.userRoles?.[0]?.role?.name || 'buyer', + role: this.getUserRole(result.user),Also applies to: 122-122, 155-155, 179-179
24-31: Consider adding JSDoc comments to interfacesThe interfaces would benefit from documentation to clarify their purpose and field meanings, especially for the optional fields.
+/** + * Response structure for user data + */ interface UserResponse { + /** User's wallet address (primary identifier) */ walletAddress: string; + /** User's display name */ name: string; + /** User's email address */ email: string; + /** User's role in the system */ role: string; + /** Timestamp of user creation */ createdAt?: Date; + /** Timestamp of last update */ updatedAt?: Date; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
docs/UUID-MIGRATION.md(1 hunks)src/migrations/1751199237000-MigrateUserIdToUUID.ts(1 hunks)src/migrations/1751199238000-UpdateForeignKeysToUUID.ts(1 hunks)src/modules/auth/controllers/role.controller.ts(1 hunks)src/modules/auth/entities/user-role.entity.ts(1 hunks)src/modules/auth/services/auth.service.ts(6 hunks)src/modules/auth/services/role.service.ts(1 hunks)src/modules/auth/strategies/jwt.strategy.ts(1 hunks)src/modules/buyer-requests/dto/buyer-request-response.dto.ts(1 hunks)src/modules/buyer-requests/entities/buyer-request.entity.ts(1 hunks)src/modules/buyer-requests/services/buyer-requests.service.ts(4 hunks)src/modules/reviews/controllers/review.controller.ts(2 hunks)src/modules/reviews/dto/review.dto.ts(1 hunks)src/modules/reviews/entities/review.entity.ts(1 hunks)src/modules/reviews/services/review.service.ts(3 hunks)src/modules/users/controllers/user.controller.ts(2 hunks)src/modules/users/entities/user.entity.ts(3 hunks)src/modules/users/services/user.service.ts(4 hunks)src/modules/users/tests/user-update-api.spec.ts(1 hunks)src/types/auth-request.type.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/modules/buyer-requests/services/buyer-requests.service.ts (1)
src/modules/buyer-requests/dto/buyer-request-response.dto.ts (1)
BuyerRequestResponseDto(3-22)
src/modules/users/services/user.service.ts (1)
src/utils/errors.ts (1)
BadRequestError(12-16)
src/modules/auth/services/auth.service.ts (2)
src/utils/errors.ts (1)
BadRequestError(12-16)src/modules/auth/types/role.types.ts (1)
RoleName(1-1)
src/modules/auth/services/role.service.ts (1)
src/modules/auth/types/role.types.ts (1)
RoleName(1-1)
src/modules/users/controllers/user.controller.ts (2)
src/modules/auth/dto/auth.dto.ts (1)
UpdateUserDto(54-70)src/utils/errors.ts (1)
UnauthorizedError(18-22)
🪛 markdownlint-cli2 (0.17.2)
docs/UUID-MIGRATION.md
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (32)
src/modules/reviews/dto/review.dto.ts (1)
10-10: Type change aligns with UUID migrationThe change from
numbertostringforuserIdcorrectly aligns with the system-wide UUID migration for user IDs.src/modules/buyer-requests/entities/buyer-request.entity.ts (1)
54-55: UUID column type correctly appliedThe change from numeric to UUID type for
userIdis properly implemented with the correct TypeORM column decorator.src/modules/buyer-requests/dto/buyer-request-response.dto.ts (1)
11-11: DTO types correctly updated for UUID migrationBoth
userIdand nesteduser.idfields are properly updated tostringtype, maintaining consistency with the entity changes.Also applies to: 16-16
src/types/auth-request.type.ts (1)
5-5: Auth types properly aligned with UUID migrationThe type changes from
string | numbertostringwith UUID comments properly document the migration and ensure type safety throughout the authentication flow.Also applies to: 16-16
src/modules/users/entities/user.entity.ts (2)
17-18: UUID primary key correctly implementedThe change from numeric auto-increment to UUID generation is properly implemented using TypeORM's built-in UUID generator.
26-28: Index on walletAddress improves query performanceAdding an index on
walletAddressis a good optimization since it's now being used as the primary public identifier for API operations.src/migrations/1751199238000-UpdateForeignKeysToUUID.ts (2)
19-47: Foreign key constraints properly configuredThe foreign key constraints with CASCADE DELETE are correctly added to maintain referential integrity.
8-14: Migration order verified—no further action required
The1751199237000-MigrateUserIdToUUIDruns before1751199238000-UpdateForeignKeysToUUIDas expected. PostgreSQL will fail the::uuidcasts ifusers.idisn’t already a UUID, so an extra pre-validation query isn’t necessary. Proceed as-is.src/modules/buyer-requests/services/buyer-requests.service.ts (5)
27-27: API signature changes look good for UUID migrationThe updated method signatures correctly accept
userIdas string, aligning with the UUID migration. The type changes are consistent across all affected methods.Also applies to: 138-138, 189-189, 289-289
149-151: String comparison for userId is appropriateThe ownership check correctly uses string comparison (
!==) which is appropriate for UUID strings.
199-201: Consistent ownership validationThe ownership check maintains consistency with other methods, using string comparison for UUID validation.
300-302: Ownership check properly handles UUID stringsThe string comparison for ownership validation is correctly implemented for the UUID-based userId.
269-270: Verify userId field presence in response DTOThe response includes
userId: buyerRequest.userIdwhich correctly passes the UUID string to the DTO.src/modules/reviews/controllers/review.controller.ts (3)
16-16: Direct assignment of userId maintains type consistencyRemoving the numeric coercion and directly assigning
req.user.idpreserves the UUID string type, which aligns with the UUID migration. This ensures type consistency throughout the authentication flow.
81-81: Consistent UUID handling in deleteReviewThe removal of numeric coercion maintains type consistency with the UUID migration across both create and delete operations.
32-32: Service calls correctly pass UUID string userIdBoth service method calls now receive the userId as a string type, matching the updated service signatures that expect UUID strings.
Also applies to: 91-91
src/modules/auth/entities/user-role.entity.ts (2)
10-11: UUID column type correctly configuredThe
userIdfield is properly configured withtype: 'uuid'and typed asstring, aligning with the UUID migration strategy.
16-18: Foreign key relationship maintained correctlyThe
@JoinColumndecorator correctly references 'userId' which will properly map to the UUID column in the database after migration.src/modules/auth/strategies/jwt.strategy.ts (2)
27-36: Well-structured dual authentication path for migrationThe implementation provides a clean migration path by:
- Preferring walletAddress-based authentication (the new primary identifier)
- Falling back to UUID-based authentication for backward compatibility
- Properly validating the token payload structure
This approach ensures zero-downtime migration while existing tokens remain valid.
42-46: Clear separation of internal and public identifiersThe returned user object properly maintains both identifiers with clear documentation about their purposes - UUID for internal use and walletAddress for public APIs.
src/modules/auth/controllers/role.controller.ts (1)
12-16: API correctly updated to use walletAddress as primary identifierThe endpoint now accepts
walletAddressinstead ofuserId, aligning with the architectural shift to use wallet addresses as the primary public identifier. The removal oftoString()conversions is appropriate since both parameters are already strings.src/modules/reviews/services/review.service.ts (1)
95-108: deleteReview signature update to string UUID is correct.The ownership check remains valid with string IDs.
src/modules/auth/services/role.service.ts (2)
55-57: removeRoleFromUser userId: string change is correct.Query shape now matches the updated UUID FK type.
61-63: Query by string userId is consistent with UUID migration.Looks good.
src/modules/users/services/user.service.ts (4)
57-61: getUserById switch to string id is correct.The repository query now matches UUIDs without numeric coercion.
72-88: updateUser via walletAddress is correct; email uniqueness guard is solid.
- Good: prevent email collisions by checking
existingUser.id !== user.id.- Ensure a unique index exists on
You can check for a unique index via migration files or DB schema. If not present, I can propose a migration.
90-93: getUserOrders by walletAddress is aligned.Loads orders via relation; consistent with wallet-first.
103-106: getUserWishlist by walletAddress is aligned.Loads wishlist via relation; consistent with wallet-first.
src/modules/users/tests/user-update-api.spec.ts (3)
114-128: Unauthorized update test is sound; confirm controller throws the same error type.You’re asserting
UnauthorizedError(custom). Verify the controller uses this class rather than Nest’sUnauthorizedExceptionto avoid brittle test coupling.I can scan the controller to confirm the error type used and adjust the test accordingly.
130-155: Partial update and email-only tests add valuable coverage.These assert field-level updates and retention of other fields correctly.
214-228: Unauthorized GET test is correct; ensure controller/guides enforce wallet-based ownership.This protects against cross-account reads. No additional issues.
src/modules/auth/services/auth.service.ts (1)
165-167: getUserById: switch to string UUID query is correct.This aligns with the UUID migration.
| ``` | ||
| PUT /users/update/:id | ||
| GET /users/:id | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add language identifiers to fenced code blocks (markdownlint MD040).
Specify a language for the HTTP route examples to satisfy markdownlint and improve rendering.
Apply this diff:
-```
+```http
PUT /users/update/:id
GET /users/:id- +http
PUT /users/update/:walletAddress
GET /users/:walletAddress
Run markdownlint to verify:
Also applies to: 38-41
🏁 Script executed:
#!/bin/bash
npx --yes markdownlint-cli2 "docs/**/*.md" || true
# Or quick grep for missing-language fences:
rg -n "^```$" docs || trueLength of output: 18567
🏁 Script executed:
#!/bin/bash
set -e
echo "Listing all fenced code blocks in docs/UUID-MIGRATION.md with line numbers:"
rg -n "```" docs/UUID-MIGRATION.md
echo
echo "Displaying lines 30 to 50 of docs/UUID-MIGRATION.md for context:"
sed -n '30,50p' docs/UUID-MIGRATION.mdLength of output: 286
🏁 Script executed:
#!/bin/bash
set -e
echo "Fenced code blocks in docs/UUID-MIGRATION.md (line numbers):"
rg -n "```" docs/UUID-MIGRATION.md
echo
echo "Showing lines 31–42 for context:"
sed -n '31,42p' docs/UUID-MIGRATION.mdLength of output: 256
Add HTTP language identifiers to code fences in docs/UUID-MIGRATION.md
To satisfy markdownlint MD040 and improve readability, the unnamed fenced blocks under “Before (using numeric ID)” and “After (using walletAddress)” need a language identifier.
• docs/UUID-MIGRATION.md, lines 32–35 (Before)
• docs/UUID-MIGRATION.md, lines 38–41 (After)
Apply this diff:
-```
+```http
PUT /users/update/:id
GET /users/:id
-```
+```
-```
+```http
PUT /users/update/:walletAddress
GET /users/:walletAddress
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/UUID-MIGRATION.md around lines 32–35 and 38–41, the fenced code blocks
under “Before (using numeric ID)” and “After (using walletAddress)” are missing
a language identifier and the second block should show walletAddress; update
both fences to start with ```http and end with ``` and change the routes in the
“After” block from /users/update/:id and /users/:id to
/users/update/:walletAddress and /users/:walletAddress respectively so the
examples are language-tagged and correct.
| await queryRunner.query(`ALTER TABLE "users" ADD COLUMN "id_new" UUID DEFAULT gen_random_uuid()`); | ||
|
|
||
| // Update existing records to have unique UUIDs | ||
| await queryRunner.query(`UPDATE "users" SET "id_new" = gen_random_uuid() WHERE "id_new" IS NULL`); |
There was a problem hiding this comment.
Redundant UUID generation in UPDATE statement
Line 11 attempts to update records where id_new IS NULL, but line 8 already sets a default value of gen_random_uuid() for the column. This UPDATE will never find any NULL values.
Remove the redundant UPDATE or modify the logic:
// First, add a new UUID column
- await queryRunner.query(`ALTER TABLE "users" ADD COLUMN "id_new" UUID DEFAULT gen_random_uuid()`);
+ await queryRunner.query(`ALTER TABLE "users" ADD COLUMN "id_new" UUID`);
// Update existing records to have unique UUIDs
- await queryRunner.query(`UPDATE "users" SET "id_new" = gen_random_uuid() WHERE "id_new" IS NULL`);
+ await queryRunner.query(`UPDATE "users" SET "id_new" = gen_random_uuid()`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await queryRunner.query(`ALTER TABLE "users" ADD COLUMN "id_new" UUID DEFAULT gen_random_uuid()`); | |
| // Update existing records to have unique UUIDs | |
| await queryRunner.query(`UPDATE "users" SET "id_new" = gen_random_uuid() WHERE "id_new" IS NULL`); | |
| // First, add a new UUID column | |
| await queryRunner.query(`ALTER TABLE "users" ADD COLUMN "id_new" UUID`); | |
| // Update existing records to have unique UUIDs | |
| await queryRunner.query(`UPDATE "users" SET "id_new" = gen_random_uuid()`); |
🤖 Prompt for AI Agents
In src/migrations/1751199237000-MigrateUserIdToUUID.ts around lines 8–11, the
migration adds id_new with DEFAULT gen_random_uuid(), so the subsequent UPDATE
... WHERE "id_new" IS NULL is redundant (it will never match); remove the UPDATE
statement (or replace it with a simple UPDATE "users" SET "id_new" =
gen_random_uuid() without the WHERE if you intentionally want to force new UUIDs
for all existing rows), and keep only the ADD COLUMN with the default (and later
steps to backfill/replace the old id if needed).
| public async down(queryRunner: QueryRunner): Promise<void> { | ||
| // Revert to SERIAL id | ||
| await queryRunner.query(`ALTER TABLE "users" ADD COLUMN "id_old" SERIAL`); | ||
|
|
||
| // Drop the UUID primary key constraint | ||
| await queryRunner.query(`ALTER TABLE "users" DROP CONSTRAINT "PK_a3ffb1c0c8416b9fc6f907b7433"`); | ||
|
|
||
| // Rename columns | ||
| await queryRunner.query(`ALTER TABLE "users" DROP COLUMN "id"`); | ||
| await queryRunner.query(`ALTER TABLE "users" RENAME COLUMN "id_old" TO "id"`); | ||
|
|
||
| // Restore the SERIAL primary key | ||
| await queryRunner.query(`ALTER TABLE "users" ADD CONSTRAINT "PK_a3ffb1c0c8416b9fc6f907b7433" PRIMARY KEY ("id")`); | ||
|
|
||
| // Drop the walletAddress index | ||
| await queryRunner.query(`DROP INDEX IF EXISTS "IDX_users_walletAddress"`); |
There was a problem hiding this comment.
Down migration is destructive and will fail
The down migration attempts to convert UUIDs back to SERIAL integers, which will fail because:
- UUIDs cannot be cast to integers
- Even if it worked, all relationships would be broken
- The SERIAL sequence wouldn't know what value to continue from
This migration should be marked as irreversible or implement a proper rollback strategy:
public async down(queryRunner: QueryRunner): Promise<void> {
+ throw new Error('This migration cannot be reversed. UUID to integer conversion would result in data loss.');
- // Revert to SERIAL id
- await queryRunner.query(`ALTER TABLE "users" ADD COLUMN "id_old" SERIAL`);
- // ... rest of the down migration
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async down(queryRunner: QueryRunner): Promise<void> { | |
| // Revert to SERIAL id | |
| await queryRunner.query(`ALTER TABLE "users" ADD COLUMN "id_old" SERIAL`); | |
| // Drop the UUID primary key constraint | |
| await queryRunner.query(`ALTER TABLE "users" DROP CONSTRAINT "PK_a3ffb1c0c8416b9fc6f907b7433"`); | |
| // Rename columns | |
| await queryRunner.query(`ALTER TABLE "users" DROP COLUMN "id"`); | |
| await queryRunner.query(`ALTER TABLE "users" RENAME COLUMN "id_old" TO "id"`); | |
| // Restore the SERIAL primary key | |
| await queryRunner.query(`ALTER TABLE "users" ADD CONSTRAINT "PK_a3ffb1c0c8416b9fc6f907b7433" PRIMARY KEY ("id")`); | |
| // Drop the walletAddress index | |
| await queryRunner.query(`DROP INDEX IF EXISTS "IDX_users_walletAddress"`); | |
| public async down(queryRunner: QueryRunner): Promise<void> { | |
| throw new Error('This migration cannot be reversed. UUID to integer conversion would result in data loss.'); | |
| } |
| await queryRunner.query(`ALTER TABLE "user_roles" ALTER COLUMN "userId" TYPE INTEGER USING "userId"::integer`); | ||
| await queryRunner.query(`ALTER TABLE "buyer_requests" ALTER COLUMN "userId" TYPE INTEGER USING "userId"::integer`); | ||
| await queryRunner.query(`ALTER TABLE "reviews" ALTER COLUMN "userId" TYPE INTEGER USING "userId"::integer`); |
There was a problem hiding this comment.
Down migration will lose UUID data
Converting UUIDs back to integers in the down migration will result in data loss since UUIDs cannot be meaningfully converted to integers. This makes the migration effectively irreversible in production.
Consider documenting this as a one-way migration or implementing a mapping table approach:
// Revert column types to integer (this will require data migration in a real scenario)
+ // WARNING: This is a destructive operation that will lose the UUID values
+ // Consider implementing a mapping table if rollback capability is needed
await queryRunner.query(`ALTER TABLE "user_roles" ALTER COLUMN "userId" TYPE INTEGER USING "userId"::integer`);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/migrations/1751199238000-UpdateForeignKeysToUUID.ts around lines 59 to
61, the down migration attempts to cast UUID foreign-key values back to INTEGER
which will lose data; either treat this migration as irreversible and make
down() throw or log a clear one-way migration notice, or implement a reversible
strategy: create and populate mapping tables before the up() change that map old
integer IDs to new UUIDs (and persist those maps), then in down() use those
mapping tables to restore integer IDs and update FK references and constraints;
update migration comments and tests accordingly to reflect the chosen approach.
| userId, | ||
| roleId: role.id, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Saving string userId to UserRole is correct; add type-safety and dedup protection.
- Use the shared RoleName type instead of local string for
assignRoleToUserto prevent drift. - Consider preventing duplicate role rows (unique constraint on [userId, roleId] and/or idempotent assign).
Follow-ups (outside selected lines):
- Replace the local alias with an import:
// top of file
- type RoleName = 'buyer' | 'seller' | 'admin';
+ import type { RoleName } from '../types/role.types';- Strongly type the parameter:
- async assignRoleToUser(userId: string, roleName: string): Promise<void> {
+ async assignRoleToUser(userId: string, roleName: RoleName): Promise<void> {- Optionally, enforce idempotency:
const existing = await this.userRoleRepository.findOne({ where: { userId, roleId: role.id } });
if (existing) return;🤖 Prompt for AI Agents
In src/modules/auth/services/role.service.ts around lines 50 to 52, the method
currently accepts a plain string roleName and writes userId/roleId rows without
guarding duplicates; update the file to import and use the shared RoleName type
for the assignRoleToUser parameter to ensure type-safety (replace local alias
with import at the top of the file and change the method signature to accept
RoleName), and add idempotency/dedup protection before inserting (check for an
existing UserRole row with the same userId and roleId and return early if found)
or ensure a unique constraint on [userId, roleId] so duplicate rows are
prevented.
| userId: string, | ||
| productId: number, | ||
| rating: number, | ||
| comment?: string |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Signature update to userId: string is correct and consistent.
The public method now aligns with UUID-based user IDs.
Ensure all controller and DTO entry points now pass a string UUID for userId. Run:
🏁 Script executed:
#!/bin/bash
# Find any parseInt/Number conversions for userId that may remain
rg -n "parseInt\\(\\s*userId\\s*\\)|Number\\(\\s*userId\\s*\\)|: number\\s*[),]" -A 2Length of output: 23595
Remove remaining numeric conversions of userId
Several parseInt(userId) and Number(userId) usages remain in your services and controllers, which will break once userId is treated exclusively as a UUID string. Please update these to accept and propagate the string UUID (or convert at a single, well-defined boundary).
• src/modules/shared/services/role.service.ts
– line 49: save({ userId: parseInt(userId), … })
– line 58: where: { userId: parseInt(userId) }
• src/modules/wishlist/services/wishlist.service.ts
– lines 28–30, 48–50, 60–62: calls to this.toNumber(userId)
• src/modules/products/controllers/product.controller.ts
– line 78: sellerId: Number(userId)
Please remove or centralize these conversions so that all entry points consistently accept a string userId UUID.
🤖 Prompt for AI Agents
In src/modules/reviews/services/review.service.ts around lines 21–24, and
related services/controllers noted in the review, several places still convert
userId to a number (parseInt/Number/this.toNumber); remove those numeric
conversions and treat userId as a UUID string throughout the service layer:
update calls to save({ userId, … }) and where: { userId } to pass the string
UUID, remove usages of this.toNumber(userId) and Number(userId) in the listed
files, and if numeric conversion is absolutely required keep it only at a
single, well-defined boundary (e.g., an adapter or controller entry point) and
change downstream types/signatures to accept string userId.
| const currentUserWalletAddress = req.user?.walletAddress; | ||
| const currentUserRole = req.user?.role?.[0]; | ||
|
|
||
| // Check if user is updating their own profile or is admin | ||
| if (userId !== currentUserId && currentUserRole !== 'admin') { | ||
| if (walletAddress !== currentUserWalletAddress && currentUserRole !== 'admin') { | ||
| throw new UnauthorizedError('You can only update your own profile'); | ||
| } |
There was a problem hiding this comment.
Potential security issue: Missing null checks for user authentication
The code accesses req.user?.walletAddress and req.user?.role?.[0] but doesn't explicitly verify that req.user exists before the authorization check. While the JwtAuthGuard should ensure authentication, adding explicit null checks would make the code more defensive.
async updateUser(
@Param('walletAddress') walletAddress: string,
@Body() updateDto: UpdateUserDto,
@Req() req: Request
): Promise<UserUpdateResponse> {
+ if (!req.user) {
+ throw new UnauthorizedError('User not authenticated');
+ }
const currentUserWalletAddress = req.user?.walletAddress;
const currentUserRole = req.user?.role?.[0];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const currentUserWalletAddress = req.user?.walletAddress; | |
| const currentUserRole = req.user?.role?.[0]; | |
| // Check if user is updating their own profile or is admin | |
| if (userId !== currentUserId && currentUserRole !== 'admin') { | |
| if (walletAddress !== currentUserWalletAddress && currentUserRole !== 'admin') { | |
| throw new UnauthorizedError('You can only update your own profile'); | |
| } | |
| async updateUser( | |
| @Param('walletAddress') walletAddress: string, | |
| @Body() updateDto: UpdateUserDto, | |
| @Req() req: Request | |
| ): Promise<UserUpdateResponse> { | |
| if (!req.user) { | |
| throw new UnauthorizedError('User not authenticated'); | |
| } | |
| const currentUserWalletAddress = req.user?.walletAddress; | |
| const currentUserRole = req.user?.role?.[0]; | |
| // Check if user is updating their own profile or is admin | |
| if (walletAddress !== currentUserWalletAddress && currentUserRole !== 'admin') { | |
| throw new UnauthorizedError('You can only update your own profile'); | |
| } | |
| // ...rest of the update logic... | |
| } |
🤖 Prompt for AI Agents
In src/modules/users/controllers/user.controller.ts around lines 106 to 112, the
code uses req.user?.walletAddress and req.user?.role?.[0] but doesn't explicitly
verify req.user exists before performing the auth check; add a defensive check
that throws UnauthorizedError (or same auth error) if req.user is
null/undefined, and ensure req.user.role is an array (or treat missing role as
non-admin) before comparing role[0] to 'admin' so the authorization condition
reliably handles unauthenticated requests and missing role data.
| if (walletAddress !== currentUserWalletAddress && currentUserRole !== 'admin') { | ||
| throw new UnauthorizedError('You can only update your own profile'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider case-insensitive wallet address comparison
Ethereum wallet addresses are case-insensitive in their canonical form, but may be provided in different cases (checksummed vs lowercase). Consider normalizing the addresses before comparison to avoid false negatives.
// Check if user is updating their own profile or is admin
- if (walletAddress !== currentUserWalletAddress && currentUserRole !== 'admin') {
+ if (walletAddress.toLowerCase() !== currentUserWalletAddress?.toLowerCase() && currentUserRole !== 'admin') {
throw new UnauthorizedError('You can only update your own profile');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (walletAddress !== currentUserWalletAddress && currentUserRole !== 'admin') { | |
| throw new UnauthorizedError('You can only update your own profile'); | |
| } | |
| // Check if user is updating their own profile or is admin | |
| if ( | |
| walletAddress.toLowerCase() !== currentUserWalletAddress?.toLowerCase() && | |
| currentUserRole !== 'admin' | |
| ) { | |
| throw new UnauthorizedError('You can only update your own profile'); | |
| } |
🤖 Prompt for AI Agents
In src/modules/users/controllers/user.controller.ts around lines 110 to 112, the
wallet address equality check is case-sensitive which can cause false negatives
for checksummed vs lowercase addresses; normalize both addresses before
comparing (e.g., trim and convert both walletAddress and
currentUserWalletAddress to the same case such as .toLowerCase(), or use a
reliable library method like ethers.utils.getAddress to canonicalize them) and
then perform the comparison, ensuring you also handle null/undefined values
safely before normalization.
2 participants
🚀 StarShop Pull Request
Mark with an
xall the checkboxes that apply (like[x])📌 Type of Change
📝 Changes description
Security Enhancement: UUID Migration and walletAddress as Primary Identifier
This PR implements a comprehensive security upgrade by migrating from numeric IDs to UUIDs and using walletAddress as the primary identifier for all public API interactions.
Key Changes:
Database Schema Migration:
Entity Updates:
Service Layer Refactoring:
Controller Updates:
Authentication & JWT Updates:
DTO Updates:
Comprehensive Testing:
Documentation:
Security Benefits:
API Changes:
📸 Evidence (A photo is required as evidence)
Database Migrations Created:
Entity Files Updated:
Service Files Refactored:
Controller Files Updated:
Test Files Added:
Documentation Created:
⏰ Time spent breakdown
🌌 Comments
Technical Implementation Notes:
Backward Compatibility: The implementation maintains backward compatibility by:
Performance Considerations:
Security Enhancements:
Migration Strategy:
Testing Coverage:
Future Considerations:
This implementation significantly enhances the security posture of StarShop while maintaining all existing functionality and providing a clear migration path for production deployment.
Thank you for contributing to StarShop, we are glad that you have chosen us as your project of choice and we hope that you continue to contribute to this great project, so that together we can make our mark at the top!
Summary by CodeRabbit