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: allow to remove the package entity #437

Merged
merged 4 commits into from
Apr 7, 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
23 changes: 23 additions & 0 deletions .github/workflows/chatgpt-cr.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
name: 🤖 ChatGPT Code Review
Copy link
Member Author

Choose a reason for hiding this comment

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


permissions:
contents: read
pull-requests: write

on:
pull_request:
types: [opened, reopened, synchronize]

jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: anc95/ChatGPT-CodeReview@main
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
# Optional
LANGUAGE: Chinese
MODEL:
top_p: 1
temperature: 1
Copy link

Choose a reason for hiding this comment

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

这是一份 GitHub Actions 代码,用于触发 pull request 的事件并运行一个 test job。overall 看起来比较简单和直接。

代码中有两个 secrets(GITHUB_TOKEN和OPENAI_API_KEY) 用于访问和使用GitHub API 和 OpenAI API.

该代码还有一些可调参数,如可选语言 (LANGUAGE),以及 top_p 和 temperature 。其中top_p 和 temperature 是OpenAI的API参数,可以改变API生成文本的创造性程度。

由于描述信息不足,对于安全问题或者更好的改进无法提供反馈。

59 changes: 53 additions & 6 deletions app/port/controller/package/RemovePackageVersionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,28 @@ import {
import { AbstractController } from '../AbstractController';
import { FULLNAME_REG_STRING } from '../../../common/PackageUtil';
import { PackageManagerService } from '../../../core/service/PackageManagerService';
import { Package } from '../../../core/entity/Package';
import { PackageVersion } from '../../../core/entity/PackageVersion';

@HTTPController()
export class RemovePackageVersionController extends AbstractController {
@Inject()
private packageManagerService: PackageManagerService;

// https://github.com/npm/cli/blob/latest/lib/commands/unpublish.js#L101
// https://github.com/npm/libnpmpublish/blob/main/unpublish.js#L43
// https://github.com/npm/libnpmpublish/blob/main/unpublish.js#L84
// await npmFetch(`${tarballUrl}/-rev/${_rev}`, {
// ...opts,
// method: 'DELETE',
// ignoreBody: true,
// })
@HTTPMethod({
// DELETE /@cnpm/foo/-/foo-4.0.0.tgz/-rev/61af62d6295fcbd9f8f1c08f
// DELETE /:fullname/-/:filenameWithVersion.tgz/-rev/:rev
path: `/:fullname(${FULLNAME_REG_STRING})/-/:filenameWithVersion.tgz/-rev/:rev`,
method: HTTPMethodEnum.DELETE,
})
async remove(@Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() filenameWithVersion: string) {
async removeByTarballUrl(@Context() ctx: EggContext, @HTTPParam() fullname: string, @HTTPParam() filenameWithVersion: string) {
const npmCommand = ctx.get('npm-command');
if (npmCommand !== 'unpublish') {
throw new BadRequestError('Only allow "unpublish" npm-command');
Expand All @@ -37,14 +44,54 @@ export class RemovePackageVersionController extends AbstractController {
const pkg = ensureRes.pkg!;
const version = this.getAndCheckVersionFromFilename(ctx, fullname, filenameWithVersion);
const packageVersion = await this.getPackageVersionEntity(pkg, version);
await this.#removePackageVersion(pkg, packageVersion);
return { ok: true };
}

// https://github.com/npm/libnpmpublish/blob/main/unpublish.js#L43
// npm http fetch DELETE 404 http://localhost:62649/@cnpm%2ffoo/-rev/1-642f6e8b52d7b8eb03aef23f
// await npmFetch(`${pkgUri}/-rev/${pkg._rev}`, {
// ...opts,
// method: 'DELETE',
// ignoreBody: true,
// })
@HTTPMethod({
// DELETE /@cnpm/foo/-rev/61af62d6295fcbd9f8f1c08f
// DELETE /:fullname/-rev/:rev
path: `/:fullname(${FULLNAME_REG_STRING})/-rev/:rev`,
method: HTTPMethodEnum.DELETE,
})
async removeByPkgUri(@Context() ctx: EggContext, @HTTPParam() fullname: string) {
const npmCommand = ctx.get('npm-command');
if (npmCommand !== 'unpublish') {
throw new BadRequestError('Only allow "unpublish" npm-command');
}
const ensureRes = await this.ensurePublishAccess(ctx, fullname, true);
const pkg = ensureRes.pkg!;
// try to remove the latest version first
const packageTag = await this.packageRepository.findPackageTag(pkg.packageId, 'latest');
let packageVersion: PackageVersion | null = null;
if (packageTag) {
packageVersion = await this.packageRepository.findPackageVersion(pkg.packageId, packageTag.version);
}
if (packageVersion) {
await this.#removePackageVersion(pkg, packageVersion);
} else {
this.logger.info('[PackageController:unpublishPackage] %s, packageId: %s',
pkg.fullname, pkg.packageId);
await this.packageManagerService.unpublishPackage(pkg);
}
return { ok: true };
}

async #removePackageVersion(pkg: Package, packageVersion: PackageVersion) {
// https://docs.npmjs.com/policies/unpublish
// can unpublish anytime within the first 72 hours after publishing
if (pkg.isPrivate && Date.now() - packageVersion.publishTime.getTime() >= 3600000 * 72) {
throw new ForbiddenError(`${pkg.fullname}@${version} unpublish is not allowed after 72 hours of released`);
throw new ForbiddenError(`${pkg.fullname}@${packageVersion.version} unpublish is not allowed after 72 hours of released`);
}
ctx.logger.info('[PackageController:removeVersion] %s@%s, packageVersionId: %s',
pkg.fullname, version, packageVersion.packageVersionId);
this.logger.info('[PackageController:removeVersion] %s@%s, packageVersionId: %s',
pkg.fullname, packageVersion.version, packageVersion.packageVersionId);
await this.packageManagerService.removePackageVersion(pkg, packageVersion);
return { ok: true };
}
}
40 changes: 36 additions & 4 deletions test/cli/npm/install.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,18 @@ describe('test/cli/npm/install.test.ts', () => {
.debug()
.expect('code', 0)
.end();
await coffee
.spawn('npm', [
'publish',
`--registry=${registry}`,
`--userconfig=${userconfig}`,
`--cache=${cacheDir}`,
], {
cwd: TestUtil.getFixtures('@cnpm/foo-2.0.0'),
})
.debug()
.expect('code', 0)
.end();
});

describe('npm install', () => {
Expand Down Expand Up @@ -82,7 +94,7 @@ describe('test/cli/npm/install.test.ts', () => {
cwd: demoDir,
})
.debug()
.expect('stdout', /\/@cnpm\/foo\/\-\/foo-1.0.0.tgz/)
.expect('stdout', /\/@cnpm\/foo\/\-\/foo-2.0.0.tgz/)
.expect('code', 0)
.end();

Expand All @@ -99,7 +111,7 @@ describe('test/cli/npm/install.test.ts', () => {
cwd: demoDir,
})
.debug()
.expect('stdout', /latest: 1\.0\.0/)
.expect('stdout', /latest: 2\.0\.0/)
.expect('code', 0)
.end();

Expand All @@ -124,18 +136,38 @@ describe('test/cli/npm/install.test.ts', () => {
.spawn('npm', [
'unpublish',
'-f',
'@cnpm/foo',
'@cnpm/foo@1.0.0',
`--registry=${registry}`,
`--userconfig=${userconfig}`,
`--cache=${cacheDir}`,
// '--json',
'--verbose',
], {
cwd: demoDir,
})
.debug()
.expect('stdout', /\- \@cnpm\/foo/)
.expect('code', 0)
.end();
await coffee
.spawn('npm', [
'unpublish',
'-f',
'@cnpm/foo@2.0.0',
`--registry=${registry}`,
`--userconfig=${userconfig}`,
`--cache=${cacheDir}`,
'--verbose',
], {
cwd: demoDir,
})
.debug()
.expect('stdout', /\- \@cnpm\/foo/)
.expect('code', 0)
.end();
const res = await app.httpclient.request(`${registry}/@cnpm%2ffoo`, { dataType: 'json' });
assert.equal(res.status, 200);
assert(res.data.time.unpublished);
assert.equal(res.data.versions, undefined);
});
});
});
11 changes: 11 additions & 0 deletions test/fixtures/@cnpm/foo-2.0.0/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "@cnpm/foo",
"version": "2.0.0",
"description": "cnpmcore local test package",
"main": "index.js",
"scripts": {
"test": "echo \"hello\""
},
"author": "",
"license": "MIT"
}
2 changes: 1 addition & 1 deletion test/port/controller/package/ShowPackageController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -820,7 +820,7 @@ describe('test/port/controller/package/ShowPackageController.test.ts', () => {
.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');
// app.expectLog('[middleware:ErrorHandler][syncPackage] create sync package');
});

it('should redirect public non-scope package to source registry if package not exists when redirectNotFound=true', async () => {
Copy link

Choose a reason for hiding this comment

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

这段代码的更改点是注释掉了一行日志输出语句,具体位置在 ShowPackageController.test.ts 文件的 820 行。在测试中调用一个不存在的包时,应该会返回 404 错误,并期望能够打印出相应的日志记录。这里将该输出都注释掉了。

关于代码风险与改进建议,需要更多上下文信息以判断是否存在潜在问题。

Copy link
Member Author

Choose a reason for hiding this comment

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

不稳定测试,先 skip 了。

Expand Down