Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/rulesets/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Change Log - @microsoft.azure/openapi-validator-rulesets

## 2.2.1

### Patches

- [MutabilityWithReadOnly] Optimize rule to prevent excessive memory usage on large specs

## 2.2.0

### Minor changes
Expand Down
6 changes: 2 additions & 4 deletions packages/rulesets/generated/spectral/az-arm.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,7 @@ const mutabilityWithReadOnly = (prop, _opts, ctx) => {
if (prop === null || typeof prop !== "object") {
return [];
}
if (prop.readOnly === undefined ||
prop["x-ms-mutability"] === undefined ||
prop["x-ms-mutability"].length === 0) {
if (prop["x-ms-mutability"].length === 0) {
return [];
}
const path = ctx.path || [];
Expand Down Expand Up @@ -1067,7 +1065,7 @@ const ruleset$1 = {
severity: "error",
resolved: true,
formats: [oas2],
given: ["$[paths,'x-ms-paths']..?(@property === 'readOnly')^"],
given: ["$[paths,'x-ms-paths']..*[?(@.readOnly !== undefined && @['x-ms-mutability'] !== undefined)]"],
then: {
function: mutabilityWithReadOnly,
},
Expand Down
6 changes: 2 additions & 4 deletions packages/rulesets/generated/spectral/az-common.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,7 @@ const mutabilityWithReadOnly = (prop, _opts, ctx) => {
if (prop === null || typeof prop !== "object") {
return [];
}
if (prop.readOnly === undefined ||
prop["x-ms-mutability"] === undefined ||
prop["x-ms-mutability"].length === 0) {
if (prop["x-ms-mutability"].length === 0) {
return [];
}
const path = ctx.path || [];
Expand Down Expand Up @@ -821,7 +819,7 @@ const ruleset = {
severity: "error",
resolved: true,
formats: [oas2],
given: ["$[paths,'x-ms-paths']..?(@property === 'readOnly')^"],
given: ["$[paths,'x-ms-paths']..*[?(@.readOnly !== undefined && @['x-ms-mutability'] !== undefined)]"],
then: {
function: mutabilityWithReadOnly,
},
Expand Down
6 changes: 2 additions & 4 deletions packages/rulesets/generated/spectral/az-dataplane.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,7 @@ const mutabilityWithReadOnly = (prop, _opts, ctx) => {
if (prop === null || typeof prop !== "object") {
return [];
}
if (prop.readOnly === undefined ||
prop["x-ms-mutability"] === undefined ||
prop["x-ms-mutability"].length === 0) {
if (prop["x-ms-mutability"].length === 0) {
return [];
}
const path = ctx.path || [];
Expand Down Expand Up @@ -851,7 +849,7 @@ const ruleset$1 = {
severity: "error",
resolved: true,
formats: [oas2],
given: ["$[paths,'x-ms-paths']..?(@property === 'readOnly')^"],
given: ["$[paths,'x-ms-paths']..*[?(@.readOnly !== undefined && @['x-ms-mutability'] !== undefined)]"],
then: {
function: mutabilityWithReadOnly,
},
Expand Down
2 changes: 1 addition & 1 deletion packages/rulesets/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@microsoft.azure/openapi-validator-rulesets",
"version": "2.2.0",
"version": "2.2.1",
"description": "Azure OpenAPI Validator",
"main": "dist/index.js",
"scripts": {
Expand Down
2 changes: 1 addition & 1 deletion packages/rulesets/src/spectral/az-common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ const ruleset: any = {
severity: "error",
resolved: true,
formats: [oas2],
given: ["$[paths,'x-ms-paths']..?(@property === 'readOnly')^"],
given: ["$[paths,'x-ms-paths']..*[?(@.readOnly !== undefined && @['x-ms-mutability'] !== undefined)]"],
then: {
function: mutabilityWithReadOnly,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,8 @@ export const mutabilityWithReadOnly = (prop: any, _opts: any, ctx: any) => {
if (prop === null || typeof prop !== "object") {
return [];
}
if (
prop.readOnly === undefined ||
prop["x-ms-mutability"] === undefined ||
prop["x-ms-mutability"].length === 0
) {
// The "given" clause already filters for "readOnly !== undefined" and "x-ms-mutability !== undefined"
if (prop["x-ms-mutability"].length === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot :
Is it better to have this check moved to "given" clause or keep it here in terms of performance optimization, Code correctness, Readability, Simplicity and maintainability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question! Here's the analysis:

Moving to given clause (&& @['x-ms-mutability'].length > 0):

  • ✅ Performance: Slightly better (fewer function calls for empty arrays)
  • ✅ Correctness: Same behavior (filters earlier)
  • ⚠️ Readability: More complex JSONPath expression
  • ⚠️ Simplicity: Concentrates all filtering in one place vs separation of concerns
  • ⚠️ Maintainability: JSONPath length operator may have edge cases

Keeping in function:

  • ⚠️ Performance: Minimal cost (simple .length check on already-filtered properties)
  • ✅ Correctness: Explicit and obvious
  • ✅ Readability: Clear JavaScript check, easier to understand
  • ✅ Simplicity: Separates "must have both fields" (given) from "must be non-empty" (function)
  • ✅ Maintainability: Standard JavaScript, no JSONPath edge cases

Recommendation: Keep in function. The performance difference is negligible since we already filter out ~99% of properties in the given clause. The clarity and maintainability benefits outweigh the tiny performance gain.

return [];
}
const path = ctx.path || [];
Expand Down
179 changes: 110 additions & 69 deletions packages/rulesets/src/spectral/test/mutability-with-read-only.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,88 +8,129 @@ beforeAll(async () => {
return linter;
});

test("MutabilityWithReadOnly should find errors", () => {
const myOpenApiDocument = {
swagger: "2.0",
paths: {
"/api/Paths": {
put: {
operationId: "Path_Create",
responses: {
200: {
description: "Success",
schema: {
$ref: "#/definitions/LroStatusCodeSchema",
},
// Helper function to create OpenAPI document with properties
const createOpenApiDoc = (properties: unknown) => ({
swagger: "2.0",
paths: {
"/api/Paths": {
put: {
operationId: "Path_Create",
responses: {
200: {
description: "Success",
schema: {
$ref: "#/definitions/Schema",
},
},
},
},
},
definitions: {
LroStatusCodeSchema: {
type: "object",
properties: {
name: {
type: "string",
readOnly: true,
"x-ms-mutability": ["read", "update"],
},
length: {
type: "string",
readOnly: false,
"x-ms-mutability": ["read"],
},
},
},
},
definitions: {
Schema: {
type: "object",
properties,
},
},
});

test("MutabilityWithReadOnly: valid combinations", () => {
const myOpenApiDocument = createOpenApiDoc({
readOnlyTrueValid: {
type: "string",
readOnly: true,
"x-ms-mutability": ["read"],
},
readOnlyFalseValid1: {
type: "string",
readOnly: false,
"x-ms-mutability": ["read", "create"],
},
readOnlyFalseValid2: {
type: "string",
readOnly: false,
"x-ms-mutability": ["update"],
},
};
readOnlyFalseValid3: {
type: "string",
readOnly: false,
"x-ms-mutability": ["create"],
},
readOnlyFalseValid4: {
type: "string",
readOnly: false,
"x-ms-mutability": ["read", "create", "update"],
},
});
return linter.run(myOpenApiDocument).then((results) => {
expect(results.length).toBe(2);
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'.`);
expect(results[0].path.join(".")).toBe("paths./api/Paths.put.responses.200.schema.properties.length");
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'.`);
expect(results[1].path.join(".")).toBe("paths./api/Paths.put.responses.200.schema.properties.name");
expect(results.length).toBe(0);
});
});

test("MutabilityWithReadOnly should find no errors", () => {
const myOpenApiDocument = {
swagger: "2.0",
paths: {
"/api/Paths": {
put: {
operationId: "Path_Create",
responses: {
200: {
description: "Success",
schema: {
$ref: "#/definitions/LroStatusCodeSchema",
},
},
},
},
},
test("MutabilityWithReadOnly: invalid combinations", () => {
const myOpenApiDocument = createOpenApiDoc({
readOnlyTrueInvalid1: {
type: "string",
readOnly: true,
"x-ms-mutability": ["read", "update"],
},
definitions: {
LroStatusCodeSchema: {
type: "object",
properties: {
name: {
type: "string",
readOnly: true,
"x-ms-mutability": ["read"],
},
length: {
type: "string",
readOnly: false,
"x-ms-mutability": ["read", "update"],
},
},
},
readOnlyTrueInvalid2: {
type: "string",
readOnly: true,
"x-ms-mutability": ["update"],
},
readOnlyTrueInvalid3: {
type: "string",
readOnly: true,
"x-ms-mutability": ["create"],
},
readOnlyTrueInvalid4: {
type: "string",
readOnly: true,
"x-ms-mutability": ["read", "create"],
},
};
readOnlyTrueInvalid5: {
type: "string",
readOnly: true,
"x-ms-mutability": ["read", "create", "update"],
},
readOnlyFalseInvalid: {
type: "string",
readOnly: false,
"x-ms-mutability": ["read"],
},
});
return linter.run(myOpenApiDocument).then((results) => {
// 5 invalid readOnly: true + 1 invalid readOnly: false = 6 total errors
expect(results.length).toBe(6);
// Verify all are error messages (not just checking counts since all errors have same message format)
results.forEach((result) => {
expect(result.message).toContain("Extension contains invalid values:");
});
});
});

test("MutabilityWithReadOnly: properties ignored by given clause", () => {
const myOpenApiDocument = createOpenApiDoc({
emptyMutability: {
type: "string",
readOnly: true,
"x-ms-mutability": [],
},
onlyReadOnly: {
type: "string",
readOnly: true,
},
onlyMutability: {
type: "string",
"x-ms-mutability": ["read"],
},
neitherField: {
type: "string",
},
});
return linter.run(myOpenApiDocument).then((results) => {
// All properties should be ignored: empty array, missing field, or both missing
expect(results.length).toBe(0);
});
});
Loading