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

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

Open
BugRaptor opened this issue Feb 18, 2016 · 24 comments

Comments

@BugRaptor
Copy link

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
Copy link
Contributor

wing328 commented Feb 18, 2016

I would suggest you to try 2 things:

@wing328 wing328 added this to the v2.1.6 milestone Feb 18, 2016
@BugRaptor
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Contributor

wing328 commented Feb 22, 2016

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

@jimschubert
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Contributor

wing328 commented Mar 5, 2016

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

cc @jimschubert

@BugRaptor
Copy link
Author

@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
Copy link
Author

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.

  • Actually, the synchronous implementatiom of the client's publishing method now works fine, only the asynchronous publishing method raises the exception when called.
  • Note that the client's file publishing method receives a Stream as parameter (the content of the file to upload on the server).

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

wing328 commented Mar 14, 2016

restsharp/RestSharp#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
Copy link
Author

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
Copy link
Contributor

wing328 commented Mar 15, 2016

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

@BugRaptor
Copy link
Author

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
Copy link
Contributor

wing328 commented Mar 15, 2016

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

@Arctic-Circuits
Copy link

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
Copy link

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?

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