Skip to content

CSharp code generator - http return code 201 causes an exception #730

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

Closed
boazsapir opened this issue May 7, 2015 · 36 comments
Closed

CSharp code generator - http return code 201 causes an exception #730

boazsapir opened this issue May 7, 2015 · 36 comments

Comments

@boazsapir
Copy link

In the CSharp generated code, when the api call returns 201 an exception is thrown although this code does not indicate any error or problem

@wing328
Copy link
Contributor

wing328 commented May 7, 2015

Is it correct to say that the issue also occurs in the "csharp_restcsharp" PR #700 ?

@boazsapir
Copy link
Author

Yes, this is the generated code:
if (webResponse.StatusCode != HttpStatusCode.OK)
{
webResponse.Close();
throw new ApiException((int)webResponse.StatusCode, webResponse.StatusDescription);
}

@psmay
Copy link
Contributor

psmay commented May 7, 2015

See #731 for a tiny fix.

@psmay
Copy link
Contributor

psmay commented May 7, 2015

#700 seems to remove/replace the broken code also.

@wing328
Copy link
Contributor

wing328 commented May 8, 2015

@boazsapir In the generated C# code (csharp_restsharp branch), I couldn't find any reference to HttpStatusCode.

csharp|csharp_restcsharp ⇒ ls
bin         compile.bat src
csharp|csharp_restcsharp ⇒ grep -R -i HttpStatusCode *
csharp|csharp_restcsharp ⇒ 

@psmay
Copy link
Contributor

psmay commented May 10, 2015

#731 reworked to #740. Again, this patch is not necessary if #700 / RestSharp is used instead.

@boazsapir
Copy link
Author

@wing328 - sorry for the confusion, I see now that the file ApiInvoker.cs (which had been originally created by the master code) has not been re-created when I used the csharp_restsharp code for generation, therefore the HttpStatusCode problem remained.
The reason for this is an exception that was thrown during the code generation. Looks like property types are checked against a list of reserved words, and this is bound to fail since the list contains the words 'object', 'string', etc. (this check does not happen in the master branch since getSwaggerType() does not call toModelName(), therefore I did not open a separate issue for this).

java.lang.RuntimeException: object (reserved word) cannot be used as a model name
at com.wordnik.swagger.codegen.languages.CSharpClientCodegen.toModelName(CSharpClientCodegen.java:133)
at com.wordnik.swagger.codegen.languages.CSharpClientCodegen.getSwaggerType(CSharpClientCodegen.java:174)
at com.wordnik.swagger.codegen.DefaultCodegen.fromProperty(DefaultCodegen.java:554)
at com.wordnik.swagger.codegen.DefaultCodegen.fromResponse(DefaultCodegen.java:866)
at com.wordnik.swagger.codegen.DefaultCodegen.fromOperation(DefaultCodegen.java:729)
at com.wordnik.swagger.codegen.DefaultGenerator.processOperation(DefaultGenerator.java:327)
at com.wordnik.swagger.codegen.DefaultGenerator.processPaths(DefaultGenerator.java:299)
at com.wordnik.swagger.codegen.DefaultGenerator.generate(DefaultGenerator.java:156)
at com.wordnik.swagger.codegen.cmd.Generate.run(Generate.java:77)
at com.wordnik.swagger.codegen.SwaggerCodegen.main(SwaggerCodegen.java:33)

@wing328
Copy link
Contributor

wing328 commented May 10, 2015

I would recommend you to test against the develop_2.0 branch which contains bug fixes and enhancements that will be merged into master later

toModelName (in develop_2.0 branch) aims to prevent the use of C# reserved words as class/object name

screen shot 2015-05-10 at 5 01 42 pm

Would it be possible to rename the model using a non-reserved keyword ?

@boazsapir
Copy link
Author

I do not have a model named 'object', the failure is because the code checks the property type against the list of reserved words:

@Override
  public String getSwaggerType(Property p) {
    String swaggerType = super.getSwaggerType(p);
    String type = null;
    if(typeMapping.containsKey(swaggerType)) {
      type = typeMapping.get(swaggerType);
      if(languageSpecificPrimitives.contains(type))
        return type;
    }
    else
      type = swaggerType;
    return toModelName(type);
  }

@wing328
Copy link
Contributor

wing328 commented May 10, 2015

@boazapir you're right. I'll try to fix that in the csharp_restsharp branch.

@wing328
Copy link
Contributor

wing328 commented May 10, 2015

@boazsapir in the spec is there a parameter or a property with type set to "object" ?

@boazsapir
Copy link
Author

@wing328 I have parameters that have a schema which points to an object. Example:

          "parameters": [
                {
                    "name": "Input",
                    "in": "body",
                    "schema": {
                        "$ref": "#/definitions/KeyInput"
                    }
                }
            ],

"definitions": {
    "KeyInput": {
        "properties": {
            "metadata": {
                "description": "user-defined metadata, a printable string",
                "type": "string",
                "maxLength": 1024
            }
        },
        "type": "object"
    }

}

@wing328
Copy link
Contributor

wing328 commented May 11, 2015

@boazsapir based on my understanding, object is not a valid type (ref: Swager Spec 2.0 DataType)

I've not seen the use of type for models. If you look at the models defined at Swagger Petstore. None of the models have type defined (in the same level as properties).

May I know how the Swagger spec is generated ?

@webron
Copy link
Contributor

webron commented May 11, 2015

@wing328 - actually, that section in the spec refers to primitives:

Primitive data types in the Swagger Specification are based on the types supported by the JSON-Schema Draft 4. Models are described using the Schema Object which is a subset of JSON Schema Draft 4.

object is a valid type and in important one too. The output we have for the petstore is incomplete. It should definitely have "type": "object" for the models. The fact that it's missing there actually makes the meaning of the schema slightly different (and not in a good way). It's always better to include the type, even when it's object. I'd even make the type field mandatory, but that would hurt some other definition variations. This is one of the things I hope to treat better in the next version of the spec.

@boazsapir
Copy link
Author

My Swagger spec is generated from php annotations.
For my example:

     * @SWG\Definition(definition="KeyInput", type="object",
     *   @SWG\Property(property="metadata", type="string", description="user-defined metadata, a printable string", maxLength=1024)
     * )

    *   @SWG\Parameter(name="Input", in="body", @SWG\Schema(ref="#/definitions/UpdateEncryptionKeyInput")),

@wing328
Copy link
Contributor

wing328 commented May 11, 2015

@webron Thanks for the clarification. I would recommend updating swagger.json for petstore with "type":"object" to illustrate that object is also a valid type.

I'll update the Petstore JSON locally to see if I can repeat the issue.

@webron
Copy link
Contributor

webron commented May 11, 2015

@wing328
Copy link
Contributor

wing328 commented May 11, 2015

@webron thanks!

@boazsapir I added "type":"object" to models defined in petstore.json in my local drive but couldn't repeat the issue with develop_2.0 branch nor csharp_restsharp branch.

If I manually update the parameter from string to object as follows:

        "parameters": [
          {
            "name": "tags",
            "in": "query",
            "description": "Tags to filter by",
            "required": false,
            "type": "array",
            "items": {
              "type": "object"
            },
            "collectionFormat": "multi"
          }
        ],

I could repeat the issue.

Do you mind double checking to see where else you can find "type": "object" in the spec ?

@boazsapir
Copy link
Author

You are right, I also have it in a response:

               "responses": {
                    "200": {
                        "description": "Swagger JSON Specification",
                        "schema": {
                            "title": "SwaggerSchema",
                            "type": "object"
                        }
                    }
                }

When I remove this part from my spec the problem does not occur anymore

@boazsapir
Copy link
Author

@wing328 I have some issues that seem to be specific to the csharp_restsharp branch. How do you want me to report on them?
Most critical problem is that it seems like the default headers are not used when making a request (I added a default header using AddDefaultHeader() but I don't see that GetDefaultHeader() is used subsequently

@wing328
Copy link
Contributor

wing328 commented May 12, 2015

@boazsapir Thanks for reporting the issue. I've fixed the bug. In addition, I've updated the package name to conform to the C# package naming convention as follows:

using IO.Swagger.Api;
using IO.Swagger.Model;
using IO.Swagger.Client;

Please kindly give it another try.

@wing328
Copy link
Contributor

wing328 commented May 12, 2015

@boazsapir if response is an object (model). I believe the response should be defined as something like this:

responses: {
  200: {
    description: "successful operation",
    schema: {
      $ref: "#/definitions/Pet"
    }
  }
}

@boazsapir
Copy link
Author

@wing328

  • the problem with the default header is solved now.
  • a new problem that is blocking me now: if the http response code is an error code (e.g. 401), this information is not propagated to the user and is practically lost. Example of generated code:

try {
// make the HTTP request
IRestResponse response = restClient.Execute(_request);
return (StoredProtectedItem) ApiInvoker.Deserialize(response.Content, typeof(StoredProtectedItem));
} catch (Exception ex) {
if(ex != null) {
return null;
}
else {
throw ex;
}
}

If there is an error code it will appear in the StatusCode field of the response but no exception will be thrown (I guess this is the behavior of restClient). StatusCode is not checked by the generated function, the response content (which is null) is deserialized and returned to the caller. The caller cannot know what was the error code.

P.S. what is the status of the problem with the reserved words?

Thank you
Boaz

@wing328
Copy link
Contributor

wing328 commented May 14, 2015

@boazsapir I'll look into the error code issue.

For reserved word, you mentioned the response is

               "responses": {
                    "200": {
                        "description": "Swagger JSON Specification",
                        "schema": {
                            "title": "SwaggerSchema",
                            "type": "object"
                        }
                    }
                }

which is not the correct way to define a model(object) response based on my knowledge. It should look something like this:

responses: {
  200: {
    description: "successful operation",
    schema: {
      $ref: "#/definitions/Pet"
    }
  }
}

If you add a proper $ref, would the exception about reserved keyword still occur ?

@wing328
Copy link
Contributor

wing328 commented May 14, 2015

@boazsapir Updated csharp_restsharp to throw an exception if status code >= 400

Here is a sample exception message:

Error calling GetPetById: {"code":1,"type":"error","message":"Pet not found"}

Once again, appreciate your help to test the change.

@wing328
Copy link
Contributor

wing328 commented May 20, 2015

@boazsapir may I know if you've a chance to do a test? please let me know if you still have issue not getting the proper exception.

@boazsapir
Copy link
Author

@wing328 I tried the new code and the problem seems to be fixed. I will do some more tests tomorrow and let you know if there are additional problems.

Regarding the reserved keyword issue, using $ref solves the problem, however according to the spec I don't see why it is not legal to have an 'inline' schema definition without a $ref.
See this example which appears in the spec:

Response with a string type:

{
  "description": "A simple string response",
  "schema": {
    "type": "string"
  }
}

description: A simple string response
schema:
  type: string

Can you elaborate on this?

@wing328
Copy link
Contributor

wing328 commented May 21, 2015

@boazsapir my understanding is that if the type is "object", you will need to provide the definition of the object that you've defined in the "definitions" section of the spec.

@webron
Copy link
Contributor

webron commented May 21, 2015

@wing328 - it's valid inline as well, doesn't have to be defined under definitions.

@wing328
Copy link
Contributor

wing328 commented May 21, 2015

@webron Thanks for the clarification.

@boazsapir I'll try to address this issue and keep you posted. The fix should generate the method to just return a C# Object.

@fehguy
Copy link
Contributor

fehguy commented May 21, 2015

To be clear, we have not added support in codegen for in-line models, even though they're valid in the spec. It's not hard, but we'll have to add a general mechanism for handling anonymous definitions as many languages don't support them.

@boazsapir
Copy link
Author

Note that in Java code generation I had no problem with my inline model definition

@wing328
Copy link
Contributor

wing328 commented May 22, 2015

Submitted #775 to address the issue. The endpoint with response type set to object without $ref will return an Object (HTTP body in the form of string)

@boazsapir
Copy link
Author

@wing328 sorry I was travelling and could not respond. I will try to check this week whether it solves my problem

@wing328
Copy link
Contributor

wing328 commented Jun 3, 2015

@boazsapir enjoy your travel and let me know later if you've any feedback on the fix

@boazsapir
Copy link
Author

inline object works fine now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants