[csharp] fix: ApiResponse Header values #6840
Conversation
…has the correct values
faryu
changed the title
| @@ -33,7 +33,7 @@ namespace {{packageName}}.Client | |||
| /// <param name="statusCode">HTTP status code.</param> | |||
| /// <param name="headers">HTTP headers.</param> | |||
| /// <param name="data">Data (parsed HTTP body)</param> | |||
| public ApiResponse(int statusCode, IDictionary<string, string> headers, T data) | |||
| public ApiResponse(int statusCode, IDictionary<string, string[]> headers, T data) | |||
There was a problem hiding this comment.
NOTE: Because this generator isn't currently intended to be regenerated over existing code, I wouldn't consider this a breaking change.
There was a problem hiding this comment.
n/m. I was looking at this wrong. This is a breaking change.
This should probably be an overloaded constructor which converts the values to a comma separated value per RFC 2616
There was a problem hiding this comment.
I just fixed it. It's no longer a breaking change for generated netframework clients.
It is a "breaking" (didn't work previously) change for netstandard clients, but they never received the correct values, so I think this is acceptable.
| @@ -297,12 +297,12 @@ namespace {{packageName}}.{{apiPackage}} | |||
|
|
|||
| {{#returnType}} | |||
| return new ApiResponse<{{{returnType}}}>(localVarStatusCode, | |||
| localVarResponse.Headers.ToDictionary(x => x.{{^netStandard}}Name{{/netStandard}}{{#netStandard}}Key{{/netStandard}}, x => x.Value.ToString()), | |||
| localVarResponse.Headers.ToDictionary(x => x.{{^netStandard}}Name{{/netStandard}}{{#netStandard}}Key{{/netStandard}}, x => x.Value.ToArray()), | |||
There was a problem hiding this comment.
iirc, this ToArray() won't work in .NET 3.5/4.x. Can you regenerate samples and test both the ^netStandard and #netStandard branches here?
There was a problem hiding this comment.
Thank you for your quick response!
I will take a look at the earlier .NET versions.
|
Now it doesn't break the build of any netframework project. I digged a bit deeper to locate the cause of the bug and found, that the different RestSharp versions where the cause. RestSharp.portable has a list of values, whereas RestSharp has a combined string. Now I moved the RestSharp version dependent code into the ApiResponse class. netstandard / netcore libraries have In order to normalize the ApiResponse class either the combined value needs to be splitt or the list of values be combined. I don't know all possible ways of combining http header values (which delimiters are allowed, how are single values escaped if they contain the delimiter, etc.), which makes converting them from one format to another complicated for myself, so I just keep the values as they are. |
|
@faryu Thanks, now that you say the issue that is correctly what I remembered. I couldn't remember the details, just that there is a wonky difference between the two. Ideally, I'd like to invest some time into removing RestSharp, but I haven't had much OSS contribution time for a few months. The point about the constructor was regarding existing code which may be using the ApiClient directly. I'm ok with your change, since it will affect less consumers. And if it has a negative impact on more than anticipated, we can add the additional constructor. The reason I don't consider it a huge deal is because consumers could just as well modify the csharp templates and add the constructor. @wing328 thoughts on that? |
The Header values within the ApiResponse class where all "System.String[]". (The string, not a real string array)
With this fix it has the correct header values.
Also added an option to the petstore example in order to use the correct csproj format for the netstandard project generation.