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: create sync task by 'GET /:fullname/-/:filenameWithVersion.tgz' #526

Merged
merged 6 commits into from
Jun 21, 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
12 changes: 6 additions & 6 deletions app/port/controller/AbstractController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,25 +146,25 @@ export abstract class AbstractController extends MiddlewareController {
return new UnavailableForLegalReasonsError(`${message}, reason: ${reason}`);
}

protected async getPackageEntityByFullname(fullname: string): Promise<PackageEntity> {
protected async getPackageEntityByFullname(fullname: string, allowSync?: boolean): Promise<PackageEntity> {
const [ scope, name ] = getScopeAndName(fullname);
return await this.getPackageEntity(scope, name);
return await this.getPackageEntity(scope, name, allowSync);
}

// try to get package entity, throw NotFoundError when package not exists
protected async getPackageEntity(scope: string, name: string): Promise<PackageEntity> {
protected async getPackageEntity(scope: string, name: string, allowSync?:boolean): Promise<PackageEntity> {
const packageEntity = await this.packageRepository.findPackage(scope, name);
if (!packageEntity) {
const fullname = getFullname(scope, name);
throw this.createPackageNotFoundErrorWithRedirect(fullname);
throw this.createPackageNotFoundErrorWithRedirect(fullname, undefined, allowSync);
}
return packageEntity;
}

protected async getPackageVersionEntity(pkg: PackageEntity, version: string): Promise<PackageVersionEntity> {
protected async getPackageVersionEntity(pkg: PackageEntity, version: string, allowSync?: boolean): Promise<PackageVersionEntity> {
const packageVersion = await this.packageRepository.findPackageVersion(pkg.packageId, version);
if (!packageVersion) {
throw this.createPackageNotFoundErrorWithRedirect(pkg.fullname, version);
throw this.createPackageNotFoundErrorWithRedirect(pkg.fullname, version, allowSync);
}
return packageVersion;
}

Choose a reason for hiding this comment

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

这段代码通过增加一个新的可选参数 allowSync 来改进三个方法:getPackageEntityByFullname, getPackageEntitygetPackageVersionEntity. 这个参数允许控制数据库中检索实体时,是否可以跨过异步执行任务(例如基于 Promise 的查询)来同步获取数据. 在代码变更的详细实现方式看不到前面的某些代码。因此无法评价可能存在的任何其他风险或限制。

Expand Down
5 changes: 3 additions & 2 deletions app/port/controller/package/DownloadPackageVersionTar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ export class DownloadPackageVersionTarController extends AbstractController {
}

// check package version in database
const pkg = await this.getPackageEntityByFullname(fullname);
const packageVersion = await this.getPackageVersionEntity(pkg, version);
const allowSync = this.getAllowSync(ctx);
const pkg = await this.getPackageEntityByFullname(fullname, allowSync);
const packageVersion = await this.getPackageVersionEntity(pkg, version, allowSync);

// read by nfs url
if (downloadUrl) {

Choose a reason for hiding this comment

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

这段代码可以进行如下改进:

1.注释修改:代码中的一些注释需要更改。特别是“// try nfs url first, avoid db query”和“// read by nfs url”的注释可能与实际情况不符。

2.异常处理:代码中没有异常处理机制,建议添加适当的异常处理机制。

3.代码可读性提高:代码中存在一些重复的方法调用,建议使用变量来存储结果。还建议在命名参数时使用有意义的名称。

4.潜在 bug 风险:下载 URL 可能会返回一个空字符串,这可能导致问题。建议添加一个检查以防止返回空字符串时出现问题。

Choose a reason for hiding this comment

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

这段代码需要下载软件包。以下是我对该代码的审查:

1.15行的'getPackageEntityByFullname'和28行的'getPackageVersionEntity'在哪里定义?是否存在将被调用的类型定义文件?如果没有,建议添加类型定义。

2.引入allowSync变量的设计和使用方式不清楚。建议提供有关其作用和目的的注释。

3.建议添加错误处理和恢复机制,以保证稳健性。

4.建议考虑支持其他协议(例如HTTP)中的URL下载。

5.42行建议更改为异步调用以优化性能

在上述东西得到满足之前不能确认缺陷和修复建议。

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,31 @@ describe('test/port/controller/package/DownloadPackageVersionTarController.test.
error: '[NOT_FOUND] @cnpm/testmodule-download-version-tar@1.0.404404 not found',
});
});

it('should not create sync task when package version tgz not exists and syncNotFound=false', async () => {
mock(app.config.cnpmcore, 'syncMode', 'exist');
mock(app.config.cnpmcore, 'syncNotFound', false);
mock(app.config.cnpmcore, 'redirectNotFound', false);
const res = await app.httpRequest()
.get('/lodash/-/lodash-1.404.404.tgz')
.set('user-agent', publisher.ua + ' node/16.0.0')
.set('Accept', 'application/vnd.npm.install-v1+json');
assert(res.status === 404);
app.notExpectLog('[middleware:ErrorHandler][syncPackage] create sync package');
});

it('should create sync task when package version tgz not exists and syncNotFound=true', async () => {
mock(app.config.cnpmcore, 'syncMode', 'exist');
mock(app.config.cnpmcore, 'syncNotFound', true);
mock(app.config.cnpmcore, 'redirectNotFound', false);
const res = await app.httpRequest()
.get('/lodash/-/lodash-1.404.404.tgz')
.set('user-agent', publisher.ua + ' node/16.0.0')
.set('Accept', 'application/vnd.npm.install-v1+json');
assert(res.status === 404);
app.expectLog('[middleware:ErrorHandler][syncPackage] create sync package');
});

});

describe('[GET /:fullname/download/:fullname-:version.tgz] deprecatedDownload()', () => {
Expand Down