Feature/use drizzle relationships #114
Conversation
Hajbo
force-pushed
the
feature/use-drizzle-relationships
branch
from
b71f0a8 to
2297f5a
Compare
WalkthroughThis update introduces documentation improvements, database migration enhancements, and refactors to the ORM-based database access layer. The README now includes testing instructions. Database scripts have been modified to support dropping multiple tables and resetting constraints on the user_follows table, along with new migration snapshot and journal entries. The provider, repository, schema, and service modules for profiles and users have been updated to use combined schemas and enhanced query methods, with type and signature changes that simplify relations and data retrieval. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProfilesService
participant ProfilesRepository
participant Database
Client->>ProfilesService: Request profile (targetUsername, currentUserId)
ProfilesService->>ProfilesRepository: findByUsername(targetUsername)
ProfilesRepository->>Database: Query users with username filter and related followers
Database-->>ProfilesRepository: Return user profile data
ProfilesRepository-->>ProfilesService: Return profile data
ProfilesService->>ProfilesService: Generate profile response (check if currentUserId is among followers)
ProfilesService-->>Client: Return profile response
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 2
🧹 Nitpick comments (4)
src/profiles/profiles.schema.ts (1)
8-9: Profile type adjustments
DefiningProfileasStatic<typeof SelectUserSchema>plus afollowersarray is straightforward. Confirm thatFollowerSchema[]provides the fields you need for any profile-based logic; if fields are missing, consider extending it.If some profiles might have zero followers, consider specifying the array as optional if that usage pattern arises.
src/profiles/profiles.service.ts (1)
64-66: Updated following check to use the new relationship structure.This change properly leverages the new relationship structure by checking if any of the user's followers match the currentUserId.
However, this approach requires iterating through all followers, which could be inefficient with many followers.
Consider optimizing this check if performance becomes an issue:
- following: user.followers.some( - (follower) => follower.follower_id === currentUserId, - ), + following: Boolean(user.followers.find( + (follower) => follower.follower_id === currentUserId, + )),Both approaches work, but
find()will stop iterating once it finds a match, whilesome()will continue until it processes all elements or finds a match.db/migrations/meta/0005_snapshot.json (2)
7-69: User Follows Table: Structure and Relationships Verified.
Theuser_followstable is correctly defined with the necessary columns, a composite primary key, and two foreign key constraints that both correctly specifyonDelete: cascadeto maintain referential integrity.It might be beneficial to consider adding explicit indexes on
user_idandfollower_idif performance issues arise during JOIN queries, though the composite primary key may already cover this.
70-145: Users Table Definition is Robust.
Theuserstable includes all essential fields with appropriate data types and constraints. The unique constraints onusernamereinforce data integrity. Additionally, the default values (for example, setting default dates withCURRENT_DATEand providing defaults forbioandimage) are properly configured.Consider verifying that the default empty string for
bioand the provided default URL forimagealign with your business logic and presentation needs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (14)
README.md(1 hunks)db/drop.ts(1 hunks)db/migrations/0005_little_paladin.sql(1 hunks)db/migrations/meta/0005_snapshot.json(1 hunks)db/migrations/meta/_journal.json(1 hunks)package.json(1 hunks)src/database.providers.ts(1 hunks)src/profiles/profiles.repository.ts(1 hunks)src/profiles/profiles.schema.ts(2 hunks)src/profiles/profiles.service.ts(3 hunks)src/users/users.model.ts(2 hunks)src/users/users.repository.ts(2 hunks)src/users/users.schema.ts(2 hunks)src/users/users.service.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/users/users.service.ts
🔇 Additional comments (34)
package.json (1)
29-29: New dependency added: commanderThe addition of the Commander.js package is appropriate for enhancing command-line interface capabilities. This aligns with the PR objective of utilizing Drizzle relationships and potentially supporting the test bash command mentioned in the PR summary.
db/migrations/meta/_journal.json (1)
39-46: New migration entry looks goodThe addition of a new migration entry "0005_little_paladin" with proper versioning is consistent with the PR objective of generating migrations with foreign keys for Drizzle relationships.
README.md (1)
50-54: Well-documented test instructionsThe addition of clear test instructions with the appropriate command is valuable for developers. This directly fulfills the PR objective of adding a test bash command to the README.
src/users/users.schema.ts (2)
3-3: Updated import to include userFollowsThe import statement has been correctly updated to include the
userFollowstype from the users model, which is necessary for the relationship improvements described in the PR.
52-52: New FollowerSchema export typeThe addition of the
FollowerSchematype properly exposes the inferred select type from theuserFollowstable. This enhances type safety when working with user relationships and supports the PR's goal of utilizing Drizzle relationships.src/database.providers.ts (2)
2-2: Namespace import looks good
ImportingusersSchemavia a namespace is a neat way to keep all exported members in a single object. No issues found here.
10-14:❓ Verification inconclusive
Schema composition and typed database instantiation
Combining multiple schemas intoCombinedSchemasand defining aDatabaseSchematype helps maintain consistency across repository classes. This approach is easily extensible if new schemas are added in the future.Would you like to verify that all
dbreferences now align with theDatabaseSchematype? If so, you can run:
🏁 Script executed:
#!/bin/bash rg "import { db }" -A 5Length of output: 130
Updated
dbReference Verification CommandThe schema composition and typed instantiation in
src/database.providers.ts(lines 10–14) look solid and maintain the extensibility intended for future schema additions. However, the suggested verification command produced a regex error because the curly braces were not escaped. To verify that all references todbalign with theDatabaseSchematype, please run the updated command below:#!/bin/bash rg 'import \{ db \}' -A 5This updated command escapes the curly braces correctly. Once run, manually verify that every
dbimport and usage across the repository aligns with the definedDatabaseSchematype.db/migrations/0005_little_paladin.sql (2)
6-8: Reintroducing constraints with cascading deletes
Defining a composite primary key on(user_id, follower_id)and referencing theuserstable withON DELETE CASCADEensures relational integrity and automatic cleanup of user-follows relationships. This aligns with the PR’s goal of properly cascading deletes.
1-5:❓ Verification inconclusive
Dropping old constraints
Removing the existing foreign key and composite constraints is necessary before reintroducing them with the updated primary key and cascading rules. Ensure no existing data violates these constraints.
🏁 Script executed:
#!/bin/bash # Check for any records in user_follows that might conflict with the new constraints. echo "Verifying user_follows data..." # If there's referencing rows with missing user IDs, foreign key addition will fail. cat db/migrations/*.sql | grep -A 5 user_followsLength of output: 2284
Constraint Update – Verify Data Integrity for Safe Migration
The migration in
db/migrations/0005_little_paladin.sqlcorrectly drops the old foreign key and composite primary key constraints and reintroduces them with updated cascading rules. However, the verification script output only confirms that the expected SQL commands are present in the migration files—it does not validate that the actual data inuser_followscomplies with these constraints. Please ensure that:
- All records in
user_followshave valid, non-nulluser_idandfollower_idvalues.- No orphaned or conflicting records exist that might cause the new foreign key constraints (with
ON DELETE cascade) to fail in a live environment.- Adequate testing or additional data validation queries are performed in your staging/test environment before deploying the migration to production.
src/profiles/profiles.schema.ts (2)
1-6: Updated imports to match new user schema
Bringing inSelectUserSchema,FollowerSchema, andinsertUserSchemaRawfrom@users/users.schemaensures the profile schema accurately reflects the new relationship definitions.
25-25:✅ Verification successful
Introduction of ParsedProfileSchema
UsingStatic<typeof ReturnedProfileSchema>for broader typing is a clean approach. It helps maintain type safety across the codebase.If you’d like to confirm references to
ParsedProfileSchema, run:
🏁 Script executed:
#!/bin/bash rg -A 5 "ParsedProfileSchema"Length of output: 914
Verified: Clean Introduction of ParsedProfileSchema
The type definition using
Static<typeof ReturnedProfileSchema>is correctly implemented insrc/profiles/profiles.schema.tsand is clearly referenced insrc/profiles/profiles.service.ts(both in the import statement and in function return types). This confirms the intended usage and maintains type safety across the code.
- Definition confirmed:
src/profiles/profiles.schema.tsdefinesParsedProfileSchemaas intended.- Usage confirmed: References in
src/profiles/profiles.service.tsvalidate that the type is actively utilized.src/profiles/profiles.service.ts (8)
2-2: Added ParsedProfileSchema type import for better type safety.Good addition - using a strongly typed schema for the profile response ensures consistency across the application and prevents potential type mismatches.
13-13: Simplified findByUsername call by removing unnecessary parameter.The removal of the
currentUserIdparameter from the repository call is appropriate since the relationship data is now fetched through Drizzle's relational queries instead of manual filtering.
17-17: Updated method call to match new parameter requirements.Correctly passing the
currentUserIdto thegenerateProfileResponsemethod to maintain the ability to determine follower relationships while leveraging the new database structure.
29-30: Simplified repository call in followUser method.The method now correctly uses the simplified repository call that leverages Drizzle relationships instead of manually passing the current user ID.
35-35: Updated generateProfileResponse call to include currentUserId.This change maintains the functionality to check if the current user is following the profile, while taking advantage of the new relationship structure.
39-39: Simplified repository call in unfollowUser method.Similar to the followUser method, this change correctly removes the unnecessary currentUserId parameter from the repository call.
Also applies to: 46-47
52-52: Updated generateProfileResponse call to include currentUserId.Correctly passing the currentUserId to the method for consistent behavior across all profile response generation.
55-58: Enhanced generateProfileResponse method signature with explicit types.Good improvement to the method signature - it now clearly shows the requirements (currentUserId as a parameter) and the return type (Promise), making the code more maintainable and self-documenting.
src/profiles/profiles.repository.ts (4)
1-1: Updated import to use DatabaseSchema type.Good change - using a specific DatabaseSchema type instead of PostgresJsDatabase provides better type safety and aligns with the PR objective of using a schema for the Drizzle object.
3-3: Added imports for and, eq from drizzle-orm.These imports are necessary for the updated query syntax using Drizzle ORM. The
eqoperator will be used for equality conditions in the queries.
6-6: Updated constructor to use DatabaseSchema type.This change aligns with the other repositories and ensures consistent type usage across the application.
8-12: Refactored findByUsername to leverage Drizzle relations.The method has been nicely refactored to use Drizzle's query builder with relations. This provides several benefits:
- It's more declarative and easier to understand
- It leverages the ORM's ability to handle relationships
- It removes the need for manual joins or additional queries
The
with: { followers: true }ensures that the follower relationships are eager-loaded, which is needed for the profile generation logic.src/users/users.model.ts (3)
1-1: Added relations import from drizzle-orm.Good addition - this is necessary for defining the relationship mappings between entities in the Drizzle ORM context.
24-27: Added userRelations definition.This is a key implementation of the PR objective - properly defining the relationships between users and their followers/following. The many-to-many relationship is correctly modeled through the userFollows junction table.
33-33: Added cascade delete behavior to foreign key relationships.Excellent addition - implementing cascading deletes ensures referential integrity in the database. When a user is deleted, all their follower/following relationships will be automatically removed, preventing orphaned records.
Also applies to: 36-36
src/users/users.repository.ts (6)
1-1: Updated import to use DatabaseSchema type.Good change - using the specific DatabaseSchema type instead of PostgresJsDatabase aligns with the PR objective of using a schema for the Drizzle object and provides better type safety.
7-7: Updated constructor to use DatabaseSchema type.This ensures consistency with other repositories and properly implements the schema-based approach.
16-20: Refactored findById with simplified error handling.The method now uses a cleaner query approach with Drizzle's
findFirstand has simplified error handling. This is more concise and readable.Notice that error handling has changed from checking array length to checking if the result is falsy. This is fine, but it's a change in pattern compared to other methods.
24-26: Updated findByEmail method to use Drizzle query builder.The method now properly uses Drizzle's query builder while maintaining the same error handling logic as before.
37-39: Updated findByUsername method to use Drizzle query builder.The query has been properly updated to use Drizzle's query builder while maintaining the same error handling logic.
10-12:✅ Verification successful
Enhanced findAll method with relationship loading.
The method now properly uses Drizzle's query builder with relationship loading. This is a good implementation of the PR objective to leverage the relationships.
However, be aware that loading all followers and following relationships for all users could be inefficient with large datasets.
Consider whether eager loading all relationships is always necessary or if it could be made optional based on the use case:
🏁 Script executed:
#!/bin/bash # Check where findAll is called and how the results are used rg -A 5 "findAll" src/Length of output: 333
Action: Confirm relationship loading implementation and verify potential performance implications
The enhanced implementation of the
findAllmethod now correctly leverages Drizzle’s query builder to eagerly load bothfollowersandfollowing. The usage appears consistent based on our repository search—findAllis defined and used only insrc/users/users.repository.ts. That said, please keep in mind that eager loading all these relationships for every user might negatively impact performance as your dataset grows. Consider evaluating whether:
- Eager loading of relationships is necessary for all use cases, or
- Making the relationship loading configurable (e.g., optionally enabled or paginated) might be more efficient for large datasets.
Overall, great work on aligning the implementation with the PR objective.
db/migrations/meta/0005_snapshot.json (2)
1-6: Database Snapshot Metadata is Well-Formed.
The snapshot metadata (id, prevId, version, and dialect) is clearly defined and conforms to expected JSON schema standards.
146-159: Supplementary Migration Snapshot Sections are in Place.
The empty objects for enums, schemas, sequences, roles, policies, views, and the_metafield indicate a forward-compatible structure, ensuring that future schema extensions can be incorporated without breaking the existing migration snapshot.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
db/drop.ts (2)
10-17: Consider adding transaction support for related table operations.When deleting data from related tables, using a transaction would ensure that either all deletions succeed or none do, preserving data integrity if an error occurs mid-process.
-for (const table of tables) { - const name = getTableName(table); - console.log(`Dropping ${name}`); - await db.delete(table); - console.log(`Dropped ${name}`); - const tableResult = await db.select().from(table); - console.log(`${name} result: `, tableResult); -} +// Use a transaction to ensure all deletions succeed or none do +await db.transaction(async (tx) => { + for (const table of tables) { + const name = getTableName(table); + console.log(`Dropping ${name}`); + await tx.delete(table); + console.log(`Dropped ${name}`); + const tableResult = await tx.select().from(table); + console.log(`${name} result: `, tableResult); + } +});
8-21: Consider adding error handling.The script would benefit from try-catch blocks to gracefully handle database operation failures, especially when dealing with multiple tables and potential foreign key constraints.
-console.log('Dropping the entire database'); - -for (const table of tables) { - const name = getTableName(table); - console.log(`Dropping ${name}`); - await db.delete(table); - console.log(`Dropped ${name}`); - const tableResult = await db.select().from(table); - console.log(`${name} result: `, tableResult); -} - -console.log('Database dropped'); - -exit(0); +console.log('Dropping the entire database'); + +try { + for (const table of tables) { + const name = getTableName(table); + console.log(`Dropping ${name}`); + try { + await db.delete(table); + console.log(`Dropped ${name}`); + const tableResult = await db.select().from(table); + console.log(`${name} result: `, tableResult); + } catch (error) { + console.error(`Error dropping ${name}:`, error); + } + } + + console.log('Database dropped'); + + exit(0); +} catch (error) { + console.error('Failed to drop database:', error); + exit(1); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
db/drop.ts(1 hunks)src/profiles/profiles.service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/profiles/profiles.service.ts
🔇 Additional comments (3)
db/drop.ts (3)
3-3: Import now includes userFollows model.The import statement has been updated to include both
userFollowsandusersfrom the users model, aligning with the PR's focus on implementing proper relationships with cascading deletes using Drizzle.
6-6: Tables array expanded to include userFollows.Good update to include both
usersanduserFollowstables in the drop operation, supporting the multi-table cascade deletion implementation.
13-13: Correctly addressed previous review feedback.The code now correctly uses
db.delete(table)instead ofdb.delete(users), ensuring that all tables in the array are properly deleted. This fixes the issue identified in the previous review.
- use camelCase for model objects (keep sanke_case column names) - rename user_id to followed_id - enable logging for drizzle
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/database.providers.ts (1)
10-15: Consider disabling the logger for production environments.
logger: truecan be convenient for debugging but may clutter logs in production. You can manage this setting from a config variable to allow easy on/off toggling.src/profiles/profiles.repository.ts (1)
19-25: Consider adding error handling for duplicate follow attemptsThe current implementation doesn't handle the case where a user tries to follow someone they already follow, which could result in database constraint violations.
async followUser(currentUserId: number, userToFollow: number) { + // Check if already following + const existing = await this.db.query.userFollows.findFirst({ + where: and( + eq(userFollows.followedId, userToFollow), + eq(userFollows.followerId, currentUserId) + ) + }); + + if (existing) { + return existing; + } + const result = await this.db .insert(userFollows) .values({ followedId: userToFollow, followerId: currentUserId }) .returning(); return result[0]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
db/drop.ts(1 hunks)db/migrations/0006_goofy_diamondback.sql(1 hunks)db/migrations/meta/0006_snapshot.json(1 hunks)db/migrations/meta/_journal.json(1 hunks)src/database.providers.ts(1 hunks)src/profiles/profiles.repository.ts(3 hunks)src/profiles/profiles.schema.ts(2 hunks)src/profiles/profiles.service.ts(3 hunks)src/users/users.model.ts(2 hunks)src/users/users.schema.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/users/users.schema.ts
- src/profiles/profiles.service.ts
🔇 Additional comments (18)
db/migrations/meta/_journal.json (1)
39-53: Validate that the top-level version aligns with the newly introduced version.The top-level field at line 2 still shows
"version": "5", whereas these newly added entries indicate version 7. Please verify that this is intentional and confirm if the file’s top-level version should also be incremented.src/database.providers.ts (1)
2-2: Import path looks valid.No concerns here. The path
@users/users.modelis consistent with your project structure.db/migrations/0006_goofy_diamondback.sql (1)
1-6:❓ Verification inconclusive
Double-check the ON UPDATE clause and rename references throughout the codebase.
Currently,
ON UPDATE no actionmight unintentionally leave related records stale if an ID changes. Confirm whether you intend a surface-level rename or require a thorough reference update to replaceuser_idwithfollowed_idthroughout your code.
🏁 Script executed:
#!/bin/bash # Verify any leftover references to "user_id" in the codebase. rg "user_id"Length of output: 4630
Please review the foreign key update behavior and overall consistency of the column renaming.
- In
db/migrations/0006_goofy_diamondback.sql, the column is renamed fromuser_idtofollowed_idand the associated constraints are updated. However, the foreign key still usesON UPDATE no action, which will not propagate updates if a user’s ID changes.- Your earlier migrations (e.g. in
0005_little_paladin.sqland snapshots underdb/migrations/meta/) still referenceuser_idand use the sameON UPDATE no action. Please confirm whether these historical references are intended (for instance, if primary keys remain immutable) or if you need a consistent, thorough update across your migration history.- If the application requires automatic propagation of changes (e.g. via
ON UPDATE CASCADE), you'll need to adjust the clause and update any dependent references accordingly.db/drop.ts (1)
3-28: Transaction-based table drops look robust.This approach resolves the previous concern about deleting from the wrong table. Using a transaction with
DROP TABLE IF EXISTS … CASCADE;ensures consistency and simplifies cleanup.src/profiles/profiles.schema.ts (4)
1-6: Looks good.
The new imports fromusers.schemaneatly align with the revised schemas. No issues found.
8-9: Confirm iffollowersshould be optional or required.
Currently,followersis required onProfile. If there's a scenario where a profile could have no followers, consider making this property optional (e.g.,followers?: FollowerSchema[];) or returning an empty array by default.
18-19: Ensure consistent naming of date fields.
You’re omittingcreatedAtandupdatedAtin this schema. Confirm whether they’re intentionally excluded or if you’d prefer to expose them with custom naming.
24-25: Type alias naming looks fine.
ParsedProfileSchemais a clear name for referencing the static version of the returned profile. No concerns here.src/users/users.model.ts (4)
20-21: Date columns with defaults look correct.
SettingcreatedAtandupdatedAtto default toCURRENT_DATEis a common approach, although some prefer timestamps with timezone for higher precision.
24-27: Many-to-many relationship definition is valid.
Usingmany(userFollows, { relationName: 'followed' })andmany(userFollows, { relationName: 'follower' })inuserRelationsis correct for representing that each user can have multiple followers and is following multiple users.
32-36: Cascade deletes may have broad impact.
When a user is deleted, all related follow records will be removed. Verify this aligns with your business logic; if so, proceed.
44-55: One-to-one references within the join table look correct.
Defining each follow record as referencing exactly onefollowerand onefolloweduser enforces the intended join table logic. Good job matching the role of each foreign key.db/migrations/meta/0006_snapshot.json (1)
1-159: Migration snapshot successfully captures the updated schema.
The composite primary key on(followed_id, follower_id), cascading deletions, and date fields with defaults correctly match the new design. Keep an eye on naming differences (camelCase vs. snake_case) between code and database if you plan to map them directly.src/profiles/profiles.repository.ts (5)
1-1: Import changes align well with the shift to structured queriesThe transition from
PostgresJsDatabasetoDatabaseSchemasupports the PR objective of implementing proper relationships. Adding theand, eqimports from Drizzle ORM enables the structured query syntax used throughout the file.Also applies to: 3-3
6-6: Constructor parameter update correctly implements the type changeThe constructor now uses the
DatabaseSchematype, which is consistent with the imports and aligns with similar changes across the codebase.
22-22: Improved naming convention for database fieldsThe parameter rename from
user_id/follower_idtofollowedId/followerIdmakes the relationship clearer and more semantic.
32-33: Consistent parameter naming in unfollowUser methodThe parameter names have been consistently updated here to match the new naming convention used in the rest of the codebase.
8-12:❓ Verification inconclusive
Good use of structured queries with relationship loading
The refactored
findByUsernamemethod now uses Drizzle's query builder instead of raw SQL, which is more type-safe and maintainable. The inclusion ofwith: { followers: true }effectively leverages the relationship definitions.Consider the performance implications of eagerly loading all followers for users with large follower counts.
🏁 Script executed:
#!/bin/bash # Check if there are any pagination implementations for followers elsewhere in the codebase rg -A 2 "followers.*limit|pagination.*followers" --type tsLength of output: 60
Attention: Verify Follower Loading Performance
The refactored
findByUsernamemethod now efficiently uses Drizzle's query builder with relationship loading. However, our search for any existing pagination or limiting mechanism for followers (via the providedrgcommand) returned no results. This indicates that all followers are eager-loaded, which could raise performance concerns for users with large follower counts. Please verify whether the absence of pagination is acceptable in this context or if further optimization (e.g., implementing pagination or lazy loading) should be considered.
- File:
src/profiles/profiles.repository.ts(lines 8-12)- Concern: Eagerly loading all followers without a limit or pagination mechanism may negatively impact performance for users with large follower bases.
| ]), | ||
| Type.Object({ following: Type.Boolean() }), | ||
| ]), | ||
| }); | ||
|
|
||
| export type ParsedProfileSchema = Static<typeof ReturnedProfileSchema>; |
There was a problem hiding this comment.
I'm not a big fan of "Schema" in a static type name, to me "Schema" implies a runtime type validator (e.g. TypeBox, Zod, ArkType etc.)
We should address the variable names when we reconsider TypeBox (which we should - in 2025 there are better alternatives)
There was a problem hiding this comment.
Sure, we can rename things any time. Does Elysia work with different validators? As far as I remember it's quite tightly coupled with TypeBox, but that might've changed since then
There was a problem hiding this comment.
You're right - ElysiaJS supports TypeBox only, however the developer of TypeBox created a utility to convert Zod and Valibot to TypeBox:
elysiajs/elysia#20 (comment)
So we should discuss whether we want to use this - I vote a big fat "yes"!
There was a problem hiding this comment.
This sounds doable 🤔 I guess we need the drizzle-arktype project for this too, or we can just define types manually instead of generating the schemas from the models
There was a problem hiding this comment.
I vote against a type-first design and in favor of schema-first for 2 reasons:
- Drizzle provides schemas in Zod, TypeBox and Valibot directly from the tables.
- We can use the schemas to produce types, OpenAPI, and validation.
- It reduces SoT.
I'm not sold on any validator though - I opened a discussion for this: #127
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/users/users.repository.ts (4)
24-26: Consider using findFirst for unique email checksWhile using
findManyworks for checking duplicates, since emails should be unique in most systems, consider usingfindFirstto be consistent with thefindByIdmethod and potentially improve performance slightly.async findByEmail(email: string) { - const result = await this.db.query.users.findMany({ + const result = await this.db.query.users.findFirst({ where: eq(users.email, email), }); - if (result.length === 0) { + if (!result) { return null; } - if (result.length > 1) { - throw new Error(`More than one user found with the same email: ${email}`); - } - return result[0]; + return result; }
37-39: Consider using findFirst for unique username checksSimilar to the email lookup, since usernames should be unique, consider using
findFirstinstead offindManyto be consistent with thefindByIdmethod and potentially improve performance slightly.async findByUsername(username: string) { - const result = await this.db.query.users.findMany({ + const result = await this.db.query.users.findFirst({ where: eq(users.username, username), }); - if (result.length === 0) { + if (!result) { return null; } - if (result.length > 1) { - throw new Error( - `More than one user found with the same username: ${username}`, - ); - } - return result[0]; + return result; }
51-59: Consider updating createUser to use Drizzle's new query styleWhile the current implementation works, for consistency with the other methods, consider updating this method to use the new query style introduced in this PR.
61-68: Consider updating updateUser to use Drizzle's new query styleSimilar to the createUser method, consider updating this method to use the new query style for consistency across the repository.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/database.providers.ts(1 hunks)src/profiles/profiles.repository.ts(3 hunks)src/users/users.repository.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/database.providers.ts
- src/profiles/profiles.repository.ts
🔇 Additional comments (4)
src/users/users.repository.ts (4)
1-1: Well-structured database importsThe import change from
PostgresJsDatabasetoDatabasereflects a move to a more abstracted database type that likely includes the schema configurations mentioned in the PR objectives. This aligns with the goal of implementing proper relationships in Drizzle.
7-7: Constructor signature update looks goodThe parameter type change from
PostgresJsDatabasetoDatabaseis consistent with the import change and supports the schema enhancements in this PR.
10-12: Great relationship implementationThe new implementation properly utilizes Drizzle's relationship capabilities to include followers and following data in the query results. This is a significant improvement over the previous SQL-like syntax and directly supports the PR's objective of implementing proper relationships.
16-20: Improved query and error handlingThe
findByIdmethod now usesfindFirstwith a direct equality check, which is more appropriate for unique ID lookups. The error handling has also been improved by checking if the result is falsy rather than checking for an empty array.
|
@yamcodes Is there a setting to keep approves even if the branch gets updated? It's annoying that the review gets dismissed when I update to main - which I'm forced to do. |
|
Should be fixed now @Hajbo, try |
Description
PR Checklist (Please do not remove)
type(scope): descriptionortype: descriptionformatbun docsSummary by CodeRabbit
Documentation
New Features
Refactor