diff --git a/README.md b/README.md index ffb70efcf0..cba4ba81e9 100644 --- a/README.md +++ b/README.md @@ -198,6 +198,10 @@ final OpenAPI openAPI = new OpenAPIV3Parser().read("a.yaml", null, parseOptions) This applies to schemas, parameters, responses, pretty much everything containing a ref. +For OpenAPI 3.0 documents, the `OpenAPIResolver` and `ExternalRefProcessor` path adds numeric suffixes such as `Name_1` for name collisions. +This behavior prevents data loss. It can change generated component keys, local `$ref` values, and component counts for documents with name collisions. +OpenAPI 3.1 uses a separate dereferencer. This numeric-suffix change does not modify OpenAPI 3.1 resolution. + #### 2. resolveFully: ```java diff --git a/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ComponentNameAllocator.java b/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ComponentNameAllocator.java new file mode 100644 index 0000000000..5a137e5c85 --- /dev/null +++ b/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ComponentNameAllocator.java @@ -0,0 +1,70 @@ +package io.swagger.v3.parser.processors; + +import java.util.Map; +import java.util.Objects; +import java.util.function.BiPredicate; +import java.util.function.Function; + +import io.swagger.v3.parser.ResolverCache; +import org.slf4j.Logger; + +/** + * Allocates unique names for resolved OpenAPI components. + * + *

The caller supplies the policies which are specific to a component type: + * key lookup, reference equivalence, and reuse of an already resolved value.

+ */ +final class ComponentNameAllocator { + + private final Logger logger; + private final ResolverCache cache; + + ComponentNameAllocator(Logger logger, ResolverCache cache) { + this.logger = logger; + this.cache = cache; + } + + String allocate(Map components, String baseName, + T incoming, String incomingRef, Function refOf) { + return allocate(components, baseName, incoming, incomingRef, refOf, + name -> components.containsKey(name) ? name : null, + (existing, ignored) -> false); + } + + String allocate(Map components, String baseName, + T incoming, String incomingRef, Function refOf, + Function keyOf, BiPredicate reusePolicy) { + for (int suffix = 0; ; suffix++) { + String candidate = suffix == 0 ? baseName : baseName + "_" + suffix; + String existingKey = keyOf.apply(candidate); + if (existingKey == null) { + return candidate; + } + if (canReuse(components.get(existingKey), incoming, incomingRef, refOf, reusePolicy)) { + return existingKey; + } + logger.debug("A different component already claims the name {}", existingKey); + } + } + + private boolean canReuse(T existing, T incoming, String incomingRef, + Function refOf, BiPredicate reusePolicy) { + String existingRef = existing == null ? null : refOf.apply(existing); + if (existingRef != null) { + return cache.refsAreEquivalent(existingRef, incomingRef); + } + return Objects.equals(incoming, existing) || reusePolicy.test(existing, incoming); + } + + static Function caseInsensitiveKey(Map components) { + return candidate -> { + if (components.containsKey(candidate)) { + return candidate; + } + return components.keySet().stream() + .filter(name -> name.equalsIgnoreCase(candidate)) + .findFirst() + .orElse(null); + }; + } +} diff --git a/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java b/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java index 40b7f543dd..7d549985a1 100644 --- a/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java +++ b/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java @@ -48,43 +48,24 @@ public final class ExternalRefProcessor { private final ResolverCache cache; private final OpenAPI openAPI; + private final ComponentNameAllocator nameAllocator; public ExternalRefProcessor(ResolverCache cache, OpenAPI openAPI) { this.cache = cache; this.openAPI = openAPI; + this.nameAllocator = new ComponentNameAllocator(LOGGER, cache); } - private String finalNameRec(Map schemas, String possiblyConflictingDefinitionName, Schema newSchema, - int iteration, String incomingRef) { - String tryName = - iteration == 0 ? possiblyConflictingDefinitionName : possiblyConflictingDefinitionName + "_" + iteration; - Schema existingModel = schemas.get(tryName); - if (existingModel == null) { - for (String name : schemas.keySet()) { - if (name.equalsIgnoreCase(tryName)) { - existingModel = schemas.get(name); - tryName = name; - break; - } - } - } - if (existingModel != null) { - if (existingModel.get$ref() != null) { - if (incomingRef != null && !cache.refsAreEquivalent(existingModel.get$ref(), incomingRef)) { - LOGGER.debug("A different external $ref already claims the name " + tryName); - return finalNameRec(schemas, possiblyConflictingDefinitionName, newSchema, ++iteration, incomingRef); - } - // use the new model - existingModel = null; - } else if (!newSchema.equals(existingModel)) { - if (cache.getRenamedRef(newSchema.get$ref()) != null) { - return tryName; - } - LOGGER.debug("A model for " + existingModel + " already exists"); - return finalNameRec(schemas, possiblyConflictingDefinitionName, newSchema, ++iteration, incomingRef); - } - } - return tryName; + private void warnUnableToLoadReference(String ref) { + LOGGER.warn("unable to load model reference from `{}`. It may not be available or the reference isn't a valid model schema", ref); + } + + private String allocateSchemaName(Map schemas, String baseName, + Schema incoming, String incomingRef) { + return nameAllocator.allocate(schemas, baseName, incoming, incomingRef, + Schema::get$ref, + ComponentNameAllocator.caseInsensitiveKey(schemas), + (existing, inc) -> cache.getRenamedRef(inc.get$ref()) != null); } public String processRefToExternalSchema(String $ref, RefFormat refFormat) { @@ -97,7 +78,7 @@ public String processRefToExternalSchema(String $ref, RefFormat refFormat) { if(schema == null) { // stop! There's a problem. retain the original ref - LOGGER.warn("unable to load model reference from `{}`. It may not be available or the reference isn't a valid model schema", $ref); + warnUnableToLoadReference($ref); return $ref; } String newRef; @@ -112,7 +93,7 @@ public String processRefToExternalSchema(String $ref, RefFormat refFormat) { } final String possiblyConflictingDefinitionName = computeDefinitionName($ref); - newRef = finalNameRec(schemas, possiblyConflictingDefinitionName, schema, 0, $ref); + newRef = allocateSchemaName(schemas, possiblyConflictingDefinitionName, schema, $ref); cache.putRenamedRef($ref, newRef); Schema existingModel = schemas.get(newRef); if(existingModel != null && existingModel.get$ref() != null) { @@ -416,6 +397,12 @@ public String processRefToExternalResponse(String $ref, RefFormat refFormat) { } final ApiResponse response = cache.loadRef($ref, refFormat, ApiResponse.class); + if(response == null) { + // stop! There's a problem. retain the original ref + warnUnableToLoadReference($ref); + return $ref; + } + String newRef; if (openAPI.getComponents() == null) { @@ -427,21 +414,11 @@ public String processRefToExternalResponse(String $ref, RefFormat refFormat) { responses = new LinkedHashMap<>(); } - final String possiblyConflictingDefinitionName = computeDefinitionName($ref); - - ApiResponse existingResponse = responses.get(possiblyConflictingDefinitionName); - - if (existingResponse != null) { - LOGGER.debug("A model for " + existingResponse + " already exists"); - if(existingResponse.get$ref() != null) { - // use the new model - existingResponse = null; - } - } - newRef = possiblyConflictingDefinitionName; + newRef = nameAllocator.allocate(responses, computeDefinitionName($ref), response, $ref, ApiResponse::get$ref); cache.putRenamedRef($ref, newRef); + ApiResponse existingResponse = responses.get(newRef); - if(existingResponse == null) { + if(existingResponse == null || existingResponse.get$ref() != null) { // don't overwrite existing model reference openAPI.getComponents().addResponses(newRef, response); cache.addReferencedKey(newRef); @@ -498,8 +475,7 @@ public String processRefToExternalRequestBody(String $ref, RefFormat refFormat) if(body == null) { // stop! There's a problem. retain the original ref - LOGGER.warn("unable to load model reference from `" + $ref + "`. It may not be available " + - "or the reference isn't a valid model schema"); + warnUnableToLoadReference($ref); return $ref; } String newRef; @@ -513,21 +489,11 @@ public String processRefToExternalRequestBody(String $ref, RefFormat refFormat) bodies = new LinkedHashMap<>(); } - final String possiblyConflictingDefinitionName = computeDefinitionName($ref); - - RequestBody existingBody= bodies.get(possiblyConflictingDefinitionName); - - if (existingBody != null) { - LOGGER.debug("A model for " + existingBody + " already exists"); - if(existingBody.get$ref() != null) { - // use the new model - existingBody = null; - } - } - newRef = possiblyConflictingDefinitionName; + newRef = nameAllocator.allocate(bodies, computeDefinitionName($ref), body, $ref, RequestBody::get$ref); cache.putRenamedRef($ref, newRef); + RequestBody existingBody = bodies.get(newRef); - if(existingBody == null) { + if(existingBody == null || existingBody.get$ref() != null) { // don't overwrite existing model reference openAPI.getComponents().addRequestBodies(newRef, body); cache.addReferencedKey(newRef); @@ -558,8 +524,7 @@ public String processRefToExternalHeader(String $ref, RefFormat refFormat) { if(header == null) { // stop! There's a problem. retain the original ref - LOGGER.warn("unable to load model reference from `" + $ref + "`. It may not be available " + - "or the reference isn't a valid model schema"); + warnUnableToLoadReference($ref); return $ref; } String newRef; @@ -573,21 +538,11 @@ public String processRefToExternalHeader(String $ref, RefFormat refFormat) { headers = new LinkedHashMap<>(); } - final String possiblyConflictingDefinitionName = computeDefinitionName($ref); - - Header existingHeader = headers.get(possiblyConflictingDefinitionName); - - if (existingHeader != null) { - LOGGER.debug("A model for " + existingHeader + " already exists"); - if(existingHeader.get$ref() != null) { - // use the new model - existingHeader = null; - } - } - newRef = possiblyConflictingDefinitionName; + newRef = nameAllocator.allocate(headers, computeDefinitionName($ref), header, $ref, Header::get$ref); cache.putRenamedRef($ref, newRef); + Header existingHeader = headers.get(newRef); - if(existingHeader == null) { + if(existingHeader == null || existingHeader.get$ref() != null) { // don't overwrite existing model reference openAPI.getComponents().addHeaders(newRef, header); cache.addReferencedKey(newRef); @@ -625,8 +580,7 @@ public String processRefToExternalSecurityScheme(String $ref, RefFormat refForma if(securityScheme == null) { // stop! There's a problem. retain the original ref - LOGGER.warn("unable to load model reference from `" + $ref + "`. It may not be available " + - "or the reference isn't a valid model schema"); + warnUnableToLoadReference($ref); return $ref; } String newRef; @@ -640,21 +594,12 @@ public String processRefToExternalSecurityScheme(String $ref, RefFormat refForma securitySchemeMap = new LinkedHashMap<>(); } - final String possiblyConflictingDefinitionName = computeDefinitionName($ref); - - SecurityScheme existingSecurityScheme = securitySchemeMap.get(possiblyConflictingDefinitionName); - - if (existingSecurityScheme != null) { - LOGGER.debug("A model for " + existingSecurityScheme + " already exists"); - if(existingSecurityScheme.get$ref() != null) { - // use the new model - existingSecurityScheme = null; - } - } - newRef = possiblyConflictingDefinitionName; + newRef = nameAllocator.allocate(securitySchemeMap, computeDefinitionName($ref), securityScheme, $ref, + SecurityScheme::get$ref); cache.putRenamedRef($ref, newRef); + SecurityScheme existingSecurityScheme = securitySchemeMap.get(newRef); - if(existingSecurityScheme == null) { + if(existingSecurityScheme == null || existingSecurityScheme.get$ref() != null) { // don't overwrite existing model reference openAPI.getComponents().addSecuritySchemes(newRef, securityScheme); cache.addReferencedKey(newRef); @@ -683,8 +628,7 @@ public String processRefToExternalLink(String $ref, RefFormat refFormat) { if(link == null) { // stop! There's a problem. retain the original ref - LOGGER.warn("unable to load model reference from `" + $ref + "`. It may not be available " + - "or the reference isn't a valid model schema"); + warnUnableToLoadReference($ref); return $ref; } String newRef; @@ -698,21 +642,11 @@ public String processRefToExternalLink(String $ref, RefFormat refFormat) { links = new LinkedHashMap<>(); } - final String possiblyConflictingDefinitionName = computeDefinitionName($ref); - - Link existingLink = links.get(possiblyConflictingDefinitionName); - - if (existingLink != null) { - LOGGER.debug("A model for " + existingLink + " already exists"); - if(existingLink.get$ref() != null) { - // use the new model - existingLink = null; - } - } - newRef = possiblyConflictingDefinitionName; + newRef = nameAllocator.allocate(links, computeDefinitionName($ref), link, $ref, Link::get$ref); cache.putRenamedRef($ref, newRef); + Link existingLink = links.get(newRef); - if(existingLink == null) { + if(existingLink == null || existingLink.get$ref() != null) { // don't overwrite existing model reference openAPI.getComponents().addLinks(newRef, link); cache.addReferencedKey(newRef); @@ -741,8 +675,7 @@ public String processRefToExternalExample(String $ref, RefFormat refFormat) { if(example == null) { // stop! There's a problem. retain the original ref - LOGGER.warn("unable to load model reference from `" + $ref + "`. It may not be available " + - "or the reference isn't a valid model schema"); + warnUnableToLoadReference($ref); return $ref; } String newRef; @@ -756,21 +689,11 @@ public String processRefToExternalExample(String $ref, RefFormat refFormat) { examples = new LinkedHashMap<>(); } - final String possiblyConflictingDefinitionName = computeDefinitionName($ref); - - Example existingExample = examples.get(possiblyConflictingDefinitionName); - - if (existingExample != null) { - LOGGER.debug("A model for " + existingExample + " already exists"); - if(existingExample.get$ref() != null) { - // use the new model - existingExample = null; - } - } - newRef = possiblyConflictingDefinitionName; + newRef = nameAllocator.allocate(examples, computeDefinitionName($ref), example, $ref, Example::get$ref); cache.putRenamedRef($ref, newRef); + Example existingExample = examples.get(newRef); - if(existingExample == null) { + if(existingExample == null || existingExample.get$ref() != null) { // don't overwrite existing model reference openAPI.getComponents().addExamples(newRef, example); cache.addReferencedKey(newRef); @@ -798,8 +721,7 @@ public String processRefToExternalParameter(String $ref, RefFormat refFormat) { if(parameter == null) { // stop! There's a problem. retain the original ref - LOGGER.warn("unable to load model reference from `" + $ref + "`. It may not be available " + - "or the reference isn't a valid model schema"); + warnUnableToLoadReference($ref); return $ref; } String newRef; @@ -813,21 +735,11 @@ public String processRefToExternalParameter(String $ref, RefFormat refFormat) { parameters = new LinkedHashMap<>(); } - final String possiblyConflictingDefinitionName = computeDefinitionName($ref); - - Parameter existingParameters = parameters.get(possiblyConflictingDefinitionName); - - if (existingParameters != null) { - LOGGER.debug("A model for " + existingParameters + " already exists"); - if(existingParameters.get$ref() != null) { - // use the new model - existingParameters = null; - } - } - newRef = possiblyConflictingDefinitionName; + newRef = nameAllocator.allocate(parameters, computeDefinitionName($ref), parameter, $ref, Parameter::get$ref); cache.putRenamedRef($ref, newRef); + Parameter existingParameters = parameters.get(newRef); - if(existingParameters == null) { + if(existingParameters == null || existingParameters.get$ref() != null) { // don't overwrite existing model reference openAPI.getComponents().addParameters(newRef, parameter); cache.addReferencedKey(newRef); @@ -883,8 +795,7 @@ public String processRefToExternalCallback(String $ref, RefFormat refFormat) { if(callback == null) { // stop! There's a problem. retain the original ref - LOGGER.warn("unable to load model reference from `" + $ref + "`. It may not be available " + - "or the reference isn't a valid model schema"); + warnUnableToLoadReference($ref); return $ref; } String newRef; @@ -898,21 +809,11 @@ public String processRefToExternalCallback(String $ref, RefFormat refFormat) { callbacks = new LinkedHashMap<>(); } - final String possiblyConflictingDefinitionName = computeDefinitionName($ref); - - Callback existingCallback = callbacks.get(possiblyConflictingDefinitionName); - - if (existingCallback != null) { - LOGGER.debug("A model for " + existingCallback + " already exists"); - if(existingCallback.get$ref() != null) { - // use the new model - existingCallback = null; - } - } - newRef = possiblyConflictingDefinitionName; + newRef = nameAllocator.allocate(callbacks, computeDefinitionName($ref), callback, $ref, Callback::get$ref); cache.putRenamedRef($ref, newRef); + Callback existingCallback = callbacks.get(newRef); - if(existingCallback == null) { + if(existingCallback == null || existingCallback.get$ref() != null) { // don't overwrite existing model reference openAPI.getComponents().addCallbacks(newRef, callback); cache.addReferencedKey(newRef); diff --git a/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/processors/ExternalRefProcessorTest.java b/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/processors/ExternalRefProcessorTest.java index 4c490cb25b..b6690104e0 100644 --- a/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/processors/ExternalRefProcessorTest.java +++ b/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/processors/ExternalRefProcessorTest.java @@ -5,6 +5,8 @@ import io.swagger.v3.oas.models.OpenAPI; import io.swagger.v3.oas.models.media.Schema; import io.swagger.v3.oas.models.media.StringSchema; +import io.swagger.v3.oas.models.responses.ApiResponse; +import io.swagger.v3.oas.models.security.SecurityScheme; import io.swagger.v3.parser.OpenAPIV3Parser; import io.swagger.v3.parser.ResolverCache; import io.swagger.v3.parser.core.models.AuthorizationValue; @@ -27,6 +29,7 @@ import static org.junit.Assert.assertTrue; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertSame; public class ExternalRefProcessorTest { @@ -65,6 +68,182 @@ public void testProcessRefToExternalDefinition_NoNameConflict() { assertEquals(newRef, "bar"); } + @Test + public void testSecuritySchemesWithSameExternalNameReceiveDistinctKeys() { + final String firstRef = "https://example.test/first.yaml#/sharedAuth"; + final String secondRef = "https://example.test/second.yaml#/sharedAuth"; + SecurityScheme first = new SecurityScheme().type(SecurityScheme.Type.APIKEY).name("X-API-Key") + .in(SecurityScheme.In.HEADER); + SecurityScheme second = new SecurityScheme().type(SecurityScheme.Type.HTTP).scheme("bearer"); + OpenAPI testedOpenAPI = new OpenAPI().components(new Components()); + + new Expectations() {{ + cache.loadRef(firstRef, RefFormat.URL, SecurityScheme.class); + result = first; + cache.loadRef(secondRef, RefFormat.URL, SecurityScheme.class); + result = second; + }}; + + ExternalRefProcessor processor = new ExternalRefProcessor(cache, testedOpenAPI); + assertEquals(processor.processRefToExternalSecurityScheme(firstRef, RefFormat.URL), "sharedAuth"); + assertEquals(processor.processRefToExternalSecurityScheme(secondRef, RefFormat.URL), "sharedAuth_1"); + assertEquals(testedOpenAPI.getComponents().getSecuritySchemes().get("sharedAuth"), first); + assertEquals(testedOpenAPI.getComponents().getSecuritySchemes().get("sharedAuth_1"), second); + } + + @Test + public void testResponseAllocatorExaminesOccupiedSuffixesWithoutOverwritingPlaceholder() { + final String ref = "https://example.test/incoming.yaml#/sharedResponse"; + ApiResponse placeholder = new ApiResponse().$ref("https://example.test/other.yaml#/sharedResponse"); + ApiResponse firstSuffix = new ApiResponse().description("first suffix"); + ApiResponse secondSuffix = new ApiResponse().description("second suffix"); + ApiResponse incoming = new ApiResponse().description("incoming"); + Components components = new Components() + .addResponses("sharedResponse", placeholder) + .addResponses("sharedResponse_1", firstSuffix) + .addResponses("sharedResponse_2", secondSuffix); + OpenAPI testedOpenAPI = new OpenAPI().components(components); + + new Expectations() {{ + cache.loadRef(ref, RefFormat.URL, ApiResponse.class); + result = incoming; + }}; + + String assignedName = new ExternalRefProcessor(cache, testedOpenAPI) + .processRefToExternalResponse(ref, RefFormat.URL); + + assertEquals(assignedName, "sharedResponse_3"); + assertSame(testedOpenAPI.getComponents().getResponses().get("sharedResponse"), placeholder); + assertEquals(placeholder.get$ref(), "https://example.test/other.yaml#/sharedResponse"); + assertSame(testedOpenAPI.getComponents().getResponses().get("sharedResponse_3"), incoming); + } + + @Test + public void testEqualResolvedResponsesReuseOneKey() { + final String ref = "https://example.test/incoming.yaml#/sharedResponse"; + ApiResponse existing = new ApiResponse().description("same response"); + ApiResponse incoming = new ApiResponse().description("same response"); + OpenAPI testedOpenAPI = new OpenAPI().components( + new Components().addResponses("sharedResponse", existing)); + + new Expectations() {{ + cache.loadRef(ref, RefFormat.URL, ApiResponse.class); + result = incoming; + }}; + + String assignedName = new ExternalRefProcessor(cache, testedOpenAPI) + .processRefToExternalResponse(ref, RefFormat.URL); + + assertEquals(assignedName, "sharedResponse"); + assertEquals(testedOpenAPI.getComponents().getResponses().size(), 1); + assertSame(testedOpenAPI.getComponents().getResponses().get("sharedResponse"), existing); + } + + @Test + public void testEquivalentResponsePlaceholderUsesCanonicalReferenceIdentity() { + final String ref = "https://example.test/responses/common.yaml#/sharedResponse"; + final String equivalentRef = "https://example.test/responses/../responses/common.yaml#/sharedResponse"; + ApiResponse placeholder = new ApiResponse().$ref(equivalentRef); + ApiResponse incoming = new ApiResponse().description("resolved response"); + OpenAPI testedOpenAPI = new OpenAPI().components( + new Components().addResponses("sharedResponse", placeholder)); + + new Expectations() {{ + cache.loadRef(ref, RefFormat.URL, ApiResponse.class); + result = incoming; + cache.refsAreEquivalent(equivalentRef, ref); + result = true; + }}; + + String assignedName = new ExternalRefProcessor(cache, testedOpenAPI) + .processRefToExternalResponse(ref, RefFormat.URL); + + assertEquals(assignedName, "sharedResponse"); + assertEquals(testedOpenAPI.getComponents().getResponses().size(), 1); + assertSame(testedOpenAPI.getComponents().getResponses().get("sharedResponse"), incoming); + } + + @Test + public void testNonSchemaComponentNamesRemainCaseSensitive() { + final String ref = "https://example.test/responses.yaml#/SharedResponse"; + ApiResponse lowerCase = new ApiResponse().description("lower case key"); + ApiResponse incoming = new ApiResponse().description("upper case key"); + OpenAPI testedOpenAPI = new OpenAPI().components( + new Components().addResponses("sharedResponse", lowerCase)); + + new Expectations() {{ + cache.loadRef(ref, RefFormat.URL, ApiResponse.class); + result = incoming; + }}; + + String assignedName = new ExternalRefProcessor(cache, testedOpenAPI) + .processRefToExternalResponse(ref, RefFormat.URL); + + assertEquals(assignedName, "SharedResponse"); + assertEquals(testedOpenAPI.getComponents().getResponses().size(), 2); + assertSame(testedOpenAPI.getComponents().getResponses().get("sharedResponse"), lowerCase); + assertSame(testedOpenAPI.getComponents().getResponses().get("SharedResponse"), incoming); + } + + @Test + public void testFailedResponseLoadReturnsOriginalRef() { + final String ref = "https://example.test/missing.yaml#/MissingResponse"; + OpenAPI testedOpenAPI = new OpenAPI(); + + new Expectations() {{ + cache.loadRef(ref, RefFormat.URL, ApiResponse.class); + result = null; + }}; + + assertEquals(new ExternalRefProcessor(cache, testedOpenAPI) + .processRefToExternalResponse(ref, RefFormat.URL), ref); + } + + @Test + public void testRecursiveExternalSchemaReusesRenameCacheEntry() { + final String ref = "schemas.yaml#/Recursive"; + Schema recursive = new Schema().$ref(ref); + OpenAPI testedOpenAPI = new OpenAPI().components(new Components()); + + new Expectations() {{ + cache.getRenamedRef(ref); + returns(null, "Recursive"); + cache.loadRef(ref, RefFormat.RELATIVE, Schema.class); + result = recursive; + }}; + + String assignedName = new ExternalRefProcessor(cache, testedOpenAPI) + .processRefToExternalSchema(ref, RefFormat.RELATIVE); + + assertEquals(assignedName, "Recursive"); + assertEquals(testedOpenAPI.getComponents().getSchemas().size(), 1); + assertSame(testedOpenAPI.getComponents().getSchemas().get("Recursive"), recursive); + assertEquals(recursive.get$ref(), "#/components/schemas/Recursive"); + } + + @Test + public void testSchemaLookupPrefersExactKeyBeforeCaseInsensitiveFallback() { + final String ref = "https://example.test/schemas.yaml#/Pet"; + Schema lowerCaseSchema = new Schema().addProperties("lower", new StringSchema()); + Schema exactSchema = new Schema().addProperties("exact", new StringSchema()); + Schema incoming = new Schema().addProperties("exact", new StringSchema()); + OpenAPI testedOpenAPI = new OpenAPI().components(new Components() + .addSchemas("pet", lowerCaseSchema) + .addSchemas("Pet", exactSchema)); + + new Expectations() {{ + cache.loadRef(ref, RefFormat.URL, Schema.class); + result = incoming; + }}; + + String assignedName = new ExternalRefProcessor(cache, testedOpenAPI) + .processRefToExternalSchema(ref, RefFormat.URL); + + assertEquals(assignedName, "Pet"); + assertEquals(testedOpenAPI.getComponents().getSchemas().size(), 2); + assertSame(testedOpenAPI.getComponents().getSchemas().get("Pet"), exactSchema); + assertSame(testedOpenAPI.getComponents().getSchemas().get("pet"), lowerCaseSchema); + } @Test public void testNestedExternalRefs() { diff --git a/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/test/Issue2055Test.java b/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/test/Issue2055Test.java new file mode 100644 index 0000000000..5277b08da9 --- /dev/null +++ b/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/test/Issue2055Test.java @@ -0,0 +1,228 @@ +package io.swagger.v3.parser.test; + +import io.swagger.v3.oas.models.OpenAPI; +import io.swagger.v3.oas.models.Operation; +import io.swagger.v3.oas.models.callbacks.Callback; +import io.swagger.v3.oas.models.examples.Example; +import io.swagger.v3.oas.models.headers.Header; +import io.swagger.v3.oas.models.links.Link; +import io.swagger.v3.oas.models.media.Content; +import io.swagger.v3.oas.models.parameters.Parameter; +import io.swagger.v3.oas.models.parameters.RequestBody; +import io.swagger.v3.oas.models.responses.ApiResponse; +import io.swagger.v3.oas.models.security.SecurityScheme; +import io.swagger.v3.parser.OpenAPIV3Parser; +import io.swagger.v3.parser.core.models.ParseOptions; +import io.swagger.v3.parser.core.models.SwaggerParseResult; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +import java.util.Arrays; +import java.util.Map; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotEquals; +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertSame; +import static org.testng.Assert.assertTrue; + +public class Issue2055Test { + + @Test + public void externalParametersWithSameNameKeepBothDefinitions() { + OpenAPI openAPI = parse(); + Operation post = openAPI.getPaths().get("/a-path").getPost(); + Operation put = openAPI.getPaths().get("/a-path").getPut(); + + assertEquals(post.getParameters().get(0).getDescription(), "There can be only 1"); + assertEquals(put.getParameters().get(0).getDescription(), "There can be only 2"); + + Map parameters = openAPI.getComponents().getParameters(); + assertEquals(parameters.size(), 10); + assertTrue(parameters.keySet().containsAll(Arrays.asList("limit", "limit_1"))); + assertTrue(parameters.values().stream() + .anyMatch(parameter -> "There can be only 1".equals(parameter.getDescription()))); + assertTrue(parameters.values().stream() + .anyMatch(parameter -> "There can be only 2".equals(parameter.getDescription()))); + } + + @DataProvider(name = "parameterLocations") + public Object[][] parameterLocations() { + return new Object[][]{ + {"path", "pathParam", 0}, + {"query", "queryParam", 1}, + {"header", "headerParam", 2}, + {"cookie", "cookieParam", 3} + }; + } + + @Test(dataProvider = "parameterLocations") + public void externalParametersWithSameNameKeepBothDefinitionsForEveryLocation( + String location, String componentBaseName, int parameterIndex) { + OpenAPI openAPI = parse(); + Operation post = openAPI.getPaths().get("/parameter-types/{pathParam}").getPost(); + Operation put = openAPI.getPaths().get("/parameter-types/{pathParam}").getPut(); + Parameter first = post.getParameters().get(parameterIndex); + Parameter second = put.getParameters().get(parameterIndex); + + assertEquals(first.getIn(), location); + assertEquals(second.getIn(), location); + assertEquals(first.getDescription(), location + " parameter from file one"); + assertEquals(second.getDescription(), location + " parameter from file two"); + + Map parameters = openAPI.getComponents().getParameters(); + assertTrue(parameters.keySet().containsAll(Arrays.asList(componentBaseName, componentBaseName + "_1"))); + assertTrue(parameters.values().stream() + .anyMatch(parameter -> (location + " parameter from file one").equals(parameter.getDescription()))); + assertTrue(parameters.values().stream() + .anyMatch(parameter -> (location + " parameter from file two").equals(parameter.getDescription()))); + } + + @DataProvider(name = "externalContentComponentTypes") + public Object[][] externalContentComponentTypes() { + return new Object[][]{ + {"requestBodies", "a-request", 2}, + {"responses", "a-response", 3} + }; + } + + @Test(dataProvider = "externalContentComponentTypes") + public void externalContentComponentsWithSameNameKeepBothDefinitions( + String componentType, String componentBaseName, int expectedComponentCount) { + OpenAPI openAPI = parse(); + Operation post = openAPI.getPaths().get("/a-path").getPost(); + Operation put = openAPI.getPaths().get("/a-path").getPut(); + String postRef = contentComponentRef(post, componentType); + String putRef = contentComponentRef(put, componentType); + + assertNotEquals(postRef, putRef); + + Map components = contentComponents(openAPI, componentType); + assertEquals(components.size(), expectedComponentCount); + assertTrue(components.keySet().containsAll(Arrays.asList(componentBaseName, componentBaseName + "_1"))); + assertTrue(content(assertLocalRefResolves(postRef, componentType, components)).containsKey("text/plain")); + assertTrue(content(assertLocalRefResolves(putRef, componentType, components)).containsKey("application/json")); + } + + @Test + public void allCollisionProneComponentTypesKeepDistinctDefinitions() { + OpenAPI openAPI = parse("src/test/resources/issue-2055/all-components.yaml"); + + Map headers = openAPI.getComponents().getHeaders(); + assertTrue(headers.keySet().containsAll(Arrays.asList( + "sharedHeader", "sharedHeader_1", "sharedHeader_2", "sharedHeader_3", "sharedHeader_4"))); + assertEquals(headers.size(), 5, "Repeated references after suffix allocation must reuse their assigned key"); + assertTrue(headers.values().stream().anyMatch(header -> "header from file one".equals(header.getDescription()))); + assertTrue(headers.values().stream().anyMatch(header -> "header from file two".equals(header.getDescription()))); + + Map links = openAPI.getComponents().getLinks(); + assertTrue(links.keySet().containsAll(Arrays.asList("sharedLink", "sharedLink_1"))); + assertNotEquals(links.get("sharedLink").getOperationId(), links.get("sharedLink_1").getOperationId()); + + Map examples = openAPI.getComponents().getExamples(); + assertTrue(examples.keySet().containsAll(Arrays.asList( + "sharedExample", "sharedExample_1", "equalExample"))); + assertFalse(examples.containsKey("equalExample_1")); + + Map callbacks = openAPI.getComponents().getCallbacks(); + assertTrue(callbacks.keySet().containsAll(Arrays.asList("sharedCallback", "sharedCallback_1"))); + assertEquals(callbacks.size(), 2, "Equivalent URI spellings must reuse the canonical callback key"); + } + + @Test + public void securitySchemesRetainTheirLocalKeys() { + OpenAPI openAPI = parse("src/test/resources/issue-2055/all-components.yaml"); + Map schemes = openAPI.getComponents().getSecuritySchemes(); + + assertEquals(schemes.size(), 2); + assertEquals(schemes.get("firstAuth").getType(), SecurityScheme.Type.APIKEY); + assertEquals(schemes.get("secondAuth").getType(), SecurityScheme.Type.HTTP); + } + + @Test + public void everyRewrittenLocalReferenceResolvesToItsExpectedComponent() { + OpenAPI openAPI = parse("src/test/resources/issue-2055/all-components.yaml"); + Operation first = openAPI.getPaths().get("/first").getPost(); + Operation second = openAPI.getPaths().get("/second").getPost(); + Operation equivalent = openAPI.getPaths().get("/equivalent").getPost(); + + RequestBody firstRequest = assertLocalRefResolves(first.getRequestBody().get$ref(), "requestBodies", + openAPI.getComponents().getRequestBodies()); + RequestBody secondRequest = assertLocalRefResolves(second.getRequestBody().get$ref(), "requestBodies", + openAPI.getComponents().getRequestBodies()); + assertEquals(firstRequest.getDescription(), "request from file one"); + assertEquals(secondRequest.getDescription(), "request from file two"); + + ApiResponse firstResponse = assertLocalRefResolves(first.getResponses().get("200").get$ref(), "responses", + openAPI.getComponents().getResponses()); + ApiResponse secondResponse = assertLocalRefResolves(second.getResponses().get("200").get$ref(), "responses", + openAPI.getComponents().getResponses()); + assertEquals(firstResponse.getDescription(), "response from file one"); + assertEquals(secondResponse.getDescription(), "response from file two"); + + Callback firstCallback = assertLocalRefResolves(first.getCallbacks().get("event").get$ref(), "callbacks", + openAPI.getComponents().getCallbacks()); + Callback secondCallback = assertLocalRefResolves(second.getCallbacks().get("event").get$ref(), "callbacks", + openAPI.getComponents().getCallbacks()); + Callback repeatedFirstCallback = assertLocalRefResolves(equivalent.getCallbacks().get("event").get$ref(), + "callbacks", openAPI.getComponents().getCallbacks()); + assertTrue(firstCallback.containsKey("{$request.body#/callbackUrl}")); + assertTrue(secondCallback.containsKey("{$request.body#/callbackUrl}")); + assertEquals(firstCallback.get("{$request.body#/callbackUrl}").getPost() + .getResponses().get("204").getDescription(), "callback from file one"); + assertEquals(secondCallback.get("{$request.body#/callbackUrl}").getPost() + .getResponses().get("204").getDescription(), "callback from file two"); + assertSame(repeatedFirstCallback, firstCallback); + } + + private OpenAPI parse() { + return parse("src/test/resources/issue-2055/openapi.json"); + } + + private OpenAPI parse(String location) { + ParseOptions options = new ParseOptions(); + options.setResolve(true); + + SwaggerParseResult result = new OpenAPIV3Parser() + .readLocation(location, null, options); + + assertNotNull(result.getOpenAPI()); + assertTrue(result.getMessages().isEmpty(), "Unexpected parser messages: " + result.getMessages()); + assertNotNull(result.getOpenAPI().getComponents()); + return result.getOpenAPI(); + } + + private String componentName(String ref) { + return ref.substring(ref.lastIndexOf('/') + 1); + } + + private String contentComponentRef(Operation operation, String componentType) { + if ("requestBodies".equals(componentType)) { + return operation.getRequestBody().get$ref(); + } + return operation.getResponses().get("200").get$ref(); + } + + private Map contentComponents(OpenAPI openAPI, String componentType) { + if ("requestBodies".equals(componentType)) { + return openAPI.getComponents().getRequestBodies(); + } + return openAPI.getComponents().getResponses(); + } + + private Content content(Object component) { + if (component instanceof RequestBody) { + return ((RequestBody) component).getContent(); + } + return ((ApiResponse) component).getContent(); + } + + private T assertLocalRefResolves(String ref, String componentType, Map components) { + assertNotNull(ref); + assertTrue(ref.startsWith("#/components/" + componentType + "/"), "Expected a local component ref: " + ref); + T component = components.get(componentName(ref)); + assertNotNull(component, "Reference does not resolve to a component: " + ref); + return component; + } +} diff --git a/modules/swagger-parser-v3/src/test/resources/issue-2055/all-components.yaml b/modules/swagger-parser-v3/src/test/resources/issue-2055/all-components.yaml new file mode 100644 index 0000000000..8e59af6483 --- /dev/null +++ b/modules/swagger-parser-v3/src/test/resources/issue-2055/all-components.yaml @@ -0,0 +1,58 @@ +openapi: 3.0.3 +info: + title: External component collisions + version: 1.0.0 +paths: + /first: + post: + operationId: firstOperation + requestBody: + $ref: './external-components-1.yaml#/sharedRequest' + callbacks: + event: + $ref: './external-components-1.yaml#/sharedCallback' + responses: + '200': + $ref: './external-components-1.yaml#/sharedResponse' + /second: + post: + operationId: secondOperation + requestBody: + $ref: 'external-components-2.yaml#/sharedRequest' + callbacks: + event: + $ref: 'external-components-2.yaml#/sharedCallback' + responses: + '200': + $ref: 'external-components-2.yaml#/sharedResponse' + /equivalent: + post: + operationId: equivalentReference + callbacks: + event: + $ref: 'external-components-1.yaml#/sharedCallback' + responses: + default: + description: default + headers: + X-Shared: + $ref: 'external-components-1.yaml#/sharedHeader' +components: + headers: + sharedHeader: + description: occupied base name + schema: + type: boolean + sharedHeader_1: + description: occupied first suffix + schema: + type: number + sharedHeader_2: + description: occupied second suffix + schema: + type: string + securitySchemes: + firstAuth: + $ref: './external-components-1.yaml#/sharedAuth' + secondAuth: + $ref: './external-components-2.yaml#/sharedAuth' diff --git a/modules/swagger-parser-v3/src/test/resources/issue-2055/external-components-1.yaml b/modules/swagger-parser-v3/src/test/resources/issue-2055/external-components-1.yaml new file mode 100644 index 0000000000..8b711389d0 --- /dev/null +++ b/modules/swagger-parser-v3/src/test/resources/issue-2055/external-components-1.yaml @@ -0,0 +1,45 @@ +sharedRequest: + description: request from file one + content: + text/plain: + schema: + type: string +sharedResponse: + description: response from file one + headers: + X-Shared: + $ref: '#/sharedHeader' + links: + next: + $ref: '#/sharedLink' + content: + text/plain: + schema: + type: string + examples: + distinct: + $ref: '#/sharedExample' + equal: + $ref: '#/equalExample' +sharedHeader: + description: header from file one + schema: + type: string +sharedLink: + operationId: firstOperation +sharedExample: + summary: example from file one + value: one +equalExample: + summary: equal example + value: equal +sharedCallback: + '{$request.body#/callbackUrl}': + post: + responses: + '204': + description: callback from file one +sharedAuth: + type: apiKey + in: header + name: X-API-Key diff --git a/modules/swagger-parser-v3/src/test/resources/issue-2055/external-components-2.yaml b/modules/swagger-parser-v3/src/test/resources/issue-2055/external-components-2.yaml new file mode 100644 index 0000000000..78aa67f45b --- /dev/null +++ b/modules/swagger-parser-v3/src/test/resources/issue-2055/external-components-2.yaml @@ -0,0 +1,44 @@ +sharedRequest: + description: request from file two + content: + application/json: + schema: + type: object +sharedResponse: + description: response from file two + headers: + X-Shared: + $ref: '#/sharedHeader' + links: + next: + $ref: '#/sharedLink' + content: + application/json: + schema: + type: object + examples: + distinct: + $ref: '#/sharedExample' + equal: + $ref: '#/equalExample' +sharedHeader: + description: header from file two + schema: + type: integer +sharedLink: + operationId: secondOperation +sharedExample: + summary: example from file two + value: two +equalExample: + summary: equal example + value: equal +sharedCallback: + '{$request.body#/callbackUrl}': + post: + responses: + '204': + description: callback from file two +sharedAuth: + type: http + scheme: bearer diff --git a/modules/swagger-parser-v3/src/test/resources/issue-2055/external_ref_1.json b/modules/swagger-parser-v3/src/test/resources/issue-2055/external_ref_1.json new file mode 100644 index 0000000000..22474deb3c --- /dev/null +++ b/modules/swagger-parser-v3/src/test/resources/issue-2055/external_ref_1.json @@ -0,0 +1,54 @@ +{ + "limit": { + "name": "limit", + "description": "There can be only 1", + "in": "query", + "schema": { + "type": "integer", + "maximum": 1, + "minimum": 0 + } + }, + "pathParam": { + "name": "pathParam", + "description": "path parameter from file one", + "in": "path", + "required": true, + "schema": { "type": "string" } + }, + "queryParam": { + "name": "queryParam", + "description": "query parameter from file one", + "in": "query", + "schema": { "type": "string" } + }, + "headerParam": { + "name": "X-Header-Param", + "description": "header parameter from file one", + "in": "header", + "schema": { "type": "string" } + }, + "cookieParam": { + "name": "cookieParam", + "description": "cookie parameter from file one", + "in": "cookie", + "schema": { "type": "string" } + }, + "a-request": { + "required": true, + "description": "A text request", + "content": { + "text/plain": { + "schema": { "type": "string" } + } + } + }, + "a-response": { + "description": "A text response", + "content": { + "text/plain": { + "schema": { "type": "string" } + } + } + } +} diff --git a/modules/swagger-parser-v3/src/test/resources/issue-2055/external_ref_2.json b/modules/swagger-parser-v3/src/test/resources/issue-2055/external_ref_2.json new file mode 100644 index 0000000000..9beaa875f9 --- /dev/null +++ b/modules/swagger-parser-v3/src/test/resources/issue-2055/external_ref_2.json @@ -0,0 +1,60 @@ +{ + "limit": { + "name": "limit", + "description": "There can be only 2", + "in": "query", + "schema": { + "type": "integer", + "maximum": 2, + "minimum": 0 + } + }, + "pathParam": { + "name": "pathParam", + "description": "path parameter from file two", + "in": "path", + "required": true, + "schema": { "type": "integer" } + }, + "queryParam": { + "name": "queryParam", + "description": "query parameter from file two", + "in": "query", + "schema": { "type": "integer" } + }, + "headerParam": { + "name": "X-Header-Param", + "description": "header parameter from file two", + "in": "header", + "schema": { "type": "integer" } + }, + "cookieParam": { + "name": "cookieParam", + "description": "cookie parameter from file two", + "in": "cookie", + "schema": { "type": "integer" } + }, + "a-request": { + "required": true, + "description": "A JSON request", + "content": { + "application/json": { + "schema": { "$ref": "#/an-object" } + } + } + }, + "a-response": { + "description": "A JSON response", + "content": { + "application/json": { + "schema": { "$ref": "#/an-object" } + } + } + }, + "an-object": { + "type": "object", + "properties": { + "a-prop": { "type": "string" } + } + } +} diff --git a/modules/swagger-parser-v3/src/test/resources/issue-2055/openapi.json b/modules/swagger-parser-v3/src/test/resources/issue-2055/openapi.json new file mode 100644 index 0000000000..01f618fc17 --- /dev/null +++ b/modules/swagger-parser-v3/src/test/resources/issue-2055/openapi.json @@ -0,0 +1,71 @@ +{ + "openapi": "3.0.3", + "info": { + "version": "1.0.0", + "title": "OpenAPI Service" + }, + "paths": { + "/a-path": { + "post": { + "operationId": "ignoredRefPost", + "parameters": [ + { "$ref": "./external_ref_1.json#/limit" } + ], + "requestBody": { "$ref": "external_ref_1.json#/a-request" }, + "responses": { + "200": { "$ref": "external_ref_1.json#/a-response" }, + "default": { "$ref": "#/components/responses/default-string" } + } + }, + "put": { + "operationId": "ignoredRefPut", + "parameters": [ + { "$ref": "./external_ref_2.json#/limit" } + ], + "requestBody": { "$ref": "external_ref_2.json#/a-request" }, + "responses": { + "200": { "$ref": "external_ref_2.json#/a-response" }, + "default": { "$ref": "#/components/responses/default-string" } + } + } + }, + "/parameter-types/{pathParam}": { + "post": { + "operationId": "parameterTypesFromFirstFile", + "parameters": [ + { "$ref": "./external_ref_1.json#/pathParam" }, + { "$ref": "./external_ref_1.json#/queryParam" }, + { "$ref": "./external_ref_1.json#/headerParam" }, + { "$ref": "./external_ref_1.json#/cookieParam" } + ], + "responses": { + "204": { "description": "No content" } + } + }, + "put": { + "operationId": "parameterTypesFromSecondFile", + "parameters": [ + { "$ref": "./external_ref_2.json#/pathParam" }, + { "$ref": "./external_ref_2.json#/queryParam" }, + { "$ref": "./external_ref_2.json#/headerParam" }, + { "$ref": "./external_ref_2.json#/cookieParam" } + ], + "responses": { + "204": { "description": "No content" } + } + } + } + }, + "components": { + "responses": { + "default-string": { + "description": "Default response", + "content": { + "text/plain": { + "schema": { "type": "string" } + } + } + } + } + } +}