Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consider callbacks in unused schemas #1232

Merged
merged 1 commit into from Oct 18, 2018
Merged

Consider callbacks in unused schemas #1232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 72 additions & 26 deletions modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@
import io.swagger.v3.oas.models.OpenAPI;
import io.swagger.v3.oas.models.Operation;
import io.swagger.v3.oas.models.PathItem;
import io.swagger.v3.oas.models.callbacks.Callback;
import io.swagger.v3.oas.models.headers.Header;
import io.swagger.v3.oas.models.media.ArraySchema;
import io.swagger.v3.oas.models.media.BinarySchema;
Expand Down Expand Up @@ -167,40 +168,56 @@ private static void visitOpenAPI(OpenAPI openAPI, OpenAPISchemaVisitor visitor)

if (paths != null) {
for (PathItem path : paths.values()) {
List<Operation> allOperations = path.readOperations();
if (allOperations != null) {
for (Operation operation : allOperations) {
//Params:
if (operation.getParameters() != null) {
for (Parameter p : operation.getParameters()) {
Parameter parameter = getReferencedParameter(openAPI, p);
if (parameter.getSchema() != null) {
visitSchema(openAPI, parameter.getSchema(), null, visitedSchemas, visitor);
}
}
visitPathItem(path, openAPI, visitor, visitedSchemas);
}
}
}

private static void visitPathItem(PathItem pathItem, OpenAPI openAPI, OpenAPISchemaVisitor visitor, List<String> visitedSchemas) {
List<Operation> allOperations = pathItem.readOperations();
if (allOperations != null) {
for (Operation operation : allOperations) {
//Params:
if (operation.getParameters() != null) {
Copy link
Member

Choose a reason for hiding this comment

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

I know most of the code here was just reorganized, but I just had a question. I'm wondering why we follow visitor pattern only for a few elements (visitSchema) but not others which are commented here like "Params", "RequestBody", "Responses", and "Callbacks"? It seems like this would make for smaller surface area for unit testing edge cases.

Copy link
Member Author

@jmini jmini May 19, 2020 •
edited

Choose a reason for hiding this comment

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

@jimschubert: I never forgot your comment about having a true visitor Pattern for an OpenAPI spec.

I have implemented it in OpenAPITools/empoa#14

See: https://openapitools.github.io/empoa/#visitor-pattern

for (Parameter p : operation.getParameters()) {
Parameter parameter = getReferencedParameter(openAPI, p);
if (parameter.getSchema() != null) {
visitSchema(openAPI, parameter.getSchema(), null, visitedSchemas, visitor);
}
}
}

//RequestBody:
RequestBody requestBody = getReferencedRequestBody(openAPI, operation.getRequestBody());
if (requestBody != null && requestBody.getContent() != null) {
for (Entry<String, MediaType> e : requestBody.getContent().entrySet()) {
//RequestBody:
RequestBody requestBody = getReferencedRequestBody(openAPI, operation.getRequestBody());
if (requestBody != null && requestBody.getContent() != null) {
for (Entry<String, MediaType> e : requestBody.getContent().entrySet()) {
if (e.getValue().getSchema() != null) {
visitSchema(openAPI, e.getValue().getSchema(), e.getKey(), visitedSchemas, visitor);
}
}
}

//Responses:
if (operation.getResponses() != null) {
for (ApiResponse r : operation.getResponses().values()) {
ApiResponse apiResponse = getReferencedApiResponse(openAPI, r);
if (apiResponse != null && apiResponse.getContent() != null) {
for (Entry<String, MediaType> e : apiResponse.getContent().entrySet()) {
if (e.getValue().getSchema() != null) {
visitSchema(openAPI, e.getValue().getSchema(), e.getKey(), visitedSchemas, visitor);
}
}
}
}
}

//Responses:
if (operation.getResponses() != null) {
for (ApiResponse r : operation.getResponses().values()) {
ApiResponse apiResponse = getReferencedApiResponse(openAPI, r);
if (apiResponse != null && apiResponse.getContent() != null) {
for (Entry<String, MediaType> e : apiResponse.getContent().entrySet()) {
if (e.getValue().getSchema() != null) {
visitSchema(openAPI, e.getValue().getSchema(), e.getKey(), visitedSchemas, visitor);
}
}
}
//Callbacks:
if (operation.getCallbacks() != null) {
for (Callback c : operation.getCallbacks().values()) {
Callback callback = getReferencedCallback(openAPI, c);
if (callback != null) {
for (PathItem p : callback.values()) {
visitPathItem(p, openAPI, visitor, visitedSchemas);
}
}
}
Expand Down Expand Up @@ -603,6 +620,35 @@ public static Parameter getParameter(OpenAPI openAPI, String name) {
return null;
}

/**
* If a Callback contains a reference to an other Callback with '$ref', returns the referenced Callback if it is found or the actual Callback in the other cases.
*
* @param openAPI specification being checked
* @param callback potentially containing a '$ref'
* @return callback without '$ref'
*/
public static Callback getReferencedCallback(OpenAPI openAPI, Callback callback) {
if (callback != null && StringUtils.isNotEmpty(callback.get$ref())) {
String name = getSimpleRef(callback.get$ref());
Callback referencedCallback = getCallback(openAPI, name);
if (referencedCallback != null) {
return referencedCallback;
}
}
return callback;
}

public static Callback getCallback(OpenAPI openAPI, String name) {
if (name == null) {
return null;
}

if (openAPI != null && openAPI.getComponents() != null && openAPI.getComponents().getCallbacks() != null) {
return openAPI.getComponents().getCallbacks().get(name);
}
return null;
}

/**
* Return the first defined Schema for a RequestBody
*
Expand Down
10 changes: 8 additions & 2 deletions modules/openapi-generator/src/test/java/org/openapitools/codegen/utils/ModelUtilsTest.java
View file
Open in desktop
Expand Up @@ -19,7 +19,11 @@

import io.swagger.parser.OpenAPIParser;
import io.swagger.v3.oas.models.OpenAPI;
import io.swagger.v3.oas.models.media.*;
import io.swagger.v3.oas.models.media.ComposedSchema;
import io.swagger.v3.oas.models.media.IntegerSchema;
import io.swagger.v3.oas.models.media.ObjectSchema;
import io.swagger.v3.oas.models.media.Schema;
import io.swagger.v3.oas.models.media.StringSchema;
import io.swagger.v3.oas.models.parameters.Parameter;
import io.swagger.v3.oas.models.parameters.RequestBody;
import io.swagger.v3.oas.models.responses.ApiResponse;
Expand All @@ -40,7 +44,7 @@ public class ModelUtilsTest {
public void testGetAllUsedSchemas() {
final OpenAPI openAPI = new OpenAPIParser().readLocation("src/test/resources/3_0/unusedSchemas.yaml", null, new ParseOptions()).getOpenAPI();
List<String> allUsedSchemas = ModelUtils.getAllUsedSchemas(openAPI);
Assert.assertEquals(allUsedSchemas.size(), 28);
Assert.assertEquals(allUsedSchemas.size(), 30);

Assert.assertTrue(allUsedSchemas.contains("SomeObjShared"), "contains 'SomeObjShared'");
Assert.assertTrue(allUsedSchemas.contains("SomeObj1"), "contains 'UnusedObj1'");
Expand Down Expand Up @@ -70,6 +74,8 @@ public void testGetAllUsedSchemas() {
Assert.assertTrue(allUsedSchemas.contains("Obj19ByType"), "contains 'Obj19ByType'");
Assert.assertTrue(allUsedSchemas.contains("SomeObj20"), "contains 'SomeObj20'");
Assert.assertTrue(allUsedSchemas.contains("OtherObj20"), "contains 'OtherObj20'");
Assert.assertTrue(allUsedSchemas.contains("PingDataInput21"), "contains 'PingDataInput21'");
Assert.assertTrue(allUsedSchemas.contains("PingDataOutput21"), "contains 'PingDataOutput21'");
}

@Test
Expand Down
47 changes: 46 additions & 1 deletion modules/openapi-generator/src/test/resources/3_0/unusedSchemas.yaml
View file
Open in desktop
Expand Up @@ -233,6 +233,36 @@ paths:
text/plain:
schema:
$ref: '#/components/schemas/SomeObj20'
/some/p21:
post:
operationId: op21
parameters:
- name: callbackUrl
in: query
required: true
schema:
type: string
format: uri
example: https://some-server.com
responses:
'201':
description: OK
callbacks:
onPing:
'{$request.query.callbackUrl}/ping':
post:
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/PingDataInput21'
responses:
'200':
description: Ok
content:
application/json:
schema:
$ref: '#/components/schemas/PingDataOutput21'
components:
schemas:
UnusedObj1:
Expand Down Expand Up @@ -463,6 +493,20 @@ components:
OtherObj20:
type: string
enum: [A, B, C]
PingDataInput21:
type: object
properties:
id:
type: integer
data:
type: String
PingDataOutput21:
type: object
properties:
id:
type: integer
reply:
type: String
SomeObjShared:
type: object
properties:
Expand Down Expand Up @@ -515,4 +559,5 @@ components:
name: id
in: query
schema:
$ref: '#/components/schemas/SomeObj11'
$ref: '#/components/schemas/SomeObj11'