chore(compartment-mapper): peer review edits

- fix for mermaid graph
- fix for prePackageDependenciesFilter, plus comment
- makePackagePolicy throws if falsy label provided

Author: boneskull

packages/compartment-mapper/src/hooks.md

@@ -11,6 +11,7 @@
 [Type declarations for the hooks](./types/external.ts)
 
 ```mermaid
+
 graph TB
 
   exports((public exports))

packages/compartment-mapper/src/node-modules.js

@@ -104,6 +104,8 @@ const defaultUnknownCanonicalNameHandler = ({
  * Default filter for package dependencies based on policy.
  * Filters out dependencies not allowed by the package policy.
  *
+ * **Note:** This filter is _only_ applied if a policy is provided.
+ *
  * @param {object} params - The parameters object
  * @param {CanonicalName} params.canonicalName - The canonical name of the package
  * @param {Readonly<Set<CanonicalName>>} params.dependencies - The set of dependencies
@@ -116,6 +118,9 @@ const prePackageDependenciesFilter = (
   policy,
 ) => {
   const packagePolicy = makePackagePolicy(canonicalName, { policy });
+  if (!packagePolicy) {
+    return { dependencies };
+  }
   const filteredDependencies = new Set(
     [...dependencies].filter(dependency => {
       const allowed = dependencyAllowedByPolicy(dependency, packagePolicy);
@@ -128,9 +133,7 @@ const prePackageDependenciesFilter = (
     }),
   );
 
-  return filteredDependencies.size !== dependencies.size
-    ? { dependencies: filteredDependencies }
-    : undefined;
+  return { dependencies: filteredDependencies };
 };
 
 /**
@@ -897,7 +900,7 @@ const translateGraph = (
 
     // if "dependencies" are in here, then something changed the list.
     if (packageDependenciesHookResult?.dependencies) {
-      const { size } = packageDependenciesHookResult.dependencies;
+      const size = packageDependenciesHookResult.dependencies.size;
       if (typeof size === 'number' && size > 0) {
         // because the list of dependencies contains canonical names, we need to lookup any new ones.
         const nodesByCanonicalName = new Map(

packages/compartment-mapper/src/policy.js

@@ -129,35 +129,24 @@ export const dependencyAllowedByPolicy = (canonicalName, packagePolicy) => {
 };
 
 /**
- * Generates the {@link SomePackagePolicy} value to be used in {@link CompartmentDescriptor.policy}
- * @overload
- * @param {CanonicalName | typeof ATTENUATORS_COMPARTMENT} label
- * @param {object} options Options
- * @param {SomePolicy} options.policy User-supplied policy
- * @param {SomePackagePolicy} [options.packagePolicy] Package policy, if already known
- * @returns {SomePackagePolicy} Package policy from `policy` or empty object; returns `params.packagePolicy` if provided
- */
-/**
- * Generates the {@link SomePackagePolicy} value to be used in {@link CompartmentDescriptor.policy}
+ * Generates the {@link SomePackagePolicy} value to be used in
+ * {@link CompartmentDescriptor.policy}
  *
- * @overload
- * @param {CanonicalName | typeof ATTENUATORS_COMPARTMENT} label
+ * @param {CanonicalName | typeof ATTENUATORS_COMPARTMENT | typeof ENTRY_COMPARTMENT} label
  * @param {object} [options] Options
  * @param {SomePolicy} [options.policy] User-supplied policy
- * @param {SomePackagePolicy} [options.packagePolicy] Package policy, if already known
- * @returns {SomePackagePolicy|undefined} Package policy from `policy` or empty object; returns `params.packagePolicy` if provided
- */
-/**
- * Generates the {@link SomePackagePolicy} value to be used in {@link CompartmentDescriptor.policy}
- *
- * @param {CanonicalName | typeof ATTENUATORS_COMPARTMENT} label
- * @param {object} [options] Options
- * @param {SomePolicy} [options.policy] User-supplied policy
- * @returns {SomePackagePolicy|undefined} Package policy from `policy` or empty object; returns `params.packagePolicy` if provided
+ * @returns {SomePackagePolicy|undefined} Package policy from `policy` or empty
+ * object; returns `params.packagePolicy` if provided. If entry compartment,
+ * returns the `entry` property of the policy verbatim.
  */
 export const makePackagePolicy = (label, { policy } = {}) => {
   /** @type {SomePackagePolicy|undefined} */
   let packagePolicy;
+  if (!label) {
+    throw new TypeError(
+      `Invalid arguments: label must be a non-empty string; got ${q(label)}`,
+    );
+  }
   if (policy) {
     if (label === ATTENUATORS_COMPARTMENT) {
       packagePolicy = {
@@ -166,6 +155,7 @@ export const makePackagePolicy = (label, { policy } = {}) => {
       };
     } else if (label === ENTRY_COMPARTMENT) {
       packagePolicy = policy.entry;
+      return packagePolicy;
     } else if (label) {
       packagePolicy = policy.resources?.[label];
     }

packages/compartment-mapper/test/policy.test.js

@@ -3,7 +3,12 @@
 import 'ses';
 import test from 'ava';
 import { moduleify, scaffold, sanitizePaths } from './scaffold.js';
-import { WILDCARD_POLICY_VALUE } from '../src/policy-format.js';
+import {
+  WILDCARD_POLICY_VALUE,
+  ATTENUATORS_COMPARTMENT,
+  ENTRY_COMPARTMENT,
+} from '../src/policy-format.js';
+import { makePackagePolicy } from '../src/policy.js';
 
 function combineAssertions(...assertionFunctions) {
   return async (...args) => {
@@ -455,3 +460,176 @@ scaffold(
     policy: nestedAttenuator(policy),
   },
 );
+
+// Unit tests for makePackagePolicy
+test('makePackagePolicy() - no policy provided', t => {
+  t.is(makePackagePolicy('alice'), undefined);
+  t.is(makePackagePolicy(ATTENUATORS_COMPARTMENT), undefined);
+  t.is(makePackagePolicy(ENTRY_COMPARTMENT), undefined);
+  t.is(makePackagePolicy('alice', {}), undefined);
+});
+
+test('makePackagePolicy() - ATTENUATORS_COMPARTMENT label', t => {
+  const testPolicy = {
+    defaultAttenuator: 'myattenuator',
+    entry: { packages: { alice: true } },
+    resources: {},
+  };
+
+  const result = makePackagePolicy(ATTENUATORS_COMPARTMENT, {
+    policy: testPolicy,
+  });
+
+  t.deepEqual(result, {
+    defaultAttenuator: 'myattenuator',
+    packages: WILDCARD_POLICY_VALUE,
+  });
+});
+
+test('makePackagePolicy() - ATTENUATORS_COMPARTMENT label without defaultAttenuator', t => {
+  const testPolicy = {
+    entry: { packages: { alice: true } },
+    resources: {},
+  };
+
+  const result = makePackagePolicy(ATTENUATORS_COMPARTMENT, {
+    policy: testPolicy,
+  });
+
+  t.deepEqual(result, {
+    defaultAttenuator: undefined,
+    packages: WILDCARD_POLICY_VALUE,
+  });
+});
+
+test('makePackagePolicy() - ENTRY_COMPARTMENT label', t => {
+  const entryPolicy = {
+    globals: { bluePill: true },
+    packages: { alice: true },
+    builtins: { builtin: { attenuate: 'myattenuator', params: ['a', 'b'] } },
+  };
+  const testPolicy = {
+    defaultAttenuator: 'myattenuator',
+    entry: entryPolicy,
+    resources: {},
+  };
+
+  const result = makePackagePolicy(ENTRY_COMPARTMENT, { policy: testPolicy });
+
+  t.is(result, entryPolicy);
+  t.deepEqual(result, entryPolicy);
+});
+
+test('makePackagePolicy() - ENTRY_COMPARTMENT label with undefined entry', t => {
+  const testPolicy = {
+    defaultAttenuator: 'myattenuator',
+    resources: {},
+  };
+
+  const result = makePackagePolicy(ENTRY_COMPARTMENT, { policy: testPolicy });
+
+  t.is(result, undefined);
+});
+
+test('makePackagePolicy() - regular canonical name that exists in resources', t => {
+  const resourcePolicy = {
+    packages: { 'alice>carol': true },
+  };
+  const testPolicy = {
+    entry: { packages: { alice: true } },
+    resources: {
+      alice: resourcePolicy,
+    },
+  };
+
+  const result = makePackagePolicy('alice', { policy: testPolicy });
+
+  t.is(result, resourcePolicy);
+  t.deepEqual(result, resourcePolicy);
+});
+
+test('makePackagePolicy() - regular canonical name that does not exist in resources', t => {
+  const testPolicy = {
+    entry: { packages: { alice: true } },
+    resources: {
+      alice: { globals: { santorum: true } },
+    },
+  };
+
+  const result = makePackagePolicy('nonexistent', { policy: testPolicy });
+
+  t.not(result, undefined);
+  t.is(Object.getPrototypeOf(result), null);
+  t.deepEqual(result, {});
+  t.is(Object.keys(result).length, 0);
+});
+
+test('makePackagePolicy() - regular canonical name with resources undefined', t => {
+  const testPolicy = {
+    entry: { packages: { alice: true } },
+  };
+
+  const result = makePackagePolicy('alice', { policy: testPolicy });
+
+  t.not(result, undefined);
+  t.is(Object.getPrototypeOf(result), null);
+  t.deepEqual(result, {});
+  t.is(Object.keys(result).length, 0);
+});
+
+test('makePackagePolicy() - empty label throws', t => {
+  const testPolicy = {
+    entry: { packages: { alice: true } },
+    resources: {
+      alice: { globals: { santorum: true } },
+    },
+  };
+
+  t.throws(() => makePackagePolicy(null, { policy: testPolicy }), {
+    message: /Invalid arguments: label must be a non-empty string; got null/i,
+  });
+  t.throws(() => makePackagePolicy(undefined, { policy: testPolicy }), {
+    message:
+      /Invalid arguments: label must be a non-empty string; got undefined/i,
+  });
+  t.throws(() => makePackagePolicy('', { policy: testPolicy }), {
+    message: /Invalid arguments: label must be a non-empty string; got ""/i,
+  });
+});
+
+test('makePackagePolicy() - preserves object reference for entry', t => {
+  const entryPolicy = { packages: { alice: true } };
+  const testPolicy = {
+    entry: entryPolicy,
+    resources: {},
+  };
+
+  const result = makePackagePolicy(ENTRY_COMPARTMENT, { policy: testPolicy });
+
+  t.is(result, entryPolicy);
+});
+
+test('makePackagePolicy() - preserves object reference for resources', t => {
+  const resourcePolicy = { globals: { redPill: true } };
+  const testPolicy = {
+    entry: { packages: { alice: true } },
+    resources: {
+      alice: resourcePolicy,
+    },
+  };
+
+  const result = makePackagePolicy('alice', { policy: testPolicy });
+
+  t.is(result, resourcePolicy);
+});
+
+test('makePackagePolicy() - empty resources object returns empty package policy', t => {
+  const testPolicy = {
+    entry: { packages: { alice: true } },
+    resources: {},
+  };
+
+  const result = makePackagePolicy('alice', { policy: testPolicy });
+
+  t.deepEqual(result, {});
+});