Fix #357: ignore changes inside readOnly properties in requests

core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/SchemaDiffResult.java

@@ -25,8 +25,12 @@
 import org.openapitools.openapidiff.core.model.schema.ChangedReadOnly;
 import org.openapitools.openapidiff.core.model.schema.ChangedRequired;
 import org.openapitools.openapidiff.core.model.schema.ChangedUniqueItems;
+import org.openapitools.openapidiff.core.utils.RefPointer;
+import org.openapitools.openapidiff.core.utils.RefType;
 
 public class SchemaDiffResult {
+  private static final RefPointer<Schema<?>> refPointer = new RefPointer<>(RefType.SCHEMAS);
+
   protected ChangedSchema changedSchema;
   protected OpenApiDiff openApiDiff;
 
@@ -106,6 +110,22 @@ public <V extends Schema<X>, X> DeferredChanged<ChangedSchema> diff(
     Map<String, Schema> rightProperties = right.getProperties();
     MapKeyDiff<String, Schema> propertyDiff = MapKeyDiff.diff(leftProperties, rightProperties);
     for (String key : propertyDiff.getSharedKey()) {
+      if (isReadOnlyPropertyInRequest(
+          leftComponents,
+          rightComponents,
+          leftProperties.get(key),
+          rightProperties.get(key),
+          context)) {
+        // The property is treated as unchanged, so any parent `required` change for it is
+        // dropped as well; otherwise adding a response-only field to `required` would still
+        // flag the request as breaking.
+        changedSchema.getRequired().getIncreased().remove(key);
+        changedSchema.getRequired().getMissing().remove(key);
+        // Changes inside a readOnly property cannot affect the request contract, since
+        // clients never send readOnly properties, so the property is compared as unchanged.
+        // When the readOnly flag itself differs, one side is not readOnly and is still compared.
+        continue;
+      }
       builder
           .with(
               openApiDiff
@@ -163,6 +183,25 @@ private boolean isPropertyApplicable(Schema<?> schema, DiffContext context) {
         && !(context.isRequest() && Boolean.TRUE.equals(schema.getReadOnly()));
   }
 
+  private boolean isReadOnlyPropertyInRequest(
+      Components leftComponents,
+      Components rightComponents,
+      Schema<?> leftProperty,
+      Schema<?> rightProperty,
+      DiffContext context) {
+    if (!context.isRequest() || leftProperty == null || rightProperty == null) {
+      return false;
+    }
+    // Properties may be `$ref` wrappers without a readOnly flag of their own, so resolve
+    // the refs before checking the flag.
+    Schema<?> leftResolved =
+        refPointer.resolveRef(leftComponents, leftProperty, leftProperty.get$ref());
+    Schema<?> rightResolved =
+        refPointer.resolveRef(rightComponents, rightProperty, rightProperty.get$ref());
+    return Boolean.TRUE.equals(leftResolved.getReadOnly())
+        && Boolean.TRUE.equals(rightResolved.getReadOnly());
+  }
+
   private DiffContext required(DiffContext context, String key, List<String> required) {
     return context.copyWithRequired(required != null && required.contains(key));
   }

core/src/test/java/org/openapitools/openapidiff/core/Issue357Test.java

@@ -0,0 +1,104 @@
+package org.openapitools.openapidiff.core;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.jupiter.api.Test;
+import org.openapitools.openapidiff.core.model.ChangedOpenApi;
+import org.openapitools.openapidiff.core.model.ChangedSchema;
+
+public class Issue357Test {
+
+  private final String READONLY_REQUEST_OLD = "issue-357_1.json";
+  private final String READONLY_REQUEST_NEW = "issue-357_2.json";
+  private final String WRITEONLY_RESPONSE_OLD = "issue-357-writeonly-response_1.json";
+  private final String WRITEONLY_RESPONSE_NEW = "issue-357-writeonly-response_2.json";
+  private final String READONLY_RESPONSE_OLD = "issue-357-readonly-response_1.json";
+  private final String READONLY_RESPONSE_NEW = "issue-357-readonly-response_2.json";
+  private final String READONLY_REQUIRED_OLD = "issue-357-readonly-required_1.json";
+  private final String READONLY_REQUIRED_NEW = "issue-357-readonly-required_2.json";
+  private final String READONLY_REF_OLD = "issue-357-readonly-ref_1.json";
+  private final String READONLY_REF_NEW = "issue-357-readonly-ref_2.json";
+  private final String READONLY_TRANSITION_OFF_OLD = "issue-357-readonly-transition-off_1.json";
+  private final String READONLY_TRANSITION_OFF_NEW = "issue-357-readonly-transition-off_2.json";
+  private final String READONLY_TRANSITION_ON_OLD = "issue-357-readonly-transition-on_1.json";
+  private final String READONLY_TRANSITION_ON_NEW = "issue-357-readonly-transition-on_2.json";
+
+  @Test // issue #357
+  public void changesInsideReadOnlyPropertyInRequestAreIgnored() {
+    ChangedOpenApi changedOpenApi =
+        OpenApiCompare.fromLocations(READONLY_REQUEST_OLD, READONLY_REQUEST_NEW);
+
+    assertThat(changedOpenApi.getChangedOperations()).isEmpty();
+    assertThat(changedOpenApi.getChangedSchemas()).isEmpty();
+    assertThat(changedOpenApi.isUnchanged()).isTrue();
+  }
+
+  @Test // changes to the parent `required` list of a skipped readOnly property are ignored too
+  public void requiredChangeForReadOnlyPropertyInRequestIsIgnored() {
+    ChangedOpenApi changedOpenApi =
+        OpenApiCompare.fromLocations(READONLY_REQUIRED_OLD, READONLY_REQUIRED_NEW);
+
+    assertThat(changedOpenApi.getChangedOperations()).isEmpty();
+    assertThat(changedOpenApi.getChangedSchemas()).isEmpty();
+    assertThat(changedOpenApi.isUnchanged()).isTrue();
+  }
+
+  @Test // a property referenced via `$ref` that resolves to a readOnly schema is ignored too,
+  // including parent `required` changes for the referenced property
+  public void changesInsideReferencedReadOnlyPropertyInRequestAreIgnored() {
+    ChangedOpenApi changedOpenApi =
+        OpenApiCompare.fromLocations(READONLY_REF_OLD, READONLY_REF_NEW);
+
+    assertThat(changedOpenApi.getChangedOperations()).isEmpty();
+    assertThat(changedOpenApi.getChangedSchemas()).isEmpty();
+    assertThat(changedOpenApi.isUnchanged()).isTrue();
+  }
+
+  @Test // readOnly true -> false: the flag change is a real wire change and must be reported
+  public void readOnlyFlagRemovedInRequestStillReportsNestedChanges() {
+    ChangedOpenApi changedOpenApi =
+        OpenApiCompare.fromLocations(READONLY_TRANSITION_OFF_OLD, READONLY_TRANSITION_OFF_NEW);
+
+    assertThat(changedOpenApi.getChangedSchemas()).isNotEmpty();
+    assertThat(changedOpenApi.isChanged().isDifferent()).isTrue();
+    assertThat(changedOpenApi.isIncompatible()).isTrue();
+    assertThat(anyNestedTypeChange(changedOpenApi)).isTrue();
+  }
+
+  @Test // readOnly false -> true: the flag change is a real wire change and must be reported
+  public void readOnlyFlagAddedInRequestStillReportsNestedChanges() {
+    ChangedOpenApi changedOpenApi =
+        OpenApiCompare.fromLocations(READONLY_TRANSITION_ON_OLD, READONLY_TRANSITION_ON_NEW);
+
+    assertThat(changedOpenApi.getChangedSchemas()).isNotEmpty();
+    assertThat(changedOpenApi.isChanged().isDifferent()).isTrue();
+    assertThat(changedOpenApi.isIncompatible()).isTrue();
+    assertThat(anyNestedTypeChange(changedOpenApi)).isTrue();
+  }
+
+  private static boolean anyNestedTypeChange(ChangedOpenApi changedOpenApi) {
+    return changedOpenApi.getChangedSchemas().stream()
+        .flatMap(changedSchema -> changedSchema.getChangedProperties().values().stream())
+        .anyMatch(ChangedSchema::isChangedType);
+  }
+
+  @Test // counter-case: writeOnly in a response is still reported
+  public void changesInsideWriteOnlyPropertyInResponseAreStillReported() {
+    ChangedOpenApi changedOpenApi =
+        OpenApiCompare.fromLocations(WRITEONLY_RESPONSE_OLD, WRITEONLY_RESPONSE_NEW);
+
+    assertThat(changedOpenApi.getChangedSchemas()).isNotEmpty();
+    assertThat(changedOpenApi.isChanged().isDifferent()).isTrue();
+    assertThat(changedOpenApi.isIncompatible()).isTrue();
+  }
+
+  @Test // response-side behavior must stay unchanged: readOnly in a response is still reported
+  public void changesInsideReadOnlyPropertyInResponseAreStillReported() {
+    ChangedOpenApi changedOpenApi =
+        OpenApiCompare.fromLocations(READONLY_RESPONSE_OLD, READONLY_RESPONSE_NEW);
+
+    assertThat(changedOpenApi.getChangedOperations()).isNotEmpty();
+    assertThat(changedOpenApi.isChanged().isDifferent()).isTrue();
+    assertThat(changedOpenApi.isIncompatible()).isTrue();
+  }
+}

core/src/test/resources/issue-357-readonly-ref_1.json

@@ -0,0 +1,68 @@
+{
+  "openapi": "3.0.0",
+  "info": {
+    "title": "API",
+    "version": "0.1.0"
+  },
+  "paths": {
+    "/resource": {
+      "post": {
+        "responses": {
+          "200": {
+            "description": "Created resource",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/CreateResourceResponse"
+                }
+              }
+            }
+          }
+        },
+        "summary": "Create resource",
+        "requestBody": {
+          "content": {
+            "application/json": {
+              "schema": {
+                "$ref": "#/components/schemas/CreateResourceRequest"
+              }
+            }
+          },
+          "description": "Definition of the resource"
+        }
+      }
+    }
+  },
+  "components": {
+    "schemas": {
+      "CreateResourceResponse": {
+        "type": "object",
+        "properties": {
+          "resources": {
+            "type": "array",
+            "items": {
+              "type": "string"
+            }
+          }
+        }
+      },
+      "CreateResourceRequest": {
+        "type": "object",
+        "properties": {
+          "foo": {
+            "$ref": "#/components/schemas/Foo"
+          }
+        }
+      },
+      "Foo": {
+        "type": "object",
+        "readOnly": true,
+        "properties": {
+          "bar": {
+            "type": "string"
+          }
+        }
+      }
+    }
+  }
+}

core/src/test/resources/issue-357-readonly-ref_2.json

@@ -0,0 +1,69 @@
+{
+  "openapi": "3.0.0",
+  "info": {
+    "title": "API",
+    "version": "0.1.0"
+  },
+  "paths": {
+    "/resource": {
+      "post": {
+        "responses": {
+          "200": {
+            "description": "Created resource",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/CreateResourceResponse"
+                }
+              }
+            }
+          }
+        },
+        "summary": "Create resource",
+        "requestBody": {
+          "content": {
+            "application/json": {
+              "schema": {
+                "$ref": "#/components/schemas/CreateResourceRequest"
+              }
+            }
+          },
+          "description": "Definition of the resource"
+        }
+      }
+    }
+  },
+  "components": {
+    "schemas": {
+      "CreateResourceResponse": {
+        "type": "object",
+        "properties": {
+          "resources": {
+            "type": "array",
+            "items": {
+              "type": "string"
+            }
+          }
+        }
+      },
+      "CreateResourceRequest": {
+        "type": "object",
+        "required": ["foo"],
+        "properties": {
+          "foo": {
+            "$ref": "#/components/schemas/Foo"
+          }
+        }
+      },
+      "Foo": {
+        "type": "object",
+        "readOnly": true,
+        "properties": {
+          "bar": {
+            "type": "integer"
+          }
+        }
+      }
+    }
+  }
+}

core/src/test/resources/issue-357-readonly-required_1.json

@@ -0,0 +1,79 @@
+{
+  "openapi": "3.0.0",
+  "info": {
+    "title": "API",
+    "version": "0.1.0"
+  },
+  "paths": {
+    "/resource": {
+      "post": {
+        "responses": {
+          "200": {
+            "description": "Created resource",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/CreateResourceResponse"
+                }
+              }
+            }
+          }
+        },
+        "summary": "Create resource",
+        "requestBody": {
+          "content": {
+            "application/json": {
+              "schema": {
+                "$ref": "#/components/schemas/CreateResourceRequest"
+              }
+            }
+          },
+          "description": "Definition of the resource"
+        }
+      }
+    }
+  },
+  "components": {
+    "schemas": {
+      "CreateResourceResponse": {
+        "type": "object",
+        "properties": {
+          "resources": {
+            "type": "array",
+            "items": {
+              "type": "string"
+            }
+          }
+        }
+      },
+      "CreateResourceRequest": {
+        "type": "object",
+        "properties": {
+          "foo": {
+            "$ref": "#/components/schemas/Foo"
+          }
+        }
+      },
+      "Foo": {
+        "type": "object",
+        "properties": {
+          "bar": {
+            "type": "array",
+            "items": {
+              "$ref": "#/components/schemas/Bar"
+            },
+            "readOnly": true
+          }
+        }
+      },
+      "Bar": {
+        "type": "object",
+        "properties": {
+          "name": {
+            "type": "string"
+          }
+        }
+      }
+    }
+  }
+}