Skip to content

Commit

Permalink
fix: ignore npm registry 404 status response on sync process (#740)
Browse files Browse the repository at this point in the history 
closes #739

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Enhanced error handling for package synchronization, including
specific logging for package not found scenarios.
	- Simplified criteria for identifying removed packages.

- **Bug Fixes**
	- Corrected documentation for the `syncMode` property.

- **Chores**
	- Updated dependency versions in `package.json`.

- **Tests**
- Added new test cases and refined existing assertions to improve
logging and error handling verification.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
@fengmk2
fengmk2 authored Dec 9, 2024
1 parent 167e37c commit 57226c5
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 12 deletions.
40 changes: 35 additions & 5 deletions app/core/service/PackageSyncerService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ export class PackageSyncerService extends AbstractService {
const { status, data } = remoteFetchResult;

// deleted or blocked
if (status === 404 || status === 451) {
if (status === 451) {
return true;
}

Expand All @@ -245,6 +245,23 @@ export class PackageSyncerService extends AbstractService {

// security holder
// test/fixtures/registry.npmjs.org/security-holding-package.json
// {
// "_id": "xxx",
// "_rev": "9-a740a77bcd978abeec47d2d027bf688c",
// "name": "xxx",
// "time": {
// "modified": "2017-11-28T00:45:24.162Z",
// "created": "2013-09-20T23:25:18.122Z",
// "0.0.0": "2013-09-20T23:25:20.242Z",
// "1.0.0": "2016-06-22T00:07:41.958Z",
// "0.0.1-security": "2016-12-15T01:03:58.663Z",
// "unpublished": {
// "time": "2017-11-28T00:45:24.163Z",
// "versions": []
// }
// },
// "_attachments": {}
// }
let isSecurityHolder = true;
for (const versionInfo of Object.entries<{ _npmUser?: { name: string } }>(data.versions || {})) {
const [ v, info ] = versionInfo;
Expand Down Expand Up @@ -455,6 +472,19 @@ export class PackageSyncerService extends AbstractService {
return;
}

if (status === 404) {
// ignore 404 status
// https://github.com/node-modules/detect-port/issues/57
task.error = `Package not found, status 404, data: ${JSON.stringify(data)}`;
logs.push(`[${isoNow()}] ❌ ${task.error}, log: ${logUrl}`);
logs.push(`[${isoNow()}] ❌ Synced ${fullname} fail, ${task.error}, log: ${logUrl}`);
logs.push(`[${isoNow()}] ❌❌❌❌❌ ${fullname} ❌❌❌❌❌`);
this.logger.info('[PackageSyncerService.executeTask:fail-request-error] taskId: %s, targetName: %s, %s',
task.taskId, task.targetName, task.error);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
return;
}

let readme = data.readme || '';
if (typeof readme !== 'string') {
readme = JSON.stringify(readme);
Expand Down Expand Up @@ -545,9 +575,9 @@ export class PackageSyncerService extends AbstractService {
task.error = `invalid maintainers: ${JSON.stringify(maintainers)}`;
logs.push(`[${isoNow()}] ❌ ${task.error}, log: ${logUrl}`);
logs.push(`[${isoNow()}] ${failEnd}`);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
this.logger.info('[PackageSyncerService.executeTask:fail-invalid-maintainers] taskId: %s, targetName: %s, %s',
task.taskId, task.targetName, task.error);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
return;
}

Expand Down Expand Up @@ -575,9 +605,9 @@ export class PackageSyncerService extends AbstractService {
task.error = 'There is no available specific versions, stop task.';
logs.push(`[${isoNow()}] ${task.error}, log: ${logUrl}`);
logs.push(`[${isoNow()}] ❌❌❌❌❌ ${fullname} ❌❌❌❌❌`);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
this.logger.info('[PackageSyncerService.executeTask:fail-empty-list] taskId: %s, targetName: %s, %s',
task.taskId, task.targetName, task.error);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
return;
}
if (specificVersions) {
Expand Down Expand Up @@ -764,9 +794,9 @@ export class PackageSyncerService extends AbstractService {
logs.push(`[${isoNow()}] ❌ All versions sync fail, package not exists, log: ${logUrl}`);
logs.push(`[${isoNow()}] ${failEnd}`);
task.error = lastErrorMessage;
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
this.logger.info('[PackageSyncerService.executeTask:fail] taskId: %s, targetName: %s, package not exists',
task.taskId, task.targetName);
await this.taskService.finishTask(task, TaskState.Fail, logs.join('\n'));
return;
}

Expand Down Expand Up @@ -924,8 +954,8 @@ export class PackageSyncerService extends AbstractService {
logs.push(`[${isoNow()}] 📝 Log URL: ${logUrl}`);
logs.push(`[${isoNow()}] 🔗 ${url}`);
task.error = lastErrorMessage;
await this.taskService.finishTask(task, TaskState.Success, logs.join('\n'));
this.logger.info('[PackageSyncerService.executeTask:success] taskId: %s, targetName: %s',
task.taskId, task.targetName);
await this.taskService.finishTask(task, TaskState.Success, logs.join('\n'));
}
}
2 changes: 1 addition & 1 deletion app/port/config.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export type CnpmcoreConfig = {
/**
* sync mode
* - none: don't sync npm package
* - admin: don't sync npm package,only admin can create sync task by sync contorller.
* - admin: don't sync npm package,only admin can create sync task by sync controller.
* - all: sync all npm packages
* - exist: only sync exist packages, effected when `enableCheckRecentlyUpdated` or `enableChangesStream` is enabled
*/
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@
"ssri": "^8.0.1",
"type-fest": "^2.5.3",
"ua-parser-js": "^1.0.34",
"validate-npm-package-name": "^3.0.0"
"validate-npm-package-name": "^6.0.0"
},
"optionalDependencies": {
"s3-cnpmcore": "^1.1.2"
Expand All @@ -138,7 +138,7 @@
"@types/semver": "^7.3.12",
"@types/tar": "^6.1.4",
"@types/ua-parser-js": "^0.7.36",
"@types/validate-npm-package-name": "^4.0.0",
"@types/validate-npm-package-name": "^4.0.2",
"coffee": "^5.4.0",
"egg-bin": "^6.0.0",
"egg-mock": "^5.10.4",
Expand Down
52 changes: 48 additions & 4 deletions test/core/service/PackageSyncerService/executeTask.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ describe('test/core/service/PackageSyncerService/executeTask.test.ts', () => {
app.mockAgent().assertNoPendingInterceptors();
});

it('should sync cnpmcore-test-sync-deprecated and mock 404', async () => {
it('should sync cnpmcore-test-sync-deprecated and mock 451', async () => {
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-deprecated', 'GET', {
data: await TestUtil.readFixturesFile('registry.npmjs.org/cnpmcore-test-sync-deprecated.json'),
persist: false,
Expand All @@ -209,9 +209,9 @@ describe('test/core/service/PackageSyncerService/executeTask.test.ts', () => {
assert.equal(abbreviatedManifests!.data!.versions['0.0.0']!._hasShrinkwrap, false);
app.mockAgent().assertNoPendingInterceptors();

// mock 404 and unpublished
// mock 451 and unpublished
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-deprecated', 'GET', {
status: 404,
status: 451,
data: '{"error":"Not found"}',
persist: false,
});
Expand Down Expand Up @@ -260,6 +260,50 @@ describe('test/core/service/PackageSyncerService/executeTask.test.ts', () => {
app.mockAgent().assertNoPendingInterceptors();
});

it('should sync cnpmcore-test-sync-deprecated and ignore 404 in removed', async () => {
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-deprecated', 'GET', {
data: await TestUtil.readFixturesFile('registry.npmjs.org/cnpmcore-test-sync-deprecated.json'),
persist: false,
});
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-deprecated/-/cnpmcore-test-sync-deprecated-0.0.0.tgz', 'GET', {
data: await TestUtil.readFixturesFile('registry.npmjs.org/foobar/-/foobar-1.0.0.tgz'),
persist: false,
});
const name = 'cnpmcore-test-sync-deprecated';
await packageSyncerService.createTask(name);
let task = await packageSyncerService.findExecuteTask();
assert(task);
await packageSyncerService.executeTask(task);
const manifests = await packageManagerService.listPackageFullManifests('', name);
assert(manifests.data);
assert(manifests!.data!.versions['0.0.0']);

assert.equal(manifests.data.versions['0.0.0'].deprecated, 'only test for cnpmcore');
assert.equal(manifests.data.versions['0.0.0']._hasShrinkwrap, false);
const abbreviatedManifests = await packageManagerService.listPackageAbbreviatedManifests('', name);
assert.equal(abbreviatedManifests!.data!.versions['0.0.0']!.deprecated, 'only test for cnpmcore');
assert.equal(abbreviatedManifests!.data!.versions['0.0.0']!._hasShrinkwrap, false);
app.mockAgent().assertNoPendingInterceptors();

// mock 404 and no unpublished
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-deprecated', 'GET', {
status: 404,
data: '{"error":"Not found"}',
persist: false,
});
await packageSyncerService.createTask(name);
task = await packageSyncerService.findExecuteTask();
assert(task);
await packageSyncerService.executeTask(task);
const stream = await packageSyncerService.findTaskLog(task);
assert(stream);
const log = await TestUtil.readStreamToLog(stream);
// console.log(log);
assert(!log.includes(`] 🟢 Package "${name}" was removed in remote registry`));
assert(log.includes('Package not found, status 404'));
app.mockAgent().assertNoPendingInterceptors();
});

it('should sync fail when package not exists', async () => {
app.mockHttpclient('https://registry.npmjs.org/cnpmcore-test-sync-package-not-exists', 'GET', {
status: 404,
Expand All @@ -275,7 +319,7 @@ describe('test/core/service/PackageSyncerService/executeTask.test.ts', () => {
assert(stream);
const log = await TestUtil.readStreamToLog(stream);
// console.log(log);
assert(log.includes('] ❌ Package not exists, response data: '));
assert(log.includes('Package not found, status 404'));
app.mockAgent().assertNoPendingInterceptors();
});

Expand Down

0 comments on commit 57226c5

Please sign in to comment.