-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
Proxy forms without relying on the request body #61
Conversation
For some reason .Net Core quotes the boundary, but objects to being given a quoted boundary.
This is awesome: thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, would like some comments.
@@ -67,5 +72,35 @@ internal static string TrimTrailingSlashes(this string s) | |||
|
|||
return s.Substring(0, s.Length - count); | |||
} | |||
|
|||
internal static HttpContent ToHttpContent(this IFormCollection collection, string contentType) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add just a few comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I hope they're what you're after. I've also made a few other changes, including at least one that was a fix.
Unfortunately that for some reason has created a code coverage issue for the invalid content type path, and said path is impossible to hit due to how this is used...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine. I can write unit test to wriggle in there.
src/Core/Helpers/Helpers.cs
Outdated
if (!contentType.StartsWith("multipart/form-data;", StringComparison.OrdinalIgnoreCase)) | ||
throw new Exception($"Unknown form content type \"{contentType[0]}\""); | ||
|
||
const string boundary = "boundary="; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, a comment with a link to how this boundary works would be cool. 😄
This allows avoiding string manipulation to get a multipart boundary, and also ensures no non-interesting parameters interefere with matching. I've removed the check for an empty content type, as it is unnecessary. Also added comments.
Codecov Report
@@ Coverage Diff @@
## master #61 +/- ##
===========================================
- Coverage 100.00% 99.84% -0.16%
===========================================
Files 12 12
Lines 628 658 +30
===========================================
+ Hits 628 657 +29
- Misses 0 1 +1
Continue to review full report at Codecov.
|
@PreferLinux, didn't think it needed a PR... |
Looks good! |
Fixes #57 and replaced #60.
For requests with a form content type .Net seems to consume the request body if model binding / method parameters are present on a controller, even if they should be using route or query string values. Avoid the problem by re-creating the request body from the form data.