refactor: change scope of reused utility

Author: ravipanguluri

src/package/packageConvert.ts

@@ -30,9 +30,15 @@ import {
   ScratchOrgSettingsGenerator,
 } from '@salesforce/core';
 import { camelCaseToTitleCase, Duration, env } from '@salesforce/kit';
+import { isPackagingDirectory } from '@salesforce/core/project';
 import { Many } from '@salesforce/ts-types';
 import * as pkgUtils from '../utils/packageUtils';
-import { copyDescriptorProperties, generatePackageAliasEntry, uniqid } from '../utils/packageUtils';
+import {
+  copyDescriptorProperties,
+  generatePackageAliasEntry,
+  uniqid,
+  resolveBuildUserPermissions,
+} from '../utils/packageUtils';
 import {
   ConvertPackageOptions,
   PackageDescriptorJson,
@@ -199,7 +205,7 @@ export async function createPackageVersionCreateRequest(
   const metadataZipFile = path.join(packageVersBlobDirectory, 'package.zip');
   const packageVersBlobZipFile = path.join(packageVersTmpRoot, 'package-version-info.zip');
 
-  let packageDescriptorJson: PackageDescriptorJson = pkgUtils.buildPackageDescriptorJson({
+  let packageDescriptorJson: PackageDescriptorJson = buildPackageDescriptorJson({
     packageId,
     base: { versionNumber: context.patchversion },
     project,
@@ -259,7 +265,7 @@ export async function createPackageVersionCreateRequest(
   // Zip the packageVersMetadataFolder folder and put the zip in {packageVersBlobDirectory}/package.zip
   await pkgUtils.zipDir(packageVersMetadataFolder, metadataZipFile);
 
-  resolveBuildUserPermissions(packageDescriptorJson, context.codecoverage ?? false);
+  packageDescriptorJson = resolveBuildUserPermissions(packageDescriptorJson, context.codecoverage ?? false);
 
   await fs.promises.writeFile(
     path.join(packageVersBlobDirectory, 'package2-descriptor.json'),
@@ -270,48 +276,38 @@ export async function createPackageVersionCreateRequest(
   return createRequestObject(packageId, context, packageVersTmpRoot, packageVersBlobZipFile);
 }
 
-/** side effect: modifies the passed in parameter! */
-function resolveBuildUserPermissions(packageDescriptorJson: PackageDescriptorJson, codecoverage: boolean): void {
-  // Process permissionSet and permissionSetLicenses that should be enabled when running Apex tests
-  // This only applies if code coverage is enabled
-  if (codecoverage) {
-    // Assuming no permission sets are named 0, 0n, null, undefined, false, NaN, and the empty string
-    if (packageDescriptorJson.apexTestAccess?.permissionSets) {
-      let permSets = packageDescriptorJson.apexTestAccess.permissionSets;
-      if (!Array.isArray(permSets)) {
-        permSets = permSets.split(',');
-      }
-      packageDescriptorJson.permissionSetNames = permSets.map((s) => s.trim());
-    }
-
-    if (packageDescriptorJson.apexTestAccess?.permissionSetLicenses) {
-      let permissionSetLicenses = packageDescriptorJson.apexTestAccess.permissionSetLicenses;
-      if (!Array.isArray(permissionSetLicenses)) {
-        permissionSetLicenses = permissionSetLicenses.split(',');
-      }
-      packageDescriptorJson.permissionSetLicenseDeveloperNames = permissionSetLicenses.map((s) => s.trim());
-    }
-  }
+function buildPackageDescriptorJson(args: {
+  packageId: string;
+  base?: Partial<PackageDescriptorJson>;
+  project?: SfProject;
+}): PackageDescriptorJson {
+  const { packageId, base, project } = args;
+  const descriptor: Partial<PackageDescriptorJson> = {
+    id: packageId,
+    ...(base ?? {}),
+  };
 
-  // Process permissionSet and permissionsetLicenses that should be enabled for the package metadata deploy
-  if (packageDescriptorJson.packageMetadataAccess?.permissionSets) {
-    let permSets = packageDescriptorJson.packageMetadataAccess.permissionSets;
-    if (!Array.isArray(permSets)) {
-      permSets = permSets.split(',');
-    }
-    packageDescriptorJson.packageMetadataPermissionSetNames = permSets.map((s) => s.trim());
-  }
+  if (project) {
+    const packageObject = project.findPackage((namedPackageDir) => {
+      if (!isPackagingDirectory(namedPackageDir)) return false;
+      const dirPackageId = project.getPackageIdFromAlias(namedPackageDir.package) ?? namedPackageDir.package;
+      return dirPackageId === packageId;
+    });
 
-  if (packageDescriptorJson.packageMetadataAccess?.permissionSetLicenses) {
-    let permissionSetLicenses = packageDescriptorJson.packageMetadataAccess.permissionSetLicenses;
-    if (!Array.isArray(permissionSetLicenses)) {
-      permissionSetLicenses = permissionSetLicenses.split(',');
+    if (packageObject && isPackagingDirectory(packageObject)) {
+      const allowedKeys: Array<keyof PackageDescriptorJson> = ['apexTestAccess'];
+      for (const key of allowedKeys) {
+        if (Object.prototype.hasOwnProperty.call(packageObject, key)) {
+          const value = (packageObject as unknown as PackageDescriptorJson)[key];
+          if (value !== undefined) {
+            (descriptor as PackageDescriptorJson)[key] = value as never;
+          }
+        }
+      }
     }
-    packageDescriptorJson.packageMetadataPermissionSetLicenseNames = permissionSetLicenses.map((s) => s.trim());
   }
 
-  delete packageDescriptorJson.apexTestAccess;
-  delete packageDescriptorJson.packageMetadataAccess;
+  return descriptor as PackageDescriptorJson;
 }
 
 async function createRequestObject(

src/package/packageVersionCreate.ts

@@ -409,7 +409,10 @@ export class PackageVersionCreate {
       packageDescriptorJson = copyDescriptorProperties(packageDescriptorJson, definitionFileJson);
     }
 
-    this.resolveBuildUserPermissions(packageDescriptorJson);
+    packageDescriptorJson = pkgUtils.resolveBuildUserPermissions(
+      packageDescriptorJson,
+      this.options.codecoverage ?? false
+    );
 
     // All dependencies for the packaging dir should be resolved to an 04t id to be passed to the server.
     // (see resolveSubscriberPackageVersionId() for details)
@@ -609,49 +612,7 @@ export class PackageVersionCreate {
     await zipDir(packageVersBlobDirectory, packageVersBlobZipFile);
   }
 
-  /** side effect: modifies the passed in parameter! */
-  private resolveBuildUserPermissions(packageDescriptorJson: PackageDescriptorJson): void {
-    // Process permissionSet and permissionSetLicenses that should be enabled when running Apex tests
-    // This only applies if code coverage is enabled
-    if (this.options.codecoverage) {
-      // Assuming no permission sets are named 0, 0n, null, undefined, false, NaN, and the empty string
-      if (packageDescriptorJson.apexTestAccess?.permissionSets) {
-        let permSets = packageDescriptorJson.apexTestAccess.permissionSets;
-        if (!Array.isArray(permSets)) {
-          permSets = permSets.split(',');
-        }
-        packageDescriptorJson.permissionSetNames = permSets.map((s) => s.trim());
-      }
-
-      if (packageDescriptorJson.apexTestAccess?.permissionSetLicenses) {
-        let permissionSetLicenses = packageDescriptorJson.apexTestAccess.permissionSetLicenses;
-        if (!Array.isArray(permissionSetLicenses)) {
-          permissionSetLicenses = permissionSetLicenses.split(',');
-        }
-        packageDescriptorJson.permissionSetLicenseDeveloperNames = permissionSetLicenses.map((s) => s.trim());
-      }
-    }
-
-    // Process permissionSet and permissionsetLicenses that should be enabled for the package metadata deploy
-    if (packageDescriptorJson.packageMetadataAccess?.permissionSets) {
-      let permSets = packageDescriptorJson.packageMetadataAccess.permissionSets;
-      if (!Array.isArray(permSets)) {
-        permSets = permSets.split(',');
-      }
-      packageDescriptorJson.packageMetadataPermissionSetNames = permSets.map((s) => s.trim());
-    }
-
-    if (packageDescriptorJson.packageMetadataAccess?.permissionSetLicenses) {
-      let permissionSetLicenses = packageDescriptorJson.packageMetadataAccess.permissionSetLicenses;
-      if (!Array.isArray(permissionSetLicenses)) {
-        permissionSetLicenses = permissionSetLicenses.split(',');
-      }
-      packageDescriptorJson.packageMetadataPermissionSetLicenseNames = permissionSetLicenses.map((s) => s.trim());
-    }
-
-    delete packageDescriptorJson.apexTestAccess;
-    delete packageDescriptorJson.packageMetadataAccess;
-  }
+  // removed: resolveBuildUserPermissions - replaced by pkgUtils.normalizeBuildUserPermissions
 
   // eslint-disable-next-line complexity
   private async packageVersionCreate(): Promise<PackageVersionCreateRequestResult> {

src/utils/packageUtils.ts

@@ -20,7 +20,6 @@ import { pipeline as cbPipeline } from 'node:stream';
 import util, { promisify } from 'node:util';
 import { randomBytes } from 'node:crypto';
 import { Connection, Logger, Messages, ScratchOrgInfo, SfdcUrl, SfError, SfProject } from '@salesforce/core';
-import { isPackagingDirectory } from '@salesforce/core/project';
 import { isNumber, isString, Many, Optional } from '@salesforce/ts-types';
 import type { SaveError } from '@jsforce/jsforce-node';
 import { Duration, ensureArray } from '@salesforce/kit';
@@ -51,9 +50,7 @@ const ID_REGISTRY = [
     label: 'Subscriber Package Version Id',
   },
 ];
-const PACKAGE_DESCRIPTOR_FIELD_ALLOWLIST: ReadonlyArray<keyof PackageDescriptorJson> = [
-  'apexTestAccess',
-] as const satisfies ReadonlyArray<keyof PackageDescriptorJson>;
+//
 
 export type IdRegistryValue = { prefix: string; label: string };
 export type IdRegistry = {
@@ -527,66 +524,57 @@ export function copyDescriptorProperties(
 }
 
 /**
- * Copies only the allowlisted properties from source to target. Mutates and returns target.
+ * Resolve descriptor permissions for build/test execution.
+ *
+ * When {@link codecoverage} is true, converts {@link apexTestAccess} settings into
+ * permissionSetNames and permissionSetLicenseDeveloperNames.
+ * Always converts {@link packageMetadataAccess} settings into
+ * packageMetadataPermissionSetNames and packageMetadataPermissionSetLicenseNames.
+ * Removes apexTestAccess and packageMetadataAccess from the returned descriptor.
+ *
+ * @param descriptor Package descriptor to normalize
+ * @param codecoverage Whether to enable apexTestAccess-based permission processing
+ * @returns A normalized copy of the descriptor with flattened permission fields
  */
-function copyPropsAllowlist<T extends object, K extends keyof T>(
-  target: Partial<T>,
-  source: T | undefined,
-  allowed: readonly K[]
-): Partial<T> {
-  if (!source) return target;
-  for (const key of allowed) {
-    if (Object.prototype.hasOwnProperty.call(source, key)) {
-      const value = source[key];
-      if (value !== undefined) {
-        // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-        // @ts-ignore - index type on generic target
-        target[key] = value;
-      }
+export function resolveBuildUserPermissions(
+  descriptor: PackageDescriptorJson,
+  codecoverage: boolean
+): PackageDescriptorJson {
+  const copy = structuredClone(descriptor);
+
+  if (codecoverage) {
+    if (copy.apexTestAccess?.permissionSets) {
+      let permSets = copy.apexTestAccess.permissionSets;
+      if (!Array.isArray(permSets)) permSets = permSets.split(',');
+      copy.permissionSetNames = permSets.map((s) => s.trim());
     }
-  }
-  return target;
-}
 
-/**
- * Builds a base PackageDescriptorJson for a given packageId and optionally copies
- * an allowlist of properties from the matching packaging directory in the project.
- *
- * @param packageId
- * @param base - a partial package descriptor json with predefined properties
- * @param project
- * @returns a package descriptor json with the predefined properties and allowed properties from the matching packaging directory in the project
- */
-export function buildPackageDescriptorJson(args: {
-  packageId: string;
-  base?: Partial<PackageDescriptorJson>;
-  project?: SfProject;
-}): PackageDescriptorJson {
-  const { packageId, base, project } = args;
-  const descriptor: Partial<PackageDescriptorJson> = {
-    id: packageId,
-    ...(base ?? {}),
-  };
-
-  if (project) {
-    const packageObject = project.findPackage((namedPackageDir) => {
-      if (!isPackagingDirectory(namedPackageDir)) return false;
-      const dirPackageId = project.getPackageIdFromAlias(namedPackageDir.package) ?? namedPackageDir.package;
-      return dirPackageId === packageId;
-    });
-
-    if (packageObject && isPackagingDirectory(packageObject)) {
-      copyPropsAllowlist(
-        descriptor as PackageDescriptorJson,
-        packageObject as unknown as PackageDescriptorJson,
-        PACKAGE_DESCRIPTOR_FIELD_ALLOWLIST
-      );
+    if (copy.apexTestAccess?.permissionSetLicenses) {
+      let psl = copy.apexTestAccess.permissionSetLicenses;
+      if (!Array.isArray(psl)) psl = psl.split(',');
+      copy.permissionSetLicenseDeveloperNames = psl.map((s) => s.trim());
     }
   }
 
-  return descriptor as PackageDescriptorJson;
+  if (copy.packageMetadataAccess?.permissionSets) {
+    let permSets = copy.packageMetadataAccess.permissionSets;
+    if (!Array.isArray(permSets)) permSets = permSets.split(',');
+    copy.packageMetadataPermissionSetNames = permSets.map((s) => s.trim());
+  }
+
+  if (copy.packageMetadataAccess?.permissionSetLicenses) {
+    let psl = copy.packageMetadataAccess.permissionSetLicenses;
+    if (!Array.isArray(psl)) psl = psl.split(',');
+    copy.packageMetadataPermissionSetLicenseNames = psl.map((s) => s.trim());
+  }
+
+  delete copy.apexTestAccess;
+  delete copy.packageMetadataAccess;
+  return copy;
 }
 
+//
+
 /**
  * Brand new SFDX projects contain a force-app directory tree containing empty folders
  * and a few .eslintrc.json files. We still want to consider such a directory tree

test/package/packageConvert.test.ts

@@ -223,67 +223,6 @@ describe('packageConvert', () => {
       // apexTestAccess should be removed from the descriptor
       expect(package2DescriptorJson).to.not.have.string('apexTestAccess');
     });
-
-    it('should NOT process apexTestAccess permissions when codecoverage is false', async () => {
-      $$.inProject(true);
-      const project = SfProject.getInstance();
-
-      // Create the force-app directory that's referenced in sfdx-project.json
-      await fs.promises.mkdir(path.join(project.getPath(), 'force-app'), { recursive: true });
-
-      // Set up sfdx-project.json with packageDirectory containing apexTestAccess
-      project.getSfProjectJson().set('packageDirectories', [
-        {
-          path: 'force-app',
-          package: '0Ho3i000000Gmj6CAC',
-          apexTestAccess: {
-            permissionSets: ['Test_Permission_Set'],
-            permissionSetLicenses: ['TestPsl'],
-          },
-        },
-      ]);
-      await project.getSfProjectJson().write();
-
-      // Definition file is for scratch org settings only (no apexTestAccess)
-      const definitionFile = {
-        orgName: 'test org name',
-        edition: 'Developer',
-      };
-      const packageVersTmpRoot = path.join(os.tmpdir(), 'config-no-codecoverage');
-      await fs.promises.mkdir(packageVersTmpRoot, { recursive: true });
-      const scratchDefPath = path.join(packageVersTmpRoot, 'scratch-no-codecoverage.json');
-      await fs.promises.writeFile(scratchDefPath, JSON.stringify(definitionFile, undefined, 2));
-
-      // Create a spy to capture what's written to package2-descriptor.json
-      const writeFileSpy = $$.SANDBOX.spy(fs.promises, 'writeFile');
-
-      const request = await createPackageVersionCreateRequest(
-        { installationkey: '123', definitionfile: scratchDefPath, buildinstance: 'myInstance', codecoverage: false },
-        '0Ho3i000000Gmj6CAC',
-        '54.0',
-        project
-      );
-
-      expect(request).to.have.all.keys(
-        'CalculateCodeCoverage',
-        'InstallKey',
-        'Instance',
-        'IsConversionRequest',
-        'Package2Id',
-        'VersionInfo'
-      );
-      expect(request.CalculateCodeCoverage).to.equal(false);
-
-      // Get the contents of package2-descriptor.json
-      const package2DescriptorJson = writeFileSpy.secondCall.args[1];
-
-      // Verify package2-descriptor.json does NOT contain permission set fields when codecoverage is false
-      expect(package2DescriptorJson).to.not.be.undefined;
-      expect(package2DescriptorJson).to.not.have.string('permissionSetNames');
-      expect(package2DescriptorJson).to.not.have.string('permissionSetLicenseDeveloperNames');
-      // apexTestAccess should still be removed from the descriptor
-      expect(package2DescriptorJson).to.not.have.string('apexTestAccess');
-    });
   });
 
   describe('findOrCreatePackage2', () => {