1 2 Previous Next 26 Replies Latest reply on Nov 19, 2008 2:26 AM by trustin Go to original post
      • 15. Re: Netty HTTP transport
        trustin

        Hi Andy,

        Sorry for the late response. I was busy moving to a new house. :)

        First off, you have done a fantastic job! I'd like to know if there was any difficulty in implementing HTTP on top of Netty so that I can make the Netty API more user-friendly. :)

        Now, some subtle points:

        1) IIUC, HTTP allows multiple headers with the same key. It seems like the current HttpMessage interface assumes there's only one value per key. Therefore, the headers should be represented as Set<String, List> instead of Map<String, String>.

        This issue will lead to various changes in the header accessor methods of the HttpMessage interface.

        2) HTTP allows multiple parameters with the same key. Therefore, the parameters should be represented as Set<String, List> instead of Map<String, String>. (Same with the issue 1)

        3) I'm not sure if a multi-line header is decoded correctly:

        http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

        4) Reason (error message) is missing in HttpResponse. HttpResponseStatusCode already has the pre-defined reason phrases, but it should be customizable on a per-response basis. (Perhaps we could simply make the HttpResponseStatusCode constructor public?)

        5) What about renaming HttpProtocol to HttpVersion? P of HTTP already stands for 'protocol'. Also, HttpRequest.get/setProtocol() needs to be renamed to get/setVersion() or get/setProtocolVersion().

        6) java.net.URI already has a query property. I think we don't need the parameter accessor methods in the HttpRequest interface. We could provide a helper class like UriBuilder later.

        • 16. Re: Netty HTTP transport
          trustin

           

          "trustin" wrote:
          2) HTTP allows multiple parameters with the same key. Therefore, the parameters should be represented as Set<String, List<String>> instead of Map<String, String>. (Same with the issue 1)


          This issue should go away if we are going to remove the parameter accessor methods.

          • 17. Re: Netty HTTP transport
            ataylor

             

            I'd like to know if there was any difficulty in implementing HTTP on top of Netty so that I can make the Netty API more user-friendly. :)


            The API was cool Trustin. Especially the ReplayingDecoder, it makes decoding complex messages much easier.

            1) IIUC, HTTP allows multiple headers with the same key. It seems like the current HttpMessage interface assumes there's only one value per key. Therefore, the headers should be represented as Set<String, List> instead of Map<String, String>.


            Ok, thats done and ive changed the API.

            2) HTTP allows multiple parameters with the same key. Therefore, the parameters should be represented as Set<String, List> instead of Map<String, String>. (Same with the issue 1)


            also done

            3) I'm not sure if a multi-line header is decoded correctly:


            no, you are correct. If i understand this correctly it should be as follows.

            A header valu ecan be on multiple lines if the next line starts with a space or tab, so:

            aheader: abcdefg
             hij


            would be the same as:

            aheader: abcdefghij


            also multiple values are seperated by a comma, i.e.

            aheader: a,b,c


            actually has 3 values a, b and c. The caveat here is Date type headers such as Date, Expires etc as they have commas within them.

            4) Reason (error message) is missing in HttpResponse. HttpResponseStatusCode already has the pre-defined reason phrases, but it should be customizable on a per-response basis. (Perhaps we could simply make the HttpResponseStatusCode constructor public?)


            enums can't have public constructors so ive added another static method for overriding the reason description.

            5) What about renaming HttpProtocol to HttpVersion? P of HTTP already stands for 'protocol'. Also, HttpRequest.get/setProtocol() needs to be renamed to get/setVersion() or get/setProtocolVersion().


            I agree, done.

            6) java.net.URI already has a query property. I think we don't need the parameter accessor methods in the HttpRequest interface. We could provide a helper class like UriBuilder later.


            not sure i follow completely, i need the URI for encoding purposes so i need an accessor. Unless you meant replace it with a String?

            • 18. Re: Netty HTTP transport
              trustin

               

              "ataylor" wrote:
              The API was cool Trustin. Especially the ReplayingDecoder, it makes decoding complex messages much easier.


              I'm happy to hear that. :)

              "ataylor" wrote:
              "trustin" wrote:
              1) IIUC, HTTP allows multiple headers with the same key. It seems like the current HttpMessage interface assumes there's only one value per key. Therefore, the headers should be represented as Set<String, List> instead of Map<String, String>.


              Ok, thats done and ive changed the API.


              I forgot to mention that the header name is case insensitive. TreeMap with a custom Comparator should be used perhaps?

              "ataylor" wrote:
              also multiple values are seperated by a comma, i.e.

              aheader: a,b,c


              actually has 3 values a, b and c. The caveat here is Date type headers such as Date, Expires etc as they have commas within them.


              Commas are interpreted as header value separators only when explicitly specified in the HTTP specification AFAIK. I think we don't need to take care of commas and left it to the user application.

              "ataylor" wrote:
              "trustin" wrote:
              4) Reason (error message) is missing in HttpResponse. HttpResponseStatusCode already has the pre-defined reason phrases, but it should be customizable on a per-response basis. (Perhaps we could simply make the HttpResponseStatusCode constructor public?)


              enums can't have public constructors so ive added another static method for overriding the reason description.


              What do you think about making it non-enum?

              "ataylor" wrote:
              "trustin" wrote:
              5) What about renaming HttpProtocol to HttpVersion? P of HTTP already stands for 'protocol'. Also, HttpRequest.get/setProtocol() needs to be renamed to get/setVersion() or get/setProtocolVersion().


              I agree, done.


              I ran svn up, but it's not showing up.

              "ataylor" wrote:
              "trustin" wrote:
              6) java.net.URI already has a query property. I think we don't need the parameter accessor methods in the HttpRequest interface. We could provide a helper class like UriBuilder later.


              not sure i follow completely, i need the URI for encoding purposes so i need an accessor. Unless you meant replace it with a String?


              When you create a new URI instance, you can specify query parameters and get them by calling URI.getQuery(). Of course, it's not a convenient form and it requires additional processing. My point is we don't need to hold duplicate information though. We could provide two helper classes:

              a) UriBuilder - helps a user to build a URI easily.

              UriBuilder ub = new UriBuilder("http://www.jboss.org/hello");
              ub.addParameter("name", "Andy");
              URI uri = ub.toUri();

              b) UriQueryDecoder - helps a user to decode the query string

              UriQueryDecoder uqd = new UriQueryDecoder(uri);
              uqd.getParameter("name");


              • 19. Re: Netty HTTP transport
                trustin

                 

                "trustin" wrote:
                I ran svn up, but it's not showing up.

                Eclipse didn't refresh my workspace. It's there. Sorry for the false alarm. :)

                • 20. Re: Netty HTTP transport
                  ataylor

                   

                  I forgot to mention that the header name is case insensitive. TreeMap with a custom Comparator should be used perhaps?


                  ok, cool.


                  Commas are interpreted as header value separators only when explicitly specified in the HTTP specification AFAIK. I think we don't need to take care of commas and left it to the user application.


                  Ok, so all i have to cope with is multiline headers?

                  What do you think about making it non-enum?


                  fine with me, I'll change it

                  When you create a new URI instance, you can specify query parameters and get them by calling URI.getQuery(). Of course, it's not a convenient form and it requires additional processing. My point is we don't need to hold duplicate information though. We could provide two helper classes:

                  a) UriBuilder - helps a user to build a URI easily.

                  UriBuilder ub = new UriBuilder("http://www.jboss.org/hello");
                  ub.addParameter("name", "Andy");
                  URI uri = ub.toUri();

                  b) UriQueryDecoder - helps a user to decode the query string

                  UriQueryDecoder uqd = new UriQueryDecoder(uri);
                  uqd.getParameter("name");


                  Ok, so I'll add these and then use them in decode/encode?

                  • 21. Re: Netty HTTP transport
                    trustin

                     

                    "ataylor" wrote:
                    Ok, so all i have to cope with is multiline headers?

                    Yes.
                    "ataylor" wrote:
                    Ok, so I'll add these and then use them in decode/encode?

                    That would be great! :)

                    • 22. Re: Netty HTTP transport
                      ataylor

                      Trustin, Ive made you're suggested changes and checked them into the branch.

                      I'll catch you tomorrow and we can catch up and decide what to do next.

                      • 23. Re: Netty HTTP transport
                        trustin

                        I have reviewed the code and made some quick modification by myself (mostly renaming and access modifier changes):

                        http://fisheye.jboss.org/changelog/Netty/branches/Branch_HTTP

                        I think there's only a couple issues left now.

                        1) What I meant was actually that HttpRequest.getUri() should be retained and all parameter accessor methods need to be removed. Then, HttpRequestEncoder and HttpRequestDecoder won't need to build or decode an URI at all. Just calling URI.toString() and new URI(String) would be enough.

                        Now assuming that we don't have any parameter accessor methods but URI property, how do we encode / decode a query in JBM? There are a few choices:

                        1a) Use GET method and append a usual query string to the URI. The query string is encoded by QueryStringEncoder, which replaces UriBuilder.
                        1b) Use POST method and put the query string into the content of the HttpMessage. The query string is encoded by QueryStringEncoder, which replaces UriBuilder.
                        1c) Use REST style URI and use regular expression matching. (No need for UriBuilder, UriQueryDecoder, etc)

                        QueryStringEncoder should be pretty much same with UriBuilder except that it builds a query string part only - it's just renaming and removing several lines.

                        Also, UriQueryDecoder should be renamed to QueryStringDecoder and modified to start to decode the specified String or URI from the first occurrence of '?'.

                        The removal of the parameter accessor methods makes HttpRequest closer to the original HTTP request message model specified by the relevant standards. There's no restriction in how a query string should look like, and we can't assume that a query string always exists in the URI - it can even exist in the content in case of POST, and actually it's entirely up to how a user wants to use HTTP. That's why I'd like to isolate query string (or parameter) processing from the core classes. A user can always use utility classes (QueryStringEncoder/Decoder) if necessary.

                        2) (optional - UTF-8 only support is fine at this moment) QueryStringEncoder and QueryStringDecoder should consider charsets. A sensible default could be UTF-8, but some people might want something like EUC-JP. Adding an optional charset parameter to the constructor would be just fine.

                        Please let me know if I am missing something.

                        • 24. Re: Netty HTTP transport
                          ataylor

                          Ok, Trustin.

                          So I'll change the URIEncoder/Decoder to just be QueryStringEncoder/Decoder and then the request just deals with a URI. I'll also add charset support as well.

                          With regard to JBM we'll be using the POST method, however i'm not sure whether we need a query string at all, i guess it'll just be the original buffer as the content.

                          • 25. Re: Netty HTTP transport
                            ataylor

                            Thats all done now Trustin, Ive left the charset encoding for you to do at a later date.

                            • 26. Re: Netty HTTP transport
                              trustin

                              Looks great! I've just merged it into the trunk. Let me release 3.1.0.ALPHA1 which contains the HTTP codec + some demanded features this week. Thank you for your effort. :)

                              1 2 Previous Next