Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: should redirect when nfs adapter support url #522

Merged
merged 3 commits into from
Jun 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/port/controller/AbstractController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ export abstract class AbstractController extends MiddlewareController {
protected async getPackageVersionEntity(pkg: PackageEntity, version: string): Promise<PackageVersionEntity> {
const packageVersion = await this.packageRepository.findPackageVersion(pkg.packageId, version);
if (!packageVersion) {
throw new NotFoundError(`${pkg.fullname}@${version} not found`);
throw this.createPackageNotFoundErrorWithRedirect(pkg.fullname, version);
}
return packageVersion;
}
fengmk2 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
8 changes: 5 additions & 3 deletions app/port/controller/package/DownloadPackageVersionTar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,17 @@ export class DownloadPackageVersionTarController extends AbstractController {
const version = this.getAndCheckVersionFromFilename(ctx, fullname, filenameWithVersion);
const storeKey = `/packages/${fullname}/${version}/${filenameWithVersion}.tgz`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

原来这里已经有跳过 db 直接访问 oss 的逻辑,计算 semver 逻辑里一样可以跳过了。

本来还单性可能 oss path 发生变化的情况。

const downloadUrl = await this.nfsAdapter.getDownloadUrl(storeKey);
// check package version in database
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个改动会导致 registry.npmmirror.com 出现大量 db 查询,我先加一个开关

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const pkg = await this.getPackageEntityByFullname(fullname);
const packageVersion = await this.getPackageVersionEntity(pkg, version);

// read by nfs url
if (downloadUrl) {
this.packageManagerService.plusPackageVersionCounter(fullname, version);
ctx.redirect(downloadUrl);
return;
}

// read from database
const pkg = await this.getPackageEntityByFullname(fullname);
const packageVersion = await this.getPackageVersionEntity(pkg, version);
ctx.logger.info('[PackageController:downloadVersionTar] %s@%s, packageVersionId: %s',
pkg.fullname, version, packageVersion.packageVersionId);
const urlOrStream = await this.packageManagerService.downloadPackageVersionTar(packageVersion);
fengmk2 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ describe('test/port/controller/package/DownloadPackageVersionTarController.test.
});

it('should 404 when package version not exists', async () => {
mock(app.config.cnpmcore, 'redirectNotFound', false);
if (process.env.CNPMCORE_NFS_TYPE === 'oss') {
mock(nfsClientAdapter, 'url', async () => {
return undefined;
Expand All @@ -176,6 +177,25 @@ describe('test/port/controller/package/DownloadPackageVersionTarController.test.
});
});

it('should redirect to source registry when package version not exists', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我补了一个单测

mock(nfsClientAdapter, 'url', async () => {
return 'http://foo.com/foo.tgz';
});

await app.httpRequest()
.get(`/${name}/-/${name}-1.0.404404.tgz`)
.expect(302)
.expect('location', `https://registry.npmjs.org/${name}/-/${name}-1.0.404404.tgz`);

// not redirect the private package
await app.httpRequest()
.get(`/${scopedName}/-/${name}-1.0.404404.tgz`)
.expect(404)
.expect({
error: `[NOT_FOUND] ${scopedName}@1.0.404404 not found`,
});
});

it('should not redirect public package to source registry when syncMode=all', async () => {
if (process.env.CNPMCORE_NFS_TYPE === 'oss') {
mock(nfsClientAdapter, 'url', async () => {
Expand Down Expand Up @@ -222,6 +242,7 @@ describe('test/port/controller/package/DownloadPackageVersionTarController.test.
.expect(302)
.expect('location', 'https://registry.npmjs.org/foo/-/foo-1.0.404404.tgz?t=123');

mock(app.config.cnpmcore, 'redirectNotFound', false);
// not redirect when package exists
await app.httpRequest()
.get(`/${name}/-/${name}-1.0.404404.tgz`)
fengmk2 marked this conversation as resolved.
Show resolved Hide resolved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这段代码是一个测试用例,主要负责测试当包版本不存在时的行为。建议在代码审查中注重以下几点:

  1. 确认该测试涵盖了所有可能情况

  2. 检查是否需要提供更多的上下文或者注释来帮助理解该测试

  3. 评估该测试的可读性和可维护性,包括变量和方法命名的清晰度以及代码格式的一致性

  4. 确保在修改代码时更新相应的测试用例,并考虑是否需要新增测试用例

在这个具体的代码片段中,测试增加了针对未发布版本的测试,还模拟了 redirectNotFound 和 url 方法。有两个测试用例被添加到测试套件中,一个测试了包版本不存在时的 404 响应,另一个则确保了当指定源注册表时,CNPM 应该将其重定向到该源。

目前代码看起来没有风险和改进的地方。

Expand Down
Loading