fix: latest range #954
fix: latest range #954
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe version resolution logic in PackageVersionService is modified to prioritize the 'latest' dist-tag when it satisfies the requested semantic version range, falling back to range-based resolution only when necessary. Corresponding test cases validate this behavior with various range specifications. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @elrrrrrrr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求修复了包版本解析逻辑中的一个问题，即在某些情况下，即使 "latest" 标签指向的版本满足请求的语义化版本范围，系统仍会返回仓库中存在的最新版本。通过引入对 "latest" 标签版本的优先判断，确保了在满足范围的前提下，始终优先返回 "latest" 标签所指定的版本，从而使版本解析行为更符合预期，尤其是在处理像 "^4" 这样的范围请求时。 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
本次 PR 修复了在解析 semver 范围时，未优先返回满足条件的 latest 标签对应版本的问题。核心逻辑修改在 PackageVersionService.ts 中，通过在范围解析时优先检查 latest 标签来解决此问题。同时，PR 在 ShowPackageVersionController.test.ts 中增加了一个非常全面的集成测试，完整地覆盖了修复的场景，确保了修复的正确性。代码改动清晰，测试用例充分。我有一个关于性能影响的建议，请查看具体评论。
| const latestVersion = await this.packageVersionRepository.findVersionByTag(scope, name, 'latest'); | ||
| if (latestVersion && semver.satisfies(latestVersion, range)) { | ||
| version = latestVersion; | ||
| } else { | ||
| version = await this.packageVersionRepository.findMaxSatisfyVersion(scope, name, paddingSemVer); | ||
| const paddingSemVer = new SqlRange(range); | ||
| if (paddingSemVer.containPreRelease) { | ||
| const versions = await this.packageVersionRepository.findSatisfyVersionsWithPrerelease( | ||
| scope, | ||
| name, | ||
| paddingSemVer, | ||
| ); | ||
| version = semver.maxSatisfying(versions, range); | ||
| } else { | ||
| version = await this.packageVersionRepository.findMaxSatisfyVersion(scope, name, paddingSemVer); | ||
| } | ||
| } |
There was a problem hiding this comment.
这个改动正确地实现了优先返回 latest 标签版本的功能。
不过，这个实现为每个版本范围（range）的解析都增加了一次数据库查询（findVersionByTag）。在某些情况下（例如，当 fetchSpec 本身也是一个标签时），这可能导致在一次版本解析的流程中有多达三次数据库查询。
考虑到版本范围解析是一个高频操作，这可能会对服务的整体性能产生影响。建议考虑对 latest 标签对应的版本号进行缓存，以减少数据库的压力。例如，可以为 findVersionByTag(scope, name, 'latest') 的结果添加一个较短时间的缓存（比如 1-5 分钟）。
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #954 +/- ##
===========================================
- Coverage 95.38% 82.21% -13.17%
===========================================
Files 197 197
Lines 22264 22271 +7
Branches 2463 1859 -604
===========================================
- Hits 21236 18311 -2925
- Misses 1028 3960 +2932 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| version = semver.maxSatisfying(versions, range); | ||
| // Prefer latest tag version if it satisfies the range | ||
| // e.g., latest=4.1.0, newest=4.2.0, range=^4 should return 4.1.0 | ||
| const latestVersion = await this.packageVersionRepository.findVersionByTag(scope, name, 'latest'); |
There was a problem hiding this comment.
类似的 latest-4, latest-1 这种，需要做优先级处理吗？
如果同时存在 latest-4 和 latest，并且它们指向的版本不一致，以那个 tag 为最高优先级判断？
There was a problem hiding this comment.
Pull request overview
This PR fixes version range resolution to prioritize the latest dist-tag over the maximum satisfying version. When a package has latest=4.1.0 and a newer version 4.2.0, requesting ^4 should return 4.1.0 (the latest tag) instead of 4.2.0 (the maximum version).
Key Changes:
- Modified
PackageVersionService.getVersion()to check if thelatesttag satisfies the requested range before falling back to maximum version resolution - Added comprehensive test coverage for the new behavior including edge cases with URL-encoded ranges and tag updates
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
app/core/service/PackageVersionService.ts |
Added logic to prefer latest tag version when it satisfies the semver range, before falling back to maximum satisfying version lookup |
test/port/controller/package/ShowPackageVersionController.test.ts |
Added comprehensive test case covering the new behavior with multiple scenarios including tag updates and different range formats |
| const latestVersion = await this.packageVersionRepository.findVersionByTag(scope, name, 'latest'); | ||
| if (latestVersion && semver.satisfies(latestVersion, range)) { | ||
| version = latestVersion; |
There was a problem hiding this comment.
The new logic to prefer the latest tag applies to ALL range queries, including the wildcard *. This will change the behavior of wildcard queries from returning the maximum satisfying version to returning the latest tag version when it satisfies the range.
This is a breaking change that will affect the existing test at test/core/service/PackageVersionService.test.ts lines 119-124, which expects getVersion(npa('mock_package@*')) to return '1.1.0' (the max version) when the latest tag points to '1.0.0'.
Consider adding special handling for the wildcard * range to preserve backward compatibility, or update all related tests to reflect this new behavior. The wildcard * is typically expected to return the highest version, not necessarily the latest tag.
| it('should prefer latest tag version when it satisfies semver range', async () => { | ||
| // Scenario: latest=4.1.0, newest=4.2.0, request ^4 should return 4.1.0 | ||
| mock(app.config.cnpmcore, 'allowPublishNonScopePackage', true); | ||
|
|
||
| // Publish 4.1.0 | ||
| const pkg410 = await TestUtil.getFullPackage({ | ||
| name: 'foo-prefer-latest', | ||
| version: '4.1.0', | ||
| versionObject: { | ||
| description: 'version 4.1.0', | ||
| }, | ||
| }); | ||
| await app | ||
| .httpRequest() | ||
| .put(`/${pkg410.name}`) | ||
| .set('authorization', publisher.authorization) | ||
| .set('user-agent', publisher.ua) | ||
| .send(pkg410) | ||
| .expect(201); | ||
|
|
||
| // Publish 4.2.0 without updating latest tag | ||
| const pkg420 = await TestUtil.getFullPackage({ | ||
| name: 'foo-prefer-latest', | ||
| version: '4.2.0', | ||
| versionObject: { | ||
| description: 'version 4.2.0', | ||
| }, | ||
| }); | ||
| // Without dist-tags, latest tag won't be updated | ||
| delete pkg420['dist-tags']; | ||
| await app | ||
| .httpRequest() | ||
| .put(`/${pkg420.name}`) | ||
| .set('authorization', publisher.authorization) | ||
| .set('user-agent', publisher.ua) | ||
| .send(pkg420) | ||
| .expect(201); | ||
|
|
||
| // Verify latest tag is still 4.1.0 | ||
| let res = await app.httpRequest().get(`/${pkg410.name}/latest`).expect(200); | ||
| assert.equal(res.body.version, '4.1.0'); | ||
|
|
||
| // Request ^4 should return latest tag version 4.1.0, not max version 4.2.0 | ||
| res = await app.httpRequest().get(`/${pkg410.name}/^4`).expect(200); | ||
| assert.equal(res.body.version, '4.1.0'); | ||
|
|
||
| res = await app.httpRequest().get(`/${pkg410.name}/%5E4`).expect(200); | ||
| assert.equal(res.body.version, '4.1.0'); | ||
|
|
||
| // Request ^4.2.0 should return 4.2.0 (latest 4.1.0 doesn't satisfy ^4.2.0) | ||
| res = await app.httpRequest().get(`/${pkg410.name}/^4.2.0`).expect(200); | ||
| assert.equal(res.body.version, '4.2.0'); | ||
|
|
||
| // Request >=4 should also return latest 4.1.0 | ||
| res = await app.httpRequest().get(`/${pkg410.name}/>=4`).expect(200); | ||
| assert.equal(res.body.version, '4.1.0'); | ||
|
|
||
| // After updating latest tag to 4.2.0, ^4 should return 4.2.0 | ||
| await app | ||
| .httpRequest() | ||
| .put(`/-/package/${pkg410.name}/dist-tags/latest`) | ||
| .set('authorization', publisher.authorization) | ||
| .set('user-agent', publisher.ua) | ||
| .set('content-type', 'application/json') | ||
| .send(JSON.stringify('4.2.0')) | ||
| .expect(200); | ||
|
|
||
| res = await app.httpRequest().get(`/${pkg410.name}/^4`).expect(200); | ||
| assert.equal(res.body.version, '4.2.0'); | ||
| }); |
There was a problem hiding this comment.
The test scenario is well-designed and covers multiple edge cases including URL-encoded version ranges. However, consider adding a test case for the wildcard * range to ensure it behaves as expected with the new logic. The wildcard is also parsed as a range and will be affected by the change to prefer the latest tag.
| // Verify latest tag is still 4.1.0 | ||
| let res = await app.httpRequest().get(`/${pkg410.name}/latest`).expect(200); |
There was a problem hiding this comment.
The test setup is incomplete. After publishing version 4.2.0, the test should verify that the package has both versions available before making assertions about which version is returned. Consider adding an assertion to confirm that both versions exist in the system after the second publish operation completes.
| // Verify latest tag is still 4.1.0 | |
| let res = await app.httpRequest().get(`/${pkg410.name}/latest`).expect(200); | |
| // Verify both versions exist after second publish | |
| let res = await app.httpRequest().get(`/${pkg410.name}/4.1.0`).expect(200); | |
| assert.equal(res.body.version, '4.1.0'); | |
| res = await app.httpRequest().get(`/${pkg410.name}/4.2.0`).expect(200); | |
| assert.equal(res.body.version, '4.2.0'); | |
| // Verify latest tag is still 4.1.0 | |
| res = await app.httpRequest().get(`/${pkg410.name}/latest`).expect(200); |
getVersionfirst check if the latest tag satisfies the version range