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.

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

support binary input and output #1124

Merged
merged 4 commits into from
Aug 24, 2015
Merged

support binary input and output #1124

merged 4 commits into from
Aug 24, 2015

Conversation

boazsapir
Copy link

This is my implementation of Java code generation, including the generic (not language specific) support needed, for binary data in the request body and in the response.
This implementation is based on the agreed spec, as discussed in OAI/OpenAPI-Specification#50 and depends on my pull request to swagger-core which was merged on Aug-6 (swagger-api/swagger-core#1335)
This PR replaces #1065

b_sapir added 4 commits August 24, 2015 14:33

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…h type "string" and format "binary". Implemented for Java.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…h type "string" and format "binary". Implemented for Java.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Conflicts:
	modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JavaClientCodegen.java
	modules/swagger-codegen/src/main/resources/Java/ApiClient.mustache
	modules/swagger-codegen/src/main/resources/Java/api.mustache
fehguy added a commit that referenced this pull request Aug 24, 2015
support binary input and output
@fehguy fehguy merged commit 698ed97 into swagger-api:master Aug 24, 2015
{{/isBinary}}

{{/isDefault}}
{{/responses}}
Copy link
Contributor

Choose a reason for hiding this comment

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

@boazsapir the above changes seem to make integration tests fail for the Petstore sample:

mvn clean package
./bin/java-petstore.sh
cd samples/client/petstore/java/default
mvn clean test
Results :

Failed tests:   testFindPetsByStatus(io.swagger.petstore.test.PetApiTest)
  testFindPetsByTags(io.swagger.petstore.test.PetApiTest)

Tests in error:
  testUpdatePetWithForm(io.swagger.petstore.test.PetApiTest): {"code":1,"type":"error","message":"Pet not found"}
  testDeletePet(io.swagger.petstore.test.PetApiTest): {"code":1,"type":"error","message":"Pet not found"}
  testCreateAndGetPet(io.swagger.petstore.test.PetApiTest): {"code":1,"type":"error","message":"Pet not found"}
  testUpdatePet(io.swagger.petstore.test.PetApiTest): {"code":1,"type":"error","message":"Pet not found"}

Tests run: 33, Failures: 2, Errors: 4, Skipped: 0

In the addPet method of PetApi, the apiClient.invokeAPI statement is missing, so no HTTP request is sent at all.

Copy link
Author

Choose a reason for hiding this comment

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

@xhh while trying to test my fix I encountered a new problem in JavaClientCodegen.PostProcessModels, which blocks me from testing.

row 341:
List values = (List) allowableValues.get("values");

'values' is set to null and is later referenced (row 344).

This is the definition that causes the crash:

   "DerivationInput": {
        "description": "Key derivation input",
        "required": [
            "length",
            "data"
        ],
        "properties": {
            "length": {
                "description": "Derived key size",
                "type": "integer",
                "maximum": 8192,
                "minimum": 1
            },
            "data": {
                "description": "Data for key derivation",
                "type": "string",
                "maxLength": 256
            }
        },
        "type": "object"
    },

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for that, my bad. I believe it is fixed via #1144, could you add a null check when testing?

Copy link
Author

Choose a reason for hiding this comment

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

I merged with the upstream branch to get fix #1144 and this solved the problem.
Now I encounter a new problem:
I have this enumeration defined:

            "cloud": {
                "description": "cloud provider",
                "type": "string",
                "enum": [
                    "ec2",
                    "vmware",
                    "openstack"
                ]
            },

The generated code has a syntax error: missing return type for function StatusEnum (should be void):

public enum CloudEnum {
eC2("ec2"), VMWARE("vmware"), OPENSTACK("openstack");

private String value;

StatusEnum(String value) {
this.value = value;
}

@OverRide
public String toString() {
return value;
}
}

Do you know anything about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

oops, StatusEnum should be CloudEnum there, I'll submit a PR to fix it

Copy link
Contributor

Choose a reason for hiding this comment

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

submitted #1168 to fix it

Copy link
Author

Choose a reason for hiding this comment

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

submitted PR #1169 to fix the original problem

@boazsapir
Copy link
Author

@xhh the failure is because addPet (also updatePet, updatePetWithForm, deletePet ) do not have a default response in their definition, only error responses. See this code in DefaultCodegen.java:
private Response findMethodResponse(Map<String, Response> responses) {

    String code = null;
    for (String responseCode : responses.keySet()) {
        if (responseCode.startsWith("2") || responseCode.equals("default")) {
            if (code == null || code.compareTo(responseCode) > 0) {
                code = responseCode;
            }
        }
    }
    if (code == null) {
        return null;
    }
    return responses.get(code);
}

If I add
"200": {
"description": "OK"
},
to the responses section of addPet, updatePet, updatePetWithForm, deletePet - then the test succeeds.
Is this a valid situation to have an operation without a default response?

@xhh
Copy link
Contributor

xhh commented Aug 27, 2015

I think you're right that at least a successful or default response should be defined. According to swagger spec:

it is expected from the documentation to cover a successful operation response and any known errors

The thing is, swagger-codegen did work before even with this "invalid" spec of petstore, so we should either keep tolerant to this case in swagger-codegen, or fix the petstore spec to make it work.

@webron
Copy link
Contributor

webron commented Aug 27, 2015

"SHOULD" - the definition for the pet store is valid. Could be improved, but valid.

@boazsapir
Copy link
Author

ok, I will fix the template to support also the case of no default response

boazsapir pushed a commit to boazsapir/swagger-codegen that referenced this pull request Sep 1, 2015
wing328 added a commit that referenced this pull request Sep 2, 2015
Handle correctly the case of no default response (see comment in #1124)
@fehguy fehguy modified the milestone: v2.1.4 Oct 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants