swagger-api
Changes to support modelAsSchema in response
This PR supports the changes made in: The Response attribute schema, has been change from property to model, this is a big change and affects the projects below, including codegen. I write here the PR's of every project and the changes made, also the issues related.
Core: https://github.com/swagger-api/swagger-core/pull/2583 Parser: https://github.com/swagger-api/swagger-parser/pull/610 Issues: swagger-api/swagger-parser#567 Inflector: https://github.com/swagger-api/swagger-inflector/pull/231
@gracekarina did you get the following error when building swagger codegen?
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /private/tmp/swagger-codegen/modules/swagger-codegen/src/main/java/io/swagger/codegen/DefaultCodegen.java:[2327,28] cannot find symbol
symbol: method getResponseSchema()
location: variable response of type io.swagger.models.Response
[INFO] 1 error
Current master is 2.3.1 (patch release) so we should not target this PR to the current master. I would suggest targeting this breaking change to the 3.0.0 branch.
cc the core team @cbornet @jimschubert @jaz-ah
cc @HugoMario as he's actively working on the 3.0.0 branch
@wing328 you need to build, core (feature/modelInResponse_refactored), parser (testing-response-schema) and codegen (testing-response-schema) in the correspondent branch.
About pointing the breaking change to 3.0, this issue doesn't happen in 3.0. @webron
About pointing the breaking change to 3.0, this issue doesn't happen in 3.0.
Then instead of fixing it in 2.x, what about asking users to use 3.0.0 (which will be released later in Q1/Q2?) instead if they hit this bug?
I commited a change without the method that breaks the compilation, @wing328 about: Current master is 2.3.1 (patch release) so we should not target this PR to the current master. How do I point to that? Maybe I miss the branch :)
I just changed it to 2.4.0. (there's an edit button next to the PR subject to change that)
2.4.0 should be the last 2.x release as we'll ask users to use 3.0.0 instead.
@wing328 We (@webron @frantuma) are trying to determine the impact on codegen by implementing this change.
- This will mean changing the deprecated methods (getSchema, setSchema, schema) in codegen, replacing the property in response for the model new one.
- Does it affects templates?
- We need to make the changes in core and parser, and those would end up breaking the codegen if it tries to use newer versions of the parser. How much of an effort it would be?
@gracekarina To answer your questions, I need more time to build the parser and core locally in order to confirm the impact to swagger codegen.
Is it correct to say that having the users to use 3.0.0 is not an option?
@wing328 while 2.4 would be (hopefully) the last minor version for 2.X, we can't guarantee that 2.4.0 will be the last release.
We keep maintaining swagger-core 1.5 and swagger-parser 1.0 as there are plenty of users that still use Swagger/OpenAPI 2.0 and we have not announced EOL for those versions.
The codegen is slightly different in the sense 3.0.0 supports older versions of the spec, however, there might still be reasons why we'd need to have maintenance releases of 2.4. If a security issue is found in the parser 1.0 for example. While we would encourage users to use 3.0.0, we can't force immediate migration to it until we announce a public EOL for the 2.X branch. This is obviously extra maintenance work, but there's no way around it.
We're trying to understand the amount of work this kind of a change will bring on the codegen, to see if it even makes sense or that we should try to come up with alternatives. We've been asked to make these some of these changes - and they also impact codegen 3.0.0 because they affect our ability to properly convert 2.0 definitions to 3.0.
Unfortunately, the underlying change is a necessary evil due to mistakes done in the original structure.
@wing328 you need to build, core (feature/modelInResponse_refactored), parser (testing-response-schema) and codegen (testing-response-schema) in the correspondent branch.
UPDATE: I built core (feature/modelInResponse_refactored), parser (testing-response-schema) and codegen (testing-response-schema) locally and checkout aa213c5850c7f602c419209b015a5c8555862dc9 in this PR. Built without issues.
Then I ran some bin scripts under "./bin/" to test client generators (Java (okhttp), C#, Go, Ruby, PHP, Python) and server generators (Python Flask, PHP Symfony, Go server, Spring, Rust). The good thing is that there's no difference in terms of the output (auto-generated code).
Can you also test locally by running the following bin scripts (or windows batch files under .\bin\windows)and use git diff to verify if there's any change?
./bin/php-petstore.sh
./bin/ruby-petstore.sh
./bin/java-petstore-okhttp-gson.sh
./bin/csharp-petstore.sh
./bin/rust-server-petstore.sh
./bin/php-symfony-petstore.sh
@wing328, I've tried locally and the difference apparently it's just the codegen version. can you help us with this part:
@wing328 We (@webron @frantuma) are trying to determine the impact on codegen by implementing this change. This will mean changing the deprecated methods (getSchema, setSchema, schema) in codegen, replacing the property in response for the model new one (getResponseSchema, setResponseSchema, ResponseSchema). Does it affects templates? We need to make the changes in core and parser, and those would end up breaking the codegen if it tries to use newer versions of the parser. How much of an effort it would be?
Does it affects templates?
Based on my test result, it does not affect the templates. In other words, we don't need to update the template for this change
I've just seen this issue, the email notification was sorted weirdly in my email. I'll try to review these changes tonight or tomorrow. Sorry for the delay in my response.
I've built this branch and run locally ./bin/run-all-petstore.
I admittedly haven't dug into the linked issues to understand the changes being made that in depth. I have built those dependent branches and then run the codegen on this PR's branch.
I didn't see any errors, and I only see a handful of changes in samples outputs that I'd ask maintainers about. Some example stuff from the languages where I noticed changes is below.
I then tried to run the C# client codegen against the yaml from swagger-api/swagger-parser#488, but I didn't get an expected output from the generation. I'll dig more into that possibly tomorrow.
Eiffel Client
Eiffel client seems to generate empty assertions (truncated diff):
diff --git a/samples/client/petstore/eiffel/src/api/fake_api.e b/samples/client/petstore/eiffel/src/api/fake_api.e
index 653d53808..5f7b962d3 100644
--- a/samples/client/petstore/eiffel/src/api/fake_api.e
+++ b/samples/client/petstore/eiffel/src/api/fake_api.e
@@ -32,6 +32,7 @@ feature -- API Access
--
--
-- Result OUTER_BOOLEAN
+ require
local
l_path: STRING
l_request: API_CLIENT_REQUEST
@@ -42,6 +43,7 @@ feature -- API Access
l_request.set_body(body)
l_path := "/fake/outer/boolean"
+
if attached {STRING} api_client.select_header_accept (<<>>) as l_accept then
l_request.add_header(l_accept,"Accept");
end
@@ -65,6 +67,7 @@ feature -- API Access
--
--
-- Result OUTER_COMPOSITE
+ require
local
l_path: STRING
l_request: API_CLIENT_REQUEST
@@ -75,6 +78,7 @@ feature -- API Access
l_request.set_body(body)
l_path := "/fake/outer/composite"
I'm not familiar enough with Eiffel to know if this is valid syntax.
Apache 2 configs
I don't know if this is related to changes in the dependent projects, as I haven't had time to investigate… but apache locations seem to be duplicating paths. samples/config/petstore/apache2/PetApi.conf:
diff --git a/samples/config/petstore/apache2/PetApi.conf b/samples/config/petstore/apache2/PetApi.conf
index c92843e26..2d28dcdaa 100644
--- a/samples/config/petstore/apache2/PetApi.conf
+++ b/samples/config/petstore/apache2/PetApi.conf
@@ -1,4 +1,4 @@
-<Location "/v2/pet">
+<Location "/v2/pet/pet/">
AuthBasicProvider file
AuthUserFile "/var/www/html/htpwd"
AuthGroupFile "/var/www/html/groups"
@@ -16,7 +16,7 @@
Require group read:pets
</Limit>
</Location>
-<Location "/v2/pet/*">
+<Location "/v2/pet/{petId}/pet/*/">
AuthBasicProvider file
AuthUserFile "/var/www/html/htpwd"
AuthGroupFile "/var/www/html/groups"
@@ -34,7 +34,7 @@
Require group read:pets
</Limit>
</Location>
-<Location "/v2/pet/findByStatus">
+<Location "/v2/pet/findByStatus/pet/findByStatus/">
AuthBasicProvider file
AuthUserFile "/var/www/html/htpwd"
AuthGroupFile "/var/www/html/groups"
@@ -48,7 +48,7 @@
Require group read:pets
</Limit>
</Location>
-<Location "/v2/pet/findByTags">
+<Location "/v2/pet/findByTags/pet/findByTags/">
AuthBasicProvider file
AuthUserFile "/var/www/html/htpwd"
AuthGroupFile "/var/www/html/groups"
@@ -62,7 +62,7 @@
Require group read:pets
</Limit>
</Location>
-<Location "/v2/pet/*/uploadImage">
+<Location "/v2/pet/{petId}/uploadImage/pet/*/uploadImage/">
AuthBasicProvider file
AuthUserFile "/var/www/html/htpwd"
AuthGroupFile "/var/www/html/groups"
Erlang client
Erlang client seems to require two body parameters on post operations, but it looks like this could have possibly been a change since this branch's merge base (again, truncated):
diff --git a/samples/client/petstore/erlang-client/src/swagger_user_api.erl b/samples/client/petstore/erlang-client/src/swagger_user_api.erl
index 837546f27..d70bbfa6a 100644
--- a/samples/client/petstore/erlang-client/src/swagger_user_api.erl
+++ b/samples/client/petstore/erlang-client/src/swagger_user_api.erl
@@ -14,7 +14,7 @@
%% @doc Create user
%% This can only be done by the logged in user.
-spec create_user(swagger_user:swagger_user(), term()) -> ok | {error, integer()}.
-create_user(Body) ->
+create_user(Body, Body) ->
create_user(Body, Body, #{}).
-spec create_user(swagger_user:swagger_user(), term(), maps:map()) -> ok | {error, integer()}.
@@ -36,7 +36,7 @@ create_user(Body, Body, _Optional) ->
%% @doc Creates list of users with given input array
-spec create_users_with_array_input(list(), term()) -> ok | {error, integer()}.
-create_users_with_array_input(Body) ->
+create_users_with_array_input(Body, Body) ->
create_users_with_array_input(Body, Body, #{}).
Groovy client
A Groovy client inline response object has changed from Map to Integer (samples/client/petstore/android/httpclient has the same issue):
diff --git a/samples/client/petstore/groovy/src/main/groovy/io/swagger/api/StoreApi.groovy b/samples/client/petstore/groovy/src/main/groovy/io/swagger/api/StoreApi.groovy
index bf74dd212..311c36bc2 100644
--- a/samples/client/petstore/groovy/src/main/groovy/io/swagger/api/StoreApi.groovy
+++ b/samples/client/petstore/groovy/src/main/groovy/io/swagger/api/StoreApi.groovy
@@ -52,7 +52,7 @@ class StoreApi {
invokeApi(onSuccess, onFailure, basePath, versionPath, resourcePath, queryParams, headerParams,
"GET", "map",
- Map.class )
+ Integer.class )
}
def getOrderById ( Long orderId, Closure onSuccess, Closure onFailure) {
The client was last generated in March 2017, and it doesn't look like this api (/store/inventory) has changed its response object since then. Here's the path definition from modules/swagger-codegen/src/test/resources/2_0/petstore.yaml:
/store/inventory:
get:
tags:
- store
summary: Returns pet inventories by status
description: Returns a map of status codes to quantities
operationId: getInventory
produces:
- application/json
parameters: []
responses:
'200':
description: successful operation
schema:
type: object
additionalProperties:
type: integer
format: int32
security:
- api_key: []
I've merged master into this branch locally and rebuilt to test that yaml again. Here's the yaml for reference:
swagger: "2.0"
info:
description: "Stupid API"
version: "1.0.0"
title: "Swagger API"
host: "localhost"
basePath: "/"
schemes:
- "http"
paths:
'/what':
get:
summary: ok
responses:
200:
description: "Success"
schema:
$ref: '#/definitions/AllOfArray'
definitions:
Obj1:
type: object
properties:
prop1:
type: string
AllOfArray:
type: array
items:
allOf:
- $ref: "#/definitions/Obj1"
- properties:
prop:
type: string
Looks like the parent property isn't being set in C# in master, either.
I tested a new Java generation with the sample input, and java results in this model signature:
public class AllOfArray extends ArrayList<ERRORUNKNOWN> {
}
This is a log of the output from generating the java client: https://gist.github.com/jimschubert/edf66a3c92e8ba223e784bab98bdb6a5
I then generated a java client against the bug.in.yaml from issue 567 in swagger-parser (on this branch with the merged master changes), and it results in this model:
/**
* InlineResponse200
*/
@javax.annotation.Generated(value = "io.swagger.codegen.languages.JavaClientCodegen", date = "2018-01-24T13:34:41.977-05:00")
public class InlineResponse200 {
@SerializedName("ReferencedObject1")
private ReferencedObject1 referencedObject1 = null;
@SerializedName("ReferencedObject2")
private ReferencedObject2 referencedObject2 = null;
public InlineResponse200 referencedObject1(ReferencedObject1 referencedObject1) {
this.referencedObject1 = referencedObject1;
return this;
}
/**
* Get referencedObject1
* @return referencedObject1
**/
@ApiModelProperty(value = "")
public ReferencedObject1 getReferencedObject1() {
return referencedObject1;
}
public void setReferencedObject1(ReferencedObject1 referencedObject1) {
this.referencedObject1 = referencedObject1;
}
public InlineResponse200 referencedObject2(ReferencedObject2 referencedObject2) {
this.referencedObject2 = referencedObject2;
return this;
}
/**
* Get referencedObject2
* @return referencedObject2
**/
@ApiModelProperty(value = "")
public ReferencedObject2 getReferencedObject2() {
return referencedObject2;
}
public void setReferencedObject2(ReferencedObject2 referencedObject2) {
this.referencedObject2 = referencedObject2;
}
@Override
public boolean equals(java.lang.Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
InlineResponse200 inlineResponse200 = (InlineResponse200) o;
return Objects.equals(this.referencedObject1, inlineResponse200.referencedObject1) &&
Objects.equals(this.referencedObject2, inlineResponse200.referencedObject2);
}
@Override
public int hashCode() {
return Objects.hash(referencedObject1, referencedObject2);
}
@Override
public String toString() {
StringBuilder sb = new StringBuilder();
sb.append("class InlineResponse200 {\n");
sb.append(" referencedObject1: ").append(toIndentedString(referencedObject1)).append("\n");
sb.append(" referencedObject2: ").append(toIndentedString(referencedObject2)).append("\n");
sb.append("}");
return sb.toString();
}
/**
* Convert the given object to string with each line indented by 4 spaces
* (except the first line).
*/
private String toIndentedString(java.lang.Object o) {
if (o == null) {
return "null";
}
return o.toString().replace("\n", "\n ");
}
}
So, it looks like the change fixes inline allOf, but may not fully support allOf with arrays as reported in 488 on the parser.
Hi, @jimschubert thanks for testing, you're right the #488 is not in the scope of this PR. It's related. but it will required another PR for the solution. 1.About the others issues found in the testing, are they related to this PR? Do you think we could merge? 2.Can someone help with the implementation of the new changes in codegen (removing deprecated methods) before merging? Thanks. @wing328 @frantuma @webron
My input for question 1 is that I think the issues I presented above would be more easily resolved by the community after merge.
Hi @wing328 @jimschubert,thanks for your validations, we have merged the PR's in core/parser and inflector related to this change. swagger-api/swagger-core#2583 swagger-api/swagger-parser#610 swagger-api/swagger-inflector#231