Skip to content

Commit dfbe103

Browse files
Peter HaugeCopilot
andcommitted
fix: correct singleton policy overrides and add policies filter
Bug fixes: - ServicePolicy override no longer throws RangeError (nameParts: []) - ApiPolicy/ProductPolicy singleton child overrides now use fixed name "policy" - ApiOperationPolicy grandchild override uses correct 2-part nameParts - Recursive override parser passes bare section kind for child lookup - Tests updated to use verified descriptor nameParts from buildDescriptor Parity additions: - Added policies filter key for ServicePolicy (Toolkit parity) - Strip apiRevision/isCurrent from API overrides (matches Toolkit) - Workspace sub-filter docs marked as not yet implemented (#119) 957 tests pass, lint and TypeScript clean. Closes #114 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 99fbd6a commit dfbe103

8 files changed

Lines changed: 210 additions & 32 deletions

File tree

docs/guides/filtering-resources.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,9 @@ apis:
104104

105105
### Workspace sub-resource filters
106106

107-
For workspaces, you can specify which workspace-scoped resources to extract:
107+
> **Note:** Workspace sub-resource filtering is parsed but not yet applied at runtime. Currently, including a workspace extracts all resources within it. This matches the Toolkit's configuration format for forward compatibility. See [#119](https://github.com/Azure/apiops-cli/issues/119) for tracking.
108+
109+
The configuration format supports specifying which workspace-scoped resources to extract:
108110

109111
```yaml
110112
workspaces:
@@ -140,6 +142,7 @@ Supported workspace sub-filter keys: `apis`, `backends`, `diagnostics`, `groups`
140142
| `groups` | Groups | `developers`, `partners`, `admins` |
141143
| `subscriptions` | Subscriptions | `team-a-subscription` |
142144
| `schemas` | Global Schemas | `shared-error-schema` |
145+
| `policies` | Service-level Policies | `policy` |
143146
| `policyRestrictions` | Policy Restrictions | `no-external-calls` |
144147
| `documentations` | Documentation | `getting-started`, `changelog` |
145148
| `workspaces` | Workspaces | `team-a-workspace`, `team-b-workspace` |

src/lib/config-loader.ts

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ const FILTER_KEY_ALIASES: Record<FilterYamlKey, string> = {
116116
groups: 'groupNames',
117117
subscriptions: 'subscriptionNames',
118118
schemas: 'schemaNames',
119+
policies: 'policyNames',
119120
policyRestrictions: 'policyRestrictionNames',
120121
documentations: 'documentationNames',
121122
workspaces: 'workspaceNames',
@@ -320,34 +321,42 @@ const OVERRIDE_CHILD_SECTIONS: Record<string, Set<string>> = {
320321
* Normalize one override section into OverrideSection format.
321322
* Supports Toolkit list format: `[{ name, properties, ...childSections }]`
322323
* Recursively parses nested child sections.
324+
*
325+
* @param section - The raw YAML value for this section
326+
* @param displayPath - Full dotted path for error messages (e.g., "apis.my-api.operations")
327+
* @param sectionKind - Bare section key for child lookup (e.g., "operations")
323328
*/
324329
function normalizeOverrideSectionRecursive(
325330
section: unknown,
326-
sectionName: string
331+
displayPath: string,
332+
sectionKind?: string
327333
): OverrideSection | undefined {
334+
// Use sectionKind for child lookup; fall back to displayPath for top-level calls
335+
const lookupKey = sectionKind ?? displayPath;
336+
328337
if (section === undefined || section === null) {
329338
return undefined;
330339
}
331340

332341
if (!Array.isArray(section)) {
333342
throw new Error(
334-
`Invalid overrides.${sectionName}: expected an array in toolkit format ` +
343+
`Invalid overrides.${displayPath}: expected an array in toolkit format ` +
335344
`([ { name, properties } ]), got ${typeof section}.`
336345
);
337346
}
338347

339348
const normalized: OverrideSection = {};
340-
const childKeys = OVERRIDE_CHILD_SECTIONS[sectionName] ?? new Set<string>();
349+
const childKeys = OVERRIDE_CHILD_SECTIONS[lookupKey] ?? new Set<string>();
341350

342351
for (const item of section) {
343352
if (!isPlainObject(item)) {
344-
logger.warn(`Ignoring invalid item in overrides.${sectionName}; expected object.`);
353+
logger.warn(`Ignoring invalid item in overrides.${displayPath}; expected object.`);
345354
continue;
346355
}
347356

348357
const name = item.name;
349358
if (typeof name !== 'string' || name.trim().length === 0) {
350-
logger.warn(`Ignoring item in overrides.${sectionName}; "name" is required.`);
359+
logger.warn(`Ignoring item in overrides.${displayPath}; "name" is required.`);
351360
continue;
352361
}
353362

@@ -356,7 +365,7 @@ function normalizeOverrideSectionRecursive(
356365
if (item.properties !== undefined && item.properties !== null) {
357366
if (!isPlainObject(item.properties)) {
358367
logger.warn(
359-
`Ignoring item '${name}' in overrides.${sectionName}; ` +
368+
`Ignoring item '${name}' in overrides.${displayPath}; ` +
360369
`"properties" must be an object, got ${typeof item.properties}.`
361370
);
362371
continue;
@@ -369,7 +378,7 @@ function normalizeOverrideSectionRecursive(
369378
);
370379
if (Object.keys(fallbackFields).length > 0) {
371380
logger.debug(
372-
`Item '${name}' in overrides.${sectionName} is missing 'properties'; using fields directly.`
381+
`Item '${name}' in overrides.${displayPath} is missing 'properties'; using fields directly.`
373382
);
374383
properties = fallbackFields;
375384
} else {
@@ -382,7 +391,11 @@ function normalizeOverrideSectionRecursive(
382391
for (const childKey of childKeys) {
383392
const childValue: unknown = item[childKey];
384393
if (childValue !== undefined) {
385-
const childSection = normalizeOverrideSectionRecursive(childValue, `${sectionName}.${name}.${childKey}`);
394+
const childSection = normalizeOverrideSectionRecursive(
395+
childValue,
396+
`${displayPath}.${name}.${childKey}`,
397+
childKey
398+
);
386399
if (childSection !== undefined) {
387400
if (!children) children = {};
388401
children[childKey] = childSection;
@@ -396,11 +409,11 @@ function normalizeOverrideSectionRecursive(
396409
// Only add entry if it has properties or children
397410
if (Object.keys(properties).length > 0 || children) {
398411
if (normalized[name] !== undefined) {
399-
logger.warn(`Duplicate name '${name}' in overrides.${sectionName}; later entry overwrites earlier one.`);
412+
logger.warn(`Duplicate name '${name}' in overrides.${displayPath}; later entry overwrites earlier one.`);
400413
}
401414
normalized[name] = entry;
402415
} else {
403-
logger.warn(`Ignoring item '${name}' in overrides.${sectionName}; no override properties or child sections.`);
416+
logger.warn(`Ignoring item '${name}' in overrides.${displayPath}; no override properties or child sections.`);
404417
}
405418
}
406419

src/models/config.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ export interface FilterConfig {
6565
groups?: string[];
6666
subscriptions?: string[];
6767
schemas?: string[];
68+
policies?: string[];
6869
policyRestrictions?: string[];
6970
documentations?: string[];
7071
workspaces?: string[];

src/services/filter-service.ts

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*/
88

99
import { FilterConfig, ApiSubFilter } from '../models/config.js';
10-
import { ResourceType } from '../models/resource-types.js';
10+
import { ResourceType, RESOURCE_TYPE_METADATA } from '../models/resource-types.js';
1111
import { ResourceDescriptor } from '../models/types.js';
1212
import { logger } from '../lib/logger.js';
1313
import { getNamePart } from '../lib/resource-path.js';
@@ -29,6 +29,7 @@ const FILTER_FIELD_MAP: Partial<Record<ResourceType, keyof FilterConfig>> = {
2929
[ResourceType.Group]: 'groups',
3030
[ResourceType.Subscription]: 'subscriptions',
3131
[ResourceType.GlobalSchema]: 'schemas',
32+
[ResourceType.ServicePolicy]: 'policies',
3233
[ResourceType.PolicyRestriction]: 'policyRestrictions',
3334
[ResourceType.Documentation]: 'documentations',
3435
[ResourceType.Workspace]: 'workspaces',
@@ -92,7 +93,11 @@ export function shouldIncludeResource(
9293
// Check direct filter field for this resource type
9394
const directField = FILTER_FIELD_MAP[descriptor.type];
9495
if (directField) {
95-
return matchesFilter(getNamePart(descriptor.nameParts, 0), filter[directField] as string[] | undefined);
96+
// Singleton resources (e.g., ServicePolicy) have nameParts: [] — use fixed name from ARM path
97+
const resourceName = descriptor.nameParts.length > 0
98+
? getNamePart(descriptor.nameParts, 0)
99+
: getSingletonFilterName(descriptor.type);
100+
return matchesFilter(resourceName, filter[directField] as string[] | undefined);
96101
}
97102

98103
// Check parent-based filter for child resource types
@@ -114,15 +119,19 @@ export function shouldIncludeResource(
114119
}
115120
}
116121

117-
// ServicePolicy has no filter — always included
118-
if (descriptor.type === ResourceType.ServicePolicy) {
119-
return true;
120-
}
121-
122122
// Unknown types are included by default
123123
return true;
124124
}
125125

126+
/**
127+
* Get the fixed singleton name for a resource type from its ARM path.
128+
* E.g., ServicePolicy → "policy"
129+
*/
130+
function getSingletonFilterName(type: ResourceType): string {
131+
const meta = RESOURCE_TYPE_METADATA[type];
132+
return meta.armPathSuffix.split('/').pop() ?? '';
133+
}
134+
126135
/**
127136
* Check if an API child resource passes the API sub-resource filter.
128137
* If the parent API has no sub-filter entry, all children are included.

src/services/override-merger.ts

Lines changed: 70 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@
88
*/
99

1010
import { ResourceDescriptor } from '../models/types.js';
11-
import { ResourceType } from '../models/resource-types.js';
11+
import { ResourceType, RESOURCE_TYPE_METADATA } from '../models/resource-types.js';
1212
import { OverrideConfig, OverrideSection, OverrideEntry } from '../models/config.js';
1313
import { logger } from '../lib/logger.js';
14-
import { getNameFromNameParts } from '../lib/resource-path.js';
14+
import { getNameFromNameParts, isSingletonType } from '../lib/resource-path.js';
1515

1616
/**
1717
* Map resource types to their top-level override config section key.
@@ -98,13 +98,15 @@ export function applyOverrides(
9898

9999
/**
100100
* Apply override from a direct section match.
101+
* For singleton resources (e.g., ServicePolicy with nameParts: []),
102+
* uses the fixed singleton name from the ARM path (e.g., "policy").
101103
*/
102104
function applyFromSection(
103105
descriptor: ResourceDescriptor,
104106
json: Record<string, unknown>,
105107
section: OverrideSection
106108
): Record<string, unknown> {
107-
const resourceName = getNameFromNameParts(descriptor.nameParts);
109+
const resourceName = getSingletonOrNamePartName(descriptor);
108110
const entry = findEntryByName(section, resourceName);
109111

110112
if (!entry) {
@@ -116,7 +118,25 @@ function applyFromSection(
116118
}
117119

118120
// ARM resources have all overridable fields inside 'properties'
119-
const wrappedOverride = { properties: entry.properties };
121+
let overrideProperties = entry.properties;
122+
123+
// Strip apiRevision and isCurrent from API overrides — matches Toolkit behavior.
124+
// These fields should not be overridden per environment.
125+
if (descriptor.type === ResourceType.Api) {
126+
const { apiRevision, isCurrent, ...rest } = overrideProperties;
127+
if (apiRevision !== undefined || isCurrent !== undefined) {
128+
logger.warn(
129+
`Ignoring 'apiRevision' and/or 'isCurrent' in API override for '${resourceName}'; ` +
130+
`these fields cannot be overridden (matching Toolkit behavior).`
131+
);
132+
}
133+
overrideProperties = rest;
134+
if (Object.keys(overrideProperties).length === 0) {
135+
return { ...json };
136+
}
137+
}
138+
139+
const wrappedOverride = { properties: overrideProperties };
120140
const result = deepMerge(json, wrappedOverride);
121141

122142
logger.debug(
@@ -129,7 +149,10 @@ function applyFromSection(
129149

130150
/**
131151
* Apply nested child override (e.g., ApiDiagnostic under apis.children.diagnostics).
132-
* nameParts layout: [parentName, childName, ...]
152+
*
153+
* nameParts layout varies by resource type:
154+
* - Named children (ApiDiagnostic, ApiOperation, ApiRelease): [parentName, childName]
155+
* - Singleton children (ApiPolicy, ProductPolicy): [parentName] (child name is always "policy")
133156
*/
134157
function applyNestedOverride(
135158
descriptor: ResourceDescriptor,
@@ -149,8 +172,11 @@ function applyNestedOverride(
149172
const childSection = parentEntry.children[mapping.childKey];
150173
if (!childSection) return { ...json };
151174

152-
// Child name is the second name part (e.g., the diagnostic name, operation name)
153-
const childName = descriptor.nameParts[1];
175+
// For singleton children (e.g., ApiPolicy), the name is fixed (e.g., "policy"),
176+
// NOT in nameParts. For named children, it's nameParts[1].
177+
const childName = isSingletonType(descriptor.type)
178+
? getSingletonName(descriptor.type)
179+
: descriptor.nameParts[1];
154180
if (!childName) return { ...json };
155181

156182
const childEntry = findEntryByName(childSection, childName);
@@ -171,8 +197,10 @@ function applyNestedOverride(
171197

172198
/**
173199
* Apply grandchild (3-level) override.
174-
* E.g., ApiOperationPolicy: apis[apiName].children.operations[opName].children.policies[policyName]
175-
* nameParts layout: [parentName, childName, grandchildName]
200+
* E.g., ApiOperationPolicy: apis[apiName].children.operations[opName].children.policies[policy]
201+
*
202+
* nameParts layout for ApiOperationPolicy: [apiName, operationName]
203+
* The grandchild name is always the fixed singleton name (e.g., "policy").
176204
*/
177205
function applyGrandchildOverride(
178206
descriptor: ResourceDescriptor,
@@ -201,7 +229,8 @@ function applyGrandchildOverride(
201229
const grandchildSection = childEntry.children[mapping.grandchildKey];
202230
if (!grandchildSection) return { ...json };
203231

204-
const grandchildName = descriptor.nameParts[2];
232+
// Grandchild is always a singleton (e.g., "policy") — name is NOT in nameParts
233+
const grandchildName = getSingletonName(descriptor.type);
205234
if (!grandchildName) return { ...json };
206235

207236
const grandchildEntry = findEntryByName(grandchildSection, grandchildName);
@@ -220,6 +249,37 @@ function applyGrandchildOverride(
220249
return result;
221250
}
222251

252+
/**
253+
* Get the resource name for override lookup, handling singletons correctly.
254+
* - Top-level singletons (ServicePolicy with nameParts: []) use the fixed singleton name
255+
* - Named resources use the last element of nameParts
256+
*/
257+
function getSingletonOrNamePartName(descriptor: ResourceDescriptor): string {
258+
if (descriptor.nameParts.length === 0) {
259+
// Top-level singleton (e.g., ServicePolicy)
260+
const name = getSingletonName(descriptor.type);
261+
if (!name) {
262+
throw new RangeError(
263+
`getSingletonOrNamePartName: ${descriptor.type} has empty nameParts ` +
264+
`but no known singleton name`
265+
);
266+
}
267+
return name;
268+
}
269+
return getNameFromNameParts(descriptor.nameParts);
270+
}
271+
272+
/**
273+
* Get the fixed singleton name for a resource type from its ARM path.
274+
* E.g., ServicePolicy → "policy", ApiPolicy → "policy", ApiWiki → "default"
275+
* Returns undefined if the resource type is not a singleton.
276+
*/
277+
function getSingletonName(type: ResourceType): string | undefined {
278+
if (!isSingletonType(type)) return undefined;
279+
const meta = RESOURCE_TYPE_METADATA[type];
280+
return meta.armPathSuffix.split('/').pop();
281+
}
282+
223283
/**
224284
* Find an override entry by name using case-insensitive matching.
225285
*/

src/templates/configs/filter-config.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@ export function generateFilterConfig(): string {
8181
# schemas:
8282
# - pet-schema
8383
84+
# Filter service-level policies
85+
# policies:
86+
# - policy
87+
8488
# Extract only specific policy restrictions
8589
# policyRestrictions:
8690
# - global-policy-restriction

tests/unit/services/filter-service.test.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ describe('filter-service', () => {
134134
expect(shouldIncludeResource(productApi, filter)).toBe(false);
135135
});
136136

137-
it('should always include ServicePolicy', () => {
137+
it('should include ServicePolicy when policies filter is undefined', () => {
138138
const filter: FilterConfig = { apis: [] };
139139
const descriptor: ResourceDescriptor = {
140140
type: ResourceType.ServicePolicy,
@@ -143,6 +143,22 @@ describe('filter-service', () => {
143143
expect(shouldIncludeResource(descriptor, filter)).toBe(true);
144144
});
145145

146+
it('should filter ServicePolicy via policies key', () => {
147+
// ServicePolicy has nameParts: [] — uses fixed singleton name "policy"
148+
const descriptor: ResourceDescriptor = {
149+
type: ResourceType.ServicePolicy,
150+
nameParts: [],
151+
};
152+
153+
// Include when listed
154+
const includeFilter: FilterConfig = { policies: ['policy'] };
155+
expect(shouldIncludeResource(descriptor, includeFilter)).toBe(true);
156+
157+
// Exclude when empty array
158+
const excludeFilter: FilterConfig = { policies: [] };
159+
expect(shouldIncludeResource(descriptor, excludeFilter)).toBe(false);
160+
});
161+
146162
it('should filter named values', () => {
147163
const filter: FilterConfig = { namedValues: ['my-secret'] };
148164
const included: ResourceDescriptor = {

0 commit comments

Comments
 (0)