feat: remove package version files #781
Conversation
WalkthroughThe pull request introduces new methods and interfaces for managing the removal of package version files and their corresponding distribution files. Changes span across the service, repository, and controller layers by adding methods to remove these files in a transactional manner. Additional tests have been added to validate the new deletion logic and ensure proper handling of edge cases such as non-existent packages and unauthorized access. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant P as PackageVersionFileController
participant PM as PackageManagerService
participant PS as PackageVersionFileService
participant R as PackageVersionFileRepository
C->>P: DELETE /:fullname/:versionSpec/files
P->>P: Validate context and auth
P->>PM: showPackageVersionByVersionOrTag(scope, name, versionSpec)
alt Package version exists
P->>PS: removePackageVersionFiles(packageVersion)
PS->>PM: removePackageVersionFileAndDist(pkgVersion)
PM->>R: findPackageVersionFileDists(packageVersionId)
R->>R: Retrieve corresponding dist entries
PM->>R: removePackageVersionFiles(packageVersionId)
R-->>PM: Return removed dist data
PM-->>PS: Return removal result
PS-->>P: Return transformed PackageVersionFile objects
P-->>C: Return deleted file list
else Package version not found
P-->>C: 404 Not Found
end
Suggested reviewers
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: 0
🧹 Nitpick comments (3)
app/core/service/PackageVersionFileService.ts (1)
338-356: Well-implemented file removal method - consider adding documentationThe
removePackageVersionFilesmethod follows the existing pattern of checking configuration flags and package existence before performing operations. It uses thePackageManagerServiceto handle file removal and then transforms the results intoPackageVersionFileobjects.Consider adding a JSDoc comment to document the method's purpose, parameters, and return value for better code maintainability:
+/** + * Removes all files associated with a package version + * @param pkgVersion The package version whose files should be removed + * @returns Array of PackageVersionFile objects representing the removed files + */ async removePackageVersionFiles(pkgVersion: PackageVersion) { // implementation... }app/core/service/PackageManagerService.ts (1)
732-742: Well-structured file and dist removal methodThe
removePackageVersionFileAndDistmethod effectively:
- Retrieves all file dists associated with the package version
- Removes the physical files from storage
- Removes the database records
- Returns the removed dists for further processing
Consider adding a JSDoc comment to document this public method:
+/** + * Removes all files and their corresponding dists associated with a package version + * @param pkgVersion The package version whose files should be removed + * @returns Array of Dist objects representing the removed file dists + */ public async removePackageVersionFileAndDist(pkgVersion: PackageVersion) { // implementation... }app/repository/PackageVersionFileRepository.ts (1)
108-125: Excellent transaction handling for atomicity.The implementation correctly uses a transaction to ensure that both the PackageVersionFile and Dist records are removed atomically, preventing potential data inconsistencies. The logging of removal counts is also helpful for debugging and auditing purposes.
Consider adding error handling within the transaction to provide more specific error messages when removal operations fail:
async removePackageVersionFiles(packageVersionId: string) { const fileDists = await this.findPackageVersionFiles(packageVersionId); const distIds = fileDists.map(dist => dist.distId); await this.PackageVersionFile.transaction(async transaction => { + try { const removeCount = await this.PackageVersionFile.remove({ packageVersionId }, true, transaction); this.logger.info('[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile, packageVersionId: %s', removeCount, packageVersionId); const distCount = await this.Dist.remove({ distId: distIds, }, true, transaction); this.logger.info('[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist, packageVersionId: %s', distCount, packageVersionId); + } catch (error) { + this.logger.error('[PackageVersionFileRepository:removePackageVersionFiles:error] Failed to remove files for packageVersionId: %s, error: %s', + packageVersionId, error); + throw error; + } }); return fileDists; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/core/service/PackageManagerService.ts(3 hunks)app/core/service/PackageVersionFileService.ts(1 hunks)app/port/controller/PackageVersionFileController.ts(1 hunks)app/repository/PackageVersionFileRepository.ts(2 hunks)test/port/controller/PackageVersionFileController/removeFiles.test.ts(1 hunks)test/port/controller/package/RemovePackageVersionController.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
app/port/controller/PackageVersionFileController.ts (3)
app/common/PackageUtil.ts (2)
FULLNAME_REG_STRING(20-21)getScopeAndName(23-28)app/core/entity/Package.ts (1)
fullname(66-68)app/port/typebox.ts (1)
Spec(65-68)
test/port/controller/PackageVersionFileController/removeFiles.test.ts (1)
app/core/service/PackageVersionFileService.ts (1)
pkg(358-406)
app/repository/PackageVersionFileRepository.ts (1)
app/repository/util/ModelConvertor.ts (1)
ModelConvertor(19-139)
app/core/service/PackageVersionFileService.ts (2)
app/core/entity/PackageVersionFile.ts (1)
PackageVersionFile(15-47)app/common/FileUtil.ts (1)
mimeLookup(138-149)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-mysql57-s3-nfs (22, ubuntu-latest)
- GitHub Check: Trag Review
🔇 Additional comments (11)
test/port/controller/package/RemovePackageVersionController.test.ts (1)
211-251: Good test coverage for package version file removalThis test effectively verifies that when a package version is removed, all associated files are also properly removed. The test setup, execution, and verification process are comprehensive.
app/core/service/PackageManagerService.ts (2)
103-104: Properly injected repository dependencyThe
PackageVersionFileRepositorydependency is correctly injected for handling package version file operations.
726-729: Good integration with existing removal processThe code now correctly includes package version file cleanup when removing a package version, which ensures all associated resources are properly cleaned up.
test/port/controller/PackageVersionFileController/removeFiles.test.ts (4)
15-39: Good test for configuration checkThis test properly verifies that the endpoint returns a 404 when the
enableUnpkgconfiguration flag is disabled, ensuring that the feature is properly gated by configuration.
41-109: Comprehensive test for the happy pathThis test thoroughly verifies the file removal functionality by:
- Creating a package with the appropriate configuration
- Syncing package version files
- Removing the files
- Verifying the response structure
- Checking log messages for proper operations
The assertions are detailed and verify both the response format and the internal operations through logs.
111-121: Good error handling test for non-existent packageThis test verifies that attempting to remove files from a non-existent package properly returns a 404 error with an appropriate message.
123-129: Good authorization testThis test verifies that non-admin users cannot remove package version files, ensuring proper access control.
app/repository/PackageVersionFileRepository.ts (1)
53-60: Good implementation of findPackageVersionFiles method.The method efficiently retrieves all distribution entities associated with a package version ID using a two-step approach: first fetching the PackageVersionFile models and then retrieving the corresponding Dist models.
app/port/controller/PackageVersionFileController.ts (3)
102-106: HTTP DELETE endpoint follows RESTful conventions.The method properly uses DELETE HTTP method with a consistent URL pattern that matches other endpoints in the controller.
117-118:✅ Verification successful
Ensure proper typing alignment between service and controller.
The controller expects
diststo be objects compatible withformatFileItem, but the service might returnDistEntityobjects directly. Verify that the types are compatible to avoid runtime errors.
🏁 Script executed:
#!/bin/bash # Check the implementation of packageVersionFileService.removePackageVersionFiles rg -A 10 "async removePackageVersionFiles" app/core/service/PackageVersionFileService.tsLength of output: 713
Verification Complete: Type Alignment Confirmed
The implementation in the service correctly transforms the raw entities from
removePackageVersionFileAndDistintoPackageVersionFileobjects usingPackageVersionFile.create(). This mapping ensures that the controller receives objects compatible withformatFileItem, so no further type adjustments are required.
102-119:✅ Verification successful
Well-structured API endpoint with proper access control.
The implementation follows good practices by:
- Using proper middleware for admin-only access
- Validating input parameters
- Checking feature flag status
- Handling not found cases
Let's verify if the file deletion functionality also removes files from the NFS as specified in the PR objectives:
🏁 Script executed:
#!/bin/bash # Look for file system operations in the PackageVersionFileService rg -A 10 "removePackageVersionFiles" app/core/service/PackageVersionFileService.tsLength of output: 705
Verified: File deletion functionality properly removes files from NFS when enabled.
- The controller endpoint in
app/port/controller/PackageVersionFileController.tscontinues to validate inputs, enforce admin access, and delegate file removal correctly.- The service method
removePackageVersionFilesinapp/core/service/PackageVersionFileService.tsconfirms that file operations occur only when bothenableUnpkgandenableSyncUnpkgFilesconfiguration flags are active.- Once these flags are set, the method delegates the removal to
packageManagerService.removePackageVersionFileAndDist, which is expected to handle removal from the NFS as per the PR objectives.Overall, the code meets the intended functionality.
coolyuantao
force-pushed
the
fix/remove-version-files
branch
from
4ff8de9 to
a6172c8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #781 +/- ##
==========================================
- Coverage 95.51% 95.36% -0.15%
==========================================
Files 192 192
Lines 23407 23560 +153
Branches 1915 2621 +706
==========================================
+ Hits 22357 22469 +112
- Misses 1050 1091 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/core/service/PackageVersionFileService.ts (1)
338-362: Add error handling for the removePackageVersionFileAndDist callThe implementation is missing error handling for the call to
this.packageManagerService.removePackageVersionFileAndDist(pkgVersion). If this operation fails, the error will propagate up without any context or specific handling.Consider adding a try-catch block to handle potential errors from the file removal operation, similar to how
syncPackageVersionFileshandles errors:async removePackageVersionFiles(pkgVersion: PackageVersion) { const files: PackageVersionFile[] = []; if (!this.config.cnpmcore.enableUnpkg) return files; if (!this.config.cnpmcore.enableSyncUnpkgFiles) return files; const pkg = await this.packageRepository.findPackageByPackageId( pkgVersion.packageId ); if (!pkg) return files; + try { const fileDists = await this.packageManagerService.removePackageVersionFileAndDist( pkgVersion ); return fileDists.map(dist => { const { directory, name } = this.#getDirectoryAndName(dist.path); return PackageVersionFile.create({ packageVersionId: pkgVersion.packageVersionId, directory, name, dist, contentType: mimeLookup(dist.path), mtime: pkgVersion.publishTime, }); }); + } catch (err) { + this.logger.warn( + '[PackageVersionFileService.removePackageVersionFiles:error] packageVersionId: %s, error: %s', + pkgVersion.packageVersionId, + err + ); + throw err; + } }app/repository/PackageVersionFileRepository.ts (2)
53-63: Good implementation, but consider handling empty results explicitly.The method correctly retrieves all distribution entities associated with a package version. It efficiently queries for all package version files and then fetches their corresponding distributions.
Consider adding explicit handling for the case when no files are found, possibly by logging this scenario:
async findPackageVersionFiles(packageVersionId: string) { const fileModels = await this.PackageVersionFile.find({ packageVersionId, }); + if (fileModels.length === 0) { + this.logger.info('[PackageVersionFileRepository:findPackageVersionFiles] No files found for packageVersionId: %s', packageVersionId); + return []; + } const distIds = fileModels.map(item => item.distId); const distModels = await this.Dist.find({ distId: distIds }); return distModels.map(distModel => ModelConvertor.convertModelToEntity(distModel, DistEntity) ); }
111-142:❓ Verification inconclusive
Effective transactional implementation, but review PII logging.
The method correctly uses a transaction to ensure atomicity when removing package version files and their distributions. This is crucial for maintaining data integrity.
Logging sensitive information such as packageVersionId can lead to privacy issues. Consider masking or omitting sensitive data in logs to adhere to privacy standards.
Consider checking if
distIdsis empty before attempting removal to avoid unnecessary database operations:async removePackageVersionFiles(packageVersionId: string) { const fileDists = await this.findPackageVersionFiles(packageVersionId); const distIds = fileDists.map(dist => dist.distId); + if (distIds.length === 0) { + this.logger.info('[PackageVersionFileRepository:removePackageVersionFiles] No distributions found for packageVersionId: %s', packageVersionId); + return fileDists; + } await this.PackageVersionFile.transaction(async transaction => { const removeCount = await this.PackageVersionFile.remove( { packageVersionId }, true, transaction ); this.logger.info( '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile, packageVersionId: %s', removeCount, packageVersionId ); const distCount = await this.Dist.remove( { distId: distIds, }, true, transaction ); this.logger.info( '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist, packageVersionId: %s', distCount, packageVersionId ); }); return fileDists; }
🏁 Script executed:
#!/bin/bash # Search for other logging patterns in the codebase to verify how PII/sensitive IDs are handled # Look for logging practices with IDs across the codebase rg -A 1 -B 1 "packageVersionId.*%s" --glob "*.ts" # Check if there are any data masking utilities in the codebase rg -A 3 "mask|anonymize|obfuscate" --glob "*.ts"Length of output: 3398
Action Required: Review Logging of Sensitive Data and Optimize Removal Logic
The transactional logic in
removePackageVersionFilesis robust. However, based on our repository search:
- Sensitive Logging: The
packageVersionIdis logged in multiple files without any apparent masking or anonymization. Logging such identifiers could risk exposing PII. Please verify whether this identifier should be masked or omitted in logs to ensure privacy compliance.- Unnecessary Operations: Consider adding a check for an empty
distIdsarray before invoking the removal operation onDistto avoid unnecessary database calls. For instance:async removePackageVersionFiles(packageVersionId: string) { const fileDists = await this.findPackageVersionFiles(packageVersionId); const distIds = fileDists.map(dist => dist.distId); + if (distIds.length === 0) { + this.logger.info('[PackageVersionFileRepository:removePackageVersionFiles] No distributions found for packageVersionId: %s', packageVersionId); + return fileDists; + }Please manually verify the handling of sensitive data in logs across the codebase and adjust accordingly.
app/port/controller/PackageVersionFileController.ts (1)
102-119: Well-structured endpoint with proper validation and authorization.The implementation follows best practices for REST API design, including input validation, authorization checks, and proper error handling.
Consider adding more descriptive error handling for specific failure cases that might occur during the file deletion process:
async removeFiles(@Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() versionSpec: string) { ctx.tValidate(Spec, `${fullname}@${versionSpec}`); this.#requireUnpkgEnable(); const [ scope, name ] = getScopeAndName(fullname); const { packageVersion } = await this.packageManagerService.showPackageVersionByVersionOrTag( scope, name, versionSpec); if (!packageVersion) { throw new NotFoundError(`${fullname}@${versionSpec} not found`); } - const dists = await this.packageVersionFileService.removePackageVersionFiles(packageVersion); - return dists.map(file => formatFileItem(file)); + try { + const dists = await this.packageVersionFileService.removePackageVersionFiles(packageVersion); + return { + success: true, + removed: dists.map(file => formatFileItem(file)), + }; + } catch (error) { + this.logger.error('[PackageVersionFileController:removeFiles] Failed to remove files: %s', error); + throw error; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/core/service/PackageManagerService.ts(3 hunks)app/core/service/PackageVersionFileService.ts(1 hunks)app/port/controller/PackageVersionFileController.ts(1 hunks)app/repository/PackageVersionFileRepository.ts(2 hunks)test/port/controller/PackageVersionFileController/removeFiles.test.ts(1 hunks)test/port/controller/package/RemovePackageVersionController.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
test/port/controller/PackageVersionFileController/removeFiles.test.ts (1)
app/core/service/PackageVersionFileService.ts (1)
pkg(364-412)
app/core/service/PackageManagerService.ts (2)
app/core/service/PackageVersionFileService.ts (1)
pkgVersion(71-96)app/core/entity/PackageVersion.ts (1)
PackageVersion(19-57)
app/core/service/PackageVersionFileService.ts (2)
app/core/entity/PackageVersionFile.ts (1)
PackageVersionFile(15-47)app/common/FileUtil.ts (1)
mimeLookup(138-149)
app/repository/PackageVersionFileRepository.ts (1)
app/repository/util/ModelConvertor.ts (1)
ModelConvertor(19-139)
app/port/controller/PackageVersionFileController.ts (3)
app/common/PackageUtil.ts (2)
FULLNAME_REG_STRING(20-21)getScopeAndName(23-28)app/core/entity/Package.ts (1)
fullname(66-68)app/port/typebox.ts (1)
Spec(65-68)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Trag Review
- GitHub Check: test-mysql57-s3-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
🔇 Additional comments (9)
test/port/controller/package/RemovePackageVersionController.test.ts (1)
211-251: Thorough test implementation for package file removalThis new test case effectively verifies that package version files are properly removed when a package is deleted. It checks both the file system operations via log assertions and validates the API response patterns.
app/core/service/PackageManagerService.ts (4)
33-33: Appropriate import for the repository interfaceThe additional import for PackageVersionFileRepository is correctly placed with other repository imports.
104-104: Added dependency injection for PackageVersionFileRepositoryProperly injected the required repository as a dependency for the new functionality.
726-729: Updated method to remove package version filesGood modification of the existing
_removePackageVersionAndDistmethod to call the new method for removing package version files, ensuring complete cleanup when a package version is removed.
731-748: Well-structured method for removing package version files and their distributionsThe implementation handles the file cleanup process in two phases:
- Fetching file distributions
- Removing the files from NFS
- Cleaning up database records
This ensures data consistency and proper resource cleanup.
test/port/controller/PackageVersionFileController/removeFiles.test.ts (4)
15-39: Good test for configuration validationThis test correctly verifies that the API returns a 404 when the
enableUnpkgconfiguration is disabled, preventing unnecessary processing when the feature is turned off.
41-108: Comprehensive happy path test with proper assertionsThis test thoroughly validates the file removal functionality including:
- Setup with required configuration flags
- Publishing a package
- Syncing package files
- Removing package files
- Validating the response structure
- Verifying proper logging of operations
110-120: Proper 404 error handling testThis test ensures the API correctly handles requests for non-existent packages with appropriate error messages.
122-128: Security check for non-admin requestsThis test verifies that only admin users can remove package files, which is an important security consideration to prevent unauthorized file deletions.
coolyuantao
force-pushed
the
fix/remove-version-files
branch
from
a6172c8 to
fd79e08
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/port/controller/package/RemovePackageVersionController.test.ts (1)
211-237:⚠️ Potential issueHandle response from the DELETE request
The DELETE request at line 230 doesn't handle the response, which could lead to unhandled promise rejections if the request fails.
Apply this fix to properly handle the response:
await app .httpRequest() .delete(`/${pkg.name}/-rev/${pkgVersion._rev}`) .set('authorization', adminUser.authorization) .set('npm-command', 'unpublish') .set('user-agent', adminUser.ua) - .expect(200); + .expect(200) + .then(response => { + assert.equal(response.body.ok, true); + });
🧹 Nitpick comments (2)
app/core/service/PackageVersionFileService.ts (1)
340-341: Enhance configuration checks with detailed loggingThe method silently returns on configuration check failures without logging. Adding logs would help with debugging if files aren't being removed as expected.
Consider adding logging for early returns:
async removePackageVersionFiles(pkgVersion: PackageVersion) { const files: PackageVersionFile[] = []; - if (!this.config.cnpmcore.enableUnpkg) return files; - if (!this.config.cnpmcore.enableSyncUnpkgFiles) return files; + if (!this.config.cnpmcore.enableUnpkg) { + this.logger.debug('[PackageVersionFileService.removePackageVersionFiles] enableUnpkg is false, skipping'); + return files; + } + if (!this.config.cnpmcore.enableSyncUnpkgFiles) { + this.logger.debug('[PackageVersionFileService.removePackageVersionFiles] enableSyncUnpkgFiles is false, skipping'); + return files; + }app/port/controller/PackageVersionFileController.ts (1)
117-118: Consider adding error handling for removePackageVersionFiles operationThe method currently doesn't include specific error handling for potential failures in the
removePackageVersionFilesoperation. While errors might propagate naturally, explicit handling would make the behavior more predictable and maintainable.- const dists = await this.packageVersionFileService.removePackageVersionFiles(packageVersion); - return dists.map(file => formatFileItem(file)); + try { + const dists = await this.packageVersionFileService.removePackageVersionFiles(packageVersion); + return { + success: true, + files: dists.map(file => formatFileItem(file)), + }; + } catch (error) { + ctx.logger.error('[PackageVersionFileController.removeFiles] error: %s', error); + throw error; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/core/service/PackageManagerService.ts(3 hunks)app/core/service/PackageVersionFileService.ts(1 hunks)app/port/controller/PackageVersionFileController.ts(1 hunks)app/repository/PackageVersionFileRepository.ts(2 hunks)test/port/controller/PackageVersionFileController/removeFiles.test.ts(1 hunks)test/port/controller/package/RemovePackageVersionController.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/port/controller/PackageVersionFileController/removeFiles.test.ts
🧰 Additional context used
🧬 Code Definitions (3)
app/core/service/PackageManagerService.ts (2)
app/core/service/PackageVersionFileService.ts (1)
pkgVersion(71-96)app/core/entity/PackageVersion.ts (1)
PackageVersion(19-57)
app/core/service/PackageVersionFileService.ts (2)
app/core/entity/PackageVersionFile.ts (1)
PackageVersionFile(15-47)app/common/FileUtil.ts (1)
mimeLookup(138-149)
app/port/controller/PackageVersionFileController.ts (3)
app/common/PackageUtil.ts (2)
FULLNAME_REG_STRING(20-21)getScopeAndName(23-28)app/core/entity/Package.ts (1)
fullname(66-68)app/port/typebox.ts (1)
Spec(65-68)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Trag Review
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (20, ubuntu-latest)
🔇 Additional comments (10)
test/port/controller/package/RemovePackageVersionController.test.ts (1)
211-272: Comprehensive test for package version file removalThe test case provides a thorough verification of the file removal functionality, including checking file accessibility before and after deletion, and verifying the expected log messages for each step of the deletion process.
app/core/service/PackageManagerService.ts (2)
738-747: Limit concurrency to prevent resource exhaustionUsing Promise.all with a large array can lead to high memory consumption and potential blocking. The current implementation uses pMap with a high concurrency value (50), which might overwhelm system resources.
Consider reducing the concurrency level:
pMap( fileDists, async dist => { await this.distRepository.destroyDist(dist); }, { - concurrency: 50, + concurrency: 10, stopOnError: false, } );
726-729: Well-integrated file removal in the package version deletion processThe updated
_removePackageVersionAndDistmethod now properly calls the newremovePackageVersionFileAndDistmethod to ensure package version files are cleaned up when a package version is removed.app/core/service/PackageVersionFileService.ts (1)
338-362: Well-structured implementation of the removePackageVersionFiles methodThe new method follows the established pattern in the service, with appropriate early returns based on configuration settings and proper error handling. It successfully leverages the PackageManagerService to perform the actual removal and maps the results back to the expected format.
app/repository/PackageVersionFileRepository.ts (4)
121-125: Avoid logging sensitive dataLogging sensitive information such as packageVersionId can lead to privacy issues. Consider sanitizing or omitting sensitive data from logs to comply with privacy standards.
Consider sanitizing the packageVersionId in logs:
this.logger.info( '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile, packageVersionId: %s', removeCount, - packageVersionId + packageVersionId.substring(0, 8) + '...' // Show only first few characters );
134-138: Avoid logging sensitive dataLogging sensitive information such as packageVersionId can lead to privacy issues. Consider sanitizing or omitting sensitive data from logs to comply with privacy standards.
Consider sanitizing the packageVersionId in logs:
this.logger.info( '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist, packageVersionId: %s', distCount, - packageVersionId + packageVersionId.substring(0, 8) + '...' // Show only first few characters );
111-142: Well-implemented transaction for atomic operationsThe implementation correctly uses a transaction to ensure that both the PackageVersionFile and Dist records are removed atomically, which prevents orphaned records in case of failures during the removal process.
53-63: Efficient implementation of findPackageVersionFilesThe implementation efficiently retrieves all file models for a package version and then fetches the associated dist models in a single query, minimizing database roundtrips.
app/port/controller/PackageVersionFileController.ts (2)
102-119: Good implementation of the removeFiles endpoint!The new DELETE endpoint follows best practices by:
- Requiring admin access via the AdminAccess middleware
- Validating input parameters
- Checking feature flags before proceeding
- Properly handling the not-found case
- Returning the list of removed files with correct formatting
The implementation aligns well with the PR objective to remove package version files when packages are removed.
116-118:✅ Verification successful
Add transaction management for data consistency
Since this operation removes data from multiple tables ("Dists" and "PackageVersionFiles") and from NFS as mentioned in the PR description, consider ensuring this is done in a transactional manner to maintain data consistency in case of failures.
Could you verify if the underlying
removePackageVersionFilesimplementation handles transactions properly? If not, this could lead to orphaned records if the operation fails halfway through.
🏁 Script executed:
#!/bin/bash # Check if removePackageVersionFiles uses transactions rg -A 10 "removePackageVersionFiles" --type tsLength of output: 8487
Transaction management verified
The implementation inapp/repository/PackageVersionFileRepository.tscorrectly wraps both the deletion of PackageVersionFiles and the corresponding Dist removals in a transaction. This ensures database consistency and prevents orphaned records even if one of the removal operations fails. While the review comment also mentions NFS removals, the transactional integrity for database operations is adequately maintained. Please double-check that any file system (NFS) cleanup is handled separately if needed.
coolyuantao
force-pushed
the
fix/remove-version-files
branch
from
fd79e08 to
758d66c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/core/service/PackageManagerService.ts (1)
731-755:⚠️ Potential issueMissing await for pMap to ensure operation completes.
The
pMapcall is not awaited, which could lead to the function returning before all file removals are completed. This would result in incomplete cleanup of files.Apply this fix to ensure all promises are properly awaited:
public async removePackageVersionFileAndDist(pkgVersion: PackageVersion) { const fileDists = await this.packageVersionFileRepository.findPackageVersionFiles( pkgVersion.packageVersionId ); // remove nfs dists - pMap( + await pMap( fileDists, async dist => { await this.distRepository.destroyDist(dist); }, { concurrency: 50, stopOnError: false, } ); // remove package version files from repository await this.packageVersionFileRepository.removePackageVersionFiles( pkgVersion.packageVersionId ); return fileDists; }
🧹 Nitpick comments (1)
app/port/controller/PackageVersionFileController.ts (1)
108-119: Consider setting appropriate HTTP status code for DELETE operations.The method doesn't explicitly set an HTTP status code for successful deletion. Consider setting a 204 No Content status, which is more appropriate for DELETE operations that don't need to return a body.
Apply this change to set the appropriate status code:
async removeFiles(@Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() versionSpec: string) { ctx.tValidate(Spec, `${fullname}@${versionSpec}`); this.#requireUnpkgEnable(); const [ scope, name ] = getScopeAndName(fullname); const { packageVersion } = await this.packageManagerService.showPackageVersionByVersionOrTag( scope, name, versionSpec); if (!packageVersion) { throw new NotFoundError(`${fullname}@${versionSpec} not found`); } const dists = await this.packageVersionFileService.removePackageVersionFiles(packageVersion); + ctx.status = 204; // No Content return dists.map(file => formatFileItem(file)); }Alternatively, if returning the list of deleted files is important for the API consumers:
async removeFiles(@Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() versionSpec: string) { // ...existing code... const dists = await this.packageVersionFileService.removePackageVersionFiles(packageVersion); + ctx.status = 200; // OK - explicitly set for clarity return dists.map(file => formatFileItem(file)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/core/service/PackageManagerService.ts(3 hunks)app/core/service/PackageVersionFileService.ts(1 hunks)app/port/controller/PackageVersionFileController.ts(1 hunks)app/repository/PackageVersionFileRepository.ts(2 hunks)test/port/controller/PackageVersionFileController/removeFiles.test.ts(1 hunks)test/port/controller/package/RemovePackageVersionController.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/port/controller/PackageVersionFileController/removeFiles.test.ts
- test/port/controller/package/RemovePackageVersionController.test.ts
🧰 Additional context used
🧬 Code Definitions (4)
app/core/service/PackageManagerService.ts (2)
app/core/service/PackageVersionFileService.ts (1)
pkgVersion(71-96)app/core/entity/PackageVersion.ts (1)
PackageVersion(19-57)
app/core/service/PackageVersionFileService.ts (2)
app/core/entity/PackageVersionFile.ts (1)
PackageVersionFile(15-47)app/common/FileUtil.ts (1)
mimeLookup(138-149)
app/port/controller/PackageVersionFileController.ts (3)
app/common/PackageUtil.ts (2)
FULLNAME_REG_STRING(20-21)getScopeAndName(23-28)app/core/entity/Package.ts (1)
fullname(66-68)app/port/typebox.ts (1)
Spec(65-68)
app/repository/PackageVersionFileRepository.ts (1)
app/repository/util/ModelConvertor.ts (1)
ModelConvertor(19-139)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Trag Review
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
🔇 Additional comments (7)
app/core/service/PackageManagerService.ts (3)
33-33: LGTM: Appropriate import for the new repository interface.The added import for
PackageVersionFileRepositoryaligns with the service's new functionality to manage package version files.
104-104: LGTM: Proper dependency injection.The
@Inject()decorator correctly injects thepackageVersionFileRepositorydependency, following the established pattern in this service.
726-729: LGTM: Updated method to clean up package version files.The existing
_removePackageVersionAndDistmethod now properly calls the newremovePackageVersionFileAndDistmethod to ensure all associated files are cleaned up when removing a package version.app/core/service/PackageVersionFileService.ts (1)
338-362: LGTM: Well-implemented method for removing package version files.The
removePackageVersionFilesmethod correctly:
- Checks configuration flags before proceeding
- Looks up required data
- Delegates the actual removal to
packageManagerService- Transforms the result to maintain consistent return types with other file operations
The early returns for disabled features and not-found package are well implemented, preventing unnecessary processing.
app/repository/PackageVersionFileRepository.ts (2)
53-63: LGTM: Well-structured method to retrieve package version files.The
findPackageVersionFilesmethod efficiently queries the database to retrieve all distributions associated with a package version, using appropriate models and entity conversion.
111-142: Avoid logging sensitive information in logs.The method logs the
packageVersionIdwhich could potentially contain sensitive information. Consider redacting or anonymizing sensitive data in logs to comply with privacy standards.Apply this change to redact sensitive information:
this.logger.info( - '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile, packageVersionId: %s', + '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile, packageVersionId: [REDACTED]', removeCount, - packageVersionId ); const distCount = await this.Dist.remove( { distId: distIds, }, true, transaction ); this.logger.info( - '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist, packageVersionId: %s', + '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist, packageVersionId: [REDACTED]', distCount, - packageVersionId );app/port/controller/PackageVersionFileController.ts (1)
102-107: LGTM: Proper HTTP method and route definition.The controller method is correctly decorated with
@HTTPMethodand uses the appropriate HTTP methodDELETEfor the resource removal operation. The path is also well-defined using theFULLNAME_REG_STRINGpattern.
coolyuantao
force-pushed
the
fix/remove-version-files
branch
from
758d66c to
0ac9fb5
Compare
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/core/service/PackageVersionFileService.ts (1)
338-372: Add transaction handling for improved fault toleranceThe
removePackageVersionFilesmethod doesn't handle transaction rollback if theremovePackageVersionFileAndDistcall succeeds but the subsequent mapping operations fail. Consider wrapping the entire operation in a transaction to ensure consistency.Additionally, the method returns reconstructed
PackageVersionFileobjects rather than the actual deleted files, which might lead to inconsistencies if the reconstruction process doesn't match the original files exactly.async removePackageVersionFiles(pkgVersion: PackageVersion) { const files: PackageVersionFile[] = []; if (!this.config.cnpmcore.enableUnpkg) return files; if (!this.config.cnpmcore.enableSyncUnpkgFiles) return files; const pkg = await this.packageRepository.findPackageByPackageId( pkgVersion.packageId ); if (!pkg) return files; try { + // First, get the file metadata before deletion to return accurate information + const fileModels = await this.packageVersionFileRepository.findPackageVersionFileModels( + pkgVersion.packageVersionId + ); + const fileDists = await this.packageManagerService.removePackageVersionFileAndDist( pkgVersion ); - return fileDists.map(dist => { + // Map using the original file metadata if available, otherwise fallback to reconstruction + return fileModels.length > 0 ? fileModels : fileDists.map(dist => { const { directory, name } = this.#getDirectoryAndName(dist.path); return PackageVersionFile.create({ packageVersionId: pkgVersion.packageVersionId, directory, name, dist, contentType: mimeLookup(dist.path), mtime: pkgVersion.publishTime, }); }); } catch (err) { this.logger.warn( '[PackageVersionFileService.removePackageVersionFiles:warn] packageVersionId: %s, error: %s', pkgVersion.packageVersionId, err ); throw err; } }app/repository/PackageVersionFileRepository.ts (1)
111-142: Add error handling and return complete file informationThe
removePackageVersionFilesmethod should include error handling to log specific errors that might occur during the transaction. Additionally, consider returning the completePackageVersionFileentities rather than just the dist entities to maintain consistency with other methods in the class.async removePackageVersionFiles(packageVersionId: string) { const fileDists = await this.findPackageVersionFiles(packageVersionId); const distIds = fileDists.map(dist => dist.distId); + try { await this.PackageVersionFile.transaction(async transaction => { const removeCount = await this.PackageVersionFile.remove( { packageVersionId }, true, transaction ); this.logger.info( '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile, packageVersionId: %s', removeCount, packageVersionId ); const distCount = await this.Dist.remove( { distId: distIds, }, true, transaction ); this.logger.info( '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist, packageVersionId: %s', distCount, packageVersionId ); }); + } catch (err) { + this.logger.error( + '[PackageVersionFileRepository:removePackageVersionFiles:error] Failed to remove files, packageVersionId: %s, error: %s', + packageVersionId, + err + ); + throw err; + } return fileDists; } + // Add a new method to get the complete file information before deletion + async findPackageVersionFileModels(packageVersionId: string) { + const fileModels = await this.PackageVersionFile.find({ + packageVersionId, + }); + const distIds = fileModels.map(item => item.distId); + const distModels = await this.Dist.find({ distId: distIds }); + + const distMap = new Map(); + for (const distModel of distModels) { + const dist = ModelConvertor.convertModelToEntity(distModel, DistEntity); + distMap.set(distModel.distId, dist); + } + + return fileModels.map(model => { + const dist = distMap.get(model.distId); + return ModelConvertor.convertModelToEntity( + model, + PackageVersionFileEntity, + { dist } + ); + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/core/service/PackageManagerService.ts(3 hunks)app/core/service/PackageVersionFileService.ts(1 hunks)app/port/controller/PackageVersionFileController.ts(1 hunks)app/repository/PackageVersionFileRepository.ts(2 hunks)test/port/controller/PackageVersionFileController/removeFiles.test.ts(1 hunks)test/port/controller/package/RemovePackageVersionController.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/port/controller/package/RemovePackageVersionController.test.ts
- test/port/controller/PackageVersionFileController/removeFiles.test.ts
🧰 Additional context used
🧬 Code Definitions (4)
app/core/service/PackageManagerService.ts (2)
app/core/service/PackageVersionFileService.ts (1)
pkgVersion(71-96)app/core/entity/PackageVersion.ts (1)
PackageVersion(19-57)
app/core/service/PackageVersionFileService.ts (2)
app/core/entity/PackageVersionFile.ts (1)
PackageVersionFile(15-47)app/common/FileUtil.ts (1)
mimeLookup(138-149)
app/repository/PackageVersionFileRepository.ts (1)
app/repository/util/ModelConvertor.ts (1)
ModelConvertor(19-139)
app/port/controller/PackageVersionFileController.ts (3)
app/common/PackageUtil.ts (2)
FULLNAME_REG_STRING(20-21)getScopeAndName(23-28)app/core/entity/Package.ts (1)
fullname(66-68)app/port/typebox.ts (1)
Spec(65-68)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Trag Review
- GitHub Check: test-mysql57-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
🔇 Additional comments (5)
app/core/service/PackageManagerService.ts (3)
33-33: LGTM: Added PackageVersionFileRepository importThe addition of the import for PackageVersionFileRepository is appropriate for the new functionality being implemented.
104-104: LGTM: Added repository dependency injectionThe PackageVersionFileRepository is properly injected as a private readonly property in the PackageManagerService class.
726-729: LGTM: Updated _removePackageVersionAndDist method to clean up package version filesThe method now calls the new removePackageVersionFileAndDist method to clean up package version files after removing the package version itself. This ensures proper cleanup of all associated resources.
app/repository/PackageVersionFileRepository.ts (1)
53-63: LGTM: Implementation of findPackageVersionFilesThe method properly retrieves all package version files associated with a package version and returns the corresponding distribution entities. The code efficiently maps file models to their dist IDs and then fetches the corresponding dist models.
app/port/controller/PackageVersionFileController.ts (1)
102-119: LGTM: Implemented removeFiles controller method with proper validationThe new endpoint is well-implemented with proper access control, input validation, and error handling. The method correctly uses the AdminAccess middleware to restrict access to administrators, validates the input parameters, and checks if the unpkg feature is enabled before proceeding with the operation.
coolyuantao
force-pushed
the
fix/remove-version-files
branch
from
0ac9fb5 to
c9b02a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/core/service/PackageManagerService.ts(3 hunks)app/core/service/PackageVersionFileService.ts(1 hunks)app/port/controller/PackageVersionFileController.ts(1 hunks)app/repository/PackageVersionFileRepository.ts(2 hunks)test/port/controller/PackageVersionFileController/removeFiles.test.ts(1 hunks)test/port/controller/package/RemovePackageVersionController.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/port/controller/package/RemovePackageVersionController.test.ts
- test/port/controller/PackageVersionFileController/removeFiles.test.ts
- app/core/service/PackageVersionFileService.ts
🧰 Additional context used
🧬 Code Definitions (3)
app/core/service/PackageManagerService.ts (2)
app/core/service/PackageVersionFileService.ts (1)
pkgVersion(71-96)app/core/entity/PackageVersion.ts (1)
PackageVersion(19-57)
app/port/controller/PackageVersionFileController.ts (3)
app/common/PackageUtil.ts (2)
FULLNAME_REG_STRING(20-21)getScopeAndName(23-28)app/core/entity/Package.ts (1)
fullname(66-68)app/port/typebox.ts (1)
Spec(65-68)
app/repository/PackageVersionFileRepository.ts (1)
app/repository/util/ModelConvertor.ts (1)
ModelConvertor(19-139)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test-mysql57-s3-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
🔇 Additional comments (5)
app/core/service/PackageManagerService.ts (3)
727-729: Implementation logic update for package version cleanupThe code now properly calls the new
removePackageVersionFileAndDistmethod during package version removal, ensuring associated file resources are cleaned up.
731-755: Ensure proper error handling during file cleanupThe
removePackageVersionFileAndDistmethod lacks error handling, which could lead to incomplete cleanup if an error occurs during the process. Consider implementing try-catch blocks and a fallback mechanism to ensure all files are properly cleaned up even if some operations fail.public async removePackageVersionFileAndDist(pkgVersion: PackageVersion) { + try { const fileDists = await this.packageVersionFileRepository.findPackageVersionFiles( pkgVersion.packageVersionId ); // remove nfs dists await pMap( fileDists, async dist => { + try { await this.distRepository.destroyDist(dist); + } catch (err) { + this.logger.warn( + '[packageManagerService.removePackageVersionFileAndDist:warn] Failed to destroy dist: %s, distId: %s, error: %s', + pkgVersion.packageVersionId, + dist.distId, + err + ); + // Continue despite errors to ensure we attempt to clean up all files + } }, { concurrency: 50, stopOnError: false, } ); // remove package version files from repository await this.packageVersionFileRepository.removePackageVersionFiles( pkgVersion.packageVersionId ); return fileDists; + } catch (err) { + this.logger.error( + '[packageManagerService.removePackageVersionFileAndDist:error] Failed to remove package version files: %s, error: %s', + pkgVersion.packageVersionId, + err + ); + throw err; + } }
737-747: Implement concurrent file deletion with proper promise handlingThe implementation of
pMapfor removing dist files doesn't await the Promise.all result, which could lead to operations completing after the function returns.Apply this fix to ensure all promises are properly awaited:
public async removePackageVersionFileAndDist(pkgVersion: PackageVersion) { const fileDists = await this.packageVersionFileRepository.findPackageVersionFiles( pkgVersion.packageVersionId ); // remove nfs dists - pMap( + await pMap( fileDists, async dist => { await this.distRepository.destroyDist(dist); }, { concurrency: 50, stopOnError: false, } ); // remove package version files from repository await this.packageVersionFileRepository.removePackageVersionFiles( pkgVersion.packageVersionId ); return fileDists; }app/repository/PackageVersionFileRepository.ts (2)
53-63: Well-implemented method to find package version filesThe implementation correctly retrieves all distribution entities associated with a package version, which will be useful for file removal operations.
111-142: Consider anonymizing sensitive data in logsLogging sensitive information such as packageVersionId can lead to privacy issues. Consider redacting or anonymizing sensitive data in logs to comply with privacy standards.
Also, the method correctly uses a transaction to ensure that both the package version files and their corresponding distribution entries are removed atomically, which is good practice.
coolyuantao
force-pushed
the
fix/remove-version-files
branch
from
c9b02a0 to
f2d1cc6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/core/service/PackageManagerService.ts (1)
731-755: 🛠️ Refactor suggestionAdd error handling to prevent incomplete cleanup.
The
removePackageVersionFileAndDistmethod lacks error handling, which could lead to incomplete cleanup if an error occurs during the process.Consider implementing try-catch blocks to ensure all files are properly cleaned up even if some operations fail:
public async removePackageVersionFileAndDist(pkgVersion: PackageVersion) { try { const fileDists = await this.packageVersionFileRepository.findPackageVersionFiles( pkgVersion.packageVersionId ); // remove nfs dists await pMap( fileDists, async dist => { try { await this.distRepository.destroyDist(dist); } catch (err) { this.logger.warn( '[packageManagerService.removePackageVersionFileAndDist:warn] Failed to destroy dist: %s, distId: %s, error: %s', pkgVersion.packageVersionId, dist.distId, err ); // Continue despite errors to ensure we attempt to clean up all files } }, { concurrency: 50, stopOnError: false, } ); // remove package version files from repository await this.packageVersionFileRepository.removePackageVersionFiles( pkgVersion.packageVersionId ); return fileDists; + } catch (err) { + this.logger.error( + '[packageManagerService.removePackageVersionFileAndDist:error] Failed to remove package version files: %s, error: %s', + pkgVersion.packageVersionId, + err + ); + throw err; + } }app/core/service/PackageVersionFileService.ts (1)
338-363:⚠️ Potential issueAdd null checking for the return value of removePackageVersionFileAndDist.
The current implementation assumes
fileDistswill always be an array, but ifremovePackageVersionFileAndDistencounters an error or returns null/undefined, the map function will cause a runtime error.Apply this fix to ensure robust error handling:
async removePackageVersionFiles(pkgVersion: PackageVersion) { const files: PackageVersionFile[] = []; if (!this.config.cnpmcore.enableUnpkg) return files; if (!this.config.cnpmcore.enableSyncUnpkgFiles) return files; const pkg = await this.packageRepository.findPackageByPackageId( pkgVersion.packageId ); if (!pkg) return files; const fileDists = await this.packageManagerService.removePackageVersionFileAndDist( pkgVersion ); - return fileDists.map(dist => { + return (fileDists || []).map(dist => { const { directory, name } = this.#getDirectoryAndName(dist.path); return PackageVersionFile.create({ packageVersionId: pkgVersion.packageVersionId, directory, name, dist, contentType: mimeLookup(dist.path), mtime: pkgVersion.publishTime, }); }); }
🧹 Nitpick comments (4)
app/repository/PackageVersionFileRepository.ts (1)
111-142: Improve security by limiting sensitive information in logs.The current implementation logs the
packageVersionIdwhich could be considered sensitive information. This practice might lead to privacy issues if logs are exposed.Consider redacting or anonymizing sensitive data in logs:
this.logger.info( - '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile, packageVersionId: %s', + '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile', removeCount, - packageVersionId ); // Similar change for the second log message this.logger.info( - '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist, packageVersionId: %s', + '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist', distCount, - packageVersionId );Alternatively, if this information is necessary for debugging, consider implementing a logging pattern that masks part of the ID.
app/port/controller/PackageVersionFileController.ts (3)
102-130: Add error handling for NFS file removal failuresThe method currently has no explicit error handling for potential failures during file deletion from the NFS. According to the PR objectives, this functionality should delete both database entries and files from the NFS.
Consider adding explicit error handling to ensure both database and file system operations complete successfully, or proper rollback happens if one fails.
async removeFiles( @Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() versionSpec: string ) { ctx.tValidate(Spec, `${fullname}@${versionSpec}`); this.#requireUnpkgEnable(); const [scope, name] = getScopeAndName(fullname); const { packageVersion } = await this.packageManagerService.showPackageVersionByVersionOrTag( scope, name, versionSpec ); if (!packageVersion) { throw new NotFoundError(`${fullname}@${versionSpec} not found`); } + try { const files = await this.packageVersionFileService.removePackageVersionFiles( packageVersion ); return files.map(file => formatFileItem(file)); + } catch (err) { + this.logger.error('[PackageVersionFileController.removeFiles] error: %s', err); + throw err; + } }
102-106: Add JSDoc comments for the new controller methodThe other methods in this controller have JSDoc comments (like
#searchPossibleEntries). For consistency and better maintainability, add JSDoc comments to describe the purpose of this new endpoint, its parameters, and return value.@HTTPMethod({ // DELETE /:fullname/:versionSpec/files path: `/:fullname(${FULLNAME_REG_STRING})/:versionSpec/files`, method: HTTPMethodEnum.DELETE, }) +/** + * Remove all files associated with a specific package version. + * This deletes both database entries from Dists and PackageVersionFiles tables + * and the actual files from NFS storage. + * @param {EggContext} ctx - The egg context + * @param {string} fullname - The package name + * @param {string} versionSpec - The package version + * @returns {Array<FileItem>} The list of removed files + */ @Middleware(AdminAccess)
115-124: Enhance robustness with service response validationThe code doesn't validate if
packageVersionexists in the service response. While there is a check against the entire object being falsy, the response structure should be validated to prevent potential null reference errors.const [scope, name] = getScopeAndName(fullname); -const { packageVersion } = +const result = await this.packageManagerService.showPackageVersionByVersionOrTag( scope, name, versionSpec ); +const { packageVersion } = result || {}; if (!packageVersion) { throw new NotFoundError(`${fullname}@${versionSpec} not found`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/core/service/PackageManagerService.ts(3 hunks)app/core/service/PackageVersionFileService.ts(1 hunks)app/port/controller/PackageVersionFileController.ts(1 hunks)app/repository/PackageVersionFileRepository.ts(2 hunks)test/port/controller/PackageVersionFileController/removeFiles.test.ts(1 hunks)test/port/controller/package/RemovePackageVersionController.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/port/controller/package/RemovePackageVersionController.test.ts
🧰 Additional context used
🧬 Code Definitions (2)
app/repository/PackageVersionFileRepository.ts (1)
app/repository/util/ModelConvertor.ts (1)
ModelConvertor(19-139)
test/port/controller/PackageVersionFileController/removeFiles.test.ts (1)
app/core/service/PackageVersionFileService.ts (1)
pkg(365-413)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Trag Review
- GitHub Check: test-mysql57-s3-nfs (20, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20.18.0, ubuntu-latest)
🔇 Additional comments (7)
app/core/service/PackageManagerService.ts (3)
33-33: Good addition of the required import.The import for
PackageVersionFileRepositoryis correctly added to support the new functionality of removing package version files.
104-104: Good dependency injection practice.The
packageVersionFileRepositoryis properly injected following the established pattern in this service class.
726-729: Good integration of file cleanup in the version removal process.The addition of file cleanup to
_removePackageVersionAndDistensures that package version files are properly removed when a package version is removed, which aligns with the PR objective.test/port/controller/PackageVersionFileController/removeFiles.test.ts (1)
1-146: Comprehensive test coverage for the new functionality.The test suite thoroughly covers various scenarios including successful removal, error cases, and permission checks. It follows the established testing patterns and includes appropriate assertions.
app/repository/PackageVersionFileRepository.ts (1)
53-63: Well-implemented method to fetch package version files.The
findPackageVersionFilesmethod efficiently retrieves files based on the package version ID and properly converts models to entities.app/port/controller/PackageVersionFileController.ts (2)
102-130: Verify the formatting of returned file objectsThe
removeFilesmethod returns the result of callingformatFileItem(file)on each item infiles, but based on the method nameremovePackageVersionFiles, it's likely that this service returnsDistobjects rather thanPackageVersionFileobjects that are expected byformatFileItem. This type mismatch could cause runtime errors.Consider updating the implementation to properly format the response based on what the service actually returns:
async removeFiles( @Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() versionSpec: string ) { ctx.tValidate(Spec, `${fullname}@${versionSpec}`); this.#requireUnpkgEnable(); const [scope, name] = getScopeAndName(fullname); const { packageVersion } = await this.packageManagerService.showPackageVersionByVersionOrTag( scope, name, versionSpec ); if (!packageVersion) { throw new NotFoundError(`${fullname}@${versionSpec} not found`); } const files = await this.packageVersionFileService.removePackageVersionFiles( packageVersion ); - return files.map(file => formatFileItem(file)); + // Create a simplified response structure based on what the service returns + return files.map(dist => ({ + path: dist.name || dist.distId, + type: 'file', + contentType: 'application/octet-stream', + integrity: dist.integrity, + lastModified: dist.updatedAt || new Date(), + size: dist.size, + })); }Alternatively, ensure that the
removePackageVersionFilesservice returns properly structuredPackageVersionFileobjects to be compatible withformatFileItem.
125-128:❓ Verification inconclusive
Verify transaction handling across database and NFS operations
The PR objective states that this feature should remove entries from both the database tables and files from the NFS. However, it's unclear if the
removePackageVersionFilesservice method handles these operations within a transaction to ensure atomicity.
🏁 Script executed:
#!/bin/bash # Search for the implementation of removePackageVersionFiles to verify transaction handling ast-grep --pattern $'async removePackageVersionFiles($packageVersion) { $$$ }'Length of output: 81
ACTION: Manually verify transaction handling in
removePackageVersionFilesThe initial verification command returned no output, which prevents us from confirming whether transactional operations (ensuring atomic removal from the database and the NFS) are handled within the service method. It appears the search pattern might not align with the actual implementation. Please take the following steps:
- Check the implementation of
removePackageVersionFiles(likely in the PackageVersionFileService module) to verify if it wraps both the database and file system removal steps inside a transaction.- Look for keywords such as “transaction”, “BEGIN”, “COMMIT”, or related transactional API calls in that function.
- Confirm that any failure in one operation triggers a rollback for consistency.
Based on these findings, update the service implementation if the operations are not already managed transactionally.
fengmk2
changed the title
|
@coolyuantao 😄 感谢贡献 |
coolyuantao
force-pushed
the
fix/remove-version-files
branch
from
f2d1cc6 to
7ce444e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/core/service/PackageManagerService.ts (1)
731-760: 🛠️ Refactor suggestionImprove error handling in removePackageVersionFileAndDist method.
The method lacks error handling which could lead to incomplete cleanup if errors occur during the removal process. This issue was previously flagged in a past review comment but hasn't been addressed.
Consider implementing try-catch blocks to ensure proper error reporting and recovery:
public async removePackageVersionFileAndDist(pkgVersion: PackageVersion) { + try { let fileDists: Dist[] = []; if (!this.config.cnpmcore.enableUnpkg) return fileDists; if (!this.config.cnpmcore.enableSyncUnpkgFiles) return fileDists; fileDists = await this.packageVersionFileRepository.findPackageVersionFileDists( pkgVersion.packageVersionId ); // remove nfs dists await pMap( fileDists, async dist => { + try { await this.distRepository.destroyDist(dist); + } catch (err) { + this.logger.warn( + '[packageManagerService.removePackageVersionFileAndDist:warn] Failed to destroy dist: %s, distId: %s, error: %s', + pkgVersion.packageVersionId, + dist.distId, + err + ); + // Continue despite errors to ensure we attempt to clean up all files + } }, { concurrency: 50, stopOnError: false, } ); // remove package version files from repository await this.packageVersionFileRepository.removePackageVersionFiles( pkgVersion.packageVersionId, fileDists.map(dists => dists.distId) ); return fileDists; + } catch (err) { + this.logger.error( + '[packageManagerService.removePackageVersionFileAndDist:error] Failed to remove package version files: %s, error: %s', + pkgVersion.packageVersionId, + err + ); + throw err; + } }app/core/service/PackageVersionFileService.ts (1)
338-363: 🛠️ Refactor suggestionAdd null check for the fileDists returned from removePackageVersionFileAndDist.
The code assumes that
fileDistsreturned fromremovePackageVersionFileAndDistis always an array, but it could be undefined if an error occurs.async removePackageVersionFiles(pkgVersion: PackageVersion) { const files: PackageVersionFile[] = []; if (!this.config.cnpmcore.enableUnpkg) return files; if (!this.config.cnpmcore.enableSyncUnpkgFiles) return files; const pkg = await this.packageRepository.findPackageByPackageId( pkgVersion.packageId ); if (!pkg) return files; const fileDists = await this.packageManagerService.removePackageVersionFileAndDist( pkgVersion ); - return fileDists.map(dist => { + return (fileDists || []).map(dist => { const { directory, name } = this.#getDirectoryAndName(dist.path); return PackageVersionFile.create({ packageVersionId: pkgVersion.packageVersionId, directory, name, dist, contentType: mimeLookup(dist.path), mtime: pkgVersion.publishTime, }); }); }app/port/controller/PackageVersionFileController.ts (1)
102-130:⚠️ Potential issueFix type mismatch in return value formatting
The
removeFilesmethod at line 129 returns the result of callingformatFileItem(file)on each item infiles, but based on the PR context,removePackageVersionFileslikely returnsDistobjects whileformatFileItemexpectsPackageVersionFileobjects. This type mismatch could cause runtime errors.Apply this diff to properly format the response:
async removeFiles( @Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() versionSpec: string ) { ctx.tValidate(Spec, `${fullname}@${versionSpec}`); this.#requireUnpkgEnable(); const [scope, name] = getScopeAndName(fullname); const { packageVersion } = await this.packageManagerService.showPackageVersionByVersionOrTag( scope, name, versionSpec ); if (!packageVersion) { throw new NotFoundError(`${fullname}@${versionSpec} not found`); } const files = await this.packageVersionFileService.removePackageVersionFiles( packageVersion ); - return files.map(file => formatFileItem(file)); + // Create a proper response structure for Dist objects + return files.map(dist => ({ + path: dist.name || dist.distId, + type: 'file', + contentType: 'application/octet-stream', + integrity: dist.integrity, + lastModified: dist.updatedAt || new Date(), + size: dist.size, + })); }
🧹 Nitpick comments (4)
app/core/service/PackageVersionFileService.ts (1)
338-340: Add error handling to the removePackageVersionFiles method.There's no error handling in this method, which could lead to unhandled exceptions if errors occur during the removal process.
async removePackageVersionFiles(pkgVersion: PackageVersion) { + try { const files: PackageVersionFile[] = []; if (!this.config.cnpmcore.enableUnpkg) return files; if (!this.config.cnpmcore.enableSyncUnpkgFiles) return files; const pkg = await this.packageRepository.findPackageByPackageId( pkgVersion.packageId ); if (!pkg) return files; const fileDists = await this.packageManagerService.removePackageVersionFileAndDist( pkgVersion ); return (fileDists || []).map(dist => { const { directory, name } = this.#getDirectoryAndName(dist.path); return PackageVersionFile.create({ packageVersionId: pkgVersion.packageVersionId, directory, name, dist, contentType: mimeLookup(dist.path), mtime: pkgVersion.publishTime, }); }); + } catch (err) { + this.logger.error( + '[PackageVersionFileService.removePackageVersionFiles:error] Failed to remove package version files: %s, error: %s', + pkgVersion.packageVersionId, + err + ); + throw err; + } }app/repository/PackageVersionFileRepository.ts (2)
53-69: Fix variable name typo and add pagination for better performance.There's a minor typo in the variable name
qunee(should bequeue), and this method could benefit from pagination when dealing with large package repositories.async findPackageVersionFileDists(packageVersionId: string) { const fileDists: DistEntity[] = []; - const qunee = ['/']; + const queue = ['/']; + const batchSize = 100; // Configurable batch size - while (qunee.length > 0) { - const directory = qunee.shift(); + while (queue.length > 0) { + const directory = queue.shift(); if (!directory) { continue; } const { files, directories } = await this.listPackageVersionFiles( packageVersionId, directory ); fileDists.push(...files.map(file => file.dist)); - qunee.push(...directories); + queue.push(...directories); + + // Implement batching if fileDists grows too large + if (fileDists.length > batchSize) { + this.logger.info( + '[PackageVersionFileRepository:findPackageVersionFileDists] Processing batch of %d files, packageVersionId: %s', + fileDists.length, + packageVersionId + ); + } } return fileDists; }
117-127: Consider adding directory filtering option for better performance.A previous review comment suggested filtering by directory, which would improve performance when dealing with large package repositories by limiting the scope of removal operations.
async removePackageVersionFiles( packageVersionId: string, + directory?: string, distIds?: string[] ) { if (!distIds) { + if (directory) { + // Optimize by only finding files in the specified directory + const { files } = await this.listPackageVersionFiles( + packageVersionId, + directory + ); + distIds = files.map(file => file.dist.distId); + } else { const fileDists = await this.findPackageVersionFileDists(packageVersionId); distIds = fileDists.map(dist => dist.distId); + } }app/port/controller/PackageVersionFileController.ts (1)
102-130: Consider adding validation for the package ownerWhile the endpoint is protected by
AdminAccessmiddleware, it would be a good practice to validate if the admin has specific permissions to delete files for this particular package, especially for packages owned by other organizations.Consider adding owner validation:
async removeFiles( @Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() versionSpec: string ) { ctx.tValidate(Spec, `${fullname}@${versionSpec}`); this.#requireUnpkgEnable(); const [scope, name] = getScopeAndName(fullname); const { packageVersion } = await this.packageManagerService.showPackageVersionByVersionOrTag( scope, name, versionSpec ); if (!packageVersion) { throw new NotFoundError(`${fullname}@${versionSpec} not found`); } + // Validate if admin has permission for this package (optional for super admins) + if (!ctx.user.isAdmin && !await this.packageManagerService.hasManagePermission(ctx.user, scope, name)) { + throw new ForbiddenError(`No permission to manage package ${fullname}`); + } const files = await this.packageVersionFileService.removePackageVersionFiles( packageVersion ); return files.map(file => formatFileItem(file)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/core/service/PackageManagerService.ts(3 hunks)app/core/service/PackageVersionFileService.ts(1 hunks)app/port/controller/PackageVersionFileController.ts(1 hunks)app/repository/PackageVersionFileRepository.ts(2 hunks)test/port/controller/PackageVersionFileController/removeFiles.test.ts(1 hunks)test/port/controller/package/RemovePackageVersionController.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/port/controller/package/RemovePackageVersionController.test.ts
🧰 Additional context used
🧬 Code Definitions (2)
app/port/controller/PackageVersionFileController.ts (4)
app/common/PackageUtil.ts (2)
FULLNAME_REG_STRING(20-21)getScopeAndName(23-28)app/core/entity/Package.ts (1)
fullname(66-68)app/port/typebox.ts (1)
Spec(65-68)app/core/service/PackageVersionFileService.ts (1)
files(439-453)
app/core/service/PackageManagerService.ts (2)
app/core/service/PackageVersionFileService.ts (1)
pkgVersion(71-96)app/core/entity/PackageVersion.ts (1)
PackageVersion(19-57)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Trag Review
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
🔇 Additional comments (7)
app/core/service/PackageManagerService.ts (4)
33-33: LGTM: Import added for PackageVersionFileRepository.This import is correctly added to support the new functionality for removing package version files.
104-104: LGTM: Added repository injection for PackageVersionFileRepository.This correctly injects the repository dependency needed for the new file removal functionality.
728-729: Updated _removePackageVersionAndDist to also clean up package version files.Good addition to ensure all related files are removed when a package version is deleted, implementing the core functionality from the PR requirements.
731-731: Consider implementing the reviewer's suggestion to make this method private.A previous review comment suggested making this method private, but it's currently public. Since this method is an implementation detail that should only be called from within the service layer, it would be more appropriate as a private method.
-public async removePackageVersionFileAndDist(pkgVersion: PackageVersion) { +private async removePackageVersionFileAndDist(pkgVersion: PackageVersion) {test/port/controller/PackageVersionFileController/removeFiles.test.ts (1)
1-146: LGTM: Comprehensive test coverage for file removal functionality.The test suite covers all critical scenarios:
- Default configuration behavior when enableUnpkg is false
- Successful file removal with proper assertions
- Error handling for non-existent packages
- Authorization checks for admin-only functionality
This is a well-structured test suite that validates the complete file removal feature.
app/repository/PackageVersionFileRepository.ts (1)
133-137: Limit sensitive data in logs to reduce privacy risks.Logging sensitive information such as
packageVersionIdcould lead to privacy issues if logs are exposed. Consider redacting or anonymizing these identifiers in logs.this.logger.info( - '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile, packageVersionId: %s', - removeCount, - packageVersionId + '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile', + removeCount ); // And similar changes for the Dist logging this.logger.info( - '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist, packageVersionId: %s', - distCount, - packageVersionId + '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist', + distCount );Also applies to: 148-151
app/port/controller/PackageVersionFileController.ts (1)
102-130: Enhance security during file removal processThe removal of files should include additional measures to ensure that no sensitive data is inadvertently exposed or logged during the process, which could violate privacy and security standards.
Consider implementing redaction of sensitive information in logs and adding more detailed audit logging:
async removeFiles( @Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() versionSpec: string ) { ctx.tValidate(Spec, `${fullname}@${versionSpec}`); this.#requireUnpkgEnable(); const [scope, name] = getScopeAndName(fullname); + this.logger.info('[PackageVersionFileController:removeFiles] Removing files for %s@%s', fullname, versionSpec); const { packageVersion } = await this.packageManagerService.showPackageVersionByVersionOrTag( scope, name, versionSpec ); if (!packageVersion) { throw new NotFoundError(`${fullname}@${versionSpec} not found`); } + // Record audit log for this admin action + await this.auditLogger?.log(ctx.user, 'remove_package_version_files', { + package: packageVersion.package.fullname, + version: packageVersion.version, + }); const files = await this.packageVersionFileService.removePackageVersionFiles( packageVersion ); + this.logger.info('[PackageVersionFileController:removeFiles] Successfully removed %d files for %s@%s', + files.length, fullname, versionSpec); return files.map(file => formatFileItem(file)); }Let's verify if the
auditLoggerservice is available in this class by searching for its usage:#!/bin/bash # Check if AbstractController (parent class) or PackageVersionFileController uses auditLogger rg -A 2 -B 2 "auditLogger" --type ts
coolyuantao
force-pushed
the
fix/remove-version-files
branch
from
7ce444e to
25ec72e
Compare
Pull request summary
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
app/core/service/PackageManagerService.ts (2)
742-751:⚠️ Potential issueMissing await on pMap call.
The
pMapfunction returns a Promise, but you're not awaiting it. This could lead to operations completing after the function returns.// remove nfs dists - pMap( + await pMap( fileDists, async dist => { await this.distRepository.destroyDist(dist); }, { concurrency: 50, stopOnError: false, } );
731-760: 🛠️ Refactor suggestionImplement error handling for the file removal process.
The current implementation lacks error handling for the file removal operations, which could lead to partial cleanups if errors occur during the process.
public async removePackageVersionFileAndDist(pkgVersion: PackageVersion) { let fileDists: Dist[] = []; if (!this.config.cnpmcore.enableUnpkg) return fileDists; if (!this.config.cnpmcore.enableSyncUnpkgFiles) return fileDists; try { fileDists = await this.packageVersionFileRepository.findPackageVersionFileDists( pkgVersion.packageVersionId ); // remove nfs dists + await pMap( - pMap( fileDists, async dist => { + try { await this.distRepository.destroyDist(dist); + } catch (err) { + this.logger.warn( + '[packageManagerService.removePackageVersionFileAndDist:warn] Failed to destroy dist: %s, distId: %s, error: %s', + pkgVersion.packageVersionId, + dist.distId, + err + ); + // Continue despite errors to ensure we attempt to clean up all files + } }, { concurrency: 50, stopOnError: false, } ); // remove package version files from repository await this.packageVersionFileRepository.removePackageVersionFiles( pkgVersion.packageVersionId, fileDists.map(dists => dists.distId) ); return fileDists; + } catch (err) { + this.logger.error( + '[packageManagerService.removePackageVersionFileAndDist:error] Failed to remove package version files: %s, error: %s', + pkgVersion.packageVersionId, + err + ); + throw err; + } }app/core/service/PackageVersionFileService.ts (1)
338-363: 🛠️ Refactor suggestionAdd error handling for package version file removal.
The current implementation lacks error handling for the
removePackageVersionFileAndDistcall. Also, there's a potential issue iffileDistsis not an array.async removePackageVersionFiles(pkgVersion: PackageVersion) { const files: PackageVersionFile[] = []; if (!this.config.cnpmcore.enableUnpkg) return files; if (!this.config.cnpmcore.enableSyncUnpkgFiles) return files; const pkg = await this.packageRepository.findPackageByPackageId( pkgVersion.packageId ); if (!pkg) return files; + try { const fileDists = await this.packageManagerService.removePackageVersionFileAndDist( pkgVersion ); + if (!fileDists || !Array.isArray(fileDists)) { + this.logger.warn( + '[packageVersionFileService.removePackageVersionFiles:warn] Invalid fileDists returned: %j, packageVersionId: %s', + fileDists, + pkgVersion.packageVersionId + ); + return files; + } return fileDists.map(dist => { const { directory, name } = this.#getDirectoryAndName(dist.path); return PackageVersionFile.create({ packageVersionId: pkgVersion.packageVersionId, directory, name, dist, contentType: mimeLookup(dist.path), mtime: pkgVersion.publishTime, }); }); + } catch (err) { + this.logger.error( + '[packageVersionFileService.removePackageVersionFiles:error] Failed to remove package version files: %s, error: %s', + pkgVersion.packageVersionId, + err + ); + throw err; + } }app/port/controller/PackageVersionFileController.ts (2)
129-129:⚠️ Potential issuePotential type mismatch in formatting returned objects.
The
removePackageVersionFilesmethod likely returnsDistobjects, butformatFileItemexpectsPackageVersionFileobjects. This will cause runtime errors.- return files.map(file => formatFileItem(file)); + return files.map(dist => ({ + path: dist.name || dist.distId, + type: 'file', + contentType: 'application/octet-stream', + integrity: dist.integrity, + lastModified: dist.updatedAt || new Date(), + size: dist.size, + }));
125-128: 🛠️ Refactor suggestionAdd error handling for file removal operations.
The implementation should handle potential errors during file removal to prevent inconsistent states where database entries are removed but files remain on disk, or vice versa.
const files = await this.packageVersionFileService.removePackageVersionFiles( packageVersion + ).catch(err => { + this.logger.error('[PackageVersionFileController:removeFiles] Failed to remove files: %s', err); + throw new Error(`Failed to remove package version files: ${err.message}`); + });
🧹 Nitpick comments (1)
app/repository/PackageVersionFileRepository.ts (1)
117-156: Reduce exposure of sensitive information in logs.The current implementation logs the
packageVersionIdwhich could be considered sensitive information. Consider anonymizing or minimizing the exposure of such identifiers in logs.this.logger.info( - '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile, packageVersionId: %s', + '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in PackageVersionFile', removeCount, - packageVersionId ); if (distIds.length > 0) { const distCount = await this.Dist.remove( { distId: distIds, }, true, transaction ); this.logger.info( - '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist, packageVersionId: %s', + '[PackageVersionFileRepository:removePackageVersionFiles:remove] %d rows in Dist', distCount, - packageVersionId ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/core/service/PackageManagerService.ts(3 hunks)app/core/service/PackageVersionFileService.ts(1 hunks)app/port/controller/PackageVersionFileController.ts(1 hunks)app/repository/PackageVersionFileRepository.ts(2 hunks)test/port/controller/PackageVersionFileController/removeFiles.test.ts(1 hunks)test/port/controller/package/RemovePackageVersionController.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/port/controller/package/RemovePackageVersionController.test.ts
🧰 Additional context used
🧬 Code Definitions (3)
test/port/controller/PackageVersionFileController/removeFiles.test.ts (1)
app/core/service/PackageVersionFileService.ts (1)
pkg(365-413)
app/core/service/PackageManagerService.ts (2)
app/core/service/PackageVersionFileService.ts (1)
pkgVersion(71-96)app/core/entity/PackageVersion.ts (1)
PackageVersion(19-57)
app/core/service/PackageVersionFileService.ts (2)
app/core/entity/PackageVersionFile.ts (1)
PackageVersionFile(15-47)app/common/FileUtil.ts (1)
mimeLookup(138-149)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Trag Review
- GitHub Check: test-postgresql-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20.18.0, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (20, ubuntu-latest)
🔇 Additional comments (7)
app/core/service/PackageManagerService.ts (3)
33-33: Imports look good!The import for
PackageVersionFileRepositoryis appropriately added.
104-104: Dependency injection looks good!The repository is correctly injected using the
@Inject()decorator.
727-729: Good integration with existing functionality.The call to
removePackageVersionFileAndDistis appropriately added in the_removePackageVersionAndDistmethod, ensuring file cleanup happens after removing the package version.test/port/controller/PackageVersionFileController/removeFiles.test.ts (1)
1-146: Comprehensive test coverage for the file removal functionality.The test suite covers various scenarios including configuration variations, successful removals, error cases, and permission checks. Great job implementing tests for this new feature!
app/repository/PackageVersionFileRepository.ts (1)
53-69: Efficient implementation for collecting all file distributions.The breadth-first search approach is well-suited for traversing file hierarchies. Consider adding pagination or batch processing for repositories with a large number of files.
app/port/controller/PackageVersionFileController.ts (2)
102-130: Successfully implemented the package version file removal feature.The new
removeFilesmethod nicely implements the PR objective of ensuring package version files are removed when packages are deleted from the system. The endpoint is properly secured with admin access, validates input parameters, and integrates with the existing service layer.
125-128: Ensure file removal operations don't expose sensitive data.The file removal process should ensure that no sensitive data is inadvertently exposed or logged during the process, which could violate privacy and security standards.
#!/bin/bash # Check for logging practices in the file removal process # Look for logging in PackageVersionFileService implementation echo "Checking for logging in PackageVersionFileService..." rg -A 5 "removePackageVersionFiles.*log|log.*removePackageVersionFiles" --type ts # Look for general logging practices with sensitive data echo "Checking for sensitive data handling patterns..." rg -A 5 "sensitiveData|maskSensitive|redact" --type ts
Delete package version files when deleting packages.
Summary by CodeRabbit
New Features
Tests