Apache AWF
  1. Apache AWF
  2. AWF-160

POST/PUT support in AsynchronousHttpClient

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Labels:
      None
    1. deft_117.patch
      68 kB
      Johnathan Meehan
    2. deft_117-v2.patch
      67 kB
      Roger Schildmeijer
    3. deft_117-v3.patch
      68 kB
      Roger Schildmeijer

      Activity

      Hide
      Roger Schildmeijer added a comment -

      Thanks Emmanuel, really appreciated.

      Show
      Roger Schildmeijer added a comment - Thanks Emmanuel, really appreciated.
      Hide
      Johnathan Meehan added a comment -

      Code committed to branch; setting to resolved.

      Show
      Johnathan Meehan added a comment - Code committed to branch; setting to resolved.
      Hide
      Emmanuel Lecharny added a comment -

      Patch applied. Committed revision 1151259.

      Show
      Emmanuel Lecharny added a comment - Patch applied. Committed revision 1151259.
      Hide
      Johnathan Meehan added a comment -

      That's all fine by me (we really should finish off that discussion on coding standards ). Must remember to update CHANGES and NOTES in the future...

      Show
      Johnathan Meehan added a comment - That's all fine by me (we really should finish off that discussion on coding standards ). Must remember to update CHANGES and NOTES in the future...
      Hide
      Roger Schildmeijer added a comment -

      Forgot to update CHANGES and NEWS in previous patch (v2)

      Show
      Roger Schildmeijer added a comment - Forgot to update CHANGES and NEWS in previous patch (v2)
      Hide
      Roger Schildmeijer added a comment -

      Looks good. codewise +1 (good job!)

      some minor comments.
      1, removed /**

      {@inheritDoc}

      */ from some interface impl.
      2, added, asf license header to KnuthMorrisPrattAlgorithm (was my bad, hopefully we can submit this patch and get DEFT-115 for free )
      3, AsynchronousHttpClient.this.method(); => method();
      4, unnecessary this. prefix (removed them)

      I guess you have some IDE formatting on that made 3 and 4. I manually fixed the issues above and created a v2 patch. Feel free to take a look at it. (so we agree)

      Show
      Roger Schildmeijer added a comment - Looks good. codewise +1 (good job!) some minor comments. 1, removed /** {@inheritDoc} */ from some interface impl. 2, added, asf license header to KnuthMorrisPrattAlgorithm (was my bad, hopefully we can submit this patch and get DEFT-115 for free ) 3, AsynchronousHttpClient.this.method(); => method(); 4, unnecessary this. prefix (removed them) I guess you have some IDE formatting on that made 3 and 4. I manually fixed the issues above and created a v2 patch. Feel free to take a look at it. (so we agree)
      Hide
      Johnathan Meehan added a comment -

      I'd be grateful for a review of this work.
      Note 1: It includes DEFT-115, so kindly ignore.
      Note 2: I created another example to go with the existing asynchronous client example. The old one is now for GET, whilst the other is for POST as it highlights direct use of the Request object. The URLs currently point to "http://incubator.apache.org/deft/" - perhaps we could create a small page for the GET to pull, and even a simple server-side script to return a short message for POST.

      Show
      Johnathan Meehan added a comment - I'd be grateful for a review of this work. Note 1: It includes DEFT-115 , so kindly ignore. Note 2: I created another example to go with the existing asynchronous client example. The old one is now for GET, whilst the other is for POST as it highlights direct use of the Request object. The URLs currently point to "http://incubator.apache.org/deft/" - perhaps we could create a small page for the GET to pull, and even a simple server-side script to return a short message for POST.
      Hide
      Roger Schildmeijer added a comment -

      let me know when its ready for review

      Show
      Roger Schildmeijer added a comment - let me know when its ready for review
      Hide
      Johnathan Meehan added a comment - - edited

      Added patch of initial implementation (which includes DEFT-115), for safekeeping and the curious. Need to review that things are actually okay (I think they are), review the methods which do and do not have bodies, and see about bringing AsynchronousHttpClient under better test.

      Edit 1:
      Will not bother checking if bodies are necessary when composing the request. As per Roy Fielding's comment:
      "In other words, any HTTP request message is allowed to contain a message body, and thus must parse messages with that in mind."
      May not be useful, but should be allowed: http://tech.groups.yahoo.com/group/rest-discuss/message/9962

      Will create method signatures in line with general expectation, though. So, requests such as GET will have to call the setBody accessor; requests such as POST will have a parameter. The current HttpVerb.hasRequestBody check is not needed; will just check for a body that is not null (as opposed to empty, which is content length 0).

      Show
      Johnathan Meehan added a comment - - edited Added patch of initial implementation (which includes DEFT-115 ), for safekeeping and the curious. Need to review that things are actually okay (I think they are), review the methods which do and do not have bodies, and see about bringing AsynchronousHttpClient under better test. Edit 1: Will not bother checking if bodies are necessary when composing the request. As per Roy Fielding's comment: "In other words, any HTTP request message is allowed to contain a message body, and thus must parse messages with that in mind." May not be useful, but should be allowed: http://tech.groups.yahoo.com/group/rest-discuss/message/9962 Will create method signatures in line with general expectation, though. So, requests such as GET will have to call the setBody accessor; requests such as POST will have a parameter. The current HttpVerb.hasRequestBody check is not needed; will just check for a body that is not null (as opposed to empty, which is content length 0).
      Hide
      Johnathan Meehan added a comment -

      Good thinking; done.

      Show
      Johnathan Meehan added a comment - Good thinking; done.
      Hide
      Roger Schildmeijer added a comment -

      cool...

      maybe its a good idea to work on this issue when you have DEFT-115 (Raw byte[] instead of US_ASCII encoded String) applied locally?
      To avoid rebasing...

      Show
      Roger Schildmeijer added a comment - cool... maybe its a good idea to work on this issue when you have DEFT-115 (Raw byte[] instead of US_ASCII encoded String) applied locally? To avoid rebasing...

        People

        • Assignee:
          Johnathan Meehan
          Reporter:
          Niklas Gustavsson
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development