feat: compatible verdaccio path style #723
Conversation
WalkthroughThe changes replace the implementation of the Changes
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 new `downloadVerdaccioPathStyle` endpoint that verify: 📖 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: 0
🧹 Outside diff range and nitpick comments (1)
app/port/controller/package/DownloadPackageVersionTar.ts (1)
137-155: Consider alternative approaches for path compatibility.While the current implementation works, consider these architectural improvements:
- Instead of duplicating route handlers, consider implementing path normalization middleware that converts Verdaccio-style paths to your standard format before reaching the controller.
- This would reduce code duplication and make it easier to maintain path compatibility in the future.
Example middleware approach:
// middleware/normalizePackagePath.ts export async function normalizePackagePath(ctx: EggContext, next: () => Promise<void>) { const match = ctx.path.match(/\/-\/@(.+?)\//); if (match) { ctx.path = ctx.path.replace(`/-/@${match[1]}/`, '/-/'); } await next(); }Would you like me to help create a detailed proposal for this alternative approach?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/port/controller/package/DownloadPackageVersionTar.ts(1 hunks)
🔇 Additional comments (3)
app/port/controller/package/DownloadPackageVersionTar.ts (3)
136-138: LGTM! Good section separation.
The added comment and spacing improve code organization and readability.
148-155:
Add validation and documentation for Verdaccio path style.
Similar issues as the OPTIONS handler, plus missing documentation about the Verdaccio path style compatibility.
Apply similar changes to this method:
@HTTPMethod({
- // GET /:fullname/-/:scope/:filenameWithVersion.tgz
+ // GET /:fullname/-/@:scope/:filenameWithVersion.tgz
- path: `/:fullname(${FULLNAME_REG_STRING})/-/:scope/:filenameWithVersion.tgz`,
+ path: `/:fullname(${FULLNAME_REG_STRING})/-/@:scope/:filenameWithVersion.tgz`,
method: HTTPMethodEnum.GET,
})
- async downloadVerdaccioPathStyle(@Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() filenameWithVersion: string) {
+ async downloadVerdaccioPathStyle(@Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() scope: string, @HTTPParam() filenameWithVersion: string) {
+ const [ pkgScope ] = getScopeAndName(fullname);
+ if (pkgScope !== `@${scope}`) {
+ throw new NotFoundError(`Scope mismatch: ${pkgScope} !== @${scope}`);
+ }
return this.download(ctx, fullname, filenameWithVersion);
}Add JSDoc comments to document the Verdaccio path style compatibility:
+ /**
+ * Handle package downloads using Verdaccio-style paths
+ * Example: /@scope/pkg/-/@scope/pkg-1.0.0.tgz
+ * This provides compatibility for projects migrating from Verdaccio
+ * without requiring changes to their lock files.
+ */
@HTTPMethod({Let's verify the path style compatibility with Verdaccio:
#!/bin/bash
# Search for examples of Verdaccio path patterns in the codebase
rg -l "verdaccio" . | while read -r file; do
echo "=== $file ==="
rg -A 5 "/-/@.*\.tgz" "$file" || true
done139-146:
Method naming and path handling need improvements.
Several issues to address:
- The method name
downloadVerdaccioPathStyleorOptionshas inconsistent casing. Consider renaming todownloadVerdaccioPathStyleOrOptions. - The
:scopeparameter in the path pattern is captured but unused. Consider validating it matches the scope infullname. - The path pattern might conflict with the original download path.
Consider applying these changes:
@HTTPMethod({
- // GET /:fullname/-/:scope/:filenameWithVersion.tgz
+ // GET /:fullname/-/@:scope/:filenameWithVersion.tgz
- path: `/:fullname(${FULLNAME_REG_STRING})/-/:scope/:filenameWithVersion.tgz`,
+ path: `/:fullname(${FULLNAME_REG_STRING})/-/@:scope/:filenameWithVersion.tgz`,
method: HTTPMethodEnum.OPTIONS,
})
- async downloadVerdaccioPathStyleorOptions(@Context() ctx: EggContext) {
+ async downloadVerdaccioPathStyleOrOptions(@Context() ctx: EggContext, @HTTPParam() scope: string) {
+ const [ pkgScope ] = getScopeAndName(ctx.params.fullname);
+ if (pkgScope !== `@${scope}`) {
+ throw new NotFoundError(`Scope mismatch: ${pkgScope} !== @${scope}`);
+ }
return this.downloadForOptions(ctx);
}Let's verify if there are any potential path conflicts:
fengmk2
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
app/port/controller/package/DownloadPackageVersionTar.ts (2)
77-90: Consider documenting the OSS-specific behavior.The test for non-async URL function is skipped for OSS storage type. Consider adding a comment explaining why this test is not applicable for OSS to improve maintainability.
if (process.env.CNPMCORE_NFS_TYPE !== 'oss') { + // OSS SDK only supports async URL generation it('should download a version tar redirect to mock cdn success with url function is not async function', async () => {
374-377: Remove commented debug statements.Remove the commented
console.logstatement to keep the code clean.mock(nfsClientAdapter, 'url', async (storeKey: string) => { - // console.log('call url: ', storeKey); return `https://cdn.mock.com${storeKey}`; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/port/controller/package/DownloadPackageVersionTar.ts(1 hunks)
🧰 Additional context used
🪛 Biome
app/port/controller/package/DownloadPackageVersionTar.ts
[error] 19-41: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (1)
app/port/controller/package/DownloadPackageVersionTar.ts (1)
1-419: Test implementation looks good!
The test suite is comprehensive and covers:
- Different download paths (main, deprecated, and scoped)
- CDN redirects and streaming downloads
- CORS handling
- Error cases and edge scenarios
- Sync behavior for different modes
This addresses the previous review comment requesting unit tests.
🧰 Tools
🪛 Biome
[error] 19-41: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
fangzhengjin
force-pushed
the
feature/CompatibleVerdaccioPathStyle
branch
from
f58615c to
88b4f78
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/port/controller/package/DownloadPackageVersionTarController.test.ts (1)
375-376: Remove commented console.log statement.Commented debug statements should be removed to maintain clean test code.
- // console.log('call url: ', storeKey);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/port/controller/package/DownloadPackageVersionTarController.test.ts(2 hunks)
🔇 Additional comments (3)
test/port/controller/package/DownloadPackageVersionTarController.test.ts (3)
16-18: LGTM! Clear and reusable variable structure.
The refactoring of package name variables improves code clarity and better supports the Verdaccio path style compatibility objective.
372-418: Verify test coverage for Verdaccio path style compatibility.
While the basic success and error cases are covered, we should verify if additional edge cases need testing:
- Invalid scope format
- Mismatched scope between URL and package name
- Non-scoped packages using this route
398-400:
Fix incorrect response object usage in assertions.
The assertions are using the response object from the first request instead of the second request's response.
- assert(res.status === 200);
- assert(res.headers['content-type'] === 'application/octet-stream');
- assert(res.headers['content-disposition'] === `attachment; filename="${name}-1.0.0.tgz"`);
+ const scopedRes = await app.httpRequest()
+ .get(`/${scopedName}/-/${scope}/${scopedName}-1.0.0.tgz`);
+ assert(scopedRes.status === 200);
+ assert(scopedRes.headers['content-type'] === 'application/octet-stream');
+ assert(scopedRes.headers['content-disposition'] === `attachment; filename="${name}-1.0.0.tgz"`);Likely invalid or redundant comment.
test/port/controller/package/DownloadPackageVersionTarController.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/port/controller/package/DownloadPackageVersionTarController.test.ts (1)
375-376: Remove debug comment.The commented console.log statement should be removed as it's not providing value.
- // console.log('call url: ', storeKey);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/port/controller/package/DownloadPackageVersionTarController.test.ts(2 hunks)
🔇 Additional comments (2)
test/port/controller/package/DownloadPackageVersionTarController.test.ts (2)
16-18: LGTM! Good variable organization.
The separation of scope and name variables improves code clarity and maintainability while supporting the new Verdaccio path style tests.
379-384:
Simplify URL pattern to match Verdaccio style.
The current URL pattern includes the scope twice (in scopedName and in the path), which doesn't match Verdaccio's path style.
Apply this fix:
- .get(`/${name}/-/${scope}/${name}-1.0.0.tgz`);
+ .get(`/${name}/-/${name}-1.0.0.tgz`);
- .get(`/${scopedName}/-/${scope}/${name}-1.0.0.tgz`);
+ .get(`/${scopedName}/-/${name}-1.0.0.tgz`);
test/port/controller/package/DownloadPackageVersionTarController.test.ts
Show resolved
Hide resolved
|
@fengmk2 已添加 |
fengmk2
changed the title
[skip ci] ## [3.66.0](v3.65.0...v3.66.0) (2024-11-03) ### Features * compatible verdaccio path style ([#723](#723)) ([7158e66](7158e66))
兼容Verdaccio下载地址风格，镜像库从Verdaccio切换至cnpmcore后无需大面积调整lock文件
Summary by CodeRabbit
New Features
Bug Fixes