feat: proxy模式增强 #724
feat: proxy模式增强 #724
Conversation
WalkthroughThe Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hey @fangzhengjin, here is an example of how you can ask me to improve this pull request: @Sweep Add unit tests for the updated `getPackageManifest` method to verify error handling behavior when cache retrieval fails 📖 For more information on how to use Sweep, please read our documentation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/core/service/ProxyCacheService.ts (1)
184-194: Avoid variable shadowing of 'manifest'Within the
if (status === 404)block, the variablemanifestis redefined when destructuring fromdata, which can lead to confusion due to variable shadowing. Consider renaming the innermanifestvariable to avoid this issue.Apply this diff to rename the variable:
- const { etag, data: manifest, blockReason } = ... + const { etag, data: privateManifest, blockReason } = ...And update the return statement:
- return manifest as any; + return privateManifest as any;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/core/service/ProxyCacheService.ts(6 hunks)
🧰 Additional context used
📓 Learnings (1)
app/core/service/ProxyCacheService.ts (3)
Learnt from: hezhengxu2018
PR: cnpm/cnpmcore#571
File: app/core/service/ProxyCacheService.ts:161-209
Timestamp: 2024-10-09T03:45:47.108Z
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.
Learnt from: hezhengxu2018
PR: cnpm/cnpmcore#571
File: app/core/service/ProxyCacheService.ts:161-209
Timestamp: 2024-10-09T03:45:48.990Z
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/core/service/ProxyCacheService.ts (1)
218-225: Add timestamp validation for time mergingThe time merging logic should validate timestamp formats to prevent invalid dates.
Consider adding timestamp validation:
if (manifest.time) { const privateTimeMap = data?.time || {}; for (const key in privateTimeMap) { + const timestamp = privateTimeMap[key]; + if (!timestamp || isNaN(Date.parse(timestamp))) continue; if (!manifest.time[key]) { manifest.time[key] = privateTimeMap[key]; } } }app/port/controller/package/UpdatePackageController.ts (2)
74-74: Maintain consistency in comment languageThe comment on line 74 is in Chinese:
// 代理模式下，更新代理缓存. To maintain consistency across the codebase, consider translating comments to English.
79-79: Maintain consistency in comment languageThe comment on line 79 is in Chinese:
// 仅manifests需要更新，指定版本的package.json文件发布后不会改变. To maintain consistency across the codebase, consider translating comments to English.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/core/service/ProxyCacheService.ts(6 hunks)app/port/controller/package/UpdatePackageController.ts(3 hunks)
🧰 Additional context used
📓 Learnings (1)
app/core/service/ProxyCacheService.ts (3)
Learnt from: hezhengxu2018
PR: cnpm/cnpmcore#571
File: app/core/service/ProxyCacheService.ts:161-209
Timestamp: 2024-10-09T03:45:47.108Z
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.
Learnt from: hezhengxu2018
PR: cnpm/cnpmcore#571
File: app/core/service/ProxyCacheService.ts:161-209
Timestamp: 2024-10-09T03:45:48.990Z
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.
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 (6)
app/core/service/ProxyCacheService.ts (4)
20-20: LGTM: PackageManagerService integration
The PackageManagerService dependency is correctly imported and injected following the established pattern.
Also applies to: 55-56
66-76: LGTM: Robust error handling for cache retrieval
The error handling implementation is well-structured:
- Gracefully handles cache misses
- Properly logs errors without breaking execution
- Uses correct context in log messages
Line range hint 197-226: Optimize version map merging logic
The current implementation of merging private and public versions could be more efficient and safer.
Consider refactoring to use a more functional approach:
- const privateVersionMap = data?.versions || {};
- for (const key in privateVersionMap) {
- if (!versionMap[key]) {
- versionMap[key] = privateVersionMap[key];
- }
- }
+ manifest.versions = {
+ ...privateVersionMap,
+ ...versionMap,
+ };Also, consider adding a lock or synchronization mechanism to prevent potential race conditions during version map updates.
184-194: Consider adding manifest validation
The code handles 404 status and private package retrieval, but there's no validation of the manifest structure before processing.
Consider adding validation before processing the manifest:
const { status, data: { manifest } } = responseResult;
+if (!manifest || typeof manifest !== 'object') {
+ throw new Error('Invalid manifest structure received');
+}
// sourceRegistry not found, check private packageapp/port/controller/package/UpdatePackageController.ts (2)
20-22: Imports are properly added
The modules SyncMode, ProxyCacheRepository, isPkgManifest, and ProxyCacheService are correctly imported and used in the code.
36-39: Dependency injection setup is correct
The dependencies proxyCacheRepository and proxyCacheService are properly injected as private readonly class properties with the @Inject() decorator.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/core/service/ProxyCacheService.ts(5 hunks)app/port/controller/package/UpdatePackageController.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/core/service/ProxyCacheService.ts
fangzhengjin
force-pushed
the
feature/proxySupportPrivatePackage
branch
from
c54a9e4
to
3aea4c8
Compare
|
@fengmk2 空了麻烦看下有啥要调整的 |
| @@ -171,9 +181,24 @@ export class ProxyCacheService extends AbstractService { | |||
| } | |||
|
|
|||
| // replace tarball url | |||
| const manifest = responseResult.data; | |||
| const { status, data: manifest } = responseResult; | |||
There was a problem hiding this comment.
manifest 是 const 的话，下面会被重新赋值，应该先设置它是 let manifest = undefined，确保代码的可读性。
There was a problem hiding this comment.
manifest 是 const 的话，下面会被重新赋值，应该先设置它是
let manifest = undefined，确保代码的可读性。
两个manifest的作用域不一样，不存在重新赋值的说法吧？如果是为了可读性的话，一个改成remoteManifest一个改成privateManifest，这样可以吗？
|
需要增加对应的单元测试 |
|
需要 @hezhengxu2018 参与 CR |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
app/core/service/ProxyCacheService.ts (1)
184-194: Add error logging for 404 scenariosWhile the 404 handling logic is correct, consider adding error logging to track when packages are not found in the source registry and successfully retrieved from private registry. This will help with monitoring and debugging.
if (status === 404) { + this.logger.info('[ProxyCacheService:getRewrittenManifest] Package %s not found in source registry, checking private registry', fullname); const { etag, data: manifest, blockReason } = fileType === DIST_NAMES.FULL_MANIFESTS ? await this.packageManagerService.listPackageFullManifests(scope, name, false) : await this.packageManagerService.listPackageAbbreviatedManifests(scope, name, false); // found in private package if (etag && !blockReason) { + this.logger.info('[ProxyCacheService:getRewrittenManifest] Package %s found in private registry', fullname); return manifest as any; } + this.logger.info('[ProxyCacheService:getRewrittenManifest] Package %s not found in private registry', fullname); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/core/service/ProxyCacheService.ts(6 hunks)app/port/controller/package/UpdatePackageController.ts(3 hunks)
🧰 Additional context used
📓 Learnings (1)
app/core/service/ProxyCacheService.ts (3)
Learnt from: hezhengxu2018
PR: cnpm/cnpmcore#571
File: app/core/service/ProxyCacheService.ts:161-209
Timestamp: 2024-10-09T03:45:47.108Z
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.
Learnt from: hezhengxu2018
PR: cnpm/cnpmcore#571
File: app/core/service/ProxyCacheService.ts:161-209
Timestamp: 2024-10-09T03:45:48.990Z
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.
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 (5)
app/port/controller/package/UpdatePackageController.ts (3)
20-22: LGTM! Clean import additions for proxy functionality
The new imports are well-organized and properly scoped for the proxy mode enhancement.
36-39: LGTM! Well-structured service injections
The new proxy-related services are properly injected with readonly modifiers, following good dependency injection practices.
74-93: Verify package type check for private packages
The implementation refreshes proxy cache on package updates, but let's verify that it correctly identifies private packages.
app/core/service/ProxyCacheService.ts (2)
20-21: LGTM: Clean dependency injection for PackageManagerService
The addition of PackageManagerService as a dependency is well-structured and necessary for implementing the private package handling functionality.
Also applies to: 55-56
66-76: LGTM: Robust error handling for cache retrieval
The added try-catch block with proper error logging ensures system resilience during cache retrieval failures, allowing the system to fall back to fresh manifest retrieval.
| // 代理模式下，更新代理缓存 | ||
| if (this.config.cnpmcore.syncMode === SyncMode.proxy) { | ||
| const refreshList = await this.proxyCacheRepository.findProxyCaches(fullname); | ||
| if (refreshList.length !== 0) { | ||
| const taskList = refreshList | ||
| // 仅manifests需要更新，指定版本的package.json文件发布后不会改变 | ||
| .filter(i => isPkgManifest(i.fileType)) | ||
| .map(async item => { | ||
| const task = await this.proxyCacheService.createTask( | ||
| `${item.fullname}/${item.fileType}`, | ||
| { | ||
| fullname: item.fullname, | ||
| fileType: item.fileType, | ||
| }, | ||
| ); | ||
| return task; | ||
| }); | ||
| await Promise.all(taskList); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for proxy cache updates
While the proxy cache update logic is correct, it lacks error handling for the Promise.all operation. Failed cache updates should be logged but not block the main operation.
Consider applying this improvement:
if (this.config.cnpmcore.syncMode === SyncMode.proxy) {
const refreshList = await this.proxyCacheRepository.findProxyCaches(fullname);
if (refreshList.length !== 0) {
const taskList = refreshList
.filter(i => isPkgManifest(i.fileType))
.map(item =>
this.proxyCacheService.createTask(
`${item.fullname}/${item.fileType}`,
{
fullname: item.fullname,
fileType: item.fileType,
},
)
);
+ try {
await Promise.all(taskList);
+ } catch (error) {
+ // Log error but don't block the main operation
+ ctx.logger.error('[UpdatePackageController] Failed to update proxy cache', {
+ error,
+ fullname,
+ taskCount: taskList.length,
+ });
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
| // private manifest | ||
| if (hasPrivatePackage) { | ||
| const privateVersionMap = data?.versions || {}; | ||
| for (const key in privateVersionMap) { | ||
| if (!versionMap[key]) { | ||
| versionMap[key] = privateVersionMap[key]; | ||
| } | ||
| } | ||
| if (manifest.time) { | ||
| const privateTimeMap = data?.time || {}; | ||
| for (const key in privateTimeMap) { | ||
| if (!manifest.time[key]) { | ||
| manifest.time[key] = privateTimeMap[key]; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive checks for manifest merging
The manifest merging logic should include additional checks to handle edge cases and ensure data integrity.
-if (hasPrivatePackage) {
+if (hasPrivatePackage && data && manifest) {
+ // Ensure versions object exists
+ manifest.versions = manifest.versions || {};
+ manifest.time = manifest.time || {};
+
const privateVersionMap = data?.versions || {};
for (const key in privateVersionMap) {
- if (!versionMap[key]) {
+ if (!versionMap[key] && privateVersionMap[key]?.version === key) {
versionMap[key] = privateVersionMap[key];
}
}
- if (manifest.time) {
- const privateTimeMap = data?.time || {};
- for (const key in privateTimeMap) {
- if (!manifest.time[key]) {
- manifest.time[key] = privateTimeMap[key];
- }
+ const privateTimeMap = data?.time || {};
+ for (const key in privateTimeMap) {
+ const timeValue = privateTimeMap[key];
+ if (!manifest.time[key] && typeof timeValue === 'string' && isValidISODateString(timeValue)) {
+ manifest.time[key] = timeValue;
}
}
}Consider adding a helper function to validate ISO date strings:
function isValidISODateString(dateString: string): boolean {
if (typeof dateString !== 'string') return false;
const date = new Date(dateString);
return date instanceof Date && !isNaN(date.getTime()) && date.toISOString() === dateString;
}🛠️ Refactor suggestion
Add version format validation before merging
The version merging logic should validate the format of version strings before merging to prevent invalid versions from being added to the manifest. Consider using the semver package that's already imported.
if (hasPrivatePackage) {
const privateVersionMap = data?.versions || {};
for (const key in privateVersionMap) {
- if (!versionMap[key]) {
+ if (!versionMap[key] && semverValid(key)) {
versionMap[key] = privateVersionMap[key];
+ } else if (!semverValid(key)) {
+ this.logger.warn('[ProxyCacheService:getRewrittenManifest] Skipping invalid version %s from private package %s', key, fullname);
}
}
if (manifest.time) {
const privateTimeMap = data?.time || {};
for (const key in privateTimeMap) {
- if (!manifest.time[key]) {
+ if (!manifest.time[key] && (key === 'created' || key === 'modified' || semverValid(key))) {
manifest.time[key] = privateTimeMap[key];
+ } else if (key !== 'created' && key !== 'modified' && !semverValid(key)) {
+ this.logger.warn('[ProxyCacheService:getRewrittenManifest] Skipping invalid time entry %s from private package %s', key, fullname);
}
}
}
}Committable suggestion skipped: line range outside the PR's diff.
|
这确实是现在这个功能和verdaccio不一致。最关键的区别是verdaccio有一个自己的存储区还有一个对依赖的缓存区，但是现在cnpmcore的模式是不是缓存，而是对包的一次sync。比如自己先在仓库发布了一个自己修改版的react依赖在这个仓库中对依赖有发布权限的人就成了你，当你去同步公共npm仓库的react的包时会因为包的维护信息不一致而更新错误（应该）。即这个包在当前仓库的所有者到底公网npm仓库的团队还是本地的npm仓库，应该用哪个manifest？所以现在的代理模式还是作为一种特殊情况下的同步模式（ 另外同步时间上确实设置的过于保守了，nexus的代理缓存刷新时间是30分钟还是15分钟，现在只有每天凌晨3点去刷新缓存。但是这个好像不能通过配置项去设置，需要的话可以在启动之前改一下这个参数。 |
我看过verdaccio的数据存储结构，实际上并没有两个独立的存储区，对于相同package的不同版本（包括自定义版本）都是存储在一个目录在的，在这个package的manifest中会合并自定义版本与代理缓存版本的time / version等数据，他是先在存储的manifest中查找你要下载的版本在存储中是否存在，如果不存在则立即向上游尝试下载你请求的版本并添加到存储中，同时刷新存储中的manifest（manifest中会包含自定义版本），当然他可能会有其他地方会记录哪些版本是自定义的，但是在存储区并没有看到这个东西 所以并不存在说manifest混乱这个事情，如果有相同的package，但是有一些私有版本，verdaccio返回的始终都是合并私有包后的manifest 
|
|
不管是npm还是Maven等，如果对上游仓库的公共包进行二次修改在私服发布，不覆盖公共仓库中的版本号应该是一个共识吧？所以作者怎么变根本无所谓呀，只有你单独查询你的这个私有版本的时候会返回你自己定义的manifest，但是如果你查询的是版本列表，那么以公共仓库的manifest为基础，仅仅是合并你私有的版本信息，就更没有问题了，不是吗？ 对于404这个，如果公共仓库都找不到这个package，那么从本地存储中查找并返回，这个逻辑与开不开代理都保持了一致吧？怎么会混乱呢？毕竟如果不开代理模式，默认的策略不就是从本地存储查找并返回吗 |
|
如果verdaccio的模式是这样的话其实这个问题就成了这个功能应该更像是verdaccio还是更像是nexus。nexus的代理仓库仅作代理，想发包就去hosted repository，想同时使用两个仓库就建group repository。 |
|
@fengmk2 有什么建议？ |
其实这种部署多套还可能存在一个问题，就是如果在第一层注册表里找不到，重定向到第二层注册表的时候，走的好像是302，这种在内网环境下其实不太友好，尤其是通过内网域名提供服务都情况下，本来很多机器已经配置过第一层的映射了，那么这样还需要再次添加第二层注册表的映射，对于通过IP+端口提供服务的，还需要在走一批白名单申请 |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation