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.

Sign up for GitHub

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

feat: proxy模式增强 #724

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: proxy模式增强 #724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
58 changes: 50 additions & 8 deletions app/core/service/ProxyCacheService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import { calculateIntegrity } from '../../common/PackageUtil';
import { ABBREVIATED_META_TYPE, PROXY_CACHE_DIR_NAME } from '../../common/constants';
import { DIST_NAMES } from '../entity/Package';
import type { AbbreviatedPackageManifestType, AbbreviatedPackageJSONType, PackageManifestType, PackageJSONType } from '../../repository/PackageRepository';
import { PackageManagerService } from './PackageManagerService';
import { getScopeAndName } from '../../common/PackageUtil';

function isoNow() {
return new Date().toISOString();
Expand Down Expand Up @@ -50,6 +52,8 @@ export class ProxyCacheService extends AbstractService {
private readonly cacheService: CacheService;
@Inject()
private readonly backgroundTaskHelper:BackgroundTaskHelper;
@Inject()
private readonly packageManagerService: PackageManagerService;

async getPackageVersionTarResponse(fullname: string, ctx: EggContext): Promise<HttpClientResponse> {
if (this.config.cnpmcore.syncPackageBlockList.includes(fullname)) {
Expand All @@ -59,12 +63,17 @@ export class ProxyCacheService extends AbstractService {
}

async getPackageManifest(fullname: string, fileType: DIST_NAMES.FULL_MANIFESTS| DIST_NAMES.ABBREVIATED_MANIFESTS): Promise<AbbreviatedPackageManifestType|PackageManifestType> {
const cachedStoreKey = (await this.proxyCacheRepository.findProxyCache(fullname, fileType))?.filePath;
if (cachedStoreKey) {
const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey);
const nfsString = Buffer.from(nfsBytes!).toString();
const nfsPkgManifgest = JSON.parse(nfsString);
return nfsPkgManifgest;
try {
const cachedStoreKey = (await this.proxyCacheRepository.findProxyCache(fullname, fileType))?.filePath;
if (cachedStoreKey) {
const nfsBytes = await this.nfsAdapter.getBytes(cachedStoreKey);
const nfsString = Buffer.from(nfsBytes!).toString();
const nfsPkgManifest = JSON.parse(nfsString);
return nfsPkgManifest;
}
} catch (e) {
this.logger.error(e);
this.logger.error('[ProxyCacheService.getPackageManifest:error] get cache error, ignore');
}

const manifest = await this.getRewrittenManifest<typeof fileType>(fullname, fileType);
Expand Down Expand Up @@ -152,6 +161,7 @@ export class ProxyCacheService extends AbstractService {
}

async getRewrittenManifest<T extends DIST_NAMES>(fullname:string, fileType: T, versionOrTag?:string): Promise<GetSourceManifestAndCacheReturnType<T>> {
const [ scope, name ] = getScopeAndName(fullname);
let responseResult;
switch (fileType) {
case DIST_NAMES.FULL_MANIFESTS:
Expand All @@ -171,9 +181,24 @@ export class ProxyCacheService extends AbstractService {
}

// replace tarball url
const manifest = responseResult.data;
const { status, data: manifest } = responseResult;
Copy link
Member

Choose a reason for hiding this comment

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

manifest 是 const 的话，下面会被重新赋值，应该先设置它是 let manifest = undefined，确保代码的可读性。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

manifest 是 const 的话，下面会被重新赋值，应该先设置它是 let manifest = undefined，确保代码的可读性。

两个manifest的作用域不一样，不存在重新赋值的说法吧？如果是为了可读性的话，一个改成remoteManifest一个改成privateManifest，这样可以吗？

// sourceRegistry not found, check private package
if (status === 404) {
const { etag, data: manifest, blockReason } = fileType === DIST_NAMES.FULL_MANIFESTS ?
await this.packageManagerService.listPackageFullManifests(scope, name, false) :
await this.packageManagerService.listPackageAbbreviatedManifests(scope, name, false);
// found in private package
if (etag && !blockReason) {
return manifest as any;
}
}
const { sourceRegistry, registry } = this.config.cnpmcore;
if (isPkgManifest(fileType)) {
const { etag, data, blockReason } = fileType === DIST_NAMES.FULL_MANIFESTS ?
await this.packageManagerService.listPackageFullManifests(scope, name, false) :
await this.packageManagerService.listPackageAbbreviatedManifests(scope, name, false);
const hasPrivatePackage = etag && !blockReason;

// pkg manifest
const versionMap = manifest.versions || {};
for (const key in versionMap) {
Expand All @@ -182,9 +207,26 @@ export class ProxyCacheService extends AbstractService {
versionItem.dist.tarball = versionItem.dist.tarball.replace(sourceRegistry, registry);
}
}
// private manifest
if (hasPrivatePackage) {
const privateVersionMap = data?.versions || {};
for (const key in privateVersionMap) {
if (!versionMap[key]) {
versionMap[key] = privateVersionMap[key];
}
}
if (manifest.time) {
const privateTimeMap = data?.time || {};
for (const key in privateTimeMap) {
if (!manifest.time[key]) {
manifest.time[key] = privateTimeMap[key];
}
}
}
}
Comment on lines +210 to +226
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add defensive checks for manifest merging

The manifest merging logic should include additional checks to handle edge cases and ensure data integrity.

-if (hasPrivatePackage) {
+if (hasPrivatePackage && data && manifest) {
+  // Ensure versions object exists
+  manifest.versions = manifest.versions || {};
+  manifest.time = manifest.time || {};
+
   const privateVersionMap = data?.versions || {};
   for (const key in privateVersionMap) {
-    if (!versionMap[key]) {
+    if (!versionMap[key] && privateVersionMap[key]?.version === key) {
       versionMap[key] = privateVersionMap[key];
     }
   }
-  if (manifest.time) {
-    const privateTimeMap = data?.time || {};
-    for (const key in privateTimeMap) {
-      if (!manifest.time[key]) {
-        manifest.time[key] = privateTimeMap[key];
-      }
+  const privateTimeMap = data?.time || {};
+  for (const key in privateTimeMap) {
+    const timeValue = privateTimeMap[key];
+    if (!manifest.time[key] && typeof timeValue === 'string' && isValidISODateString(timeValue)) {
+      manifest.time[key] = timeValue;
     }
   }
 }

Consider adding a helper function to validate ISO date strings:

function isValidISODateString(dateString: string): boolean {
  if (typeof dateString !== 'string') return false;
  const date = new Date(dateString);
  return date instanceof Date && !isNaN(date.getTime()) && date.toISOString() === dateString;
}

🛠️ Refactor suggestion

Add version format validation before merging

The version merging logic should validate the format of version strings before merging to prevent invalid versions from being added to the manifest. Consider using the semver package that's already imported.

 if (hasPrivatePackage) {
   const privateVersionMap = data?.versions || {};
   for (const key in privateVersionMap) {
-    if (!versionMap[key]) {
+    if (!versionMap[key] && semverValid(key)) {
       versionMap[key] = privateVersionMap[key];
+    } else if (!semverValid(key)) {
+      this.logger.warn('[ProxyCacheService:getRewrittenManifest] Skipping invalid version %s from private package %s', key, fullname);
     }
   }
   if (manifest.time) {
     const privateTimeMap = data?.time || {};
     for (const key in privateTimeMap) {
-      if (!manifest.time[key]) {
+      if (!manifest.time[key] && (key === 'created' || key === 'modified' || semverValid(key))) {
         manifest.time[key] = privateTimeMap[key];
+      } else if (key !== 'created' && key !== 'modified' && !semverValid(key)) {
+        this.logger.warn('[ProxyCacheService:getRewrittenManifest] Skipping invalid time entry %s from private package %s', key, fullname);
       }
     }
   }
 }

Committable suggestion skipped: line range outside the PR's diff.

} else {
// pkg version manifest
const distItem = manifest.dist || {};
const distItem = manifest?.dist || {};
if (distItem.tarball) {
distItem.tarball = distItem.tarball.replace(sourceRegistry, registry);
}
Expand Down
28 changes: 27 additions & 1 deletion app/port/controller/package/UpdatePackageController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ import { AbstractController } from '../AbstractController';
import { FULLNAME_REG_STRING } from '../../../common/PackageUtil';
import { User as UserEntity } from '../../../core/entity/User';
import { PackageManagerService } from '../../../core/service/PackageManagerService';
import { SyncMode } from '../../../common/constants';
import { ProxyCacheRepository } from '../../../repository/ProxyCacheRepository';
import { isPkgManifest, ProxyCacheService } from '../../../core/service/ProxyCacheService';

const MaintainerDataRule = Type.Object({
maintainers: Type.Array(Type.Object({
Expand All @@ -30,7 +33,10 @@ type Maintainer = Static<typeof MaintainerDataRule>;
export class UpdatePackageController extends AbstractController {
@Inject()
private packageManagerService: PackageManagerService;

@Inject()
private readonly proxyCacheRepository: ProxyCacheRepository;
@Inject()
private readonly proxyCacheService: ProxyCacheService;
// https://github.com/npm/cli/blob/latest/lib/commands/owner.js#L191
@HTTPMethod({
// PUT /:fullname/-rev/:rev
Expand Down Expand Up @@ -65,6 +71,26 @@ export class UpdatePackageController extends AbstractController {
}

await this.packageManagerService.replacePackageMaintainersAndDist(pkg, users);
// 代理模式下，更新代理缓存
fangzhengjin marked this conversation as resolved.
Show resolved Hide resolved
if (this.config.cnpmcore.syncMode === SyncMode.proxy) {
const refreshList = await this.proxyCacheRepository.findProxyCaches(fullname);
if (refreshList.length !== 0) {
const taskList = refreshList
// 仅manifests需要更新，指定版本的package.json文件发布后不会改变
.filter(i => isPkgManifest(i.fileType))
.map(async item => {
const task = await this.proxyCacheService.createTask(
`${item.fullname}/${item.fileType}`,
{
fullname: item.fullname,
fileType: item.fileType,
},
);
return task;
});
await Promise.all(taskList);
}
}
Comment on lines +74 to +93
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for proxy cache updates

While the proxy cache update logic is correct, it lacks error handling for the Promise.all operation. Failed cache updates should be logged but not block the main operation.

Consider applying this improvement:

    if (this.config.cnpmcore.syncMode === SyncMode.proxy) {
      const refreshList = await this.proxyCacheRepository.findProxyCaches(fullname);
      if (refreshList.length !== 0) {
        const taskList = refreshList
          .filter(i => isPkgManifest(i.fileType))
          .map(item => 
            this.proxyCacheService.createTask(
              `${item.fullname}/${item.fileType}`,
              {
                fullname: item.fullname,
                fileType: item.fileType,
              },
            )
          );
+       try {
          await Promise.all(taskList);
+       } catch (error) {
+         // Log error but don't block the main operation
+         ctx.logger.error('[UpdatePackageController] Failed to update proxy cache', {
+           error,
+           fullname,
+           taskCount: taskList.length,
+         });
+       }
      }
    }

Committable suggestion skipped: line range outside the PR's diff.

return { ok: true };
}

Expand Down
Loading