feat: strict validate deps #720
Conversation
WalkthroughThe changes introduce significant enhancements to the package management system, focusing on dependency validation. A new method Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (5)
test/fixtures/registry.npmjs.org/invalid-deps.json (2)
40-43: Ensure timestamps are consistent with test requirements.The timestamps are set to October 2024, which is appropriate for current testing. However, consider:
- Using static/deterministic timestamps for consistent test behavior
- Adding a comment explaining why these specific timestamps were chosen
"time": { - "created": "2024-10-24T02:54:18.513Z", - "1.0.0": "2024-10-24T02:54:18.686Z", - "modified": "2024-10-24T02:54:18.896Z" + "created": "2024-01-01T00:00:00.000Z", // Static timestamp for consistent testing + "1.0.0": "2024-01-01T00:00:00.000Z", // Static timestamp for consistent testing + "modified": "2024-01-01T00:00:00.000Z" // Static timestamp for consistent testing },
46-48: Consider adding test-specific README content.The current README shows an error state. Consider adding minimal README content to:
- Document the purpose of this test fixture
- Help other developers understand how to use it in tests
- "readme": "ERROR: No README data found!", - "readmeFilename": "" + "readme": "# Invalid Dependencies Test Fixture\n\nThis package contains deliberately invalid dependencies for testing dependency validation features.", + "readmeFilename": "README.md"test/core/service/PackageManagerService/publish.test.ts (1)
116-138: Add test coverage for valid dependencies scenario.The test suite only covers the rejection case for invalid dependencies. Consider adding a test case that verifies the publish succeeds when dependencies are valid and strict validation is enabled.
Here's a suggested test case to add:
it('should publish successfully with valid deps when strict validation enabled', async () => { mock(app.config.cnpmcore, 'strictValidatePackageDeps', true); const { packageId } = await packageManagerService.publish({ dist: { localFile: TestUtil.getFixtures('registry.npmjs.org/pedding/-/pedding-1.1.0.tgz'), }, tags: [''], scope: '', name: 'pedding', description: 'pedding description', packageJson: { name: 'pedding', version: '1.1.0', dependencies: { // Add a known valid dependency here 'lodash': '^4.0.0' } }, readme: '', version: '1.1.0', isPrivate: false, }, publisher); const pkgVersion = await packageRepository.findPackageVersion(packageId, '1.1.0'); assert(pkgVersion); assert.equal(pkgVersion.version, '1.1.0'); });app/port/config.ts (1)
174-177: Enhance documentation for the new configuration option.The implementation looks good, but the documentation could be more comprehensive to help users understand the implications of enabling this option.
Consider expanding the JSDoc comment to include:
/** - * strictly enforces/validates dependencies version when publish or sync + * Strictly enforces/validates dependencies version when publishing or syncing packages. + * When enabled: + * - Publishing will be interrupted if dependencies for the current version are not found + * - Failed sync operations will queue the package for up to 3 retry attempts + * @default false */ strictValidatePackageDeps?: boolean,config/config.default.ts (1)
62-62: Add documentation for the new configuration option.The new configuration option is well-placed near other validation flags. Consider adding a code comment to document its purpose and impact on package dependency validation during sync and publish operations.
+ // When enabled, strictly validates package dependencies during sync and publish operations strictValidatePackageDeps: false,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- app/core/service/PackageManagerService.ts (4 hunks)
- app/core/service/PackageSyncerService.ts (2 hunks)
- app/port/config.ts (1 hunks)
- config/config.default.ts (1 hunks)
- test/core/service/PackageManagerService/publish.test.ts (1 hunks)
- test/core/service/PackageSyncerService/executeTask.test.ts (1 hunks)
- test/fixtures/registry.npmjs.org/invalid-deps.json (1 hunks)
🧰 Additional context used
🪛 GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
app/core/service/PackageManagerService.ts
[failure] 995-995:
A space is required after '['
[failure] 995-995:
A space is required before ']'
🪛 GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
app/core/service/PackageManagerService.ts
[failure] 995-995:
A space is required after '['
[failure] 995-995:
A space is required before ']'
🪛 GitHub Check: test-mysql57-fs-nfs (18, ubuntu-latest)
app/core/service/PackageManagerService.ts
[failure] 995-995:
A space is required after '['
[failure] 995-995:
A space is required before ']'
🔇 Additional comments (8)
test/fixtures/registry.npmjs.org/invalid-deps.json (2)
17-28: LGTM: Integrity and signature data look valid.The test fixture correctly includes:
- SHA512 integrity hash
- SHA1 shasum
- NPM registry tarball URL
- Package signatures with keyid and signature
9-10: Verify test coverage for all invalid dependency scenarios.The test fixture includes two types of invalid dependencies:
"invalid-semver": "@*(5895jk!"- Invalid semver syntax"invalid-tag": "gutu"- Invalid tag/versionConsider adding more edge cases to ensure comprehensive validation testing:
- Empty version string
- Malformed ranges (e.g., ">=1.0.0 <=")
- Non-string values
✅ Verification successful
Let me fix the syntax error in the shell script and try a different approach to verify the test coverage.
This will help us:
- Find all test fixtures that contain dependency definitions
- Identify test files related to dependency validation to understand the existing test coverage
Let me check the content of some relevant test fixtures to understand the existing test coverage for invalid dependencies.
This will help us understand:
- What other dependency validation test cases already exist
- Where the
strictValidatePackageDepsfeature is being tested- Whether we need additional test cases in
invalid-deps.json
Let me check the test files that use
strictValidatePackageDepsto understand how the feature is being tested.This will help us understand:
- How
strictValidatePackageDepsis being used in tests- What validation scenarios are already covered
- Whether our new test fixture complements existing test cases
Based on the test files and fixtures analyzed, I can now provide a final response about the test coverage for invalid dependency scenarios.
The
invalid-deps.jsontest fixture provides good coverage for invalid dependency validationThe current test fixture with two invalid dependency cases (
"invalid-semver": "@*(5895jk!"and"invalid-tag": "gutu") is sufficient because:
- The test files show that
strictValidatePackageDepsvalidation is tested in both package publishing (PackageManagerService/publish.test.ts) and package syncing (PackageSyncerService/executeTask.test.ts) scenarios- The invalid dependency patterns cover both malformed semver syntax and invalid tag/version formats
- Additional edge cases like empty strings or non-string values would be caught by the same validation logic, as they would fail semver parsing similarly
The test fixture aligns well with the existing test infrastructure and provides appropriate test cases for the
strictValidatePackageDepsfeature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are other test fixtures with invalid dependencies # to ensure we're not duplicating test cases # Search for other test fixtures with invalid dependency patterns rg -l '"dependencies":\s*{[^}]*["\''][^"\'':]+["\'\']\s*:\s*["\''][^0-9A-Za-z\^~>=<\s][^"\'\']*["\'\']' test/fixtures/Length of output: 93
Script:
#!/bin/bash # Search for files containing "dependencies" in test fixtures rg -l '"dependencies"' test/fixtures/ # Also search for any test files related to dependency validation fd -e js -e json . test/ -x grep -l "dependencies\|validateDeps\|strictValidatePackageDeps"Length of output: 3264
Script:
#!/bin/bash # Check content of test fixtures that might contain dependency validation cases echo "=== cnpmcore-test-sync-dependencies.json ===" cat test/fixtures/registry.npmjs.org/cnpmcore-test-sync-dependencies.json echo -e "\n=== accept-dependencies-module-cnpmsync.json ===" cat test/fixtures/registry.npmjs.org/accept-dependencies-module-cnpmsync.json # Also let's check if there are any test files specifically for dependency validation rg -l "strictValidatePackageDeps" test/Length of output: 9287
Script:
#!/bin/bash # Check the test files that use strictValidatePackageDeps echo "=== PackageManagerService/publish.test.ts ===" cat test/core/service/PackageManagerService/publish.test.ts | grep -A 10 -B 10 "strictValidatePackageDeps" echo -e "\n=== PackageSyncerService/executeTask.test.ts ===" cat test/core/service/PackageSyncerService/executeTask.test.ts | grep -A 10 -B 10 "strictValidatePackageDeps"Length of output: 2851
test/core/service/PackageManagerService/publish.test.ts (1)
115-115: LGTM! Well-integrated test case.The new test case maintains consistency with the existing test suite structure and properly utilizes the shared test utilities and setup/teardown hooks.
config/config.default.ts (1)
62-62: LGTM! The configuration change looks good.The new configuration option:
- Is correctly typed through the
CnpmcoreConfiginterface- Has a safe default value of
falsefor backward compatibility- Is logically placed near other validation-related settings
- Aligns with the PR objectives for enhancing dependency validation
test/core/service/PackageSyncerService/executeTask.test.ts (1)
2556-2570: LGTM on test setup.The test setup with mock data and HTTP client configuration is well structured and follows the testing patterns used throughout the file.
app/core/service/PackageSyncerService.ts (1)
365-366: 🛠️ Refactor suggestionSimplify conditional expressions for clarity
The conditionals for
skipDependenciesandsyncUpstreamcan be simplified to improve readability by removing unnecessary operators.Apply this diff to simplify the expressions:
-const skipDependencies = taskQueueInHighWaterState ? true : !!originSkipDependencies; +const skipDependencies = taskQueueInHighWaterState || !!originSkipDependencies; -const syncUpstream = !!(!taskQueueInHighWaterState && this.config.cnpmcore.sourceRegistryIsCNpm && this.config.cnpmcore.syncUpstreamFirst && registry.name === PresetRegistryName.default); +const syncUpstream = !taskQueueInHighWaterState && this.config.cnpmcore.sourceRegistryIsCNpm && this.config.cnpmcore.syncUpstreamFirst && registry.name === PresetRegistryName.default;Likely invalid or redundant comment.
app/core/service/PackageManagerService.ts (2)
106-108: Changes to 'publish' method enhance dependency validation.The addition of strict dependency validation based on the
strictValidatePackageDepsconfiguration improves the publishing process by ensuring that all dependencies are valid before publishing.
987-1006: Addition of '_checkPackageDepsVersion' method is well-implemented.The new
_checkPackageDepsVersionmethod correctly validates the versions of the package dependencies and handles exceptions appropriately.🧰 Tools
🪛 GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
[failure] 995-995:
A space is required after '['
[failure] 995-995:
A space is required before ']'🪛 GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
[failure] 995-995:
A space is required after '['
[failure] 995-995:
A space is required before ']'🪛 GitHub Check: test-mysql57-fs-nfs (18, ubuntu-latest)
[failure] 995-995:
A space is required after '['
[failure] 995-995:
A space is required before ']'
elrrrrrrr
force-pushed
the
strict-validate-deps
branch
from
ec55a48 to
8df2857
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
app/core/service/PackageManagerService.ts (2)
1001-1001: Enhance error message clarity.The current error message could be more descriptive about the validation failure.
Consider this improvement:
- throw new BadRequestError(`deps ${fullname}@${spec} not found`); + throw new BadRequestError(`Package dependency ${fullname}@${spec} was not found in the registry. Please verify the package name and version specification.`);
1003-1005: Document concurrency limit rationale.The concurrency limit of 12 for parallel dependency validation seems arbitrary. Consider adding a comment explaining the rationale for this specific value or make it configurable.
}, { - concurrency: 12, + // Limit concurrent validations to avoid overwhelming the registry + // TODO: Consider making this configurable via config + concurrency: 12, });test/core/service/PackageSyncerService/executeTask.test.ts (1)
2578-2579: Uncomment task validation assertions.These assertions appear to be intentionally commented out but are important for validating the task state after execution.
- // assert(!await TaskModel.findOne({ taskId: task.taskId })); - // assert(await HistoryTaskModel.findOne({ taskId: task.taskId })); + assert(!await TaskModel.findOne({ taskId: task.taskId })); + assert(await HistoryTaskModel.findOne({ taskId: task.taskId }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- app/core/service/PackageManagerService.ts (4 hunks)
- app/core/service/PackageSyncerService.ts (2 hunks)
- app/port/config.ts (1 hunks)
- config/config.default.ts (1 hunks)
- test/core/service/PackageManagerService/publish.test.ts (1 hunks)
- test/core/service/PackageSyncerService/executeTask.test.ts (1 hunks)
- test/fixtures/registry.npmjs.org/invalid-deps.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/port/config.ts
- config/config.default.ts
- test/core/service/PackageManagerService/publish.test.ts
- test/fixtures/registry.npmjs.org/invalid-deps.json
🔇 Additional comments (4)
app/core/service/PackageManagerService.ts (1)
106-108: LGTM! Clean integration of dependency validation.The conditional validation is well-placed and aligns with the feature's opt-in nature.
test/core/service/PackageSyncerService/executeTask.test.ts (1)
2585-2586:⚠️ Potential issueFix package name in model query.
The package name in the model query ('invalid-pkgs') doesn't match the package name used in the test ('invalid-deps').
- const model = await PackageModel.findOne({ scope: '', name: 'invalid-pkgs' }); + const model = await PackageModel.findOne({ scope: '', name: 'invalid-deps' });Likely invalid or redundant comment.
app/core/service/PackageSyncerService.ts (2)
Line range hint
363-372: Logic for Task Queue High Water State is CorrectThe implementation for handling the task queue's high water state by adjusting
skipDependenciesandsyncUpstreamis appropriate and enhances resource management during peak loads.
734-741: Settask.errorbefore LoggingIn the error handling block,
task.erroris being logged but may not have been set, which could result in loggingundefined. Ensure thattask.erroris assigned before the logging statement.Apply this diff to set
task.error:+ task.error = lastErrorMessage; if (err.name === 'BadRequestError') { // 由于当前版本的依赖不满足，尝试重试 // 默认会在当前队列最后重试 this.logger.info('[PackageSyncerService.executeTask:fail-validate-deps] taskId: %s, targetName: %s, %s', task.taskId, task.targetName, task.error); await this.taskService.retryTask(task, logs.join('\n')); return; }
| const pkgVersion = await this.packageVersionService.getVersion(npa(`${fullname}@${spec}`)); | ||
| assert(pkgVersion); | ||
| } catch (e) { | ||
| throw new BadRequestError(`deps ${fullname}@${spec} not found`); |
There was a problem hiding this comment.
嗯 是的 默认的失败逻辑会自动放到队列最后 + 最大次数限制。
主要针对场景是 monorepo 并发发包同步，发布期间查询失败。
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #720 +/- ##
==========================================
- Coverage 96.46% 96.46% -0.01%
==========================================
Files 188 188
Lines 18803 18844 +41
Branches 2463 2481 +18
==========================================
+ Hits 18139 18178 +39
- Misses 664 666 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
test/core/service/PackageSyncerService/executeTask.test.ts (1)
2556-2570: LGTM! Consider adding descriptive comments.The test suite setup is well structured with proper mocking of HTTP responses and fixtures. Consider adding JSDoc comments to describe the test suite's purpose and the invalid-deps test data structure.
Add descriptive comments like:
+ /** + * Test suite for strictValidatePackageDeps feature + * Validates package dependency validation during sync + */ describe('strictValidatePackageDeps = true', async () => { + // Setup test fixtures and mock responses for invalid-deps package + // The package contains intentionally invalid dependencies for testing beforeEach(async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- test/core/service/PackageManagerService/publish.test.ts (1 hunks)
- test/core/service/PackageSyncerService/executeTask.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/core/service/PackageManagerService/publish.test.ts
[skip ci] ## [3.65.0](v3.64.0...v3.65.0) (2024-10-26) ### Features * strict validate deps ([#720](#720)) ([12650ac](12650ac))
strictValidatePackageDepsconfiguration, disabled by default.strictValidatePackageDeps配置，默认关闭dependencies不存在，则中断发布Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores