Skip to content

Commit b812e6c

Browse files
CopilotAkhilaIllamikeharder
authored
[MutabilityWithReadOnly] Optimize to prevent excessive memory usage on large specs (#813)
* Initial plan * Fix MutabilityWithReadOnly rule to avoid out of memory issue by filtering earlier Co-authored-by: AkhilaIlla <36493984+AkhilaIlla@users.noreply.github.com> * Remove redundant checks from mutability-with-read-only function Co-authored-by: AkhilaIlla <36493984+AkhilaIlla@users.noreply.github.com> * Add Array.isArray check for x-ms-mutability defensive programming Co-authored-by: AkhilaIlla <36493984+AkhilaIlla@users.noreply.github.com> * Move length check to given clause for maximum efficiency Co-authored-by: AkhilaIlla <36493984+AkhilaIlla@users.noreply.github.com> * Remove Array.isArray check that changed behavior - keep performance optimization only Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> * Simplify to minimal change: only filter readOnly and x-ms-mutability in given clause Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> * Add comprehensive tests for all readOnly and x-ms-mutability combinations Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> * Add tests for properties with omitted readOnly or x-ms-mutability fields Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> * Refactor tests to reduce redundancy using helper function and consolidated test cases Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> * Change helper function type from any to unknown for better type safety Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> * improve comment * Add Rush change file for MutabilityWithReadOnly performance fix Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> * Update package.json to 2.2.1 and add CHANGELOG entry Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> * improve changelog * Apply suggestion from @mikeharder --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: AkhilaIlla <36493984+AkhilaIlla@users.noreply.github.com> Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> Co-authored-by: Mike Harder <mharder@microsoft.com>
1 parent f299af5 commit b812e6c

8 files changed

Lines changed: 126 additions & 88 deletions

File tree

‎packages/rulesets/CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
# Change Log - @microsoft.azure/openapi-validator-rulesets
22

3+
## 2.2.1
4+
5+
### Patches
6+
7+
- [MutabilityWithReadOnly] Optimize rule to prevent excessive memory usage on large specs
8+
39
## 2.2.0
410

511
### Minor changes

‎packages/rulesets/generated/spectral/az-arm.js‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -144,9 +144,7 @@ const mutabilityWithReadOnly = (prop, _opts, ctx) => {
144144
if (prop === null || typeof prop !== "object") {
145145
return [];
146146
}
147-
if (prop.readOnly === undefined ||
148-
prop["x-ms-mutability"] === undefined ||
149-
prop["x-ms-mutability"].length === 0) {
147+
if (prop["x-ms-mutability"].length === 0) {
150148
return [];
151149
}
152150
const path = ctx.path || [];
@@ -1067,7 +1065,7 @@ const ruleset$1 = {
10671065
severity: "error",
10681066
resolved: true,
10691067
formats: [oas2],
1070-
given: ["$[paths,'x-ms-paths']..?(@property === 'readOnly')^"],
1068+
given: ["$[paths,'x-ms-paths']..*[?(@.readOnly !== undefined && @['x-ms-mutability'] !== undefined)]"],
10711069
then: {
10721070
function: mutabilityWithReadOnly,
10731071
},

‎packages/rulesets/generated/spectral/az-common.js‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -141,9 +141,7 @@ const mutabilityWithReadOnly = (prop, _opts, ctx) => {
141141
if (prop === null || typeof prop !== "object") {
142142
return [];
143143
}
144-
if (prop.readOnly === undefined ||
145-
prop["x-ms-mutability"] === undefined ||
146-
prop["x-ms-mutability"].length === 0) {
144+
if (prop["x-ms-mutability"].length === 0) {
147145
return [];
148146
}
149147
const path = ctx.path || [];
@@ -821,7 +819,7 @@ const ruleset = {
821819
severity: "error",
822820
resolved: true,
823821
formats: [oas2],
824-
given: ["$[paths,'x-ms-paths']..?(@property === 'readOnly')^"],
822+
given: ["$[paths,'x-ms-paths']..*[?(@.readOnly !== undefined && @['x-ms-mutability'] !== undefined)]"],
825823
then: {
826824
function: mutabilityWithReadOnly,
827825
},

‎packages/rulesets/generated/spectral/az-dataplane.js‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -141,9 +141,7 @@ const mutabilityWithReadOnly = (prop, _opts, ctx) => {
141141
if (prop === null || typeof prop !== "object") {
142142
return [];
143143
}
144-
if (prop.readOnly === undefined ||
145-
prop["x-ms-mutability"] === undefined ||
146-
prop["x-ms-mutability"].length === 0) {
144+
if (prop["x-ms-mutability"].length === 0) {
147145
return [];
148146
}
149147
const path = ctx.path || [];
@@ -851,7 +849,7 @@ const ruleset$1 = {
851849
severity: "error",
852850
resolved: true,
853851
formats: [oas2],
854-
given: ["$[paths,'x-ms-paths']..?(@property === 'readOnly')^"],
852+
given: ["$[paths,'x-ms-paths']..*[?(@.readOnly !== undefined && @['x-ms-mutability'] !== undefined)]"],
855853
then: {
856854
function: mutabilityWithReadOnly,
857855
},

‎packages/rulesets/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@microsoft.azure/openapi-validator-rulesets",
3-
"version": "2.2.0",
3+
"version": "2.2.1",
44
"description": "Azure OpenAPI Validator",
55
"main": "dist/index.js",
66
"scripts": {

‎packages/rulesets/src/spectral/az-common.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ const ruleset: any = {
263263
severity: "error",
264264
resolved: true,
265265
formats: [oas2],
266-
given: ["$[paths,'x-ms-paths']..?(@property === 'readOnly')^"],
266+
given: ["$[paths,'x-ms-paths']..*[?(@.readOnly !== undefined && @['x-ms-mutability'] !== undefined)]"],
267267
then: {
268268
function: mutabilityWithReadOnly,
269269
},

‎packages/rulesets/src/spectral/functions/Extensions/mutability-with-read-only.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,8 @@ export const mutabilityWithReadOnly = (prop: any, _opts: any, ctx: any) => {
44
if (prop === null || typeof prop !== "object") {
55
return [];
66
}
7-
if (
8-
prop.readOnly === undefined ||
9-
prop["x-ms-mutability"] === undefined ||
10-
prop["x-ms-mutability"].length === 0
11-
) {
7+
// The "given" clause already filters for "readOnly !== undefined" and "x-ms-mutability !== undefined"
8+
if (prop["x-ms-mutability"].length === 0) {
129
return [];
1310
}
1411
const path = ctx.path || [];

‎packages/rulesets/src/spectral/test/mutability-with-read-only.test.ts‎

Lines changed: 110 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -8,88 +8,129 @@ beforeAll(async () => {
88
return linter;
99
});
1010

11-
test("MutabilityWithReadOnly should find errors", () => {
12-
const myOpenApiDocument = {
13-
swagger: "2.0",
14-
paths: {
15-
"/api/Paths": {
16-
put: {
17-
operationId: "Path_Create",
18-
responses: {
19-
200: {
20-
description: "Success",
21-
schema: {
22-
$ref: "#/definitions/LroStatusCodeSchema",
23-
},
11+
// Helper function to create OpenAPI document with properties
12+
const createOpenApiDoc = (properties: unknown) => ({
13+
swagger: "2.0",
14+
paths: {
15+
"/api/Paths": {
16+
put: {
17+
operationId: "Path_Create",
18+
responses: {
19+
200: {
20+
description: "Success",
21+
schema: {
22+
$ref: "#/definitions/Schema",
2423
},
2524
},
2625
},
2726
},
2827
},
29-
definitions: {
30-
LroStatusCodeSchema: {
31-
type: "object",
32-
properties: {
33-
name: {
34-
type: "string",
35-
readOnly: true,
36-
"x-ms-mutability": ["read", "update"],
37-
},
38-
length: {
39-
type: "string",
40-
readOnly: false,
41-
"x-ms-mutability": ["read"],
42-
},
43-
},
44-
},
28+
},
29+
definitions: {
30+
Schema: {
31+
type: "object",
32+
properties,
33+
},
34+
},
35+
});
36+
37+
test("MutabilityWithReadOnly: valid combinations", () => {
38+
const myOpenApiDocument = createOpenApiDoc({
39+
readOnlyTrueValid: {
40+
type: "string",
41+
readOnly: true,
42+
"x-ms-mutability": ["read"],
43+
},
44+
readOnlyFalseValid1: {
45+
type: "string",
46+
readOnly: false,
47+
"x-ms-mutability": ["read", "create"],
48+
},
49+
readOnlyFalseValid2: {
50+
type: "string",
51+
readOnly: false,
52+
"x-ms-mutability": ["update"],
4553
},
46-
};
54+
readOnlyFalseValid3: {
55+
type: "string",
56+
readOnly: false,
57+
"x-ms-mutability": ["create"],
58+
},
59+
readOnlyFalseValid4: {
60+
type: "string",
61+
readOnly: false,
62+
"x-ms-mutability": ["read", "create", "update"],
63+
},
64+
});
4765
return linter.run(myOpenApiDocument).then((results) => {
48-
expect(results.length).toBe(2);
49-
expect(results[0].message).toBe(`When property is modeled as "readOnly": true then x-ms-mutability extension can only have "read" value. When property is modeled as "readOnly": false then applying x-ms-mutability extension with only "read" value is not allowed. Extension contains invalid values: 'read'.`);
50-
expect(results[0].path.join(".")).toBe("paths./api/Paths.put.responses.200.schema.properties.length");
51-
expect(results[1].message).toBe(`When property is modeled as "readOnly": true then x-ms-mutability extension can only have "read" value. When property is modeled as "readOnly": false then applying x-ms-mutability extension with only "read" value is not allowed. Extension contains invalid values: 'read, update'.`);
52-
expect(results[1].path.join(".")).toBe("paths./api/Paths.put.responses.200.schema.properties.name");
66+
expect(results.length).toBe(0);
5367
});
5468
});
5569

56-
test("MutabilityWithReadOnly should find no errors", () => {
57-
const myOpenApiDocument = {
58-
swagger: "2.0",
59-
paths: {
60-
"/api/Paths": {
61-
put: {
62-
operationId: "Path_Create",
63-
responses: {
64-
200: {
65-
description: "Success",
66-
schema: {
67-
$ref: "#/definitions/LroStatusCodeSchema",
68-
},
69-
},
70-
},
71-
},
72-
},
70+
test("MutabilityWithReadOnly: invalid combinations", () => {
71+
const myOpenApiDocument = createOpenApiDoc({
72+
readOnlyTrueInvalid1: {
73+
type: "string",
74+
readOnly: true,
75+
"x-ms-mutability": ["read", "update"],
7376
},
74-
definitions: {
75-
LroStatusCodeSchema: {
76-
type: "object",
77-
properties: {
78-
name: {
79-
type: "string",
80-
readOnly: true,
81-
"x-ms-mutability": ["read"],
82-
},
83-
length: {
84-
type: "string",
85-
readOnly: false,
86-
"x-ms-mutability": ["read", "update"],
87-
},
88-
},
89-
},
77+
readOnlyTrueInvalid2: {
78+
type: "string",
79+
readOnly: true,
80+
"x-ms-mutability": ["update"],
81+
},
82+
readOnlyTrueInvalid3: {
83+
type: "string",
84+
readOnly: true,
85+
"x-ms-mutability": ["create"],
86+
},
87+
readOnlyTrueInvalid4: {
88+
type: "string",
89+
readOnly: true,
90+
"x-ms-mutability": ["read", "create"],
9091
},
91-
};
92+
readOnlyTrueInvalid5: {
93+
type: "string",
94+
readOnly: true,
95+
"x-ms-mutability": ["read", "create", "update"],
96+
},
97+
readOnlyFalseInvalid: {
98+
type: "string",
99+
readOnly: false,
100+
"x-ms-mutability": ["read"],
101+
},
102+
});
103+
return linter.run(myOpenApiDocument).then((results) => {
104+
// 5 invalid readOnly: true + 1 invalid readOnly: false = 6 total errors
105+
expect(results.length).toBe(6);
106+
// Verify all are error messages (not just checking counts since all errors have same message format)
107+
results.forEach((result) => {
108+
expect(result.message).toContain("Extension contains invalid values:");
109+
});
110+
});
111+
});
112+
113+
test("MutabilityWithReadOnly: properties ignored by given clause", () => {
114+
const myOpenApiDocument = createOpenApiDoc({
115+
emptyMutability: {
116+
type: "string",
117+
readOnly: true,
118+
"x-ms-mutability": [],
119+
},
120+
onlyReadOnly: {
121+
type: "string",
122+
readOnly: true,
123+
},
124+
onlyMutability: {
125+
type: "string",
126+
"x-ms-mutability": ["read"],
127+
},
128+
neitherField: {
129+
type: "string",
130+
},
131+
});
92132
return linter.run(myOpenApiDocument).then((results) => {
133+
// All properties should be ignored: empty array, missing field, or both missing
93134
expect(results.length).toBe(0);
94135
});
95136
});

0 commit comments

Comments
 (0)
⚡