-
-
Notifications
You must be signed in to change notification settings - Fork 214
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
Fix new Delete with body missing from IWireMockAdminApi interface #413
Fix new Delete with body missing from IWireMockAdminApi interface #413
Conversation
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.
You need to add the code changes in that 1 file from PR #409 because this was also reverted.
I didn't realize that was reverted too. I resubmitted the code. |
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.
This change does not work correctly, because the body is not parsed in this line:
if (request.Body != null && BodyParser.ShouldParseBody(method, options.AllowBodyForAllHttpMethods == true)) |
So maybe skip this PR or change it so that your just provide all GUID's in the query string?
I'm not sure what you mean by not working correctly? I tested it and it works fine... as long as Also, before opening this PR I looked into whether DELETE can have body semantics and found this documentation saying it may have a body. Am I missing something? |
And also in the RFC, it is not forbidden. It only says that |
Currently, the logic form WireMock is that DELETE cannot have a body:
Because of this logic, all requests (normal + admin) will not allow a body for delete. This behaviour can indeed be changed by AllowBodyForAllHttpMethods, however this settings is actually meant for normal requests, for admin requests, this should actually not be applied (for admin requests: all is allowed) So a fix could be:
|
Then I think that this should be the fix since the RFC indicates that body is allowed for DELETE. |
You also need to update some other unit-tests because of this update. |
Yes I see that now. Thanks I'll get to it soon. |
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.
please take a look at some 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.
see remark about [Theory]
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.
thank you very much
As requested.