Solr
  1. Solr
  2. SOLR-7217

Auto-detect HTTP body content-type for curl

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.1
    • Component/s: None
    • Labels:
      None

      Description

      It's nice to be able to leave off the specification of content type when hand crafting a request (i.e. from the command line) and for our documentation examples.

      For example:

      curl http://localhost:8983/solr/query -d '
      {
        query:"hero"
      }'
      

      Note the missing

      -H 'Content-type:application/json'
      

      that would otherwise be needed everywhere

      1. SOLR-7217.patch
        25 kB
        Yonik Seeley

        Issue Links

          Activity

          Hide
          Noble Paul added a comment -

          The problem is that curl automatically adds a default content type as application/x-www-form-urlencoded if you skip that

          Show
          Noble Paul added a comment - The problem is that curl automatically adds a default content type as application/x-www-form-urlencoded if you skip that
          Hide
          Yonik Seeley added a comment - - edited

          Right, so the logic is to autodetect when there is no content-type or if the client is curl with the default that curl adds.

          Show
          Yonik Seeley added a comment - - edited Right, so the logic is to autodetect when there is no content-type or if the client is curl with the default that curl adds.
          Hide
          Noble Paul added a comment -

          I didn't get that. what does the server do if content-type application/x-www-form-urlencoded ?

          Show
          Noble Paul added a comment - I didn't get that. what does the server do if content-type application/x-www-form-urlencoded ?
          Hide
          Yonik Seeley added a comment -

          If the client is curl AND the content-type is curl's default (i.e. "application/x-www-form-urlencoded") then we auto-detect instead of just trusting curl.

          Show
          Yonik Seeley added a comment - If the client is curl AND the content-type is curl's default (i.e. "application/x-www-form-urlencoded") then we auto-detect instead of just trusting curl.
          Hide
          Yonik Seeley added a comment -

          If anyone wants to try it out in practice before it gets backported here, it's implemented in heliosearch.

          Show
          Yonik Seeley added a comment - If anyone wants to try it out in practice before it gets backported here, it's implemented in heliosearch.
          Hide
          Noble Paul added a comment -

          But how do we know that client is curl? does it send an extra header?

          Show
          Noble Paul added a comment - But how do we know that client is curl? does it send an extra header?
          Hide
          Yonik Seeley added a comment -

          But how do we know that client is curl? does it send an extra header?

          Yes.

          Show
          Yonik Seeley added a comment - But how do we know that client is curl? does it send an extra header? Yes.
          Hide
          Noble Paul added a comment -

          Good , I overlooked it. I was trying to cleanup our examples and the user-agent thing didn't strike me.
          ++1

          Show
          Noble Paul added a comment - Good , I overlooked it. I was trying to cleanup our examples and the user-agent thing didn't strike me. ++1
          Hide
          Uwe Schindler added a comment -

          Hi, where is a patch or commit link in Heliosearch?

          Show
          Uwe Schindler added a comment - Hi, where is a patch or commit link in Heliosearch?
          Hide
          Yonik Seeley added a comment -

          I haven't created a patch yet... it was part of a larger commit in helio: ff43c0a 2014-12-03 | json requests [yonik]

          Show
          Yonik Seeley added a comment - I haven't created a patch yet... it was part of a larger commit in helio: ff43c0a 2014-12-03 | json requests [yonik]
          Hide
          Yonik Seeley added a comment -

          Heres a patch. I reworked it a fair amount rather than just do a straight Heliosearch backport (since in some of my Helio work the goal was to minimize merge conflicts).

          • rewrote StandardRequestParser.parseParamsAndFillStreams() for clarity (the big "if" in the previous version was pretty confusion), and commented what I've figured out along the way.
          • improved the pattern matching for "restlet paths" /config and /schema
            so something like a core name of "schema_free" wouldn't accidentally cause a match.
          • form data from curl now autodetects JSON & XML
          • StringStream now allows the specification of a content type and only tries to auto-detect if not given. It also only auto-detects once and remembers it.
          • tried to make those easymock based tests a little less fragile

          Aside: I discovered that SOLR-6787 changed the behavior of a POST without content type. Previously it would throw an exception, but now it will use the raw request parser. Not sure if that was on purpose or not.

          Show
          Yonik Seeley added a comment - Heres a patch. I reworked it a fair amount rather than just do a straight Heliosearch backport (since in some of my Helio work the goal was to minimize merge conflicts). rewrote StandardRequestParser.parseParamsAndFillStreams() for clarity (the big "if" in the previous version was pretty confusion), and commented what I've figured out along the way. improved the pattern matching for "restlet paths" /config and /schema so something like a core name of "schema_free" wouldn't accidentally cause a match. form data from curl now autodetects JSON & XML StringStream now allows the specification of a content type and only tries to auto-detect if not given. It also only auto-detects once and remembers it. tried to make those easymock based tests a little less fragile Aside: I discovered that SOLR-6787 changed the behavior of a POST without content type. Previously it would throw an exception, but now it will use the raw request parser. Not sure if that was on purpose or not.
          Hide
          Uwe Schindler added a comment -

          Thanks! I will look into the patch tomorrow.

          One question ahead: Is it intended to do trim() on all formdata keys? This is unrelated to this issue and in my opinion not a good idea! No servlet container /webserver does this (not even PHP), so our formadata parser should not do it, too.

          Show
          Uwe Schindler added a comment - Thanks! I will look into the patch tomorrow. One question ahead: Is it intended to do trim() on all formdata keys? This is unrelated to this issue and in my opinion not a good idea! No servlet container /webserver does this (not even PHP), so our formadata parser should not do it, too.
          Hide
          Yonik Seeley added a comment -

          Trimming keys allows things like the following to work (notice the json.jacet query parameter on the next line) and leads to less surprising behavior.

          $ curl http://localhost:8983/solr/query -d 'q=*:*&rows=0&
           json.facet={
             categories:{
               terms:{
                 field : cat,
                 sort : { x : desc},
                 facet:{
                   x : "avg(price)",
                   y : "sum(price)"
                 }
               }
             }
           }
          '
          
          Show
          Yonik Seeley added a comment - Trimming keys allows things like the following to work (notice the json.jacet query parameter on the next line) and leads to less surprising behavior. $ curl http: //localhost:8983/solr/query -d 'q=*:*&rows=0& json.facet={ categories:{ terms:{ field : cat, sort : { x : desc}, facet:{ x : "avg(price)" , y : "sum(price)" } } } } '
          Hide
          Uwe Schindler added a comment -

          Patch looks almost fine, although I am not so happy about having the detection stuff inside the formData parser. Maybe the content-type detection should be part of Standard Parser? I think we could move the detection part into the "if (isFormdata())

          {...}

          " part. There is also the place where we decide if it's "curl", so the detection should only happen there.

          Show
          Uwe Schindler added a comment - Patch looks almost fine, although I am not so happy about having the detection stuff inside the formData parser. Maybe the content-type detection should be part of Standard Parser? I think we could move the detection part into the "if (isFormdata()) {...} " part. There is also the place where we decide if it's "curl", so the detection should only happen there.
          Hide
          ASF subversion and git services added a comment -

          Commit 1667457 from Yonik Seeley in branch 'dev/trunk'
          [ https://svn.apache.org/r1667457 ]

          SOLR-7217: autodetect POST body for curl

          Show
          ASF subversion and git services added a comment - Commit 1667457 from Yonik Seeley in branch 'dev/trunk' [ https://svn.apache.org/r1667457 ] SOLR-7217 : autodetect POST body for curl
          Hide
          ASF subversion and git services added a comment -

          Commit 1667458 from Yonik Seeley in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1667458 ]

          SOLR-7217: autodetect POST body for curl

          Show
          ASF subversion and git services added a comment - Commit 1667458 from Yonik Seeley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1667458 ] SOLR-7217 : autodetect POST body for curl
          Hide
          Yonik Seeley added a comment -

          Moved the autodetect code to a separate method and committed.

          Show
          Yonik Seeley added a comment - Moved the autodetect code to a separate method and committed.
          Hide
          Uwe Schindler added a comment -

          Thanks. I have seen the commit, looks perfect

          The 5.x backport seems to break one of the tests, Jenkins is complaining. Looks like a class is missing.

          Show
          Uwe Schindler added a comment - Thanks. I have seen the commit, looks perfect The 5.x backport seems to break one of the tests, Jenkins is complaining. Looks like a class is missing.
          Hide
          ASF subversion and git services added a comment -

          Commit 1667551 from Yonik Seeley in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1667551 ]

          SOLR-7217: add missing ByteServletInputStream to test

          Show
          ASF subversion and git services added a comment - Commit 1667551 from Yonik Seeley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1667551 ] SOLR-7217 : add missing ByteServletInputStream to test
          Hide
          Timothy Potter added a comment -

          Bulk close after 5.1 release

          Show
          Timothy Potter added a comment - Bulk close after 5.1 release

            People

            • Assignee:
              Yonik Seeley
              Reporter:
              Yonik Seeley
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development