RicoSuter / NSwag

The Swagger/OpenAPI toolchain for .NET, ASP.NET Core and TypeScript.
http://NSwag.org
MIT License
6.79k stars 1.29k forks source link

nswag v13 can't handle text\plain and string response #2384

Open antonGritsenko opened 5 years ago

antonGritsenko commented 5 years ago

This is very old issue and it was fixed in v12, but appears again in v13: #701

Controller's attributes:

[Produces("text/plain")]
[ProducesResponseType(typeof(FileContentResult), (int)HttpStatusCode.OK)]
[ProducesResponseType(typeof(string), (int)HttpStatusCode.OK)]
[ProducesResponseType(typeof(ErrorResponseMessage), (int)HttpStatusCode.BadRequest)]
[ProducesResponseType(typeof(ErrorResponseMessage), (int)HttpStatusCode.NotFound)]

Swagger:

"get": {
        "tags": [ "Binary" ],
        "summary": "Download binary as base64 string.",
        "operationId": "DownloadBinaryAsBase64",
        "consumes": [],
        "produces": [ "text/plain" ],
        "parameters": [
          {
            "name": "binaryId",
            "in": "path",
            "description": "Id of binary to fetch.",
            "required": true,
            "type": "string",
            "format": "guid"
          }
        ],
        "responses": {
          "200": {
            "description": "Content downloaded.",
            "schema": { "type": "string" }
          },
          "400": {
            "description": "Request contains wrong data.",
            "schema": { "$ref": "#/definitions/ErrorResponseMessage" }
          },
          "404": {
            "description": "Not Found",
            "schema": { "$ref": "#/definitions/ErrorResponseMessage" }
          }
        }

Produced code (part):

var status_ = ((int)response_.StatusCode).ToString();
if (status_ == "200") 
{
    var objectResponse_ = await ReadObjectResponseAsync<string>(response_, headers_).ConfigureAwait(false);
    return objectResponse_.Object;
}

ReadObjectResponseAsync always try to deserialize the string and of course it will fail.

Revert to v12.2.5 fix the issue.

et1975 commented 5 years ago

https://github.com/RicoSuter/NSwag/pull/1976/files#diff-4cbdc640407fc6a649e91bcc43983584L17 :(

RicoSuter commented 4 years ago

So its a regression of this pr? https://github.com/RicoSuter/NSwag/pull/1976

/cc @Jehoel

daiplusplus commented 4 years ago

@RicoSuter Did V12.2.5 specifically support text/plain or was it supported by coincidence? I note that there isn't a check like {% if response.IsPlainText %} in the .liquid templates to handle this case. I remember my changes were only to the "is expecting a JSON object response" code-path.

et1975 commented 4 years ago

@Jehoel text/plain support was specifically added.

daiplusplus commented 4 years ago

@et1975 ah, thank you - I overlooked that.

I’ll work on a patch right away.

daiplusplus commented 4 years ago

Here's my patch: https://github.com/RicoSuter/NSwag/pull/2587

I'd appreciate it if you can test and verify it @et1975 as I don't have any Swagger files to-hand with text/plain responses.

tmort93 commented 4 years ago

Any idea when this patch will be released?

FrancoisCamus commented 4 years ago

I had to rollback to 13.2.0.0 to make it work

ghost commented 4 years ago

Thanks @FrancoisCamus. Rolling back to 13.2.0 "fixed" the problem for me.

jeremyVignelles commented 4 years ago

The patch has been merged and was released as the 13.2.0 it seems. Do you mean that newer versions are broken ? Is that really the same issue ?

ghost commented 4 years ago

Yes it seems like the same bug for me and that at least 13.6.2 is broken. I recerated the code again with 13.6.2 and 13.2.0 to compare it. In 13.6.2 the client code tries to parse the string with Newtonsoft.Json what leads to an exception.

My swagger.yaml looks like this

swagger: "2.0"
info:
  description: "This is a REST server via which data can be queried by the TMS."
  version: "1.0.0"
  title: "REST API for TMS"
host: localhost:8081 # TODO replace with docker parameter for base url
schemes:
  - http # TODO use protocol from docker parameter base url
basePath: "/"
paths:
...
  /topic/{topicId}/subject/de:
    get:
      summary: "Get subject by the topicId"
      operationId: "getDeSubjectByTopicId"
      tags:
        - Topic
      description: "Returns the subject from the passed topic id."
      produces:
      - "text/plain"
      parameters:
      - name: "topicId"
        in: "path"
        description: "ID of the topic"
        required: true
        type: "integer"
        format: "int32"
      responses:
        "200":
          description: "successful operation"
          schema:
            type: string
            example: subject
        "400":
          description: "Invalid ID supplied"
        "404":
          description: "Topic not found"
      security:
      - api_key: []
...

C# generated with 13.2.0:

public async System.Threading.Tasks.Task<string> GetDeSubjectByTopicIdAsync(int topicId, System.Threading.CancellationToken cancellationToken)
{
   ...
   if (status_ == "200") 
   {
      var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);
      var result_ = (string)System.Convert.ChangeType(responseData_, typeof(string));
      return result_;
   }
   ...
}

C# generated with 13.6.2:

public async System.Threading.Tasks.Task<string> GetDeSubjectByTopicIdAsync(int topicId, System.Threading.CancellationToken cancellationToken)
{
   ...
   if (status_ == "200") 
   {
      var objectResponse_ = await ReadObjectResponseAsync<string>(response_, headers_).ConfigureAwait(false);
      return objectResponse_.Object;
   }
   ...
}

protected virtual async System.Threading.Tasks.Task<ObjectResponseResult<T>> ReadObjectResponseAsync<T>(System.Net.Http.HttpResponseMessage response, System.Collections.Generic.IReadOnlyDictionary<string, System.Collections.Generic.IEnumerable<string>> headers)
{
    if (response == null || response.Content == null)
    {
        return new ObjectResponseResult<T>(default(T), string.Empty);
    }

    if (ReadResponseAsString)
    {
        var responseText = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
        try
        {
            var typedBody = Newtonsoft.Json.JsonConvert.DeserializeObject<T>(responseText, JsonSerializerSettings);
            return new ObjectResponseResult<T>(typedBody, responseText);
        }
        catch (Newtonsoft.Json.JsonException exception)
        {
            var message = "Could not deserialize the response body string as " + typeof(T).FullName + ".";
            throw new ApiException(message, (int)response.StatusCode, responseText, headers, exception);
        }
    }
    else
    {
        try
        {
            using (var responseStream = await response.Content.ReadAsStreamAsync().ConfigureAwait(false))
            using (var streamReader = new System.IO.StreamReader(responseStream))
            using (var jsonTextReader = new Newtonsoft.Json.JsonTextReader(streamReader))
            {
                var serializer = Newtonsoft.Json.JsonSerializer.Create(JsonSerializerSettings);
                var typedBody = serializer.Deserialize<T>(jsonTextReader);
                return new ObjectResponseResult<T>(typedBody, string.Empty);
            }
        }
        catch (Newtonsoft.Json.JsonException exception)
        {
            var message = "Could not deserialize the response body stream as " + typeof(T).FullName + ".";
            throw new ApiException(message, (int)response.StatusCode, string.Empty, headers, exception);
        }
    }
}
daiplusplus commented 4 years ago

@ZukenHammer as far as I know my patch (that's merged) shouldn't cause this...

Can you tweak your .liquid templates on your local machine (and add C# comments) so we can see what's going on?

ghost commented 4 years ago

I installed swagger by npm and never read the .liquid templates or even seen them. I can try to build a minified sample project and share it with this error.

jeremyVignelles commented 4 years ago

You can download templates from this repo inside a directory, and specify them in the nswag config : https://github.com/RicoSuter/NSwag/wiki/Templates

That way, you can change the template to try things.

RicoSuter commented 4 years ago

Please test with the latest version of NSwag, this has been changed a bit and might solve your problem.

FuturistiCoder commented 4 years ago

I tried with 13.9.3. I have "produces": ["text/plain"] in my swagger.json 2.0 I still got generated code like this:

if (status_ == 200)
                        {
                            var objectResponse_ = await ReadObjectResponseAsync<string>(response_, headers_).ConfigureAwait(false);
                            if (objectResponse_.Object == null)
                            {
                                throw new ApiException("Response was null which was not expected.", status_, objectResponse_.Text, headers_, null);
                            }
                            return objectResponse_.Object;
                        }

await ReadObjectResponseAsync<string>(....) will cause the problem.

svickers commented 3 years ago

As a workaround for this bug I set the swagger response schema to be file, and then just read the string from the returned stream.

"200": {
  "description": "Query Result",
  "schema": {
    "type": "file"
  }
}
  var results = await _client.RunInlineQueryAsync(query, limit, formatResults, userToken);

  using(var sr = new StreamReader(results.Stream))
  {
      var plainString = sr.ReadToEnd();
      return plainString;
  }
Fredolx commented 3 years ago

Still a bug while using Visual Studio OpenAPI Connected Services

Fredolx commented 3 years ago

Should I try working on this issue? Or is someone else already on it?

icnocop commented 3 years ago

Please try again with NSwag v13.12.0, as I believe it was fixed in PR https://github.com/RicoSuter/NSwag/pull/3535.

Thank you.

GregaMohorko commented 3 years ago

I have tried with NSwag v13.12.0, and the generated result is still the same as @ZukenHammer and @FuturistiCoder said.

erikbozic commented 3 years ago

Is the problem here? https://github.com/RicoSuter/NSwag/blob/86735476b1e971a69b22a01179d676736e65a904/src/NSwag.Core/OpenApiResponse.cs#L76-L89

As far as I can tell the Content is only ever set here. And It's either "application/octet-stream" or "application/json". So then the liquid template never gets response.IsPlaintText true.

https://github.com/RicoSuter/NSwag/blob/86735476b1e971a69b22a01179d676736e65a904/src/NSwag.CodeGeneration.CSharp/Templates/Client.Class.ProcessResponse.liquid#L12-L24

https://github.com/RicoSuter/NSwag/blob/86735476b1e971a69b22a01179d676736e65a904/src/NSwag.CodeGeneration/Models/ResponseModelBase.cs#L88-L95

The issue started with this commit: https://github.com/RicoSuter/NSwag/commit/4fbc0ccf65750f6e5d36abb0218da0862b6b96c7 Since Content is always "application/json" as shown above, the change in this commit doesn't work as intended probably.

shawty commented 3 years ago

What's happening with this one folks, I've just hit this issue with version 13.12 of NSwag studio:

image

My generated client (With no modifications to any templates or anything) generates an object read for 404 errors:

image

When my controller states, that on a 404 the response is a string:

image

And thus in my code calling the client, I get the dreaded Json exception of not being able to deserialize the string to an object, which means I get an exception different to what I'm trying to handle.

Any updates?

RicoSuter commented 3 years ago

@shawty can you post the generated OpenAPI spec for the "Render" operation?

shawty commented 3 years ago

Hi @RicoSuter , yea sure.... here you go

image

Had to reduce the font a little to make it fit on the screen...

Last bottom little bit is just the dispose calls

image

shawty commented 3 years ago

@RicoSuter for reference, this is what I get thrown back at me:

image

shawty commented 3 years ago

I can snap shot the response from Fiddler if you want, but I know what the response looks like, it's not JSON, it's definitely a 404 code with just a single line of UTF8 text

RicoSuter commented 3 years ago

Can you post the OpenAPI spec (JSON), not the generated code?

shawty commented 3 years ago

That might be a bit of a problem :-)

I generate the client code direct into my project using NSwag, I don't save or make a copy of the open API spec code.

Scrap that comment :-)

image

I screen shotted NSwag.

daiplusplus commented 3 years ago

@shawty Please post text, not screenshots - we can't search nor copy+paste code and data from screenshots: https://meta.stackoverflow.com/questions/303812/discourage-screenshots-of-code-and-or-errors


As for your problem:

If you're returning plain-text (as in text/plain) in your 404 responses then your [ProducesResponseType] attributes are wrong because typeof(string) combined with Content-Type set to application/json means that your action will return JSON string for 404 - not that it will return a text/plain response. Rephrased: The String type does not correspond to any particular HTTP response Content-Type header value.

Also, application/json is also inappropriate and incorrect for FileStreamResult - but that didn't cause any problems so far because FileStreamResult sets its own response Content-Type header anyway). You'll notice that the OpenAPI JSON does not list application/octet-stream for 200 responses, even though it should.

Unfortunately (and annoyingly) the [ProducesResponseType] does not support specifying an explicit Content-Type value for per-status-code responses. The workaround you'll need to customize the generated OpenAPI to change the Content-Type for those error responses:

(The below code is just an example, I haven't compiled nor tested it).

// In your `ConfigureServices(IServiceCollection services)` method, change your `AddSwaggerDocument` to include the following:

services.AddSwaggerDocument( ( AspNetCoreOpenApiDocumentGeneratorSettings config ) =>
{
    config.PostProcess = ( OpenApiDocument document ) =>
    {
        foreach( OpenApiPathItem path in document.Paths )
        {
            foreach( OpenApiOperation op in path.Values )
            {
                foreach( OpenApiResponse errorResponse in op.Responses.Where( kvp => kvp.Key.StartsWith("4") || kvp.Key.StartsWith("5") ) )
                {
                    if( !errorResponse.Content.ContainsKey( "text/plain" ) )
                    {
                        errorResponse.Content.Add( "text/plain", new OpenApiMediaType( ... ) );
                    }
                }
            }
        }
    };
});
shawty commented 3 years ago

Hi,

If you're returning plain-text (as in text/plain) in your 404 responses then your [ProducesResponseType] attributes are wrong because typeof(string) combined with Content-Type set to application/json means that your action will return JSON string for 404 - not that it will return a text/plain response. Rephrased: The String type does not correspond to any particular HTTP response Content-Type header value.

My web API is actually returning a string, NOT a JSON string, I have verified this using fiddler. The exchange between the rest endpoint and my client, is clearly and physically a 404 with a plain string and no Json Formatting (Exactly what I expected it to return based on how I've set the controller up)

As for the 'AddSwaggerDocument' call I'm assuming you mean my configuration in the webapi that produces the OpenAPI spec that's causing me the problem? If so, thanks, I'll give that a try.

I have actually patched a workaround into the problem for now by actually forcing my WebAPI to ALWAYS return Json (It was not doing this before, which as I said was expected... That was what I wanted it to do) , this keeps the C# client code generated by NSwag happy, because it now has a Json Object to decode, and so does not throw an exception when trying to pass a plain NON Json string to newton soft Json (Which is what it was trying to do when I stumbled across this)

Cheers Shawty

daiplusplus commented 3 years ago

@shawty

My web API is actually returning a string, NOT a JSON string, I have verified this using fiddler.

That's my point.

You should not be using [ProducesResponseType( typeof(String) )] to indicate text/plain.

I'm assuming you mean my configuration in the webapi that produces the OpenAPI spec that's causing me the problem? If so, thanks, I'll give that a try.

The problem isn't OpenAPI, the problem is that your controller action has the wrong documentation attributes,

shawty commented 3 years ago

@Jehoel ok I get that, so how do I make it return a string, I guess by the code you suggested... which I'm going to try.

I don't actually want a Json response, what I want in my client is to actually get an exception for not found, or bad request as I specifically handle them in my code, but I've yet to figure out how I can do that.

diogocp commented 3 years ago

Please have a look at the minimal working example here: https://github.com/diogocp/NSwag/tree/bug/plain-text-response

If you look at the generated code, you can see that it:

  1. sets an Accept: text/plain header on the request;
  2. proceeds to unconditionally try to deserialize the response as if it was JSON.
shawty commented 3 years ago

@diogocp will do, but it will be next week now, the work round I have is in place and working fine, so have been asked to do a roll out to staging over the next few days.

diogocp commented 3 years ago

@shawty my comment was not directed at you, but at @RicoSuter and anyone else interested in reproducing the issue with client generation.

Your problem seems to be a rather different one, since your OpenAPI document doesn't even have a text/plain response.

shawty commented 3 years ago

@diogocp - no worries, i was half asleep last night when I read the email, and I made the connection with your previous reply ... :-)

diogocp commented 3 years ago

The root cause seems to be the inconsistency between these two:

NSwag.CodeGeneration.CSharp/Templates/Client.Class.liquid#L282

NSwag.CodeGeneration/Models/ResponseModelBase.cs#L92

CaiusJard commented 3 years ago

I too just ran into this; I think @diogocp is right in that there is some minor discrepancy between Produces and IsPlaintText

Produces grabs the first content type and returns it

IsPlainText examines all content types and requires that text/plain be present and application/json be absent

In my mind this creates a problem if the Content collection happens to be (as they were for me) in { "text/plain", "application/json" } order. The request was going out with Accept:text/plain because Produces said it was plain, and then when the response came back as plain text (an unquoted string) from .NET core API (because after all that is what the request asked for!) IsPlainText was false because there is an app/json content in there somewhere (not in first position though) so it was being put to the json deser even though it was an unquoted string

For me, I just modified IsPlainText so it looks like:

bool IsPlainText { get => Produces == "text/plain"; }

but other than a cursory test that "it's got me over that hump" I haven't verified that it works well everywhere in my API client. It does appear that IsPlainText is only used in one liquid template, and also Produces is in one template so I think it might be safe enough - probably a better solution would be to consistently use one or the other and remove the unused one from the codebase:

I decided not to touch the liquid templates and, because whatever the first Content is becomes what is put down as the Accept, I reckoned that decisioning the response based on the first Content might be reasonable. If I get any related issues I'll post back..

Edit: relatively short lived "victory" :) - it got me over the problem of my "returns a string" controller method, but I ran into another, bigger problem in that pretty much all the controller methods were declared to be capable of returning a plain string (seems .NET API controllers have a StringFormatter enabled by default, causing every openapi returns-Content suite to contain a text/plain entry. If you set an Accept header of text/plain the API ignores it and sends you json (with application/json) anyway if the controller returns an object..
..but the workaround above causes NSwag to ask for plain and interpret plain (i.e. it expects it'll get it if it's advertised) which doesn't work out for a controller method that returns a complex object

I'm split on what to do; primarily the net API seems to be wrong- i think it shouldn't advertize Content types it isn't prepared to emit.. and it can be reconfigured in various ways to remove the advert..

..but equally perhaps the "use plain text if available" sway I've put into NSwag with the above change should be changed to more like "use plain text as a last resort/prefer application-json whenever it's available"..

..but then for those methods where they really do just eg return a string, going to the extra effort of saying "accept json" and having the api produce json and then having the client deser it when it's just a string seems like such a waste..

CaiusJard commented 3 years ago

Perhaps all this is a result of an apparent lack of dynamism on the generated code's behalf, and it should be saying "accept a or b" then examining what the response header says is in the message, a vs b, and acting based on that rather than a "swagger says a or b is available, ask for a, expect a" - this latter approach is less "be strict in what you send and liberal in what you receive" than the former

riccardominato commented 2 years ago

Bug still present in version 13.15.10.

rogerex commented 2 years ago

npm nswag 13.15.10 works for me

jamesthurley commented 2 years ago

I'm getting this issue in 13.15.10 also. It is setting the accept header to text/plain then trying to parse the result as if it had set application/json.

daiplusplus commented 2 years ago

@jamesthurley Exactly what [Produces], [ProducesResponseType] and [Consumes] attributes do you have on your route/controller/actions? Can you share a sample repro?

diogocp commented 2 years ago

I shared an example in https://github.com/RicoSuter/NSwag/issues/2384#issuecomment-897119822

daiplusplus commented 2 years ago

@diogocp I shared an example in #2384 (comment)

Your project uses <OpenApiProjectReference /> for build-time codegen of the actual C# client code, so I can't actually see the buggy client code in your repo - so can you please add a hard-copy of the bad generated code to your repo?

daiplusplus commented 2 years ago

Also, is the issue still reproducible with the latest versions 13.16.1 and later? So far everyone's only been saying 13.15 is affected.

shawty commented 2 years ago

Since I initially posted this report, the project I was working on where I encountered it has ended, I didn't get chance to snag a copy of the code for testing, nor did I get chance to follow any of it up, so I don't know if this is still present in any of my code base. If I get chance however, I will see if I can put together a repo to test.

diogocp commented 2 years ago

can you please add a hard-copy of the bad generated code to your repo?

Done.

Also, is the issue still reproducible with the latest versions 13.16.1 and later?

Yes, tested with 13.16.1.

MYriad1993 commented 2 years ago

I had the same issue after I had switched to NSwag from Swashbuckle. And all I had to do is to remove the attribute [Produces("text/plain")]. It hadn't worked in Swashbuckle without it. The method still returns content-type: application/json and the client is generated with ReadObjectResponseAsync. But the property ReadResponseAsString which is used in this method is set to true at runtime and not to false as it had been.