feat: proxy mode #571
feat: proxy mode #571
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #571 +/- ##
==========================================
+ Coverage 96.81% 96.83% +0.02%
==========================================
Files 181 188 +7
Lines 18003 18799 +796
Branches 2336 2466 +130
==========================================
+ Hits 17429 18204 +775
- Misses 574 595 +21 ☔ View full report in Codecov by Sentry. |
|
目前代理模式无法配置上游仓库认证头的信息，认证头应该作为仓库的一个属性记录在数据库里，可以通过接口修改。 |
|
@hezhengxu2018 🤩 已经改完了吗？ |
是的，现在会定时更新已缓存的manifest，不过还没有手动管理缓存的接口。verdicca和nexus都没做这个，先基础功能确定没问题再做吧 |
|
管理缓存的接口也加上了，功能相对来说比较完整了，帮忙看一下吧 @elrrrrrrr @fengmk2 |
🤩 改动内容比较多 我明天详细看下 🙏🏻 |
| pkg = await this.getPackageEntityByFullname(fullname, allowSync); | ||
| packageVersion = await this.getPackageVersionEntity(pkg, version, allowSync); | ||
| } catch (error) { | ||
| if (this.config.cnpmcore.syncMode === SyncMode.proxy) { |
There was a problem hiding this comment.
这里直接用 AOP 来组织一下逻辑会清晰一下，类似 EventCorkAdvice 的实现
在原来方法返回前，判断一下是否有返回值，如果没有的话再去触发 proxyMode 的 fallback 逻辑
There was a problem hiding this comment.
尝试了一下好像没有改写函数返回的切点？这样的话确实可以较为优雅的处理代理的逻辑但是改不了接口的返回值。用户还是会收到not found的返回。还是希望代理模式的表现和nexus与verdaccio保持一致。
|
proxyMode 定位是做代理模式，然后本地 registry 做缓存加速？需要确认一下 manifest 以本地为准还是以上游为准。 目前实现是 manifest 和 tgz 请求时，将代理结果直接返回，并创建同步任务
如果 proxyMode 中直接命中 tgz 下载，需要等异步定时任务补偿后才能继续访问，否则 manifest 会返回单个版本？客户单查包信息的时候就过期了。 我们是否可以改为 proxyMode 下，始终返回上游 registry 信息已获取更高的实时性。 |
开启proxyMode之后返回的manifest都是上游仓库上次更新的manifest，不会使用数据库里的manifest信息。如果上游仓库无法正常使用了，切回到none模式下才会返回代理仓库已缓存版本的manifest。如果异步任务还没同步完成会一直通过反向代理返回上游仓库的tgz信息，用户不会需要等待异步任务完成，当异步任务完成后才会优先从对象存储中读取tgz。 代理仓库主要的功能点是在访问外网或者官方npm仓库非常缓慢甚至无法访问的情况下能够缓存结果加速内网用户安装依赖，同时即使外网无法访问了也不影响内网用户照常使用已经缓存的依赖，所以始终返回上游仓库可能不行，如果内网用户发现代理仓库的缓存需要更新或者缓存脏数据了可以使用 |
|
manifest 以本地为准，因为nexus是这么做的。代理模式整体是一个缓存，如果网络不好还一直使用上游仓库的索引的话就没有缓存的意义了，不过nexus默认刷新manifest的频率很高，30分钟就会去刷新一次，我设置每天刷新一次感觉有点保守了。 |
hezhengxu2018
force-pushed
the
feat/proxy-mode
branch
from
6f0dce3
to
fe1c16c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- app/port/controller/ProxyCacheController.ts (1 hunks)
- app/repository/ProxyCacheRepository.ts (1 hunks)
- app/repository/model/ProxyCache.ts (1 hunks)
- sql/3.47.0.sql (1 hunks)
- test/port/controller/ProxyCacheController/index.test.ts (1 hunks)
- test/repository/ProxyCachePepository.test.ts (1 hunks)
Additional comments not posted (46)
sql/3.47.0.sql (12)
1-1: LGTM!The
CREATE TABLE IF NOT EXISTSstatement ensures that the table is only created if it does not already exist.
2-2: LGTM!The
idcolumn is correctly defined as the primary key with auto-increment.
3-3: LGTM!The
gmt_createcolumn is correctly defined to store the creation time with a comment.
4-4: LGTM!The
gmt_modifiedcolumn is correctly defined to store the modification time with a comment.
5-5: LGTM!The
fullnamecolumn is correctly defined to store the package name with a comment.
6-6: LGTM!The
versioncolumn is correctly defined to store the package version with a comment.
7-7: LGTM!The
file_typecolumn is correctly defined to store the file type with a comment.
8-8: LGTM!The
file_pathcolumn is correctly defined to store the NFS file path with a comment.
9-9: LGTM!The primary key constraint is correctly defined for the
idcolumn.
10-10: LGTM!The unique key constraint is correctly defined for the
file_pathcolumn.
11-11: LGTM!The unique key constraint is correctly defined for the combination of
fullname,file_type, andversioncolumns.
12-12: LGTM!The storage engine and default character set are correctly defined for the table.
app/repository/model/ProxyCache.ts (12)
1-1: LGTM!The import of
AttributeandModelfrom@eggjs/tegg/ormis necessary for defining the model and its attributes.
2-2: LGTM!The import of
DataTypesandBonefromleoricis necessary for defining the data types and extending theBoneclass.
3-3: LGTM!The import of
DIST_NAMESfrom../../core/entity/Packageis necessary for defining thefileTypeattribute.
5-5: LGTM!The
ProxyCachemodel class is correctly defined with the@Modeldecorator.
6-6: LGTM!Extending the
Boneclass is necessary for defining the model.
7-10: LGTM!The
idattribute is correctly defined as the primary key with theBIGINTdata type and auto-increment configuration.
13-14: LGTM!The
createdAtattribute is correctly defined with theDATEdata type and a custom name.
16-17: LGTM!The
updatedAtattribute is correctly defined with theDATEdata type and a custom name.
19-20: LGTM!The
fullnameattribute is correctly defined with theSTRINGdata type and a length of 214.
22-23: LGTM!The
fileTypeattribute is correctly defined with theSTRINGdata type and a length of 30.
25-28: LGTM!The
filePathattribute is correctly defined with theSTRINGdata type and a length of 512, and a unique constraint.
30-31: LGTM!The
versionattribute is correctly defined with theSTRINGdata type and a length of 214.app/port/controller/ProxyCacheController.ts (5)
33-48: Ensure proxy mode is enabled before listing proxy caches.The method correctly checks if the proxy mode is enabled before proceeding. This is crucial for ensuring that the functionality is only available when the proxy mode is active.
50-63: Ensure proxy mode is enabled and parameters are correctly handled.The method correctly checks if the proxy mode is enabled and handles parameters appropriately. This ensures that the functionality is only available when the proxy mode is active and the parameters are correctly processed.
65-97: Ensure proxy mode is enabled and handle task creation logic.The method correctly checks if the proxy mode is enabled, handles parameters appropriately, and includes logic for task creation. This ensures that the functionality is only available when the proxy mode is active, the parameters are correctly processed, and tasks are created as needed.
99-127: Ensure admin privileges and proxy mode are enabled before removing proxy caches.The method correctly checks for admin privileges and ensures the proxy mode is enabled before proceeding. This is crucial for ensuring that only authorized users can perform this action and that the functionality is only available when the proxy mode is active.
129-155: Ensure admin privileges and proxy mode are enabled before truncating proxy caches.The method correctly checks for admin privileges and ensures the proxy mode is enabled before proceeding. It also handles background tasks for removing the proxy cache directory. This ensures that only authorized users can perform this action, the functionality is only available when the proxy mode is active, and background tasks are managed properly.
test/port/controller/ProxyCacheController/index.test.ts (5)
45-67: Ensure comprehensive test coverage for listing proxy caches.The test case covers various scenarios for listing proxy caches, including checks for proxy mode and pagination. This ensures that the functionality is thoroughly tested and behaves as expected in different situations.
70-87: Ensure comprehensive test coverage for showing specific proxy caches.The test case covers various scenarios for showing specific proxy caches, including checks for proxy mode and not found scenarios. This ensures that the functionality is thoroughly tested and behaves as expected in different situations.
90-116: Ensure comprehensive test coverage for refreshing proxy caches.The test case covers various scenarios for refreshing proxy caches, including checks for proxy mode and task creation. This ensures that the functionality is thoroughly tested and behaves as expected in different situations.
118-159: Ensure comprehensive test coverage for removing proxy caches.The test case covers various scenarios for removing proxy caches, including checks for admin privileges, proxy mode, and not found scenarios. This ensures that the functionality is thoroughly tested and behaves as expected in different situations.
161-189: Ensure comprehensive test coverage for truncating all proxy caches.The test case covers various scenarios for truncating all proxy caches, including checks for admin privileges and proxy mode. This ensures that the functionality is thoroughly tested and behaves as expected in different situations.
test/repository/ProxyCachePepository.test.ts (7)
20-27: LGTM!The test case for creating a new proxy cache is correct and covers the necessary assertions.
29-37: LGTM!The test case for updating a proxy cache is correct and covers the necessary assertions.
39-42: LGTM!The test case for listing proxy caches is correct and covers the necessary assertions.
44-47: LGTM!The test case for querying a non-existent proxy cache is correct and covers the necessary assertions.
49-52: LGTM!The test case for querying an existing proxy cache is correct and covers the necessary assertions.
54-58: LGTM!The test case for removing a proxy cache is correct and covers the necessary assertions.
60-68: LGTM!The test case for truncating proxy caches is correct and covers the necessary assertions.
app/repository/ProxyCacheRepository.ts (5)
33-37: LGTM!The method for finding a proxy cache entity is correct and covers the necessary functionality.
39-43: LGTM!The method for finding proxy cache entities is correct and covers the necessary functionality.
45-53: LGTM!The method for listing cached files is correct and covers the necessary functionality.
55-57: LGTM!The method for removing a proxy cache entity is correct and covers the necessary functionality.
59-61: LGTM!The method for truncating all proxy cache entities is correct and covers the necessary functionality.
Comments failed to post (1)
app/repository/ProxyCacheRepository.ts
15-31: Improve error message handling and add unit tests.
The custom error message modification in the
saveProxyCachemethod lacks test coverage. This is essential for ensuring that the error handling behaves as expected.Would you like me to help by adding some test cases for this scenario?
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range, codebase verification and nitpick comments (5)
app/port/schedule/CheckProxyCacheUpdateWorker.ts (1)
45-46: Improve error logging for better debugging.The current error logging is too generic. It's beneficial to include more context about the error, especially since this is a background worker where direct debugging can be challenging.
- this.logger.error(err); + this.logger.error('Error processing proxy cache update:', err);Tools
GitHub Check: codecov/patch
[warning] 45-46: app/port/schedule/CheckProxyCacheUpdateWorker.ts#L45-L46
Added lines #L45 - L46 were not covered by testsapp/core/entity/Task.ts (1)
256-278: Ensure test coverage forcreateUpdateProxyCachemethodNo test cases were found for the
createUpdateProxyCachemethod, specifically for the new logic that throws an error when the file type is not a package manifest. Please add test cases to cover this logic to ensure proper test coverage.
- Add test cases for
createUpdateProxyCachein the relevant test files.- Ensure that the error-throwing logic is tested.
Analysis chain
New method
createUpdateProxyCacheis correctly implemented.The new method correctly creates update proxy cache tasks and integrates well with existing code.
However, ensure that the new logic for throwing an error when the file type is not a package manifest is covered by tests.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for the new task creation logic. # Test: Search for test cases covering the new logic. Expect: At least one test case covering the error-throwing logic. rg --type typescript -A 50 $'createUpdateProxyCache'Length of output: 86
Script:
#!/bin/bash # Description: Verify test coverage for the new task creation logic. # Test: Search for test cases covering the new logic. Expect: At least one test case covering the error-throwing logic. rg --glob '*.ts' -A 50 'createUpdateProxyCache'Length of output: 6935
Script:
#!/bin/bash # Description: Verify test coverage for the new task creation logic. # Search for test files that include references to ProxyCacheService or Task. rg --glob '*test*.ts' -l 'ProxyCacheService|Task'Length of output: 1615
Script:
#!/bin/bash # Description: Verify test coverage for the new task creation logic. # Search within the identified test files for test cases that involve createUpdateProxyCache. rg 'createUpdateProxyCache' test/core/service/ProxyCacheService.test.ts test/core/service/TaskService/findExecuteTask.test.tsLength of output: 125
Script:
#!/bin/bash # Description: Verify test coverage for the new task creation logic. # Search all test files for test cases that involve createUpdateProxyCache. rg 'createUpdateProxyCache' --glob '*test*.ts'Length of output: 46
Tools
Biome
[error] 275-276: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
GitHub Check: codecov/patch
[warning] 258-259: app/core/entity/Task.ts#L258-L259
Added lines #L258 - L259 were not covered by testsapp/core/service/ProxyCacheService.ts (3)
54-57: Consider logging the error before throwing it.Logging the error before throwing it can help in debugging and monitoring.
- throw new ForbiddenError(`stop proxy by block list: ${JSON.stringify(this.config.cnpmcore.syncPackageBlockList)}`); + this.logger.warn(`stop proxy by block list: ${JSON.stringify(this.config.cnpmcore.syncPackageBlockList)}`); + throw new ForbiddenError(`stop proxy by block list: ${JSON.stringify(this.config.cnpmcore.syncPackageBlockList)}`);Tools
GitHub Check: codecov/patch
[warning] 56-57: app/core/service/ProxyCacheService.ts#L56-L57
Added lines #L56 - L57 were not covered by tests
102-108: Consider adding logging for cache removals.Adding logging statements can help in tracking successful cache removals and diagnosing issues.
+ this.logger.info(`Successfully removed cache for ${fullname} with fileType ${fileType}`);
118-159: Consider adding logging for the start and successful completion of tasks.Adding logging statements can help in comprehensive debugging and monitoring.
+ this.logger.info(`[ProxyCacheService.executeTask:start] taskId: ${task.taskId}, targetName: ${task.targetName}`); + this.logger.info(`[ProxyCacheService.executeTask:success] taskId: ${task.taskId}, targetName: ${task.targetName}`);Tools
GitHub Check: codecov/patch
[warning] 132-139: app/core/service/ProxyCacheService.ts#L132-L139
Added lines #L132 - L139 were not covered by tests
[warning] 153-157: app/core/service/ProxyCacheService.ts#L153-L157
Added lines #L153 - L157 were not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (28)
- app/common/adapter/NPMRegistry.ts (1 hunks)
- app/common/constants.ts (1 hunks)
- app/common/enum/Task.ts (1 hunks)
- app/core/entity/ProxyCache.ts (1 hunks)
- app/core/entity/Task.ts (5 hunks)
- app/core/service/ProxyCacheService.ts (1 hunks)
- app/port/controller/ProxyCacheController.ts (1 hunks)
- app/port/controller/package/DownloadPackageVersionTar.ts (3 hunks)
- app/port/controller/package/ShowPackageController.ts (3 hunks)
- app/port/controller/package/ShowPackageVersionController.ts (1 hunks)
- app/port/schedule/CheckProxyCacheUpdateWorker.ts (1 hunks)
- app/port/schedule/CheckRecentlyUpdatedPackages.ts (1 hunks)
- app/port/schedule/SyncProxyCacheWorker.ts (1 hunks)
- app/repository/ProxyCacheRepository.ts (1 hunks)
- app/repository/model/ProxyCache.ts (1 hunks)
- package.json (1 hunks)
- sql/3.47.0.sql (1 hunks)
- test/core/service/ProxyCacheService.test.ts (1 hunks)
- test/fixtures/registry.npmjs.org/abbreviated_foobar.json (1 hunks)
- test/fixtures/registry.npmjs.org/foobar/1.0.0/abbreviated.json (1 hunks)
- test/fixtures/registry.npmjs.org/foobar/1.0.0/package.json (1 hunks)
- test/port/controller/ProxyCacheController/index.test.ts (1 hunks)
- test/port/controller/package/DownloadPackageVersionTarController.test.ts (2 hunks)
- test/port/controller/package/ShowPackageController.test.ts (2 hunks)
- test/port/controller/package/ShowPackageVersionController.test.ts (2 hunks)
- test/repository/ProxyCachePepository.test.ts (1 hunks)
- test/schedule/CheckProxyCacheUpdateWorker.test.ts (1 hunks)
- test/schedule/SyncProxyCacheWorker.test.ts (1 hunks)
Files skipped from review due to trivial changes (5)
- app/common/enum/Task.ts
- package.json
- test/fixtures/registry.npmjs.org/abbreviated_foobar.json
- test/fixtures/registry.npmjs.org/foobar/1.0.0/abbreviated.json
- test/fixtures/registry.npmjs.org/foobar/1.0.0/package.json
Files skipped from review as they are similar to previous changes (3)
- app/repository/model/ProxyCache.ts
- sql/3.47.0.sql
- test/repository/ProxyCachePepository.test.ts
Additional context used
GitHub Check: codecov/patch
app/port/schedule/CheckProxyCacheUpdateWorker.ts
[warning] 45-46: app/port/schedule/CheckProxyCacheUpdateWorker.ts#L45-L46
Added lines #L45 - L46 were not covered by testsapp/port/schedule/SyncProxyCacheWorker.ts
[warning] 43-46: app/port/schedule/SyncProxyCacheWorker.ts#L43-L46
Added lines #L43 - L46 were not covered by tests
[warning] 51-51: app/port/schedule/SyncProxyCacheWorker.ts#L51
Added line #L51 was not covered by testsapp/repository/ProxyCacheRepository.ts
[warning] 26-28: app/repository/ProxyCacheRepository.ts#L26-L28
Added lines #L26 - L28 were not covered by testsapp/port/controller/ProxyCacheController.ts
[warning] 117-118: app/port/controller/ProxyCacheController.ts#L117-L118
Added lines #L117 - L118 were not covered by testsapp/core/entity/Task.ts
[warning] 258-259: app/core/entity/Task.ts#L258-L259
Added lines #L258 - L259 were not covered by testsapp/core/service/ProxyCacheService.ts
[warning] 56-57: app/core/service/ProxyCacheService.ts#L56-L57
Added lines #L56 - L57 were not covered by tests
[warning] 132-139: app/core/service/ProxyCacheService.ts#L132-L139
Added lines #L132 - L139 were not covered by tests
[warning] 153-157: app/core/service/ProxyCacheService.ts#L153-L157
Added lines #L153 - L157 were not covered by tests
[warning] 177-177: app/core/service/ProxyCacheService.ts#L177
Added line #L177 was not covered by tests
[warning] 248-250: app/core/service/ProxyCacheService.ts#L248-L250
Added lines #L248 - L250 were not covered by tests
Biome
app/core/entity/Task.ts
[error] 275-276: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (49)
app/common/constants.ts (3)
4-4: Addition ofPROXY_CACHE_DIR_NAMElooks good.The constant is correctly defined and follows the naming conventions.
5-5: Addition ofABBREVIATED_META_TYPElooks good.The constant is correctly defined and follows the naming conventions.
11-11: Update toSyncModeenum looks good.The addition of the
proxyvalue is consistent with the existing enum values and aligns with the new proxy mode functionality.test/schedule/CheckProxyCacheUpdateWorker.test.ts (2)
1-8: Addition of imports looks good.The imports are correctly added and necessary for the new test case.
12-26: Addition of test case looks good.The test case is well-defined and covers the necessary functionality. It uses appropriate mocking and assertions.
app/core/entity/ProxyCache.ts (4)
1-5: Addition of imports looks good.The imports are correctly added and necessary for the
ProxyCacheentity.
6-10: Addition ofProxyCacheDatainterface looks good.The interface is well-defined and includes all necessary properties. It is consistent with the existing data structures.
12-12: Addition ofCreateProxyCacheDatatype looks good.The type is well-defined and includes all necessary properties. It is consistent with the existing data structures.
14-42: Addition ofProxyCacheclass looks good.The class is well-defined and includes all necessary properties and methods. It is consistent with the existing entity structures.
test/schedule/SyncProxyCacheWorker.test.ts (2)
1-6: Addition of imports looks good.The imports are correctly added and necessary for the new test case.
10-35: Addition of test case looks good.The test case is well-defined and covers the necessary functionality. It uses appropriate mocking and assertions.
app/port/schedule/CheckRecentlyUpdatedPackages.ts (1)
33-33: LGTM! Ensures correct behavior in proxy mode.Adding
SyncMode.proxyto thenotAllowUpdateModeListensures that the check for recently updated packages does not run in proxy mode, which is appropriate as proxy mode fetches data directly from the upstream repository.app/common/adapter/NPMRegistry.ts (1)
114-114: LGTM! Necessary for proxy mode functionality.Making the
genAuthorizationHeadermethod public allows it to be accessed from outside the class, which is necessary to support the new proxy mode functionality that requires setting authorization headers for upstream requests.app/port/controller/package/ShowPackageController.ts (5)
15-18: LGTM! Imports necessary for proxy mode.The imports for
ABBREVIATED_META_TYPE,SyncMode,ProxyCacheService, andcalculateIntegrityare necessary for handling proxy mode in theshowmethod.
26-27: LGTM! InjectingProxyCacheService.Injecting the
ProxyCacheServiceis necessary for handling proxy mode in theshowmethod.
38-38: LGTM! Determine if full manifests are requested.The
isFullManifestsvariable determines if full manifests are requested based on theAcceptheader.
72-79: LGTM! Handling proxy mode in theshowmethod.The changes handle proxy mode by fetching package manifests from the proxy cache service. This ensures that the system can return cached manifests or fetch them from the upstream repository if not found.
81-86: LGTM! Handling sync mode in theshowmethod.The changes handle sync mode by listing package manifests using the package manager service.
app/port/controller/ProxyCacheController.ts (6)
1-23: LGTM! Imports and class definition forProxyCacheController.The imports and class definition for
ProxyCacheControllerare necessary for handling HTTP requests for managing proxy caches.
24-48: LGTM! Method for listing proxy caches.The
listProxyCachemethod lists proxy caches with pagination. It includes appropriate error handling for when proxy mode is not enabled.
50-63: LGTM! Method for showing proxy caches.The
showProxyCachesmethod shows proxy caches for a specific package with pagination. It includes appropriate error handling for when proxy mode is not enabled.
65-97: LGTM! Method for refreshing proxy caches.The
refreshProxyCachesmethod refreshes proxy caches for a specific package. It includes appropriate error handling for when proxy mode is not enabled and when no caches are found.
99-127: LGTM! Method for removing proxy caches.The
removeProxyCachesmethod removes proxy caches for a specific package. It includes appropriate error handling for when proxy mode is not enabled, when the user is not an admin, and when no caches are found.Tools
GitHub Check: codecov/patch
[warning] 117-118: app/port/controller/ProxyCacheController.ts#L117-L118
Added lines #L117 - L118 were not covered by tests
129-155: LGTM! Method for truncating proxy caches.The
truncateProxyCachesmethod truncates all proxy caches. It includes appropriate error handling for when proxy mode is not enabled and when the user is not an admin.app/port/controller/package/DownloadPackageVersionTar.ts (4)
15-19: New imports are necessary and correctly used.The new imports
SyncMode,ProxyCacheService, andPackageSyncerServiceare necessary for the added proxy mode functionality.
27-31: New injected services are necessary and correctly used.The new injected services
registryManagerService,proxyCacheService, andpackageSyncerServiceare necessary for the added proxy mode functionality.
115-131: New method#getTgzStreamis correctly implemented.The new method correctly fetches the
.tgzfile from the upstream repository in proxy mode and creates a synchronization task.
66-79: New logic for handling proxy mode is correctly implemented.The new logic correctly handles proxy mode when a package version is not found by fetching the
.tgzfile from the upstream repository and creating a synchronization task.However, ensure that all function calls to
downloadhandle the new proxy mode logic correctly.test/port/controller/ProxyCacheController/index.test.ts (7)
1-8: New imports are necessary and correctly used.The new imports
ProxyCache,ProxyCacheRepository,TaskRepository, andSyncModeare necessary for the added proxy cache functionality.
14-43: Setup code is correctly implemented.The setup code correctly initializes
proxyCacheRepositoryand saves the necessary proxy cache entries for the tests.
45-67: Test cases forlistProxyCacheare correctly implemented.The test cases comprehensively cover the
listProxyCachefunctionality, including different sync modes and pagination.
70-87: Test cases forshowProxyCachesare correctly implemented.The test cases comprehensively cover the
showProxyCachesfunctionality, including different sync modes and not found scenarios.
90-115: Test cases forrefreshProxyCachesare correctly implemented.The test cases comprehensively cover the
refreshProxyCachesfunctionality, including different sync modes and task creation.
118-157: Test cases forremoveProxyCachesare correctly implemented.The test cases comprehensively cover the
removeProxyCachesfunctionality, including different sync modes, authorization, and not found scenarios.
161-189: Test cases fortruncateProxyCachesare correctly implemented.The test cases comprehensively cover the
truncateProxyCachesfunctionality, including different sync modes and authorization.app/core/entity/Task.ts (1)
Line range hint
47-76:
New types are necessary and correctly defined.The new types
UpdateProxyCacheTaskOptionsandCreateUpdateProxyCacheTaskDataare necessary for the new proxy cache functionality and are correctly defined.test/core/service/ProxyCacheService.test.ts (9)
1-9: New imports are necessary and correctly used.The new imports
ProxyCacheService,ProxyCacheRepository,TaskService, andDIST_NAMESare necessary for the added proxy cache functionality.
12-18: Setup code is correctly implemented.The setup code correctly initializes
proxyCacheServiceandproxyCacheRepositoryfor the tests.
20-51: Test cases forgetPackageManifestare correctly implemented.The test cases comprehensively cover the
getPackageManifestfunctionality, including fetching from the upstream repository and reading from NFS.
54-88: Test cases forgetPackageVersionManifestare correctly implemented.The test cases comprehensively cover the
getPackageVersionManifestfunctionality, including fetching from the upstream repository and reading from NFS.
90-114: Test cases forgetPackageVersionManifestare correctly implemented.The test cases comprehensively cover the
getPackageVersionManifestfunctionality, including fetching the correct version via tag and caching the package manifest.
117-195: Test cases forrewriteManifestAndStoreare correctly implemented.The test cases comprehensively cover the
rewriteManifestAndStorefunctionality, including fetching full and abbreviated package manifests from the upstream repository.
199-219: Test cases forremoveProxyCacheare correctly implemented.The test cases comprehensively cover the
removeProxyCachefunctionality, including removing cached entries from the repository.
222-235: Test cases forcreateTaskandfindExecuteTaskare correctly implemented.The test cases comprehensively cover the
createTaskandfindExecuteTaskfunctionality, including creating tasks and finding them in the repository.
238-275: Test cases forexecuteTaskare correctly implemented.The test cases comprehensively cover the
executeTaskfunctionality, including handling not found errors and successful updates.However, ensure that the tests also cover scenarios where the upstream registry returns errors or non-200 status codes.
Verification successful
Test cases for
executeTaskcover scenarios with upstream registry errors.The test cases in
test/core/service/PackageSyncerService/executeTask.test.tsinclude handling non-200 status codes, confirming that error scenarios from the upstream registry are tested.
test/core/service/PackageSyncerService/executeTask.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for error handling in `getSourceManifestAndCache`. # Test: Search for test cases handling non-200 status codes. Expect: At least one test case covering these scenarios. rg --type typescript -A 50 $'getSourceManifestAndCache' | grep "status !== 200"Length of output: 113
Script:
#!/bin/bash # Description: Verify test coverage for error handling in `getSourceManifestAndCache`. # Test: Search for test cases handling non-200 status codes. Expect: At least one test case covering these scenarios. rg --type ts -A 50 'getSourceManifestAndCache' | grep -A 10 -B 10 'status !== 200'Length of output: 82
Script:
#!/bin/bash # Description: Verify test coverage for error handling in `getSourceManifestAndCache`. # Step 1: Search for any HTTP status code checks within the `getSourceManifestAndCache` function. rg --type ts -A 50 'getSourceManifestAndCache' | grep -E 'status [!=<>]' # Step 2: Identify related test cases that handle these scenarios. rg --type ts -A 50 'status [!=<>]'Length of output: 320634
app/core/service/ProxyCacheService.ts (3)
110-112: LGTM!The method is correctly implemented.
114-116: LGTM!The method is correctly implemented.
211-235: LGTM!The method is correctly implemented.
test/port/controller/package/ShowPackageController.test.ts (1)
868-883: LGTM!The new test case for reading the manifest from the source in proxy mode is well-written and covers the necessary assertions.
| this.logger.error(err); | ||
| } |
There was a problem hiding this comment.
Add unit tests to cover error handling.
The error handling block in the subscribe method lacks test coverage. This is crucial for ensuring that the system behaves as expected under failure conditions.
Would you like me to help by adding some test cases for this scenario?
Tools
GitHub Check: codecov/patch
[warning] 45-46: app/port/schedule/CheckProxyCacheUpdateWorker.ts#L45-L46
Added lines #L45 - L46 were not covered by tests
| if (this.config.cnpmcore.syncMode === SyncMode.proxy) { | ||
| // proxy mode | ||
| const fileType = isFullManifests ? DIST_NAMES.FULL_MANIFESTS : DIST_NAMES.ABBREVIATED_MANIFESTS; | ||
| const pkgManifest = await this.proxyCacheService.getPackageManifest(fullname, fileType); | ||
|
|
||
| const nfsBytes = Buffer.from(JSON.stringify(pkgManifest)); | ||
| const { shasum: etag } = await calculateIntegrity(nfsBytes); | ||
| result = { data: pkgManifest, etag, blockReason: '' }; |
There was a problem hiding this comment.
Add unit tests for the show method in proxy mode.
Add unit tests to cover the error handling and cache refresh task creation in the show method when in proxy mode.
Do you want me to help by adding some test cases for this scenario?
Also applies to: 81-86
| this.logger.info('[SyncProxyCacheWorker:subscribe:executeTask] current sync task count %s, exceed max concurrent tasks %s', | ||
| executingCount, this.config.cnpmcore.syncPackageWorkerMaxConcurrentTasks); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Add unit tests to cover the concurrency control logic.
The concurrency control logic in the subscribe method lacks test coverage. This is critical for ensuring that the system correctly handles multiple concurrent tasks.
Would you like me to help by adding some test cases for this scenario?
Tools
GitHub Check: codecov/patch
[warning] 43-46: app/port/schedule/SyncProxyCacheWorker.ts#L43-L46
Added lines #L43 - L46 were not covered by tests
| task = await this.proxyCacheService.findExecuteTask(); | ||
| } | ||
| } catch (err) { | ||
| this.logger.error('[SyncProxyCacheWorker:subscribe:executeTask:error][%s] %s', executingCount, err); |
There was a problem hiding this comment.
Add unit tests to cover error handling in the subscribe method.
The error handling in the subscribe method lacks test coverage. It's important to ensure that the system behaves as expected under failure conditions.
Would you like me to help by adding some test cases for this scenario?
Tools
GitHub Check: codecov/patch
[warning] 51-51: app/port/schedule/SyncProxyCacheWorker.ts#L51
Added line #L51 was not covered by tests
| throw new NotFoundError(); | ||
| } |
There was a problem hiding this comment.
Add unit tests for the removeProxyCaches method.
The removeProxyCaches method lacks test coverage for the scenario where no caches are found.
Do you want me to help by adding some test cases for this scenario?
Tools
GitHub Check: codecov/patch
[warning] 117-118: app/port/controller/ProxyCacheController.ts#L117-L118
Added lines #L117 - L118 were not covered by tests
| it('should read package version manifest from source in proxy mode', async () => { | ||
| mock(app.config.cnpmcore, 'syncMode', SyncMode.proxy); | ||
| mock(app.config.cnpmcore, 'redirectNotFound', false); | ||
| const data = await TestUtil.readJSONFile(TestUtil.getFixtures('registry.npmjs.org/foobar/1.0.0/abbreviated.json')); | ||
| app.mockHttpclient('https://registry.npmjs.org/foobar/1.0.0', 'GET', { | ||
| data, | ||
| persist: false, | ||
| }); | ||
| const res = await app.httpRequest() | ||
| .get('/foobar/1.0.0') | ||
| .set('user-agent', publisher.ua + ' node/16.0.0') | ||
| .set('Accept', 'application/vnd.npm.install-v1+json'); | ||
| assert(res.status === 200); | ||
| assert(res.body.dist.tarball.includes(app.config.cnpmcore.registry)); | ||
| }); |
There was a problem hiding this comment.
Ensure comprehensive test coverage for proxy mode.
Currently, only one test case covers the proxy mode scenario. To ensure robustness, consider adding more tests for edge cases and error scenarios.
- Add tests for different response statuses from the upstream registry.
- Include tests for network failures or timeouts.
- Verify behavior with various package version formats and invalid inputs.
Do you want me to generate additional test cases or open a GitHub issue to track this task?
test/port/controller/package/DownloadPackageVersionTarController.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (8)
app/port/controller/package/DownloadPackageVersionTar.ts (1)
115-131: NewgetTgzProxyStreammethod looks good.The method encapsulates the logic for obtaining the tar stream from the proxy cache service. The logic is correct and follows best practices.
Consider adding logging for background task.
Adding logging statements can help in tracking the start and successful completion of the background task.
+ ctx.logger.info('[DownloadPackageVersionTarController.createSyncTask:start] taskId: %s, fullname: %s', task.taskId, fullname);test/core/service/ProxyCacheService.test.ts (2)
216-238: Test cases forremoveProxyCachelook good.The test cases cover scenarios such as removing cache and follow best practices.
Consider adding logging for cache removals.
Adding logging statements can help in tracking successful cache removals and diagnosing issues.
+ this.logger.info(`Successfully removed cache for ${fullname} with fileType ${fileType}`);
271-316: Test cases forexecuteTasklook good.The test cases cover scenarios such as throwing not found error and updating success. They follow best practices.
Consider adding logging for task execution.
Adding logging statements can help in tracking the start and successful completion of tasks.
+ this.logger.info(`[ProxyCacheService.executeTask:start] taskId: ${task.taskId}, targetName: ${task.targetName}`); + this.logger.info(`[ProxyCacheService.executeTask:success] taskId: ${task.taskId}, targetName: ${task.targetName}`);app/core/service/ProxyCacheService.ts (5)
61-76:getPackageManifestmethod looks good.The method retrieves the package manifest from NFS or rewrites and stores it. The logic is correct and follows best practices.
Use
runInBackgroundfor the background task.Using
runInBackgroundcan improve readability and maintainability.- this.backgroundTaskHelper.run(async () => { + this.backgroundTaskHelper.runInBackground(async () => {
79-100:getPackageVersionManifestmethod looks good.The method retrieves the package version manifest from NFS or rewrites and stores it. The logic is correct and follows best practices.
Use
runInBackgroundfor the background task.Using
runInBackgroundcan improve readability and maintainability.- this.backgroundTaskHelper.run(async () => { + this.backgroundTaskHelper.runInBackground(async () => {
102-108:removeProxyCachemethod looks good.The method removes the proxy cache from NFS and the repository. The logic is correct and follows best practices.
Consider adding logging for cache removals.
Adding logging statements can help in tracking successful cache removals and diagnosing issues.
+ this.logger.info(`Successfully removed cache for ${fullname} with fileType ${fileType}`);
118-149:executeTaskmethod looks good.The method executes a task, updates the cache, and logs the results. The logic is correct and follows best practices.
Consider adding logging for task execution.
Adding logging statements can help in tracking the start and successful completion of tasks.
+ this.logger.info(`[ProxyCacheService.executeTask:start] taskId: ${task.taskId}, targetName: ${task.targetName}`); + this.logger.info(`[ProxyCacheService.executeTask:success] taskId: ${task.taskId}, targetName: ${task.targetName}`);
151-199:rewriteManifestAndStoremethod looks good.The method rewrites the manifest and stores it in NFS. The logic is correct and follows best practices.
Consider adding validation for upstream responses.
Adding a check to ensure that the response from the upstream source is valid (e.g., checking for a 200 status code) before proceeding with the rewrite and storage of the manifest can improve robustness.
+ if (responseResult.status !== 200) { + throw new Error('Invalid response from upstream source'); + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- app/core/service/ProxyCacheService.ts (1 hunks)
- app/port/controller/package/DownloadPackageVersionTar.ts (3 hunks)
- test/core/service/ProxyCacheService.test.ts (1 hunks)
- test/port/controller/package/DownloadPackageVersionTarController.test.ts (2 hunks)
Additional context used
Learnings (2)
app/core/service/ProxyCacheService.ts (1)
Learnt from: hezhengxu2018 PR: cnpm/cnpmcore#571 File: app/core/service/ProxyCacheService.ts:161-209 Timestamp: 2024-07-19T06:26:05.533Z Learning: In the `ProxyCacheService` class, requests to the upstream repository already throw an error if they fail, making additional checks for the response status unnecessary.test/port/controller/package/DownloadPackageVersionTarController.test.ts (1)
Learnt from: hezhengxu2018 PR: cnpm/cnpmcore#571 File: test/port/controller/package/DownloadPackageVersionTarController.test.ts:304-315 Timestamp: 2024-07-19T06:21:32.040Z Learning: In proxy mode, the response status should be 200 when the package version is not found.
Additional comments not posted (15)
app/port/controller/package/DownloadPackageVersionTar.ts (3)
18-20: Imports look good.The new import statements for
ProxyCacheService,PackageSyncerService, andRegistryManagerServiceare appropriate.
27-31: Injected services look good.The new injected services
registryManagerService,proxyCacheService, andpackageSyncerServiceare appropriate.
66-79: Modifications to thedownloadmethod look good.The modifications improve error handling and add support for proxy mode. The logic is correct and follows best practices.
test/core/service/ProxyCacheService.test.ts (6)
15-18: Setup inbeforeEachlooks good.The initialization of
proxyCacheServiceandproxyCacheRepositoryis appropriate.
20-29: Test cases forgetPackageVersionTarResponselook good.The test cases cover scenarios such as hitting the block list and follow best practices.
32-63: Test cases forgetPackageManifestlook good.The test cases cover scenarios such as invoking
rewriteManifestAndStoreand reading data from NFS. They follow best practices.
66-127: Test cases forgetPackageVersionManifestlook good.The test cases cover scenarios such as invoking
rewriteManifestAndStore, reading data from NFS, and getting the correct version via tag. They follow best practices.
130-213: Test cases forrewriteManifestAndStorelook good.The test cases cover scenarios such as getting full package manifest, abbreviated package manifest, full package version manifest, and abbreviated package version manifest. They follow best practices.
241-268: Test cases forcreateTaskandfindExecuteTasklook good.The test cases cover scenarios such as creating a task and finding an executed task. They follow best practices.
app/core/service/ProxyCacheService.ts (5)
54-59:getPackageVersionTarResponsemethod looks good.The method checks if the package is in the block list and retrieves the proxy response. The logic is correct and follows best practices.
110-112:createTaskmethod looks good.The method creates a task using the
taskService. The logic is correct and follows best practices.
114-116:findExecuteTaskmethod looks good.The method finds an execute task using the
taskService. The logic is correct and follows best practices.
201-225:getProxyResponsemethod looks good.The method retrieves the proxy response from the upstream registry. The logic is correct and follows best practices.
228-244:getUpstream*methods look good.The methods retrieve various manifests from the upstream registry. The logic is correct and follows best practices.
test/port/controller/package/DownloadPackageVersionTarController.test.ts (1)
304-320: Ensure comprehensive test coverage.The new test case for proxy mode is well-structured and covers the creation of a synchronization task. However, consider adding assertions to verify the content of the response body and the behavior of the synchronization task.
+ assert(res.body.message.includes('task created'));
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/port/controller/package/DownloadPackageVersionTarController.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/port/controller/package/DownloadPackageVersionTarController.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (6)
app/core/service/ProxyCacheService.ts (6)
54-59: Consider adding logging for block list check.Adding a logging statement before throwing the
ForbiddenErrorcan help in diagnosing issues related to blocked packages.+ if (this.config.cnpmcore.syncPackageBlockList.includes(fullname)) { + this.logger.info(`Package ${fullname} is blocked by the block list`); throw new ForbiddenError(`stop proxy by block list: ${JSON.stringify(this.config.cnpmcore.syncPackageBlockList)}`); }
104-110: Consider adding logging for cache removal.Adding logging statements can help in tracking successful cache removals and diagnosing issues.
await this.nfsAdapter.remove(storeKey); await this.proxyCacheRepository.removeProxyCache(fullname, fileType, version); + this.logger.info(`Successfully removed cache for ${fullname} with fileType ${fileType}`);
120-152: Consider adding logging for task execution.Adding logging statements for the start and success of the task can help in tracking task execution and diagnosing issues.
logs.push(`[${isoNow()}] 🚧🚧🚧🚧🚧 Start update "${fullname}-${fileType}" 🚧🚧🚧🚧🚧`); + this.logger.info(`Starting task execution for ${fullname} with fileType ${fileType}`); try { const cachedFiles = await this.proxyCacheRepository.findProxyCache(fullname, fileType); if (!cachedFiles) throw new Error('task params error, can not found record in repo.'); cachedManifest = await this.getRewrittenManifest<typeof fileType>(fullname, fileType); await this.storeRewrittenManifest(cachedManifest, fullname, fileType); ProxyCache.update(cachedFiles); await this.proxyCacheRepository.saveProxyCache(cachedFiles); } catch (error) { task.error = error; logs.push(`[${isoNow()}] ❌ ${task.error}`); logs.push(`[${isoNow()}] ❌❌❌❌❌ ${fullname}-${fileType} ${version ?? ''} ❌❌❌❌❌`); await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n')); this.logger.info('[ProxyCacheService.executeTask:fail] taskId: %s, targetName: %s, %s', task.taskId, task.targetName, task.error); return; } logs.push(`[${isoNow()}] 🟢 Update Success.`); + this.logger.info(`Successfully executed task for ${fullname} with fileType ${fileType}`); const isFullManifests = fileType === DIST_NAMES.FULL_MANIFESTS; const cachedKey = await this.cacheService.getPackageEtag(fullname, isFullManifests); if (cachedKey) { const cacheBytes = Buffer.from(JSON.stringify(cachedManifest)); const { shasum: etag } = await calculateIntegrity(cacheBytes); await this.cacheService.savePackageEtagAndManifests(fullname, isFullManifests, etag, cacheBytes); logs.push(`[${isoNow()}] 🟢 Update Cache Success.`); } await this.taskService.finishTask(task, TaskState.Success, logs.join('\n'));
154-193: Consider adding logging for manifest retrieval.Adding logging statements for the start and success of the manifest retrieval can help in tracking the process and diagnosing issues.
switch (fileType) { case DIST_NAMES.FULL_MANIFESTS: + this.logger.info(`Retrieving full manifests for ${fullname}`); responseResult = await this.getUpstreamFullManifests(fullname); break; case DIST_NAMES.ABBREVIATED_MANIFESTS: + this.logger.info(`Retrieving abbreviated manifests for ${fullname}`); responseResult = await this.getUpstreamAbbreviatedManifests(fullname); break; case DIST_NAMES.MANIFEST: + this.logger.info(`Retrieving package version manifest for ${fullname} with version ${versionOrTag}`); responseResult = await this.getUpstreamPackageVersionManifest(fullname, versionOrTag!); break; case DIST_NAMES.ABBREVIATED: + this.logger.info(`Retrieving abbreviated package version manifest for ${fullname} with version ${versionOrTag}`); responseResult = await this.getUpstreamAbbreviatedPackageVersionManifest(fullname, versionOrTag!); break; default: break; } // replace tarball url const manifest = responseResult.data; const { sourceRegistry, registry } = this.config.cnpmcore; if (isPkgManifest(fileType)) { // pkg manifest const versionMap = manifest.versions || {}; for (const key in versionMap) { const versionItem = versionMap[key]; if (versionItem?.dist?.tarball) { versionItem.dist.tarball = versionItem.dist.tarball.replace(sourceRegistry, registry); } } } else { // pkg version manifest const distItem = manifest.dist || {}; if (distItem.tarball) { distItem.tarball = distItem.tarball.replace(sourceRegistry, registry); } } + this.logger.info(`Successfully retrieved and rewrote manifest for ${fullname}`); return manifest;
195-205: Consider adding logging for manifest storage.Adding logging statements for the start and success of the storage process can help in tracking the process and diagnosing issues.
let storeKey: string; if (isPkgManifest(fileType)) { storeKey = `/${PROXY_CACHE_DIR_NAME}/${fullname}/${fileType}`; } else { const version = manifest.version; storeKey = `/${PROXY_CACHE_DIR_NAME}/${fullname}/${version}/${fileType}`; } + this.logger.info(`Storing rewritten manifest for ${fullname} with fileType ${fileType}`); const nfsBytes = Buffer.from(JSON.stringify(manifest)); await this.nfsAdapter.uploadBytes(storeKey, nfsBytes); + this.logger.info(`Successfully stored rewritten manifest for ${fullname} with fileType ${fileType}`);
207-231: Consider adding logging for proxy response retrieval.Adding logging statements for the start and success of the proxy response retrieval can help in tracking the process and diagnosing issues.
const registry = this.npmRegistry.registry; const remoteAuthToken = await this.registryManagerService.getAuthTokenByRegistryHost(registry); const authorization = this.npmRegistry.genAuthorizationHeader(remoteAuthToken); const url = `${this.npmRegistry.registry}${ctx.url}`; + this.logger.info(`Retrieving proxy response for ${url}`); const res = await this.httpclient.request(url, { timing: true, followRedirect: true, retry: 3, dataType: 'stream', timeout: 10000, compressed: true, ...options, headers: { accept: ctx.header?.accept, 'user-agent': ctx.header?.['user-agent'], authorization, 'x-forwarded-for': ctx?.ip, via: `1.1, ${this.config.cnpmcore.registry}`, }, }) as HttpClientResponse; this.logger.info('[ProxyCacheService:getProxyStreamResponse] %s, status: %s', url, res.status); + this.logger.info(`Successfully retrieved proxy response for ${url}`); return res;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/core/service/ProxyCacheService.ts (1 hunks)
- test/core/service/ProxyCacheService.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/core/service/ProxyCacheService.test.ts
Additional context used
Learnings (1)
app/core/service/ProxyCacheService.ts (1)
Learnt from: hezhengxu2018 PR: cnpm/cnpmcore#571 File: app/core/service/ProxyCacheService.ts:161-209 Timestamp: 2024-07-19T06:26:05.533Z Learning: In the `ProxyCacheService` class, requests to the upstream repository already throw an error if they fail, making additional checks for the response status unnecessary.
Additional comments not posted (4)
app/core/service/ProxyCacheService.ts (4)
1-27: Imports and utility functions look good!The imports and utility functions (
isoNow,isPkgManifest) are appropriate and correctly implemented.
112-114: Method looks good!The
createTaskmethod is correctly implemented.
116-118: Method looks good!The
findExecuteTaskmethod is correctly implemented.
234-250: Methods look good!The private methods for retrieving upstream manifests are correctly implemented.
| // used by GET /:fullname/:versionOrTag | ||
| async getPackageVersionManifest(fullname: string, fileType: DIST_NAMES.ABBREVIATED | DIST_NAMES.MANIFEST, versionOrTag: string): Promise<AbbreviatedPackageJSONType|PackageJSONType> { | ||
| let version; | ||
| if (semverValid(versionOrTag)) { | ||
| version = versionOrTag; | ||
| } else { | ||
| const pkgManifest = await this.getPackageManifest(fullname, DIST_NAMES.ABBREVIATED_MANIFESTS); | ||
| const distTags = pkgManifest['dist-tags'] || {}; | ||
| version = distTags[versionOrTag] ? distTags[versionOrTag] : versionOrTag; | ||
| } | ||
| const cachedStoreKey = (await this.proxyCacheRepository.findProxyCache(fullname, fileType, version))?.filePath; | ||
| if (cachedStoreKey) { | ||
| const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey); | ||
| const nfsString = Buffer.from(nfsBytes!).toString(); | ||
| return JSON.parse(nfsString) as PackageJSONType | AbbreviatedPackageJSONType; | ||
| } | ||
| const manifest = await this.getRewrittenManifest(fullname, fileType, versionOrTag); | ||
| this.backgroundTaskHelper.run(async () => { | ||
| await this.storeRewrittenManifest(manifest, fullname, fileType); | ||
| const cachedFiles = ProxyCache.create({ fullname, fileType, version }); | ||
| await this.proxyCacheRepository.saveProxyCache(cachedFiles); | ||
| }); | ||
| return manifest; | ||
| } |
There was a problem hiding this comment.
Improve error handling and logging.
Consider adding error handling for the background task and logging the start and success of the task.
const manifest = await this.getRewrittenManifest(fullname, fileType, versionOrTag);
this.backgroundTaskHelper.run(async () => {
+ try {
+ this.logger.info(`Starting background task to store manifest for ${fullname}`);
await this.storeRewrittenManifest(manifest, fullname, fileType);
const cachedFiles = ProxyCache.create({ fullname, fileType, version });
await this.proxyCacheRepository.saveProxyCache(cachedFiles);
+ this.logger.info(`Successfully stored manifest for ${fullname}`);
+ } catch (error) {
+ this.logger.error(`Failed to store manifest for ${fullname}: ${error.message}`);
+ }
});
return manifest;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.
| // used by GET /:fullname/:versionOrTag | |
| async getPackageVersionManifest(fullname: string, fileType: DIST_NAMES.ABBREVIATED | DIST_NAMES.MANIFEST, versionOrTag: string): Promise<AbbreviatedPackageJSONType|PackageJSONType> { | |
| let version; | |
| if (semverValid(versionOrTag)) { | |
| version = versionOrTag; | |
| } else { | |
| const pkgManifest = await this.getPackageManifest(fullname, DIST_NAMES.ABBREVIATED_MANIFESTS); | |
| const distTags = pkgManifest['dist-tags'] || {}; | |
| version = distTags[versionOrTag] ? distTags[versionOrTag] : versionOrTag; | |
| } | |
| const cachedStoreKey = (await this.proxyCacheRepository.findProxyCache(fullname, fileType, version))?.filePath; | |
| if (cachedStoreKey) { | |
| const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey); | |
| const nfsString = Buffer.from(nfsBytes!).toString(); | |
| return JSON.parse(nfsString) as PackageJSONType | AbbreviatedPackageJSONType; | |
| } | |
| const manifest = await this.getRewrittenManifest(fullname, fileType, versionOrTag); | |
| this.backgroundTaskHelper.run(async () => { | |
| await this.storeRewrittenManifest(manifest, fullname, fileType); | |
| const cachedFiles = ProxyCache.create({ fullname, fileType, version }); | |
| await this.proxyCacheRepository.saveProxyCache(cachedFiles); | |
| }); | |
| return manifest; | |
| } | |
| const manifest = await this.getRewrittenManifest(fullname, fileType, versionOrTag); | |
| this.backgroundTaskHelper.run(async () => { | |
| try { | |
| this.logger.info(`Starting background task to store manifest for ${fullname}`); | |
| await this.storeRewrittenManifest(manifest, fullname, fileType); | |
| const cachedFiles = ProxyCache.create({ fullname, fileType, version }); | |
| await this.proxyCacheRepository.saveProxyCache(cachedFiles); | |
| this.logger.info(`Successfully stored manifest for ${fullname}`); | |
| } catch (error) { | |
| this.logger.error(`Failed to store manifest for ${fullname}: ${error.message}`); | |
| } | |
| }); | |
| return manifest; |
| async getPackageManifest(fullname: string, fileType: DIST_NAMES.FULL_MANIFESTS| DIST_NAMES.ABBREVIATED_MANIFESTS): Promise<AbbreviatedPackageManifestType|PackageManifestType> { | ||
| const cachedStoreKey = (await this.proxyCacheRepository.findProxyCache(fullname, fileType))?.filePath; | ||
| if (cachedStoreKey) { | ||
| const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey); | ||
| const nfsString = Buffer.from(nfsBytes!).toString(); | ||
| const nfsPkgManifgest = JSON.parse(nfsString); | ||
| return nfsPkgManifgest; | ||
| } | ||
|
|
||
| const manifest = await this.getRewrittenManifest<typeof fileType>(fullname, fileType); | ||
| this.backgroundTaskHelper.run(async () => { | ||
| await this.storeRewrittenManifest(manifest, fullname, fileType); | ||
| const cachedFiles = ProxyCache.create({ fullname, fileType }); | ||
| await this.proxyCacheRepository.saveProxyCache(cachedFiles); | ||
| }); | ||
| return manifest; | ||
| } |
There was a problem hiding this comment.
Improve error handling and logging.
Consider adding error handling for the background task and logging the start and success of the task.
const manifest = await this.getRewrittenManifest<typeof fileType>(fullname, fileType);
this.backgroundTaskHelper.run(async () => {
+ try {
+ this.logger.info(`Starting background task to store manifest for ${fullname}`);
await this.storeRewrittenManifest(manifest, fullname, fileType);
const cachedFiles = ProxyCache.create({ fullname, fileType });
await this.proxyCacheRepository.saveProxyCache(cachedFiles);
+ this.logger.info(`Successfully stored manifest for ${fullname}`);
+ } catch (error) {
+ this.logger.error(`Failed to store manifest for ${fullname}: ${error.message}`);
+ }
});
return manifest;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 getPackageManifest(fullname: string, fileType: DIST_NAMES.FULL_MANIFESTS| DIST_NAMES.ABBREVIATED_MANIFESTS): Promise<AbbreviatedPackageManifestType|PackageManifestType> { | |
| const cachedStoreKey = (await this.proxyCacheRepository.findProxyCache(fullname, fileType))?.filePath; | |
| if (cachedStoreKey) { | |
| const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey); | |
| const nfsString = Buffer.from(nfsBytes!).toString(); | |
| const nfsPkgManifgest = JSON.parse(nfsString); | |
| return nfsPkgManifgest; | |
| } | |
| const manifest = await this.getRewrittenManifest<typeof fileType>(fullname, fileType); | |
| this.backgroundTaskHelper.run(async () => { | |
| await this.storeRewrittenManifest(manifest, fullname, fileType); | |
| const cachedFiles = ProxyCache.create({ fullname, fileType }); | |
| await this.proxyCacheRepository.saveProxyCache(cachedFiles); | |
| }); | |
| return manifest; | |
| } | |
| async getPackageManifest(fullname: string, fileType: DIST_NAMES.FULL_MANIFESTS| DIST_NAMES.ABBREVIATED_MANIFESTS): Promise<AbbreviatedPackageManifestType|PackageManifestType> { | |
| const cachedStoreKey = (await this.proxyCacheRepository.findProxyCache(fullname, fileType))?.filePath; | |
| if (cachedStoreKey) { | |
| const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey); | |
| const nfsString = Buffer.from(nfsBytes!).toString(); | |
| const nfsPkgManifgest = JSON.parse(nfsString); | |
| return nfsPkgManifgest; | |
| } | |
| const manifest = await this.getRewrittenManifest<typeof fileType>(fullname, fileType); | |
| this.backgroundTaskHelper.run(async () => { | |
| try { | |
| this.logger.info(`Starting background task to store manifest for ${fullname}`); | |
| await this.storeRewrittenManifest(manifest, fullname, fileType); | |
| const cachedFiles = ProxyCache.create({ fullname, fileType }); | |
| await this.proxyCacheRepository.saveProxyCache(cachedFiles); | |
| this.logger.info(`Successfully stored manifest for ${fullname}`); | |
| } catch (error) { | |
| this.logger.error(`Failed to store manifest for ${fullname}: ${error.message}`); | |
| } | |
| }); | |
| return manifest; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/port/controller/package/DownloadPackageVersionTar.ts (4 hunks)
Additional context used
Learnings (1)
app/port/controller/package/DownloadPackageVersionTar.ts (1)
Learnt from: hezhengxu2018 PR: cnpm/cnpmcore#571 File: test/port/controller/package/DownloadPackageVersionTarController.test.ts:304-315 Timestamp: 2024-07-19T06:21:32.040Z Learning: In proxy mode, the response status should be 200 when the package version is not found.
Additional comments not posted (2)
app/port/controller/package/DownloadPackageVersionTar.ts (2)
68-87: LGTM! The changes enhance the package download handling in proxy mode.The key aspects of the changes are:
- If the package version is not found and the sync mode is proxy, it tries to get the package version from the proxy stream using the new
getTgzProxyStreammethod.- It uses the
PassThroughstream andpipelinefunction to stream the package data, improving performance and responsiveness.- The error handling logic is correct and aligns with the proxy mode requirements.
123-139: LGTM!The new
getTgzProxyStreammethod is well-structured and follows the single responsibility principle. It handles the proxy response and initiates the sync task correctly. The logging statement provides useful information for debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/port/controller/package/DownloadPackageVersionTar.ts (4 hunks)
Additional context used
Learnings (1)
app/port/controller/package/DownloadPackageVersionTar.ts (1)
Learnt from: hezhengxu2018 PR: cnpm/cnpmcore#571 File: test/port/controller/package/DownloadPackageVersionTarController.test.ts:304-315 Timestamp: 2024-07-19T06:21:32.040Z Learning: In proxy mode, the response status should be 200 when the package version is not found.
Additional comments not posted (3)
app/port/controller/package/DownloadPackageVersionTar.ts (3)
67-83: Review the error handling and proxy mode logic in thedownloadmethod.The method has a complex flow that handles different scenarios based on the sync mode. Here are a few observations and suggestions:
- Error Handling: The method catches errors from
getPackageEntityByFullnameandgetPackageVersionEntity. It's crucial to ensure that these errors are logged for better traceability.- Proxy Mode Logic: In proxy mode, if a package version is not found, the method attempts to fetch it using
getTgzProxyStream. This is a critical part of the functionality and should be thoroughly tested to ensure it handles all edge cases correctly.- Stream Handling: The use of
PassThroughstream is appropriate here as it allows for modifying the stream data if necessary before piping it to the response. However, ensure that the stream is properly managed to avoid memory leaks or hanging requests.Consider adding more detailed logging at each step of the process to help with debugging and maintaining the system. Also, ensure that unit tests cover all branches of this method, particularly the new proxy mode logic.
119-135: Review the implementation ofgetTgzProxyStreammethod.This method is key to handling proxy mode effectively. Here are some points to consider:
- Response Handling: The method sets the response status and headers based on the upstream response, which is crucial for client transparency. Ensure that all relevant headers are correctly forwarded to maintain session integrity and other necessary information.
- Background Task Creation: The method creates a synchronization task in the background. It's important to ensure that this task is created only when necessary and that it logs sufficient information for monitoring and debugging purposes.
- Stream Return: The method returns the response stream from the upstream service. Ensure that this stream is handled correctly in the calling method to avoid issues with stream corruption or leaks.
Verify that the headers manipulation and status setting are covered by unit tests to ensure they behave as expected under various scenarios. Also, check that the background task creation is robust and handles failures gracefully.
33-33: Approve the CORS settings in thedownloadForOptionsmethod.The method correctly sets the necessary CORS headers to allow cross-origin requests, which is essential for web applications that might access this API from different domains.
#366 开启代理模式时如果找不到依赖会直接返回上游仓库的manifest信息并缓存于nfs，当请求的tgz文件不存在时从上游仓库获取并返回，同时创建对应版本的同步任务。每小时检查更新已缓存的manifest文件保证上游仓库发布新版本时不会因为缓存落后而404。
Summary by CodeRabbit
New Features
Updates
SyncModeenum to include a new valueproxy.PROXY_CACHE_DIR_NAMEandABBREVIATED_META_TYPE.Tests
ProxyCacheService,ProxyCacheRepository, and related controllers.