fix: fix encoded semver spec #497
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -169,7 +169,8 @@ export class PackageVersionFileController extends AbstractController { | |||
| } | |||
| if (packageVersion.version !== versionOrTag) { | |||
| ctx.set('cache-control', META_CACHE_CONTROL); | |||
| const location = ctx.url.replace(`/${fullname}/${versionOrTag}/files`, `/${fullname}/${packageVersion.version}/files`); | |||
| let location = ctx.url.replace(`/${fullname}/${versionOrTag}/files`, `/${fullname}/${packageVersion.version}/files`); | |||
| location = location.replace(`/${fullname}/${encodeURIComponent(versionOrTag)}/files`, `/${fullname}/${packageVersion.version}/files`); | |||
| throw this.createControllerRedirectError(location); | |||
| } | |||
| return packageVersion; | |||
There was a problem hiding this comment.
这段代码用于从 URL 中提取 versionOrTag，并检查是否与请求的软件包版本号匹配。若不匹配，则抛出重定向错误。
代码本身没有明显错误风险，但有改进空间。其中，第 3 行应当加上类型注解；第 6 行可以通过正则表达式更精细地匹配 URL（比如，判断是否存在多余的 /files）。此外，最好在变量定义时就对其进行赋值，避免过多的变量赋值。
| assert.equal(res.headers.location, `/${pkg.name}/1.0.0/files`); | ||
| assert.equal(res.headers['cache-control'], 'public, s-maxage=600, max-age=60'); | ||
| assert.equal(res.headers.vary, 'Origin, Accept, Accept-Encoding'); | ||
|
|
||
| res = await app.httpRequest() | ||
| .get(`/${pkg.name}/latest/files?meta&foo=bar`); | ||
| assert.equal(res.status, 302); |
There was a problem hiding this comment.
这段代码添加了对查询参数的测试，主要涉及到URL重定向的测试。整体上看起来没有明显的问题和风险，但也有一些可以改进的地方：
- assert.equal()函数可能存在误差，建议使用assert.strictEqual()函数代替。
- 测试中多次用到了相同的请求和验证头信息的代码块，可以将这部分提取到单独的函数中以减少冗余代码，提高测试代码的可复用性和可读性。
- 对于异步测试，为了确保测试能够稳定和高效地运行，建议设置一个较短的超时时间。
| assert.equal(res.status, 302); | ||
| assert.equal(res.headers.location, `/${pkg.name}/1.0.0/files/package.json`); | ||
| assert.equal(res.headers['cache-control'], 'public, s-maxage=600, max-age=60'); | ||
| assert.equal(res.headers.vary, 'Origin, Accept, Accept-Encoding'); | ||
| }); | ||
|
|
||
| it('should show one package version file meta', async () => { |
There was a problem hiding this comment.
这段代码应该是针对一个 Node.js 应用的测试用例。
在第一段代码中，执行了一个 GET 请求，并检查响应头 headers 中是否返回了预期的结果。这里没有什么明显的问题和改进建议。
在第二段代码中，也是执行了两个 GET 请求，并检查相应的 headers 值是否符合预期。
需要注意的是，在代码中使用了 assert.equal 进行断言，如果跟预期结果不符，则会抛出 AssertionError 异常。因此，可能需要增加一些逻辑来处理异常情况。
此外，可以考虑增加一些边角测试或者兼容性测试等，以覆盖更多的场景和错误情况。
| .get(`/${pkg.name}/%5E2.0.0`) | ||
| .expect(200); | ||
| assert.equal(res.body.version, '2.0.0'); | ||
|
|
||
| // new beta tag | ||
| res = await app.httpRequest() | ||
| .put(`/-/package/${pkg.name}/dist-tags/beta`) |
There was a problem hiding this comment.
这段代码补丁主要修改了 ShowPackageVersionController.test.ts 文件中的一些测试用例。以下是代码审查建议：
-
第 5 行，使用了
it.only方法，意味着只会运行被选中的测试用例。在提交更改之前，需要确保该行代码没有被意外提交。如果代码合并后，单元测试不再运行其它用例，则需要删除该代码或改为it方法。 -
第 9-11 行，添加了新的测试用例。应该添加注释来解释测试预期和阐明该功能的正确运作。例如，测试是否检查请求参数是否与包的最新版本匹配，以及该版本是否位于范围内。
-
在第 19-24 行和 26-31 行中，将多次重复的代码抽取到变量
res中，这样可以使代码更加干净可读。但请确保在使用之前将变量声明为let或const。 -
在第 35-37 行，似乎有一些冗余代码因为该代码片段是裁剪过的而引起了疑问。
Codecov Report
@@ Coverage Diff @@
## master #497 +/- ##
===========================================
- Coverage 97.45% 58.50% -38.95%
===========================================
Files 168 174 +6
Lines 15691 16184 +493
Branches 2018 409 -1609
===========================================
- Hits 15291 9468 -5823
- Misses 400 6716 +6316
|
| @@ -178,7 +178,7 @@ describe('test/port/controller/package/ShowPackageVersionController.test.ts', () | |||
| assert.equal(res.headers.vary, 'Origin, Accept, Accept-Encoding'); | |||
| }); | |||
|
|
|||
| it('should latest tag with not scoped package', async () => { | |||
| it.only('should latest tag with not scoped package', async () => { | |||
killagu
force-pushed
the
fix/file_semver_encode
branch
from
9f07904 to
5ea319d
Compare
| @@ -169,7 +169,7 @@ export class PackageVersionFileController extends AbstractController { | |||
| } | |||
| if (packageVersion.version !== versionOrTag) { | |||
| ctx.set('cache-control', META_CACHE_CONTROL); | |||
| const location = ctx.url.replace(`/${fullname}/${versionOrTag}/files`, `/${fullname}/${packageVersion.version}/files`); | |||
| const location = ctx.url.replace(new RegExp(`^/${fullname}/(${versionOrTag}|${encodeURIComponent(versionOrTag)})/files`), `/${fullname}/${packageVersion.version}/files`); | |||
| throw this.createControllerRedirectError(location); | |||
| } | |||
| return packageVersion; | |||
There was a problem hiding this comment.
这段代码的目的是在一个Version Control System API中，检查传入的版本号是否匹配给定软件包的实际版本号。下面是对代码的简要审核：
- 第一步，检查versionOrTag与fullname参数是否都是有效字符串。如果不是，则可能会导致RegExp表达式中的字符串无法正确转义并产生异常。
- 此外，需要确保输入的
versionOrTag与packageVersion.version属性值是同一类型的数据类型（例如，number或string），否则比较操作可能会失败。 - 在替换“location”变量时，切换URL路径中的
${ versionOrTag }部分以匹配正确的版本号。通过使用RegExp的构造函数创建动态正则表达式模式进行匹配来提高代码的可维护性和健壮性。 - 最后，建议在处理异常之前明确设置“cache-control”标头,并添加更多的错误处理以提高代码的容错能力和可靠性。
| assert.equal(res.headers.location, `/${pkg.name}/1.0.0/files`); | ||
| assert.equal(res.headers['cache-control'], 'public, s-maxage=600, max-age=60'); | ||
| assert.equal(res.headers.vary, 'Origin, Accept, Accept-Encoding'); | ||
|
|
||
| res = await app.httpRequest() | ||
| .get(`/${pkg.name}/latest/files?meta&foo=bar`); | ||
| assert.equal(res.status, 302); |
There was a problem hiding this comment.
这段代码用于测试PackageVersionFileController的listFiles方法。以下是我的一些评注：
- 加号符号表示在原来的代码基础上新增了一些内容，减号符号表示删除了一些内容。
- 这个补丁中加入了对
/package_name/^1.0.0/files和/package_name/%5E1.0.0/files的测试，验证其是否被重定向到/package_name/1.0.0/files页面，并检查重定向后的响应头信息。 - 对于
/${pkg.name}/latest/files?meta&foo=bar请求，也进行了重定向测试，并检查重定向后的响应头信息。
因此，这个补丁添加的测试用例主要针对URL分支和重定向行为进行测试，没有发现任何显著的错误或明显改进的建议。
| assert.equal(res.status, 302); | ||
| assert.equal(res.headers.location, `/${pkg.name}/1.0.0/files/package.json`); | ||
| assert.equal(res.headers['cache-control'], 'public, s-maxage=600, max-age=60'); | ||
| assert.equal(res.headers.vary, 'Origin, Accept, Accept-Encoding'); | ||
| }); | ||
|
|
||
| it('should show one package version file meta', async () => { |
There was a problem hiding this comment.
此代码补丁中添加了三个HTTP GET请求的测试。它们断言根据不同的URL执 行HTTP重定向是否正常。代码看起来没有错误，也没有发现改进建议。建议维护人员可以根据实际情况添加额外的测试用例以验证其他接口功能。
| @@ -169,7 +169,8 @@ export class PackageVersionFileController extends AbstractController { | |||
| } | |||
| if (packageVersion.version !== versionOrTag) { | |||
| ctx.set('cache-control', META_CACHE_CONTROL); | |||
| const location = ctx.url.replace(`/${fullname}/${versionOrTag}/files`, `/${fullname}/${packageVersion.version}/files`); | |||
| let location = ctx.url.replace(`/${fullname}/${versionOrTag}/files`, `/${fullname}/${packageVersion.version}/files`); | |||
| location = location.replace(`/${fullname}/${encodeURIComponent(versionOrTag)}/files`, `/${fullname}/${packageVersion.version}/files`); | |||
| throw this.createControllerRedirectError(location); | |||
| } | |||
| return packageVersion; | |||
There was a problem hiding this comment.
这段代码的目的是用于包版本控制器的重定向检查。它在请求处理期间校验传递的参数是否匹配当前的包版本，如果不匹配，则会抛出重定向错误。以下是一些针对此代码补丁的建议：
1.第169行代码"export class PackageVersionFileController extends AbstractController {"处应当留下该文件所属的模块的名称和相关说明。
2.第171行：建议给新的location变量添加关键字const修饰符, 避免意外被修改。
-
当替换URL中的versionOrTag值含有特殊字符时（如'#', ':'等），使用encodeURIComponent()函数来编码URL路径，确保重定向URL正确可用。
-
我注意到该段代码只把cache-control头信息设置在异常/ redirect响应之前，考虑在所有响应中都设置cache-control头信息。
-
建议添加一些try-catch块，捕获可能由URL路径验证问题引起的错误。
经过以上更新后的代码如下：
/**
- 用于检查传入的versionOrTag值是否与当前包版本匹配
- @param {*} fullname 包名全称
- @param {*} versionOrTag 版本号或标签
- @param {*} ctx 请求上下文
/
export class PackageVersionFileController extends AbstractController {
/*- 检查指定的版本是否与当前的包版本匹配
- 如果不匹配，则使用新的URL进行重定向。
- @param {*} fullname
- @param {*} versionOrTag
- @param {*} ctx
*/
checkPackageVersion(fullname, versionOrTag, ctx) {
let packageVersion = this.packageService.getPackageVersion(fullname, versionOrTag);
if (!packageVersion) {
const notFoundError = new NotFoundError(Package version ${versionOrTag} not found: ${fullname});
notFoundError.headers = { 'cache-control': META_CACHE_CONTROL };
throw notFoundError;
} else {
// 确保所有响应都包含Cache-Control头信息
ctx.set('cache-control', META_CACHE_CONTROL);
try {
if (packageVersion.version !== versionOrTag) {
let location = ctx.url.replace(
/${fullname}/${encodeURIComponent(versionOrTag)}/files, // 当涉及特殊字符时，使用encodeURIComponent()函数编码路径
/${fullname}/${packageVersion.version}/files
);
throw this.createControllerRedirectError(location);
}
} catch (err) {
const serverError = new InternalServerError(Server error: ${err.message});
serverError.originalError = err;
throw serverError;
}
}
return packageVersion;
}
}
| .get(`/${pkg.name}/%5E2.0.0`) | ||
| .expect(200); | ||
| assert.equal(res.body.version, '2.0.0'); | ||
|
|
||
| // new beta tag | ||
| res = await app.httpRequest() | ||
| .put(`/-/package/${pkg.name}/dist-tags/beta`) |
There was a problem hiding this comment.
代码看起来很简洁，是针对 ShowPackageVersionController.test.ts 的测试用例。需要注意以下两点：
- 第 8-16 行是新增代码，在测试获取指定版本号的接口（通过
/package_name/semver_version或/%5Esemver_version访问）。 - 涉及到路由的接口测试时，需要确认被测试的路由已在主应用中注册。
可能有改进空间的建议：
- 最好使用常量或配置文件来定义被频繁使用的值，例如
/^2.0.0和%5E2.0.0，方便修改。 - 可以使用数组和循环来避免重复编写类似逻辑的代码段，提高代码的可维护性。
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
[skip ci] ## [3.29.1](v3.29.0...v3.29.1) (2023-06-07) ### Bug Fixes * fix encoded semver spec ([#497](#497)) ([aa39ced](aa39ced))
No description provided.