Fix for manual JSON body content parameter #1817
Conversation
kendallb
force-pushed
the
feature/manual-body-content-1816
branch
2 times, most recently
from
0854302 to
7d10290
Compare
kendallb
force-pushed
the
feature/manual-body-content-1816
branch
from
7d10290 to
8e27df0
Compare
| ParameterType.UrlSegment => new UrlSegmentParameter(name!, value?.ToString()!, encode), | ||
| ParameterType.HttpHeader => new HeaderParameter(name, value?.ToString()), | ||
| ParameterType.RequestBody => new BodyParameter(name, value!, Serializers.ContentType.Plain), | ||
| ParameterType.RequestBody => new BodyParameter("", value!, string.IsNullOrEmpty(name) ? Serializers.ContentType.Plain : name!), |
There was a problem hiding this comment.
That will break things. Unlike RS 106, the latest version allows you to add multiple body parameters. It works with multipart form data. There, parameters must have names, also body parameters.
There was a problem hiding this comment.
Yeah, not sure I like the fix either but so much code does it that way :(
There was a problem hiding this comment.
The question is: do we want to make it compatible or force people to use the right API. In the latter case, we can throw an exception if the parameter name looks like content type, and the exception would say: "it won't work, use AddStringBody". I can add an overload for AddStringBody that would accept the parameter name.
There was a problem hiding this comment.
That’s a good idea. For us it took a long time to figure out what was wrong as I had to inspect the outgoing packets to finally work out it was not sent with the right content type. A logical exception explaining what is wrong would save those folks with the same problem a lot of time.
|
I made a similar change in #1869 and kept the ability to add named body parameters. The check looks like a hack as it's based on looking for a slash in the parameter name. Hope it will solve some of the issues. I got a bit tired of all the complaints, and, honestly, until SwaggerGen and Postman code generators get fixed (which I don't expect to happen soon), these complaints will keep copming. |
|
Checked #1869 and it looks like a good solution for me. Deleting my branch. |
|
Btw, Postman is being updated, they asked me to review the new generator version. |
Description
Fix for #1816
Purpose
This pull request is a:
Checklist