Feature: Clean architecture entities, controllers and services #132
Conversation
This commit implements the clean architecture concept. The "features" folder has been introduced, containing controllers, entities, and services for different modules. Currently the event and user modules have been implemented with basic fetch functions (get all and find by ID). The ErrorHandler class is introduced for universal error handling. - Added User, UserController and UserService - Added Event, EventController and EventService - Added ErrorHandler (2024/12/14)
WalkthroughA comprehensive set of changes has been introduced to enhance the backend infrastructure for user and event management. The changes include the creation of new utility classes, controllers, entities, and services across multiple files. A new Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
src/features/user/Controllers/UserController.ts (2)
16-18: Correct type annotation in documentationIn the JSDoc example, use
Array<User>instead ofArray<Users>to match the correct type name.Apply this diff to fix the documentation:
* @usage userController.getUsers(<PARAMS>).then( - * (users: Array<Users>) => { ... } + * (users: Array<User>) => { ... } * )
76-80: Correct documentation to reference 'user' instead of 'event'The JSDoc comments for
findUserByIDincorrectly referenceeventControllerandEventinstead ofuserControllerandUser. This appears to be a copy-paste error.Apply this diff to fix the documentation:
/** * Find a single user by ID * - * @usage eventController.FindEventByID(<PARAMS>).then( - * (event: Event) => { ... } + * @usage userController.findUserByID(<PARAMS>).then( + * (user: User) => { ... } * )src/features/event/Controllers/EventController.ts (2)
18-18: Correct type annotation in documentationIn the JSDoc example, use
Array<Event>instead ofArray<Events>to match the correct type name.Apply this diff to fix the documentation:
* @usage eventController.getEvents(<PARAMS>).then( - * (events: Array<Events>) => { ... } + * (events: Array<Event>) => { ... } * )
80-80: Correct method name casing in documentationThe method name in the JSDoc usage example should be
findEventByIDwith a lowercase 'f' to match the actual method name.Apply this diff to fix the documentation:
* @usage eventController.FindEventByID(<PARAMS>).then( + * @usage eventController.findEventByID(<PARAMS>).then(src/utils/ErrorHandler.ts (1)
11-17: Refactor 'ErrorHandler' to use simple functions instead of a static-only classThe
ErrorHandlerclass contains only static members, which can be simplified by exporting functions directly. This reduces complexity and aligns with best practices.Apply this diff to refactor the code:
import { PostgrestError } from "@supabase/supabase-js"; -/** - * A universal error handler class. - * - * This will be called by all controllers and is useful for general - * error-handling logic. - * - * @author Henry - */ -export default class ErrorHandler { - - public static handleSupabaseError(error: PostgrestError) { - console.error(error) - } - -} +/** + * A universal error handler function. + * + * This function is called by all controllers and is useful for general + * error-handling logic. + * + * @author Henry + */ +export function handleSupabaseError(error: PostgrestError) { + console.error(error) +}You will also need to update the imports in other files that use
ErrorHandler:- import ErrorHandler from "../../../utils/ErrorHandler"; + import { handleSupabaseError } from "../../../utils/ErrorHandler";And update the usage:
- ErrorHandler.handleSupabaseError(error) + handleSupabaseError(error)🧰 Tools
🪛 Biome (1.9.4)
[error] 11-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/features/event/Entities/Event.ts (1)
9-22: Consider using an interface for the 'Event' data structureSince the
Eventclass only contains properties and no methods, it might be more appropriate to define it as an interface. This can enhance type safety and flexibility in your code.Apply this diff to refactor
Eventinto an interface:export default class Event { - public id: number = 0; - public name: string = ""; - public type: number = 0; - public description: string = ""; - public startTime: string = ""; - public endTime: string = ""; - public location: string = ""; - public fee: number = 0; - public userID: string = ""; - public createdAt: string = ""; +} +export interface Event { + id: number; + name: string; + type: number; + description: string; + startTime: string; + endTime: string; + location: string; + fee: number; + userID: string; + createdAt: string; }This change will require updating any code that instantiates
Eventobjects using thenewkeyword, as interfaces cannot be instantiated.src/features/event/Services/EventService.ts (1)
12-13: Consider using ISO date strings for time fields.The startTime and endTime fields should use a consistent date format. Consider using ISO strings for better compatibility.
- event.startTime = record.start_time ? record.start_time : "" - event.endTime = record.end_time ? record.end_time : "" + event.startTime = record.start_time ? new Date(record.start_time).toISOString() : "" + event.endTime = record.end_time ? new Date(record.end_time).toISOString() : ""Also applies to: 13-14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/features/event/Controllers/EventController.ts(1 hunks)src/features/event/Entities/Event.ts(1 hunks)src/features/event/Services/EventService.ts(1 hunks)src/features/user/Controllers/UserController.ts(1 hunks)src/features/user/Entities/User.ts(1 hunks)src/features/user/Services/UserService.ts(1 hunks)src/utils/ErrorHandler.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/utils/ErrorHandler.ts
[error] 11-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/features/user/Services/UserService.ts
[error] 3-24: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/features/event/Services/EventService.ts
[error] 3-22: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (2)
src/features/user/Controllers/UserController.ts (1)
46-47:
Fix range conditional check to include zero values
The current condition if (rangeStart && rangeEnd) will fail when rangeStart or rangeEnd is 0, because 0 is a falsy value in JavaScript/TypeScript. To allow ranges starting at 0, check for undefined instead.
Apply this diff to fix the issue:
if (orderBy)
query.order(orderBy, { ascending: !orderDescending })
-if (rangeStart && rangeEnd)
+if (rangeStart !== undefined && rangeEnd !== undefined)
query.range(rangeStart, rangeEnd)Likely invalid or redundant comment.
src/features/event/Controllers/EventController.ts (1)
47-48:
Fix range conditional check to include zero values
The condition if (rangeStart && rangeEnd) will fail when rangeStart or rangeEnd is 0. Modify the condition to check for undefined instead to allow ranges starting at 0.
Apply this diff to fix the issue:
if (orderBy)
query.order(orderBy, { ascending: !orderDescending })
-if (rangeStart && rangeEnd)
+if (rangeStart !== undefined && rangeEnd !== undefined)
query.range(rangeStart, rangeEnd)Likely invalid or redundant comment.
There was a problem hiding this comment.
@yeahlowflicker Fantastic work!
This PR is very important since it will solidify future code styles.
I'm wondering if Entities are really necessary. They seem redundant since we already have Supabase defining models. What are the benefits?
Also, what does features mean actually? Is there a better vocabulary for it?
|
@daviddaytw Thank you David. I introduced the entities because of the following 3 reasons:
Regarding the folder name, what do you think about the term These are based on my limited understanding of the Supabase types. Please feel free to correct me if there's anything wrong, thanks. |
|
I see the reasons. I'm okay with that if an internal consensus is made. |
|
@daviddaytw Thanks for the advice! I think |
|
@daviddaytw @1989ONCE |
This renaming is made to prevent confusion.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
src/backend/event/Controllers/EventController.ts (1)
80-80: Correct the method name in usage exampleThe method name in the usage example should match the actual method
findEventByID(all lowercase). The current example usesFindEventByIDwith an uppercase 'F'.Suggested fix:
- * @usage eventController.FindEventByID(<PARAMS>).then( + * @usage eventController.findEventByID(<PARAMS>).then(src/backend/event/Services/EventService.ts (1)
3-22: Consider refactoringEventServiceto use a function instead of a static-only classThe
EventServiceclass currently contains only static methods. According to best practices, it's preferable to use simple functions instead of classes with only static members to reduce unnecessary complexity.Suggested change:
- export default class EventService { - - public static parseEvent(record: DBEvent) : Event { + export function parseEvent(record: DBEvent) : Event { const event = new Event() event.id = record.id event.name = record.name ?? "" event.type = record.type ?? 0 event.description = record.description ?? "" event.startTime = record.start_time ?? "" event.endTime = record.end_time ?? "" event.location = record.location ?? "" event.fee = record.fee ?? 0 event.userID = record.user_id event.createdAt = record.created_at return event - } - }🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: Expected an expression but instead found ':'.
Expected an expression here.
(parse)
[error] 3-22: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/backend/user/Controllers/UserController.ts (2)
95-96: Redundant.limit(1)before.single()The
.single()method already retrieves a single record and returns it as an object. Using.limit(1)is redundant.Apply this diff to remove the unnecessary
.limit(1):.eq("uuid", userID) .single() - .limit(1)
33-39: Provide default values for optional parametersThe optional parameters
orderDescending,rangeStart, andrangeEnddo not have default values. This may lead to unexpected behavior if they areundefined.Apply this diff to set default values:
-public async getUsers( - fields: string, - orderBy?: string, - orderDescending?: boolean, - rangeStart?: number, - rangeEnd?: number -) : Promise<Array<User> | null> { +public async getUsers( + fields: string, + orderBy?: string, + orderDescending: boolean = false, + rangeStart: number = 0, + rangeEnd: number = 100 +) : Promise<Array<User> | null> {src/backend/user/Services/UserService.ts (1)
3-24: Refactor static-only class into a module with functionsThe
UserServiceclass contains only a static method and no instance members. It's more conventional in TypeScript to use standalone functions or an object when only static members are present.Apply this refactor to convert the class into a module with an exported function:
-export default class UserService { - - /** - * Convert a user from default Supabase type to User entity - * - * @param {DBUser} record - The record retrieved from Supabase - * @returns {User} - Converted user entity - * - * @author ... - */ - public static parseUser(record: DBUser) : User { - const user = new User() - user.id = record.uuid - user.username = record.name - user.email = record.fk_email - user.identity = record.fk_identity - user.avatar = record.avatar - - return user - } - -} +/** + * Convert a user from default Supabase type to User entity + * + * @param {DBUser} record - The record retrieved from Supabase + * @returns {User} - Converted user entity + * + */ +export function parseUser(record: DBUser) : User { + const user = new User() + user.id = record.uuid + user.username = record.name + user.email = record.fk_email + user.identity = record.fk_identity + user.avatar = record.avatar + + return user +}🧰 Tools
🪛 Biome (1.9.4)
[error] 3-24: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/backend/user/Entities/User.ts (1)
21-21: Reconsider default value forjoinedAtpropertyInitializing
joinedAtwithnew Date()sets it to the current date, which may not reflect the user's actual join date from the database. Consider setting the default value tonullor handling the assignment during data parsing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/backend/event/Controllers/EventController.ts(1 hunks)src/backend/event/Entities/Event.ts(1 hunks)src/backend/event/Services/EventService.ts(1 hunks)src/backend/user/Controllers/UserController.ts(1 hunks)src/backend/user/Entities/User.ts(1 hunks)src/backend/user/Services/UserService.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/backend/user/Services/UserService.ts
[error] 3-24: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/backend/event/Services/EventService.ts
[error] 15-15: Expected an expression but instead found ':'.
Expected an expression here.
(parse)
[error] 3-22: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (2)
src/backend/event/Entities/Event.ts (1)
1-22: Code looks good
The Event class and DBEvent type are well-defined with appropriate default values. This ensures that event objects are consistently initialized.
src/backend/user/Controllers/UserController.ts (1)
46-47: Confirm default sorting order when orderDescending is undefined
When orderDescending is undefined, !orderDescending evaluates to true, resulting in ascending order. Ensure this aligns with the intended default sorting order.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/backend/event/Services/EventService.ts (1)
5-20: Add JSDoc documentation for the parsing method.The method would benefit from clear documentation explaining the transformation logic and default values.
Add documentation:
+ /** + * Converts a database event record to an Event entity + * @param record - The database event record + * @returns An Event entity with validated and transformed data + * @throws Error if required fields are missing + */ public static parseEvent(record: DBEvent) : Event {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/backend/event/Services/EventService.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/backend/event/Services/EventService.ts
[error] 3-22: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (1)
src/backend/event/Services/EventService.ts (1)
1-1: LGTM! Import follows the architectural pattern.
The import statement correctly follows the Clean Architecture pattern by importing both the domain entity (Event) and its database representation (DBEvent).
Extra checkings have been added to service and controller methods to ensure type safety and null safety, as recommended by coderabbit. (2024/12/21)
924f5ef
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/backend/event/Services/EventService.ts (1)
3-25: Well-implemented Clean Architecture service layer.The service effectively fulfills its role in the Clean Architecture by:
- Converting database types (DBEvent) to domain entities (Event)
- Handling data validation and transformation
- Maintaining clear separation of concerns
Consider adding JSDoc comments to document the service's role in the Clean Architecture and any assumptions about the data transformation process.
src/backend/user/Services/UserService.ts (1)
5-12: Enhance method documentation with @throws and @exampleThe documentation is good but could be more comprehensive by including error scenarios and usage examples.
Consider adding:
/** * Convert a user from default Supabase type to User entity * * @param {DBUser} record - The record retrieved from Supabase * @returns {User} - Converted user entity + * @throws {Error} - When uuid field is missing + * @example + * const dbUser = { uuid: '123', name: 'John', ... } + * const user = UserService.parseUser(dbUser) * * @author Henry C. (@yeahlowflicker) */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/backend/event/Controllers/EventController.ts(1 hunks)src/backend/event/Services/EventService.ts(1 hunks)src/backend/user/Controllers/UserController.ts(1 hunks)src/backend/user/Services/UserService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend/user/Controllers/UserController.ts
- src/backend/event/Controllers/EventController.ts
🔇 Additional comments (3)
src/backend/event/Services/EventService.ts (2)
1-3: LGTM! Good choice of using object literal pattern.
The object literal pattern is a clean and maintainable approach, avoiding the complexity of class-based implementations.
11-19: LGTM! Good handling of types and optional fields.
The implementation shows good practices:
- Proper type checking for numeric fields
- Consistent date format using ISO strings
- Appropriate default values for optional fields
src/backend/user/Services/UserService.ts (1)
1-1: Verify User entity structure matches database schema
Let's ensure the User entity fields align with the database schema and are used consistently across the codebase.
✅ Verification successful
Let me gather more information about the database schema by checking the DBUser type and any SQL schema definitions.
Let me gather one final piece of information about the database types to complete the verification.
Based on the gathered information, I can now provide a final response:
✓ User entity structure aligns with database schema
The User entity fields match the members table schema from the SQL dump:
- The
DBUsertype is correctly mapped to the members table Row type - User entity fields (id, username, email, phone, avatar, etc.) correspond to the members table columns
- The
parseUsermethod in UserService properly handles the conversion between database and entity models
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify User entity structure and usage
# Check User entity structure
echo "Checking User entity structure..."
ast-grep --pattern 'class User {
$$$
}'
# Check for User entity usage patterns
echo "Checking User entity usage..."
rg -A 5 "new User\(\)"
# Check for parseUser method usage
echo "Checking parseUser usage..."
rg -A 5 "parseUser\("
Length of output: 3555
Script:
#!/bin/bash
# Check DBUser type definition
echo "Checking DBUser type definition..."
ast-grep --pattern 'type DBUser = {
$$$
}'
# Look for SQL schema files
echo "Looking for SQL schema files..."
fd -e sql -x cat {}
# Check for database-related type definitions
echo "Checking database-related types..."
rg -A 10 "interface.*User|type.*User"
Length of output: 29924
Script:
#!/bin/bash
# Check Database type definitions
echo "Checking Database type definitions..."
rg -A 5 "type Database = {"
# Check members table structure
echo "Checking members table fields..."
rg -A 10 "members.*Row.*="
Length of output: 316
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/backend/user/Services/UserService.ts (1)
1-3: 🛠️ Refactor suggestionConsider implementing UserService as a class
The current object literal pattern doesn't fully align with Clean Architecture principles.
src/backend/event/Services/EventService.ts (1)
5-10:⚠️ Potential issueAdd validation for userID as a required field.
According to the PR objectives, both
idanduserIDare required fields.if (!record.id) throw new Error('id is a required field') +if (!record.user_id) + throw new Error('user_id is a required field')
🧹 Nitpick comments (4)
src/backend/user/Services/UserService.ts (2)
5-12: Enhance method documentation with error casesThe documentation should include
@throwssections to document the error conditions:
- When record is invalid/null
- When uuid is missing
/** * Convert a user from default Supabase type to User entity * * @param {DBUser} record - The record retrieved from Supabase * @returns {User} - Converted user entity + * @throws {Error} - When record is invalid or null + * @throws {Error} - When uuid is missing * * @author Henry C. (@yeahlowflicker) */
1-33: Consider Clean Architecture best practicesTo better align with Clean Architecture principles:
- Define an interface
IUserServiceto abstract the service layer- Implement dependency injection to improve testability
- Consider adding a repository layer to handle data access
Example interface:
interface IUserService { parseUser(record: DBUser): User; // Add future methods here }src/backend/event/Services/EventService.ts (2)
23-23: Consider using current timestamp as default for createdAt.When
created_atis null/undefined, it might be more appropriate to use the current timestamp instead of an empty string, as this field typically represents the record creation time.-event.createdAt = record.created_at ? new Date(record.created_at).toISOString() : "" +event.createdAt = record.created_at ? new Date(record.created_at).toISOString() : new Date().toISOString()
6-7: Consider integrating with ErrorHandler utility.Since the PR introduces an
ErrorHandlerutility for universal error handling, consider using it here instead of throwing raw Error objects. This would ensure consistent error handling across the application.if (!record || typeof record !== 'object') - throw new Error('Invalid record provided') + throw ErrorHandler.createError('INVALID_RECORD', 'Invalid record provided')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/backend/event/Services/EventService.ts(1 hunks)src/backend/user/Services/UserService.ts(1 hunks)
🔇 Additional comments (3)
src/backend/user/Services/UserService.ts (1)
13-29: Add comprehensive validation and sanitization
The current implementation only validates the uuid field.
Let's verify if the User entity provides default values for optional fields:
#!/bin/bash
# Description: Check User entity implementation for default values
# Search for User class definition and constructor
ast-grep --pattern 'class User {
$$$
constructor() {
$$$
}
$$$
}'src/backend/event/Services/EventService.ts (2)
1-3: LGTM! Good implementation of the service pattern.
The object literal pattern is a clean and maintainable approach for implementing services in TypeScript.
25-30: Verify service integration with controllers.
The implementation looks good. Let's verify that this service is properly imported and used in the controllers.
✅ Verification successful
Service integration is properly implemented
The EventService is correctly imported and utilized in the EventController. The parseEvent method is used appropriately in both getEvents and findEventByID methods for converting database events (DBEvent) to the Event type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for EventService imports and usage in controllers
rg -l "import.*EventService" src/
rg "EventService\.parseEvent" src/
Length of output: 327
Script:
#!/bin/bash
# Let's check the actual implementation in EventController to verify correct usage
ast-grep --pattern 'class EventController {
$$$
}'
Length of output: 8717
引入 Clean Architecture 概念，包含 Entity, Controller 和 Service：
每個 Entity 內會多含一個 DBXXXX 的 type，如 User 有 DBUser，後者是 Supabase 產生的 type，會透過 Service 轉成正常的 entity type。
ErrorHandler是通用的錯誤處理器，供後續作 error display、狀態更新使用。其餘細節、用法請參閱各 function 註解。
(2024/12/14)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation