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

Reworked C# client response handling #2959

Merged
merged 7 commits into from
Aug 3, 2020

Conversation

jeremyVignelles
Copy link
Collaborator

@jeremyVignelles jeremyVignelles commented Jul 17, 2020

This PR reworks how the response parsing works in the generated C# client:

Before:

var status_ = ((int)response_.StatusCode).ToString();
if (status_ == "200") 
{
    var objectResponse_ = await ReadObjectResponseAsync<ApiModel>(response_, headers_).ConfigureAwait(false);
    return objectResponse_.Object;
}
else
if (status_ != "200" && status_ != "204")
{
    var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false); 
    throw new ApiException("The HTTP status code of the response was not expected (" + (int)response_.StatusCode + ").", (int)response_.StatusCode, responseData_, headers_, null);
}

return default(ApiModel)!

The status code 200 was controlled twice, and default(ApiModel)! was returned while a null was not expected (#2944 )

The new code looks like this:

var status_ = (int)response_.StatusCode;
if (status_ == 200)
{
    var objectResponse_ = await ReadObjectResponseAsync<ApiModel>(response_, headers_).ConfigureAwait(false);
    return objectResponse_.Object;
}
else
{
    var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false); 
    throw new ApiException("The HTTP status code of the response was not expected (" + status_ + ").", status_, responseData_, headers_, null);
}

now, the else is the default case where the "unexpected HTTP status code" exception is thrown. There is no need to return default(T)!; anymore.

I tried to be careful not to break things, and as far as I know, there is only one thing that might break:
If there is an explicit success status code declared in the openApi definition, the else if (status_ == 200 || status_ == 204) that returns a default(T)! (or a wrapped instance) is not written anymore, and it will fall in the default else case.
In that case where a 200 is declared, but the server returns a 204 (undeclared), the code will no longer return null but will throw an "unexpected HTTP status code". I'd argue that this is cleaner and, in fact, expected : the fix is to declare the 204 on the server side.

{% else -%}
return result_;
{% endif -%}
{% endif -%}
{% else -%}
var objectResponse_ = await ReadObjectResponseAsync<{{ response.Type }}>(response_, headers_).ConfigureAwait(false);
{% if response.IsNullable == false -%}
if (objectResponse_.Object == null)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixes the second part of #2944

{% else -%}
return objectResponse_.Object;
{% endif -%}
{% endif -%}
{% if response.IsSuccess == false -%}
{% if response.InheritsExceptionSchema -%}
var responseObject_ = objectResponse_.Object != null ? objectResponse_.Object : new {{ response.Type }}();
responseObject_.Data.Add("HttpStatus", status_);
responseObject_.Data.Add("HttpStatus", status_.ToString());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's the only place where a string is needed (admittedly, I didn't even test without the ToString()). All other places should work fine (if not better) with ints.

{% if operation.HasDefaultResponse == false or (operation.DefaultResponse.HasType == false and operation.HasSuccessResponse == false) -%}
{% if operation.HasResultType -%}

{% elseif operation.HasResultType -%}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I admit that this is quite hard to review with the diff tools.
I took the part inside if operation.HasDefaultResponse == false or (operation.DefaultResponse.HasType == false and operation.HasSuccessResponse == false) and moved that to the two separate places where it was needed.

This duplicates some code, but in the end, I find it clearer and this allows the generated code to be cleaner.

Then, see below for the second major change

Otherwise, return default values on success because we don't want to throw on "unknown status code".
Success is always expected
{% endcomment -%}
if (status_ == 200 || status_ == 204)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logic has been inverted there, and the second code is pasted in here. (and only enabled if no success code has been handled yet)
The unexpected throw is in the final else.

@@ -74,7 +93,11 @@ namespace {{ Namespace }}
get { return StatusCode == 206; }
}

{% if GenerateNullableReferenceTypes -%}
public FileResponse(int statusCode, System.Collections.Generic.IReadOnlyDictionary<string, System.Collections.Generic.IEnumerable<string>> headers, System.IO.Stream stream, System.IDisposable? client, System.IDisposable? response)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Stream is not nullable since a NullStream is given here upstream.

@RicoSuter
Copy link
Owner

Just checked with some projects, lgtm.
Thank you very much for the PR and sorry for the delay (holidays).

@RicoSuter RicoSuter merged commit 3787156 into RicoSuter:master Aug 3, 2020
RicoSuter added a commit that referenced this pull request Aug 3, 2020
* Add support for using controllers' summary as tag description (#2949)

* Add support for using controllers' summary as tag description

* Ensure tag only added to document if there's description

* Cleanup

* Move code

Co-authored-by: Rico Suter <mail@rsuter.com>

* Fix contract, closes #2933

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* Bump elliptic from 6.4.0 to 6.5.3 in /src/NSwag.Sample.NetCoreAngular (#2968)

Bumps [elliptic](https://github.com/indutny/elliptic) from 6.4.0 to 6.5.3.
- [Release notes](https://github.com/indutny/elliptic/releases)
- [Commits](indutny/elliptic@v6.4.0...v6.5.3)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix for File Parameter interface not generated in some instances (#2972)

- Split the Binary tests into 3 so each endpoint can be generated properly without missing the FileParameter interface
- changed RequiresFileParameterInterface logic so it checks isBinary on arrays and nested objects

Co-authored-by: Craig Ngu <Craig.Ngu@globalx.com.au>

* Correct link in markdown for MS Build Targets (#2958)

* Aligned OpenAPI code generation for file more with open API specification (#2896)

* Reworked C# client response handling (#2959)

* Reworked C# client response handling

* Disallowed null responses if the response type is not declared as nullable

Fixed the remark in #2944

* Fixed nullability on File I/O

* Fixed nullability on ConvertToString

* Fixed PR remarks

* Avoid nullability issues in Query parameters

Addresses RicoSuter/NJsonSchema#1069 (comment)

* Fixed invalid code if array value type is not string

* v13.7.0 (#2978)

* v13.7.0

* Fix test

Co-authored-by: Leon V <leon99@outlook.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: craigngu <42369784+craigngu@users.noreply.github.com>
Co-authored-by: Craig Ngu <Craig.Ngu@globalx.com.au>
Co-authored-by: Jedidiah May <33727409+portlandtn@users.noreply.github.com>
Co-authored-by: Nicolas Fløysvik <nico-floysvik@hotmail.com>
Co-authored-by: Jérémy VIGNELLES <jeremyVignelles@users.noreply.github.com>
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.

C# client code returns default(T)! while result is not nullable
2 participants