httpDelete did not send body payload

[babedev]

Fuel: v1.12.0

I tried to sent HTTP DELETE with JSON body. I checked HttpClient.kt that it skip body set up for GET, DELETE, HEAD

private fun setDoOutput(connection: HttpURLConnection, method: Method) = when (method) {
   Method.GET, Method.DELETE, Method.HEAD -> connection.doOutput = false
   Method.POST, Method.PUT, Method.PATCH -> connection.doOutput = true
}

Should we allow Method.DELETE? Because I have scenario that API need body payload for DELETE.

Same issue #245

[kittinunf]

Yeah, I think by the time I have designed this base on the RFC spec. It is not specified as permitted or not so that I didn't follow through with the implementation for this. However, most of the modern HTTP libraries support this so we should support it too!

@babedev Would you mind creating a PR for this? I can help you out as well.

[babedev]

Sure. I will let it support both GET and DELETE.

[simonharrer]

Is there already a PR open for this? I need to interface an API that requires me to send a json body alongside a GET request.... :-(

[kittinunf]

I haven't gotten time to work on this yet, sorry. 😢

[kmcgill88]

Any movement on this? I need my DELETE to have a payload.

[markGilchrist]

@kittinunf do you want me to look at this?

[kmcgill88]

Well I shaped up and changed my implementation to not send a body in a DELETE per the RFC.

[markGilchrist]

@kittinunf @babedev is there still a need for this as a feature?

[SleeplessByte]

Before, the spec defined that the body SHOULD be ignored (and therefore a lot of clients did not allow for a DELETE request's body, but this was updated with RFC7231:

https://tools.ietf.org/html/rfc7231#section-4.3.5

4.3.5.  DELETE
  
   [...]
   
   A payload within a DELETE request message has no defined semantics;
   sending a payload body on a DELETE request might cause some existing
   implementations to reject the request.

   Responses to the DELETE method are not cacheable.  If a DELETE
   request passes through a cache that has one or more stored responses
   for the effective request URI, those stored responses will be
   invalidated (see Section 4.4 of [RFC7234]).

Therefore, even though current users might not need this as a feature, it SHOULD be implemented.

[kittinunf]

Yes, I agree that we should update this to accommodate users' needs.

[simonharrer]

The same holds true for GET requests with payloads. See https://tools.ietf.org/html/rfc7231#section-4.3.1

[markGilchrist]

@babedev @simonharrer @kittinunf @SleeplessByte @kmcgill88

Guys I am going to pick this up I have opened a WIP Pr (https://github.com/kittinunf/Fuel/pull/417)

I would really appreciate your feedback and reviews.

[ontje-dev]

is there an estimated time when this will be merged? i just recognized that i use an API that needs a request-body for a DELETE and i dont want to change the lib for a single request.

[iNoles]

@markGilchrist In my opinion, doOutput should be configurable to any developers.

[markGilchrist]

@eybeestudios this is now merged in

[simonharrer]

@markGilchrist Thanks for your effort! Had a look at your PR - nice work btw., but are you sure that GET requests can have a payload now as well? From the PR #417 I can only see the change for the DELETE requests.

[markGilchrist]

@simonharrer No that work was not included in this PR it only covers DELETE, I am working on a second PR for GET, if I tag it in here I would really appreciate any comments you may have!

[SleeplessByte]

Seems that this can be closed now, and we can continue on the PR once it's up ;)

[smallufo]

Hi , is this issue really solved ? I try to fire an DELETE request (with body) to FB's API , but it seems it dosen't work. (version 1.15.0)

sample code :

API spec from FB doc : https://developers.facebook.com/docs/messenger-platform/reference/messenger-profile-api/#delete

val uri = "https://graph.facebook.com/$version/me/messenger_profile?access_token=$pageToken"

val root = json {
  obj("fields" to array(PERSISTENT_MENU))
}
val json = klaxon.toJsonString(root)
logger.info("delete json = {}" , json)

val (req: Request, res: Response, result: Result<String, FuelError>) = Fuel.delete(uri)
  .header(mapOf("Content-Type" to "application/json"))
  .body(json)
  .responseString()

logger.info("req string = {}" , req.toString())

But FB seems not receiving the body . See the attachment

But the same code works in curl :

I am confused , what may go wrong here ?

[SleeplessByte]

val version = 3
val pageToken = UUID.randomUUID().toString()
val uri = "https://graph.facebook.com/$version/me/messenger_profile"

val root = "{ \"fields\": [\"$PERSISTENT_MENU\"] }"

val request = Fuel.delete(uri, parameters = listOf("access_token" to pageToken))
  .header(mapOf(Headers.CONTENT_TYPE to "application/json"))
  .body(root)

println(request.cUrlString())

This gives me

$ curl -i -X DELETE -d "{ \"fields\": [\"persistent_menu\"] }" -H "Content-Type:application/json" https://graph.facebook.com/3/me/messenger_profile?access_token=7940c9cc-656c-42c3-9d9d-f786bd30699d

I've also tested this via a mocked request, and it seems fine :/

What version are you on?

[smallufo]

I am using Fuel 1.15.0 , with FB's graph version v3.2 BTW , I don't think your code works , because it's not a valid page_access_token. If it passes access token check , it will check the content, which leads it cannot find my fields field

Is there anyway to truly dump the network layer stream with Fuel's API ? I am not sure if it FB or Fuel's implementation problem... (network sniffer maybe useless because it is https)

[SleeplessByte]

Obviously I wasn't going to post my actual token hehe ;)

That said, everything is piped to HttpURLConnection and then it leaves fuel. No real way of intercepting it at that link without using a proxy.

Can you print your request curlstring and see it that one works?

[smallufo]

Wow , I don't know Fuel has curlString function , great. This is my result :

curl -i -X DELETE -d "{\"fields\": [\"persistent_menu\"]}" -H "Accept-Encoding:compress;q=0.5, gzip;q=1.0" -H "Content-Type:application/json" https://graph.facebook.com/v3.2/me/messenger_profile?access_token=EAAGpXXX

The result is the same.

BUT , very interesting . After I copy the curlString to console and execute , it is OK !!!

This is so weird.

[SleeplessByte]

That's actually really helpful.

I can think of a few things, but for now, can you also check this slightly weird thing:

val (request, _, _) = what you already have.response() println(request.cUrlString())

My initial thought is that Facebook is actually choking on something else, or that this still has the content type double header bug.

On Sun, 28 Oct 2018, 17:58 smallufo, [email protected] wrote:

Wow , I don't know Fuel has curlString function , great. This is my result :

curl -i -X DELETE -d "{"fields": ["persistent_menu"]}" -H "Accept-Encoding:compress;q=0.5, gzip;q=1.0" -H "Content-Type:application/json" https://graph.facebook.com/v3.2/me/messenger_profile?access_token=EAAGpXXX

[image: intellij] https://user-images.githubusercontent.com/626659/47619055-35e81b00-db15-11e8-8191-d5e42be34c2e.png The result is the same.

BUT , very interesting . After I copy the curlString to console and execute , it is OK !!! [image: curl] https://user-images.githubusercontent.com/626659/47619066-51ebbc80-db15-11e8-9ca2-53a084a98176.png

This is so weird.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/kittinunf/Fuel/issues/306#issuecomment-433722437, or mute the thread https://github.com/notifications/unsubscribe-auth/AB35WFmyITv-ChOB55jlsSAvUAYrXsqCks5upeIlgaJpZM4R2vRI .

[smallufo]

Sorry , isn't it what I have already done ?

val (req: Request, res: Response, result: Result<String, FuelError>) = Fuel.delete(uri)
  .header(mapOf("Content-Type" to "application/json"))
  .body(json)
  .responseString()

logger.info("curlString = {}" , req.cUrlString())
logger.info("req string = {}" , req.toString())

The result is as the screenshot I pasted.

[smallufo]

I try to remove the Accept-Encoding header ,

val (req: Request, res: Response, result: Result<String, FuelError>) = Fuel.delete(uri).also {
  it.headers.remove("Accept-Encoding")
}
  .header(mapOf("Content-Type" to "application/json"))
  .body(json)
  .responseString()

logger.info("curlString = {}" , req.cUrlString())

which generates this :

curl -i -X DELETE -d "{\"fields\": [\"persistent_menu\"]}" -H "Content-Type:application/json" https://graph.facebook.com/v3.2/me/messenger_profile?access_token=EAAGpxxx

The result is the same : failed in kotlin code , success in curl console.

If I remove Content-Type header (which leads empty header) , it won't pass FB (both code and cURL) . So Content-Type header is mandatory here.

[SleeplessByte]

Alright. This might be an issue on our side. I will try some things out over here and see if I can make this work (and write a test so it doesn't fail again/regression)

[iNoles]

I noticed curl response contained HTTP/2 200. I don't know HttpUrlConnection able to support HTTP/2.

[SleeplessByte]

It doesn't.

On Tue, 6 Nov 2018, 05:27 Jonathan, [email protected] wrote:

I noticed curl response contained HTTP/2 200. I don't know HttpUrlConnection able to support HTTP/2.

— You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub https://github.com/kittinunf/Fuel/issues/306#issuecomment-436127259, or mute the thread https://github.com/notifications/unsubscribe-auth/AB35WEObtZoUnkdv2MeOBgODjrARPTO-ks5usQ-9gaJpZM4R2vRI .

[checketts]

Just as a datapoint, RequestBody with DELETE wasn't working for me on 1.13.0, and after upgrading to 1.16.0 it is working for me. I'm not discounting @smallufo 's report, just letting you know that in my case it is working.