From 9472c69ab0bf61ffe7b4b674418ceed164d15703 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 10:39:02 +1100 Subject: [PATCH 01/18] fix: match a named value reference by display name only, ignoring case, as APIM does --- src/services/named-value-names.ts | 78 +++++++++++++++ src/services/publish-service.ts | 78 ++++++++++++--- src/services/transitive-extractor.ts | 98 ++++++++++++++----- src/services/transitive-resolver.ts | 72 ++++++++++++++ tests/unit/services/extract-service.test.ts | 3 + tests/unit/services/named-value-names.test.ts | 56 +++++++++++ tests/unit/services/publish-service.test.ts | 70 ++++++++++++- .../services/transitive-extractor.test.ts | 47 ++++++--- .../unit/services/workspace-extractor.test.ts | 3 + 9 files changed, 453 insertions(+), 52 deletions(-) create mode 100644 src/services/named-value-names.ts create mode 100644 tests/unit/services/named-value-names.test.ts diff --git a/src/services/named-value-names.ts b/src/services/named-value-names.ts new file mode 100644 index 00000000..39450999 --- /dev/null +++ b/src/services/named-value-names.ts @@ -0,0 +1,78 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * The rule for what a `{{reference}}` to a named value means, in one place + * for extract and publish: it names the named value in its own scope (the + * service, or one workspace) with that display name, ignoring case + * (NamedValueNames). APIM matches display names only: it rejects a policy + * whose reference is only a named value's resource name. + * + * Extract builds the names from the named values APIM lists; publish builds + * them from the artifacts. On publish a reference can use either the display + * name a named value is published with (overrides applied) or, where an + * override changes it, its display name in the source. + */ + +/** A named value as far as matching references to it goes. */ +export interface NamedValueEntry { + /** Its resource name. */ + name: string; + /** Its display name (on publish, with overrides applied), when known. */ + displayName?: unknown; + /** + * On publish, its display name in the source. A reference to it still names + * the named value where an override changes the display name; a display + * name another named value is published with wins over it. + */ + sourceDisplayName?: unknown; +} + +/** The named values of one scope, matched as APIM matches a `{{reference}}`. */ +export class NamedValueNames { + /** Lower-case display name → resource name. */ + private readonly byReference = new Map(); + + constructor(namedValues: Iterable) { + const entries = [...namedValues]; + for (const key of ['sourceDisplayName', 'displayName'] as const) { + for (const entry of entries) { + const displayName = entry[key]; + if (typeof displayName === 'string') { + this.byReference.set(displayName.toLowerCase(), entry.name); + } + } + } + } + + /** + * The resource name of the named value with the reference as its display + * name, or undefined when none has it. + */ + resolve(reference: string): string | undefined { + return this.byReference.get(reference.toLowerCase()); + } +} + +/** + * NamedValueNames for each scope of the given named values, built on first + * use. + */ +export function namedValueNamesByScope( + namedValues: Iterable +): (workspace: string | undefined) => NamedValueNames { + const byScope = new Map>(); + for (const entry of namedValues) { + const scope = entry.workspace?.toLowerCase() ?? ''; + byScope.set(scope, [...(byScope.get(scope) ?? []), entry]); + } + const built = new Map(); + return (workspace) => { + const scope = workspace?.toLowerCase() ?? ''; + let names = built.get(scope); + if (!names) { + names = new NamedValueNames(byScope.get(scope) ?? []); + built.set(scope, names); + } + return names; + }; +} diff --git a/src/services/publish-service.ts b/src/services/publish-service.ts index 6d1e966a..77209d5c 100644 --- a/src/services/publish-service.ts +++ b/src/services/publish-service.ts @@ -50,9 +50,9 @@ import { } from './delete-unmatched-service.js'; import { computeGitDiff } from './git-diff-service.js'; import { scanForRedactionMarkers } from './secret-redaction-guard.js'; -import { hasNamedValueOverride } from './override-merger.js'; +import { applyOverrides, hasNamedValueOverride } from './override-merger.js'; import { REDACTION_MARKER } from './secret-redactor.js'; -import { scanArtifactReferences } from './transitive-resolver.js'; +import { createArtifactReferenceResolver, scanArtifactReferences } from './transitive-resolver.js'; import { validateAndBuildEnvMapping } from './env-mapping-validator.js'; import { mapDescriptor } from './env-mapper.js'; @@ -124,10 +124,15 @@ async function runPublishPipeline( // Step 0: Pre-flight validation — ensure the resource group and APIM service exist await client.validatePreFlight(config.service); + // What the source's references resolve to, read once for this run when + // first needed. + const sourceReferences = createSourceReferences(store, config); + // Step 1: Determine target descriptors based on incremental mode const { targetDescriptors, deletedDescriptors } = await determinePublishTargets( store, - config + config, + sourceReferences ); // Step 1a: For env-mapping validation and policy ref rewriting we need @@ -279,7 +284,8 @@ async function runPublishPipeline( */ async function determinePublishTargets( store: IArtifactStore, - config: PublishConfig + config: PublishConfig, + sourceReferences: SourceReferences ): Promise { let targetDescriptors: ResourceDescriptor[]; let deletedDescriptors: ResourceDescriptor[]; @@ -303,29 +309,73 @@ async function determinePublishTargets( deletedDescriptors = filterPublishDescriptors(deletedDescriptors, config.filter); if (config.filter && config.includeTransitive !== false) { - const availableDescriptors = await store.listResources(config.sourceDir); targetDescriptors = await expandTransitivePublishTargets( store, config.sourceDir, targetDescriptors, - availableDescriptors, - config.filter + config.filter, + sourceReferences ); } return { targetDescriptors, deletedDescriptors }; } +/** + * The source's artifacts and what references to them resolve to, as + * published: each read on first use and then shared, so a publish run lists + * the source and reads its named values once. + */ +interface SourceReferences { + /** Every artifact in the source. */ + readonly available: () => Promise; + /** + * The artifact a reference resolves to: a `{{reference}}` by the display + * names named values are published with (NamedValueNames). + */ + readonly resolve: (reference: ResourceDescriptor) => Promise; + /** A named value's artifact as stored, read once per run. */ + readonly readNamedValue: (descriptor: ResourceDescriptor) => Promise | undefined>; +} + +function createSourceReferences(store: IArtifactStore, config: PublishConfig): SourceReferences { + let available: Promise | undefined; + let resolver: Promise<(reference: ResourceDescriptor) => Promise> | undefined; + const namedValues = new Map | undefined>>(); + const references: SourceReferences = { + readNamedValue: (descriptor) => { + const key = getResourceDescriptorKey(descriptor); + let json = namedValues.get(key); + if (!json) { + json = store.readResource(config.sourceDir, descriptor); + namedValues.set(key, json); + } + return json; + }, + available: () => (available ??= store.listResources(config.sourceDir)), + resolve: async (reference) => { + resolver ??= references.available().then((descriptors) => + createArtifactReferenceResolver( + descriptors, + references.readNamedValue, + undefined, + (descriptor, json) => applyOverrides(descriptor, json, config.overrides) + ) + ); + return (await resolver)(reference); + }, + }; + return references; +} + async function expandTransitivePublishTargets( store: IArtifactStore, sourceDir: string, initialDescriptors: ResourceDescriptor[], - availableDescriptors: ResourceDescriptor[], - filter: FilterConfig + filter: FilterConfig, + sourceReferences: SourceReferences ): Promise { - const availableByKey = new Map( - availableDescriptors.map((descriptor) => [getResourceDescriptorKey(descriptor), descriptor]) - ); + const availableDescriptors = await sourceReferences.available(); const expanded = [...initialDescriptors]; const included = new Set(expanded.map(getResourceDescriptorKey)); const scanQueue = [...initialDescriptors]; @@ -385,7 +435,9 @@ async function expandTransitivePublishTargets( continue; } for (const reference of references) { - const available = availableByKey.get(getResourceDescriptorKey(reference)); + // A {{reference}} names a named value by its display name, which can + // differ from the name its artifact is stored under. + const available = await sourceReferences.resolve(reference); if (available && !included.has(getResourceDescriptorKey(available))) { included.add(getResourceDescriptorKey(available)); expanded.push(available); diff --git a/src/services/transitive-extractor.ts b/src/services/transitive-extractor.ts index 4f59fc56..82ccfa9d 100644 --- a/src/services/transitive-extractor.ts +++ b/src/services/transitive-extractor.ts @@ -11,6 +11,7 @@ import { logger } from '../lib/logger.js'; import { runParallel } from '../lib/parallel-runner.js'; import { redactSecrets } from './secret-redactor.js'; import { findTransitiveDependencies } from './transitive-resolver.js'; +import { NamedValueNames, type NamedValueEntry } from './named-value-names.js'; const DEFAULT_CONCURRENCY = 5; @@ -27,27 +28,31 @@ export interface TransitiveExtractionResult { interface TransitiveTaskResult { dep: ResourceDescriptor; json?: Record; - /** Resolved to a resource already extracted under its real name. */ + /** Resolved to a resource already extracted or claimed by another reference. */ skipped?: boolean; } /** - * Per-context (keyed by baseUrl) index of named value displayName → resource name. - * Stored as promises so concurrent tasks share a single LIST call. + * Per-context (keyed by baseUrl) NamedValueNames of the named values APIM + * lists. Stored as promises so concurrent tasks share a single LIST call. */ -type NamedValueNameIndex = Map>>; +type NamedValueNameIndex = Map>; + +interface ListedNamedValues { + names: NamedValueNames; + /** False when the named values couldn't be listed, so `names` is incomplete. */ + listed: boolean; +} async function loadNamedValueNames( client: IApimClient, context: ApimServiceContext -): Promise> { - const byDisplayName = new Map(); +): Promise { + const entries: NamedValueEntry[] = []; try { for await (const json of client.listResources(context, ResourceType.NamedValue)) { - const name = json.name; - const displayName = (json.properties as Record | undefined)?.displayName; - if (typeof name === 'string' && typeof displayName === 'string') { - byDisplayName.set(displayName, name); + if (typeof json.name === 'string') { + entries.push({ name: json.name, displayName: (json.properties as Record | undefined)?.displayName }); } } } catch (error) { @@ -56,30 +61,46 @@ async function loadNamedValueNames( `Failed to list named values for display name resolution; ` + `falling back to resolving {{references}} as resource names: ${message}` ); + return { names: new NamedValueNames(entries), listed: false }; } - return byDisplayName; + return { names: new NamedValueNames(entries), listed: true }; } -/** - * Policies reference named values as {{displayName}}, but the ARM GET endpoint - * is keyed by resource name. Map the reference to the resource name when they - * differ; otherwise return the descriptor unchanged. - */ -async function resolveNamedValueDescriptor( +/** The named value name index for a context, listed once and shared. */ +function namedValueNames( client: IApimClient, context: ApimServiceContext, - dep: ResourceDescriptor, index: NamedValueNameIndex -): Promise { +): Promise { let names = index.get(context.baseUrl); if (!names) { names = loadNamedValueNames(client, context); index.set(context.baseUrl, names); } + return names; +} +/** + * Policies reference named values as {{displayName}}, but the ARM GET endpoint + * is keyed by resource name: the named value a reference names, under its + * resource name. Undefined when the named values were listed and none has the + * reference as its display name (APIM matches display names only). When they + * couldn't be listed, the reference is tried as the resource name, which it + * usually also is. + */ +async function resolveNamedValueDescriptor( + client: IApimClient, + context: ApimServiceContext, + dep: ResourceDescriptor, + index: NamedValueNameIndex +): Promise { const ref = dep.nameParts[0] ?? ''; - const name = (await names).get(ref); - if (!name || name === ref) { + const { names, listed } = await namedValueNames(client, context, index); + const name = names.resolve(ref); + if (name === undefined) { + return listed ? undefined : dep; + } + if (name === ref) { return dep; } @@ -99,20 +120,42 @@ export async function extractTransitiveDependencies( workspace?: string, serviceContext?: ApimServiceContext ): Promise { - const attempted = new Set(alreadyExtracted.map(getResourceDescriptorKey)); + // A {{reference}} to an extracted named value's name can be another named + // value's display name, so named value references are only compared with + // what is extracted once resolved (claimedNamedValues). + const attempted = new Set( + alreadyExtracted.filter((descriptor) => descriptor.type !== ResourceType.NamedValue).map(getResourceDescriptorKey) + ); const claimedNamedValues = new Set(alreadyExtracted.map(getResourceDescriptorKey)); const extractedDescriptors: ResourceDescriptor[] = []; const namedValueIndex: NamedValueNameIndex = new Map(); + const contextFor = (dep: ResourceDescriptor): ApimServiceContext => + serviceContext && dep.workspace !== workspace ? serviceContext : context; let errorCount = 0; let foundDependencies = false; while (true) { - const newDeps = findTransitiveDependencies( + const found = findTransitiveDependencies( policies, apis, workspace, resources ).filter((dep) => !attempted.has(getResourceDescriptorKey(dep))); + const newDeps: ResourceDescriptor[] = []; + for (const dep of found) { + if (dep.type !== ResourceType.NamedValue) { + newDeps.push(dep); + continue; + } + const resolved = await resolveNamedValueDescriptor(client, contextFor(dep), dep, namedValueIndex); + if (!resolved) { + // APIM accepts only references to display names, so none is expected. + attempted.add(getResourceDescriptorKey(dep)); + logger.warn(`No named value has the display name {{${dep.nameParts[0] ?? ''}}}, so it was not extracted`); + } else if (!claimedNamedValues.has(getResourceDescriptorKey(resolved))) { + newDeps.push(dep); + } + } if (newDeps.length === 0) { if (!foundDependencies) { @@ -129,11 +172,14 @@ export async function extractTransitiveDependencies( const tasks = newDeps.map((dep) => async (): Promise => { try { - const dependencyContext = - serviceContext && dep.workspace !== workspace ? serviceContext : context; + const dependencyContext = contextFor(dep); let target = dep; if (dep.type === ResourceType.NamedValue) { - target = await resolveNamedValueDescriptor(client, dependencyContext, dep, namedValueIndex); + const resolved = await resolveNamedValueDescriptor(client, dependencyContext, dep, namedValueIndex); + if (!resolved) { + return { dep, skipped: true }; + } + target = resolved; const targetKey = getResourceDescriptorKey(target); if (claimedNamedValues.has(targetKey)) { return { dep: target, skipped: true }; diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index 1251e823..c08d592d 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -13,6 +13,8 @@ import { ResourceDescriptor } from '../models/types.js'; import type { IArtifactStore } from '../clients/iartifact-store.js'; import { logger } from '../lib/logger.js'; import { getResourceDescriptorKey } from '../lib/resource-path.js'; +import { buildResourceLabel } from '../lib/resource-uri.js'; +import { type NamedValueEntry, NamedValueNames, namedValueNamesByScope } from './named-value-names.js'; /** * Reference detection patterns for policy XML content. @@ -311,6 +313,76 @@ export async function scanArtifactReferences( return deduplicateDescriptors(references); } +/** + * Turns an artifact's JSON into the payload that is published, such as with + * overrides applied. + */ +export type PrepareJson = (descriptor: ResourceDescriptor, json: Record) => Record; + +/** + * Resolve the references scanArtifactReferences returns to the artifacts + * they name. Policies reference named values as {{displayName}}, which can + * differ from the resource name the artifact is stored under; as in APIM, a + * reference matches display names only (NamedValueNames). Named value + * artifacts are read once, on the first named value reference. + * + * @param available - The artifacts references can resolve to + * @param readResource - Reads an artifact's JSON + * @param onUnreadable - Called for a named value artifact that can't be read, + * so a {{reference}} to its display name may resolve to nothing + * @param prepareJson - Turns a named value's JSON into what is published, + * such as with overrides applied; a reference then matches the display name + * it is published with, or the one in the source (NamedValueEntry) + */ +export function createArtifactReferenceResolver( + available: ReadonlyArray, + readResource: (descriptor: ResourceDescriptor) => Promise | undefined>, + onUnreadable?: (descriptor: ResourceDescriptor) => void, + prepareJson?: PrepareJson +): (reference: ResourceDescriptor) => Promise { + const byKey = new Map(available.map((descriptor) => [getResourceDescriptorKey(descriptor), descriptor])); + let names: Promise<(workspace: string | undefined) => NamedValueNames> | undefined; + return async (reference) => { + if (reference.type !== ResourceType.NamedValue) { + return byKey.get(getResourceDescriptorKey(reference)); + } + names ??= readNamedValueNames(available, readResource, onUnreadable, prepareJson); + const name = (await names)(reference.workspace).resolve(reference.nameParts[0] ?? ''); + return name === undefined ? undefined : byKey.get(getResourceDescriptorKey({ ...reference, nameParts: [name] })); + }; +} + +/** + * The NamedValueNames of the named value artifacts among `available`. One that + * can't be read can't be matched. + */ +async function readNamedValueNames( + available: ReadonlyArray, + readResource: (descriptor: ResourceDescriptor) => Promise | undefined>, + onUnreadable?: (descriptor: ResourceDescriptor) => void, + prepareJson?: PrepareJson +): Promise<(workspace: string | undefined) => NamedValueNames> { + const displayNameOf = (json: Record | undefined) => + (json?.properties as Record | undefined)?.displayName; + const entries: Array = []; + for (const descriptor of available.filter((d) => d.type === ResourceType.NamedValue)) { + let json: Record | undefined; + try { + json = await readResource(descriptor); + } catch (error) { + logger.warn(`Unable to read ${buildResourceLabel(descriptor)} to match {{references}} by display name: ${String(error)}`); + onUnreadable?.(descriptor); + } + entries.push({ + name: descriptor.nameParts[0] ?? '', + displayName: displayNameOf(json && prepareJson ? prepareJson(descriptor, json) : json), + sourceDisplayName: prepareJson ? displayNameOf(json) : undefined, + workspace: descriptor.workspace, + }); + } + return namedValueNamesByScope(entries); +} + /** * Find API or Product targets referenced by a subscription payload. * diff --git a/tests/unit/services/extract-service.test.ts b/tests/unit/services/extract-service.test.ts index 090b8209..a4bcbde5 100644 --- a/tests/unit/services/extract-service.test.ts +++ b/tests/unit/services/extract-service.test.ts @@ -765,6 +765,9 @@ describe('extract-service', () => { }, }, ], + [ResourceType.NamedValue]: [ + { name: 'shared-secret', properties: { displayName: 'shared-secret', secret: true } }, + ], }); client.getResource = vi.fn().mockImplementation(async (_ctx, descriptor) => { if (descriptor.type === ResourceType.Backend && descriptor.nameParts[0] === 'member') { diff --git a/tests/unit/services/named-value-names.test.ts b/tests/unit/services/named-value-names.test.ts new file mode 100644 index 00000000..c903c980 --- /dev/null +++ b/tests/unit/services/named-value-names.test.ts @@ -0,0 +1,56 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +/** + * Unit tests for the named value reference rules + */ + +import { describe, it, expect } from 'vitest'; +import { NamedValueNames, namedValueNamesByScope } from '../../../src/services/named-value-names.js'; + +describe('NamedValueNames', () => { + it('should match a reference by display name, ignoring case', () => { + const names = new NamedValueNames([{ name: 'nv-1', displayName: 'Backend_Id' }]); + expect(names.resolve('backend_id')).toBe('nv-1'); + }); + + it('should not match a resource name, as APIM does not', () => { + const names = new NamedValueNames([{ name: 'nv-1', displayName: 'Something' }]); + expect(names.resolve('nv-1')).toBeUndefined(); + }); + + it("should match another named value's display name, not a resource name", () => { + const names = new NamedValueNames([ + { name: 'one', displayName: 'two' }, + { name: 'two', displayName: 'three' }, + ]); + expect(names.resolve('two')).toBe('one'); + expect(names.resolve('three')).toBe('two'); + }); + + it('should match a source display name an override changes, with published display names winning', () => { + const names = new NamedValueNames([ + { name: 'key', displayName: 'prod-key', sourceDisplayName: 'key' }, + { name: 'other', displayName: 'key', sourceDisplayName: 'other' }, + ]); + expect(names.resolve('prod-key')).toBe('key'); + expect(names.resolve('key')).toBe('other'); + expect(names.resolve('other')).toBe('other'); + }); + + it('should not match a named value whose display name is unknown', () => { + const names = new NamedValueNames([{ name: 'unread' }]); + expect(names.resolve('unread')).toBeUndefined(); + }); +}); + +describe('namedValueNamesByScope', () => { + it('should match a reference only among the named values of its own scope', () => { + const names = namedValueNamesByScope([ + { name: 'service-nv', displayName: 'key' }, + { name: 'team-nv', displayName: 'key', workspace: 'Team-A' }, + ]); + expect(names(undefined).resolve('key')).toBe('service-nv'); + expect(names('team-a').resolve('key')).toBe('team-nv'); + expect(names('team-b').resolve('key')).toBeUndefined(); + }); +}); diff --git a/tests/unit/services/publish-service.test.ts b/tests/unit/services/publish-service.test.ts index 6bcac061..f1be55c8 100644 --- a/tests/unit/services/publish-service.test.ts +++ b/tests/unit/services/publish-service.test.ts @@ -8,7 +8,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { ResourceType } from '../../../src/models/resource-types.js'; import { ResourceDescriptor, ApimServiceContext } from '../../../src/models/types.js'; import { PublishConfig } from '../../../src/models/config.js'; -import { LogLevel } from '../../../src/lib/logger.js'; +import { LogLevel, logger } from '../../../src/lib/logger.js'; import { recordRetry, withRetryResource } from '../../../src/lib/retry-tracker.js'; import { getResourceDescriptorKey } from '../../../src/lib/resource-path.js'; @@ -231,6 +231,70 @@ describe('publish-service', () => { )).toBe(true); }); + describe('with a {{reference}} in a policy fragment the filter selects', () => { + // `one` has the display name `two`; `two` has `three`. + const publishFragment = async ( + fragment: string, + overrides?: PublishConfig['overrides'], + readNamedValue?: (name: string) => Record + ) => { + const client = createMockClient(); + const store = createMockStore([ + { type: ResourceType.PolicyFragment, nameParts: ['auth'] }, + { type: ResourceType.NamedValue, nameParts: ['one'] }, + { type: ResourceType.NamedValue, nameParts: ['two'] }, + ]); + store.readResource.mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => { + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: fragment } }; + } + const name = descriptor.nameParts[0] ?? ''; + return readNamedValue?.(name) ?? { properties: { displayName: name === 'one' ? 'two' : 'three', value: 'x' } }; + }); + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter: { policyFragments: ['auth'], namedValues: [] }, + overrides, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + return client.putResource.mock.calls + .filter((call) => call[1].type === ResourceType.NamedValue) + .map((call) => call[1].nameParts[0]); + }; + + it('should include the named value with that display name, in any case, not one with that resource name', async () => { + expect(await publishFragment('{{TWO}}')).toEqual(['one']); + }); + + it('should not include a named value a reference names only by its resource name', async () => { + expect(await publishFragment('{{one}}')).toEqual([]); + }); + + it('should match the display name an override gives a named value, and the one in the source', async () => { + const overrides = { namedValues: { one: { properties: { displayName: 'Prod_Two' } } } }; + expect(await publishFragment('{{prod_two}}', overrides)).toEqual(['one']); + expect(await publishFragment('{{two}}', overrides)).toEqual(['one']); + }); + + it('should warn and still match the other named values when one cannot be read', async () => { + const warn = vi.spyOn(logger, 'warn').mockImplementation(() => undefined); + let reads = 0; + const puts = await publishFragment('{{three}}', undefined, (name) => { + if (name === 'one' && ++reads === 1) { + throw new Error('Unexpected token in JSON'); + } + return { properties: { displayName: name === 'one' ? 'two' : 'three', value: 'x' } }; + }); + + expect(warn).toHaveBeenCalledWith(expect.stringContaining('to match {{references}} by display name')); + expect(puts).toEqual(['two']); + warn.mockRestore(); + }); + }); + it('should include every dependency type supported by extract', async () => { const resources: ResourceDescriptor[] = [ { type: ResourceType.Api, nameParts: ['orders'] }, @@ -251,7 +315,9 @@ describe('publish-service', () => { '/subscriptions/s/resourceGroups/r/providers/Microsoft.ApiManagement/service/a/apiVersionSets/orders-v1', }, } - : { name: descriptor.nameParts[0] ?? '', properties: {} } + : descriptor.type === ResourceType.NamedValue + ? { name: descriptor.nameParts[0] ?? '', properties: { displayName: descriptor.nameParts[0] } } + : { name: descriptor.nameParts[0] ?? '', properties: {} } ); store.readContent.mockImplementation(async (_sourceDir, descriptor) => descriptor.type === ResourceType.ApiPolicy diff --git a/tests/unit/services/transitive-extractor.test.ts b/tests/unit/services/transitive-extractor.test.ts index 31ead3d4..a974954b 100644 --- a/tests/unit/services/transitive-extractor.test.ts +++ b/tests/unit/services/transitive-extractor.test.ts @@ -8,6 +8,7 @@ import { describe, it, expect, vi } from 'vitest'; import { ResourceType } from '../../../src/models/resource-types.js'; import { ApimServiceContext, ResourceDescriptor } from '../../../src/models/types.js'; import { extractTransitiveDependencies } from '../../../src/services/transitive-extractor.js'; +import { logger } from '../../../src/lib/logger.js'; const testContext: ApimServiceContext = { subscriptionId: 'sub-1', @@ -90,24 +91,47 @@ describe('transitive-extractor', () => { ); }); - it('should fall back to the reference as resource name when no displayName matches', async () => { + it('should not follow a reference that is only a resource name, as APIM does not', async () => { + const warn = vi.spyOn(logger, 'warn').mockImplementation(() => undefined); const client = createMockClient(); - client.listResources = namedValueList({ name: 'other', displayName: 'other' }); - client.getResource.mockImplementation(async (_ctx, descriptor: ResourceDescriptor) => { - if (descriptor.type === ResourceType.NamedValue && descriptor.nameParts[0] === 'plain-ref') { - return { name: 'plain-ref', properties: { displayName: 'plain-ref' } }; - } - return undefined; - }); - const store = createMockStore(); + client.listResources = namedValueList({ name: 'plain-ref', displayName: 'Plain_Ref' }); + client.getResource.mockResolvedValue({ name: 'plain-ref', properties: { displayName: 'Plain_Ref' } }); const result = await extractTransitiveDependencies( - client, store, testContext, '/output', policiesWith('plain-ref'), new Map(), [], [] + client, createMockStore(), testContext, '/output', policiesWith('plain-ref'), new Map(), [], [] ); expect(result.errorCount).toBe(0); + expect(result.extractedDescriptors).toEqual([]); + expect(client.getResource).not.toHaveBeenCalled(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('{{plain-ref}}')); + warn.mockRestore(); + }); + + it('should match a display name ignoring case', async () => { + const client = createMockClient(); + client.listResources = namedValueList({ name: 'foo', displayName: 'Something' }); + client.getResource.mockResolvedValue({ properties: { value: 'x' } }); + + await extractTransitiveDependencies(client, createMockStore(), testContext, '/out', policiesWith('SOMETHING'), new Map(), [], []); + + expect(client.getResource.mock.calls.map((call) => call[1].nameParts)).toEqual([['foo']]); + }); + + it("should extract a named value whose display name is an extracted named value's name", async () => { + // `one` has the display name `two`, so {{two}} is `one`, not the + // selected `two`. + const client = createMockClient(); + client.listResources = namedValueList({ name: 'one', displayName: 'two' }, { name: 'two', displayName: 'three' }); + client.getResource.mockResolvedValue({ name: 'one', properties: { displayName: 'two' } }); + const two = { type: ResourceType.NamedValue, nameParts: ['two'] }; + + const result = await extractTransitiveDependencies( + client, createMockStore(), testContext, '/output', policiesWith('two'), new Map(), [], [two] + ); + expect(result.extractedDescriptors).toEqual([ - expect.objectContaining({ type: ResourceType.NamedValue, nameParts: ['plain-ref'] }), + expect.objectContaining({ type: ResourceType.NamedValue, nameParts: ['one'] }), ]); }); @@ -234,6 +258,7 @@ describe('transitive-extractor', () => { it('should count a missing dependency as an error', async () => { const client = createMockClient(); + client.listResources = namedValueList({ name: 'missing', displayName: 'missing' }); const store = createMockStore(); const result = await extractTransitiveDependencies( diff --git a/tests/unit/services/workspace-extractor.test.ts b/tests/unit/services/workspace-extractor.test.ts index e24f1b1f..a3a04b02 100644 --- a/tests/unit/services/workspace-extractor.test.ts +++ b/tests/unit/services/workspace-extractor.test.ts @@ -485,6 +485,9 @@ describe('workspace-extractor', () => { }, }; } + if (type === ResourceType.NamedValue) { + yield { name: 'workspace-secret', properties: { displayName: 'workspace-secret', secret: true } }; + } if (type === ResourceType.Backend) { yield { name: 'workspace-pool', From 11ea0c271e1b4a2ff6e4fa43450aee481f4fce82 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 10:42:57 +1100 Subject: [PATCH 02/18] fix: ignore references in XML comments and Liquid output, and follow the named values APIM substitutes in Liquid bodies Closes #276 Refs #305 --- src/services/transitive-extractor.ts | 75 +++++++++++++-- src/services/transitive-resolver.ts | 93 ++++++++++++++++++- tests/unit/services/publish-service.test.ts | 35 +++++++ .../services/transitive-extractor.test.ts | 85 +++++++++++++++++ .../unit/services/transitive-resolver.test.ts | 42 +++++++++ 5 files changed, 320 insertions(+), 10 deletions(-) diff --git a/src/services/transitive-extractor.ts b/src/services/transitive-extractor.ts index 82ccfa9d..b08f7480 100644 --- a/src/services/transitive-extractor.ts +++ b/src/services/transitive-extractor.ts @@ -10,7 +10,7 @@ import { buildResourceLabel } from '../lib/resource-uri.js'; import { logger } from '../lib/logger.js'; import { runParallel } from '../lib/parallel-runner.js'; import { redactSecrets } from './secret-redactor.js'; -import { findTransitiveDependencies } from './transitive-resolver.js'; +import { findTransitiveDependencies, scanLiquidNamedValueCandidates } from './transitive-resolver.js'; import { NamedValueNames, type NamedValueEntry } from './named-value-names.js'; const DEFAULT_CONCURRENCY = 5; @@ -108,6 +108,42 @@ async function resolveNamedValueDescriptor( return { ...dep, nameParts: [name] }; } +/** The `{{name}}`s in a policy fragment's Liquid bodies (scanLiquidNamedValueCandidates). */ +function fragmentLiquidCandidates({ descriptor, json }: TransitiveResourceArtifact): string[] { + if (descriptor.type !== ResourceType.PolicyFragment) { + return []; + } + const properties = json.properties as Record | undefined; + return [properties?.value, properties?.policyContent] + .filter((text): text is string => typeof text === 'string') + .flatMap(scanLiquidNamedValueCandidates); +} + +/** + * The named values Liquid bodies reference: each `{{name}}` candidate that a + * named value in this scope has as its display name. The others are Liquid, + * skipped without a warning. `listed` is false when the named values couldn't + * be listed, so a candidate that is one may be missed. + */ +async function liquidNamedValueDependencies( + client: IApimClient, + context: ApimServiceContext, + candidates: ReadonlySet, + workspace: string | undefined, + index: NamedValueNameIndex +): Promise<{ dependencies: ResourceDescriptor[]; listed: boolean }> { + if (candidates.size === 0) { + return { dependencies: [], listed: true }; + } + const { names, listed } = await namedValueNames(client, context, index); + return { + dependencies: [...candidates] + .filter((candidate) => names.resolve(candidate) !== undefined) + .map((candidate) => ({ type: ResourceType.NamedValue, nameParts: [candidate], workspace })), + listed, + }; +} + export async function extractTransitiveDependencies( client: IApimClient, store: IArtifactStore, @@ -133,14 +169,39 @@ export async function extractTransitiveDependencies( serviceContext && dep.workspace !== workspace ? serviceContext : context; let errorCount = 0; let foundDependencies = false; + let reportedUnlisted = false; + + // The {{name}}s in Liquid bodies: the policies' found once, and each + // fragment's as it is extracted. + const liquidCandidates = new Set([...policies.values()].flatMap(scanLiquidNamedValueCandidates)); + let resourcesScanned = 0; while (true) { - const found = findTransitiveDependencies( - policies, - apis, - workspace, - resources - ).filter((dep) => !attempted.has(getResourceDescriptorKey(dep))); + for (; resourcesScanned < resources.length; resourcesScanned++) { + fragmentLiquidCandidates(resources[resourcesScanned]).forEach((name) => liquidCandidates.add(name)); + } + const liquid = await liquidNamedValueDependencies(client, context, liquidCandidates, workspace, namedValueIndex); + if (!liquid.listed && !reportedUnlisted) { + // A named value used only in a Liquid body may be missing from the + // output, so the extract is incomplete. + reportedUnlisted = true; + errorCount++; + logger.warn('Unable to tell which {{...}} in Liquid bodies are named values, since the named values could not be listed'); + } + // Each once: a policy can reference a named value inside and outside a + // Liquid body. + const seen = new Set(); + const found = [ + ...findTransitiveDependencies(policies, apis, workspace, resources), + ...liquid.dependencies, + ].filter((dep) => { + const key = getResourceDescriptorKey(dep); + if (attempted.has(key) || seen.has(key)) { + return false; + } + seen.add(key); + return true; + }); const newDeps: ResourceDescriptor[] = []; for (const dep of found) { if (dep.type !== ResourceType.NamedValue) { diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index c08d592d..c0edad79 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -23,6 +23,69 @@ const NAMED_VALUE_PATTERN = /\{\{([^}]+)\}\}/g; const BACKEND_PATTERN = /`, + * lets a value contain `>` (as policy expressions can). + */ +const XML_ATTRIBUTE = String.raw`\s+[\w:.-]+\s*=\s*(?:"[^"]*"|'[^']*')`; + +/** + * XML comments: references inside them are not in effect. Only a closed + * comment is removed, so a stray `/g; + +/** + * The body of a `` element (capture group 3): its + * `{{ ... }}` are mostly Liquid output expressions, not named value + * references; see scanLiquidNamedValueCandidates. A self-closing set-body has + * no body and is not matched. + */ +const LIQUID_BODY_PATTERN = new RegExp( + `()` + + `([\\s\\S]*?)(<\\/set-body\\s*>)`, + 'gi' +); + +/** + * A `{{name}}` whose name has only the characters a named value's name can: + * letters, digits, '.', '-' and '_'. Spaces or a filter (`|`) make it Liquid. + */ +const LIQUID_NAMED_VALUE_PATTERN = /\{\{([A-Za-z0-9._-]+)\}\}/g; + +/** + * Policy XML with the parts that cannot reference other resources removed: + * XML comments, and the bodies of Liquid templates. + */ +function stripNonReferenceContent(policyXml: string): string { + return policyXml + .replace(XML_COMMENT_PATTERN, '') + .replace(LIQUID_BODY_PATTERN, (_match, open: string, _quote: string, _body: string, close: string) => `${open}${close}`); +} + +/** + * The names a `` body may reference named values + * by: each `{{name}}` there whose name a named value could have. APIM + * substitutes a named value inside a Liquid body when one has that display + * name, and leaves every other `{{ ... }}` (with spaces, a filter, or no such + * named value, like `{{body.id}}`) to Liquid. So each is a reference only when + * a named value with that display name exists; callers check before following + * one, and skip the rest without an error. XML comments are ignored. + */ +export function scanLiquidNamedValueCandidates(policyXml: string): string[] { + const names = new Set(); + for (const body of policyXml.replace(XML_COMMENT_PATTERN, '').matchAll(LIQUID_BODY_PATTERN)) { + for (const match of (body[3] ?? '').matchAll(LIQUID_NAMED_VALUE_PATTERN)) { + if (match[1]) { + names.add(match[1]); + } + } + } + return [...names]; +} + /** * Represents a discovered transitive dependency. */ @@ -46,12 +109,19 @@ const POLICY_RESOURCE_TYPES = new Set([ * - Named values: {{namedValueName}} syntax * - Backends: * - Policy fragments: + * + * References inside XML comments, and `{{ ... }}` inside a + * `` body, are ignored: most of those are Liquid + * output. The bare `{{name}}`s there that APIM substitutes when a named value + * has that display name are found by scanLiquidNamedValueCandidates, for + * callers to check. */ export function scanPolicyReferences(policyXml: string): TransitiveDependency[] { const dependencies: TransitiveDependency[] = []; + const scannable = stripNonReferenceContent(policyXml); // Named value references - for (const match of policyXml.matchAll(NAMED_VALUE_PATTERN)) { + for (const match of scannable.matchAll(NAMED_VALUE_PATTERN)) { if (match[1]) { dependencies.push({ type: ResourceType.NamedValue, @@ -61,7 +131,7 @@ export function scanPolicyReferences(policyXml: string): TransitiveDependency[] } // Backend references - for (const match of policyXml.matchAll(BACKEND_PATTERN)) { + for (const match of scannable.matchAll(BACKEND_PATTERN)) { if (match[1]) { dependencies.push({ type: ResourceType.Backend, @@ -71,7 +141,7 @@ export function scanPolicyReferences(policyXml: string): TransitiveDependency[] } // Policy fragment references - for (const match of policyXml.matchAll(FRAGMENT_PATTERN)) { + for (const match of scannable.matchAll(FRAGMENT_PATTERN)) { if (match[1]) { dependencies.push({ type: ResourceType.PolicyFragment, @@ -310,6 +380,23 @@ export async function scanArtifactReferences( ) ); + // A {{name}} in a Liquid body is a reference only when a named value has + // that display name; one that resolves to no artifact is simply not followed. + const fragment = descriptor.type === ResourceType.PolicyFragment + ? (json?.properties as Record | undefined) + : undefined; + for (const text of [...policies.values(), fragment?.value, fragment?.policyContent]) { + if (typeof text === 'string') { + references.push( + ...scanLiquidNamedValueCandidates(text).map((name) => ({ + type: ResourceType.NamedValue, + nameParts: [name], + workspace: descriptor.workspace, + })) + ); + } + } + return deduplicateDescriptors(references); } diff --git a/tests/unit/services/publish-service.test.ts b/tests/unit/services/publish-service.test.ts index f1be55c8..eb3915c9 100644 --- a/tests/unit/services/publish-service.test.ts +++ b/tests/unit/services/publish-service.test.ts @@ -295,6 +295,41 @@ describe('publish-service', () => { }); }); + it('should include a named value a Liquid body references, and nothing for Liquid output', async () => { + const resources = [ + { type: ResourceType.Api, nameParts: ['orders'] }, + { type: ResourceType.ApiPolicy, nameParts: ['orders'] }, + { type: ResourceType.NamedValue, nameParts: ['backend-id'] }, + { type: ResourceType.NamedValue, nameParts: ['unused'] }, + ]; + const client = createMockClient(); + const store = createMockStore(resources); + store.readContent.mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.ApiPolicy + ? { content: 'nv=[{{Backend_Id}}] liquid=[{{body.id}}]' } + : undefined + ); + store.readResource.mockImplementation(async (_sourceDir, descriptor) => + descriptor.type === ResourceType.NamedValue + ? { properties: { displayName: descriptor.nameParts[0] === 'backend-id' ? 'Backend_Id' : 'unused', value: 'x' } } + : { properties: {} } + ); + + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter: { apis: ['orders'], namedValues: [] }, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + + const namedValuePuts = client.putResource.mock.calls + .filter((call) => call[1].type === ResourceType.NamedValue) + .map((call) => call[1].nameParts[0]); + expect(namedValuePuts).toEqual(['backend-id']); + }); + it('should include every dependency type supported by extract', async () => { const resources: ResourceDescriptor[] = [ { type: ResourceType.Api, nameParts: ['orders'] }, diff --git a/tests/unit/services/transitive-extractor.test.ts b/tests/unit/services/transitive-extractor.test.ts index a974954b..2b39d4ba 100644 --- a/tests/unit/services/transitive-extractor.test.ts +++ b/tests/unit/services/transitive-extractor.test.ts @@ -270,3 +270,88 @@ describe('transitive-extractor', () => { }); }); }); + +describe('extractTransitiveDependencies with Liquid bodies', () => { + it('should extract a named value a Liquid body references and skip Liquid output without an error', async () => { + const warn = vi.spyOn(logger, 'warn').mockImplementation(() => undefined); + const client = createMockClient(); + client.listResources = namedValueList({ name: 'backend-id', displayName: 'demo-backend-id' }); + client.getResource.mockImplementation(async (_ctx: ApimServiceContext, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.NamedValue && descriptor.nameParts[0] === 'backend-id' + ? { name: 'backend-id', properties: { displayName: 'demo-backend-id', value: 'function-backend' } } + : undefined + ); + const store = createMockStore(); + const policies = new Map([[ + 'api:liquid-test:policy:0', + 'nv=[{{demo-backend-id}}] liquid=[{{body.id}}] spaced=[{{ context.Request.Url }}]', + ]]); + + const result = await extractTransitiveDependencies(client, store, testContext, '/out', policies, new Map(), [], []); + + const fetched = client.getResource.mock.calls.map((call) => `${call[1].type}:${call[1].nameParts.join('/')}`); + expect(fetched).toEqual(['NamedValue:backend-id']); + expect(result.errorCount).toBe(0); + expect(result.extractedDescriptors.map((d) => d.nameParts[0])).toEqual(['backend-id']); + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it('should report an error when the named values for a Liquid body cannot be listed', async () => { + const warn = vi.spyOn(logger, 'warn').mockImplementation(() => undefined); + const client = createMockClient(); + // eslint-disable-next-line require-yield + client.listResources = async function* () { + throw new Error('HTTP 503'); + }; + const policies = new Map([['api:orders:policy:0', '{{demo-backend-id}}']]); + + try { + const result = await extractTransitiveDependencies(client, createMockStore(), testContext, '/out', policies, new Map(), [], []); + + expect(result.errorCount).toBe(1); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Unable to tell which {{...}} in Liquid bodies are named values')); + } finally { + warn.mockRestore(); + } + }); + + it("should follow a Liquid {{name}} in a fragment extracted in a later round", async () => { + const client = createMockClient(); + client.listResources = namedValueList({ name: 'frag-nv', displayName: 'frag-nv' }); + client.getResource.mockImplementation(async (_ctx: ApimServiceContext, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.PolicyFragment + ? { properties: { value: '{{frag-nv}} {{body.id}}' } } + : { properties: { value: 'x' } } + ); + const policies = new Map([['api:orders:policy:0', '']]); + + const result = await extractTransitiveDependencies(client, createMockStore(), testContext, '/out', policies, new Map(), [], []); + + expect(client.getResource.mock.calls.map((call) => `${call[1].type}:${call[1].nameParts.join('/')}`)).toEqual([ + 'PolicyFragment:route', + 'NamedValue:frag-nv', + ]); + expect(result.errorCount).toBe(0); + }); + + it('should look a Liquid {{name}} up in the workspace being extracted', async () => { + const client = createMockClient(); + const workspaceContext = { ...testContext, baseUrl: `${testContext.baseUrl}/workspaces/team-a` }; + client.listResources = vi.fn(async function* (ctx: ApimServiceContext, type: ResourceType) { + if (type === ResourceType.NamedValue && ctx.baseUrl === workspaceContext.baseUrl) { + yield { name: 'ws-key', properties: { displayName: 'ws-key' } }; + } + }); + client.getResource.mockResolvedValue({ properties: { value: 'x' } }); + const policies = new Map([['api:orders:policy:0', '{{ws-key}}']]); + + await extractTransitiveDependencies( + client, createMockStore(), workspaceContext, '/out', policies, new Map(), [], [], 'team-a', testContext + ); + + expect(client.getResource.mock.calls.map((call) => call[1])).toEqual([ + { type: ResourceType.NamedValue, nameParts: ['ws-key'], workspace: 'team-a' }, + ]); + }); +}); diff --git a/tests/unit/services/transitive-resolver.test.ts b/tests/unit/services/transitive-resolver.test.ts index 8d05947e..802bb017 100644 --- a/tests/unit/services/transitive-resolver.test.ts +++ b/tests/unit/services/transitive-resolver.test.ts @@ -8,6 +8,7 @@ import { describe, it, expect, vi } from 'vitest'; import { ResourceType } from '../../../src/models/resource-types.js'; import { FilterConfig } from '../../../src/models/config.js'; import { + scanLiquidNamedValueCandidates, scanPolicyReferences, scanApiVersionSetReference, resolveTransitiveDependencies, @@ -82,6 +83,47 @@ describe('transitive-resolver', () => { const refs = scanPolicyReferences(policy); expect(refs[0]?.name).toBe('my-secret'); }); + + it('should ignore references inside XML comments (#305)', () => { + const refs = scanPolicyReferences( + '' + + '' + ); + + expect(refs).toEqual([{ type: ResourceType.Backend, name: 'live-backend' }]); + }); + it('should not let a stray " + {{outside-liquid}} + `; + + expect(scanLiquidNamedValueCandidates(policy)).toEqual(['demo-backend-id', 'body.id']); + }); + it('should ignore Liquid output in set-body templates but keep named values elsewhere (#276)', () => { + const refs = scanPolicyReferences(` + {{real-value}} + {{context.Request.MatchedParameters["id"]}} + {{body.envelope.body.fault.faultcode}} + + {{after-liquid}} + {{not-liquid}}`); + + expect(refs).toEqual([ + { type: ResourceType.NamedValue, name: 'real-value' }, + { type: ResourceType.NamedValue, name: 'after-liquid' }, + { type: ResourceType.NamedValue, name: 'not-liquid' }, + ]); + }); }); describe('scanApiVersionSetReference', () => { From 2405277fc6ddb756e96936f0301b8761c70c9cdd Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 10:44:52 +1100 Subject: [PATCH 03/18] fix: prefix named value references by the named value they resolve to, across the whole source --- src/models/config.ts | 22 +-- src/services/policy-ref-rewriter.ts | 59 ++++---- src/services/publish-service.ts | 11 +- src/services/resource-publisher.ts | 135 +++++++++++++----- src/services/transitive-resolver.ts | 29 ++++ .../unit/services/policy-ref-rewriter.test.ts | 96 ++++++++++--- tests/unit/services/publish-service.test.ts | 34 ++++- .../resource-publisher.env-mapping.test.ts | 89 +++++++++--- 8 files changed, 359 insertions(+), 116 deletions(-) diff --git a/src/models/config.ts b/src/models/config.ts index 1498eef3..39f5d01a 100644 --- a/src/models/config.ts +++ b/src/models/config.ts @@ -5,6 +5,7 @@ * ExtractConfig, FilterConfig, PublishConfig, OverrideConfig, InitConfig */ +import type { ResourceType } from './resource-types.js'; import { ApimServiceContext } from './types.js'; import { LogLevel } from '../lib/logger.js'; import type { EnvMapping } from '../services/env-mapper.js'; @@ -74,16 +75,21 @@ export interface FilterConfig { } /** - * Sets of canonical artifact names used to gate policy XML reference rewriting. - * Populated once per publish from the full artifact descriptor list. + * What publish knows about the source to rewrite references from canonical to + * deployed names. Built once per publish from every artifact in the source, + * whatever the filter selects. */ export interface KnownArtifactSets { - /** Canonical NamedValue resource names */ - namedValues: ReadonlySet; - /** Canonical PolicyFragment resource names */ - fragments: ReadonlySet; - /** Canonical Backend resource names */ - backends: ReadonlySet; + /** Whether the source has a resource of `type` named `name` (any case) in `workspace`. */ + has(type: ResourceType, name: string, workspace?: string): boolean; + /** + * The display name the named value a `{{reference}}` in `workspace` names + * (NamedValueNames) is deployed with: an explicit displayName override as + * written, otherwise with the environment's affix when it applies to named + * values. Undefined when no named value in the source has the reference as + * its display name. + */ + deployedNamedValueReference(reference: string, workspace?: string): string | undefined; } export interface PublishConfig { diff --git a/src/services/policy-ref-rewriter.ts b/src/services/policy-ref-rewriter.ts index 25e2119a..12fadbb0 100644 --- a/src/services/policy-ref-rewriter.ts +++ b/src/services/policy-ref-rewriter.ts @@ -15,27 +15,22 @@ * token, which is extremely unlikely in practice. * * Known-set filter safety: - * Only tokens/attribute values whose exact text is present in the - * corresponding known set are rewritten. This naturally excludes APIM - * runtime context references such as {{context.request.headers.foo}} - * because those strings will never appear as artifact resource names. + * Only tokens that are the display name of a named value in the source, and + * attribute values that name a resource in the source, are rewritten. This + * naturally excludes APIM runtime context references such as + * {{context.request.headers.foo}}, which no named value has as its name. */ import { ResourceType } from '../models/resource-types.js'; import type { EnvMapping } from './env-mapper.js'; import { toDeployedName } from './env-mapper.js'; import type { KnownArtifactSets } from '../models/config.js'; +import { replaceNamedValueReferences } from './transitive-resolver.js'; // --------------------------------------------------------------------------- // Regex patterns // --------------------------------------------------------------------------- -/** - * Matches {{token}} with optional whitespace inside the braces. - * Group 1 = token name (no whitespace). - */ -const NV_PATTERN = /\{\{\s*([A-Za-z0-9._-]+)\s*\}\}/g; - /** * Matches the fragment-id attribute on an element. * Groups: 1 = tag prefix through `=`, 2 = opening quote, 3 = id value, @@ -61,44 +56,50 @@ export type { KnownArtifactSets } from '../models/config.js'; * deployed names. * * Handled reference styles: - * 1. `{{tokenName}}` — NamedValue reference. Only rewritten when - * tokenName ∈ knownArtifactSets.namedValues AND - * ResourceType.NamedValue ∈ mapping.appliesTo. - * 2. `fragment-id="name"` on . Only rewritten when - * name ∈ knownArtifactSets.fragments AND + * 1. `{{token}}` — a named value reference where APIM substitutes one + * (replaceNamedValueReferences: inside a Liquid body, only a bare + * `{{name}}`), resolved as APIM resolves it (by + * display name, ignoring case, in the policy's scope) and rewritten to the + * display name that named value is deployed with + * (KnownArtifactSets.deployedNamedValueReference). Rewritten even without + * a mapping when an explicit displayName override changes it. + * 2. `fragment-id="name"` on . Only rewritten when the + * source has that fragment in the policy's scope AND * ResourceType.PolicyFragment ∈ mapping.appliesTo. - * 3. `backend-id="name"` on . Only rewritten when - * name ∈ knownArtifactSets.backends AND + * 3. `backend-id="name"` on . Only rewritten when the + * source has that backend in the policy's scope AND * ResourceType.Backend ∈ mapping.appliesTo. * - * Returns the input unchanged if `mapping` is undefined or no matches apply. + * @param workspace - The policy's workspace, if any: references name + * resources in its own scope */ export function rewritePolicyRefs( xml: string, mapping: EnvMapping | undefined, known: KnownArtifactSets, + workspace?: string, ): string { - if (mapping === undefined) return xml; + // A token names the named value with that display name in the policy's + // scope, and becomes the display name it is deployed with: changed by an + // explicit displayName override even without a mapping. APIM matches + // display names ignoring case, so one that differs only in case is kept. + let result = replaceNamedValueReferences(xml, (name, match) => { + const deployed = known.deployedNamedValueReference(name, workspace); + return deployed === undefined || deployed.toLowerCase() === name.toLowerCase() ? match : `{{${deployed}}}`; + }); - let result = xml; - - if (mapping.appliesTo.has(ResourceType.NamedValue)) { - result = result.replace(NV_PATTERN, (match, name: string) => { - if (!known.namedValues.has(name)) return match; - return `{{${toDeployedName(name, ResourceType.NamedValue, mapping)}}}`; - }); - } + if (mapping === undefined) return result; if (mapping.appliesTo.has(ResourceType.PolicyFragment)) { result = result.replace(FRAGMENT_PATTERN, (match, prefix: string, quote: string, id: string) => { - if (!known.fragments.has(id)) return match; + if (!known.has(ResourceType.PolicyFragment, id, workspace)) return match; return `${prefix}${quote}${toDeployedName(id, ResourceType.PolicyFragment, mapping)}${quote}`; }); } if (mapping.appliesTo.has(ResourceType.Backend)) { result = result.replace(BACKEND_PATTERN, (match, prefix: string, quote: string, id: string) => { - if (!known.backends.has(id)) return match; + if (!known.has(ResourceType.Backend, id, workspace)) return match; return `${prefix}${quote}${toDeployedName(id, ResourceType.Backend, mapping)}${quote}`; }); } diff --git a/src/services/publish-service.ts b/src/services/publish-service.ts index 77209d5c..5ae6de77 100644 --- a/src/services/publish-service.ts +++ b/src/services/publish-service.ts @@ -149,8 +149,15 @@ async function runPublishPipeline( // Step 1b: Validate env mapping and store it on config for downstream use validateAndBuildEnvMapping(config.overrides, allDescriptors, config); - // Step 1c: Build known artifact sets for policy ref rewriting - config.knownArtifactSets = buildKnownArtifactSets(allDescriptors); + // Step 1c: What publish knows about the whole source to rewrite references + // (whatever the filter or incremental diff selects, a reference can name + // any artifact in it). + config.knownArtifactSets = await buildKnownArtifactSets( + store, + config, + await sourceReferences.available(), + sourceReferences.readNamedValue + ); logger.debug( `Publishing ${targetDescriptors.length} resources (dry-run: ${config.dryRun})` diff --git a/src/services/resource-publisher.ts b/src/services/resource-publisher.ts index fe9f3729..532beae9 100644 --- a/src/services/resource-publisher.ts +++ b/src/services/resource-publisher.ts @@ -18,6 +18,8 @@ import type { PublishConfig } from '../models/config.js'; import { ResourceType, RESOURCE_TYPE_METADATA, MANAGED_GATEWAY_NAME } from '../models/resource-types.js'; import { applyOverrides, resolveOverrideSection } from './override-merger.js'; import { checkKeyVaultSecretAccess } from './keyvault-checker.js'; +import { namedValueNamesByScope } from './named-value-names.js'; +import { runParallel } from '../lib/parallel-runner.js'; import { getNamePart, getResourceDescriptorKey, @@ -61,22 +63,90 @@ export interface AssociationPublicationPlan extends PublishEligibility { } /** - * Build the known artifact name sets from a list of descriptors. - * Called once per publish run after determinePublishTargets. + * Build what publish knows about the source (KnownArtifactSets) from every + * artifact in it, with each named value read as it is published. Called once + * per publish run, once the environment mapping is known. */ -export function buildKnownArtifactSets(descriptors: ResourceDescriptor[]): KnownArtifactSets { - const namedValues = new Set(); - const fragments = new Set(); - const backends = new Set(); - for (const d of descriptors) { - if (d.nameParts.length > 0) { - const name = d.nameParts[0]; - if (d.type === ResourceType.NamedValue) namedValues.add(name); - else if (d.type === ResourceType.PolicyFragment) fragments.add(name); - else if (d.type === ResourceType.Backend) backends.add(name); - } +export async function buildKnownArtifactSets( + store: IArtifactStore, + config: PublishConfig, + descriptors: ReadonlyArray, + readNamedValue: (descriptor: ResourceDescriptor) => Promise | undefined> = + (descriptor) => store.readResource(config.sourceDir, descriptor) +): Promise { + const key = (type: ResourceType, name: string, workspace: string | undefined) => + `${type}|${(workspace ?? '').toLowerCase()}|${name.toLowerCase()}`; + const resources = new Set( + descriptors + .filter((descriptor) => descriptor.nameParts.length > 0) + .map((descriptor) => key(descriptor.type, getNamePart(descriptor.nameParts, 0), descriptor.workspace)) + ); + const displayNameOf = (json: Record | undefined) => + (json?.properties as Record | undefined)?.displayName; + const namedValues = descriptors.filter((descriptor) => descriptor.type === ResourceType.NamedValue); + const read = await runParallel( + namedValues.map((descriptor) => async () => { + const json = await readNamedValue(descriptor); + const published = json ? applyOverrides(descriptor, json, config.overrides) : undefined; + return { + name: getNamePart(descriptor.nameParts, 0), + workspace: descriptor.workspace, + displayName: displayNameOf(published), + sourceDisplayName: displayNameOf(json ?? undefined), + deployed: published ? deployedNamedValueDisplayName(descriptor, published, config) : undefined, + }; + }), + KNOWN_ARTIFACT_READ_CONCURRENCY + ); + // A named value that can't be read can't be matched by display name; + // publishing it reports the error. + const entries = read.flatMap((result) => (result.status === 'fulfilled' && result.value ? [result.value] : [])); + const deployed = new Map(entries.map((entry) => [key(ResourceType.NamedValue, entry.name, entry.workspace), entry.deployed])); + const names = namedValueNamesByScope(entries); + return { + has: (type, name, workspace) => resources.has(key(type, name, workspace)), + deployedNamedValueReference: (reference, workspace) => { + const name = names(workspace).resolve(reference); + return name === undefined ? undefined : deployed.get(key(ResourceType.NamedValue, name, workspace)); + }, + }; +} + +/** Named value artifacts read at once while building KnownArtifactSets. */ +const KNOWN_ARTIFACT_READ_CONCURRENCY = 5; + +/** + * KnownArtifactSets for a publish: the run's own, which publish-service builds + * once per run, or, for a resource published on its own, built from the + * source. + */ +async function knownArtifactSetsFor(store: IArtifactStore, config: PublishConfig): Promise { + return config.knownArtifactSets ?? buildKnownArtifactSets(store, config, await store.listResources(config.sourceDir)); +} + +/** + * The display name a named value is deployed with, from its JSON with + * overrides applied: an explicit displayName override as written, otherwise + * its display name (or, without one, its name) with the environment's affix + * when the affix applies to named values. APIM resolves {{references}} by + * display name, so references are rewritten to this. + */ +function deployedNamedValueDisplayName( + descriptor: ResourceDescriptor, + published: Record, + config: PublishConfig +): string | undefined { + const name = getNamePart(descriptor.nameParts, 0); + const displayName = (published.properties as Record | undefined)?.displayName; + const hasDisplayNameOverride = hasExplicitPropertyOverride( + name, + 'displayName', + resolveOverrideSection(config.overrides, 'namedValues', descriptor.workspace), + ); + if (!config.envMapping?.appliesTo.has(ResourceType.NamedValue) || hasDisplayNameOverride) { + return typeof displayName === 'string' ? displayName : undefined; } - return { namedValues, fragments, backends }; + return toDeployedName(typeof displayName === 'string' ? displayName : name, ResourceType.NamedValue, config.envMapping); } /** @@ -348,28 +418,13 @@ export async function publishResource( if (descriptor.type === ResourceType.NamedValue) { // Affix displayName when envMapping applies and user hasn't set an explicit override. // APIM resolves {{ref}} tokens by displayName, so the deployed displayName must - // match the affixed name used elsewhere (descriptor name, policy refs, etc.). + // match the one references are rewritten to (deployedNamedValueDisplayName). if (config.envMapping?.appliesTo.has(ResourceType.NamedValue) === true) { - const canonicalName = getNamePart(descriptor.nameParts, 0); - const hasDisplayNameOverride = hasExplicitPropertyOverride( - canonicalName, - 'displayName', - resolveOverrideSection(config.overrides, 'namedValues', descriptor.workspace), - ); - if (!hasDisplayNameOverride) { - const props = json.properties as Record | undefined; - const currentDisplayName = - typeof props?.displayName === 'string' ? props.displayName : canonicalName; - const deployedDisplayName = toDeployedName( - currentDisplayName, - ResourceType.NamedValue, - config.envMapping, - ); - json = { - ...json, - properties: { ...(props ?? {}), displayName: deployedDisplayName }, - }; - } + const props = json.properties as Record | undefined; + json = { + ...json, + properties: { ...(props ?? {}), displayName: deployedNamedValueDisplayName(descriptor, json, config) }, + }; } const props = json.properties as Record | undefined; @@ -537,10 +592,11 @@ export async function publishResource( json = applyApiPathPrefix(json, descriptor, config); // For PolicyFragment: rewrite cross-resource refs in properties.value (policy XML) - if (descriptor.type === ResourceType.PolicyFragment && config.envMapping && config.knownArtifactSets) { + if (descriptor.type === ResourceType.PolicyFragment) { const props = json.properties as Record | undefined; if (typeof props?.value === 'string') { - const rewrittenXml = rewritePolicyRefs(props.value, config.envMapping, config.knownArtifactSets); + const known = await knownArtifactSetsFor(store, config); + const rewrittenXml = rewritePolicyRefs(props.value, config.envMapping, known, descriptor.workspace); json = { ...json, properties: { ...props, value: rewrittenXml } }; } } @@ -1048,11 +1104,12 @@ async function publishPolicy( let mergedPayload = applyOverrides(descriptor, payload, config.overrides); // Rewrite policy XML references from canonical → deployed names - if (config.envMapping && config.knownArtifactSets) { + { const mProps = mergedPayload.properties as Record | undefined; const mValue = mProps?.value; if (typeof mValue === 'string') { - const rewrittenXml = rewritePolicyRefs(mValue, config.envMapping, config.knownArtifactSets); + const known = await knownArtifactSetsFor(store, config); + const rewrittenXml = rewritePolicyRefs(mValue, config.envMapping, known, descriptor.workspace); mergedPayload = { ...mergedPayload, properties: { ...(mergedPayload.properties as object), value: rewrittenXml }, diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index c0edad79..c19ee2d2 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -65,6 +65,35 @@ function stripNonReferenceContent(policyXml: string): string { .replace(LIQUID_BODY_PATTERN, (_match, open: string, _quote: string, _body: string, close: string) => `${open}${close}`); } +/** + * Replace each `{{reference}}` in policy XML the way APIM substitutes named + * values: any `{{ name }}` outside Liquid bodies, and inside a + * `` body only a bare `{{name}}` + * (scanLiquidNamedValueCandidates); the rest there is Liquid output. `replace` + * gets the trimmed name and the whole match, and returns the text to put in + * its place. + */ +export function replaceNamedValueReferences(policyXml: string, replace: (name: string, match: string) => string): string { + const replaceIn = (text: string, pattern: RegExp) => + text.replace(pattern, (match: string, name: string) => replace(name.trim(), match)); + // Liquid bodies are found as the scanner finds them, ignoring XML comments: + // on a copy with each comment blanked out, so offsets still match. + const uncommented = policyXml.replace(XML_COMMENT_PATTERN, (comment) => ' '.repeat(comment.length)); + let result = ''; + let last = 0; + for (const body of uncommented.matchAll(LIQUID_BODY_PATTERN)) { + const start = body.index ?? 0; + const [, open = '', , content = ''] = body; + const contentStart = start + open.length; + const end = start + body[0].length; + result += replaceIn(policyXml.slice(last, contentStart), NAMED_VALUE_PATTERN); + result += replaceIn(policyXml.slice(contentStart, contentStart + content.length), LIQUID_NAMED_VALUE_PATTERN); + result += policyXml.slice(contentStart + content.length, end); + last = end; + } + return result + replaceIn(policyXml.slice(last), NAMED_VALUE_PATTERN); +} + /** * The names a `` body may reference named values * by: each `{{name}}` there whose name a named value could have. APIM diff --git a/tests/unit/services/policy-ref-rewriter.test.ts b/tests/unit/services/policy-ref-rewriter.test.ts index 956131af..ec95974d 100644 --- a/tests/unit/services/policy-ref-rewriter.test.ts +++ b/tests/unit/services/policy-ref-rewriter.test.ts @@ -5,6 +5,7 @@ import { describe, it, expect } from 'vitest'; import { rewritePolicyRefs } from '../../../src/services/policy-ref-rewriter.js'; import type { KnownArtifactSets } from '../../../src/services/policy-ref-rewriter.js'; import type { EnvMapping } from '../../../src/services/env-mapper.js'; +import { toDeployedName } from '../../../src/services/env-mapper.js'; import { ResourceType } from '../../../src/models/resource-types.js'; // --------------------------------------------------------------------------- @@ -21,12 +22,39 @@ function devPrefixMapping(types: ResourceType[]): EnvMapping { const ALL_TYPES = [ResourceType.NamedValue, ResourceType.PolicyFragment, ResourceType.Backend]; -const baseKnown: KnownArtifactSets = { - namedValues: new Set(['myNv', 'apiUrl']), - fragments: new Set(['myFrag', 'authFrag']), - backends: new Set(['myBackend']), +// Each named value's display name is its name here. +const BASE: Names = { + namedValues: ['myNv', 'apiUrl'], + fragments: ['myFrag', 'authFrag'], + backends: ['myBackend'], }; +interface Names { + namedValues: string[]; + fragments: string[]; + backends: string[]; +} + +/** KnownArtifactSets for `names`, with named values deployed as `mapping` affixes them. */ +function knownSets(mapping: EnvMapping | undefined, names = BASE): KnownArtifactSets { + const find = (candidates: string[], name: string) => + candidates.find((candidate) => candidate.toLowerCase() === name.toLowerCase()); + const byType: Partial> = { + [ResourceType.NamedValue]: names.namedValues, + [ResourceType.PolicyFragment]: names.fragments, + [ResourceType.Backend]: names.backends, + }; + return { + has: (type, name, workspace) => !workspace && find(byType[type] ?? [], name) !== undefined, + deployedNamedValueReference: (reference) => { + const name = find(names.namedValues, reference); + return name === undefined || !mapping ? name : toDeployedName(name, ResourceType.NamedValue, mapping); + }, + }; +} + +const rewrite = (xml: string, mapping: EnvMapping | undefined) => rewritePolicyRefs(xml, mapping, knownSets(mapping)); + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -34,52 +62,52 @@ const baseKnown: KnownArtifactSets = { describe('rewritePolicyRefs', () => { it('returns xml unchanged when mapping is undefined', () => { const xml = '{{myNv}}'; - expect(rewritePolicyRefs(xml, undefined, baseKnown)).toBe(xml); + expect(rewrite(xml, undefined)).toBe(xml); }); it('returns empty string unchanged', () => { - expect(rewritePolicyRefs('', devPrefixMapping(ALL_TYPES), baseKnown)).toBe(''); + expect(rewrite('', devPrefixMapping(ALL_TYPES))).toBe(''); }); it('rewrites known {{token}} to deployed name', () => { const xml = '{{myNv}}'; - const result = rewritePolicyRefs(xml, devPrefixMapping([ResourceType.NamedValue]), baseKnown); + const result = rewrite(xml, devPrefixMapping([ResourceType.NamedValue])); expect(result).toBe('{{dev-myNv}}'); }); it('leaves unknown runtime context token untouched', () => { const xml = ''; // context.request.headers.foo is not in namedValues — passes through - expect(rewritePolicyRefs(xml, devPrefixMapping(ALL_TYPES), baseKnown)).toBe(xml); + expect(rewrite(xml, devPrefixMapping(ALL_TYPES))).toBe(xml); }); it('leaves known named value untouched when NamedValue is not in appliesTo', () => { const xml = '{{myNv}}'; const mapping = devPrefixMapping([ResourceType.Backend]); // NamedValue excluded - expect(rewritePolicyRefs(xml, mapping, baseKnown)).toBe(xml); + expect(rewrite(xml, mapping)).toBe(xml); }); it('rewrites fragment-id with double quotes', () => { const xml = ''; - const result = rewritePolicyRefs(xml, devPrefixMapping([ResourceType.PolicyFragment]), baseKnown); + const result = rewrite(xml, devPrefixMapping([ResourceType.PolicyFragment])); expect(result).toBe(''); }); it('rewrites fragment-id with single quotes', () => { const xml = ""; - const result = rewritePolicyRefs(xml, devPrefixMapping([ResourceType.PolicyFragment]), baseKnown); + const result = rewrite(xml, devPrefixMapping([ResourceType.PolicyFragment])); expect(result).toBe(""); }); it('leaves fragment-id unchanged when name is not in known fragments', () => { const xml = ''; - const result = rewritePolicyRefs(xml, devPrefixMapping([ResourceType.PolicyFragment]), baseKnown); + const result = rewrite(xml, devPrefixMapping([ResourceType.PolicyFragment])); expect(result).toBe(xml); }); it('rewrites backend-id with single quotes', () => { const xml = ""; - const result = rewritePolicyRefs(xml, devPrefixMapping([ResourceType.Backend]), baseKnown); + const result = rewrite(xml, devPrefixMapping([ResourceType.Backend])); expect(result).toBe(""); }); @@ -90,7 +118,7 @@ describe('rewritePolicyRefs', () => { '' + '' + ''; - const result = rewritePolicyRefs(xml, devPrefixMapping(ALL_TYPES), baseKnown); + const result = rewrite(xml, devPrefixMapping(ALL_TYPES)); expect(result).toBe( '' + '{{dev-myNv}}' + @@ -107,7 +135,7 @@ describe('rewritePolicyRefs', () => { appliesTo: new Set(ALL_TYPES), }; const xml = '{{myNv}}'; - const result = rewritePolicyRefs(xml, suffixMapping, baseKnown); + const result = rewrite(xml, suffixMapping); expect(result).toBe('{{myNv-dev}}'); }); @@ -127,7 +155,7 @@ describe('rewritePolicyRefs', () => { '', ].join('\n'); - const result = rewritePolicyRefs(xml, devPrefixMapping(ALL_TYPES), baseKnown); + const result = rewrite(xml, devPrefixMapping(ALL_TYPES)); expect(result).toContain('backend-id="dev-myBackend"'); expect(result).toContain('{{dev-apiUrl}}'); @@ -139,13 +167,13 @@ describe('rewritePolicyRefs', () => { it('handles whitespace inside {{ token }} braces', () => { const xml = '{{ myNv }}'; - const result = rewritePolicyRefs(xml, devPrefixMapping([ResourceType.NamedValue]), baseKnown); + const result = rewrite(xml, devPrefixMapping([ResourceType.NamedValue])); expect(result).toBe('{{dev-myNv}}'); }); it('matches fragment-id regardless of preceding attribute ordering', () => { const xml = ''; - const result = rewritePolicyRefs(xml, devPrefixMapping([ResourceType.PolicyFragment]), baseKnown); + const result = rewrite(xml, devPrefixMapping([ResourceType.PolicyFragment])); expect(result).toBe(''); }); @@ -157,8 +185,36 @@ describe('rewritePolicyRefs', () => { const mapping = devPrefixMapping(ALL_TYPES); // After first pass: deployed names like dev-myNv are not in known sets, // so the second pass finds nothing to rewrite. - const first = rewritePolicyRefs(xml, mapping, baseKnown); - const second = rewritePolicyRefs(first, mapping, baseKnown); + const first = rewrite(xml, mapping); + const second = rewrite(first, mapping); expect(second).toBe(first); }); + + it("only rewrites names of resources in the policy's own scope", () => { + const xml = ''; + const mapping = devPrefixMapping([ResourceType.Backend]); + expect(rewritePolicyRefs(xml, mapping, knownSets(mapping), 'team-a')).toBe(xml); + }); + + it.each([ + [ + 'a commented-out Liquid open tag', + '{{ myNv }}{{ myNv }}', + '{{dev-myNv}}{{ myNv }}', + ], + [ + 'a commented-out close tag inside a Liquid body', + '{{ myNv }}{{ myNv }}', + '{{ myNv }}{{dev-myNv}}', + ], + ])('finds Liquid bodies as the scanner does, ignoring %s', (_kind, xml, expected) => { + expect(rewrite(xml, devPrefixMapping([ResourceType.NamedValue]))).toBe(expected); + }); + + it('rewrites only a bare {{name}} inside a Liquid body, leaving Liquid output', () => { + const xml = '{{myNv}} {{ myNv }} {{myNv | upcase}} {{body.id}}{{ myNv }}'; + expect(rewrite(xml, devPrefixMapping([ResourceType.NamedValue]))).toBe( + '{{dev-myNv}} {{ myNv }} {{myNv | upcase}} {{body.id}}{{dev-myNv}}' + ); + }); }); diff --git a/tests/unit/services/publish-service.test.ts b/tests/unit/services/publish-service.test.ts index eb3915c9..d2654d6d 100644 --- a/tests/unit/services/publish-service.test.ts +++ b/tests/unit/services/publish-service.test.ts @@ -1403,8 +1403,38 @@ describe('publish-service', () => { // envMapping must be populated on config after validation expect(config.envMapping).toBeDefined(); // knownArtifactSets must contain the FULL set of named values, not just changed - expect(config.knownArtifactSets?.namedValues.has('nv-unchanged')).toBe(true); - expect(config.knownArtifactSets?.namedValues.has('nv-changed')).toBe(true); + expect(config.knownArtifactSets?.has(ResourceType.NamedValue, 'nv-unchanged')).toBe(true); + expect(config.knownArtifactSets?.has(ResourceType.NamedValue, 'nv-changed')).toBe(true); + }); + + it("should rewrite a published fragment's references to resources the filter leaves out", async () => { + const client = createMockClient(); + const store = createMockStore([ + { type: ResourceType.PolicyFragment, nameParts: ['frag'] }, + { type: ResourceType.NamedValue, nameParts: ['same'] }, + { type: ResourceType.Backend, nameParts: ['new-backend'] }, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.NamedValue + ? { properties: { displayName: 'same', value: 'x' } } + : { properties: { format: 'rawxml', value: '{{same}}' } } + ); + + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter: { policyFragments: ['frag'], namedValues: [], backends: [] }, + includeTransitive: false, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + overrides: { environment: { namePrefix: 'dev-' } }, + }); + + const fragmentPut = client.putResource.mock.calls.find((call) => call[1].type === ResourceType.PolicyFragment); + expect((fragmentPut?.[2] as { properties: { value: string } }).properties.value).toBe( + '{{dev-same}}' + ); }); it('should pass commit-scoped deleted descriptors to dry-run report', async () => { diff --git a/tests/unit/services/resource-publisher.env-mapping.test.ts b/tests/unit/services/resource-publisher.env-mapping.test.ts index 45eea59c..9cc7d158 100644 --- a/tests/unit/services/resource-publisher.env-mapping.test.ts +++ b/tests/unit/services/resource-publisher.env-mapping.test.ts @@ -11,7 +11,7 @@ import { ResourceType } from '../../../src/models/resource-types.js'; import type { ApimServiceContext, ResourceDescriptor } from '../../../src/models/types.js'; import type { PublishConfig, KnownArtifactSets } from '../../../src/models/config.js'; import type { EnvMapping } from '../../../src/services/env-mapper.js'; -import { DEFAULT_APPLIES_TO } from '../../../src/services/env-mapper.js'; +import { DEFAULT_APPLIES_TO, toDeployedName } from '../../../src/services/env-mapper.js'; import { LogLevel } from '../../../src/lib/logger.js'; // Mock keyvault-checker @@ -76,11 +76,28 @@ const DEV_MAPPING: EnvMapping = { appliesTo: DEFAULT_APPLIES_TO, }; -const EMPTY_KNOWN: KnownArtifactSets = { - namedValues: new Set(), - fragments: new Set(), - backends: new Set(), -}; +/** + * KnownArtifactSets for these names, with each named value's display name its + * name, deployed with DEV_MAPPING's affix. + */ +function knownSets(names: { namedValues?: string[]; fragments?: string[]; backends?: string[] } = {}): KnownArtifactSets { + const find = (candidates: string[] | undefined, name: string) => + candidates?.find((candidate) => candidate.toLowerCase() === name.toLowerCase()); + const byType: Partial> = { + [ResourceType.NamedValue]: names.namedValues, + [ResourceType.PolicyFragment]: names.fragments, + [ResourceType.Backend]: names.backends, + }; + return { + has: (type, name) => find(byType[type], name) !== undefined, + deployedNamedValueReference: (reference) => { + const name = find(names.namedValues, reference); + return name === undefined ? undefined : toDeployedName(name, ResourceType.NamedValue, DEV_MAPPING); + }, + }; +} + +const EMPTY_KNOWN = knownSets(); function makeConfig(overrides?: Partial): PublishConfig { return { ...baseConfig, ...overrides }; @@ -395,6 +412,54 @@ describe('resource-publisher env-mapping', () => { }); }); + describe('Named value references, by the named value they resolve to', () => { + // backend-id has the display name Backend_Id, in the service and in the + // team-a workspace; key-id has Key. + const namedValues: ResourceDescriptor[] = [ + { type: ResourceType.NamedValue, nameParts: ['backend-id'] }, + { type: ResourceType.NamedValue, nameParts: ['backend-id'], workspace: 'team-a' }, + { type: ResourceType.NamedValue, nameParts: ['key-id'] }, + ]; + const publishPolicy = async (xml: string, config: PublishConfig, workspace?: string) => { + store.listResources.mockResolvedValue(namedValues); + store.readResource.mockImplementation(async (_dir: string, d: ResourceDescriptor) => + d.type === ResourceType.NamedValue + ? { name: d.nameParts[0], properties: { displayName: d.nameParts[0] === 'key-id' ? 'Key' : 'Backend_Id', value: 'x' } } + : null + ); + store.readContent.mockResolvedValue({ content: xml }); + await publishResource(client, store, testContext, { type: ResourceType.ApiPolicy, nameParts: ['petstore'], workspace }, config); + const [, , payload] = client.putResource.mock.calls[0] as [unknown, unknown, Record]; + return (payload.properties as Record).value as string; + }; + + it('rewrites a token by display name, in any case, to the deployed display name', async () => { + const value = await publishPolicy('{{Backend_Id}}{{backend_id}}{{backend-id}}', makeConfig({ envMapping: DEV_MAPPING })); + // {{backend-id}} is only a resource name, which APIM doesn't resolve. + expect(value).toBe('{{dev-Backend_Id}}{{dev-Backend_Id}}{{backend-id}}'); + }); + + it('follows a display name an explicit override changes, without a mapping', async () => { + const value = await publishPolicy( + '{{Key}}{{prod_key}}', + makeConfig({ overrides: { namedValues: { 'key-id': { properties: { displayName: 'Prod_Key' } } } } }) + ); + expect(value).toBe('{{Prod_Key}}{{prod_key}}'); + }); + + it("matches a workspace policy's tokens against the workspace's named values", async () => { + const config = makeConfig({ + envMapping: DEV_MAPPING, + overrides: { + workspaces: { + 'team-a': { properties: {}, children: { namedValues: { 'backend-id': { properties: { displayName: 'Team_Backend' } } } } }, + }, + }, + }); + expect(await publishPolicy('{{Backend_Id}}', config, 'team-a')).toBe('{{Team_Backend}}'); + }); + }); + describe('Policy XML rewrite', () => { it('rewrites {{nvName}}, fragment-id, backend-id in policy content', async () => { const xml = @@ -408,11 +473,7 @@ describe('resource-publisher env-mapping', () => { type: ResourceType.ApiPolicy, nameParts: ['petstore'], }; - const known: KnownArtifactSets = { - namedValues: new Set(['myNv']), - fragments: new Set(['myFrag']), - backends: new Set(['myBackend']), - }; + const known = knownSets({ namedValues: ['myNv'], fragments: ['myFrag'], backends: ['myBackend'] }); const config = makeConfig({ envMapping: DEV_MAPPING, knownArtifactSets: known }); await publishResource(client, store, testContext, descriptor, config); const [, , payload] = client.putResource.mock.calls[0] as [unknown, unknown, Record]; @@ -437,11 +498,7 @@ describe('resource-publisher env-mapping', () => { type: ResourceType.PolicyFragment, nameParts: ['myFrag'], }; - const known: KnownArtifactSets = { - namedValues: new Set(), - fragments: new Set(), - backends: new Set(['myBackend']), - }; + const known = knownSets({ backends: ['myBackend'] }); const config = makeConfig({ envMapping: DEV_MAPPING, knownArtifactSets: known }); await publishResource(client, store, testContext, descriptor, config); const [, , payload] = client.putResource.mock.calls[0] as [unknown, unknown, Record]; From 7c4f81e51382fd172466f1bf9a66b91d2545e765 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 10:46:22 +1100 Subject: [PATCH 04/18] fix: point logger and backend credential references at deployed display names, and drop the resource-name rewrite --- src/services/override-merger.ts | 11 ++ src/services/publish-service.ts | 5 +- src/services/resource-publisher.ts | 114 +++++++----------- src/services/secret-redaction-guard.ts | 5 +- .../resource-publisher.env-mapping.test.ts | 38 +++++- .../unit/services/resource-publisher.test.ts | 33 ++++- 6 files changed, 126 insertions(+), 80 deletions(-) diff --git a/src/services/override-merger.ts b/src/services/override-merger.ts index d723a3aa..a39f65b4 100644 --- a/src/services/override-merger.ts +++ b/src/services/override-merger.ts @@ -11,6 +11,7 @@ import { ResourceDescriptor } from '../models/types.js'; import { ResourceType, RESOURCE_TYPE_METADATA } from '../models/resource-types.js'; import { OverrideConfig, OverrideSection, OverrideEntry } from '../models/config.js'; import { logger } from '../lib/logger.js'; +import { isAutoGeneratedId } from '../lib/auto-generated.js'; import { getNameFromNameParts, isSingletonType } from '../lib/resource-path.js'; /** Keys of OverrideConfig that hold OverrideSection values (excludes non-section fields). */ @@ -61,6 +62,16 @@ const GRANDCHILD_OVERRIDE_MAP: Partial, - store: IArtifactStore, - sourceDir: string, descriptor: ResourceDescriptor, - overrides?: OverrideConfig, - envMapping?: EnvMapping, -): Promise> { + known: KnownArtifactSets, +): Record { const props = json.properties as Record | undefined; const credentials = props?.credentials; if (!props || !credentials || typeof credentials !== 'object') { return json; } - // Find all {{ref}} tokens in credentials - const credStr = JSON.stringify(credentials); - const refPattern = /\{\{([^}]+)\}\}/g; const rewriteMap = new Map(); - - let match; - while ((match = refPattern.exec(credStr)) !== null) { + for (const match of JSON.stringify(credentials).matchAll(/\{\{([^}]+)\}\}/g)) { const ref = match[1]; if (rewriteMap.has(ref)) continue; - - // Try to read the named value artifact by this ref as a resource name - const nvDescriptor: ResourceDescriptor = { - type: ResourceType.NamedValue, - nameParts: [ref], - ...(descriptor.workspace ? { workspace: descriptor.workspace } : {}), - }; - let nvJson = await store.readResource(sourceDir, nvDescriptor); - if (nvJson) { - // Apply any named value overrides so displayName reflects user config - nvJson = applyOverrides(nvDescriptor, nvJson, overrides); - const nvProps = nvJson.properties as Record | undefined; - let displayName = nvProps?.displayName as string | undefined; - - // If envMapping applies to NamedValue and no explicit displayName override, - // use the affixed displayName so {{token}} refs resolve on the shared APIM. - if (envMapping?.appliesTo.has(ResourceType.NamedValue)) { - const hasDisplayNameOverride = hasExplicitPropertyOverride( - ref, - 'displayName', - resolveOverrideSection(overrides, 'namedValues', descriptor.workspace), + const name = ref.trim(); + const deployed = known.deployedNamedValueReference(name, descriptor.workspace); + if (deployed === undefined) { + if (isAutoGeneratedId(name)) { + // A 24-char hex ref is an auto-generated named value's resource name, + // which APIM doesn't resolve; its display name is + // Logger-Credentials--. Warn so the user knows the credential + // reference cannot be resolved. + logger.warn( + `${buildResourceLabel(descriptor)} references named value '${ref}' ` + + `but no named value has that display name. It may fail to publish. ` + + `Provide a credentials override or reference the named value by its display name.` ); - if (!hasDisplayNameOverride) { - const baseDisplayName = displayName ?? ref; - displayName = toDeployedName(baseDisplayName, ResourceType.NamedValue, envMapping); - } - } - - if (displayName && displayName !== ref) { - rewriteMap.set(ref, displayName); } - } else if (isAutoGeneratedId(ref)) { - // A 24-char hex ref with no matching artifact likely means the named - // value was filtered out or deleted. Warn so the user knows the logger - // credential reference cannot be resolved. - logger.warn( - `Logger '${getNamePart(descriptor.nameParts, 0)}' references named value '${ref}' ` + - `but no matching artifact exists. The logger may fail to publish. ` + - `Provide a credentials override or include the named value artifact.` - ); + continue; + } + // APIM matches display names ignoring case, so a reference that differs + // only in case already names it. + if (deployed.toLowerCase() !== name.toLowerCase()) { + rewriteMap.set(ref, deployed); } } if (rewriteMap.size === 0) return json; - // Rewrite {{name}} → {{displayName}} in credentials + // Rewrite {{ref}} → {{deployed display name}} in credentials const rewrittenCredentials = rewriteRefs(credentials, rewriteMap); logger.debug( - `Rewrote named value references in Logger '${getNamePart(descriptor.nameParts, 0)}' credentials: ` + + `Rewrote named value references in ${buildResourceLabel(descriptor)} credentials: ` + [...rewriteMap.entries()].map(([n, d]) => `{{${n}}} → {{${d}}}`).join(', ') ); diff --git a/src/services/secret-redaction-guard.ts b/src/services/secret-redaction-guard.ts index 106a4817..4c1807a6 100644 --- a/src/services/secret-redaction-guard.ts +++ b/src/services/secret-redaction-guard.ts @@ -22,12 +22,11 @@ import type { IArtifactStore } from '../clients/iartifact-store.js'; import type { PublishConfig } from '../models/config.js'; import type { ResourceDescriptor } from '../models/types.js'; import { ResourceType } from '../models/resource-types.js'; -import { applyOverrides, hasNamedValueOverride } from './override-merger.js'; +import { applyOverrides, skipsAutoGeneratedNamedValue } from './override-merger.js'; import { POLICY_TYPES } from './resource-publisher.js'; import { REDACTION_MARKER } from './secret-redactor.js'; import { buildResourceLabel } from '../lib/resource-uri.js'; import { getNamePart } from '../lib/resource-path.js'; -import { isAutoGeneratedId } from '../lib/auto-generated.js'; /** * A single artifact that still contains a redaction marker after overrides. @@ -111,7 +110,7 @@ async function scanNamedValue( // supplies an explicit override, so scanning them here would flag markers in // artifacts that are never sent to APIM. Mirror the publish filter exactly. const name = getNamePart(descriptor.nameParts, 0); - if (isAutoGeneratedId(name) && !hasNamedValueOverride(name, config.overrides, descriptor.workspace)) { + if (skipsAutoGeneratedNamedValue(name, config.overrides, descriptor.workspace)) { return undefined; } diff --git a/tests/unit/services/resource-publisher.env-mapping.test.ts b/tests/unit/services/resource-publisher.env-mapping.test.ts index 9cc7d158..e342535c 100644 --- a/tests/unit/services/resource-publisher.env-mapping.test.ts +++ b/tests/unit/services/resource-publisher.env-mapping.test.ts @@ -404,7 +404,9 @@ describe('resource-publisher env-mapping', () => { type: ResourceType.Logger, nameParts: ['my-logger'], }; - const config = makeConfig({ envMapping: DEV_MAPPING, knownArtifactSets: EMPTY_KNOWN }); + store.listResources.mockResolvedValue([{ type: ResourceType.NamedValue, nameParts: ['myNv'] }]); + // Built from the source, as for a resource published outside a publish run. + const config = makeConfig({ envMapping: DEV_MAPPING }); await publishResource(client, store, testContext, descriptor, config); const [, , payload] = client.putResource.mock.calls[0] as [unknown, unknown, Record]; const creds = (payload.properties as Record).credentials as Record; @@ -447,6 +449,40 @@ describe('resource-publisher env-mapping', () => { expect(value).toBe('{{Prod_Key}}{{prod_key}}'); }); + it("keeps a reference to an auto-generated logger key publish skips, and affixes one an override provides", async () => { + const id = '66f48e1226dab62c0823e4f8'; + const publishLogger = async (overrides?: PublishConfig['overrides']) => { + client.putResource.mockClear(); + store.listResources.mockResolvedValue([{ type: ResourceType.NamedValue, nameParts: [id] }]); + store.readResource.mockImplementation(async (_dir: string, d: ResourceDescriptor) => + d.type === ResourceType.NamedValue + ? { name: id, properties: { displayName: `Logger-Credentials--${id}`, secret: true } } + : { name: 'ai', properties: { loggerType: 'applicationInsights', credentials: { instrumentationKey: `{{Logger-Credentials--${id}}}` } } } + ); + await publishResource(client, store, testContext, { type: ResourceType.Logger, nameParts: ['ai'] }, makeConfig({ envMapping: DEV_MAPPING, overrides })); + const [, , payload] = client.putResource.mock.calls[0] as [unknown, unknown, Record]; + return ((payload.properties as Record).credentials as Record).instrumentationKey; + }; + + // Publish skips it, so the logger keeps the key the target already has. + expect(await publishLogger()).toBe(`{{Logger-Credentials--${id}}}`); + expect(await publishLogger({ namedValues: { [id]: { properties: { value: 'key' } } } })).toBe(`{{dev-Logger-Credentials--${id}}}`); + }); + + it('rewrites the tokens in backend credentials', async () => { + store.listResources.mockResolvedValue(namedValues); + store.readResource.mockImplementation(async (_dir: string, d: ResourceDescriptor) => + d.type === ResourceType.NamedValue + ? { name: d.nameParts[0], properties: { displayName: 'Backend_Id' } } + : { name: 'b', properties: { url: 'https://example.test', credentials: { header: { 'x-key': ['{{Backend_Id}}'], 'x-spaced': ['{{ Backend_Id }}'] } } } } + ); + await publishResource(client, store, testContext, { type: ResourceType.Backend, nameParts: ['b'] }, makeConfig({ envMapping: DEV_MAPPING })); + const [, , payload] = client.putResource.mock.calls[0] as [unknown, unknown, Record]; + expect((payload.properties as Record).credentials).toEqual({ + header: { 'x-key': ['{{dev-Backend_Id}}'], 'x-spaced': ['{{dev-Backend_Id}}'] }, + }); + }); + it("matches a workspace policy's tokens against the workspace's named values", async () => { const config = makeConfig({ envMapping: DEV_MAPPING, diff --git a/tests/unit/services/resource-publisher.test.ts b/tests/unit/services/resource-publisher.test.ts index 8e1154e5..f414a1c7 100644 --- a/tests/unit/services/resource-publisher.test.ts +++ b/tests/unit/services/resource-publisher.test.ts @@ -17,7 +17,7 @@ import { ResourceType } from '../../../src/models/resource-types.js'; import { ApimServiceContext, ResourceDescriptor } from '../../../src/models/types.js'; import { PublishConfig } from '../../../src/models/config.js'; import { KeyVaultAccessError } from '../../../src/services/keyvault-checker.js'; -import { LogLevel } from '../../../src/lib/logger.js'; +import { LogLevel, logger } from '../../../src/lib/logger.js'; import { REDACTION_MARKER } from '../../../src/services/secret-redactor.js'; import { buildEnvMapping } from '../../../src/services/env-mapper.js'; import { HttpError } from '../../../src/clients/apim-client.js'; @@ -2070,9 +2070,11 @@ describe('resource-publisher', () => { }); describe('Logger named value reference rewriting', () => { - it('should rewrite {{name}} to {{displayName}} in logger credentials', async () => { + it("should leave a reference to an auto-generated named value's resource name, and warn", async () => { + const warn = vi.spyOn(logger, 'warn').mockImplementation(() => undefined); const client = createMockClient(); const store = createMockStore(); + store.listResources.mockResolvedValue([{ type: ResourceType.NamedValue, nameParts: ['abc123def456abc123def456'] }]); const loggerJson = { name: 'my-logger', @@ -2109,7 +2111,10 @@ describe('resource-publisher', () => { const putPayload = client.putResource.mock.calls[0][2] as Record; const props = putPayload.properties as Record; const creds = props.credentials as Record; - expect(creds.instrumentationKey).toBe('{{Logger-Credentials--abc123def456abc123def457}}'); + // APIM matches display names only, so the reference isn't repaired. + expect(creds.instrumentationKey).toBe('{{abc123def456abc123def456}}'); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('no named value has that display name')); + warn.mockRestore(); }); it('should leave {{displayName}} references unchanged', async () => { @@ -2146,6 +2151,27 @@ describe('resource-publisher', () => { expect(creds.instrumentationKey).toBe('{{Logger-Credentials--abc123}}'); }); + it('should point a reference at the display name an override gives, and leave one already using it', async () => { + const client = createMockClient(); + const store = createMockStore(); + store.listResources.mockResolvedValue([{ type: ResourceType.NamedValue, nameParts: ['key-id'] }]); + store.readResource.mockImplementation(async (_dir: string, desc: ResourceDescriptor) => + desc.type === ResourceType.Logger + ? { properties: { loggerType: 'applicationInsights', credentials: { a: '{{Key}}', b: '{{prod_key}}' } } } + : { name: 'key-id', properties: { displayName: 'Key', secret: true } } + ); + const config: PublishConfig = { + ...testConfig, + overrides: { namedValues: { 'key-id': { properties: { displayName: 'Prod_Key' } } } }, + }; + + await publishResource(client, store, testContext, { type: ResourceType.Logger, nameParts: ['my-logger'] }, config); + + const credentials = ((client.putResource.mock.calls[0]?.[2] as Record).properties as Record) + .credentials as Record; + expect(credentials).toEqual({ a: '{{Prod_Key}}', b: '{{prod_key}}' }); + }); + it('should keep an explicit workspace named-value displayName unprefixed in logger credentials', async () => { const client = createMockClient(); const store = createMockStore(); @@ -2156,6 +2182,7 @@ describe('resource-publisher', () => { credentials: { instrumentationKey: '{{api-key}}' }, }, }; + store.listResources.mockResolvedValue([{ type: ResourceType.NamedValue, nameParts: ['api-key'], workspace: 'team-a' }]); store.readResource.mockImplementation(async (_dir: string, descriptor: ResourceDescriptor) => { if (descriptor.type === ResourceType.Logger) return loggerJson; if (descriptor.type === ResourceType.NamedValue && descriptor.nameParts[0] === 'api-key') { From 6d4d0204c7fee6213a8dc29fd3e41a3001ee5322 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 10:47:20 +1100 Subject: [PATCH 05/18] fix: prefix every policy attribute that names a resource --- src/services/policy-ref-rewriter.ts | 50 +++++-------------- src/services/transitive-resolver.ts | 26 ++++++++++ .../unit/services/policy-ref-rewriter.test.ts | 33 +++++++++++- 3 files changed, 71 insertions(+), 38 deletions(-) diff --git a/src/services/policy-ref-rewriter.ts b/src/services/policy-ref-rewriter.ts index 12fadbb0..15e9695f 100644 --- a/src/services/policy-ref-rewriter.ts +++ b/src/services/policy-ref-rewriter.ts @@ -21,29 +21,10 @@ * {{context.request.headers.foo}}, which no named value has as its name. */ -import { ResourceType } from '../models/resource-types.js'; import type { EnvMapping } from './env-mapper.js'; import { toDeployedName } from './env-mapper.js'; import type { KnownArtifactSets } from '../models/config.js'; -import { replaceNamedValueReferences } from './transitive-resolver.js'; - -// --------------------------------------------------------------------------- -// Regex patterns -// --------------------------------------------------------------------------- - -/** - * Matches the fragment-id attribute on an element. - * Groups: 1 = tag prefix through `=`, 2 = opening quote, 3 = id value, - * 4 = closing quote (backreference to group 2). - */ -const FRAGMENT_PATTERN = /(]*\bfragment-id\s*=\s*)(["'])([^"'>]+)(\2)/gi; - -/** - * Matches the backend-id attribute on a element. - * Groups: 1 = tag prefix through `=`, 2 = opening quote, 3 = id value, - * 4 = closing quote (backreference to group 2). - */ -const BACKEND_PATTERN = /(]*\bbackend-id\s*=\s*)(["'])([^"'>]+)(\2)/gi; +import { POLICY_REFERENCE_PATTERNS, replaceNamedValueReferences } from './transitive-resolver.js'; // --------------------------------------------------------------------------- // Public API @@ -63,12 +44,10 @@ export type { KnownArtifactSets } from '../models/config.js'; * display name that named value is deployed with * (KnownArtifactSets.deployedNamedValueReference). Rewritten even without * a mapping when an explicit displayName override changes it. - * 2. `fragment-id="name"` on . Only rewritten when the - * source has that fragment in the policy's scope AND - * ResourceType.PolicyFragment ∈ mapping.appliesTo. - * 3. `backend-id="name"` on . Only rewritten when the - * source has that backend in the policy's scope AND - * ResourceType.Backend ∈ mapping.appliesTo. + * 2. An attribute that names a resource (POLICY_REFERENCE_PATTERNS: + * backend-id, embeddings-backend-id, fragment-id, logger-id, schema-id). + * Rewritten when its type ∈ mapping.appliesTo and the source has that + * resource in the policy's scope. * * @param workspace - The policy's workspace, if any: references name * resources in its own scope @@ -90,17 +69,14 @@ export function rewritePolicyRefs( if (mapping === undefined) return result; - if (mapping.appliesTo.has(ResourceType.PolicyFragment)) { - result = result.replace(FRAGMENT_PATTERN, (match, prefix: string, quote: string, id: string) => { - if (!known.has(ResourceType.PolicyFragment, id, workspace)) return match; - return `${prefix}${quote}${toDeployedName(id, ResourceType.PolicyFragment, mapping)}${quote}`; - }); - } - - if (mapping.appliesTo.has(ResourceType.Backend)) { - result = result.replace(BACKEND_PATTERN, (match, prefix: string, quote: string, id: string) => { - if (!known.has(ResourceType.Backend, id, workspace)) return match; - return `${prefix}${quote}${toDeployedName(id, ResourceType.Backend, mapping)}${quote}`; + // An attribute that names a resource (backend-id, fragment-id, logger-id, + // schema-id ...) names one in the policy's scope. A policy expression or a + // named value (rewritten above) names none here. + for (const { type, pattern } of POLICY_REFERENCE_PATTERNS) { + if (!mapping.appliesTo.has(type)) continue; + result = result.replace(pattern, (match: string, quote: string, value: string) => { + if (value.startsWith('@') || value.includes('{{') || !known.has(type, value, workspace)) return match; + return `${match.slice(0, match.length - value.length - quote.length)}${toDeployedName(value, type, mapping)}${quote}`; }); } diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index c19ee2d2..7b181781 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -30,6 +30,32 @@ const FRAGMENT_PATTERN = / = [ + { type: ResourceType.Backend, pattern: attributePattern('set-backend-service', 'backend-id') }, + { type: ResourceType.Backend, pattern: attributePattern('llm-content-safety', 'backend-id') }, + { + type: ResourceType.Backend, + pattern: attributePattern('(?:azure-openai|llm)-semantic-cache-lookup', 'embeddings-backend-id'), + }, + { type: ResourceType.PolicyFragment, pattern: attributePattern('include-fragment', 'fragment-id') }, + { type: ResourceType.Logger, pattern: attributePattern('log-to-eventhub', 'logger-id') }, + // validate-content's names a schema added to the service. + { type: ResourceType.GlobalSchema, pattern: attributePattern('content', 'schema-id') }, +]; + /** * XML comments: references inside them are not in effect. Only a closed * comment is removed, so a stray `'], + ['no named value in the source', '{{on-target-only}}'], + ])('should publish a reference to %s', async (_kind, fragment) => { + const { result, client } = await publishWith(fragment, {}, `{{Logger-Credentials--${hex}}}`); + + expect(result.exitCode).toBe(0); + expect(client.putResource).toHaveBeenCalled(); + }); + }); + it('should use computeGitDiff when commitId is set (incremental mode)', async () => { const client = createMockClient(); const store = createMockStore([]); From 1ef23eddd367c427ac806a62eb6f10d8c1c8bf69 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 11:10:55 +1100 Subject: [PATCH 07/18] fix: include the named values logger and backend credentials use, as published Closes #315 --- src/services/publish-service.ts | 8 +++- src/services/transitive-resolver.ts | 47 +++++++++++++------ tests/unit/services/publish-service.test.ts | 30 ++++++++++++ .../unit/services/transitive-resolver.test.ts | 29 ++++++++++++ 4 files changed, 98 insertions(+), 16 deletions(-) diff --git a/src/services/publish-service.ts b/src/services/publish-service.ts index ad4d7fd3..240e84a2 100644 --- a/src/services/publish-service.ts +++ b/src/services/publish-service.ts @@ -355,6 +355,7 @@ async function determinePublishTargets( config.sourceDir, targetDescriptors, config.filter, + config.overrides, sourceReferences ); } @@ -468,6 +469,7 @@ async function expandTransitivePublishTargets( sourceDir: string, initialDescriptors: ResourceDescriptor[], filter: FilterConfig, + overrides: OverrideConfig | undefined, sourceReferences: SourceReferences ): Promise { const availableDescriptors = await sourceReferences.available(); @@ -522,7 +524,11 @@ async function expandTransitivePublishTargets( for (let index = 0; index < scanQueue.length; index++) { let references: ResourceDescriptor[]; try { - references = await scanArtifactReferences(store, sourceDir, scanQueue[index]); + // As published: an override can change what a backend's or logger's + // credentials or a policy's XML reference. + references = await scanArtifactReferences( + store, sourceDir, scanQueue[index], (descriptor, json) => applyOverrides(descriptor, json, overrides) + ); } catch (error) { logger.warn( `Unable to scan transitive references for ${scanQueue[index].type} "${scanQueue[index].nameParts.join('/')}": ${String(error)}` diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index cc852dd4..1ae6f328 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -355,6 +355,19 @@ export function findTransitiveDependencies( for (const { descriptor, json } of resources) { const properties = json.properties as Record | undefined; + // Logger and backend credentials can hold named value references (for + // example the Application Insights key APIM stores as a named value). + if ( + (descriptor.type === ResourceType.Logger || descriptor.type === ResourceType.Backend) && + properties?.credentials !== undefined + ) { + for (const match of JSON.stringify(properties.credentials).matchAll(NAMED_VALUE_PATTERN)) { + if (match[1]) { + dependencies.push({ type: ResourceType.NamedValue, nameParts: [match[1].trim()], workspace: descriptor.workspace }); + } + } + } + if (descriptor.type === ResourceType.Backend) { const pool = isRecord(properties?.pool) ? properties.pool : undefined; const services = pool?.services; @@ -398,11 +411,15 @@ export function findTransitiveDependencies( * * Association and subscription targets are links to independently selected * composite resources, not transitive dependencies. + * + * @param prepareJson - Turns the artifact's JSON into what is scanned, such + * as the payload publish sends with overrides applied */ export async function scanArtifactReferences( store: IArtifactStore, sourceDir: string, - descriptor: ResourceDescriptor + descriptor: ResourceDescriptor, + prepareJson?: PrepareJson ): Promise { const references: ResourceDescriptor[] = []; const policies = new Map(); @@ -410,20 +427,19 @@ export async function scanArtifactReferences( if (POLICY_RESOURCE_TYPES.has(descriptor.type)) { const content = await store.readContent(sourceDir, descriptor, 'policy'); - if (content) { - policies.set(descriptor.nameParts.join('/'), content.content); + const text = content ? preparePolicyContent(descriptor, content.content, prepareJson) : undefined; + if (typeof text === 'string') { + policies.set(descriptor.nameParts.join('/'), text); } } const infoFile = RESOURCE_TYPE_METADATA[descriptor.type]?.infoFile; - const json = POLICY_RESOURCE_TYPES.has(descriptor.type) || !infoFile?.endsWith('.json') + const stored = POLICY_RESOURCE_TYPES.has(descriptor.type) || !infoFile?.endsWith('.json') ? undefined : await store.readResource(sourceDir, descriptor); - if (json) { - if (descriptor.type === ResourceType.Api) { - apis.set(descriptor.nameParts.join('/'), json); - } - + const json = stored && prepareJson ? prepareJson(descriptor, stored) : stored; + if (json && descriptor.type === ResourceType.Api) { + apis.set(descriptor.nameParts.join('/'), json); } references.push( @@ -506,12 +522,13 @@ export async function scanArtifactNamedValueReferences( const properties = json?.properties as Record | undefined; if (descriptor.type === ResourceType.PolicyFragment) { names.push(...policyNames(properties?.value), ...policyNames(properties?.policyContent)); - } else if (properties?.credentials !== undefined) { - for (const match of JSON.stringify(properties.credentials).matchAll(NAMED_VALUE_PATTERN)) { - if (match[1]) { - names.push(match[1].trim()); - } - } + } else if (json) { + // The credentials' named values, as dependencies find them. + names.push( + ...findTransitiveDependencies(new Map(), new Map(), descriptor.workspace, [{ descriptor, json }]) + .filter((dependency) => dependency.type === ResourceType.NamedValue) + .map((dependency) => dependency.nameParts[0] ?? '') + ); } } return deduplicateDescriptors( diff --git a/tests/unit/services/publish-service.test.ts b/tests/unit/services/publish-service.test.ts index 4cfe049f..2263d13a 100644 --- a/tests/unit/services/publish-service.test.ts +++ b/tests/unit/services/publish-service.test.ts @@ -330,6 +330,36 @@ describe('publish-service', () => { expect(namedValuePuts).toEqual(['backend-id']); }); + it("should pull in the named values a backend's overridden credentials use", async () => { + const resources = [ + { type: ResourceType.Backend, nameParts: ['api-backend'] }, + { type: ResourceType.NamedValue, nameParts: ['dev-key'] }, + { type: ResourceType.NamedValue, nameParts: ['prod-key'] }, + ]; + const client = createMockClient(); + const store = createMockStore(resources); + store.readResource.mockImplementation(async (_sourceDir, descriptor) => + descriptor.type === ResourceType.Backend + ? { properties: { url: 'https://example.test', credentials: { header: { 'x-key': ['{{dev-key}}'] } } } } + : { properties: { displayName: descriptor.nameParts[0], value: 'secret' } } + ); + + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter: { backends: ['api-backend'], namedValues: [] }, + overrides: { backends: { 'api-backend': { properties: { credentials: { header: { 'x-key': ['{{prod-key}}'] } } } } } }, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + + const namedValuePuts = client.putResource.mock.calls + .filter((call) => call[1].type === ResourceType.NamedValue) + .map((call) => call[1].nameParts[0]); + expect(namedValuePuts).toEqual(['prod-key']); + }); + it('should include every dependency type supported by extract', async () => { const resources: ResourceDescriptor[] = [ { type: ResourceType.Api, nameParts: ['orders'] }, diff --git a/tests/unit/services/transitive-resolver.test.ts b/tests/unit/services/transitive-resolver.test.ts index 802bb017..b4f0b637 100644 --- a/tests/unit/services/transitive-resolver.test.ts +++ b/tests/unit/services/transitive-resolver.test.ts @@ -252,6 +252,35 @@ describe('transitive-resolver', () => { expect(deps).toHaveLength(0); }); + it('should follow named values in logger and backend credentials', () => { + const deps = findTransitiveDependencies(new Map(), new Map(), undefined, [ + { + descriptor: { type: ResourceType.Logger, nameParts: ['app-insights'] }, + json: { properties: { loggerType: 'applicationInsights', credentials: { instrumentationKey: '{{Logger-Credentials--abc}}' } } }, + }, + { + descriptor: { type: ResourceType.Backend, nameParts: ['orders'], workspace: 'team-a' }, + json: { + properties: { + url: 'https://orders.example.com', + credentials: { + header: { 'x-api-key': ['{{orders-key}}'] }, + query: { code: ['{{orders-code}}'] }, + authorization: { scheme: 'Bearer', parameter: '{{orders-token}}' }, + }, + }, + }, + }, + ]); + + expect(deps).toEqual([ + { type: ResourceType.NamedValue, nameParts: ['Logger-Credentials--abc'], workspace: undefined }, + { type: ResourceType.NamedValue, nameParts: ['orders-key'], workspace: 'team-a' }, + { type: ResourceType.NamedValue, nameParts: ['orders-code'], workspace: 'team-a' }, + { type: ResourceType.NamedValue, nameParts: ['orders-token'], workspace: 'team-a' }, + ]); + }); + describe('scanArtifactReferences', () => { it('scans policy references without parsing policy XML as JSON', async () => { const store = { From 552a74620c8258121911de3cb1b1a66c66a049ae Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 11:13:53 +1100 Subject: [PATCH 08/18] fix: include the resource a policy names through a named value's value Closes #331 --- src/services/named-value-names.ts | 26 +- src/services/publish-service.ts | 58 +++- src/services/transitive-extractor.ts | 18 +- src/services/transitive-resolver.ts | 283 +++++++++++++++++- tests/unit/services/named-value-names.test.ts | 15 +- tests/unit/services/publish-service.test.ts | 252 +++++++++++++++- .../services/transitive-extractor.test.ts | 72 +++++ .../unit/services/transitive-resolver.test.ts | 136 +++++++++ 8 files changed, 840 insertions(+), 20 deletions(-) diff --git a/src/services/named-value-names.ts b/src/services/named-value-names.ts index 39450999..4d169e38 100644 --- a/src/services/named-value-names.ts +++ b/src/services/named-value-names.ts @@ -1,11 +1,15 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. /** - * The rule for what a `{{reference}}` to a named value means, in one place - * for extract and publish: it names the named value in its own scope (the - * service, or one workspace) with that display name, ignoring case - * (NamedValueNames). APIM matches display names only: it rejects a policy - * whose reference is only a named value's resource name. + * The rules for what a `{{reference}}` to a named value means, in one place + * for extract and publish: + * + * - It names the named value in its own scope (the service, or one + * workspace) with that display name, ignoring case (NamedValueNames). APIM + * matches display names only: it rejects a policy whose reference is only a + * named value's resource name. + * - A policy attribute set to one (backend-id="{{name}}") names the resource + * its value gives, when the value is known (namedValueTargetName). * * Extract builds the names from the named values APIM lists; publish builds * them from the artifacts. On publish a reference can use either the display @@ -76,3 +80,15 @@ export function namedValueNamesByScope( return names; }; } + +/** + * The resource name a named value's value gives a policy attribute set to it, + * or undefined when it gives none: a secret named value's value isn't known, + * nor is a Key Vault one's, and a value that is itself a policy expression or + * named value reference names no resource here. + */ +export function namedValueTargetName(json: Record): string | undefined { + const properties = json.properties as Record | undefined; + const value = typeof properties?.value === 'string' ? properties.value.trim() : ''; + return properties?.secret === true || value.startsWith('@') || value.includes('{{') ? undefined : value || undefined; +} diff --git a/src/services/publish-service.ts b/src/services/publish-service.ts index 240e84a2..53a8a162 100644 --- a/src/services/publish-service.ts +++ b/src/services/publish-service.ts @@ -53,10 +53,13 @@ import { applyOverrides, skipsAutoGeneratedNamedValue } from './override-merger. import { REDACTION_MARKER } from './secret-redactor.js'; import { createArtifactReferenceResolver, + createNamedValueReader, + indexNamedValueTargetTypes, REFERENCE_SCAN_CONCURRENCY, scanArtifactNamedValueReferences, scanArtifactReferences, } from './transitive-resolver.js'; +import { namedValueTargetName } from './named-value-names.js'; import { validateAndBuildEnvMapping } from './env-mapping-validator.js'; import { mapDescriptor } from './env-mapper.js'; @@ -432,13 +435,33 @@ interface SourceReferences { readonly resolve: (reference: ResourceDescriptor) => Promise; /** A named value's artifact as stored, read once per run. */ readonly readNamedValue: (descriptor: ResourceDescriptor) => Promise | undefined>; + /** + * The resource types each named value is used for as a resource name + * (indexNamedValueTargetTypes over every policy and policy fragment). + */ + readonly namedValueTargets: () => Promise>>; } function createSourceReferences(store: IArtifactStore, config: PublishConfig): SourceReferences { let available: Promise | undefined; let resolver: Promise<(reference: ResourceDescriptor) => Promise> | undefined; + let targets: Promise>> | undefined; const namedValues = new Map | undefined>>(); const references: SourceReferences = { + namedValueTargets: () => { + targets ??= references.available().then((descriptors) => + indexNamedValueTargetTypes( + store, + config.sourceDir, + descriptors, + descriptors, + references.resolve, + // As published: an override can replace a policy's or fragment's XML. + (descriptor, json) => applyOverrides(descriptor, json, config.overrides) + ) + ); + return targets; + }, readNamedValue: (descriptor) => { const key = getResourceDescriptorKey(descriptor); let json = namedValues.get(key); @@ -473,6 +496,35 @@ async function expandTransitivePublishTargets( sourceReferences: SourceReferences ): Promise { const availableDescriptors = await sourceReferences.available(); + // A named value as it is published: with this environment's overrides, + // which can name a different resource. One that can't be read only hides + // what a policy attribute set to it names; publishing the named value + // reports the error. + const readNamedValueJson = async (descriptor: ResourceDescriptor) => { + try { + const json = await sourceReferences.readNamedValue(descriptor); + return json ? applyOverrides(descriptor, json, overrides) : undefined; + } catch (error) { + logger.warn(`Unable to read ${buildResourceLabel(descriptor)} to find what policies set to it name: ${String(error)}`); + return undefined; + } + }; + const readNamedValue = createNamedValueReader(sourceReferences.resolve, readNamedValueJson); + // A published named value needs the resource its value names when any + // policy or fragment in the source uses it as a resource name + // (backend-id="{{name}}"), even one the filter leaves out: APIM checks every + // policy on the target that uses a named value when the named value + // changes, and rejects the change while the resource it names is missing. + // The policies are read once per run, on the first named value in the + // publish set. + const findNamedValueTargets = async (namedValue: ResourceDescriptor): Promise => { + const types = (await sourceReferences.namedValueTargets()).get(getResourceDescriptorKey(namedValue)); + const json = types ? await readNamedValueJson(namedValue) : undefined; + const name = json ? namedValueTargetName(json) : undefined; + return types && name + ? [...types].map((type) => ({ type, nameParts: [name], workspace: namedValue.workspace })) + : []; + }; const expanded = [...initialDescriptors]; const included = new Set(expanded.map(getResourceDescriptorKey)); const scanQueue = [...initialDescriptors]; @@ -527,8 +579,12 @@ async function expandTransitivePublishTargets( // As published: an override can change what a backend's or logger's // credentials or a policy's XML reference. references = await scanArtifactReferences( - store, sourceDir, scanQueue[index], (descriptor, json) => applyOverrides(descriptor, json, overrides) + store, sourceDir, scanQueue[index], readNamedValue, + (descriptor, json) => applyOverrides(descriptor, json, overrides) ); + if (scanQueue[index].type === ResourceType.NamedValue) { + references.push(...await findNamedValueTargets(scanQueue[index])); + } } catch (error) { logger.warn( `Unable to scan transitive references for ${scanQueue[index].type} "${scanQueue[index].nameParts.join('/')}": ${String(error)}` diff --git a/src/services/transitive-extractor.ts b/src/services/transitive-extractor.ts index b08f7480..935a07b5 100644 --- a/src/services/transitive-extractor.ts +++ b/src/services/transitive-extractor.ts @@ -165,8 +165,16 @@ export async function extractTransitiveDependencies( const claimedNamedValues = new Set(alreadyExtracted.map(getResourceDescriptorKey)); const extractedDescriptors: ResourceDescriptor[] = []; const namedValueIndex: NamedValueNameIndex = new Map(); - const contextFor = (dep: ResourceDescriptor): ApimServiceContext => + const contextFor = (dep: { workspace?: string }): ApimServiceContext => serviceContext && dep.workspace !== workspace ? serviceContext : context; + // The named values listed in each context, once a named value has been + // extracted there (only then can a policy attribute be followed through + // one), so the attribute follows the one its reference resolves to. + const listedNames = new Map(); + const resolveNamedValueName = (reference: string, scope: string | undefined): string | undefined => { + const names = listedNames.get(contextFor({ workspace: scope }).baseUrl); + return names?.listed ? names.names.resolve(reference) : undefined; + }; let errorCount = 0; let foundDependencies = false; let reportedUnlisted = false; @@ -188,11 +196,17 @@ export async function extractTransitiveDependencies( errorCount++; logger.warn('Unable to tell which {{...}} in Liquid bodies are named values, since the named values could not be listed'); } + for (const { descriptor } of resources) { + const scopeContext = contextFor(descriptor); + if (descriptor.type === ResourceType.NamedValue && !listedNames.has(scopeContext.baseUrl)) { + listedNames.set(scopeContext.baseUrl, await namedValueNames(client, scopeContext, namedValueIndex)); + } + } // Each once: a policy can reference a named value inside and outside a // Liquid body. const seen = new Set(); const found = [ - ...findTransitiveDependencies(policies, apis, workspace, resources), + ...findTransitiveDependencies(policies, apis, workspace, resources, resolveNamedValueName), ...liquid.dependencies, ].filter((dep) => { const key = getResourceDescriptorKey(dep); diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index 1ae6f328..3cc66cde 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -14,7 +14,13 @@ import type { IArtifactStore } from '../clients/iartifact-store.js'; import { logger } from '../lib/logger.js'; import { getResourceDescriptorKey } from '../lib/resource-path.js'; import { buildResourceLabel } from '../lib/resource-uri.js'; -import { type NamedValueEntry, NamedValueNames, namedValueNamesByScope } from './named-value-names.js'; +import { + type NamedValueEntry, + NamedValueNames, + namedValueNamesByScope, + namedValueTargetName, +} from './named-value-names.js'; +import { runParallel } from '../lib/parallel-runner.js'; /** * Reference detection patterns for policy XML content. @@ -149,6 +155,20 @@ export interface TransitiveDependency { name: string; } +/** + * A whole named value reference as the value of a policy attribute that names + * a resource, such as backend-id="{{backend-name}}": the attribute names the + * resource of `type` the named value's value names. + */ +export interface NamedValueTarget { + type: ResourceType; + /** The named value as the policy references it (normally its display name). */ + namedValue: string; +} + +/** An attribute value that is one named value reference and nothing else. */ +const WHOLE_NAMED_VALUE_PATTERN = /^\{\{([^{}]+)\}\}$/; + const POLICY_RESOURCE_TYPES = new Set([ ResourceType.ServicePolicy, ResourceType.ApiPolicy, @@ -185,9 +205,10 @@ export function scanPolicyReferences(policyXml: string): TransitiveDependency[] } } - // Backend references + // Backend references. A value that is a policy expression or a named value + // reference names no resource here (see scanNamedValueTargets). for (const match of scannable.matchAll(BACKEND_PATTERN)) { - if (match[1]) { + if (match[1] && !match[1].startsWith('@') && !match[1].includes('{{')) { dependencies.push({ type: ResourceType.Backend, name: match[1].trim(), @@ -197,7 +218,7 @@ export function scanPolicyReferences(policyXml: string): TransitiveDependency[] // Policy fragment references for (const match of scannable.matchAll(FRAGMENT_PATTERN)) { - if (match[1]) { + if (match[1] && !match[1].startsWith('@') && !match[1].includes('{{')) { dependencies.push({ type: ResourceType.PolicyFragment, name: match[1].trim(), @@ -208,6 +229,42 @@ export function scanPolicyReferences(policyXml: string): TransitiveDependency[] return dependencies; } +/** + * Scan policy XML for attributes that name a resource through a named value, + * such as backend-id="{{backend-name}}". The named value itself is found by + * scanPolicyReferences; the resource its value names is found once the named + * value's value is known. + */ +export function scanNamedValueTargets(policyXml: string): NamedValueTarget[] { + const targets: NamedValueTarget[] = []; + for (const { type, value } of scanReferenceAttributes(stripNonReferenceContent(policyXml))) { + // APIM doesn't substitute a named value in include-fragment's fragment-id: + // it looks for a fragment with that literal ID and rejects the policy. + if (type === ResourceType.PolicyFragment) { + continue; + } + const namedValue = WHOLE_NAMED_VALUE_PATTERN.exec(value)?.[1]?.trim(); + if (namedValue) { + targets.push({ type, namedValue }); + } + } + return targets; +} + +/** The non-empty values of the policy attributes that name a resource, trimmed. */ +function scanReferenceAttributes(scannable: string): Array<{ type: ResourceType; value: string }> { + const attributes: Array<{ type: ResourceType; value: string }> = []; + for (const { type, pattern } of POLICY_REFERENCE_PATTERNS) { + for (const match of scannable.matchAll(pattern)) { + const value = match[2]?.trim(); + if (value) { + attributes.push({ type, value }); + } + } + } + return attributes; +} + /** * Scan API information JSON for version set reference. */ @@ -324,9 +381,21 @@ function addToFilter( return true; } +/** + * The resource name of the named value a `{{reference}}` in `workspace` + * resolves to, from a complete list of the named values there, or undefined + * when that isn't known. + */ +export type NamedValueNameResolver = (reference: string, workspace: string | undefined) => string | undefined; + /** * Build a set of descriptors from transitive dependency scanning. * Returns additional descriptors that should be extracted. + * + * @param resolveNamedValueName - Where given, a policy attribute set to a + * named value is followed only through the named value the reference + * resolves to, once it is among `resources`, rather than through another + * one there that happens to have the reference as its name */ export function findTransitiveDependencies( policies: Map, @@ -335,14 +404,17 @@ export function findTransitiveDependencies( resources: ReadonlyArray<{ descriptor: ResourceDescriptor; json: Record; - }> = [] + }> = [], + resolveNamedValueName?: NamedValueNameResolver ): ResourceDescriptor[] { const dependencies: ResourceDescriptor[] = []; + const namedValueTargets: Array = []; for (const [, policyXml] of policies) { for (const dep of scanPolicyReferences(policyXml)) { dependencies.push({ type: dep.type, nameParts: [dep.name], workspace }); } + namedValueTargets.push(...scanNamedValueTargets(policyXml).map((target) => ({ ...target, workspace }))); } for (const [, apiJson] of apis) { @@ -399,19 +471,72 @@ export function findTransitiveDependencies( workspace: descriptor.workspace, }); } + namedValueTargets.push( + ...scanNamedValueTargets(value).map((target) => ({ ...target, workspace: descriptor.workspace })) + ); } } } + dependencies.push(...findNamedValueTargetDependencies(namedValueTargets, resources, resolveNamedValueName)); + return deduplicateDescriptors(dependencies); } +/** + * The resources that policy attributes name through a named value + * (backend-id="{{backend-name}}"), for the named values among `resources` + * (NamedValueNames, namedValueTargetName). + */ +function findNamedValueTargetDependencies( + targets: ReadonlyArray, + resources: ReadonlyArray<{ descriptor: ResourceDescriptor; json: Record }>, + resolveNamedValueName?: NamedValueNameResolver +): ResourceDescriptor[] { + if (targets.length === 0) { + return []; + } + const namedValueKey = (name: string, workspace: string | undefined): string => + getResourceDescriptorKey({ type: ResourceType.NamedValue, nameParts: [name], workspace }); + const namedValues = resources.filter(({ descriptor }) => descriptor.type === ResourceType.NamedValue); + // The name each named value's value gives, or undefined when it gives none. + const byName = new Map( + namedValues.map(({ descriptor, json }) => [ + namedValueKey(descriptor.nameParts[0] ?? '', descriptor.workspace), + namedValueTargetName(json), + ]) + ); + const localNames = namedValueNamesByScope( + namedValues.map(({ descriptor, json }) => ({ + name: descriptor.nameParts[0] ?? '', + displayName: (json.properties as Record | undefined)?.displayName, + workspace: descriptor.workspace, + })) + ); + + const dependencies: ResourceDescriptor[] = []; + for (const { type, namedValue, workspace } of targets) { + // Known from the full list: the named value it resolves to, which may not + // be among `resources` yet. Otherwise only the ones here can tell. + const resolved = resolveNamedValueName?.(namedValue, workspace) ?? localNames(workspace).resolve(namedValue); + const name = resolved === undefined ? undefined : byName.get(namedValueKey(resolved, workspace)); + if (name) { + dependencies.push({ type, nameParts: [name], workspace }); + } + } + return dependencies; +} + + /** * Read intrinsic dependencies from one on-disk artifact. * * Association and subscription targets are links to independently selected * composite resources, not transitive dependencies. * + * @param readNamedValue - Reads the named value a `{{reference}}` resolves to, + * so a policy attribute set to one (backend-id="{{backend-name}}") also + * gives the resource its value names. Without it, only the named value is. * @param prepareJson - Turns the artifact's JSON into what is scanned, such * as the payload publish sends with overrides applied */ @@ -419,6 +544,7 @@ export async function scanArtifactReferences( store: IArtifactStore, sourceDir: string, descriptor: ResourceDescriptor, + readNamedValue?: NamedValueReader, prepareJson?: PrepareJson ): Promise { const references: ResourceDescriptor[] = []; @@ -442,12 +568,34 @@ export async function scanArtifactReferences( apis.set(descriptor.nameParts.join('/'), json); } + const resources = json ? [{ descriptor, json }] : []; + const resolvedNames = new Map(); + if (readNamedValue) { + const fragment = descriptor.type === ResourceType.PolicyFragment + ? (json?.properties as Record | undefined) + : undefined; + const policyTexts = [...policies.values(), fragment?.value, fragment?.policyContent] + .filter((text): text is string => typeof text === 'string'); + const namedValues = new Set(policyTexts.flatMap(scanNamedValueTargets).map((target) => target.namedValue)); + for (const namedValue of namedValues) { + const resolved = await readNamedValue({ + type: ResourceType.NamedValue, + nameParts: [namedValue], + workspace: descriptor.workspace, + }); + if (resolved) { + resources.push(resolved); + resolvedNames.set(namedValue.toLowerCase(), resolved.descriptor.nameParts[0] ?? ''); + } + } + } + + // An attribute follows the named value its reference resolved to above, + // matched as the reader matches it (a source display name an override + // changes included). references.push( - ...findTransitiveDependencies( - policies, - apis, - descriptor.workspace, - json ? [{ descriptor, json }] : [] + ...findTransitiveDependencies(policies, apis, descriptor.workspace, resources, (reference, workspace) => + workspace?.toLowerCase() === descriptor.workspace?.toLowerCase() ? resolvedNames.get(reference.toLowerCase()) : undefined ) ); @@ -600,6 +748,121 @@ async function readNamedValueNames( return namedValueNamesByScope(entries); } +/** A named value artifact, read to learn what a policy attribute set to it names. */ +export type NamedValueReader = ( + reference: ResourceDescriptor +) => Promise<{ descriptor: ResourceDescriptor; json: Record } | undefined>; + +/** + * A NamedValueReader that resolves a `{{reference}}` with `resolve` (from + * createArtifactReferenceResolver) and reads the artifact it resolves to. + */ +export function createNamedValueReader( + resolve: (reference: ResourceDescriptor) => Promise, + readResource: (descriptor: ResourceDescriptor) => Promise | undefined> +): NamedValueReader { + // Many policies can set attributes to the same named value; read it once. + const read = new Map>(); + return (reference) => { + const key = getResourceDescriptorKey(reference); + let result = read.get(key); + if (!result) { + result = (async () => { + const descriptor = await resolve(reference); + const json = descriptor ? await readResource(descriptor) : undefined; + return descriptor && json ? { descriptor, json } : undefined; + })(); + read.set(key, result); + } + return result; + }; +} + +/** + * The resource types each named value is the whole value of a reference + * attribute for (backend-id="{{name}}"), keyed by the named value's + * descriptor key. It reads the policies and policy fragments among + * `selected`, and the fragments among `available` that they include + * (), directly or through another fragment. A named value + * published without the policies that use it (only its value changed, say) + * needs the resource its value now names. + * + * @param resolve - Resolves a `{{reference}}` to the named value artifact + * @param prepareJson - Turns each policy and fragment into what is scanned, + * such as the payload publish sends with overrides applied + */ +export async function indexNamedValueTargetTypes( + store: Pick, + sourceDir: string, + selected: ReadonlyArray, + available: ReadonlyArray, + resolve: (reference: ResourceDescriptor) => Promise, + prepareJson?: PrepareJson +): Promise>> { + const isPolicy = (descriptor: ResourceDescriptor) => + POLICY_RESOURCE_TYPES.has(descriptor.type) || descriptor.type === ResourceType.PolicyFragment; + const fragments = new Map( + available + .filter((descriptor) => descriptor.type === ResourceType.PolicyFragment) + .map((descriptor) => [getResourceDescriptorKey(descriptor), descriptor]) + ); + const index = new Map>(); + const read = new Set(); + let wave = selected.filter(isPolicy); + while (wave.length > 0) { + wave.forEach((descriptor) => read.add(getResourceDescriptorKey(descriptor))); + const policies = wave; + const results = await runParallel( + policies.map((descriptor) => async (): Promise => { + if (POLICY_RESOURCE_TYPES.has(descriptor.type)) { + const content = (await store.readContent(sourceDir, descriptor, 'policy'))?.content; + return [content === undefined ? undefined : preparePolicyContent(descriptor, content, prepareJson)]; + } + const json = await store.readResource(sourceDir, descriptor); + const properties = (json && prepareJson ? prepareJson(descriptor, json) : json)?.properties as + Record | undefined; + return [properties?.value, properties?.policyContent]; + }), + REFERENCE_SCAN_CONCURRENCY + ); + wave = []; + for (const [position, result] of results.entries()) { + const descriptor = policies[position]; + if (result.status !== 'fulfilled') { + logger.warn( + `Unable to read ${buildResourceLabel(descriptor)} to find the named values its policy attributes are set to: ` + + String(result.reason) + ); + continue; + } + for (const text of result.value ?? []) { + if (typeof text !== 'string') { + continue; + } + for (const { type, namedValue } of scanNamedValueTargets(text)) { + const target = await resolve({ type: ResourceType.NamedValue, nameParts: [namedValue], workspace: descriptor.workspace }); + if (target) { + const key = getResourceDescriptorKey(target); + index.set(key, (index.get(key) ?? new Set()).add(type)); + } + } + for (const reference of scanPolicyReferences(text)) { + if (reference.type !== ResourceType.PolicyFragment) { + continue; + } + const key = getResourceDescriptorKey({ type: reference.type, nameParts: [reference.name], workspace: descriptor.workspace }); + const fragment = fragments.get(key); + if (fragment && !read.has(key)) { + read.add(key); + wave.push(fragment); + } + } + } + } + } + return index; +} + /** * Find API or Product targets referenced by a subscription payload. * diff --git a/tests/unit/services/named-value-names.test.ts b/tests/unit/services/named-value-names.test.ts index c903c980..2c433b09 100644 --- a/tests/unit/services/named-value-names.test.ts +++ b/tests/unit/services/named-value-names.test.ts @@ -5,7 +5,7 @@ */ import { describe, it, expect } from 'vitest'; -import { NamedValueNames, namedValueNamesByScope } from '../../../src/services/named-value-names.js'; +import { NamedValueNames, namedValueNamesByScope, namedValueTargetName } from '../../../src/services/named-value-names.js'; describe('NamedValueNames', () => { it('should match a reference by display name, ignoring case', () => { @@ -54,3 +54,16 @@ describe('namedValueNamesByScope', () => { expect(names('team-b').resolve('key')).toBeUndefined(); }); }); + +describe('namedValueTargetName', () => { + it.each([ + [{ properties: { value: ' backend-a ' } }, 'backend-a'], + [{ properties: { value: 'backend-a', secret: true } }, undefined], + [{ properties: { value: '@(context.Variables["b"])' } }, undefined], + [{ properties: { value: '{{other}}' } }, undefined], + [{ properties: { value: '' } }, undefined], + [{ properties: {} }, undefined], + ])('should give the resource name a value names: %j', (json, expected) => { + expect(namedValueTargetName(json)).toBe(expected); + }); +}); diff --git a/tests/unit/services/publish-service.test.ts b/tests/unit/services/publish-service.test.ts index 2263d13a..4fcf47fc 100644 --- a/tests/unit/services/publish-service.test.ts +++ b/tests/unit/services/publish-service.test.ts @@ -7,7 +7,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { ResourceType } from '../../../src/models/resource-types.js'; import { ResourceDescriptor, ApimServiceContext } from '../../../src/models/types.js'; -import { PublishConfig } from '../../../src/models/config.js'; +import { FilterConfig, PublishConfig } from '../../../src/models/config.js'; import { LogLevel, logger } from '../../../src/lib/logger.js'; import { recordRetry, withRetryResource } from '../../../src/lib/retry-tracker.js'; import { getResourceDescriptorKey } from '../../../src/lib/resource-path.js'; @@ -360,6 +360,256 @@ describe('publish-service', () => { expect(namedValuePuts).toEqual(['prod-key']); }); + it("should include the backend a policy attribute names through a named value's value (#331)", async () => { + const resources = [ + { type: ResourceType.PolicyFragment, nameParts: ['route'] }, + { type: ResourceType.NamedValue, nameParts: ['backend-id'] }, + { type: ResourceType.Backend, nameParts: ['function-backend'] }, + { type: ResourceType.Backend, nameParts: ['unused'] }, + ]; + const client = createMockClient(); + const store = createMockStore(resources); + store.readResource.mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: '' } }; + } + if (descriptor.type === ResourceType.NamedValue) { + return { properties: { displayName: 'Backend_Id', value: 'function-backend' } }; + } + return { properties: { url: 'https://example.test' } }; + }); + + const publish = (filter: FilterConfig) => runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + const backendPuts = () => client.putResource.mock.calls + .filter((call) => call[1].type === ResourceType.Backend) + .map((call) => call[1].nameParts[0]); + + await publish({ policyFragments: ['route'], namedValues: [], backends: [] }); + expect(backendPuts()).toEqual(['function-backend']); + }); + + it("should follow the overridden value of a named value a policy attribute is set to", async () => { + const resources = [ + { type: ResourceType.PolicyFragment, nameParts: ['route'] }, + { type: ResourceType.NamedValue, nameParts: ['backend-id'] }, + { type: ResourceType.Backend, nameParts: ['dev-backend'] }, + { type: ResourceType.Backend, nameParts: ['prod-backend'] }, + ]; + const client = createMockClient(); + const store = createMockStore(resources); + store.readResource.mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: '' } }; + } + if (descriptor.type === ResourceType.NamedValue) { + return { properties: { displayName: 'Backend_Id', value: 'dev-backend' } }; + } + return { properties: { url: 'https://example.test' } }; + }); + + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter: { policyFragments: ['route'], namedValues: [], backends: [] }, + overrides: { namedValues: { 'backend-id': { properties: { value: 'prod-backend' } } } }, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + + const backendPuts = client.putResource.mock.calls + .filter((call) => call[1].type === ResourceType.Backend) + .map((call) => call[1].nameParts[0]); + expect(backendPuts).toEqual(['prod-backend']); + }); + + it('should include the backend a changed named value now names when only the named value changed', async () => { + const namedValue = { type: ResourceType.NamedValue, nameParts: ['backend-id'] }; + const resources = [ + { type: ResourceType.Api, nameParts: ['orders'] }, + { type: ResourceType.ApiPolicy, nameParts: ['orders'] }, + namedValue, + { type: ResourceType.Backend, nameParts: ['old-backend'] }, + { type: ResourceType.Backend, nameParts: ['new-backend'] }, + ]; + vi.mocked(computeGitDiff).mockResolvedValue({ changedDescriptors: [namedValue], deletedDescriptors: [] }); + const client = createMockClient(); + const store = createMockStore(resources); + store.readContent.mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.ApiPolicy + ? { content: '' } + : undefined + ); + store.readResource.mockImplementation(async (_sourceDir, descriptor) => + descriptor.type === ResourceType.NamedValue + ? { properties: { displayName: 'Backend_Id', value: 'new-backend' } } + : { properties: {} } + ); + + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter: { apis: ['orders'], namedValues: ['backend-id'], backends: [] }, + commitId: 'abc123', + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + + const puts = client.putResource.mock.calls.map((call) => `${call[1].type}:${call[1].nameParts.join('/')}`).sort(); + expect(puts).toEqual(['Backend:new-backend', 'NamedValue:backend-id']); + }); + + describe('with a policy whose XML an override replaces', () => { + const servicePolicy: ResourceDescriptor = { type: ResourceType.ServicePolicy, nameParts: [] }; + const namedValue: ResourceDescriptor = { type: ResourceType.NamedValue, nameParts: ['backend-id'] }; + const overrides = { + policies: { policy: { properties: { value: '' } } }, + }; + const publishWith = async (filter: FilterConfig, commitId?: string) => { + const client = createMockClient(); + const store = createMockStore([servicePolicy, namedValue, { type: ResourceType.Backend, nameParts: ['new-backend'] }]); + store.readContent.mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.ServicePolicy ? { content: '' } : undefined + ); + store.readResource.mockImplementation(async (_sourceDir, descriptor) => + descriptor.type === ResourceType.NamedValue + ? { properties: { displayName: 'Backend_Id', value: 'new-backend' } } + : { properties: {} } + ); + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter, + overrides, + commitId, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + return client.putResource.mock.calls.map((call) => `${call[1].type}:${call[1].nameParts.join('/')}`).sort(); + }; + + it('should follow the overridden XML of a published policy', async () => { + expect(await publishWith({ namedValues: [], backends: [] })).toEqual([ + 'Backend:new-backend', + 'NamedValue:backend-id', + 'ServicePolicy:', + ]); + }); + + it('should include the backend a changed named value names in the overridden XML', async () => { + vi.mocked(computeGitDiff).mockResolvedValue({ changedDescriptors: [namedValue], deletedDescriptors: [] }); + + expect(await publishWith({ namedValues: ['backend-id'], backends: [] }, 'abc123')).toEqual([ + 'Backend:new-backend', + 'NamedValue:backend-id', + ]); + }); + }); + + it('should include the backend a changed named value names in a fragment a selected policy includes', async () => { + const namedValue = { type: ResourceType.NamedValue, nameParts: ['backend-id'] }; + const resources = [ + { type: ResourceType.Api, nameParts: ['orders'] }, + { type: ResourceType.ApiPolicy, nameParts: ['orders'] }, + { type: ResourceType.PolicyFragment, nameParts: ['route'] }, + namedValue, + { type: ResourceType.Backend, nameParts: ['new-backend'] }, + ]; + vi.mocked(computeGitDiff).mockResolvedValue({ changedDescriptors: [namedValue], deletedDescriptors: [] }); + const client = createMockClient(); + const store = createMockStore(resources); + store.readContent.mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.ApiPolicy + ? { content: '' } + : undefined + ); + store.readResource.mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: '' } }; + } + return descriptor.type === ResourceType.NamedValue + ? { properties: { displayName: 'Backend_Id', value: 'new-backend' } } + : { properties: {} }; + }); + + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter: { apis: ['orders'], namedValues: ['backend-id'], policyFragments: [], backends: [] }, + commitId: 'abc123', + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + + const puts = client.putResource.mock.calls.map((call) => `${call[1].type}:${call[1].nameParts.join('/')}`).sort(); + expect(puts).toEqual(['Backend:new-backend', 'NamedValue:backend-id']); + }); + + describe('a named value a policy outside the filter uses as a resource name', () => { + const setUp = (extra: ResourceDescriptor[] = [{ type: ResourceType.Backend, nameParts: ['billing-backend'] }]) => { + const resources = [ + { type: ResourceType.Api, nameParts: ['orders'] }, + { type: ResourceType.Api, nameParts: ['billing'] }, + { type: ResourceType.ApiPolicy, nameParts: ['billing'] }, + { type: ResourceType.NamedValue, nameParts: ['billing-id'] }, + ...extra, + ]; + const client = createMockClient(); + const store = createMockStore(resources); + store.readContent.mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.ApiPolicy + ? { content: '' } + : undefined + ); + store.readResource.mockImplementation(async (_sourceDir, descriptor) => + descriptor.type === ResourceType.NamedValue + ? { properties: { displayName: 'billing-id', value: 'billing-backend' } } + : { properties: {} } + ); + const publish = (filter: FilterConfig) => runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + const puts = () => client.putResource.mock.calls.map((call) => `${call[1].type}:${call[1].nameParts.join('/')}`).sort(); + return { publish, puts, store }; + }; + + it('should include the resource its value names, as APIM checks that policy on the target', async () => { + const { publish, puts } = setUp(); + + // Every named value is selected (the key is left out); backends: [] still lets dependencies in. + await publish({ apis: ['orders'], backends: [] }); + + expect(puts()).toEqual(expect.arrayContaining(['Backend:billing-backend', 'NamedValue:billing-id'])); + expect(puts()).not.toContain('Api:billing'); + }); + + it.each([ + ["it isn't in the source", { apis: ['orders'], backends: [] }, []], + ])('should not publish the resource when %s', async (_label, filter, extra) => { + const { publish, puts } = setUp(extra); + + await publish(filter as FilterConfig); + + expect(puts()).toContain('NamedValue:billing-id'); + expect(puts()).not.toContain('Backend:billing-backend'); + }); + }); + it('should include every dependency type supported by extract', async () => { const resources: ResourceDescriptor[] = [ { type: ResourceType.Api, nameParts: ['orders'] }, diff --git a/tests/unit/services/transitive-extractor.test.ts b/tests/unit/services/transitive-extractor.test.ts index 2b39d4ba..d322f028 100644 --- a/tests/unit/services/transitive-extractor.test.ts +++ b/tests/unit/services/transitive-extractor.test.ts @@ -355,3 +355,75 @@ describe('extractTransitiveDependencies with Liquid bodies', () => { ]); }); }); + +describe('policy attributes set to a named value (#331)', () => { + const policies = new Map([['api-1', '']]); + const backendId = { type: ResourceType.NamedValue, nameParts: ['backend-id'] }; + + it("should extract the backend named by the named value's value", async () => { + const client = createMockClient(); + client.listResources = namedValueList({ name: 'backend-id', displayName: 'Backend_Id' }); + client.getResource.mockImplementation(async (_ctx, descriptor: ResourceDescriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + return { name: 'backend-id', properties: { displayName: 'Backend_Id', value: 'function-backend' } }; + } + return descriptor.type === ResourceType.Backend ? { name: descriptor.nameParts[0], properties: {} } : undefined; + }); + + const result = await extractTransitiveDependencies( + client, createMockStore(), testContext, '/output', policies, new Map(), [], [] + ); + + expect(result.errorCount).toBe(0); + expect(result.extractedDescriptors).toEqual([ + expect.objectContaining(backendId), + expect.objectContaining({ type: ResourceType.Backend, nameParts: ['function-backend'] }), + ]); + }); + + it('should follow the named value a reference resolves to, not one the selection extracted under that name', async () => { + // `one` has the display name `two`, so {{two}} is `one`, not the + // selected `two`. + const policy = new Map([['api-1', '']]); + const two = { type: ResourceType.NamedValue, nameParts: ['two'] }; + const client = createMockClient(); + client.listResources = namedValueList({ name: 'one', displayName: 'two' }, { name: 'two', displayName: 'three' }); + client.getResource.mockImplementation(async (_ctx, descriptor: ResourceDescriptor) => { + if (descriptor.type === ResourceType.NamedValue && descriptor.nameParts[0] === 'one') { + return { name: 'one', properties: { displayName: 'two', value: 'real-backend' } }; + } + return descriptor.type === ResourceType.Backend && descriptor.nameParts[0] === 'real-backend' + ? { name: 'real-backend', properties: {} } + : undefined; + }); + + const result = await extractTransitiveDependencies( + client, createMockStore(), testContext, '/output', policy, new Map(), + [{ descriptor: two, json: { properties: { displayName: 'three', value: 'not-a-backend' } } }], + [two] + ); + + expect(result.errorCount).toBe(0); + expect(result.extractedDescriptors).toEqual([ + expect.objectContaining({ type: ResourceType.NamedValue, nameParts: ['one'] }), + expect.objectContaining({ type: ResourceType.Backend, nameParts: ['real-backend'] }), + ]); + expect(client.getResource.mock.calls.map((call) => call[1].nameParts[0])).not.toContain('not-a-backend'); + }); + + it('should follow a named value the selection already extracted', async () => { + const client = createMockClient(); + client.listResources = namedValueList({ name: 'backend-id', displayName: 'Backend_Id' }); + client.getResource.mockResolvedValue({ name: 'function-backend', properties: {} }); + + const result = await extractTransitiveDependencies( + client, createMockStore(), testContext, '/output', policies, new Map(), + [{ descriptor: backendId, json: { properties: { displayName: 'Backend_Id', value: 'function-backend' } } }], + [backendId] + ); + + expect(result.extractedDescriptors).toEqual([ + expect.objectContaining({ type: ResourceType.Backend, nameParts: ['function-backend'] }), + ]); + }); +}); diff --git a/tests/unit/services/transitive-resolver.test.ts b/tests/unit/services/transitive-resolver.test.ts index b4f0b637..3b261c84 100644 --- a/tests/unit/services/transitive-resolver.test.ts +++ b/tests/unit/services/transitive-resolver.test.ts @@ -8,6 +8,8 @@ import { describe, it, expect, vi } from 'vitest'; import { ResourceType } from '../../../src/models/resource-types.js'; import { FilterConfig } from '../../../src/models/config.js'; import { + createNamedValueReader, + scanNamedValueTargets, scanLiquidNamedValueCandidates, scanPolicyReferences, scanApiVersionSetReference, @@ -124,6 +126,15 @@ describe('transitive-resolver', () => { { type: ResourceType.NamedValue, name: 'not-liquid' }, ]); }); + + it('should not treat policy expressions or named value placeholders as resource names', () => { + const refs = scanPolicyReferences( + '' + + '' + ); + + expect(refs).toEqual([{ type: ResourceType.NamedValue, name: 'backend-name' }]); + }); }); describe('scanApiVersionSetReference', () => { @@ -281,6 +292,131 @@ describe('transitive-resolver', () => { ]); }); + describe('policy attributes set to a named value (#331)', () => { + const POLICY = ''; + const namedValue = (properties: Record, name = 'backend-id', workspace?: string) => ({ + descriptor: { type: ResourceType.NamedValue, nameParts: [name], workspace }, + json: { properties }, + }); + + it('should read each named value once, however many policies set attributes to it', async () => { + const descriptor = { type: ResourceType.NamedValue, nameParts: ['backend-id'] }; + const readResource = vi.fn(async () => ({ properties: { value: 'function-backend' } })); + const read = createNamedValueReader(async () => descriptor, readResource); + + const results = await Promise.all([read(descriptor), read(descriptor), read({ ...descriptor, nameParts: ['BACKEND-ID'] })]); + + expect(results.map((result) => result?.json)).toEqual(Array(3).fill({ properties: { value: 'function-backend' } })); + expect(readResource).toHaveBeenCalledTimes(1); + }); + + it('should find attributes whose whole value is a named value reference', () => { + expect(scanNamedValueTargets( + '' + + '' + + '' + + '' + + '' + )).toEqual([ + { type: ResourceType.Backend, namedValue: 'Backend_Id' }, + { type: ResourceType.Backend, namedValue: 'safety' }, + ]); + }); + + it("should follow the named value's value to the resource it names, matching the display name", () => { + const deps = findTransitiveDependencies(new Map([['p', POLICY]]), new Map(), undefined, [ + namedValue({ displayName: 'backend_id', value: ' function-backend ' }), + ]); + + expect(deps).toContainEqual({ type: ResourceType.Backend, nameParts: ['function-backend'], workspace: undefined }); + }); + + it('should follow only the named value with that display name, not one with that resource name', () => { + const byName = findTransitiveDependencies(new Map([['p', POLICY]]), new Map(), undefined, [ + namedValue({ displayName: 'Other', value: 'by-name' }, 'Backend_Id'), + ]); + const byDisplayName = findTransitiveDependencies(new Map([['p', POLICY]]), new Map(), undefined, [ + namedValue({ displayName: 'Other', value: 'by-name' }, 'Backend_Id'), + namedValue({ displayName: 'Backend_Id', value: 'by-display-name' }, 'nv-1'), + ]); + + expect(byName.filter((dep) => dep.type === ResourceType.Backend)).toEqual([]); + expect(byDisplayName.filter((dep) => dep.type === ResourceType.Backend)).toEqual([ + { type: ResourceType.Backend, nameParts: ['by-display-name'], workspace: undefined }, + ]); + }); + + it.each([ + ['a secret', { displayName: 'Backend_Id', value: '***REDACTED***', secret: true }], + ['a policy expression', { displayName: 'Backend_Id', value: '@(context.Variables["b"])' }], + ['a nested reference', { displayName: 'Backend_Id', value: '{{other}}' }], + ])('should not follow %s named value', (_kind, properties) => { + const deps = findTransitiveDependencies(new Map([['p', POLICY]]), new Map(), undefined, [namedValue(properties)]); + + expect(deps.filter((dep) => dep.type === ResourceType.Backend)).toEqual([]); + }); + + it("should only match a named value in the policy's own scope", () => { + const deps = findTransitiveDependencies(new Map([['p', POLICY]]), new Map(), 'team-a', [ + namedValue({ displayName: 'Backend_Id', value: 'service-backend' }), + namedValue({ displayName: 'Backend_Id', value: 'ws-backend' }, 'backend-id', 'team-a'), + ]); + + expect(deps.filter((dep) => dep.type === ResourceType.Backend)).toEqual([ + { type: ResourceType.Backend, nameParts: ['ws-backend'], workspace: 'team-a' }, + ]); + }); + + it('should follow one in a policy fragment', () => { + const deps = findTransitiveDependencies(new Map(), new Map(), undefined, [ + { descriptor: { type: ResourceType.PolicyFragment, nameParts: ['route'] }, json: { properties: { value: POLICY } } }, + namedValue({ displayName: 'Backend_Id', value: 'function-backend' }), + ]); + + expect(deps).toContainEqual({ type: ResourceType.Backend, nameParts: ['function-backend'], workspace: undefined }); + }); + + it('should read the named value when scanning an artifact, given a reader', async () => { + const store = { + readResource: vi.fn(), + readContent: vi.fn().mockResolvedValue({ content: POLICY, format: 'xml' }), + readAssociation: vi.fn(), + }; + const readNamedValue = vi.fn().mockResolvedValue( + namedValue({ displayName: 'Backend_Id', value: 'ws-backend' }, 'backend-id', 'team-a') + ); + const policy = { type: ResourceType.ApiPolicy, nameParts: ['orders'], workspace: 'team-a' }; + + const withReader = await scanArtifactReferences(store, '/source', policy, readNamedValue); + const withoutReader = await scanArtifactReferences(store, '/source', policy); + + expect(readNamedValue).toHaveBeenCalledWith({ type: ResourceType.NamedValue, nameParts: ['Backend_Id'], workspace: 'team-a' }); + expect(withReader).toEqual([ + { type: ResourceType.NamedValue, nameParts: ['Backend_Id'], workspace: 'team-a' }, + { type: ResourceType.Backend, nameParts: ['ws-backend'], workspace: 'team-a' }, + ]); + expect(withoutReader).toEqual([{ type: ResourceType.NamedValue, nameParts: ['Backend_Id'], workspace: 'team-a' }]); + }); + + it('should follow the named value the reader resolves a reference to, though an override renamed it', async () => { + const store = { + readResource: vi.fn(), + readContent: vi.fn().mockResolvedValue({ content: POLICY, format: 'xml' }), + readAssociation: vi.fn(), + }; + // The reader matched {{Backend_Id}}, the source display name, to a + // named value an override renames to Prod_Backend_Id. + const readNamedValue = vi.fn().mockResolvedValue( + namedValue({ displayName: 'Prod_Backend_Id', value: 'prod-backend' }, 'backend-id', 'team-a') + ); + const policy = { type: ResourceType.ApiPolicy, nameParts: ['orders'], workspace: 'team-a' }; + + const references = await scanArtifactReferences(store, '/source', policy, readNamedValue); + + expect(references).toContainEqual({ type: ResourceType.Backend, nameParts: ['prod-backend'], workspace: 'team-a' }); + }); + }); + describe('scanArtifactReferences', () => { it('scans policy references without parsing policy XML as JSON', async () => { const store = { From 7ed773d037cd428991374c55a60f3c3b9b627b18 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 12:20:16 +1100 Subject: [PATCH 09/18] fix: follow a backend or fragment however the policy attribute naming it is written --- src/services/transitive-resolver.ts | 35 +++++++------------ .../unit/services/transitive-resolver.test.ts | 11 ++++++ 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index 3cc66cde..d3e809ed 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -26,8 +26,8 @@ import { runParallel } from '../lib/parallel-runner.js'; * Reference detection patterns for policy XML content. */ const NAMED_VALUE_PATTERN = /\{\{([^}]+)\}\}/g; -const BACKEND_PATTERN = / = new Set([ResourceType.Backend, ResourceType.PolicyFragment]); /** * One XML attribute (`name="value"` or `name='value'`) with its leading @@ -180,10 +180,11 @@ const POLICY_RESOURCE_TYPES = new Set([ /** * Scan policy XML content for references to other resources. * - * Detects: + * Detects, with the attribute anywhere on the element and in either quote: * - Named values: {{namedValueName}} syntax - * - Backends: - * - Policy fragments: + * - Backends: , , + * and embeddings-backend-id on the semantic cache lookup policies + * - Policy fragments: * * References inside XML comments, and `{{ ... }}` inside a * `` body, are ignored: most of those are Liquid @@ -205,24 +206,12 @@ export function scanPolicyReferences(policyXml: string): TransitiveDependency[] } } - // Backend references. A value that is a policy expression or a named value - // reference names no resource here (see scanNamedValueTargets). - for (const match of scannable.matchAll(BACKEND_PATTERN)) { - if (match[1] && !match[1].startsWith('@') && !match[1].includes('{{')) { - dependencies.push({ - type: ResourceType.Backend, - name: match[1].trim(), - }); - } - } - - // Policy fragment references - for (const match of scannable.matchAll(FRAGMENT_PATTERN)) { - if (match[1] && !match[1].startsWith('@') && !match[1].includes('{{')) { - dependencies.push({ - type: ResourceType.PolicyFragment, - name: match[1].trim(), - }); + // Backends and fragments named by an attribute (POLICY_REFERENCE_PATTERNS). + // A value that is a policy expression or a named value reference names no + // resource here (see scanNamedValueTargets). + for (const { type, value } of scanReferenceAttributes(scannable)) { + if (FOLLOWED_ATTRIBUTE_TYPES.has(type) && !value.startsWith('@') && !value.includes('{{')) { + dependencies.push({ type, name: value }); } } diff --git a/tests/unit/services/transitive-resolver.test.ts b/tests/unit/services/transitive-resolver.test.ts index 3b261c84..a7177797 100644 --- a/tests/unit/services/transitive-resolver.test.ts +++ b/tests/unit/services/transitive-resolver.test.ts @@ -86,6 +86,17 @@ describe('transitive-resolver', () => { expect(refs[0]?.name).toBe('my-secret'); }); + it.each([ + ['an attribute before backend-id', '', ResourceType.Backend, 'func-backend'], + ['single quotes', "", ResourceType.Backend, 'quoted-backend'], + ['llm-content-safety', '', ResourceType.Backend, 'content-safety'], + ['azure-openai-semantic-cache-lookup', '', ResourceType.Backend, 'embeddings'], + ['llm-semantic-cache-lookup', '', ResourceType.Backend, 'llm-embeddings'], + ['include-fragment with another attribute first', '', ResourceType.PolicyFragment, 'frag-a'], + ])('should detect a reference written with %s', (_label, policy, type, name) => { + expect(scanPolicyReferences(policy)).toContainEqual({ type, name }); + }); + it('should ignore references inside XML comments (#305)', () => { const refs = scanPolicyReferences( '' + From e5444bb7b0ecb715340fa6036b66c9badb7b8cd4 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 11:16:36 +1100 Subject: [PATCH 10/18] fix: publish a named value after the resource it names, and what uses it in credentials after it --- docs/reference/dependency-graph.md | 2 +- src/services/publish-service.ts | 256 ++++++++++- src/services/transitive-resolver.ts | 12 + tests/unit/services/publish-service.test.ts | 465 ++++++++++++++++++++ 4 files changed, 723 insertions(+), 12 deletions(-) diff --git a/docs/reference/dependency-graph.md b/docs/reference/dependency-graph.md index bb45884a..de0a0ba2 100644 --- a/docs/reference/dependency-graph.md +++ b/docs/reference/dependency-graph.md @@ -15,7 +15,7 @@ apiops-cli solves this by organizing all 34 resource types into **4 tiers**. Eac | Tier | Resources | Rule | |------|-----------|------| -| **1** | NamedValue, Tag, Gateway, VersionSet, Backend, Logger, Group, PolicyFragment, GlobalSchema, PolicyRestriction, Documentation | No dependencies — safe to process in any order | +| **1** | NamedValue, Tag, Gateway, VersionSet, Backend, Logger, Group, PolicyFragment, GlobalSchema, PolicyRestriction, Documentation | No dependencies on other tiers. Within the tier, named values go first, since backends and loggers can use them; pool backends follow the backends they contain; and a named value a policy uses to name a backend, logger or schema published in the same run goes after that resource, with policy fragments after it in that case. A backend or logger in the run that uses such a named value in its credentials goes after the named value, and these are ordered so each comes after what it needs; only where that is impossible (a cycle, as when a backend uses in its credentials the named value that names it, or a pool containing it), the named values in it go first | | **2** | Diagnostic, ServicePolicy, Product, Api | Depend only on Tier 1 resources | | **3** | ProductPolicy, ProductGroup, ProductTag, ProductApi, ProductWiki, ApiPolicy, ApiTag, ApiDiagnostic, ApiOperation, ApiSchema, ApiRelease, ApiTagDescription, ApiWiki, McpServer, GraphQLResolver, GatewayApi, Subscription | Depend on Tier 1 and/or Tier 2 resources | | **4** | ApiOperationPolicy, GraphQLResolverPolicy | Depend on Tier 3 resources | diff --git a/src/services/publish-service.ts b/src/services/publish-service.ts index 53a8a162..ba0e0823 100644 --- a/src/services/publish-service.ts +++ b/src/services/publish-service.ts @@ -54,6 +54,7 @@ import { REDACTION_MARKER } from './secret-redactor.js'; import { createArtifactReferenceResolver, createNamedValueReader, + findTransitiveDependencies, indexNamedValueTargetTypes, REFERENCE_SCAN_CONCURRENCY, scanArtifactNamedValueReferences, @@ -269,7 +270,8 @@ async function runPublishPipeline( store, config.service, config, - targetDescriptors + targetDescriptors, + sourceReferences ); // Step 4: Execute DELETEs in reverse dependency order (tier 4 → tier 1) if requested @@ -656,7 +658,8 @@ async function executePuts( store: IArtifactStore, context: ApimServiceContext, config: PublishConfig, - targetDescriptors: ResourceDescriptor[] + targetDescriptors: ResourceDescriptor[], + sourceReferences: SourceReferences ): Promise { const results: PublishActionResult[] = []; @@ -710,33 +713,65 @@ async function executePuts( await publishAndOutput(client, store, context, config, workspaces, targetDescriptors, results); } - // Within Tier 1 we publish in three ordered waves to satisfy implicit + // Within Tier 1 we publish in ordered waves to satisfy implicit // runtime dependencies: // - // Wave 1 — NamedValues only. + // Wave 1 — Named values, except those in wave 4. // Loggers and backends may reference named values by name in their // configuration (e.g. AppInsights instrumentation key stored as a // named value). Publishing named values first ensures they exist // before any resource that references them. // - // Wave 2 — All remaining Tier 1 resources except pool backends. - // Loggers, tags, gateways, etc. can now reference named values safely. + // Wave 2 — The remaining Tier 1 resources except pool backends and + // wave 4 (and except policy fragments when wave 4 has any). // // Wave 3 — Pool backends. // Pool backends reference individual Backend resources and must be // published after the backends they aggregate (see pool backend // ordering comments in splitPoolBackends). - if (namedValues.length > 0) { - logger.debug(`Publishing ${namedValues.length} named value(s) first (wave 1 of tier 1)`); - await publishAndOutput(client, store, context, config, namedValues, targetDescriptors, results); + // + // Wave 4 — Named values a policy uses as a resource name + // (backend-id="{{name}}") whose value names a backend, logger or + // schema published here, after that resource: APIM checks the + // policies that use a named value when it changes, so that resource + // must exist first. With them, in an order where each goes after what + // it needs, the backends and loggers whose credentials use them and + // the pools those are members of (see + // splitNamedValuesNamingResources). + // + // Wave 5 — Policy fragments, when wave 4 has resources: a fragment can + // use its named values, and APIM rejects a policy that uses a + // missing one. + const { early: earlyNamedValues, ordered } = await splitNamedValuesNamingResources( + store, + config, + namedValues, + otherTier1, + sourceReferences + ); + if (earlyNamedValues.length > 0) { + logger.debug(`Publishing ${earlyNamedValues.length} named value(s) first (wave 1 of tier 1)`); + await publishAndOutput(client, store, context, config, earlyNamedValues, targetDescriptors, results); } + const inOrdered = new Set(ordered.flat()); const { poolBackends, regularTier1 } = await splitPoolBackends( store, config.sourceDir, - otherTier1 + otherTier1.filter((descriptor) => !inOrdered.has(descriptor)) + ); + const fragments = inOrdered.size > 0 + ? regularTier1.filter((descriptor) => descriptor.type === ResourceType.PolicyFragment) + : []; + await publishAndOutput( + client, + store, + context, + config, + regularTier1.filter((descriptor) => !fragments.includes(descriptor)), + targetDescriptors, + results ); - await publishAndOutput(client, store, context, config, regularTier1, targetDescriptors, results); if (poolBackends.length > 0) { logger.debug( `Publishing ${poolBackends.length} pool backend(s) after regular backends` @@ -751,6 +786,17 @@ async function executePuts( results ); } + if (inOrdered.size > 0) { + logger.debug( + `Publishing ${inOrdered.size} named value(s) and resource(s) in the order the named values' values need` + ); + } + for (const wave of ordered) { + await publishAndOutput(client, store, context, config, wave, targetDescriptors, results); + } + if (fragments.length > 0) { + await publishAndOutput(client, store, context, config, fragments, targetDescriptors, results); + } } else if (tier === 2) { const tier2Descriptors = filterApiRevisionsHandledByRootApis(descriptors); if (tier2Descriptors.length === 0) continue; @@ -962,6 +1008,194 @@ function splitNamedValues( return { namedValues, otherTier1 }; } +/** + * The Tier 1 order around named values a policy or policy fragment in the + * source uses as the whole value of a reference attribute + * (backend-id="{{name}}", logger-id, schema-id), and whose value (with + * overrides) names a resource of that type published in this tier. APIM + * checks the policies that use a named value when the named value changes, + * rejecting the change ("its value breaks policies") while the resource it + * now names doesn't exist, so such a named value goes after that resource. + * + * `early` named values go before the rest of Tier 1. `ordered` is published + * in turn after it, wave by wave: the late named values, the backends and + * loggers whose credentials use them (after them), the resources those name + * or pools those are members of, in an order where each comes after what it + * needs. Where there is none (a cycle, as when a backend uses in its + * credentials the named value that names it), the named values in it go + * early instead. `ordered` resources other than named values are not + * published in the earlier waves. + * + * The policies are read (once) only when a named value's value matches the + * name of such a resource here; a named value that can't be read goes first, + * and publishing it reports the error. + */ +async function splitNamedValuesNamingResources( + store: IArtifactStore, + config: PublishConfig, + namedValues: ResourceDescriptor[], + otherTier1: ResourceDescriptor[], + sourceReferences: SourceReferences +): Promise<{ early: ResourceDescriptor[]; ordered: ResourceDescriptor[][] }> { + const resourceKey = (type: ResourceType, name: string, workspace: string | undefined) => + `${type}/${workspace ?? ''}/${name.toLowerCase()}`; + const keyOf = (descriptor: ResourceDescriptor) => + resourceKey(descriptor.type, getNamePart(descriptor.nameParts, 0), descriptor.workspace); + const byResourceKey = new Map(otherTier1.map((descriptor) => [keyOf(descriptor), descriptor])); + const named = new Set( + otherTier1.filter((descriptor) => NAMED_THROUGH_NAMED_VALUE_TYPES.has(descriptor.type)).map(keyOf) + ); + const early: ResourceDescriptor[] = []; + const late: ResourceDescriptor[] = []; + if (named.size === 0) { + return { early: namedValues, ordered: [] }; + } + // The resources each late named value names, which go before it. + const targetsOf = new Map(); + // The named values policies use as resource names, and as which types. + // Every policy in the source counts, not only the ones the filter selects: + // APIM checks every policy on the target that uses the named value, and + // this only decides the order, not what is published. + const typesFor = async (namedValue: ResourceDescriptor): Promise | undefined> => + (await sourceReferences.namedValueTargets()).get(getResourceDescriptorKey(namedValue)); + for (const descriptor of namedValues) { + let targets: string[] = []; + try { + const json = await store.readResource(config.sourceDir, descriptor); + const name = json ? namedValueTargetName(applyOverrides(descriptor, json, config.overrides)) : undefined; + if (name && [...NAMED_THROUGH_NAMED_VALUE_TYPES].some((type) => named.has(resourceKey(type, name, descriptor.workspace)))) { + targets = [...(await typesFor(descriptor) ?? [])] + .map((type) => resourceKey(type, name, descriptor.workspace)) + .filter((target) => named.has(target)); + } + } catch { + targets = []; + } + if (targets.length > 0) { + late.push(descriptor); + targetsOf.set(descriptor, targets); + } else { + early.push(descriptor); + } + } + if (late.length === 0) { + return { early, ordered: [] }; + } + + // The backends and loggers whose own configuration (their credentials) + // uses a late named value, and the pools each backend is a member of. + const resolve = sourceReferences.resolve; + const lateByKey = new Map(late.map((descriptor) => [getResourceDescriptorKey(descriptor), descriptor])); + const usesOf = new Map(); + const poolsOf = new Map(); + for (const descriptor of otherTier1) { + if (descriptor.type !== ResourceType.Backend && descriptor.type !== ResourceType.Logger) { + continue; + } + let json: Record | undefined; + try { + json = await store.readResource(config.sourceDir, descriptor); + } catch { + continue; + } + if (!json) { + continue; + } + const resource = { descriptor, json: applyOverrides(descriptor, json, config.overrides) }; + const uses: ResourceDescriptor[] = []; + for (const dependency of findTransitiveDependencies(new Map(), new Map(), descriptor.workspace, [resource])) { + if (dependency.type === ResourceType.Backend) { + // A pool's member. + const memberKey = keyOf(dependency); + poolsOf.set(memberKey, [...(poolsOf.get(memberKey) ?? []), descriptor]); + continue; + } + const target = dependency.type === ResourceType.NamedValue ? await resolve(dependency) : undefined; + const used = target ? lateByKey.get(getResourceDescriptorKey(target)) : undefined; + if (used) { + uses.push(used); + } + } + if (uses.length > 0) { + usesOf.set(descriptor, uses); + } + } + + // What is ordered: the late named values, their credential users, and the + // pools those users are members of (a pool goes after its members). + const nodes = new Set([...late, ...usesOf.keys()]); + for (const node of [...nodes]) { + if (node.type === ResourceType.Backend) { + (poolsOf.get(keyOf(node)) ?? []).forEach((pool) => nodes.add(pool)); + } + } + // Each node's predecessors among the nodes. + const before = new Map>([...nodes].map((node) => [node, new Set()])); + for (const [user, uses] of usesOf) { + uses.forEach((used) => before.get(user)?.add(used)); + } + for (const [namedValue, targets] of targetsOf) { + for (const target of targets) { + const resource = byResourceKey.get(target); + if (resource && nodes.has(resource)) { + before.get(namedValue)?.add(resource); + } + } + } + for (const node of nodes) { + if (node.type === ResourceType.Backend) { + (poolsOf.get(keyOf(node)) ?? []).forEach((pool) => before.get(pool)?.add(node)); + } + } + + const ordered: ResourceDescriptor[][] = []; + const remaining = new Set(nodes); + // Whether a node needs itself through what it needs among `remaining`. + const inCycle = (start: ResourceDescriptor): boolean => { + const seen = new Set(); + const pending = [...(before.get(start) ?? [])]; + for (let node = pending.pop(); node; node = pending.pop()) { + if (node === start) { + return true; + } + if (remaining.has(node) && !seen.has(node)) { + seen.add(node); + pending.push(...(before.get(node) ?? [])); + } + } + return false; + }; + while (remaining.size > 0) { + const wave = [...remaining].filter((node) => [...(before.get(node) ?? [])].every((needed) => !remaining.has(needed))); + if (wave.length === 0) { + // A cycle: the named values in one go early, before what they name, + // and what needs them is still ordered after them. + const cyclic = [...remaining].filter((node) => node.type === ResourceType.NamedValue && inCycle(node)); + if (cyclic.length === 0) { + // None to break it with: publish the rest together, and APIM + // reports what it rejects. + ordered.push([...remaining]); + break; + } + for (const node of cyclic) { + remaining.delete(node); + early.push(node); + } + continue; + } + wave.forEach((node) => remaining.delete(node)); + ordered.push(wave); + } + return { early, ordered }; +} + +/** Tier 1 types a policy attribute can name through a named value. */ +const NAMED_THROUGH_NAMED_VALUE_TYPES: ReadonlySet = new Set([ + ResourceType.Backend, + ResourceType.Logger, + ResourceType.GlobalSchema, +]); + /** * Separates Backend descriptors that are pool backends (properties.type === "Pool") * from all other descriptors in the supplied list. diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index d3e809ed..c3357250 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -177,6 +177,15 @@ const POLICY_RESOURCE_TYPES = new Set([ ResourceType.GraphQLResolverPolicy, ]); +/** The artifact types scanArtifactReferences finds references in; the rest hold none. */ +const REFERENCING_TYPES = new Set([ + ...POLICY_RESOURCE_TYPES, + ResourceType.Api, + ResourceType.Logger, + ResourceType.Backend, + ResourceType.PolicyFragment, +]); + /** * Scan policy XML content for references to other resources. * @@ -536,6 +545,9 @@ export async function scanArtifactReferences( readNamedValue?: NamedValueReader, prepareJson?: PrepareJson ): Promise { + if (!REFERENCING_TYPES.has(descriptor.type)) { + return []; + } const references: ResourceDescriptor[] = []; const policies = new Map(); const apis = new Map>(); diff --git a/tests/unit/services/publish-service.test.ts b/tests/unit/services/publish-service.test.ts index 4fcf47fc..865df60c 100644 --- a/tests/unit/services/publish-service.test.ts +++ b/tests/unit/services/publish-service.test.ts @@ -588,6 +588,17 @@ describe('publish-service', () => { return { publish, puts, store }; }; + it('should read that policy once, though both the expansion and the order use it', async () => { + const { publish, store } = setUp(); + + await publish({ apis: ['orders'], backends: [] }); + + const billingPolicyReads = store.readContent.mock.calls.filter( + (call) => call[1].type === ResourceType.ApiPolicy && call[1].nameParts[0] === 'billing' + ); + expect(billingPolicyReads).toHaveLength(1); + }); + it('should include the resource its value names, as APIM checks that policy on the target', async () => { const { publish, puts } = setUp(); @@ -2099,6 +2110,460 @@ describe('publish-service', () => { }); describe('named value ordering within tier 1', () => { + const orderingCase = async ( + extra: ResourceDescriptor[], + policy: string, + overrides?: PublishConfig['overrides'] + ): Promise => { + const publishOrder: string[] = []; + const resources: ResourceDescriptor[] = [ + { type: ResourceType.NamedValue, nameParts: ['backend-id'] }, + { type: ResourceType.NamedValue, nameParts: ['api-key'] }, + { type: ResourceType.Backend, nameParts: ['prod-backend'] }, + { type: ResourceType.Backend, nameParts: ['dev-backend'] }, + { type: ResourceType.PolicyFragment, nameParts: ['route'] }, + ...extra, + ]; + const client = createMockClient(); + vi.mocked(client.putResource).mockImplementation(async (_ctx, descriptor) => { + publishOrder.push(descriptor.nameParts.join('/')); + }); + const store = createMockStore(resources); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + const value = { 'backend-id': 'dev-backend', 'api-key': 'prod-backend', 'schema-id': 'order-schema' }[descriptor.nameParts[0] ?? '']; + return { properties: { displayName: descriptor.nameParts[0], value } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: policy } }; + } + return { properties: { url: 'https://example.test' } }; + }); + + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + overrides, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + return publishOrder; + }; + + it('should publish a named value a policy uses as a backend after the backend its value names, and before fragments', async () => { + const order = await orderingCase( + [], + '', + { namedValues: { 'backend-id': { properties: { value: 'prod-backend' } } } } + ); + + expect(order.indexOf('backend-id')).toBeGreaterThan(order.indexOf('prod-backend')); + expect(order.indexOf('backend-id')).toBeGreaterThan(order.indexOf('dev-backend')); + // The fragment uses the named value, so it goes after it. + expect(order.indexOf('route')).toBeGreaterThan(order.indexOf('backend-id')); + // api-key's value matches a backend's name, but no policy uses it as one. + expect(order.indexOf('api-key')).toBeLessThan(order.indexOf('prod-backend')); + }); + + it('should publish a named value a policy uses as a schema after the schema its value names', async () => { + const order = await orderingCase( + [ + { type: ResourceType.NamedValue, nameParts: ['schema-id'] }, + { type: ResourceType.GlobalSchema, nameParts: ['order-schema'] }, + ], + '' + ); + + expect(order.indexOf('schema-id')).toBeGreaterThan(order.indexOf('order-schema')); + expect(order.indexOf('backend-id')).toBeLessThan(order.indexOf('dev-backend')); + }); + + const recordOrder = (resources: ResourceDescriptor[]) => { + const order: string[] = []; + const client = createMockClient(); + vi.mocked(client.putResource).mockImplementation(async (_ctx, descriptor) => { + order.push(descriptor.nameParts.join('/')); + }); + return { order, client, store: createMockStore(resources) }; + }; + const publishAll = (client: ReturnType, store: ReturnType, filter?: FilterConfig) => + runPublish(client, store, { + service: testContext, + sourceDir: '/source', + filter, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + + it('should publish a named value naming a pool backend after the pool', async () => { + const { order, client, store } = recordOrder([ + { type: ResourceType.NamedValue, nameParts: ['pool-id'] }, + { type: ResourceType.Backend, nameParts: ['member'] }, + { type: ResourceType.Backend, nameParts: ['my-pool'] }, + { type: ResourceType.PolicyFragment, nameParts: ['route'] }, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + return { properties: { displayName: 'pool-id', value: 'my-pool' } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: '' } }; + } + return descriptor.nameParts[0] === 'my-pool' + ? { properties: { type: 'Pool', pool: { services: [{ id: '/backends/member' }] } } } + : { properties: { url: 'https://example.test' } }; + }); + + await publishAll(client, store); + + expect(order.indexOf('my-pool')).toBeGreaterThan(order.indexOf('member')); + expect(order.indexOf('pool-id')).toBeGreaterThan(order.indexOf('my-pool')); + expect(order.indexOf('route')).toBeGreaterThan(order.indexOf('pool-id')); + }); + + it("should keep a named value first when a backend published with it uses it in its credentials", async () => { + const { order, client, store } = recordOrder([ + { type: ResourceType.NamedValue, nameParts: ['shared'] }, + { type: ResourceType.Backend, nameParts: ['prod-backend'] }, + { type: ResourceType.PolicyFragment, nameParts: ['route'] }, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + return { properties: { displayName: 'shared', value: 'prod-backend' } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: '' } }; + } + return { properties: { url: 'https://example.test', credentials: { header: { 'x-route': ['{{shared}}'] } } } }; + }); + + await publishAll(client, store); + + expect(order.indexOf('shared')).toBeLessThan(order.indexOf('prod-backend')); + }); + + it('should publish a backend using a named value in its credentials after the named value and the backend it names', async () => { + const { order, client, store } = recordOrder([ + { type: ResourceType.NamedValue, nameParts: ['route'] }, + { type: ResourceType.Backend, nameParts: ['new-backend'] }, + { type: ResourceType.Backend, nameParts: ['caller'] }, + { type: ResourceType.PolicyFragment, nameParts: ['frag'] }, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + return { properties: { displayName: 'route', value: 'new-backend' } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: '' } }; + } + return descriptor.nameParts[0] === 'caller' + ? { properties: { url: 'https://example.test', credentials: { header: { 'x-route': ['{{route}}'] } } } } + : { properties: { url: 'https://example.test' } }; + }); + + await publishAll(client, store); + + expect(order.indexOf('route')).toBeGreaterThan(order.indexOf('new-backend')); + expect(order.indexOf('caller')).toBeGreaterThan(order.indexOf('route')); + expect(order.indexOf('frag')).toBeGreaterThan(order.indexOf('route')); + }); + + it('should order by the display name an override gives a named value', async () => { + const { order, client, store } = recordOrder([ + { type: ResourceType.NamedValue, nameParts: ['route'] }, + { type: ResourceType.Backend, nameParts: ['new-backend'] }, + { type: ResourceType.Backend, nameParts: ['caller'] }, + { type: ResourceType.PolicyFragment, nameParts: ['frag'] }, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + return { properties: { displayName: 'route', value: 'new-backend' } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: '' } }; + } + return descriptor.nameParts[0] === 'caller' + ? { properties: { url: 'https://example.test', credentials: { header: { 'x-route': ['{{prod-route}}'] } } } } + : { properties: { url: 'https://example.test' } }; + }); + + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + overrides: { namedValues: { route: { properties: { displayName: 'prod-route' } } } }, + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }); + + expect(order.indexOf('route')).toBeGreaterThan(order.indexOf('new-backend')); + expect(order.indexOf('caller')).toBeGreaterThan(order.indexOf('route')); + }); + + it('should read the source once for both the expansion and the order', async () => { + const { client, store } = recordOrder([ + { type: ResourceType.NamedValue, nameParts: ['route'] }, + { type: ResourceType.NamedValue, nameParts: ['other'] }, + { type: ResourceType.Backend, nameParts: ['new-backend'] }, + { type: ResourceType.Backend, nameParts: ['caller'] }, + { type: ResourceType.PolicyFragment, nameParts: ['frag'] }, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + return { properties: { displayName: descriptor.nameParts[0], value: 'new-backend' } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: '' } }; + } + return descriptor.nameParts[0] === 'caller' + ? { properties: { url: 'https://example.test', credentials: { header: { 'x-route': ['{{route}}'] } } } } + : { properties: { url: 'https://example.test' } }; + }); + + await publishAll(client, store, { policyFragments: ['frag'], backends: ['caller'] }); + + // Listed for the publish set, then once for the references. + expect(store.listResources).toHaveBeenCalledTimes(2); + // A named value no reference resolves to is read once to match display + // names, and otherwise only where it is published itself. + const otherReads = vi.mocked(store.readResource).mock.calls.filter((call) => call[1].nameParts[0] === 'other'); + expect(otherReads).toHaveLength(4); + }); + + it('should order a chain of named values and the backends using them in their credentials', async () => { + const { order, client, store } = recordOrder([ + { type: ResourceType.NamedValue, nameParts: ['route-a'] }, + { type: ResourceType.NamedValue, nameParts: ['route-b'] }, + { type: ResourceType.Backend, nameParts: ['new-a'] }, + { type: ResourceType.Backend, nameParts: ['new-b'] }, + { type: ResourceType.PolicyFragment, nameParts: ['frag'] }, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + const name = descriptor.nameParts[0]; + return { properties: { displayName: name, value: name === 'route-a' ? 'new-a' : 'new-b' } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + return { + properties: { + value: '', + }, + }; + } + return descriptor.nameParts[0] === 'new-a' + ? { properties: { url: 'https://example.test', credentials: { header: { 'x-route': ['{{route-b}}'] } } } } + : { properties: { url: 'https://example.test' } }; + }); + + await publishAll(client, store); + + expect(order.indexOf('route-b')).toBeGreaterThan(order.indexOf('new-b')); + expect(order.indexOf('new-a')).toBeGreaterThan(order.indexOf('route-b')); + expect(order.indexOf('route-a')).toBeGreaterThan(order.indexOf('new-a')); + expect(order.indexOf('frag')).toBeGreaterThan(order.indexOf('route-a')); + }); + + it('should move only the named values in a cycle first, and order what needs them', async () => { + // backend-a uses {{a}}, which names backend-a: a cycle. backend-b also + // uses {{a}}, and {{b}} names backend-b, which is not in it. + const { order, client, store } = recordOrder([ + { type: ResourceType.NamedValue, nameParts: ['a'] }, + { type: ResourceType.NamedValue, nameParts: ['b'] }, + { type: ResourceType.Backend, nameParts: ['backend-a'] }, + { type: ResourceType.Backend, nameParts: ['backend-b'] }, + { type: ResourceType.PolicyFragment, nameParts: ['frag'] }, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + const name = descriptor.nameParts[0]; + return { properties: { displayName: name, value: `backend-${name}` } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + return { + properties: { + value: '', + }, + }; + } + return { properties: { url: 'https://example.test', credentials: { header: { 'x-key': ['{{a}}'] } } } }; + }); + + await publishAll(client, store); + + expect(order.indexOf('a')).toBeLessThan(order.indexOf('backend-a')); + expect(order.indexOf('backend-b')).toBeGreaterThan(order.indexOf('a')); + expect(order.indexOf('b')).toBeGreaterThan(order.indexOf('backend-b')); + expect(order.indexOf('frag')).toBeGreaterThan(order.indexOf('b')); + }); + + describe('on generated graphs', () => { + // A small seeded generator (mulberry32), so a failure names the seed + // that reproduces it. + const random = (seed: number) => () => { + seed = (seed + 0x6d2b79f5) | 0; + let t = Math.imul(seed ^ (seed >>> 15), 1 | seed); + t = (t + Math.imul(t ^ (t >>> 7), 61 | t)) ^ t; + return ((t ^ (t >>> 14)) >>> 0) / 4294967296; + }; + const pick = (next: () => number, items: T[]): T => items[Math.floor(next() * items.length)]; + const some = (next: () => number, items: T[], chance: number): T[] => items.filter(() => next() < chance); + + interface Graph { + values: Map; + credentials: Map; + members: Map; + usedAsBackend: string[]; + } + + // Named values v*, whose values name a backend or pool, or nothing; + // backends b* using some named values in their credentials; pools p* + // of some backends; and a fragment using some named values as backend-id. + const generate = (seed: number): Graph => { + const next = random(seed); + const namedValues = Array.from({ length: 1 + Math.floor(next() * 4) }, (_, i) => `v${i}`); + const backends = Array.from({ length: 1 + Math.floor(next() * 4) }, (_, i) => `b${i}`); + const pools = Array.from({ length: Math.floor(next() * 3) }, (_, i) => `p${i}`); + const values = new Map(namedValues.map((name) => [name, pick(next, [...backends, ...pools, 'elsewhere'])])); + const credentials = new Map(backends.map((backend) => [backend, some(next, namedValues, 0.3)])); + const members = new Map(pools.map((pool) => [pool, some(next, backends, 0.5)])); + return { values, credentials, members, usedAsBackend: some(next, namedValues, 0.6) }; + }; + + // What each resource needs published before it: a credential user its + // named values; a named value a policy uses as backend-id the resource + // its value names; a pool its members; the fragment its named values. + const needsOf = (graph: Graph): Map => { + const present = new Set([...graph.values.keys(), ...graph.credentials.keys(), ...graph.members.keys()]); + const needs = new Map(); + const add = (from: string, to: string) => { + if (present.has(to)) needs.set(from, [...(needs.get(from) ?? []), to]); + }; + graph.credentials.forEach((used, backend) => used.forEach((namedValue) => add(backend, namedValue))); + graph.usedAsBackend.forEach((namedValue) => add(namedValue, graph.values.get(namedValue) ?? '')); + graph.members.forEach((members, pool) => members.forEach((member) => add(pool, member))); + graph.usedAsBackend.forEach((namedValue) => add('frag', namedValue)); + return needs; + }; + const reaches = (needs: Map, from: string, to: string): boolean => { + const seen = new Set(); + const pending = [...(needs.get(from) ?? [])]; + for (let node = pending.pop(); node !== undefined; node = pending.pop()) { + if (node === to) return true; + if (!seen.has(node)) { + seen.add(node); + pending.push(...(needs.get(node) ?? [])); + } + } + return false; + }; + + it('should publish each resource after what it needs, but for named values in a cycle', async () => { + for (let seed = 1; seed <= 150; seed++) { + const graph = generate(seed); + const { order, client, store } = recordOrder([ + ...[...graph.values.keys()].map((name) => ({ type: ResourceType.NamedValue, nameParts: [name] })), + ...[...graph.credentials.keys(), ...graph.members.keys()].map((name) => ({ type: ResourceType.Backend, nameParts: [name] })), + { type: ResourceType.PolicyFragment, nameParts: ['frag'] }, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + const name = descriptor.nameParts[0] ?? ''; + if (descriptor.type === ResourceType.NamedValue) { + return { properties: { displayName: name, value: graph.values.get(name) } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + const uses = graph.usedAsBackend.map((namedValue) => ``); + return { properties: { value: `${uses.join('')}` } }; + } + const members = graph.members.get(name); + if (members) { + return { properties: { type: 'Pool', pool: { services: members.map((member) => ({ id: `/backends/${member}` })) } } }; + } + const used = graph.credentials.get(name) ?? []; + return { + properties: { + url: 'https://example.test', + ...(used.length > 0 ? { credentials: { header: { 'x-key': used.map((namedValue) => `{{${namedValue}}}`) } } } : {}), + }, + }; + }); + + await publishAll(client, store); + + const needs = needsOf(graph); + const violations: string[] = []; + const published = new Set(order); + if (published.size !== order.length || published.size !== recordedCount(graph)) { + violations.push(`published ${order.join(', ')}`); + } + needs.forEach((needed, resource) => { + for (const target of needed) { + // Only a named value in a cycle may go before what it names. + const allowed = resource.startsWith('v') && reaches(needs, target, resource); + if (order.indexOf(resource) < order.indexOf(target) && !allowed) { + violations.push(`${resource} before ${target}`); + } + } + }); + expect(violations, `seed ${seed}: ${JSON.stringify({ ...graph, values: [...graph.values], credentials: [...graph.credentials], members: [...graph.members] })}`).toEqual([]); + } + }); + + const recordedCount = (graph: Graph) => graph.values.size + graph.credentials.size + graph.members.size + 1; + }); + + it('should keep a named value first when a pool member uses it in its credentials', async () => { + const { order, client, store } = recordOrder([ + { type: ResourceType.NamedValue, nameParts: ['route'] }, + { type: ResourceType.Backend, nameParts: ['member'] }, + { type: ResourceType.Backend, nameParts: ['my-pool'] }, + { type: ResourceType.PolicyFragment, nameParts: ['frag'] }, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + return { properties: { displayName: 'route', value: 'my-pool' } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: '' } }; + } + return descriptor.nameParts[0] === 'my-pool' + ? { properties: { type: 'Pool', pool: { services: [{ id: '/backends/member' }] } } } + : { properties: { url: 'https://example.test', credentials: { header: { 'x-route': ['{{route}}'] } } } }; + }); + + await publishAll(client, store); + + expect(order.indexOf('route')).toBeLessThan(order.indexOf('member')); + expect(order.indexOf('my-pool')).toBeGreaterThan(order.indexOf('member')); + }); + + it('should order by a policy the filter leaves out, since APIM checks it on the target', async () => { + const { order, client, store } = recordOrder([ + { type: ResourceType.Api, nameParts: ['orders'] }, + { type: ResourceType.Api, nameParts: ['billing'] }, + { type: ResourceType.ApiPolicy, nameParts: ['billing'] }, + { type: ResourceType.NamedValue, nameParts: ['billing-id'] }, + { type: ResourceType.Backend, nameParts: ['new-billing'] }, + ]); + store.readContent.mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.ApiPolicy + ? { content: '' } + : undefined + ); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => + descriptor.type === ResourceType.NamedValue + ? { properties: { displayName: 'billing-id', value: 'new-billing' } } + : { properties: { url: 'https://example.test' } } + ); + + // Named values and backends are left out of the filter, so all are selected. + await publishAll(client, store, { apis: ['orders'] }); + + expect(order).not.toContain('billing'); + expect(order.indexOf('billing-id')).toBeGreaterThan(order.indexOf('new-billing')); + }); + it('should publish named values before loggers within tier 1', async () => { const publishOrder: string[] = []; From a5f6ff9a8b6bc895c54f2f427adc0b5284ba2e5d Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 11:17:14 +1100 Subject: [PATCH 11/18] fix: publish a named value a policy uses as a resource name with that resource's deployed name --- src/models/config.ts | 7 +- src/services/publish-service.ts | 4 +- src/services/resource-publisher.ts | 44 +++++++++++- .../unit/services/policy-ref-rewriter.test.ts | 1 + tests/unit/services/publish-service.test.ts | 71 +++++++++++++++++++ .../resource-publisher.env-mapping.test.ts | 1 + 6 files changed, 124 insertions(+), 4 deletions(-) diff --git a/src/models/config.ts b/src/models/config.ts index 39f5d01a..cb011a58 100644 --- a/src/models/config.ts +++ b/src/models/config.ts @@ -6,7 +6,7 @@ */ import type { ResourceType } from './resource-types.js'; -import { ApimServiceContext } from './types.js'; +import { ApimServiceContext, ResourceDescriptor } from './types.js'; import { LogLevel } from '../lib/logger.js'; import type { EnvMapping } from '../services/env-mapper.js'; @@ -90,6 +90,11 @@ export interface KnownArtifactSets { * its display name. */ deployedNamedValueReference(reference: string, workspace?: string): string | undefined; + /** + * The resource types policies use a named value for as a resource name + * (backend-id="{{name}}"). Known within a publish run only; empty otherwise. + */ + namedValueTargetTypes(namedValue: ResourceDescriptor): ReadonlySet; } export interface PublishConfig { diff --git a/src/services/publish-service.ts b/src/services/publish-service.ts index ba0e0823..a268ece0 100644 --- a/src/services/publish-service.ts +++ b/src/services/publish-service.ts @@ -164,7 +164,9 @@ async function runPublishPipeline( store, config, await sourceReferences.available(), - sourceReferences.readNamedValue + sourceReferences.readNamedValue, + // A named value's value is only rewritten when names are affixed. + config.envMapping ? await sourceReferences.namedValueTargets() : undefined ); logger.debug( diff --git a/src/services/resource-publisher.ts b/src/services/resource-publisher.ts index 364743d0..3cc21f93 100644 --- a/src/services/resource-publisher.ts +++ b/src/services/resource-publisher.ts @@ -18,7 +18,7 @@ import type { PublishConfig } from '../models/config.js'; import { ResourceType, RESOURCE_TYPE_METADATA, MANAGED_GATEWAY_NAME } from '../models/resource-types.js'; import { applyOverrides, resolveOverrideSection, skipsAutoGeneratedNamedValue } from './override-merger.js'; import { checkKeyVaultSecretAccess } from './keyvault-checker.js'; -import { namedValueNamesByScope } from './named-value-names.js'; +import { namedValueNamesByScope, namedValueTargetName } from './named-value-names.js'; import { runParallel } from '../lib/parallel-runner.js'; import { getNamePart, @@ -72,7 +72,8 @@ export async function buildKnownArtifactSets( config: PublishConfig, descriptors: ReadonlyArray, readNamedValue: (descriptor: ResourceDescriptor) => Promise | undefined> = - (descriptor) => store.readResource(config.sourceDir, descriptor) + (descriptor) => store.readResource(config.sourceDir, descriptor), + namedValueTargets: ReadonlyMap> = new Map() ): Promise { const key = (type: ResourceType, name: string, workspace: string | undefined) => `${type}|${(workspace ?? '').toLowerCase()}|${name.toLowerCase()}`; @@ -109,6 +110,7 @@ export async function buildKnownArtifactSets( const name = names(workspace).resolve(reference); return name === undefined ? undefined : deployed.get(key(ResourceType.NamedValue, name, workspace)); }, + namedValueTargetTypes: (namedValue) => namedValueTargets.get(getResourceDescriptorKey(namedValue)) ?? new Set(), }; } @@ -431,6 +433,9 @@ export async function publishResource( properties: { ...(props ?? {}), displayName: deployedNamedValueDisplayName(descriptor, json, config) }, }; } + if (config.envMapping) { + json = rewriteNamedValueTarget(json, descriptor, config.envMapping, await knownArtifactSetsFor(store, config)); + } const props = json.properties as Record | undefined; const kvBlock = props?.keyVault as Record | undefined; @@ -1702,6 +1707,41 @@ async function publishWorkspaceApiTagLink( } } +/** + * A named value a policy uses as a resource name (backend-id="{{name}}") + * names the resource by its value, so the value becomes the resource's + * deployed name when the source has that resource. A value that is secret, an + * expression or a reference names none here (namedValueTargetName). When the + * policies use it for types the mapping affixes differently, it is left, with + * a warning, since it can't name both. + */ +function rewriteNamedValueTarget( + json: Record, + descriptor: ResourceDescriptor, + mapping: EnvMapping, + known: KnownArtifactSets, +): Record { + const name = namedValueTargetName(json); + const types = [...known.namedValueTargetTypes(descriptor)].filter( + (type) => name !== undefined && known.has(type, name, descriptor.workspace) + ); + if (name === undefined || types.length === 0) { + return json; + } + const deployedNames = new Set(types.map((type) => toDeployedName(name, type, mapping))); + if (deployedNames.size > 1) { + logger.warn( + `${buildResourceLabel(descriptor)} names '${name}' as a ${types.join(' and a ')}, which are deployed under ` + + `different names (${[...deployedNames].join(', ')}), so its value is published unchanged.` + ); + return json; + } + const [deployed] = deployedNames; + return deployed === name + ? json + : { ...json, properties: { ...(json.properties as Record), value: deployed } }; +} + /** * Rewrite the named value references in a logger's or backend's credentials * to the display names the named values are deployed with diff --git a/tests/unit/services/policy-ref-rewriter.test.ts b/tests/unit/services/policy-ref-rewriter.test.ts index cbd72413..b11e2251 100644 --- a/tests/unit/services/policy-ref-rewriter.test.ts +++ b/tests/unit/services/policy-ref-rewriter.test.ts @@ -52,6 +52,7 @@ function knownSets(mapping: EnvMapping | undefined, names = BASE): KnownArtifact }; return { has: (type, name, workspace) => !workspace && find(byType[type] ?? [], name) !== undefined, + namedValueTargetTypes: () => new Set(), deployedNamedValueReference: (reference) => { const name = find(names.namedValues, reference); return name === undefined || !mapping ? name : toDeployedName(name, ResourceType.NamedValue, mapping); diff --git a/tests/unit/services/publish-service.test.ts b/tests/unit/services/publish-service.test.ts index 865df60c..0db56b52 100644 --- a/tests/unit/services/publish-service.test.ts +++ b/tests/unit/services/publish-service.test.ts @@ -1808,6 +1808,77 @@ describe('publish-service', () => { ); }); + describe('with a named value a policy uses as a resource name, and an environment prefix', () => { + const publishWith = async ( + fragment: string, + values: Record, + extra: ResourceDescriptor[], + environment: Record = { namePrefix: 'dev-' } + ) => { + const client = createMockClient(); + const store = createMockStore([ + { type: ResourceType.PolicyFragment, nameParts: ['frag'] }, + ...Object.keys(values).map((name) => ({ type: ResourceType.NamedValue, nameParts: [name] })), + ...extra, + ]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + const name = descriptor.nameParts[0] ?? ''; + return { properties: { displayName: name, value: values[name] } }; + } + return descriptor.type === ResourceType.PolicyFragment + ? { properties: { format: 'rawxml', value: fragment } } + : { properties: { url: 'https://example.test' } }; + }); + await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + overrides: { environment }, + }); + return (name: string) => + (client.putResource.mock.calls.find((call) => call[1].type === ResourceType.NamedValue && call[1].nameParts[0] === `dev-${name}`)?.[2] as + { properties: { value?: string } } | undefined)?.properties.value; + }; + + it('should publish its value as the deployed name of the resource it names', async () => { + const valueOf = await publishWith( + '', + { route: 'new-backend', plain: 'new-backend' }, + [{ type: ResourceType.Backend, nameParts: ['new-backend'] }] + ); + + expect(valueOf('route')).toBe('dev-new-backend'); + // Not used as a resource name: its value is only text. + expect(valueOf('plain')).toBe('new-backend'); + }); + + it('should leave a value that names no resource in the source', async () => { + const valueOf = await publishWith('', { route: 'elsewhere' }, []); + + expect(valueOf('route')).toBe('elsewhere'); + }); + + it('should leave, with a warning, a value used for types affixed differently', async () => { + const warn = vi.spyOn(logger, 'warn').mockImplementation(() => undefined); + const valueOf = await publishWith( + 'x', + { shared: 'shared' }, + [ + { type: ResourceType.Backend, nameParts: ['shared'] }, + { type: ResourceType.Logger, nameParts: ['shared'] }, + ], + { namePrefix: 'dev-', appliesTo: ['NamedValue', 'Backend', 'PolicyFragment'] } + ); + + expect(valueOf('shared')).toBe('shared'); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('deployed under different names')); + warn.mockRestore(); + }); + }); + it('should pass commit-scoped deleted descriptors to dry-run report', async () => { const client = createMockClient(); const store = createMockStore([]); diff --git a/tests/unit/services/resource-publisher.env-mapping.test.ts b/tests/unit/services/resource-publisher.env-mapping.test.ts index e342535c..8693d04d 100644 --- a/tests/unit/services/resource-publisher.env-mapping.test.ts +++ b/tests/unit/services/resource-publisher.env-mapping.test.ts @@ -90,6 +90,7 @@ function knownSets(names: { namedValues?: string[]; fragments?: string[]; backen }; return { has: (type, name) => find(byType[type], name) !== undefined, + namedValueTargetTypes: () => new Set(), deployedNamedValueReference: (reference) => { const name = find(names.namedValues, reference); return name === undefined ? undefined : toDeployedName(name, ResourceType.NamedValue, DEV_MAPPING); From 8ca3bec52e0f486c8b44e1e8c2053cf77bbf49c1 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 10:49:04 +1100 Subject: [PATCH 12/18] docs: describe how named value references are matched, followed and rewritten --- docs/guides/environment-overrides.md | 4 +++- docs/guides/filtering-resources.md | 10 ++++++++++ docs/guides/multi-environment-shared-apim.md | 11 ++++++----- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/docs/guides/environment-overrides.md b/docs/guides/environment-overrides.md index b6be22fa..fc3c07bd 100644 --- a/docs/guides/environment-overrides.md +++ b/docs/guides/environment-overrides.md @@ -44,7 +44,7 @@ The schema provides: ## Multi-environment on a single APIM instance -Override files support an optional top-level `environment:` block that enables publishing multiple environments (dev, qa, prod) to a **single shared APIM instance**. When present, resource names are prefixed and/or suffixed at publish time so dev, qa, and prod resources coexist without colliding. Cross-references in policy XML — `{{namedValue}}` tokens, fragment IDs, backend IDs, subscription scopes — are rewritten automatically to use the affixed names. +Override files support an optional top-level `environment:` block that enables publishing multiple environments (dev, qa, prod) to a **single shared APIM instance**. When present, resource names are prefixed and/or suffixed at publish time so dev, qa, and prod resources coexist without colliding. Cross-references — `{{namedValue}}` tokens in policies and credentials, the backend, fragment, logger and schema IDs policies name, named values used as those IDs, subscription scopes — are rewritten automatically to use the affixed names; see [Cross-reference rewrites](multi-environment-shared-apim.md#cross-reference-rewrites-automatic). Override entries always use **canonical (unprefixed) names** matching the artifact files on disk, regardless of whether an `environment:` block is present. The tool applies the affix at publish time; you never write prefixed names in override files or artifact files. @@ -295,6 +295,8 @@ loggers: isBuffered: true ``` +Logger credentials reference the auto-generated named value by its display name, `{{Logger-Credentials--}}`, as APIM requires. Artifacts extracted by `v0.1.7-alpha.0` and `v0.2.0-alpha.0` reference it by its ID (`{{66f48e1226dab62c0823e4f8}}`); publish stops on those and names the display name to use, so re-extract or change the reference. + > **Tip:** You can also use [pipeline token substitution](token-substitution.md) for logger credentials. Replace the hardcoded value with a placeholder like `{#[APPINSIGHTS_KEY]#}`, and store the actual key in your pipeline's secret store. This keeps secrets out of your repository entirely. ### Subscriptions diff --git a/docs/guides/filtering-resources.md b/docs/guides/filtering-resources.md index 4b08c976..c7577ba5 100644 --- a/docs/guides/filtering-resources.md +++ b/docs/guides/filtering-resources.md @@ -338,6 +338,16 @@ And `petstore-api` is assigned to: The extract output includes all of these, even though only `apiNames` was specified in the filter. +### Named value references + +A `{{...}}` reference in a policy names the named value with that display name, ignoring case, as APIM does. On publish, that is the display name with your `--overrides` applied, or its display name in the source when an override changes it. APIM doesn't accept a reference to a named value's resource name, so extract doesn't follow one (it warns instead), and publish stops before making any change when a policy, fragment, or logger's or backend's credentials hold one, naming the display name to use. If extract can't list the named values, it tries each reference as a resource name instead, which it usually also is. + +Named values referenced in logger and backend credentials are pulled in too. + +A policy attribute set to a named value (`backend-id="{{backend-name}}"`, and likewise `logger-id` and `schema-id`) pulls in the named value and the resource its value names, in the same scope (except ``, where APIM doesn't substitute named values). Publish reads the value with your `--overrides` applied, so each environment gets the resource its own value names. A named value that is published also brings the resource its value names whenever any policy or fragment in the source uses it that way, even one the filter leaves out: APIM checks every policy on the target that uses a named value when its value changes. A resource that isn't in the source isn't published, so it has to exist on the target already. When a policy uses a named value to name a backend, logger or schema that is published in the same run, publish creates that resource before it updates the named value, and publishes policy fragments, and any backend or logger using the named value in its credentials, after it, since APIM rejects a named value change that would leave a policy using it pointing at a resource that doesn't exist. A secret or Key Vault named value's value isn't known, so the resource it names isn't pulled in; add that resource to the filter. + +References inside XML comments are ignored. Inside a `` body, a `{{name}}` is followed only when a named value has that display name: APIM substitutes those there and leaves every other `{{ ... }}` (`{{body.id}}`, `{{ name }}`, `{{x | upcase}}`) to Liquid, so those are skipped without an error. If extract can't list the named values, it reports an error, since it can't tell the two apart. + ### Why Transitive Matters Without transitive resolution, publishing the extracted artifacts to a new APIM instance would fail — the API references a backend that doesn't exist, a named value that's missing, or a policy fragment that can't be found. diff --git a/docs/guides/multi-environment-shared-apim.md b/docs/guides/multi-environment-shared-apim.md index b51f1acf..f6a9d16f 100644 --- a/docs/guides/multi-environment-shared-apim.md +++ b/docs/guides/multi-environment-shared-apim.md @@ -216,17 +216,18 @@ NamedValues have both a `name` (resource key) and a `displayName` (the `{{token} - Both the resource name **and** the displayName receive the affix. - This ensures `{{my-token}}` in a policy becomes `{{dev-my-token}}` for dev — cross-env policy isolation is maintained automatically. -- If you set an explicit `displayName` in a NamedValue override entry, that value is used as-is (no affix applied to it). +- If you set an explicit `displayName` in a NamedValue override entry, that value is used as-is (no affix applied to it), and references to the named value are rewritten to it. +- APIM resolves a `{{token}}` by display name only (ignoring case), never by resource name, so a token is matched to the named value with that display name in the policy's own scope (service or workspace). ## Cross-reference rewrites (automatic) -When `appliesTo` includes the relevant types, `apiops publish` rewrites the following references in artifacts before sending them to APIM. You do not need to update artifact files or override files manually. +When `appliesTo` includes the relevant types, `apiops publish` rewrites the following references in artifacts before sending them to APIM. You do not need to update artifact files or override files manually. References are matched against every artifact in the source, so they are rewritten even when the filter or an incremental publish leaves out the resource they name. | Reference | Location | How it is rewritten | |---|---|---| -| `{{namedValueToken}}` | Policy XML | Token rewritten to affixed displayName when `NamedValue ∈ appliesTo` and the named value artifact exists. Unknown tokens pass through unchanged. | -| `fragment-id="..."` | Policy XML `` | Fragment ID rewritten when `PolicyFragment ∈ appliesTo`. | -| `backend-id="..."` | Policy XML `` | Backend ID rewritten when `Backend ∈ appliesTo`. | +| `{{namedValueToken}}` | Policy and policy fragment XML; logger and backend `credentials` | Token matched to the named value with that display name (ignoring case, in the same scope) and rewritten to the display name it is deployed with: affixed when `NamedValue ∈ appliesTo`, or an explicit `displayName` override as written. Tokens no named value in the source has as its display name pass through unchanged. | +| `backend-id`, `embeddings-backend-id`, `fragment-id`, `logger-id`, `schema-id` | Policy XML ``, ``, `` / ``, ``, ``, validate-content's `` | Rewritten when the resource's type (`Backend`, `PolicyFragment`, `Logger`, `GlobalSchema`) is in `appliesTo` and the source has that resource in the policy's scope. A policy expression (`@(...)`) is left as is. | +| Named value `value` | ARM named value properties | When a policy uses the named value as a resource name (`backend-id="{{name}}"`, `logger-id`, `schema-id`) and its value names that resource in the source, the value is rewritten to the resource's deployed name. Secret, Key Vault and expression values are left as is. If the policies use it for types affixed differently, it is left as is with a warning. The value changes wherever the named value is used, including as plain text. | | Subscription `scope` | ARM subscription properties | Trailing segment rewritten when the referenced type (Api or Product) is in `appliesTo`. | | ApiRelease `apiId` | ARM release properties | Api ID segment rewritten when `Api ∈ appliesTo`. | | ProductApi association | ARM resource path | Both product and API name segments rewritten per `appliesTo`. | From eae8ee2d5b6e5a003f1aaf692dce270bb2c04b36 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 17:59:47 +1100 Subject: [PATCH 13/18] fix: find a policy attribute after one holding a policy expression with quoted strings APIM's raw XML keeps a policy expression's string literals as written, so an attribute value can hold double quotes: ``. The attribute patterns ended that value at the first inner quote and never reached backend-id, so the dependency scan missed the backend and the prefix rewrite left it unaffixed. The patterns now run on a copy with each policy expression's inside blanked (blankPolicyExpressions: `@(...)` and `@{...}`, balancing brackets and skipping C# string and character literals), the same length, so the rewriter applies its matches to the original text by offset. A value that is an expression keeps its leading `@`. --- src/services/policy-ref-rewriter.ts | 20 ++++-- src/services/transitive-resolver.ts | 68 ++++++++++++++++++- .../unit/services/policy-ref-rewriter.test.ts | 6 ++ .../unit/services/transitive-resolver.test.ts | 27 ++++++++ 4 files changed, 114 insertions(+), 7 deletions(-) diff --git a/src/services/policy-ref-rewriter.ts b/src/services/policy-ref-rewriter.ts index 15e9695f..3cacf938 100644 --- a/src/services/policy-ref-rewriter.ts +++ b/src/services/policy-ref-rewriter.ts @@ -24,7 +24,7 @@ import type { EnvMapping } from './env-mapper.js'; import { toDeployedName } from './env-mapper.js'; import type { KnownArtifactSets } from '../models/config.js'; -import { POLICY_REFERENCE_PATTERNS, replaceNamedValueReferences } from './transitive-resolver.js'; +import { blankPolicyExpressions, POLICY_REFERENCE_PATTERNS, replaceNamedValueReferences } from './transitive-resolver.js'; // --------------------------------------------------------------------------- // Public API @@ -72,12 +72,22 @@ export function rewritePolicyRefs( // An attribute that names a resource (backend-id, fragment-id, logger-id, // schema-id ...) names one in the policy's scope. A policy expression or a // named value (rewritten above) names none here. + // Matched on a copy with policy expressions blanked (same offsets), so a + // quote inside one doesn't hide the attributes after it. for (const { type, pattern } of POLICY_REFERENCE_PATTERNS) { if (!mapping.appliesTo.has(type)) continue; - result = result.replace(pattern, (match: string, quote: string, value: string) => { - if (value.startsWith('@') || value.includes('{{') || !known.has(type, value, workspace)) return match; - return `${match.slice(0, match.length - value.length - quote.length)}${toDeployedName(value, type, mapping)}${quote}`; - }); + const current = result; + let rewritten = ''; + let last = 0; + for (const match of blankPolicyExpressions(current).matchAll(pattern)) { + const [whole, quote = '', value = ''] = match; + const start = match.index ?? 0; + const end = start + whole.length; + if (value.startsWith('@') || value.includes('{{') || !known.has(type, value, workspace)) continue; + rewritten += current.slice(last, end - value.length - quote.length) + toDeployedName(value, type, mapping) + quote; + last = end; + } + result = rewritten + current.slice(last); } return result; diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index c3357250..ac243857 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -87,6 +87,64 @@ const LIQUID_BODY_PATTERN = new RegExp( */ const LIQUID_NAMED_VALUE_PATTERN = /\{\{([A-Za-z0-9._-]+)\}\}/g; +/** + * Policy XML with the inside of each policy expression (`@(...)`, `@{...}`) + * replaced by spaces, the same length. APIM's raw XML keeps an expression's + * string literals as written, so an attribute value can hold double quotes + * (`sf-service-instance-name="@("fabric:/app")"`), which would end an + * attribute pattern's value early and hide the attributes after it. The `@` + * and the outer brackets are kept, so a value that is an expression still + * starts with `@`; offsets are unchanged, so matches found in the result + * apply to the original. An expression whose end can't be found is left as + * it is. + */ +export function blankPolicyExpressions(xml: string): string { + let result = ''; + let last = 0; + let index = xml.indexOf('@'); + while (index !== -1) { + const open = xml[index + 1]; + const end = open === '(' || open === '{' ? policyExpressionEnd(xml, index + 1) : -1; + if (end === -1) { + index = xml.indexOf('@', index + 1); + continue; + } + result += xml.slice(last, index + 2) + ' '.repeat(end - index - 2); + last = end; + index = xml.indexOf('@', end); + } + return result + xml.slice(last); +} + +/** + * The index of the bracket closing the one at `start` (`(` or `{`), skipping + * C# string and character literals, or -1 when the expression doesn't close. + */ +function policyExpressionEnd(xml: string, start: number): number { + const opening = xml[start]; + const closing = opening === '(' ? ')' : '}'; + let depth = 0; + for (let i = start; i < xml.length; i++) { + const char = xml[i]; + if (char === '"' || char === "'") { + // A verbatim string (@"...") doubles its quotes; others escape with '\'. + const verbatim = char === '"' && xml[i - 1] === '@'; + for (i++; i < xml.length; i++) { + if (verbatim ? xml[i] === '"' && xml[i + 1] === '"' : xml[i] === '\\') { + i++; + } else if (xml[i] === char) { + break; + } + } + } else if (char === opening) { + depth++; + } else if (char === closing && --depth === 0) { + return i; + } + } + return -1; +} + /** * Policy XML with the parts that cannot reference other resources removed: * XML comments, and the bodies of Liquid templates. @@ -249,11 +307,17 @@ export function scanNamedValueTargets(policyXml: string): NamedValueTarget[] { return targets; } -/** The non-empty values of the policy attributes that name a resource, trimmed. */ +/** + * The non-empty values of the policy attributes that name a resource, trimmed. + * Policy expressions are blanked first (blankPolicyExpressions), so a quote + * inside one doesn't end the attribute early; a value that is an expression + * keeps its leading `@`. + */ function scanReferenceAttributes(scannable: string): Array<{ type: ResourceType; value: string }> { const attributes: Array<{ type: ResourceType; value: string }> = []; + const blanked = blankPolicyExpressions(scannable); for (const { type, pattern } of POLICY_REFERENCE_PATTERNS) { - for (const match of scannable.matchAll(pattern)) { + for (const match of blanked.matchAll(pattern)) { const value = match[2]?.trim(); if (value) { attributes.push({ type, value }); diff --git a/tests/unit/services/policy-ref-rewriter.test.ts b/tests/unit/services/policy-ref-rewriter.test.ts index b11e2251..e23be27b 100644 --- a/tests/unit/services/policy-ref-rewriter.test.ts +++ b/tests/unit/services/policy-ref-rewriter.test.ts @@ -112,6 +112,12 @@ describe('rewritePolicyRefs', () => { expect(result).toBe(xml); }); + it('rewrites backend-id after an attribute whose policy expression holds quoted strings', () => { + const xml = ''; + const result = rewrite(xml, devPrefixMapping(ALL_TYPES)); + expect(result).toBe(''); + }); + it('rewrites backend-id with single quotes', () => { const xml = ""; const result = rewrite(xml, devPrefixMapping([ResourceType.Backend])); diff --git a/tests/unit/services/transitive-resolver.test.ts b/tests/unit/services/transitive-resolver.test.ts index a7177797..bf109e6e 100644 --- a/tests/unit/services/transitive-resolver.test.ts +++ b/tests/unit/services/transitive-resolver.test.ts @@ -17,6 +17,7 @@ import { findTransitiveDependencies, findSubscriptionTargets, scanArtifactReferences, + blankPolicyExpressions, } from '../../../src/services/transitive-resolver.js'; describe('transitive-resolver', () => { @@ -93,6 +94,18 @@ describe('transitive-resolver', () => { ['azure-openai-semantic-cache-lookup', '', ResourceType.Backend, 'embeddings'], ['llm-semantic-cache-lookup', '', ResourceType.Backend, 'llm-embeddings'], ['include-fragment with another attribute first', '', ResourceType.PolicyFragment, 'frag-a'], + [ + 'a policy expression with quoted strings in an attribute before it', + '("svc", "orders"))" backend-id="orders-backend" />', + ResourceType.Backend, + 'orders-backend', + ], + [ + 'a multi-statement expression before it', + '', + ResourceType.Backend, + 'sf-backend', + ], ])('should detect a reference written with %s', (_label, policy, type, name) => { expect(scanPolicyReferences(policy)).toContainEqual({ type, name }); }); @@ -558,3 +571,17 @@ describe('transitive-resolver', () => { }); }); }); + +describe('blankPolicyExpressions', () => { + it.each([ + ['a call with string literals', 'v="@(ctx.Get("a)b", \'"\'))" x', 'v="@( )" x'], + ['a verbatim string with doubled quotes', 'v="@(@"a""b)c")" x', 'v="@( )" x'], + ['an escaped quote', 'v="@("a\\"b")" x', 'v="@( )" x'], + ['a block with nested braces', 'v="@{ if (a) { return "}"; } }" x', 'v="@{ }" x'], + ['an unclosed expression', 'v="@(ctx.Get("a")" x', 'v="@(ctx.Get("a")" x'], + ['an @ that starts no expression', 'mail="a@example.com"', 'mail="a@example.com"'], + ])('should blank %s, keeping its length', (_label, xml, expected) => { + expect(blankPolicyExpressions(xml)).toBe(expected); + expect(blankPolicyExpressions(xml)).toHaveLength(xml.length); + }); +}); From b98137f0d8ba33733f5110095d65f6c269bd8594 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 18:02:17 +1100 Subject: [PATCH 14/18] fix: scan the policies an incremental publish republishes in the pre-publish checks In an incremental publish, a changed API is published with its unchanged child policies (planApiPublication), and a changed product with its policy (planProductPolicyPublication), although those aren't publish targets themselves. The checks that stop publish before any change, for a reference to a named value's resource name and for the redaction marker, scanned only the targets, so such a policy got past them and APIM rejected it after earlier resources were already published. Both checks now scan the targets and those policies. In a full publish they are targets already, so nothing more is read. --- src/services/publish-service.ts | 47 ++++++++++- tests/unit/services/publish-service.test.ts | 87 +++++++++++++++++++++ 2 files changed, 130 insertions(+), 4 deletions(-) diff --git a/src/services/publish-service.ts b/src/services/publish-service.ts index a268ece0..c2fc400b 100644 --- a/src/services/publish-service.ts +++ b/src/services/publish-service.ts @@ -40,8 +40,8 @@ import { ResourcePublishResult, buildKnownArtifactSets, } from './resource-publisher.js'; -import { publishApi } from './api-publisher.js'; -import { publishProduct } from './product-publisher.js'; +import { planApiPublication, publishApi } from './api-publisher.js'; +import { planProductPolicyPublication, publishProduct } from './product-publisher.js'; import { generateDryRunReport, DryRunReport } from './dry-run-reporter.js'; import { computeDeleteActions, @@ -173,6 +173,10 @@ async function runPublishPipeline( `Publishing ${targetDescriptors.length} resources (dry-run: ${config.dryRun})` ); + // The pre-flight checks below scan everything that will be published: + // the targets, and the policies published with them (withPlannedPolicies). + const published = await withPlannedPolicies(store, config, targetDescriptors); + // Step 1c: Redaction pre-flight gate. // Scan every artifact that would be published for leftover redaction markers // ('*** REDACTED ***'). A single leftover marker aborts the ENTIRE publish @@ -181,7 +185,7 @@ async function runPublishPipeline( const redactionFindings = await scanForRedactionMarkers( store, config, - targetDescriptors + published ); if (redactionFindings.length > 0) { logger.error( @@ -217,7 +221,7 @@ async function runPublishPipeline( // rejects a policy or credentials whose reference is a named value's // resource name instead. Stop before any PUT — and fail dry-run — rather // than partway through the publish. - const resourceNameReferences = await findResourceNameReferences(store, config, targetDescriptors, sourceReferences); + const resourceNameReferences = await findResourceNameReferences(store, config, published, sourceReferences); if (resourceNameReferences.length > 0) { logger.error( `Publish aborted: ${resourceNameReferences.length} reference(s) use a named value's resource name. ` + @@ -370,6 +374,41 @@ async function determinePublishTargets( return { targetDescriptors, deletedDescriptors }; } +/** + * The publish targets plus the policies the API and product publishers send + * with them in an incremental publish: an API's unchanged child policies + * (planApiPublication) and a product's policy (planProductPolicyPublication) + * are republished with a changed API or product without being targets + * themselves. In a full publish they are targets already. + */ +async function withPlannedPolicies( + store: IArtifactStore, + config: PublishConfig, + targets: ResourceDescriptor[] +): Promise { + if (config.commitId === undefined) { + return targets; + } + const allowed = config.filter ? targets : undefined; + const keys = new Set(targets.map(getResourceDescriptorKey)); + const planned: ResourceDescriptor[] = []; + for (const descriptor of targets) { + const children = descriptor.type === ResourceType.Api && !isApiRevisionName(getNamePart(descriptor.nameParts, 0)) + ? (await planApiPublication(store, descriptor, config, allowed)).childPuts + : descriptor.type === ResourceType.Product + ? [await planProductPolicyPublication(store, descriptor, config, allowed)] + .flatMap((policy) => (policy?.eligible ? [policy.descriptor] : [])) + : []; + for (const child of children) { + if (!keys.has(getResourceDescriptorKey(child))) { + keys.add(getResourceDescriptorKey(child)); + planned.push(child); + } + } + } + return [...targets, ...planned]; +} + /** * The `{{references}}` in the artifacts being published that no named value * has as its display name but one has as its resource name, with that named diff --git a/tests/unit/services/publish-service.test.ts b/tests/unit/services/publish-service.test.ts index 0db56b52..5da6d19a 100644 --- a/tests/unit/services/publish-service.test.ts +++ b/tests/unit/services/publish-service.test.ts @@ -32,6 +32,7 @@ import { runPublish } from '../../../src/services/publish-service.js'; import { computeGitDiff } from '../../../src/services/git-diff-service.js'; import { generateDryRunReport } from '../../../src/services/dry-run-reporter.js'; import { computeDeleteActions } from '../../../src/services/delete-unmatched-service.js'; +import { planApiPublication } from '../../../src/services/api-publisher.js'; import { publishApi } from '../../../src/services/api-publisher.js'; import { publishProduct } from '../../../src/services/product-publisher.js'; @@ -93,6 +94,15 @@ describe('publish-service', () => { action: 'put', }); + vi.mocked(planApiPublication).mockResolvedValue({ + importSpecification: false, + revisions: [], + alignActiveRevision: false, + childPuts: [], + operationDescriptionPuts: [], + operationPatches: [], + }); + vi.mocked(publishProduct).mockResolvedValue({ descriptor: { type: ResourceType.Product, nameParts: ['test-product'] }, status: 'success', @@ -1553,6 +1563,83 @@ describe('publish-service', () => { error.mockRestore(); }); + it("should abort before any PUT when an incremental publish republishes an unchanged API policy with a resource name reference", async () => { + const error = vi.spyOn(logger, 'error').mockImplementation(() => undefined); + const api: ResourceDescriptor = { type: ResourceType.Api, nameParts: ['orders'] }; + const apiPolicy: ResourceDescriptor = { type: ResourceType.ApiPolicy, nameParts: ['orders'] }; + const client = createMockClient(); + const store = createMockStore([{ type: ResourceType.NamedValue, nameParts: ['backend-id'] }, api, apiPolicy]); + vi.mocked(store.readResource).mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.NamedValue ? { properties: { displayName: 'Backend_Id', value: 'x' } } : { properties: {} } + ); + vi.mocked(store.readContent).mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.ApiPolicy + ? { content: '{{backend-id}}', format: 'xml' } + : undefined + ); + // Only the API changed; publishApi republishes its policy with it. + vi.mocked(computeGitDiff).mockResolvedValue({ changedDescriptors: [api], deletedDescriptors: [] }); + vi.mocked(planApiPublication).mockResolvedValue({ + importSpecification: false, + revisions: [], + alignActiveRevision: false, + childPuts: [apiPolicy], + operationDescriptionPuts: [], + operationPatches: [], + }); + + const result = await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + dryRun: false, + deleteUnmatched: false, + commitId: 'abc123', + logLevel: LogLevel.INFO, + }); + + expect(result.exitCode).toBe(2); + expect(vi.mocked(publishApi)).not.toHaveBeenCalled(); + expect(result.actions.map((action) => action.error?.message)).toEqual([ + expect.stringContaining('references {{backend-id}}, the resource name of named value \'backend-id\'; use its display name, {{Backend_Id}}'), + ]); + error.mockRestore(); + }); + + it('should abort before any PUT when an incremental publish republishes an unchanged API policy holding the redaction marker', async () => { + const error = vi.spyOn(logger, 'error').mockImplementation(() => undefined); + const api: ResourceDescriptor = { type: ResourceType.Api, nameParts: ['orders'] }; + const apiPolicy: ResourceDescriptor = { type: ResourceType.ApiPolicy, nameParts: ['orders'] }; + const client = createMockClient(); + const store = createMockStore([api, apiPolicy]); + vi.mocked(store.readContent).mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => + descriptor.type === ResourceType.ApiPolicy + ? { content: '*** REDACTED ***', format: 'xml' } + : undefined + ); + vi.mocked(computeGitDiff).mockResolvedValue({ changedDescriptors: [api], deletedDescriptors: [] }); + vi.mocked(planApiPublication).mockResolvedValue({ + importSpecification: false, + revisions: [], + alignActiveRevision: false, + childPuts: [apiPolicy], + operationDescriptionPuts: [], + operationPatches: [], + }); + + const result = await runPublish(client, store, { + service: testContext, + sourceDir: '/source', + dryRun: false, + deleteUnmatched: false, + commitId: 'abc123', + logLevel: LogLevel.INFO, + }); + + expect(result.exitCode).toBe(2); + expect(vi.mocked(publishApi)).not.toHaveBeenCalled(); + error.mockRestore(); + }); + it("should abort when logger credentials reference an auto-generated named value's resource name", async () => { const error = vi.spyOn(logger, 'error').mockImplementation(() => undefined); const { result, client } = await publishWith('', {}, `{{${hex}}}`); From 7a08cae1c133458010d0be11a25e807201f2c34f Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 18:24:24 +1100 Subject: [PATCH 15/18] fix: read C# comments in policy expressions, and find XML comments and expressions in one pass An apostrophe in a C# comment inside a policy expression was read as a character literal, so the expression stayed unblanked and a quoted string in it hid the reference attributes after it from the scan and from prefixed publishing. A ``, hiding the references between. Expressions now skip `//` and `/* */` comments, and XML comments and expressions are found in one pass, so neither starts inside the other. --- src/services/transitive-resolver.ts | 101 ++++++++++++------ .../unit/services/policy-ref-rewriter.test.ts | 7 ++ .../unit/services/transitive-resolver.test.ts | 25 +++++ 3 files changed, 102 insertions(+), 31 deletions(-) diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index ac243857..ec97dc0f 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -62,13 +62,6 @@ export const POLICY_REFERENCE_PATTERNS: ReadonlyArray<{ type: ResourceType; patt { type: ResourceType.GlobalSchema, pattern: attributePattern('content', 'schema-id') }, ]; -/** - * XML comments: references inside them are not in effect. Only a closed - * comment is removed, so a stray `/g; - /** * The body of a `` element (capture group 3): its * `{{ ... }}` are mostly Liquid output expressions, not named value @@ -87,6 +80,49 @@ const LIQUID_BODY_PATTERN = new RegExp( */ const LIQUID_NAMED_VALUE_PATTERN = /\{\{([A-Za-z0-9._-]+)\}\}/g; +/** + * The XML comments and policy expressions (`@(...)`, `@{...}`) in policy XML, + * in order, found in one pass: a `', i + 4) + : xml[i] === '@' && (xml[i + 1] === '(' || xml[i + 1] === '{') + ? policyExpressionEnd(xml, i + 1) + : -1; + if (close !== -1) { + const comment = xml[i] === '<'; + const end = comment ? close + 3 : close + 1; + spans.push({ kind: comment ? 'comment' : 'expression', start: i, end }); + i = end - 1; + } + } + return spans; +} + +/** + * `xml` with each span of `kind` replaced by spaces, the same length, except + * `keep` characters at its start and end. + */ +function blankSpans(xml: string, kind: 'comment' | 'expression', keep: { start: number; end: number }): string { + let result = ''; + let last = 0; + for (const span of policySpans(xml)) { + if (span.kind === kind) { + const from = span.start + keep.start; + const to = span.end - keep.end; + result += xml.slice(last, from) + ' '.repeat(to - from); + last = to; + } + } + return result + xml.slice(last); +} + /** * Policy XML with the inside of each policy expression (`@(...)`, `@{...}`) * replaced by spaces, the same length. APIM's raw XML keeps an expression's @@ -95,30 +131,27 @@ const LIQUID_NAMED_VALUE_PATTERN = /\{\{([A-Za-z0-9._-]+)\}\}/g; * attribute pattern's value early and hide the attributes after it. The `@` * and the outer brackets are kept, so a value that is an expression still * starts with `@`; offsets are unchanged, so matches found in the result - * apply to the original. An expression whose end can't be found is left as - * it is. + * apply to the original. An expression whose end can't be found, or that is + * inside an XML comment, is left as it is. */ export function blankPolicyExpressions(xml: string): string { - let result = ''; - let last = 0; - let index = xml.indexOf('@'); - while (index !== -1) { - const open = xml[index + 1]; - const end = open === '(' || open === '{' ? policyExpressionEnd(xml, index + 1) : -1; - if (end === -1) { - index = xml.indexOf('@', index + 1); - continue; - } - result += xml.slice(last, index + 2) + ' '.repeat(end - index - 2); - last = end; - index = xml.indexOf('@', end); - } - return result + xml.slice(last); + return blankSpans(xml, 'expression', { start: 2, end: 1 }); +} + +/** + * Policy XML with each XML comment replaced by spaces, the same length: + * references inside comments are not in effect. Comments are found the way + * policySpans finds them, so a `' + ); + + expect(refs).toEqual([ + { type: ResourceType.NamedValue, name: 'Api_Key' }, + { type: ResourceType.Backend, name: 'orders-backend' }, + ]); + }); + it('should not take an @( inside an XML comment for a policy expression', () => { + const refs = scanPolicyReferences( + '' + + '' + + '' + ); + + expect(refs).toEqual([{ type: ResourceType.Backend, name: 'orders-backend' }]); + }); it('should give the {{name}}s in Liquid bodies that a named value could have, for callers to check', () => { const policy = ` nv=[{{demo-backend-id}}] dotted=[{{body.id}}] spaced=[{{ context.Request.Url }}] filtered=[{{x | upcase}}] @@ -578,6 +600,9 @@ describe('blankPolicyExpressions', () => { ['a verbatim string with doubled quotes', 'v="@(@"a""b)c")" x', 'v="@( )" x'], ['an escaped quote', 'v="@("a\\"b")" x', 'v="@( )" x'], ['a block with nested braces', 'v="@{ if (a) { return "}"; } }" x', 'v="@{ }" x'], + ['a block comment with a quote', 'v="@{ /* don\'t */ return "a"; }" x', 'v="@{ }" x'], + ['a line comment with a bracket', 'v="@{ // a ) or "\nreturn 1; }" x', 'v="@{ }" x'], + ['an expression inside an XML comment', ' v="@(a)"', ' v="@( )"'], ['an unclosed expression', 'v="@(ctx.Get("a")" x', 'v="@(ctx.Get("a")" x'], ['an @ that starts no expression', 'mail="a@example.com"', 'mail="a@example.com"'], ])('should blank %s, keeping its length', (_label, xml, expected) => { From 9150d6629ef76991f88fd1e565b67f9a304cf63b Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 18:24:24 +1100 Subject: [PATCH 16/18] fix: match a pool member's workspace ignoring case when ordering named values The Tier 1 ordering key lowercased the name but not the workspace, so a pool member ID spelling its workspace differently (`TEAM-A`) didn't match its backend in `team-a`, and the pool could be published before a member deferred for a named value. --- src/services/publish-service.ts | 4 ++- tests/unit/services/publish-service.test.ts | 35 +++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/services/publish-service.ts b/src/services/publish-service.ts index c2fc400b..00f68cf1 100644 --- a/src/services/publish-service.ts +++ b/src/services/publish-service.ts @@ -1078,8 +1078,10 @@ async function splitNamedValuesNamingResources( otherTier1: ResourceDescriptor[], sourceReferences: SourceReferences ): Promise<{ early: ResourceDescriptor[]; ordered: ResourceDescriptor[][] }> { + // Names are case-insensitive, workspaces included: a pool member's ID can + // spell its workspace differently from the descriptor. const resourceKey = (type: ResourceType, name: string, workspace: string | undefined) => - `${type}/${workspace ?? ''}/${name.toLowerCase()}`; + `${type}/${(workspace ?? '').toLowerCase()}/${name.toLowerCase()}`; const keyOf = (descriptor: ResourceDescriptor) => resourceKey(descriptor.type, getNamePart(descriptor.nameParts, 0), descriptor.workspace); const byResourceKey = new Map(otherTier1.map((descriptor) => [keyOf(descriptor), descriptor])); diff --git a/tests/unit/services/publish-service.test.ts b/tests/unit/services/publish-service.test.ts index 5da6d19a..e627a8e5 100644 --- a/tests/unit/services/publish-service.test.ts +++ b/tests/unit/services/publish-service.test.ts @@ -2381,6 +2381,41 @@ describe('publish-service', () => { expect(order.indexOf('route')).toBeGreaterThan(order.indexOf('pool-id')); }); + it('should publish a pool after a member deferred for a named value, whatever the case of its workspace', async () => { + const inWorkspace = (type: ResourceType, name: string): ResourceDescriptor => + ({ type, nameParts: [name], workspace: 'team-a' }); + const { order, client, store } = recordOrder([ + inWorkspace(ResourceType.NamedValue, 'route'), + inWorkspace(ResourceType.Backend, 'new-backend'), + inWorkspace(ResourceType.Backend, 'caller'), + inWorkspace(ResourceType.Backend, 'my-pool'), + inWorkspace(ResourceType.PolicyFragment, 'frag'), + ]); + const memberId = + '/subscriptions/s/resourceGroups/r/providers/Microsoft.ApiManagement/service/a/workspaces/TEAM-A/backends/caller'; + vi.mocked(store.readResource).mockImplementation(async (_sourceDir, descriptor) => { + if (descriptor.type === ResourceType.NamedValue) { + return { properties: { displayName: 'route', value: 'new-backend' } }; + } + if (descriptor.type === ResourceType.PolicyFragment) { + return { properties: { value: '' } }; + } + switch (descriptor.nameParts[0]) { + case 'caller': + return { properties: { url: 'https://example.test', credentials: { header: { 'x-route': ['{{route}}'] } } } }; + case 'my-pool': + return { properties: { type: 'Pool', pool: { services: [{ id: memberId }] } } }; + default: + return { properties: { url: 'https://example.test' } }; + } + }); + + await publishAll(client, store); + + expect(order.indexOf('caller')).toBeGreaterThan(order.indexOf('route')); + expect(order.indexOf('my-pool')).toBeGreaterThan(order.indexOf('caller')); + }); + it("should keep a named value first when a backend published with it uses it in its credentials", async () => { const { order, client, store } = recordOrder([ { type: ResourceType.NamedValue, nameParts: ['shared'] }, From c4534f07a441f866734cf5a36b5ff3292d3ae824 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 18:50:16 +1100 Subject: [PATCH 17/18] fix: read the code inside interpolated strings in policy expressions A quote inside an interpolated string's `{...}` (`$"{ctx.Get("s)v")}"`) was taken for the end of the string, so a bracket in the nested literal ended the expression early, and the attributes after it were hidden from the scan and from prefixed publishing. Each `{...}` in a `$"..."` string is now read as code, nested literals included. --- src/services/transitive-resolver.ts | 43 +++++++++++++++---- .../unit/services/policy-ref-rewriter.test.ts | 7 +++ .../unit/services/transitive-resolver.test.ts | 10 +++++ 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index ec97dc0f..5b74783f 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -167,14 +167,9 @@ function policyExpressionEnd(xml: string, start: number): number { } i = commentEnd; } else if (char === '"' || char === "'") { - // A verbatim string (@"...") doubles its quotes; others escape with '\'. - const verbatim = char === '"' && xml[i - 1] === '@'; - for (i++; i < xml.length; i++) { - if (verbatim ? xml[i] === '"' && xml[i + 1] === '"' : xml[i] === '\\') { - i++; - } else if (xml[i] === char) { - break; - } + i = literalEnd(xml, i); + if (i === -1) { + return -1; } } else if (char === opening) { depth++; @@ -185,6 +180,38 @@ function policyExpressionEnd(xml: string, start: number): number { return -1; } +/** + * The index of the quote closing the C# string or character literal whose + * opening quote is at `start`, or -1 when it doesn't close. A verbatim string + * (`@"..."`) doubles its quotes; others escape with '\'. An interpolated + * string (`$"..."`) holds code in each `{...}` (`{{` is a literal brace), + * which can hold literals of its own (`$"{ctx.Get("a)b")}"`). + */ +function literalEnd(xml: string, start: number): number { + const quote = xml[start]; + const prefix = quote === '"' ? xml.slice(Math.max(0, start - 2), start) : ''; + const verbatim = prefix.endsWith('@') || prefix === '@$'; + const interpolated = prefix.endsWith('$') || prefix === '$@'; + for (let i = start + 1; i < xml.length; i++) { + const char = xml[i]; + if (verbatim ? char === '"' && xml[i + 1] === '"' : char === '\\') { + i++; + } else if (interpolated && char === '{') { + if (xml[i + 1] === '{') { + i++; + continue; + } + i = policyExpressionEnd(xml, i); + if (i === -1) { + return -1; + } + } else if (char === quote) { + return i; + } + } + return -1; +} + /** * Policy XML with the parts that cannot reference other resources removed: * XML comments (blanked), and the bodies of Liquid templates. diff --git a/tests/unit/services/policy-ref-rewriter.test.ts b/tests/unit/services/policy-ref-rewriter.test.ts index c40eec2b..1f9ba4ba 100644 --- a/tests/unit/services/policy-ref-rewriter.test.ts +++ b/tests/unit/services/policy-ref-rewriter.test.ts @@ -118,6 +118,13 @@ describe('rewritePolicyRefs', () => { expect(result).toBe(''); }); + it('rewrites backend-id after an interpolated string holding a literal with a bracket', () => { + const name = '@($"fabric:/{context.Variables.GetValueOrDefault("s)v")}")'; + const xml = ``; + const result = rewrite(xml, devPrefixMapping(ALL_TYPES)); + expect(result).toBe(``); + }); + it('rewrites backend-id after a policy expression whose comment holds an apostrophe', () => { const condition = '@{ /* don\'t retry here */ return context.Variables.GetValueOrDefault("resolve"); }'; const xml = ``; diff --git a/tests/unit/services/transitive-resolver.test.ts b/tests/unit/services/transitive-resolver.test.ts index 5a380178..b856b3e3 100644 --- a/tests/unit/services/transitive-resolver.test.ts +++ b/tests/unit/services/transitive-resolver.test.ts @@ -139,6 +139,13 @@ describe('transitive-resolver', () => { { type: ResourceType.Backend, name: 'orders-backend' }, ]); }); + it('should detect a backend after an interpolated string holding a literal with a bracket', () => { + const refs = scanPolicyReferences( + '' + ); + + expect(refs).toEqual([{ type: ResourceType.Backend, name: 'sf-backend' }]); + }); it('should not take an @( inside an XML comment for a policy expression', () => { const refs = scanPolicyReferences( '' + @@ -602,6 +609,9 @@ describe('blankPolicyExpressions', () => { ['a block with nested braces', 'v="@{ if (a) { return "}"; } }" x', 'v="@{ }" x'], ['a block comment with a quote', 'v="@{ /* don\'t */ return "a"; }" x', 'v="@{ }" x'], ['a line comment with a bracket', 'v="@{ // a ) or "\nreturn 1; }" x', 'v="@{ }" x'], + ['an interpolated string holding a literal', 'v="@($"a{ctx.Get("s)v")}b")" x', 'v="@( )" x'], + ['a verbatim interpolated string', 'v="@($@"a""{ctx.Get("(")}")" x', 'v="@( )" x'], + ['an interpolated string with escaped braces', 'v="@($"{{(}}{x}")" x', 'v="@( )" x'], ['an expression inside an XML comment', ' v="@(a)"', ' v="@( )"'], ['an unclosed expression', 'v="@(ctx.Get("a")" x', 'v="@(ctx.Get("a")" x'], ['an @ that starts no expression', 'mail="a@example.com"', 'mail="a@example.com"'], From dffdab2995e32e8e9f2e6c074a219137a3d44900 Mon Sep 17 00:00:00 2001 From: Chris Dayne <1642847+cdayne@users.noreply.github.com> Date: Fri, 6 Nov 2026 18:50:16 +1100 Subject: [PATCH 18/18] fix: leave XML comments as written when rewriting policy references Publish rewrote `{{name}}` tokens and resource-name attributes inside XML comments, and a rewritten name with `--` (an override to `Prod--Key`) made the comment, and the policy, invalid XML. APIM doesn't substitute named values inside comments: on a StandardV2 instance it accepts a commented reference to a missing named value and rejects the same reference outside a comment. Comments are now left as written, as the scans already ignore them. --- docs/guides/multi-environment-shared-apim.md | 2 +- src/services/policy-ref-rewriter.ts | 25 ++++++----- src/services/transitive-resolver.ts | 44 ++++++++++++------- .../unit/services/policy-ref-rewriter.test.ts | 12 +++++ 4 files changed, 55 insertions(+), 28 deletions(-) diff --git a/docs/guides/multi-environment-shared-apim.md b/docs/guides/multi-environment-shared-apim.md index f6a9d16f..f1e50b0f 100644 --- a/docs/guides/multi-environment-shared-apim.md +++ b/docs/guides/multi-environment-shared-apim.md @@ -436,7 +436,7 @@ Publish each with its respective `--service-name` (shared APIM for dev/qa, dedic ## Limitations -- **Policy XML rewriting is regex-based.** The rewriter uses targeted regular expressions for `{{token}}`, `fragment-id="..."`, and `backend-id="..."` patterns. Tokens or attributes inside CDATA sections or XML comments may not be rewritten correctly. Avoid placing canonical resource name references inside CDATA blocks or comments in policy XML. +- **Policy XML rewriting is regex-based.** The rewriter uses targeted regular expressions for `{{token}}`, `fragment-id="..."`, and `backend-id="..."` patterns. XML comments are left as written, since APIM doesn't substitute named values inside them. Tokens or attributes inside CDATA sections may not be rewritten correctly. Avoid placing canonical resource name references inside CDATA blocks in policy XML. - **Workspace container is not affixed.** The Workspace resource name itself is never prefixed/suffixed, even if `Workspace` is in `appliesTo`. Resources *inside* a workspace are affixed normally via their parent workspace's name. - **`ServicePolicy` and singleton types are never affixed.** The service-level policy, API policies, product policies, and operation policies derive their identity from their parent resource and are handled transitively — see [Types that are never affixed](#types-that-are-never-affixed). - **No `--strip-prefix` for extraction.** There is no built-in flag to strip environment prefixes when extracting from a shared APIM. Always extract from a dedicated source-of-truth APIM with canonical names. diff --git a/src/services/policy-ref-rewriter.ts b/src/services/policy-ref-rewriter.ts index 3cacf938..323b99bb 100644 --- a/src/services/policy-ref-rewriter.ts +++ b/src/services/policy-ref-rewriter.ts @@ -7,12 +7,11 @@ * * Design trade-off — regex, not XML parsing: * Aligns with the repo's opaque-payload philosophy (FR-009). XML comments - * () and CDATA sections are NOT explicitly excluded; a token like - * {{name}} inside a comment would technically be rewritten. This risk is - * acceptable because the known-set filter is the primary safety guard — - * only names present in the canonical artifact set are rewritten, so a - * false positive requires an artifact to be named exactly like a comment - * token, which is extremely unlikely in practice. + * () are left as written: APIM doesn't substitute named values + * inside them, and a rewritten name could put `--` in one, which XML + * doesn't allow. CDATA sections are NOT explicitly excluded; the + * known-set filter is the primary safety guard there — only names present + * in the canonical artifact set are rewritten. * * Known-set filter safety: * Only tokens that are the display name of a named value in the source, and @@ -24,7 +23,12 @@ import type { EnvMapping } from './env-mapper.js'; import { toDeployedName } from './env-mapper.js'; import type { KnownArtifactSets } from '../models/config.js'; -import { blankPolicyExpressions, POLICY_REFERENCE_PATTERNS, replaceNamedValueReferences } from './transitive-resolver.js'; +import { + blankPolicyExpressions, + blankXmlComments, + POLICY_REFERENCE_PATTERNS, + replaceNamedValueReferences, +} from './transitive-resolver.js'; // --------------------------------------------------------------------------- // Public API @@ -72,14 +76,15 @@ export function rewritePolicyRefs( // An attribute that names a resource (backend-id, fragment-id, logger-id, // schema-id ...) names one in the policy's scope. A policy expression or a // named value (rewritten above) names none here. - // Matched on a copy with policy expressions blanked (same offsets), so a - // quote inside one doesn't hide the attributes after it. + // Matched on a copy with XML comments and policy expressions blanked (same + // offsets): comments are left as written, and a quote inside an expression + // doesn't hide the attributes after it. for (const { type, pattern } of POLICY_REFERENCE_PATTERNS) { if (!mapping.appliesTo.has(type)) continue; const current = result; let rewritten = ''; let last = 0; - for (const match of blankPolicyExpressions(current).matchAll(pattern)) { + for (const match of blankPolicyExpressions(blankXmlComments(current)).matchAll(pattern)) { const [whole, quote = '', value = ''] = match; const start = match.index ?? 0; const end = start + whole.length; diff --git a/src/services/transitive-resolver.ts b/src/services/transitive-resolver.ts index 5b74783f..79ef62d3 100644 --- a/src/services/transitive-resolver.ts +++ b/src/services/transitive-resolver.ts @@ -140,11 +140,11 @@ export function blankPolicyExpressions(xml: string): string { /** * Policy XML with each XML comment replaced by spaces, the same length: - * references inside comments are not in effect. Comments are found the way - * policySpans finds them, so a `' + + '{{myNv}}' + + '{{myNv}}'; + expect(rewrite(xml, devPrefixMapping(ALL_TYPES))).toBe( + '' + + '{{dev-myNv}}' + + '{{dev-myNv}}' + ); + }); + it('rewrites only a bare {{name}} inside a Liquid body, leaving Liquid output', () => { const xml = '{{myNv}} {{ myNv }} {{myNv | upcase}} {{body.id}}{{ myNv }}'; expect(rewrite(xml, devPrefixMapping([ResourceType.NamedValue]))).toBe(