Conversation
WalkthroughThis pull request expands the database functionality and application features by introducing comprehensive tag management. It adds new tables and relationships for tags and article tags, updates migration scripts and schema snapshots, and refactors the articles module to remove the legacy tag list. A new tags module along with its repository, service, plugin, and schema definitions have been implemented. Additionally, the app module and TypeScript configuration have been updated to integrate the new tag functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant AS as ArticlesService
participant TS as TagsService
participant TR as TagsRepository
participant DB as Database
C->>AS: Create Article with tagList
AS->>DB: Insert article data
AS->>TS: upsertArticleTags(articleId, tagList)
TS->>TR: upsertArticleTags for tags
TR->>DB: Execute upsert for tags
DB-->>TR: Success
TR-->>TS: Tags upserted
TS-->>AS: Tag management complete
AS-->>C: Respond with article (including tags)
sequenceDiagram
participant Client as Client
participant TP as TagsPlugin
participant TS as TagsService
participant TR as TagsRepository
participant DB as Database
Client->>TP: GET /tags
TP->>TS: getTags()
TS->>TR: Retrieve all tags
TR->>DB: Query tags table
DB-->>TR: Return tags
TR-->>TS: Tags data
TS-->>TP: Format tags
TP-->>Client: Return tag list
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 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: 3
🧹 Nitpick comments (9)
src/articles/articles.module.ts (2)
19-19: Fix typo in variable nameThere's a typo in the variable name:
tagsRepositryshould betagsRepository.- const tagsRepositry = new TagsRepository(db); + const tagsRepository = new TagsRepository(db);
24-24: Update variable reference after fixing the typoAfter fixing the typo in the repository variable name, make sure to update the reference in the service instantiation.
- const tagsService = new TagsService(tagsRepositry); + const tagsService = new TagsService(tagsRepository);src/tags/tags.model.ts (1)
11-15: Consider using surrogate key for tags tableUsing the tag name as the primary key might be problematic if tag names need to be updated in the future. Consider using a surrogate key (like an auto-incrementing ID) as the primary key and adding a unique constraint on the name column.
export const tags = pgTable('tags', { - name: text('name').primaryKey(), + id: serial('id').primaryKey(), + name: text('name').notNull().unique(), createdAt: timestamp('created_at').defaultNow().notNull(), updatedAt: timestamp('updated_at').defaultNow().notNull(), });With this approach, you would also need to update the related tables and relationships. This design is more flexible for future changes and internationalization.
db/migrations/0008_tough_wolfsbane.sql (2)
18-22: Consider adding a unique index on the tag name for faster lookups.While the primary key constraint ensures uniqueness, adding an index on the tag name could improve query performance since tags will likely be frequently queried by name.
CREATE TABLE "tags" ( "name" text PRIMARY KEY NOT NULL, "created_at" timestamp DEFAULT now() NOT NULL, "updated_at" timestamp DEFAULT now() NOT NULL ); +CREATE INDEX idx_tags_name ON "tags" ("name");
34-34: Consider adding a transaction around this migration.This migration performs several related operations (creating tables, seeding data, modifying schema). Wrapping all operations in a transaction would ensure atomicity - if any part fails, all changes are rolled back.
+BEGIN; CREATE TABLE "comments" ( ... ); ... ALTER TABLE "articles" DROP COLUMN "tag_list"; +COMMIT;src/tags/tags.service.ts (2)
17-17: Fix typo in commentThere's a typo in the TODO comment.
- // TODO: use tranaction + // TODO: use transaction
23-23: Fix typo in commentThere's a spelling error in the comment.
- // Delete old tags for the artice + // Delete old tags for the articlesrc/articles/articles.repository.ts (1)
113-113: Fix typo in commentThere's a spelling error in the comment.
- // Case-when is not natively suppoerted yet + // Case-when is not natively supported yetdb/migrations/meta/0008_snapshot.json (1)
1-466: Consider adding indexes for performance optimization.While the schema is well-structured, consider adding indexes for frequently queried fields, especially:
- On article_tags.article_id to improve tag lookup by article
- On article_tags.tag_name to improve article lookup by tag
- On comments.article_id to improve comment lookup by article
Are database indexes recommended for foreign key columns in PostgreSQL?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
db/drop.ts(1 hunks)db/migrations/0008_tough_wolfsbane.sql(1 hunks)db/migrations/meta/0008_snapshot.json(1 hunks)db/migrations/meta/_journal.json(1 hunks)src/app.module.ts(2 hunks)src/articles/articles.model.ts(2 hunks)src/articles/articles.module.ts(2 hunks)src/articles/articles.repository.ts(5 hunks)src/articles/articles.schema.ts(3 hunks)src/articles/articles.service.ts(6 hunks)src/database.providers.ts(2 hunks)src/tags/tags.model.ts(1 hunks)src/tags/tags.module.ts(1 hunks)src/tags/tags.plugin.ts(1 hunks)src/tags/tags.repository.ts(1 hunks)src/tags/tags.schema.ts(1 hunks)src/tags/tags.service.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
src/tags/tags.module.ts (2)
src/tags/tags.repository.ts (1)
TagsRepository(6-62)src/tags/tags.service.ts (1)
TagsService(3-43)
src/tags/tags.schema.ts (1)
src/tags/tags.model.ts (2)
tags(11-15)articleTags(23-36)
src/tags/tags.repository.ts (2)
src/tags/tags.schema.ts (2)
TagToInsert(6-6)ArticleTagToInsert(12-15)src/tags/tags.model.ts (2)
tags(11-15)articleTags(23-36)
db/drop.ts (3)
src/tags/tags.model.ts (2)
articleTags(23-36)tags(11-15)src/users/users.model.ts (1)
users(12-23)src/articles/articles.model.ts (3)
favoriteArticles(44-57)comments(75-86)articles(14-25)
src/tags/tags.plugin.ts (2)
src/tags/tags.module.ts (1)
setupTags(6-12)src/tags/tags.schema.ts (1)
ListTagsResponseSchema(20-22)
src/tags/tags.service.ts (2)
src/tags/tags.repository.ts (1)
TagsRepository(6-62)src/tags/tags.model.ts (2)
tags(11-15)articleTags(23-36)
🔇 Additional comments (57)
tsconfig.json (1)
41-41: Path mapping addition looks good!The addition of the
@tags/*path mapping is consistent with the project's module resolution pattern and will properly support the new tags functionality.db/migrations/meta/_journal.json (1)
61-66: Migration entry looks good!The new migration entry for "0008_tough_wolfsbane" is properly formatted and follows the established pattern with an incremented index.
src/tags/tags.module.ts (1)
1-12: Module setup looks clean and follows project patternsThe tags module implementation correctly initializes the repository and service components, following the established architectural pattern in the project. The dependency injection approach with the database connection is well-implemented.
src/tags/tags.plugin.ts (1)
5-18: API endpoint implementation looks goodThe tags plugin correctly sets up a route group with appropriate metadata and implements a GET endpoint for retrieving tags with proper response schema validation.
src/app.module.ts (2)
10-10: Looks good: Added tagsPlugin importThe import of
tagsPluginis correctly added to support the new tags functionality.
59-63: Looks good: Integrated tagsPlugin into the applicationThe tagsPlugin is properly integrated into the application's API group, following the same pattern as other plugins. The refactoring to multi-line format also improves readability.
src/database.providers.ts (2)
3-3: Looks good: Added tagsSchema importThe import of the tags schema is correctly added to support the new tags functionality.
13-13: Looks good: Updated database schema configurationThe database schema configuration is properly updated to include the tags schema alongside the existing schemas.
src/articles/articles.module.ts (3)
9-10: Looks good: Added TagsRepository and TagsService importsThe necessary imports for the tags functionality are correctly added.
25-29: Looks good: Added tagsService to ArticlesService constructorThe tagsService is correctly added as a parameter to the ArticlesService constructor, enhancing the article functionality with tag support.
30-33:Details
✅ Verification successful
Verify CommentsService functionality
The usersRepository parameter has been removed from the CommentsService instantiation. Please verify that this change doesn't break existing functionality.
🏁 Script executed:
#!/bin/bash # Check if CommentsService constructor is modified to no longer require usersRepository echo "Checking CommentsService constructor definition:" rg -A 3 -B 3 "class CommentsService" src/articles/comments/comments.service.tsLength of output: 490
Verification Complete: CommentsService Constructor is Correctly Updated
The constructor in
src/articles/comments/comments.service.tsnow only requirescommentsRepositoryandprofilesService. The removal of theusersRepositoryparameter matches the intended change and should not break the existing functionality.
- Location Verified:
src/articles/comments/comments.service.ts- Constructor Definition: Accepts only
commentsRepositoryandprofilesServiceas confirmed by the output.- Impact: No additional modifications are required since the functionality aligns with the changes in
src/articles/articles.module.ts.src/tags/tags.model.ts (3)
17-21: Looks good: Tag relations properly definedThe relationship between tags and articles is correctly defined as a many-to-many relationship.
23-36: Looks good: ArticleTags junction table properly definedThe junction table for the many-to-many relationship between articles and tags is well-structured with appropriate foreign key constraints and a composite primary key.
38-49: Looks good: ArticleTags relations properly definedThe relationships between the junction table and the related tables are correctly defined with appropriate field mappings.
db/migrations/0008_tough_wolfsbane.sql (4)
1-8: LGTM: Comments table creation looks good.The comments table is well-structured with appropriate columns and constraints. The foreign key relationships will be properly defined later in the migration.
10-16: LGTM: Article tags junction table is well designed.The article_tags table correctly implements a many-to-many relationship between articles and tags, with an appropriate composite primary key. Using
tag_nameas part of the PK is a good choice for this type of relationship.
24-27: LGTM: Foreign key constraints are properly defined.All foreign key constraints are correctly set up with appropriate cascade delete behavior, which will maintain referential integrity when articles or tags are deleted.
30-32:Details
❓ Verification inconclusive
Handle potential null values in tag_list during migration.
The current implementation assumes that all entries in the tag_list column are non-null. Consider adding a WHERE clause to filter out nulls if they might exist.
🏁 Script executed:
#!/bin/bash # Check if there are any null tag_list values in the articles table psql $DATABASE_URL -c "SELECT COUNT(*) FROM articles WHERE tag_list IS NULL;"Length of output: 160
Manual Verification Required: Check for NULL tag_list values
The migration currently assumes that every
tag_listentry in thearticlestable is non-null. However, our automatic check usingpsqlfailed due to the command not being found, so we couldn’t verify this automatically. Please manually confirm whether any rows withNULLvalues exist in thearticles.tag_listcolumn. If there is any possibility ofNULLvalues, update the migration SQL to include a condition (for example, aWHERE tag_list IS NOT NULLclause) to safely filter them out.
- Verify in an environment with
psqlinstalled or use an appropriate tool to confirm the table’s data.- Update the seeding SQL in file
db/migrations/0008_tough_wolfsbane.sqlif necessary.src/articles/articles.model.ts (2)
11-11: LGTM: Good implementation of the new import.Correctly imports the articleTags table from the tags model to establish the relationship between articles and tags.
39-41: LGTM: Well-structured many-to-many relationship with tags.The relationship with tags is correctly defined as a many-to-many relationship through the articleTags junction table, aligning with the database schema changes in the migration file.
src/articles/articles.service.ts (4)
13-13: LGTM: Good addition of TagsService import.Correctly imports the TagsService type for use in the constructor.
20-20: LGTM: Good dependency injection of TagsService.Properly injects the TagsService as a dependency to handle tag-related operations.
104-106: LGTM: Good documentation of cascade behavior.Clear comment explaining that articleTags will be deleted due to the cascade rule when an article is deleted.
127-127: LGTM: Well-implemented tag mapping for the article response.Correctly maps the tags from the article to the tagList in the response, maintaining backward compatibility with the API.
db/drop.ts (2)
3-6: LGTM: Good update of imports for new tables.Correctly imports the new tables (comments, articleTags, tags) from their respective modules.
9-17: LGTM: Tables arranged in correct order for cascading drops.The tables are arranged in the appropriate order for dropping, with child tables listed before their parent tables to respect foreign key constraints. This ensures a clean removal of all tables without constraint violations.
src/tags/tags.schema.ts (1)
1-22: Type definitions and schema look well-structuredThe schema definitions for tags and article tags are well-organized and follow TypeScript best practices. You're using proper type safety with
Staticfrom typebox, and the schema structure aligns with the database model.src/articles/articles.schema.ts (7)
4-4: Good addition of ArticleTag importThis import correctly sets up the connection to the new tagging system.
12-15: Improved schema structure with Type.CompositeUsing Type.Composite to separate tagList into its own object structure is a good approach. This creates a cleaner separation of concerns between the core article properties and the tag-related functionality.
19-22: Good approach with Omit for tagListUsing Omit to exclude the tagList from ArticleToCreate is appropriate since tags will now be handled separately through the specialized tag service.
25-32: Consistent schema structureThe changes here maintain consistency with the approach used for InsertArticleSchema, creating a clear separation between core article properties and tags.
38-40: Consistent type definitionThis matches the approach used for ArticleToCreate, maintaining consistency throughout the codebase.
54-54: Proper type definition for tagListThe returned article correctly includes tagList as an array of strings.
72-72: Correctly updated ArticleInDb typeThe ArticleInDb type now properly includes tags as an array of ArticleTag objects, reflecting the relationship established in the database model.
src/tags/tags.service.ts (4)
1-5: Good use of dependency injectionThe service properly injects the repository dependency, following good design practices.
6-9: Clean implementation of getTags methodThis method elegantly transforms the database results to return just tag names, making it more suitable for API consumption.
11-14: Concise upsertTags implementationThe transformation from string array to objects is clean and straightforward.
40-42: Appropriate implementation of deleteArticleTagsThis method correctly delegates to the repository without unnecessary complexity.
src/articles/articles.repository.ts (7)
8-9: Good import organizationAdded imports for articleTags and sql are necessary for the new database queries.
87-105: Well-implemented CTE for article tagsThe Common Table Expression for article tags is well-designed and efficiently aggregates tag data. The comment explaining why the HAVING clause needs to repeat the calculation is particularly helpful.
113-120: Well-documented workaround for drizzle-orm limitationThe comment explaining the case-when limitation in drizzle-orm is helpful and provides a good reference to the GitHub issue. The SQL fallback is implemented correctly.
141-144: Correct join to include tag dataThe inner join with the articlesWithTagsCTE ensures that tag data is correctly included in the results.
171-171: Properly updated findBySlug to include tagsIncluding tags in the query options ensures that tag data is retrieved when fetching an article by slug.
188-188: Properly updated findById to include tagsIncluding tags in the query options ensures that tag data is retrieved when fetching an article by ID.
103-104:Details
❓ Verification inconclusive
Consider parameterizing SQL injection risk
Using string interpolation in SQL queries can lead to SQL injection vulnerabilities if not properly handled.
While drizzle-orm likely handles parameter sanitization, verify that the
tagparameter is properly sanitized before using it insqltemplate literals:
🏁 Script executed:
#!/bin/bash # Check if tag parameter is sanitized elsewhere in the codebase # Check for parameter validation in controller/router rg -A 10 -B 10 "tag\?:" --type typescript # Check usage of the parameter in other places rg "tag\s*\?" --type typescript # Check if there are other places where SQL queries use parameters directly rg "sql\`.*\$\{.*\}" --type typescriptLength of output: 221
Action Required: Re-run Verification Script with Correct File Pattern
The initial shell script produced errors because "typescript" wasn’t a recognized file type. Please re-run the verification using a file pattern (e.g. "*.ts") to search for parameter validation and sanitization for the
tagparameter. For example:Once you obtain the output:
- Verify that the
tagparameter is either sanitized or validated upstream (in controllers/middleware) before being used in the SQL template literal.- Confirm that drizzle-orm’s handling of SQL template literals properly parameterizes or escapes the
tagvalue.If these checks confirm proper handling, you may safely ignore this comment. Otherwise, consider refactoring to use a parameterized approach (e.g. with whitelist validations or explicit escaping for SQL identifiers).
🏁 Script executed:
#!/bin/bash # Check for parameter validation or sanitization for "tag" in TypeScript files echo "Searching for parameter definitions and validations for 'tag':" rg -A 10 -B 10 "tag\s*\?:" -g "*.ts" echo "Searching for SQL template literals with 'tag' usage:" rg -A 10 -B 10 "sql\`.*\$\{.*tag" -g "*.ts"Length of output: 2985
Attention: Please verify that the use of the
tagparameter in SQL queries is adequately protected against injection attacks.
- In
src/articles/articles.repository.ts(lines 103–104), the query uses the expressionwithout explicit in-code sanitization or validation.tag ? sql`${tag} = any(array_agg(article_tags.tag_name))` : sql`true`,- The parameter is defined in both
src/articles/articles.service.tsandsrc/articles/articles.repository.tsas an optional string, but our search did not reveal any dedicated sanitization (e.g., whitelist checks or escaping) upstream before its use.- While drizzle-orm may handle parameter binding and escaping automatically, it is important to confirm that this is indeed the case for identifiers interpolated into SQL template literals.
Please ensure through manual verification or by consulting the drizzle-orm documentation that the
tagparameter is properly sanitized, or refactor the implementation to use parameterized queries (or explicit validation) to mitigate potential SQL injection risks.src/tags/tags.repository.ts (7)
1-5: Imports look good and follow project conventions.The imports are well organized, with clear separation between local project imports and external library imports.
6-8: Class design follows repository pattern correctly.The repository is properly initialized with dependency injection of the database connection, following good practices for maintainability and testability.
9-11: Simple and effective implementation of getTags method.The method correctly uses the drizzle-orm query builder to fetch all tags from the database.
13-22: Good upsert implementation with proper conflict handling.The upsertTags method correctly handles conflicts by only updating the timestamp when a tag already exists. The method returns the result, which is good for potential further processing.
24-28: Straightforward implementation of getArticleTags.The method effectively retrieves article tags by filtering on the articleId using the equality operator.
30-39: Robust implementation of upsertArticleTags with proper composite key handling.The method correctly handles conflicts on the composite key (articleId and tagName) and updates only the timestamp when a record already exists.
41-61: Well-structured deleteArticleTags method with flexible filtering.The implementation correctly:
- Requires articleId to prevent accidental deletion of all tags
- Optionally filters by tagNames when provided
- Uses the proper SQL operators (and, eq, inArray)
- Returns the deleted records
The comment explaining the articleId requirement is helpful.
db/migrations/meta/0008_snapshot.json (5)
1-5: Database snapshot metadata looks correct.The snapshot properly identifies its version, previous snapshot ID, and dialect (PostgreSQL).
219-281: Well-designed article_tags junction table with appropriate constraints.The article_tags table correctly implements:
- A many-to-many relationship between articles and tags
- Proper foreign key constraints with cascade delete behavior
- A composite primary key on article_id and tag_name
- Timestamp fields for auditing purposes
282-314: Tags table implementation is simple and effective.The tags table correctly:
- Uses the tag name as the primary key
- Includes timestamp fields for auditing purposes
- Has no unnecessary fields or constraints
This design choice makes tag names unique across the system, which is appropriate for a tagging system.
252-269: Foreign keys are properly configured with appropriate cascade behavior.The foreign key constraints properly enforce referential integrity:
- article_tags.article_id references articles.id with cascade delete
- article_tags.tag_name references tags.name with cascade delete
This ensures that when an article or tag is deleted, the corresponding article_tags entries are also removed.
86-155: Comments table implementation is complete with proper relationships.The comments table correctly:
- Has appropriate columns including body, article_id, author_id, and timestamps
- Implements foreign key constraints to both articles and users tables
- Uses cascade delete behavior to maintain referential integrity
This implementation aligns well with the PR objective of migrating the comments table.
| // TODO: Add transaction to ensure both or none of the operations are done | ||
| const createdArticle = await this.repository.createArticle(articleToCreate); | ||
| if (!createdArticle) { | ||
| throw new BadRequestError('Article was not created'); | ||
| } | ||
| if (article.tagList) { | ||
| await this.tagsService.upsertArticleTags( | ||
| createdArticle.id, | ||
| article.tagList, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Implement transaction for article and tag creation.
The TODO comment correctly identifies the need for a transaction to ensure both the article creation and tag association are atomic operations. This should be implemented to prevent data inconsistency if one operation fails.
- // TODO: Add transaction to ensure both or none of the operations are done
- const createdArticle = await this.repository.createArticle(articleToCreate);
- if (!createdArticle) {
- throw new BadRequestError('Article was not created');
- }
- if (article.tagList) {
- await this.tagsService.upsertArticleTags(
- createdArticle.id,
- article.tagList,
- );
- }
+ // Use transaction to ensure both article and tags are created atomically
+ const createdArticle = await this.repository.transaction(async (tx) => {
+ const article = await tx.createArticle(articleToCreate);
+ if (!article) {
+ throw new BadRequestError('Article was not created');
+ }
+ if (article.tagList) {
+ await this.tagsService.upsertArticleTags(
+ article.id,
+ article.tagList,
+ tx
+ );
+ }
+ return article;
+ });📝 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.
| // TODO: Add transaction to ensure both or none of the operations are done | |
| const createdArticle = await this.repository.createArticle(articleToCreate); | |
| if (!createdArticle) { | |
| throw new BadRequestError('Article was not created'); | |
| } | |
| if (article.tagList) { | |
| await this.tagsService.upsertArticleTags( | |
| createdArticle.id, | |
| article.tagList, | |
| ); | |
| } | |
| // Use transaction to ensure both article and tags are created atomically | |
| const createdArticle = await this.repository.transaction(async (tx) => { | |
| const article = await tx.createArticle(articleToCreate); | |
| if (!article) { | |
| throw new BadRequestError('Article was not created'); | |
| } | |
| if (article.tagList) { | |
| await this.tagsService.upsertArticleTags( | |
| article.id, | |
| article.tagList, | |
| tx | |
| ); | |
| } | |
| return article; | |
| }); |
| // TODO: Add transaction to ensure both or none of the operations are done | ||
| const { tagList, ...articleData } = article; | ||
| const existingArticle = await this.repository.findBySlug(slug); |
There was a problem hiding this comment.
Implement transaction for article and tag updates.
Similar to the previous comment, a transaction should be implemented for the updateArticle method to ensure both article and tag updates are atomic operations.
- // TODO: Add transaction to ensure both or none of the operations are done
- const { tagList, ...articleData } = article;
+ // Use transaction to ensure both article and tags are updated atomically
+ return await this.repository.transaction(async (tx) => {
+ const { tagList, ...articleData } = article;Then update the remainder of the function to use the transaction:
- await this.repository.updateArticle(
- existingArticle.id,
- { ...articleData, slug: newSlug },
- currentUserId,
- );
-
- if (tagList) {
- await this.tagsService.upsertArticleTags(existingArticle.id, tagList);
- }
-
- return this.findBySlug(newSlug, currentUserId);
+ await tx.updateArticle(
+ existingArticle.id,
+ { ...articleData, slug: newSlug },
+ currentUserId,
+ );
+
+ if (tagList) {
+ await this.tagsService.upsertArticleTags(existingArticle.id, tagList, tx);
+ }
+
+ return this.findBySlug(newSlug, currentUserId);
+ });| async upsertArticleTags(articleId: number, tags: string[]) { | ||
| // TODO: use tranaction | ||
| if (tags.length === 0) return; | ||
|
|
||
| // Ensure every tag exists | ||
| await this.upsertTags(tags); | ||
|
|
||
| // Delete old tags for the artice | ||
| const articleTags = await this.repository.getArticleTags(articleId); | ||
| const tagsToDelete = articleTags | ||
| .filter((tag) => !tags.includes(tag.tagName)) | ||
| .map((tag) => tag.tagName); | ||
| if (tagsToDelete.length > 0) { | ||
| await this.repository.deleteArticleTags({ | ||
| articleId, | ||
| tagNames: tagsToDelete, | ||
| }); | ||
| } | ||
|
|
||
| // Upsert new and existing tags | ||
| const tagsToUpsert = tags.map((tagName) => ({ articleId, tagName })); | ||
| return await this.repository.upsertArticleTags(tagsToUpsert); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement transaction for database consistency
The code includes multiple database operations that should be atomic to ensure data consistency.
The TODO comment correctly identifies the need for a transaction. Implement it to ensure that all database operations (tag creation, deletion, and upserting) either succeed or fail together.
async upsertArticleTags(articleId: number, tags: string[]) {
- // TODO: use tranaction
+ // Use transaction to ensure atomicity
if (tags.length === 0) return;
+ return await this.repository.db.transaction(async (tx) => {
// Ensure every tag exists
- await this.upsertTags(tags);
+ const tagsRepo = this.repository.withTransaction(tx);
+ await tagsRepo.upsertTags(tags.map(name => ({ name })));
// Delete old tags for the artice
- const articleTags = await this.repository.getArticleTags(articleId);
+ const articleTags = await tagsRepo.getArticleTags(articleId);
const tagsToDelete = articleTags
.filter((tag) => !tags.includes(tag.tagName))
.map((tag) => tag.tagName);
if (tagsToDelete.length > 0) {
- await this.repository.deleteArticleTags({
+ await tagsRepo.deleteArticleTags({
articleId,
tagNames: tagsToDelete,
});
}
// Upsert new and existing tags
const tagsToUpsert = tags.map((tagName) => ({ articleId, tagName }));
- return await this.repository.upsertArticleTags(tagsToUpsert);
+ return await tagsRepo.upsertArticleTags(tagsToUpsert);
+ });
}Note: This implementation assumes you have or will add a withTransaction method to your repository class. You'll need to implement this method in the repository class.
📝 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 upsertArticleTags(articleId: number, tags: string[]) { | |
| // TODO: use tranaction | |
| if (tags.length === 0) return; | |
| // Ensure every tag exists | |
| await this.upsertTags(tags); | |
| // Delete old tags for the artice | |
| const articleTags = await this.repository.getArticleTags(articleId); | |
| const tagsToDelete = articleTags | |
| .filter((tag) => !tags.includes(tag.tagName)) | |
| .map((tag) => tag.tagName); | |
| if (tagsToDelete.length > 0) { | |
| await this.repository.deleteArticleTags({ | |
| articleId, | |
| tagNames: tagsToDelete, | |
| }); | |
| } | |
| // Upsert new and existing tags | |
| const tagsToUpsert = tags.map((tagName) => ({ articleId, tagName })); | |
| return await this.repository.upsertArticleTags(tagsToUpsert); | |
| } | |
| async upsertArticleTags(articleId: number, tags: string[]) { | |
| // Use transaction to ensure atomicity | |
| if (tags.length === 0) return; | |
| return await this.repository.db.transaction(async (tx) => { | |
| // Ensure every tag exists | |
| const tagsRepo = this.repository.withTransaction(tx); | |
| await tagsRepo.upsertTags(tags.map(name => ({ name }))); | |
| // Delete old tags for the artice | |
| const articleTags = await tagsRepo.getArticleTags(articleId); | |
| const tagsToDelete = articleTags | |
| .filter((tag) => !tags.includes(tag.tagName)) | |
| .map((tag) => tag.tagName); | |
| if (tagsToDelete.length > 0) { | |
| await tagsRepo.deleteArticleTags({ | |
| articleId, | |
| tagNames: tagsToDelete, | |
| }); | |
| } | |
| // Upsert new and existing tags | |
| const tagsToUpsert = tags.map((tagName) => ({ articleId, tagName })); | |
| return await tagsRepo.upsertArticleTags(tagsToUpsert); | |
| }); | |
| } |
yamcodes
left a comment
yamcodes
left a comment
There was a problem hiding this comment.
Bro this is so clean. Splitting tags into their own module makes total sense for fast db ops, and the cascade + service stitching is chef's kiss. You can really feel the long-term thinking here. Love it.
2 participants
Description
PR Checklist
bun docsSummary by CodeRabbit