-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Use body of delete #49
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.
made one comment, apologies for not thinking of this earlier, but maybe this is a much simpler solution
@@ -169,7 +169,7 @@ private T buildRequestInternal(HttpRequest request, ScriptContext context) { | |||
buildCookie(cookie); | |||
} | |||
} | |||
if ("POST".equals(method) || "PUT".equals(method) || "PATCH".equals(method)) { | |||
if ("POST".equals(method) || "PUT".equals(method) || "PATCH".equals(method) || "DELETE".equals(method)) { |
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.
sorry to think of this a little late - maybe we can simplify the logic. so if body == null or isNull, just return null.
else, no matter what the http method is, just do the other logic. we can completely avoid any check for "POST" "PUT" etc.
can you try this, and if all tests are green - we can do it this way then.
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.
Hi Peter,
Sorry but I have some questions here.
As I understand it, you are suggesting that we return null when body is null/isNull without throwing any exception. If we do that, it makes body optional for all HTTP methods (including POST, PUT, PATCH and DELETE). Is my understanding correct? If yes, in my opinion POST, PUT or PATCH requests that do NOT have a body does not make any sense. Do we really want to do this?
l agree that in the Else case (body not null/isNull) we should just call getEntityInternal(body, mediaType);
Thanks,
Satish
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 are right. for the time being I'll approve the pul request and tweak more if needed.
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.
one more request can you change the target branch to intuit:develop (and not master)
I was unable to change the merge base for this pull request, so raised a new pull request (#51) for karate:develop branch. |
closing, superseded by #51 |
Hi Peter,
Following up on our discussion about the issue 'Unable to send a body with DELETE method', I am submitting this pull request.
The code change has been done on the latest version of Karate - v0.3.1 - at the time of this writing.
I have tested the change with POST and DELETE methods and it works as expected. POST method without body throws the Runtime exception but DELETE works with/without the request body.
Kindly let me know if you have any further comments/questions on the change.
Thanks.
Satish Autade