From 917f85748a6451fc0f57fcba15c388adb53be02d Mon Sep 17 00:00:00 2001 From: mishukdutta-toast Date: Thu, 10 Mar 2022 17:32:57 -0500 Subject: [PATCH] BUG#348 parameter data detection fix when pathparams changed * When endpoint pathParams change and Parameter names are changed to be similar to pathParams - Don't remove parameters from the path --- .../core/compare/OperationDiff.java | 11 +-- .../core/compare/ParametersDiff.java | 69 +++++++++++++++++-- .../openapidiff/core/compare/PathDiff.java | 9 ++- .../openapidiff/core/compare/PathsDiff.java | 3 +- .../openapidiff/core/model/DiffContext.java | 18 +++++ .../core/PathParameterSchemaDiffTest.java | 53 ++++++++++++++ ...th_parameter_diff_param_name_diff_new.yaml | 48 +++++++++++++ ...th_parameter_diff_param_name_diff_old.yaml | 48 +++++++++++++ ..._parameter_diff_param_schema_diff_new.yaml | 48 +++++++++++++ ..._parameter_diff_param_schema_diff_old.yaml | 48 +++++++++++++ 10 files changed, 343 insertions(+), 12 deletions(-) create mode 100644 core/src/test/java/org/openapitools/openapidiff/core/PathParameterSchemaDiffTest.java create mode 100644 core/src/test/resources/path_parameter_diff_param_name_diff_new.yaml create mode 100644 core/src/test/resources/path_parameter_diff_param_name_diff_old.yaml create mode 100644 core/src/test/resources/path_parameter_diff_param_schema_diff_new.yaml create mode 100644 core/src/test/resources/path_parameter_diff_param_schema_diff_old.yaml diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/OperationDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/OperationDiff.java index a2bc17244..e226043f3 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/OperationDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/OperationDiff.java @@ -72,14 +72,15 @@ public DeferredChanged diff( .ifPresent(changedOperation::setRequestBody); } + ParametersDiffResult parametersDiffResult = + openApiDiff.getParametersDiff().diff(oldOperation.getParameters(), newOperation.getParameters(), context); builder - .with( - openApiDiff - .getParametersDiff() - .diff(oldOperation.getParameters(), newOperation.getParameters(), context)) + .with(parametersDiffResult.deferredChanged) .ifPresent( params -> { - removePathParameters(context.getParameters(), params); + if (!parametersDiffResult.sameOperationsDiffSchema) { + removePathParameters(context.getParameters(), params); + } changedOperation.setParameters(params); }); diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/ParametersDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/ParametersDiff.java index c7fb99b08..c6bf420e8 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/ParametersDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/ParametersDiff.java @@ -6,6 +6,8 @@ import java.util.List; import java.util.Objects; import java.util.Optional; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.openapitools.openapidiff.core.model.Changed; import org.openapitools.openapidiff.core.model.ChangedParameters; import org.openapitools.openapidiff.core.model.DiffContext; @@ -14,6 +16,15 @@ import org.openapitools.openapidiff.core.utils.RefPointer; import org.openapitools.openapidiff.core.utils.RefType; +class ParametersDiffResult { + protected DeferredChanged deferredChanged; + protected boolean sameOperationsDiffSchema; + + public ParametersDiffResult(DeferredChanged deferredChanged, boolean sameOperationsDiffSchema) { + this.deferredChanged = deferredChanged; + this.sameOperationsDiffSchema = sameOperationsDiffSchema; + } +} /** compare two parameter */ public class ParametersDiff { private static final RefPointer refPointer = new RefPointer<>(RefType.PARAMETERS); @@ -46,9 +57,7 @@ public static boolean same(Parameter left, Parameter right) { && Objects.equals(left.getIn(), right.getIn()); } - public DeferredChanged diff( - List left, List right, DiffContext context) { - + public ParametersDiffResult diff(List left, List right, DiffContext context) { DeferredBuilder builder = new DeferredBuilder<>(); ChangedParameters changedParameters = new ChangedParameters(left, right != null ? new ArrayList<>(right) : null, context); @@ -70,7 +79,59 @@ public DeferredChanged diff( } } changedParameters.getIncreased().addAll(right); + return new ParametersDiffResult( + builder.buildIsChanged(changedParameters), + pathUnchangedParametersChanged(changedParameters, context) + ); + } + + public boolean pathUnchangedParametersChanged( + ChangedParameters changedParameters, DiffContext context) { + if (!pathUnchanged(changedParameters, context)) return false; + // If missing and increased parameter count is different, it's a new endpoint + if (changedParameters.getMissing().size() != changedParameters.getIncreased().size()) + return false; + // Go through each missing Parameter and compare it to newly added Parameters + for (Parameter parameter : changedParameters.getMissing()) { + // Speedy Check. Use the map already created in changedParameters to check if missing param is linked to newParam + String newParameterName = context.getParameters().get(parameter.getName()); + if (newParameterName.isEmpty()) return false; + + Optional newParameter = + changedParameters.getIncreased().stream() + .filter(p -> p.getName().equals(newParameterName)) + .findFirst(); + if (!newParameter.isPresent()) return false; - return builder.buildIsChanged(changedParameters); + // Check if the old and new Parameters match . IF so, return TRUE + Parameter newParameterRealized = newParameter.get(); + newParameterRealized.setName(parameter.getName()); // Make names similar + boolean samePathDifferentParameter = !newParameterRealized.equals(parameter); + newParameterRealized.setName(newParameterName); // Important:: MUST Reset the name as this is not a copy + return samePathDifferentParameter; + } + return false; + } + + public boolean pathUnchanged(ChangedParameters changedParameters, DiffContext context) { + final String REGEX_PATH = "\\{([^/]+)}"; + String oldUrl = context.getLeftUrl(); + String newUrl = context.getRightUrl(); + ArrayList oldUrlPathParams = matchedItems(oldUrl, REGEX_PATH); + ArrayList newUrlPathParams = matchedItems(newUrl, REGEX_PATH); + // Path Param count doesn't match or param-less path doesn't match or param is changed --> It's a new endpoint + return oldUrlPathParams.size() == newUrlPathParams.size() + && changedParameters.getChanged().isEmpty() + && oldUrl.replaceAll(REGEX_PATH, "").equals(newUrl.replaceAll(REGEX_PATH, "")); + } + + public ArrayList matchedItems(String string, String regex) { + Matcher matcher = Pattern.compile(regex).matcher(string); + ArrayList matchedItems = new ArrayList<>(); + while (matcher.find()) { + String item = matcher.group(); + matchedItems.add(item.substring(0, matcher.group().length() - 1).replaceFirst("\\{", "")); + } + return matchedItems; } } diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/PathDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/PathDiff.java index 0537907a0..7d8e8baa8 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/PathDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/PathDiff.java @@ -36,8 +36,13 @@ public DeferredChanged diff(PathItem left, PathItem right, DiffCont .with( openApiDiff .getOperationDiff() - .diff(oldOperation, newOperation, context.copyWithMethod(method))) - .ifPresent(changedPath.getChanged()::add); + .diff(oldOperation, newOperation, + context.copyWithMethod(method).copyWithLeftRightUrls( + context.getLeftUrl(), + context.getRightUrl() + ) + ) + ).ifPresent(changedPath.getChanged()::add); } builder .with( diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/PathsDiff.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/PathsDiff.java index e09a4557b..7d46528d6 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/PathsDiff.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/PathsDiff.java @@ -85,6 +85,7 @@ public DeferredChanged diff( DiffContext context = new DiffContext(); context.setUrl(url); context.setParameters(params); + context.setLeftAndRightUrls(url, rightUrl); builder .with(openApiDiff.getPathDiff().diff(leftPath, rightPath, context)) .ifPresent(path -> changedPaths.getChanged().put(rightUrl, path)); @@ -131,7 +132,7 @@ private static boolean methodsAndParametersIntersect(PathItem a, PathItem b) { * @param right parameters from the second compared method * @return true in case each parameter pair is of the same type; false otherwise */ - private static boolean parametersIntersect(List left, List right) {; + private static boolean parametersIntersect(List left, List right) { int parametersSize = left.size(); long intersectedParameters = IntStream.range(0, left.size()) .filter(i -> left.get(i).getSchema().getType().equals(right.get(i).getSchema().getType())) diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/DiffContext.java b/core/src/main/java/org/openapitools/openapidiff/core/model/DiffContext.java index 7c8716ed4..718cb60f8 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/model/DiffContext.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/DiffContext.java @@ -14,6 +14,8 @@ public class DiffContext { private boolean response; private boolean request; private Boolean required; + private String leftUrl; + private String rightUrl; public DiffContext() { parameters = new HashMap<>(); @@ -37,6 +39,10 @@ public DiffContext copyAsResponse() { return copy().setResponse(); } + public DiffContext copyWithLeftRightUrls(String leftUrl, String rightUrl) { + return copy().setLeftAndRightUrls(leftUrl, rightUrl); + } + private DiffContext setRequest() { this.request = true; this.response = false; @@ -104,6 +110,16 @@ private DiffContext setRequired(boolean required) { return this; } + public DiffContext setLeftAndRightUrls(String leftUrl, String rightUrl) { + this.leftUrl = leftUrl; + this.rightUrl = rightUrl; + return this; + } + + public String getLeftUrl() { return this.leftUrl; } + + public String getRightUrl() { return this.rightUrl; } + @Override public boolean equals(Object o) { if (this == o) return true; @@ -131,6 +147,8 @@ public int hashCode() { .append(response) .append(request) .append(required) + .append(leftUrl) + .append(rightUrl) .toHashCode(); } } diff --git a/core/src/test/java/org/openapitools/openapidiff/core/PathParameterSchemaDiffTest.java b/core/src/test/java/org/openapitools/openapidiff/core/PathParameterSchemaDiffTest.java new file mode 100644 index 000000000..1c4065ed9 --- /dev/null +++ b/core/src/test/java/org/openapitools/openapidiff/core/PathParameterSchemaDiffTest.java @@ -0,0 +1,53 @@ +package org.openapitools.openapidiff.core; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.openapitools.openapidiff.core.model.ChangedOpenApi; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class PathParameterSchemaDiffTest { + final String TEST_MSG_1 = + "Testing: \n" + + "1. Same path but different pathParameters\n" + + "2. different parameters in the parameters: section\n" + + "3. Parameters have different schema\n" + + "eg:\n" + + "old path -- students/{id}\n" + + "old schema -- id: integer\n" + + "new path -- students/{username}\n" + + "new schema -- username: string"; + + final String TEST_MSG_2 = + "Testing: \n" + + "1. Same path but different pathParameters\n" + + "2. different parameters in the parameters: section\n" + + "3. Parameters have same schema\n"; + + @Test + @DisplayName( + "Same Path, different PathParams, Params in the `Parameters`: match pathParam, Different Schema") + public void pathSamePathParamsDiffParamSameAsInPathButSchemaDiff() { + final Logger logger = LoggerFactory.getLogger(PathParameterSchemaDiffTest.class); + logger.info(TEST_MSG_1); + String OPENAPI_DOC1 = "path_parameter_diff_param_schema_diff_old.yaml"; + String OPENAPI_DOC2 = "path_parameter_diff_param_schema_diff_new.yaml"; + ChangedOpenApi diff = OpenApiCompare.fromLocations(OPENAPI_DOC1, OPENAPI_DOC2); + Assertions.assertTrue(diff.isDifferent()); + Assertions.assertFalse(diff.isCompatible()); + } + + @Test + @DisplayName( + "Same Path, different PathParams, Params in the `Parameters`: match pathParam, same Schema") + public void pathSamePathParamsDiffParamNameDiffSchemaSame() { + final Logger logger = LoggerFactory.getLogger(PathParameterSchemaDiffTest.class); + logger.info(TEST_MSG_2); + String OPENAPI_DOC1 = "path_parameter_diff_param_name_diff_old.yaml"; + String OPENAPI_DOC2 = "path_parameter_diff_param_name_diff_new.yaml"; + ChangedOpenApi diff = OpenApiCompare.fromLocations(OPENAPI_DOC1, OPENAPI_DOC2); + Assertions.assertFalse(diff.isDifferent()); + Assertions.assertTrue(diff.isCompatible()); + } +} diff --git a/core/src/test/resources/path_parameter_diff_param_name_diff_new.yaml b/core/src/test/resources/path_parameter_diff_param_name_diff_new.yaml new file mode 100644 index 000000000..8faa0730e --- /dev/null +++ b/core/src/test/resources/path_parameter_diff_param_name_diff_new.yaml @@ -0,0 +1,48 @@ +openapi: 3.0.0 +info: + title: Sample API + description: Optional multiline or single-line description in [CommonMark](http://commonmark.org/help/) or HTML. + version: 0.1.9 +servers: + - url: http://api.example.com/v1 + description: Example api server + +paths: + + /student/{username}: + get: + parameters: + - in: path + name: username # Note the name is the same as in the path + required: true + schema: + type: string + minimum: 1 + + + summary: Returns data about a student + responses: + '200': # status code + description: A JSON Object student data + content: + application/json: + schema: + type: object + items: + $ref: '#/components/schemas/Student' + +components: + schemas: + Student: + type: object + properties: + id: + type: integer + example: "1" + username: + type: string + example: "alloy_horizon" + pattern: A..Z a..z 0..9 . _ - + name: + type: string + example: "Alloy Horizon" diff --git a/core/src/test/resources/path_parameter_diff_param_name_diff_old.yaml b/core/src/test/resources/path_parameter_diff_param_name_diff_old.yaml new file mode 100644 index 000000000..7ced291d1 --- /dev/null +++ b/core/src/test/resources/path_parameter_diff_param_name_diff_old.yaml @@ -0,0 +1,48 @@ +openapi: 3.0.0 +info: + title: Sample API + description: Optional multiline or single-line description in [CommonMark](http://commonmark.org/help/) or HTML. + version: 0.1.9 +servers: + - url: http://api.example.com/v1 + description: Example api server + +paths: + + /student/{id}: + get: + parameters: + - in: path + name: id # Note the name is the same as in the path + required: true + schema: + type: string + minimum: 1 + + + summary: Returns data about a student + responses: + '200': # status code + description: A JSON Object student data + content: + application/json: + schema: + type: object + items: + $ref: '#/components/schemas/Student' + +components: + schemas: + Student: + type: object + properties: + id: + type: integer + example: "1" + username: + type: string + example: "alloy_horizon" + pattern: A..Z a..z 0..9 . _ - + name: + type: string + example: "Alloy Horizon" diff --git a/core/src/test/resources/path_parameter_diff_param_schema_diff_new.yaml b/core/src/test/resources/path_parameter_diff_param_schema_diff_new.yaml new file mode 100644 index 000000000..8faa0730e --- /dev/null +++ b/core/src/test/resources/path_parameter_diff_param_schema_diff_new.yaml @@ -0,0 +1,48 @@ +openapi: 3.0.0 +info: + title: Sample API + description: Optional multiline or single-line description in [CommonMark](http://commonmark.org/help/) or HTML. + version: 0.1.9 +servers: + - url: http://api.example.com/v1 + description: Example api server + +paths: + + /student/{username}: + get: + parameters: + - in: path + name: username # Note the name is the same as in the path + required: true + schema: + type: string + minimum: 1 + + + summary: Returns data about a student + responses: + '200': # status code + description: A JSON Object student data + content: + application/json: + schema: + type: object + items: + $ref: '#/components/schemas/Student' + +components: + schemas: + Student: + type: object + properties: + id: + type: integer + example: "1" + username: + type: string + example: "alloy_horizon" + pattern: A..Z a..z 0..9 . _ - + name: + type: string + example: "Alloy Horizon" diff --git a/core/src/test/resources/path_parameter_diff_param_schema_diff_old.yaml b/core/src/test/resources/path_parameter_diff_param_schema_diff_old.yaml new file mode 100644 index 000000000..96be9b9b2 --- /dev/null +++ b/core/src/test/resources/path_parameter_diff_param_schema_diff_old.yaml @@ -0,0 +1,48 @@ +openapi: 3.0.0 +info: + title: Sample API + description: Optional multiline or single-line description in [CommonMark](http://commonmark.org/help/) or HTML. + version: 0.1.9 +servers: + - url: http://api.example.com/v1 + description: Example api server + +paths: + + /student/{id}: + get: + parameters: + - in: path + name: id # Note the name is the same as in the path + required: true + schema: + type: integer + minimum: 1 + + + summary: Returns data about a student + responses: + '200': # status code + description: A JSON Object student data + content: + application/json: + schema: + type: object + items: + $ref: '#/components/schemas/Student' + +components: + schemas: + Student: + type: object + properties: + id: + type: integer + example: "1" + username: + type: string + example: "alloy_horizon" + pattern: A..Z a..z 0..9 . _ - + name: + type: string + example: "Alloy Horizon"