feat: ✨ update user module #72
Conversation
WalkthroughThe pull request introduces several modifications across various files related to user management in the application. Key changes include the addition of a Changes
Possibly related issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 9
🧹 Outside diff range and nitpick comments (17)
libs/users/domain/src/lib/user.entity.ts (1)
4-7: Consider adding TypeScript improvementsThe entity could benefit from stronger typing and immutability.
Consider these TypeScript improvements:
export class User { constructor( public readonly id: string, private readonly _firstName: string, private readonly _lastName: string, private readonly _email: string, private readonly _hashedPassword: string, ) { // ... validation } // Getters to ensure immutability get firstName(): string { return this._firstName; } get lastName(): string { return this._lastName; } get email(): string { return this._email; } }libs/users/domain/src/lib/users.repository.ts (1)
5-5: LGTM! Well-structured interface extension.The new
findByEmailmethod follows the same pattern asfindById, maintaining consistency in the repository interface. The return typePromise<User | null>appropriately handles both successful and unsuccessful lookups.This addition aligns well with clean architecture principles by keeping the domain interface focused and implementation-agnostic.
libs/users/interface-adapters/src/lib/dto/user.dto.ts (2)
9-12: Add non-null constraints for name fields.Since these appear to be required fields, consider making them non-nullable in the GraphQL schema.
- @Field() + @Field({ nullable: false }) firstName: string; - @Field() + @Field({ nullable: false }) lastName: string;
Line range hint
4-22: Consider documentation and migration impact.This change to the user data structure has several implications:
- GraphQL schema changes might affect API consumers
- Existing data might need migration to split the name field
- Documentation should be updated to reflect the new user structure
libs/users/application/src/lib/users.service.ts (1)
13-16: Consider implementing a unified user lookup strategyAs the service grows with multiple lookup methods, consider implementing a more flexible and maintainable approach:
- Define a common interface for user lookups with different criteria
- Implement a strategy pattern for different lookup types
- Ensure consistent error handling across all lookup methods
This will make it easier to:
- Add new lookup methods in the future
- Maintain consistent behavior across different lookup types
- Test different lookup scenarios
Example approach:
type UserLookupCriteria = | { type: 'id'; value: string } | { type: 'email'; value: string }; @Injectable() export class UsersService { async findUser(criteria: UserLookupCriteria): Promise<User | null> { switch (criteria.type) { case 'id': return this.getUserUseCase.execute(criteria.value); case 'email': return this.getUserByEmailUseCase.execute(criteria.value); } } }libs/users/interface-adapters/src/lib/dto/user.dto.spec.ts (1)
4-17: Consider adding test cases for edge cases and validation.The current test only covers the happy path. Consider adding test cases for:
- Empty strings for firstName, lastName
- Invalid email formats
- Maximum length validation (if applicable)
- Special characters in names
- Email normalization (uppercase/lowercase)
Here's a suggested implementation:
describe('UserDto', () => { describe('constructor', () => { it('should create a new UserDto instance', () => { const id = '123'; const firstName = 'Test'; const lastName = 'User'; const email = '[email protected]'; const userDto = new UserDto(id, firstName, lastName, email); expect(userDto).toBeDefined(); expect(userDto.id).toBe(id); expect(userDto.firstName).toBe(firstName); expect(userDto.lastName).toBe(lastName); expect(userDto.email).toBe(email); }); it('should normalize email to lowercase', () => { const userDto = new UserDto('123', 'Test', 'User', '[email protected]'); expect(userDto.email).toBe('[email protected]'); }); it('should throw error for invalid email format', () => { expect(() => new UserDto('123', 'Test', 'User', 'invalid-email') ).toThrow('Invalid email format'); }); it('should handle special characters in names', () => { const userDto = new UserDto('123', 'José-María', "O'Connor", '[email protected]'); expect(userDto.firstName).toBe('José-María'); expect(userDto.lastName).toBe("O'Connor"); }); }); });libs/users/domain/src/lib/user.entity.spec.ts (2)
20-37: Consider more specific equality assertions.The current
toEqualassertion might be too permissive if the User entity has additional internal properties or getters. Consider either:
- Using more specific property checks
- Creating a type-safe comparison helper
Example refactor:
- expect(user).toEqual(userData); + // Explicitly verify only the expected properties + expect({ + id: user.id, + firstName: user.firstName, + lastName: user.lastName, + email: user.email, + password: user.password, + }).toEqual(userData);
Line range hint
1-40: Consider adding test cases for edge cases and error scenarios.The current test suite covers the happy path but could benefit from additional test cases:
- Empty string handling
- Email format validation
- Password requirements validation
Example additional test:
it('should validate email format', () => { expect(() => new User( 'test-id', 'John', 'Doe', 'invalid-email', // Invalid email format 'password123' )).toThrow(); });libs/users/application/src/lib/use-cases/get-user.use-case.spec.ts (2)
18-22: Improve code style consistencyThe mock user object could benefit from consistent formatting.
Apply this diff to improve readability:
- const mockUser = { id: '1', - firstName: 'Test', - lastName: 'User', - email: '[email protected]' - }; + const mockUser = { + id: '1', + firstName: 'Test', + lastName: 'User', + email: '[email protected]' + };
Line range hint
18-36: Consider adding validation tests for new user propertiesThe test suite only verifies basic find-by-id functionality. With the addition of new user properties (firstName, lastName, email), consider adding test cases that verify:
- All properties are correctly returned
- Type validation for each property
- Edge cases (empty strings, malformed email, etc.)
Would you like me to help generate additional test cases for property validation?
libs/users/domain/src/lib/users.repository.spec.ts (3)
6-19: Consider using hashed passwords and test constants in mock data.While this is test data, it's recommended to:
- Use hashed passwords instead of plain text to reinforce security best practices
- Extract mock data to constants for reusability across test files
+ const MOCK_USERS = [ + { + id: '1', + firstName: 'John', + lastName: 'Doe', + email: '[email protected]', + password: '$2b$10$dX5.4YjFc8yf9mE9UqDXXOZ7O.1TN6U1E.MAp.0TqJ3Z7fF0M2D2W' // hashed 'password123' + }, + // ... + ];
40-46: Improve test assertions and descriptions.Consider these improvements:
- Avoid exposing sensitive data in test output
- Use more descriptive test names
- Add test cases for partial object matching
- test('findById should return a user by id', async () => { + test('findById returns user with matching id excluding sensitive data', async () => { const user = await usersRepository.findById('1'); - expect(user).toEqual({ - id: '1', - firstName: 'John', - lastName: 'Doe', - email: '[email protected]', - password: 'password123' - }); + expect(user).toMatchObject({ + id: '1', + firstName: 'John', + lastName: 'Doe', + email: '[email protected]' + }); + expect(user).toHaveProperty('password'); });
Repository pattern implementation needs to be updated
The verification reveals inconsistencies in the repository pattern implementation:
- The
MongooseUsersRepositoryis missing thefindByEmailmethod that's defined in theUsersRepositoryinterface- The mongoose implementation returns a
Userwith onlyidandnameproperties, while the mock repository includes additional fields (firstName,lastName,password)🔗 Analysis chain
Line range hint
1-53: Verify repository pattern implementation across the codebase.Let's ensure the repository pattern is consistently implemented across related files.
Let me gather more information about the service layer implementation to ensure we have a complete picture.
Let me check the use case implementation to understand how the repository pattern is being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent repository pattern implementation echo "Checking repository interface..." ast-grep --pattern 'interface UsersRepository { $$$ findByEmail($$$): Promise<$$$> $$$ }' echo "Checking service implementation..." ast-grep --pattern 'class UsersService { $$$ findByEmail($$$): Promise<$$$> $$$ }'Length of output: 669
Script:
#!/bin/bash # Search for any service class that might be using the repository rg -l "UsersRepository" --type ts # Check for any service class implementation ast-grep --pattern 'class $_ { constructor(private readonly usersRepository: UsersRepository) { $$$ } }'Length of output: 560
Script:
#!/bin/bash # Check the use case implementation echo "Checking GetUserUseCase implementation..." cat libs/users/application/src/lib/use-cases/get-user.use-case.ts # Check mongoose repository implementation echo -e "\nChecking mongoose repository implementation..." cat libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.tsLength of output: 1494
libs/users/application/src/lib/users.service.spec.ts (3)
33-39: Consider using a hashed password in test dataWhile testing, it's a good practice to use hashed passwords instead of plain text, even in test data. This helps maintain consistency with production patterns and prevents accidental exposure of password patterns.
const user: User = { id: '1', firstName: 'John', lastName: 'Doe', email: '[email protected]', - password: 'password123', + password: '$2b$10$YaB6xpBcJe8Nc7rtAa7zqOYwZ6IxGQr.cimhPr8JZ8Qg8ZYvZ9vK2', };
Line range hint
29-43: Add test coverage for findByEmail methodThe AI summary indicates that a
findByEmailmethod was added to the service, but there are no corresponding test cases. Consider adding test coverage for this new functionality.Add the following test cases:
describe('findByEmail', () => { it('should return a user when found by email', async () => { const user: User = { id: '1', firstName: 'John', lastName: 'Doe', email: '[email protected]', password: '$2b$10$YaB6xpBcJe8Nc7rtAa7zqOYwZ6IxGQr.cimhPr8JZ8Qg8ZYvZ9vK2', }; jest.spyOn(getUserUseCase, 'execute').mockResolvedValue(user); const result = await service.findByEmail('[email protected]'); expect(result).toEqual(user); }); it('should return null when user not found by email', async () => { jest.spyOn(getUserUseCase, 'execute').mockResolvedValue(null); const result = await service.findByEmail('[email protected]'); expect(result).toBeNull(); }); });
Line range hint
29-31: Make test descriptions more specificThe current test description "should return a user if found" could be more descriptive given the expanded user object structure.
- it('should return a user if found', async () => { + it('should return a complete user profile when found by id', async () => {libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.spec.ts (1)
32-36: Consider improving the mock user data structureA few suggestions to enhance the test data:
- The
namefield appears redundant since you're now usingfirstNameandlastName. This could lead to data inconsistencies.- Consider using a hashed password in the mock to better reflect production scenarios.
const mockUser = { id: '507f1f77bcf86cd799439011', firstName: 'John', lastName: 'Doe', email: '[email protected]', - password: 'password123', - name: 'John Doe' + password: '$2b$10$6HvkHs6KC3GzYpYOkWM2g.zBwxgZQWQxcJOY6TJxdgA4V4FoAXNi2' // hashed 'password123' };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
libs/users/application/src/lib/use-cases/get-user.use-case.spec.ts(1 hunks)libs/users/application/src/lib/users.service.spec.ts(1 hunks)libs/users/application/src/lib/users.service.ts(1 hunks)libs/users/domain/src/lib/user.entity.spec.ts(1 hunks)libs/users/domain/src/lib/user.entity.ts(1 hunks)libs/users/domain/src/lib/users.repository.spec.ts(2 hunks)libs/users/domain/src/lib/users.repository.ts(1 hunks)libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.spec.ts(2 hunks)libs/users/interface-adapters/src/lib/dto/user.dto.spec.ts(1 hunks)libs/users/interface-adapters/src/lib/dto/user.dto.ts(1 hunks)libs/users/interface-adapters/src/lib/resolver/users.resolver.spec.ts(0 hunks)libs/users/interface-adapters/src/lib/resolver/users.resolver.ts(1 hunks)
💤 Files with no reviewable changes (1)
- libs/users/interface-adapters/src/lib/resolver/users.resolver.spec.ts
🧰 Additional context used
📓 Learnings (1)
libs/users/domain/src/lib/user.entity.spec.ts (1)
Learnt from: zhumeisongsong
PR: zhumeisongsong/graphql-federation-workspace#31
File: libs/user/domain/src/lib/user.entity.spec.ts:10-14
Timestamp: 2024-11-14T08:49:38.202Z
Learning: In the `user.entity.spec.ts` tests, we do not need to include test cases that are expected to cause TypeScript compilation errors, such as attempting to modify readonly properties.
🔇 Additional comments (6)
libs/users/interface-adapters/src/lib/dto/user.dto.ts (1)
17-21: Verify all constructor usages are updated.
This constructor signature change is a breaking change. Let's ensure all instantiations of UserDto have been updated.
✅ Verification successful
All constructor usages are correctly updated
The verification shows that all instances of UserDto constructor calls in the codebase are using the new signature with all four required parameters (id, firstName, lastName, email). Both occurrences in users.resolver.ts and user.dto.spec.ts follow the updated constructor pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UserDto instantiations to verify they use the new signature
ast-grep --pattern 'new UserDto($$$)'
Length of output: 310
libs/users/interface-adapters/src/lib/dto/user.dto.spec.ts (1)
6-16: LGTM! Test covers the basic happy path.
The test correctly validates the new UserDto structure with firstName, lastName, and email properties.
libs/users/interface-adapters/src/lib/resolver/users.resolver.ts (1)
11-13: LGTM! Improved method signature formatting
The multi-line formatting enhances readability while maintaining the same functionality.
libs/users/domain/src/lib/user.entity.spec.ts (1)
4-17: Verify password handling and input validation requirements.
While the test successfully verifies property assignments, consider:
- Storing passwords in plain text poses a security risk. The User entity should likely hash passwords before storage.
- The test doesn't verify any input validation (e.g., email format, password strength requirements).
Let's check if there are any validation rules defined:
libs/users/application/src/lib/use-cases/get-user.use-case.spec.ts (1)
11-11: Add test coverage for findByEmail functionality
The mock repository includes a findByEmail method, but there are no test cases covering this functionality. Consider adding test cases to verify the email lookup behavior.
Would you like me to help generate test cases for the findByEmail functionality?
libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.spec.ts (1)
Line range hint 32-49: Verify consistent user structure across the codebase
Since this is part of a larger user module update, we should ensure the user structure is consistent across all related files.
✅ Verification successful
Let me try another verification script with a corrected regex pattern:
Let me try one final verification to check the schema definition:
Based on the verification results, I can now provide the final response:
✓ User structure is consistent across the codebase
The verification shows consistent user structure across test files:
- The User entity in
libs/users/domain/src/lib/user.entity.tsdefines the core structure withid,firstName,lastName,email, andpassword - Mock objects in test files (
mongoose-users.repository.spec.tsandget-user.use-case.spec.ts) maintain this structure - The
namefield in the test appears to be an additional derived property and doesn't conflict with the core structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other user-related test files and check their mock structures
echo "Checking user mock structures across test files..."
rg -l "mock.*User.*=.*{" | while read -r file; do
echo "=== $file ==="
rg -A 10 "mock.*User.*=.*{" "$file"
done
# Verify User class/interface definition matches test expectations
echo -e "\nChecking User class/interface definition..."
ast-grep --pattern 'class User {
$$$
}'
ast-grep --pattern 'interface User {
$$$
}'
Length of output: 1020
Script:
#!/bin/bash
# Search for user-related test files and check their mock structures
echo "Checking user mock structures across test files..."
rg -A 5 "mockUser\s*=\s*\{"
# Search for user-related schemas and models
echo -e "\nChecking User schemas and models..."
rg -A 5 "Schema\s*\(\s*\{" "**/*user*.ts"
# Search for any other User-related type definitions
echo -e "\nChecking other User-related types..."
ast-grep --pattern 'type User = {
$$$
}'
ast-grep --pattern 'interface IUser {
$$$
}'
Length of output: 1751
Script:
#!/bin/bash
# Search for mongoose schema definition with correct path pattern
echo "Checking User schema definition..."
rg -A 5 "Schema\s*\(\s*\{" $(fd -t f ".*user.*\.ts$")
# Search for any user-related interfaces or types with different naming patterns
echo -e "\nChecking additional User interfaces..."
ast-grep --pattern 'interface UserDocument {
$$$
}'
ast-grep --pattern 'interface UserModel {
$$$
}'
Length of output: 1455
| public firstName: string, | ||
| public lastName: string, | ||
| public email: string, | ||
| public password: string, |
There was a problem hiding this comment.
Storing passwords as plain text in domain entities poses a significant security risk. The password should be hashed before storage and the hashing logic should be encapsulated in a separate service.
Consider one of these approaches:
- Remove the password from the domain entity and handle it separately in an auth-specific service
- Store only the hashed password and move the hashing logic to a factory method:
export class User {
private constructor(
public readonly id: string,
public firstName: string,
public lastName: string,
public email: string,
private readonly hashedPassword: string,
) {}
static async create(
id: string,
firstName: string,
lastName: string,
email: string,
plainTextPassword: string
): Promise<User> {
const hashedPassword = await hashPassword(plainTextPassword);
return new User(id, firstName, lastName, email, hashedPassword);
}
}| public firstName: string, | ||
| public lastName: string, | ||
| public email: string, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for user properties
The domain entity should enforce business rules through validation.
Consider adding validation for:
- Email format
- Required fields
- Name length constraints
export class User {
constructor(
public readonly id: string,
public firstName: string,
public lastName: string,
public email: string,
private readonly hashedPassword: string,
) {
this.validateEmail(email);
this.validateNames(firstName, lastName);
}
private validateEmail(email: string): void {
if (!email || !email.includes('@')) {
throw new Error('Invalid email format');
}
}
private validateNames(firstName: string, lastName: string): void {
if (!firstName || !lastName) {
throw new Error('First name and last name are required');
}
if (firstName.length < 2 || lastName.length < 2) {
throw new Error('Names must be at least 2 characters long');
}
}
}| public firstName: string, | ||
| public lastName: string, | ||
| public email: string, | ||
| public password: string, |
There was a problem hiding this comment.
💡 Codebase verification
Breaking changes detected in User entity constructor
The verification reveals significant inconsistencies and breaking changes:
- The
Userentity constructor requires 5 parameters (id, firstName, lastName, email, password) - However, the mongoose repository's
findByIdmethod creates users with only 2 parameters (id, name) - The mongoose schema only defines a single
namefield, not matching the entity structure
Key locations requiring updates:
libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.ts: Incorrect User instantiationlibs/users/infrastructure/mongoose/src/lib/user.schema.ts: Schema doesn't match entity fieldslibs/users/domain/src/lib/user.entity.spec.ts: Contains the only correct usage of the new constructor
🔗 Analysis chain
Verify the impact of constructor signature change
The constructor signature has changed significantly. This is a breaking change that will affect all code that creates User instances.
Let's verify the impact:
Let me gather more information about the actual implementation of the User class and its usage in the repository to better understand the impact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all direct instantiations of the User class
ast-grep --pattern 'new User($$$)'
# Find all test files that might need updates
fd -e spec.ts -e test.ts
# Look for potential factories or repositories that create users
rg -l "User['\"].*factory|repository" --type ts
Length of output: 2438
Script:
#!/bin/bash
# Check the User class implementation
ast-grep --pattern 'class User {
$$$
}'
# Check the mongoose repository implementation to see how it creates users
cat libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.ts
# Check the user schema to understand the data structure
cat libs/users/infrastructure/mongoose/src/lib/user.schema.ts
Length of output: 1729
| @@ -2,6 +2,7 @@ import { User } from './user.entity'; | |||
|
|
|||
| export interface UsersRepository { | |||
| findById(id: string): Promise<User | null>; | |||
| findByEmail(email: string): Promise<User | null>; | |||
There was a problem hiding this comment.
💡 Codebase verification
Implementation and tests are incomplete for findByEmail method
The MongooseUsersRepository is missing the required findByEmail implementation, and its test suite lacks coverage for this method. Here are the specific issues:
MongooseUsersRepositoryneeds to implement thefindByEmailmethod as required by the interfacemongoose-users.repository.spec.tsneeds test cases for thefindByEmailfunctionality
🔗 Analysis chain
Verify implementation completeness.
Let's ensure all repository implementations and related tests have been updated with the new method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all repository implementations include findByEmail method
# and corresponding test coverage exists
# Check for repository implementations
echo "Checking repository implementations..."
ast-grep --pattern 'class $_ implements UsersRepository {
$$$
findByEmail($_): $_ {
$$$
}
$$$
}'
# Check for test coverage
echo "Checking test coverage..."
rg -A 5 'describe.*UsersRepository'
rg -A 2 'test.*findByEmail'
Length of output: 3491
Script:
#!/bin/bash
# Let's check for the actual implementation of MongooseUsersRepository
echo "Checking MongooseUsersRepository implementation..."
ast-grep --pattern 'class MongooseUsersRepository implements UsersRepository {
$$$
findByEmail($_): $_ {
$$$
}
$$$
}'
# Let's also check for findByEmail tests specifically
echo "Checking findByEmail test cases..."
rg -B 2 -A 5 'test.*findByEmail'
# Let's verify if there are any other repository implementations
echo "Checking for other repository implementations..."
rg -l "implements UsersRepository"
Length of output: 624
Script:
#!/bin/bash
# Let's examine the MongooseUsersRepository implementation directly
echo "Content of mongoose-users.repository.ts:"
cat libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.ts
# Let's also check the test file content
echo -e "\nContent of mongoose-users.repository.spec.ts:"
cat libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.spec.ts
Length of output: 2978
| @Field() | ||
| email: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add email format validation and non-null constraint.
Consider adding validation for the email field using class-validator decorators and making it non-nullable in the GraphQL schema.
- @Field()
+ @Field({ nullable: false })
+ @IsEmail()
email: string;📝 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.
| @Field() | |
| email: string; | |
| @Field({ nullable: false }) | |
| @IsEmail() | |
| email: string; |
| async findByEmail(email: string): Promise<User | null> { | ||
| return this.getUserUseCase.execute(email); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
GetUserUseCase is not compatible with email lookups
The implementation confirms that GetUserUseCase.execute() specifically uses findById() internally, making it incompatible with email-based lookups. The current implementation will fail as it passes the email to a method expecting a valid MongoDB ObjectId.
Required changes:
- Create a dedicated
GetUserByEmailUseCasethat uses the repository'sfindByEmailmethod - Update
UsersServiceto use the new use case for email lookups - Implement the missing
findByEmailmethod inMongooseUsersRepository
🔗 Analysis chain
Review the use case implementation for email lookup
The new findByEmail method is reusing getUserUseCase.execute() which was originally designed for ID-based lookups. This could lead to incorrect behavior if the use case isn't properly handling different types of identifiers.
Consider one of these approaches:
- Create a dedicated use case for email lookup (preferred)
- Modify the existing use case to explicitly handle different identifier types
Let's verify the use case implementation:
Suggested implementation if using approach #1:
@Injectable()
export class UsersService {
constructor(
private readonly getUserUseCase: GetUserUseCase,
private readonly getUserByEmailUseCase: GetUserByEmailUseCase
) {}
async findByEmail(email: string): Promise<User | null> {
return this.getUserByEmailUseCase.execute(email);
}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the GetUserUseCase implementation to understand how it handles different identifier types
# Look for the use case implementation
ast-grep --pattern 'class GetUserUseCase {
$$$
execute($_) {
$$$
}
}'
# Check for any email-specific repository methods
ast-grep --pattern 'interface $UsersRepository {
$$$
findByEmail($_) {
$$$
}
}'
Length of output: 173
Script:
#!/bin/bash
# Let's try a different approach to find the relevant implementations
# Search for GetUserUseCase implementation
rg -l "GetUserUseCase" --type ts
# Search for UsersRepository interface
rg -l "UsersRepository" --type ts
# Search for any email-related methods in the codebase
rg "findByEmail|getByEmail" --type ts -A 3
Length of output: 2107
Script:
#!/bin/bash
# Let's check the implementation of GetUserUseCase to see how it handles the identifier
cat libs/users/application/src/lib/use-cases/get-user.use-case.ts
# Also check the mongoose repository implementation to see how it handles email lookups
cat libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.ts
Length of output: 1292
| const user = await this.usersService.findById(id); | ||
|
|
||
| if (!user) { | ||
| return null; | ||
| } | ||
|
|
||
| return new UserDto(user.id, user.name); | ||
| return new UserDto(user.id, user.firstName, user.lastName, user.email); |
There was a problem hiding this comment.
💡 Codebase verification
The UserDto structure change is compatible but requires database schema update
The verification reveals:
- UserDto class correctly defines firstName, lastName, and email fields
- User domain entity matches with the expected structure (id, firstName, lastName, email)
- UsersService.findById returns the User entity with compatible fields
- However, the UserDocument schema in Mongoose still uses a 'name' field instead of firstName/lastName
Issues found:
libs/users/infrastructure/mongoose/src/lib/user.schema.ts: Schema needs to be updated to use firstName and lastName fields instead of name
🔗 Analysis chain
Verify compatibility with updated UserDto structure
The return statement has been updated to use the new UserDto structure with firstName, lastName, and email. Let's verify the compatibility of these changes.
Let me gather more information about the UserDto class definition and its usage.
Let me check one more thing to verify the User entity structure from the GetUserUseCase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the compatibility of the updated UserDto structure
# Check UserDto constructor definition
echo "Checking UserDto constructor definition..."
ast-grep --pattern 'class UserDto {
constructor($$$) {
$$$
}
}'
# Check if UsersService.findById returns compatible structure
echo "Checking UsersService.findById implementation..."
ast-grep --pattern 'class UsersService {
$$$
async findById($$$) {
$$$
}
$$$
}'
# Look for other UserDto instantiations that might need updates
echo "Checking other UserDto instantiations..."
rg "new UserDto\(" -A 1
Length of output: 883
Script:
#!/bin/bash
# Check UserDto class definition
echo "Checking UserDto class definition..."
rg "class UserDto" -A 10
# Check for any test cases that might reveal the expected structure
echo "Checking UserDto test cases..."
rg "describe.*UserDto" -A 20
# Check UsersService implementation
echo "Checking UsersService implementation..."
rg "class UsersService" -A 20
Length of output: 3317
Script:
#!/bin/bash
# Check User entity and GetUserUseCase implementation
echo "Checking User entity definition..."
rg "class User" -A 10
echo "Checking GetUserUseCase implementation..."
rg "class GetUserUseCase" -A 10
Length of output: 4946
| async findByEmail(email: string): Promise<User | null> { | ||
| return this.users.find((user) => user.email === email) || null; | ||
| } |
There was a problem hiding this comment.
Add test cases for findByEmail method.
The new method lacks test coverage for:
- Successfully finding a user by email
- Case-sensitivity handling
- Invalid email format
- User not found scenario
Would you like me to help generate the missing test cases?
| expect(user?.firstName).toBe(mockUser.firstName); | ||
| expect(user?.lastName).toBe(mockUser.lastName); | ||
| expect(user?.email).toBe(mockUser.email); | ||
| expect(user?.password).toBe(mockUser.password); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test assertions for better security and validation
- Avoid asserting the exact password value to prevent exposing sensitive data in test outputs
- Consider adding validation for the email format
- Consider verifying the complete User instance structure
expect(user?.firstName).toBe(mockUser.firstName);
expect(user?.lastName).toBe(mockUser.lastName);
expect(user?.email).toBe(mockUser.email);
-expect(user?.password).toBe(mockUser.password);
+expect(user?.password).toBeDefined();
+expect(user?.password).not.toBe('password123');
+expect(user?.email).toMatch(/^[^\s@]+@[^\s@]+\.[^\s@]+$/);
+
+// Verify complete structure
+expect(Object.keys(user!)).toEqual(
+ expect.arrayContaining(['id', 'firstName', 'lastName', 'email', 'password'])
+);📝 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.
| expect(user?.firstName).toBe(mockUser.firstName); | |
| expect(user?.lastName).toBe(mockUser.lastName); | |
| expect(user?.email).toBe(mockUser.email); | |
| expect(user?.password).toBe(mockUser.password); | |
| expect(user?.firstName).toBe(mockUser.firstName); | |
| expect(user?.lastName).toBe(mockUser.lastName); | |
| expect(user?.email).toBe(mockUser.email); | |
| expect(user?.password).toBeDefined(); | |
| expect(user?.password).not.toBe('password123'); | |
| expect(user?.email).toMatch(/^[^\s@]+@[^\s@]+\.[^\s@]+$/); | |
| // Verify complete structure | |
| expect(Object.keys(user!)).toEqual( | |
| expect.arrayContaining(['id', 'firstName', 'lastName', 'email', 'password']) | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
libs/users/infrastructure/mongoose/src/lib/user.schema.ts (2)
9-14: Add validation constraints for name fieldsConsider adding length constraints and string validation for firstName and lastName fields to ensure data quality.
- @Prop({ required: false }) + @Prop({ required: false, minlength: 1, maxlength: 50, trim: true }) firstName!: string; - @Prop({ required: false }) + @Prop({ required: false, minlength: 1, maxlength: 50, trim: true }) lastName!: string;
19-19: Add schema options and compound indexConsider adding timestamps and a compound index for name fields to improve querying by full name.
- export const UserSchema = SchemaFactory.createForClass(UserDocument); + export const UserSchema = SchemaFactory.createForClass(UserDocument); + UserSchema.index({ firstName: 1, lastName: 1 }); + + // Add timestamps + UserSchema.set('timestamps', true);libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.ts (1)
Line range hint
1-40: Consider architectural improvements for federationAs this repository is part of a GraphQL federation workspace, consider the following architectural improvements:
- Move password handling to a dedicated auth service/repository
- Add support for batch loading users (e.g.,
findByIds) to optimize federation queries- Consider implementing cursor-based pagination for scalability
Would you like assistance in implementing any of these architectural improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
libs/users/infrastructure/mongoose/src/lib/mongoose-users.repository.ts(1 hunks)libs/users/infrastructure/mongoose/src/lib/user.schema.ts(1 hunks)
🔇 Additional comments (2)
libs/users/infrastructure/mongoose/src/lib/user.schema.ts (2)
15-16:
Review password field security implications
The password field configuration raises several concerns:
- The combination of
required: trueandnullable: trueseems contradictory - The field name should indicate if it stores a hashed value
- Allowing nullable passwords might create security vulnerabilities
- @Prop({ required: true, nullable: true })
- password!: string;
+ @Prop({ required: true })
+ passwordHash!: string;Let's check if password hashing is implemented elsewhere:
Line range hint 1-19: Verify data migration strategy
The schema changes (removing 'name' and adding new fields) will affect existing user records. Please ensure there's a migration strategy in place.
Let's check for migration scripts:
| email!: string; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unique constraint and index for email field
The email field should be unique and indexed for better query performance, especially since there's a new findByEmail method mentioned in the summary.
- @Prop({ required: true })
+ @Prop({ required: true, unique: true, index: true })
email!: string;📝 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.
| email!: string; | |
| @Prop({ required: true, unique: true, index: true }) | |
| email!: string; | |
| return new User( | ||
| userDocument.id, | ||
| userDocument.lastName, | ||
| userDocument.firstName, | ||
| userDocument.email, | ||
| userDocument.password, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Security Issue: Remove password from User object
The User constructor includes the password field, which is a security risk. Passwords should never be exposed in API responses, even if they are hashed.
Consider this safer implementation:
return new User(
userDocument.id,
userDocument.lastName,
userDocument.firstName,
userDocument.email,
- userDocument.password,
);Additionally, consider implementing a DTO mapper to explicitly control which fields are exposed in the API response.
📝 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.
| return new User( | |
| userDocument.id, | |
| userDocument.lastName, | |
| userDocument.firstName, | |
| userDocument.email, | |
| userDocument.password, | |
| ); | |
| } | |
| return new User( | |
| userDocument.id, | |
| userDocument.lastName, | |
| userDocument.firstName, | |
| userDocument.email, | |
| ); | |
| } |
| async findByEmail(email: string): Promise<User | null> { | ||
| const userDocument = await this.userModel.findOne({ email }).exec(); | ||
| return userDocument | ||
| ? new User( | ||
| userDocument.id, | ||
| userDocument.lastName, | ||
| userDocument.firstName, | ||
| userDocument.email, | ||
| userDocument.password, | ||
| ) | ||
| : null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor needed: Improve security and maintainability
Several improvements are recommended for this method:
- Remove password from the response (same security issue as
findById) - Add email format validation
- Extract the User object construction to avoid code duplication
Consider this improved implementation:
+ private createUserFromDocument(userDocument: UserDocument): User {
+ return new User(
+ userDocument.id,
+ userDocument.lastName,
+ userDocument.firstName,
+ userDocument.email,
+ );
+ }
async findByEmail(email: string): Promise<User | null> {
+ if (!email || !email.match(/^[^\s@]+@[^\s@]+\.[^\s@]+$/)) {
+ throw new Error('Invalid email format');
+ }
const userDocument = await this.userModel.findOne({ email }).exec();
- return userDocument
- ? new User(
- userDocument.id,
- userDocument.lastName,
- userDocument.firstName,
- userDocument.email,
- userDocument.password,
- )
- : null;
+ return userDocument ? this.createUserFromDocument(userDocument) : null;
}Then update findById to use the same helper method:
async findById(id: string): Promise<User | null> {
const _id = new Types.ObjectId(id);
const userDocument = await this.userModel.findById(_id).exec();
- if (!userDocument) {
- return null;
- }
- return new User(
- userDocument.id,
- userDocument.lastName,
- userDocument.firstName,
- userDocument.email,
- userDocument.password,
- );
+ return userDocument ? this.createUserFromDocument(userDocument) : null;
}📝 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.
| async findByEmail(email: string): Promise<User | null> { | |
| const userDocument = await this.userModel.findOne({ email }).exec(); | |
| return userDocument | |
| ? new User( | |
| userDocument.id, | |
| userDocument.lastName, | |
| userDocument.firstName, | |
| userDocument.email, | |
| userDocument.password, | |
| ) | |
| : null; | |
| private createUserFromDocument(userDocument: UserDocument): User { | |
| return new User( | |
| userDocument.id, | |
| userDocument.lastName, | |
| userDocument.firstName, | |
| userDocument.email, | |
| ); | |
| } | |
| async findByEmail(email: string): Promise<User | null> { | |
| if (!email || !email.match(/^[^\s@]+@[^\s@]+\.[^\s@]+$/)) { | |
| throw new Error('Invalid email format'); | |
| } | |
| const userDocument = await this.userModel.findOne({ email }).exec(); | |
| return userDocument ? this.createUserFromDocument(userDocument) : null; | |
| } | |
| async findById(id: string): Promise<User | null> { | |
| const _id = new Types.ObjectId(id); | |
| const userDocument = await this.userModel.findById(_id).exec(); | |
| return userDocument ? this.createUserFromDocument(userDocument) : null; | |
| } |
Summary by CodeRabbit
New Features
findByEmailmethod in theUsersService,UsersRepository, andMongooseUsersRepositoryfor email-based user retrieval.firstName,lastName,email, andpassword.Bug Fixes
Documentation
UserDtoandUserclasses to reflect the updated user attributes.