playframework / playframework

The Community Maintained High Velocity Web Framework For Java and Scala.
http://www.playframework.com
Apache License 2.0
12.54k stars 4.1k forks source link

POST request parameters - out of order #1814

Closed stykiaz closed 10 years ago

stykiaz commented 11 years ago

There is no way to get the POST requests parameters in the order that they were submitted.

wsargent commented 11 years ago

Please provide a minimal test case.

It looks like this is a bug: the browser always sends form data in the tree order, so that should be preserved in the API -- source is http://stackoverflow.com/a/14558030/5266

stykiaz commented 11 years ago

I am talking about manually generated requests.

If we use a method like this for example to make the request

public static HttpResponse makePostRequest( String url, List<BasicNameValuePair> p ) throws ClientProtocolException, IOException {
        HttpClient httpclient = new DefaultHttpClient();
        HttpPost httppost = new HttpPost( url );

        // Request parameters and other properties.
        httppost.setEntity(new UrlEncodedFormEntity(p, "UTF-8"));
        httppost.setEntity(new ByteArrayEntity( p.toString().getBytes("UTF-8") ));

        //Execute and get the response.
        HttpResponse response = httpclient.execute(httppost);
        return response;
    }

and use it like this for example:

List<BasicNameValuePair> params = new ArrayList<BasicNameValuePair>();
        params.add( new BasicNameValuePair("MID", "000000000000123") );
        params.add( new BasicNameValuePair("MIDName", "Example Web Shop Name") );
        .......
        HttpResponse result = utils.Http.makePostRequest("http://...."  , params);

the order of the parameters is not preserved.

wsargent commented 11 years ago

From what I can tell, you have the wrong project: you probably want Apache HttpClient here: http://hc.apache.org/httpcomponents-client-ga/

stykiaz commented 11 years ago

Alright. Maybe I am not explaining myself properly here. HttpClient is working perfectly. I am only using apache's package to send the request. It is received by a Play2.1.3 application where the request parameters are in the wrong order. And to be sure I creared a simple PHP script which outputs the request parameters in the order they were sent.

wsargent commented 11 years ago

Please create a Play project (ideally using the internal WS client or Fluentium):

http://www.playframework.com/documentation/2.2.x/JavaWS

and add a test using the test server:

http://www.playframework.com/documentation/2.2.x/JavaFunctionalTest

then upload the project to Github.

huntc commented 10 years ago

Field order is not guaranteed. See:

http://tools.ietf.org/html/rfc2388

Section 5.5:

5.5 Ordered fields and duplicated field names

The relationship of the ordering of fields within a form and the ordering of returned values within "multipart/form-data" is not defined by this specification, nor is the handling of the case where a form has multiple fields with the same name. While HTML-based forms may send back results in the order received, and intermediaries should not reorder the results, there are some systems which might not define a natural order for form fields.

jroper commented 10 years ago

@huntc

Actually, that's the spec for multipart/form-data, I think this user is referring to application/x-www-form-urlencoded, which is specified as part of the HTML spec, in section 4.10.22.4:

Let controls be a list of all the submittable elements whose form owner is form, in tree order.

Reading on from there, the spec does carefully ensure that the order of the elements is maintained. The HTML5 spec also specifies the order in which fields should appear in multipart/form-data encoding.

There are many applications that require a well defined order, including reorderable multi-selects, for example.

stykiaz commented 10 years ago

Apologies for being able to provide the tests you asked me for but @jroper is correct. I am talking about application/x-www-form-urlencoded form POST submits. Some applications do require to have the parameters in the order they were submitted and the only way, I could find, to do this is to run it through a proxy from a different language/script .. in my case a php file ... hardly very elegant or efficient. Thanks

reid-spencer commented 10 years ago

I'm taking a crack at this one.

reid-spencer commented 10 years ago

I did a simple/naive test using a FakeRequest, capturing what was sent to an action with it, and comparing. I printed it out and got:

[debug] application - Request: GET / (AnyContentAsText(One=one&Two=two&Three=three&Four=four&Five=five&Six=six&Seven=seven))
[debug] application - Received: (AnyContentAsText(One=one&Two=two&Three=three&Four=four&Five=five&Six=six&Seven=seven))

So, it seems to preserve order in simple cases.

@stykiaz - Could you give me the exact application/x-www-form-urlencoded request BODY that you claim doesn't work? Without a reliable test case that reproduces this, I can't fix it.

reid-spencer commented 10 years ago

Actually, I just realized that my test probably bypassed the entire request processing chain because I called the controller's action directly. :) Of course it was the same! I'll work a little harder on getting this reproducible. :)

stykiaz commented 10 years ago

@reid-spencer I will try and provide some examples before the end of the week.

reid-spencer commented 10 years ago

@stykiaz - thank you :)

reid-spencer commented 10 years ago

Tried to figure this out, but need some help. In my test case, I wrote a FakeApplication like so:

  var save_h :Headers = null
  var save_b: AnyContent = null

  val appWithRoutes = FakeApplication(withRoutes = {
    case ("GET", "/") =>  Action { request: Request[AnyContent] =>
      save_h = request.headers
      save_b = request.body
      Logger.error("In Action")
      Ok(response_text)
    }
    case _ => Action { Logger.error("Bad call"); Ok(response_text); }
  })

I'm trying to capture the request headers and body in the variables at the top. I get null for both after getting the response and when I try to debug this, I cannot set any breakpoints on the save_h and save_b mutations nor does either "In Action" or "Bad call" messages come out in my log (while other things do).

The rest of my test looks like:

 "Play" should {
    "return form fields in order"  in new WithServer(app = appWithRoutes, port = 34343 ) {

      val urlencoded = "One=one&Two=two&Three=three&Four=four&Five=five&Six=six&Seven=seven"
      val content_type = "application/x-www-form-urlencoded"
      val request : WSRequestHolder = WS.url("http://localhost:34343/").withHeaders(
        "Content-Type" -> content_type
      )

      val future = request.post(urlencoded)

      val response = await(future)

      Logger.debug("Response Status=" + response.status)
      response.status must equalTo(OK)

      Logger.debug("Requested: " + urlencoded )
      Logger.debug("Received : " + save_b )
      Logger.debug("Received Headers:" + save_h)
      save_h.get("Content-Type").getOrElse("") must equalTo(content_type)
      save_b must equalTo(urlencoded)
    }
  }

Note that I do get a Status=OK (200) result from the response. If my route's Handler was never called, how did we get an OK(200)?

Is debugging asynchronous Play! actions in IntelliJ a known issue? I've never had a problem before.

So .. a little confused at this point as to what is going on.

reid-spencer commented 10 years ago

I was able to get my test case into better shape by following some of the examples from play.it.http. I now get output like this:

[info] Play' form URL Decoding  should
Request: WSRequestHolder(http://localhost:19001,Map(Content-Type -> List(application/x-www-form-urlencoded)),Map(),None,None,None,Some(10000),None,None)
Requested      : One=one&Two=two&Three=three&Four=four&Five=five&Six=six&Seven=seven
Received.asText: 
Received.asXFUE: Map(One -> ArrayBuffer(one), Five -> ArrayBuffer(five), Seven -> ArrayBuffer(seven), Two -> ArrayBuffer(two), Four -> ArrayBuffer(four), Three -> ArrayBuffer(three), Six -> ArrayBuffer(six))
Received.asREUE: One=one&Five=five&Seven=seven&Two=two&Four=four&Three=three&Six=six
Response Status=200
[info] x preserve form field oder
[error]    'One=one&Five=five&Seven=seven&Two=two&Four=four&Three=three&Six=six'
[error]     is not equal to 
[error]    'One=one&Two=two&Three=three&Four=four&Five=five&Six=six&Seven=seven' (FormFieldOrderSpec.scala:66)
[error] Expected: One=one&Two=two&Three=three&Four=four&Five=five&Six=six&Seven=seven
[error] Actual:   One=one&Five=five&Seven=seven&Two=two&Four=four&Three=three&Six=six

So I believe that the entire problem is one of the arguments simply being hashed into a Map and ordered by their hashcode. While this test is now repeatable, I'm not sure there's a good fix for this without breaking the API. we could return a Seq[String,Seq[String]] from AnyContent.asFormUrlEncoded but then all clients of that call would break as they are expecting a Map[String,Map[String]]. The only reasonable thing I can think of is to add an additional method to `AnyContent, perhaps like this:

def asFormUrlEncodedPreservingOrder : Seq[String,Seq[String]] = ???

@stykiaz - Is AnyContent.asFormUrlEncoded what you're using to extract the body or are you using a Play! Form to do that? I suspect the former because the order matters to you.

For anyone interested, my test case, so far is ...


object FormFieldOrderSpec extends PlaySpecification {

  "Play' form URL Decoding " should {

    def withServer[T](action: EssentialAction)(block: Port => T) = {
      val port = testServerPort
      running(TestServer(port, FakeApplication(
        withRoutes = {
          case _ => action
        }
      ))) {
        block(port)
      }
    }

    val response_text = "preserve form field order"
    val urlencoded = "One=one&Two=two&Three=three&Four=four&Five=five&Six=six&Seven=seven"
    val content_type = "application/x-www-form-urlencoded"
    var saved_body : String = null

    "preserve form field oder" in withServer( Action { request: Request[AnyContent] =>
      println("Requested      : " + urlencoded)
      val as_text = request.body.asText.getOrElse("")
      println("Received.asText: " + as_text)
      val pairs: Seq[String] = { request.body.asFormUrlEncoded map { params: Map[String,Seq[String]] =>
      println("Received.asXFUE: " + params);
        {for ( (key:String,value:Seq[String]) <- request.body.asFormUrlEncoded.get ) yield key + "=" + value
          .mkString }.toSeq
      }}.getOrElse(Seq.empty[String])
      val reencoded = pairs.mkString("&")
      println("Received.asREUE: " + reencoded)
      saved_body = reencoded
      request.headers.get("Content-Type").getOrElse("") must equalTo(content_type)
      Results.Ok(response_text)
    }) { port =>
      val request : WSRequestHolder =
        WS.url("http://localhost:" + port).
           withHeaders("Content-Type" -> content_type).
           withRequestTimeout(10000)

      println("Request: " + request)
      val future = request.post(urlencoded)

      val response = await(future, 10000)
      println("Response Status=" + response.status)
      response.status must equalTo(OK)
      response.body must equalTo(response_text)
      saved_body must equalTo(urlencoded)
    }
  }
}
stykiaz commented 10 years ago

@reid-spencer I am using Play! Form. I am not familiar with the other method.

reid-spencer commented 10 years ago

@stykiaz - Then I'm a little confused as to why the order matters to you? Could you shed some light (code?) on how you are processing the form content? If you're using a Form with a case class, the apply/unapply methods should just DTRT and order doesn't matter.

stykiaz commented 10 years ago

@reid-spencer using request().body().asFormUrlEncoded() or Play! Form .data() method or anything else in the API (all I could fine at least) wouldn't give me the correct order of the request and I need it to generate a Signature for a particular Payment Gateway. As @jroper already pointed out in this thread the order should be preserved and I am failing to find a way of retrieving that parameters in the correct order. Seems that you have managed to reproduce that?

If you still haven't reproduced the problem yet I will try and prepare a decent example this weekend.

reid-spencer commented 10 years ago

@stykiaz - It is easily reproducible. The question now is what to do about it. Would it be sufficient to simply add a method so that you could write an action like:

def foo = Action { request: Request[AnyContent] => 
  for ( pair <- request.body.asFormUrlEncodedPreservingOrder) { ... }
}

From the way you wrote your example, am I to assume you are using the Java interface? I suppose the equivalent in java would be:

request().body().asFormUrlEncodedPreservingOrder()

which would return an Array or Vector instead of a HashMap. Is that sufficient?

stykiaz commented 10 years ago

It certainly will be good enough for me. Whether this is the best way to do it I am not an authority :) On 25 Oct 2013 00:07, "Reid Spencer" notifications@github.com wrote:

@stykiaz https://github.com/stykiaz - It is easily reproducible. The question now is what to do about it. Would it be sufficient to simply add a method so that you could write an action like:

def foo = Action { request: Request[AnyContent] => for ( pair <- request.body.asFormUrlEncodedPreservingOrder) { ... } }

From the way you wrote your example, am I to assume you are using the Java interface? I suppose the equivalent in java would be:

request().body().asFormUrlEncodedPreservingOrder()

Is that sufficient?

— Reply to this email directly or view it on GitHubhttps://github.com/playframework/playframework/issues/1814#issuecomment-27040948 .

reid-spencer commented 10 years ago

@jroper @huntc - Any thoughts on this approach?

jroper commented 10 years ago

@reid-spencer You should be able to fix it with the existing body type, using ListMap:

http://www.scala-lang.org/api/current/index.html#scala.collection.immutable.ListMap

reid-spencer commented 10 years ago

@jroper - Yep, that would be a better choice than Seq. Thanks!

reid-spencer commented 10 years ago

@jroper - That leads to the question .. would it be acceptable to modify the result type of the asFormUrlEncoded method since they are both maps and should have compatible interfaces? Or, is the project policy to keep interfaces the same and not risk breakage. Just want to know whether I ought to be adding another method and the consequent fall out of another case class for the content, BodyParser ....

reid-spencer commented 10 years ago

Unfortunately, as my tests are proving, ListMap does not guarantee iteration order is same as insertion order :(

reid-spencer commented 10 years ago

Apparently mutable.ListMap does not retain insertion order while immutable.ListMap does, so I have a workaround. Should have a patch on this within the hour.

reid-spencer commented 10 years ago

@stykiaz - A pull request (#1910) has been issued to resolve this problem. I'm looking for feedback as this is my first Play! contribution of any significance. I used it to get my feet wet with the code.

@jroper & others - feedback welcome

reid-spencer commented 10 years ago

This was fun! And, it served its purpose, for me, of providing a useful way to introduce myself to the Play! code base. @stykiaz - I hope you find 2.3 more helpful for your use case. Perhaps you could verify with your manually generated requests?

huntc commented 10 years ago

Thanks @reid-spencer !

reid-spencer commented 10 years ago

@huntc - My Pleasure!