Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Labels:
      None

      Description

      Hi,

      It seems that PUT requests that contain a large body are truncated.
      Temporarily, I resolved with the following workaround in the method HttpRequest.of ()

      if (requestLine.contains ("POST") | | requestLine.contains ("PUT")) {
      int ContentLength = Integer.parseInt (generalHeaders.get ("content-length"));
      if (ContentLength> body.length ())

      { return new PartialHttpRequest (requestLine, generalHeaders, body); }

      }

      I hope it can be helpful

        Activity

        Tony Stevenson made changes -
        Project Deft [ 12311521 ] Apache AWF [ 12313220 ]
        Key DEFT-153 AWF-154
        Reporter Niklas Gustavsson [ niklas ]
        Issue Type New Feature [ 2 ] Bug [ 1 ]
        Component/s Core [ 12315223 ]
        Fix Version/s 0.4.0 [ 12317348 ]
        Johnathan Meehan made changes -
        Status In Progress [ 3 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Johnathan Meehan added a comment -

        That has been merged with the current code, and applied. Michele, please check and see that everything is still okay by you.
        Committed: http://svn.apache.org/viewvc?rev=1153598&view=rev

        Ready for review.

        Show
        Johnathan Meehan added a comment - That has been merged with the current code, and applied. Michele, please check and see that everything is still okay by you. Committed: http://svn.apache.org/viewvc?rev=1153598&view=rev Ready for review.
        Hide
        Michele Zuccalà added a comment -

        Hi Johnathan,

        I've take a look on deft-178, and it seems promising.
        If you want, to avoid trouble, I can wait to attach a final patch

        > This is all just my opinion though, and I am not trying to push you into a particular decision just discuss things a
        > little.

        Sure, I thought, and our discussion was very useful
        Thanks

        Show
        Michele Zuccalà added a comment - Hi Johnathan, I've take a look on deft-178, and it seems promising. If you want, to avoid trouble, I can wait to attach a final patch > This is all just my opinion though, and I am not trying to push you into a particular decision just discuss things a > little. Sure, I thought, and our discussion was very useful Thanks
        Hide
        Johnathan Meehan added a comment -

        You're a patient man, Michele.
        I do see where you are coming from, but I am personally wary of introducing code that is not currently used (YAGNI, and all that). My own opinion is to code what is needed now, and refactor as the next story/issue comes into play. Will MalFormedHttpRequest use this constructor? Right now it throws an empty Map into the other one and I don't see any other HttpRequest implementations.

        > Feel free to give me advice, and with pleasure I'll change the patch
        This is all just my opinion though, and I am not trying to push you into a particular decision just discuss things a little. The patch fixes the problem, so I'm good with it. Just let me know which way you intend to go.

        Show
        Johnathan Meehan added a comment - You're a patient man, Michele. I do see where you are coming from, but I am personally wary of introducing code that is not currently used (YAGNI, and all that). My own opinion is to code what is needed now, and refactor as the next story/issue comes into play. Will MalFormedHttpRequest use this constructor? Right now it throws an empty Map into the other one and I don't see any other HttpRequest implementations. > Feel free to give me advice, and with pleasure I'll change the patch This is all just my opinion though, and I am not trying to push you into a particular decision just discuss things a little. The patch fixes the problem, so I'm good with it. Just let me know which way you intend to go.
        Hide
        Michele Zuccalà added a comment -

        > Understood thanks, but I still have questions. Bear with me.

        No problem Johnathan, I'll try to explain better

        >I'm still curious about passing null for "params". Why not just remove it from the signatrue and set the local to an
        >empty Map (or null, I suppose)?
        >If this constructor is specific to creating a HttpRequest suitable for partials, what about documenting that fact and
        >then removing the QUERY_STRING_PATTERN split? If I understand, that will always be a split on "<>", so you
        >could just set "requestPath" directly.

        You are right, but I don't have created this constructor only for Partials, because are also other classes in Deft that can switch to it (MalFormedHttpRequest for example) and save superfluous steps
        However in general, the truth is that when I develop my code, I prefer to create 'dummy' constructor (whit all, or most, settable properties) and, when needed, more constructors that, with less parameters, try to compute the others. Excuse me, if my patch contains some superfluous code
        Feel free to give me advice, and with pleasure I'll change the patch

        Last thing:

        • this is not the right place, but I think that instead of private final fields, for optional properties, can be better to find another solution (like deleting setters), because in this way subclassing HttpRequest is more difficult

        Thanks
        Michele

        Show
        Michele Zuccalà added a comment - > Understood thanks, but I still have questions. Bear with me. No problem Johnathan, I'll try to explain better >I'm still curious about passing null for "params". Why not just remove it from the signatrue and set the local to an >empty Map (or null, I suppose)? >If this constructor is specific to creating a HttpRequest suitable for partials, what about documenting that fact and >then removing the QUERY_STRING_PATTERN split? If I understand, that will always be a split on "<>", so you >could just set "requestPath" directly. You are right, but I don't have created this constructor only for Partials, because are also other classes in Deft that can switch to it (MalFormedHttpRequest for example) and save superfluous steps However in general, the truth is that when I develop my code, I prefer to create 'dummy' constructor (whit all, or most, settable properties) and, when needed, more constructors that, with less parameters, try to compute the others. Excuse me, if my patch contains some superfluous code Feel free to give me advice, and with pleasure I'll change the patch Last thing: this is not the right place, but I think that instead of private final fields, for optional properties, can be better to find another solution (like deleting setters), because in this way subclassing HttpRequest is more difficult Thanks Michele
        Hide
        Johnathan Meehan added a comment -

        Understood thanks, but I still have questions. Bear with me.

        I'm still curious about passing null for "params". Why not just remove it from the signatrue and set the local to an empty Map (or null, I suppose)?
        If this constructor is specific to creating a HttpRequest suitable for partials, what about documenting that fact and then removing the QUERY_STRING_PATTERN split? If I understand, that will always be a split on "<>", so you could just set "requestPath" directly.

        Show
        Johnathan Meehan added a comment - Understood thanks, but I still have questions. Bear with me. I'm still curious about passing null for "params". Why not just remove it from the signatrue and set the local to an empty Map (or null, I suppose)? If this constructor is specific to creating a HttpRequest suitable for partials, what about documenting that fact and then removing the QUERY_STRING_PATTERN split? If I understand, that will always be a split on "<>", so you could just set "requestPath" directly.
        Hide
        Michele Zuccalà added a comment -

        >- The current parse of "version" will return "Unfinished" rather than "Unfinished request" as it based on spaces, so
        > inital text would have to chage to something like "UnfinishedRequest".

        You are right Johnathan, but in order to make a patch less intrusive(and at this moment I not fully understand the fundament of deft) I've 'simply' added the method PUT in the existing logic

        > I was wondering why there is new constructor in HttpRequest:
        I've thinked that in some circustances (and this is one, in my opinion) can be useful.
        In my code, for example, if I use the HttpRequest(String requestLine, Map<String, String> headers) constructor:

        • I split requestLine twice
        • PartialHttpRequest is knows that not contains valid url, etc etc.. then parsing parameters for example.. is superfluous
        Show
        Michele Zuccalà added a comment - >- The current parse of "version" will return "Unfinished" rather than "Unfinished request" as it based on spaces, so > inital text would have to chage to something like "UnfinishedRequest". You are right Johnathan, but in order to make a patch less intrusive(and at this moment I not fully understand the fundament of deft) I've 'simply' added the method PUT in the existing logic > I was wondering why there is new constructor in HttpRequest: I've thinked that in some circustances (and this is one, in my opinion) can be useful. In my code, for example, if I use the HttpRequest(String requestLine, Map<String, String> headers) constructor: I split requestLine twice PartialHttpRequest is knows that not contains valid url, etc etc.. then parsing parameters for example.. is superfluous
        Johnathan Meehan made changes -
        Component/s Core [ 12315223 ]
        Hide
        Johnathan Meehan added a comment -

        Hi Michele,
        I was wondering why there is new constructor in HttpRequest:

        HttpRequest(String method, String fullUrl, String version, Map<String, String> headers, ImmutableMultimap<String, String> params)

        It seems to me that we could use the existing when calling from PartialHttpRequest:

        super(method + " <> UnfinishedRequest\r\n", generalHeaders);
        --> HttpRequest(String requestLine, Map<String, String> headers)

        • Do we need "params" in the signature, given that null is being passed? Using the existing constructor, the local "parameters" will be empty, not null.
        • The current parse of "version" will return "Unfinished" rather than "Unfinished request" as it based on spaces, so inital text would have to chage to something like "UnfinishedRequest".

        What do you think? Is there anything I missed here?

        Show
        Johnathan Meehan added a comment - Hi Michele, I was wondering why there is new constructor in HttpRequest: HttpRequest(String method, String fullUrl, String version, Map<String, String> headers, ImmutableMultimap<String, String> params) It seems to me that we could use the existing when calling from PartialHttpRequest: super(method + " <> UnfinishedRequest\r\n", generalHeaders); --> HttpRequest(String requestLine, Map<String, String> headers) Do we need "params" in the signature, given that null is being passed? Using the existing constructor, the local "parameters" will be empty, not null. The current parse of "version" will return "Unfinished" rather than "Unfinished request" as it based on spaces, so inital text would have to chage to something like "UnfinishedRequest". What do you think? Is there anything I missed here?
        Johnathan Meehan made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Johnathan Meehan made changes -
        Assignee Johnathan Meehan [ jmeehan ]
        Hide
        Johnathan Meehan added a comment -

        Will review.

        Show
        Johnathan Meehan added a comment - Will review.
        Michele Zuccalà made changes -
        Attachment deft-153_preview.patch [ 12488849 ]
        Michele Zuccalà made changes -
        Attachment deft-153_preview.patch [ 12488850 ]
        Michele Zuccalà made changes -
        Attachment deft-153_preview.patch [ 12488849 ]
        Hide
        Michele Zuccalà added a comment -

        This is a big issue for my project (deft is the REST framework for our API)
        This is my patch that has solved this problem.

        Thanks
        Michele

        Show
        Michele Zuccalà added a comment - This is a big issue for my project (deft is the REST framework for our API) This is my patch that has solved this problem. Thanks Michele
        Roger Schildmeijer made changes -
        Fix Version/s 0.4.0 [ 12317348 ]
        Roger Schildmeijer made changes -
        Issue Type Improvement [ 4 ] New Feature [ 2 ]
        Roger Schildmeijer made changes -
        Field Original Value New Value
        Issue Type Bug [ 1 ] Improvement [ 4 ]
        Priority Major [ 3 ]
        Hide
        Ulrich Stärk added a comment -

        That should work for now. Deft has no official support for PUT methods (yet )

        Show
        Ulrich Stärk added a comment - That should work for now. Deft has no official support for PUT methods (yet )
        Anonymous created issue -

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development