swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
16.94k stars 6.03k forks source link

swagger-codegen (c#) issue (or misuse ?) for file upload in a POST Web API operation #2175

Open BugRaptor opened 8 years ago

BugRaptor commented 8 years ago

I have a developed a Swagger-Api with a "File" Controller exposing a POST "Upload" operation. If I upload a file from the Swagger UI, it is correctly uploaded by the "File" Controller. I then have generated a C# API client class with swagger-codegen. (v2.1.5) The generated FileApi client class exposes a public async System.Threading.Tasks.Task<ApiResponse<Object>> FileUploadAsyncWithHttpInfo(Stream file) method. When I call the FileApi.FileUploadAsyncWithHttpInfo(stream) method of the generated Api class, I get an System.AggregateException: "One or more errors occured". The unique inner exception is a IO.Swagger.Client.ApiException: "Error calling FileUpload: The request was aborted." The stream is a MemoryStream copied from an open FileStream. I get with the debugger in the FileApi class to see what happens:

Here is the final lines of the

public async System.Threading.Tasks.Task<ApiResponse<Object>> FileUploadAsyncWithHttpInfo (Stream file)
{
    .
    .
    .

    // make the HTTP request
    IRestResponse response = (IRestResponse) await Configuration.ApiClient.CallApiAsync(path_, Method.POST, queryParams, postBody, headerParams, formParams, fileParams, pathParams);

    int statusCode = (int) response.StatusCode;

    if (statusCode >= 400)
        throw new ApiException (statusCode, "Error calling LelPublishPackage: " + response.Content, response.Content);
    else if (statusCode == 0)
        throw new ApiException (statusCode, "Error calling LelPublishPackage: " + response.ErrorMessage, response.ErrorMessage);

    return new ApiResponse<Object>(statusCode,
        response.Headers.ToDictionary(x => x.Name, x => x.Value.ToString()),
        (Object) Configuration.ApiClient.Deserialize(response, typeof(Object)));
}

When calling the CallApiAsync method, the parameters values are the following:

path_: "/api/Files"
Method.POST: POST
queryParams: Count = 0
postBody: null
headerParams: Count = 1 / [0]: {[Accept, application/json]}
formParams: Count = 0
fileParams: Count = 1 / [0]: {[file, RestSharp.FileParameter]} 
pathParams: Count = 1 / [0]: {[format, json]}

After calling the CallApiAsync method, the returned response.StatusCode is 0, its response.Status is Aborted and its response.ErrorMessage is "The request was aborted." Thus, an ApiException is thrown.

If I get into the CallApiAsync() method, the code is:

public async System.Threading.Tasks.Task<Object> CallApiAsync(
    String path, RestSharp.Method method, Dictionary<String, String> queryParams, String postBody,
    Dictionary<String, String> headerParams, Dictionary<String, String> formParams,
    Dictionary<String, FileParameter> fileParams, Dictionary<String, String> pathParams)
{
    var request = PrepareRequest(
        path, method, queryParams, postBody, headerParams, formParams, fileParams, pathParams);
    var response = await RestClient.ExecuteTaskAsync(request);
    return (Object)response;
}

The request is prepared and the the RestClient.ExecuteTaskAsync(request) method is called. The returned response has its StatusCode set to 0, its response.Status is Aborted and its response.ErrorMessage is "The request was aborted."

Do I do something wrong ?

Is there an issue in the swagger-code generated client FileApi class ?

wing328 commented 8 years ago

I would suggest you to try 2 things:

BugRaptor commented 8 years ago

Thank you for your prompt answer. I was already using RestSharp 105.1.0 I just performed a checkout from the swagger-codegen latest master and regenerated my API client. The issue remains the same.

jimschubert commented 8 years ago

The Object in your generated client shouldn't be System.Object unless you're specifically exposing System.Object. If you generated the client and cleaned up the output, make sure to fully qualify the name. This is related to #2064 .

I have a branch which may solve your issue 1255-modify-model-names. This will allow you to add a prefix or suffix to the generated model names, which would help if the Object is causing your problems.

BugRaptor commented 8 years ago

I'm not sure to fully understand. Do you mean I should modify my Web API Swagger controller code ?

Here is the current Web API controller code with its Upload() operation:


    /// <summary>
    /// The File ApiController handling file upload operation.
    /// </summary>
    [System.Web.Http.RoutePrefix("api/Files")]
    [GenerateFactory(typeof(IFileControllerFactory))]
    public class FileController : ApiController,
                                 IFileController
    { 
        .
        .
        .
        [System.Web.Http.Route("")]
        [AddFileInputParameter("File_Upload", "File", "The file to be uploaded.")]
        [SwaggerResponse(HttpStatusCode.Created, "File succesfully uploaded.")]
        [SwaggerResponse(HttpStatusCode.NotFound, "Unable to upload data. (Detailed error message available in the response body).")]
        [SwaggerResponse(HttpStatusCode.InternalServerError, "Internal server error. File not uploaded. An HttpException is returned with the original inner exception thrown. (Detailed error message available in the returned exception).", typeof(HttpException))]
        public async Task<IHttpActionResult> Upload()
        {
            .
            .
            .
        }
    }

Do you suggest I should add a Type argument in the [SwaggerResponse(...)] attribute of the operation's method in my File Web API controller ?

For example:

[SwaggerResponse(HttpStatusCode.Created, "File successfully uploaded.", typeof(System.Object))]

or

[SwaggerResponse(HttpStatusCode.Created, "File successfully uploaded.", typeof(DummyViewModel))]

Anyway in the meantime I will try these two workarounds.

Thank you for your help.

BugRaptor commented 8 years ago

I just tried to add the type argument in the [SwaggerResponse(...)] attribute of the operation's method:

[SwaggerResponse(HttpStatusCode.Created, "File successfully uploaded.", typeof(DummyViewModel))]

And I regenerated the client.

Nevertheless, the generated client's code still remains the same (refering Object type):

System.Threading.Tasks.Task<Object> FileUploadAsync (Stream File); System.Threading.Tasks.Task<ApiResponse<Object>> FileUploadAsyncWithHttpInfo (Stream File);

And calling it still raises an exception:

IO.Swagger.Client.ApiException: Error calling FileUpload: The request was
 aborted.
   at IO.Swagger.Api.FileApi.<FileUploadAsyncWithHttpInfo>d__8.MoveNext()

I suppose I did not understand your workaround advice.

BugRaptor commented 8 years ago

Sorry I was wrong saying the issue remained the same with the latest swagger-codegen master... I didn't correctly regenerated my client with the latest master... (actually, my target/swagger-codegen-cli.jar file was not recompiled !)

I've just correctly recompiled it from the latest master and then tried to regenerate my c-sharp client:

I now notice the following kind of error messages when generating my client (only on Api with POST operations):

Generating assembly C:\Git\red-full-client-v1\solution\src\app\Liebherr.RedFull.ClientV1.Generator\bin\Debug\nuget\lib\net40\RedFull.Client.dll...Failed ! 
Compilation error(s): Error File: LelApi.cs, Line number 42, 
Error Number: CS0104, ''Object' is an ambiguous reference between 'object' and 'IO.Swagger.Model.Object';

I noticed the .mustache template files have been updated and enhanced but they always contain ambiguous references to Object type.

Would they require complementary updates fully specifying the complete namespace IO.Swagger.Model.Object instead of Object in some places ?

jimschubert commented 8 years ago

@BugRaptor yeah that's the bug #2064 that I linked to.

When I've had the issue, I did just fully qualify the generated Object type manually, then later changed my swagger definition to avoid the issue. Almost all of my Web API routes have ResponseType attributes, as well as Swashbuckle's SwaggerResponse. If you're using Swashbuckle, I think you'll also need both. The ResponseType attribute is how you get the type definition into Web API's ApiExplorer, which Swashbuckle uses to generate a swagger definition. The SwaggerResponse attribute allows you to document the response types for each status code.

BugRaptor commented 8 years ago

Ok, Jim. Thank you.

But like I just said this morning, with the latest Master, I now cannot even compile my generated client. I get the error CS0104: 'Object' is an ambiguous reference between 'object' and 'IO.Swagger.Model.Object'.

I tried to modify the api.mustache and ApiClient.mustache Template files by replacing each Object reference with IO.Swagger.Model.Object. The number of found compiler errors decreased but it still remains Object references in the generated Client Api source code and I don't find any other Object reference to be changed in the mustache c-sharp Template files...

wing328 commented 8 years ago

@BugRaptor As a workaround, can you rename the model "Object" to something else (e.g. DataObject) in the spec?

jimschubert commented 8 years ago

@BugRaptor sorry I wasn't clear. What @wing328 suggested is ultimately the fix. When I said "I did just fully qualify the generated Object type manually", I meant in the generated source, I had to edit the .cs files manually to the fully qualified name for Object.

If I remember correctly, I used a type alias in all of my files to make the change quickly:

using Object = IO.Swagger.Model.Object;

BugRaptor commented 8 years ago

Ok.

If I understand well, each of my controller Api operation methods must have SwaggerResponse attributes that all must include a not empty response type parameter.

So, I defined a DefaultResponse class used for each response previously declared without any response type.

All my SwaggerResponse attributes now have this form:

[SwaggerResponse(HttpStatusCode.OK, "Log succesfully stored.", typeof(DefaultResponse))]
[SwaggerResponse(HttpStatusCode.Created, "Log succesfully stored", typeof(DefaultResponse))]
[SwaggerResponse(HttpStatusCode.Accepted, "Log succesfully stored as transient", typeof(DefaultResponse))]
[SwaggerResponse(HttpStatusCode.NotFound, "Ecu not found.", typeof(DefaultResponse))]
[SwaggerResponse(HttpStatusCode.BadRequest, "Unable to store invalid log data.", typeof(DefaultResponse))]
[SwaggerResponse(HttpStatusCode.InternalServerError, "Internal server error.", typeof(DefaultResponse))]

` And the controller Api operation method returns a NegociatedContentResult< DefaultResponse > this way:

return this.Content(responseCode, new DefaultResponse { ... });

This way, my Swagger definition file does NOT contain any 'Object' references that may appear ambiguous when compiling the client.

Am I right this way ?

jimschubert commented 8 years ago

@BugRaptor It's similar to how I've resolved the issue. This actually affected us while generating the C# client today. Someone on my team messaged me, asking why operations were now returning 'void' rather than 'Object'. In our case, I recently went through and made sure all endpoints had swagger documentation and the problem occurred when I documented a couple places with just:

[SwaggerResponse(HttpStatusCode.OK)]

I recommended a similar fix to yours (returning an empty object). This is the right thing to do from an HTTP standards perspective (https://tools.ietf.org/html/rfc7231#section-6.3.1):

The 200 (OK) status code indicates that the request has succeeded. The payload sent in a 200 response depends on the request method. For the methods defined by this specification, the intended meaning of the payload can be summarized as:

GET a representation of the target resource;

HEAD the same representation as GET, but without the representation data;

POST a representation of the status of, or results obtained from, the action;

PUT, DELETE a representation of the status of the action;

OPTIONS a representation of the communications options;

TRACE a representation of the request message as received by the end server.

Aside from responses to CONNECT, a 200 response always has a payload, though an origin server MAY generate a payload body of zero length. If no payload is desired, an origin server ought to send 204 (No Content) instead. For CONNECT, no payload is allowed because the successful result is a tunnel, which begins immediately after the 200 response header section.

A 200 response is cacheable by default; i.e., unless otherwise indicated by the method definition or explicit cache controls (see Section 4.2.2 of [RFC7234]).

For a response that has no body, I think it's technically more correct to return a status 204 No Content as highlighted above. I'm doing that almost everywhere in my current project but I haven't yet tracked down where others are returning a status 200 with no body.

wing328 commented 8 years ago

@BugRaptor I've made some improvement via #2317. Would you please pull the latest master and give it another try?

cc @jimschubert

BugRaptor commented 8 years ago

@wing328 I've just pulled the latest master and tried to generate my client. I got the following exception:

C:\Git\GitHub\swagger-codegen>java -jar modules\swagger-codegen-cli\target\swagger-codegen-cli.jar generate -i http://localhost:50273/swagger/docs/v1 -l csharp -o samples\client\swagger\
reading from http://localhost:50273/swagger/docs/v1
[main] WARN io.swagger.codegen.languages.AbstractCSharpCodegen - Object (reserved word) cannot be used as model name. Renamed to ModelObject
[main] WARN io.swagger.codegen.languages.AbstractCSharpCodegen - Object (reserved word) cannot be used as model name. Renamed to ModelObject
Exception in thread "main" java.lang.RuntimeException: Could not process model 'RedHttpResponseMessage'
        at io.swagger.codegen.DefaultGenerator.generate(DefaultGenerator.java:205)
        at io.swagger.codegen.cmd.Generate.run(Generate.java:195)
        at io.swagger.codegen.SwaggerCodegen.main(SwaggerCodegen.java:36)
Caused by: java.lang.ArrayStoreException
        at java.lang.System.arraycopy(Native Method)
        at java.util.ArrayList.toArray(Unknown Source)
        at io.swagger.codegen.languages.CSharpClientCodegen.findCommonPrefixOfVars(CSharpClientCodegen.java:412)
        at io.swagger.codegen.languages.CSharpClientCodegen.postProcessModels(CSharpClientCodegen.java:311)
        at io.swagger.codegen.DefaultGenerator.processModels(DefaultGenerator.java:788)
        at io.swagger.codegen.DefaultGenerator.generate(DefaultGenerator.java:199)
        ... 2 more

I attach here after my Swagger doc file: SwaggerDocsV1.txt

BugRaptor commented 8 years ago

Just some news about my issue and long investigations I've recently made on it with my coleagues:

1) The codegen exception issue has been solved after having modified my Web api service and its swagger documentation. (Good point: The "object" type mismatch issue appears now to be solved.)

2) Nevertheless, the file publishing operation still aborts raising an AggregateException... ("One or more errors occurred", "The request was aborted") when calling the asynchronous client method.

After long investigations I didn't find the root cause of this issue.

I especially took care not to close the stream transmitted as parameter before the async call returns. Is it possible that the swagger client might not support streaming ?...

Note that the files I have to upload may be big files, so I want to publish them using a stream and not a static byte array parameter and i need asynchronous client methods to launch the Swagger web api file publishing operation...

Any fresh idea to investigate about this issue ?

BugRaptor commented 8 years ago

Actually, it seams the issue is on RestSharp. The version 105.1.0 seems not to support streaming. I didn't check if the latest RestSharp master does support it, I'll check it on monday.. (There is a pull request from august 2015 seeming to adress this issue but it was not merged becaues of no unit tests,.. )

BugRaptor commented 8 years ago

Ok, The "The Request was aborted" exception issue, raised when trying to upload a file as a stream via the C# asynchronous method of the swagger-codegen generated client is definitively located in Restsharp.

I was using the recommended version 105.1.0 but the latest release, tagged 105.2.3, does not solve the issue. (The context-length of the http request is set to zero by the client (on asynchronous method) thus the http server aborts the request when the timeout is elapsed...) A pull request #469 "adding streaming support through FileParameter" was proposed on the GitHub RestSharp project by Peter Töviskes on Nov 15th 2013 but was not accepted on August 4 2015 because the unit tests were missing.

We finally decided to use the synchronous client method when possible in our application. (The synchronous client method fully reads the stream to a byte array before handling it). For the other contexts where we need the asynchronous client uploading method, we will update our swagger-codegen mustache template files to change the generated asynchronous client code, in order to redirect it to the synchronous byte array handling code.

wing328 commented 8 years ago

https://github.com/restsharp/RestSharp/pull/769 has been merged into the master of RestSharp but I've not seen a new release yet.

Once the new release is available, we should give that a try.

Would the workaround to use version 105.1.0 be acceptable in your use case?

BugRaptor commented 8 years ago

Oh ! We didn't even notice this recent merge fixing the content length of the file parameter !

Our mustache workaround for v 105.1.0 is not terminated yet. But I think It would be better to use the next RestSharp nuget package 105.2.4 when released. In the mean time we'll check with a local alpha version issued from the latest master. If it works, we will cancel our workaround developpement and wait for the next release.

Is the other bug you mentioned fixed ? (Those that required to stay with version 105.1.0).

Thank you.

wing328 commented 8 years ago

That's the only bug that I'm aware of preventing us upgrading to the latest version of RestSharp.

BugRaptor commented 8 years ago

Ok, we just tried to generate a RestSharp v 150.2.4-alpha prerelease nuget package from the latest master.

It now works fine !

Only one Swagger-Codegen client generator update was required in the ApiClient.mustache file (line 106 in the PrepareRequest) to pass a new long parameter to the AddFile method:

request.AddFile(
    param.Value.Name,
    param.Value.Writer, 
    param.Value.FileName, 
    param.Value.ContentLength, // New parameter
    param.Value.ContentType);

The client's async method uploading a file as Stream now finally succeeds without raising the "The request was aborted" exception, and the file content is correctly transmitted to the Web API controller on the server as a Stream.

We look forward the RestSharp Version 150.2.4 official release..

Thanks for your help.

wing328 commented 8 years ago

Thanks for the confirmation. We'll keep this issue open before the official RestSharp 150.2.4 release.

Arctic-Circuits commented 5 years ago

Ok, we just tried to generate a RestSharp v 150.2.4-alpha prerelease nuget package from the latest master.

It now works fine !

Only one Swagger-Codegen client generator update was required in the ApiClient.mustache file (line 106 in the PrepareRequest) to pass a new long parameter to the AddFile method:

request.AddFile(
    param.Value.Name,
    param.Value.Writer, 
    param.Value.FileName, 
    param.Value.ContentLength, // New parameter
    param.Value.ContentType);

Is there a benefit to setting param.Value.ContentLength versus replacing the AddFile call with request.Files.Add(param.Value);?

KutanaDev commented 3 years ago

The swagger-generated code contains a call to a four-argument version of AddFile

request.AddFile(
    param.Value.Name,
    param.Value.Writer, 
    param.Value.FileName, 
    param.Value.ContentType);

This compiles OK using RestSharp 105.

However, if I try to build using RestSharp 106, compilation fails because RestSharp 106 does not contain a four-argument AddFile method.

I'm not really happy to hand-edit the generated code. Are there plans to make the generated code compatible with RestSharp 106?