search

boblemaire / asyncHTTPrequest

asynchronous HTTP for ESP using ESPasyncTCP. Works like XMLHTTPrequest in JS.
GNU General Public License v3.0
64 stars 31 forks source link

Proper way to send HTTP request and fix schema #2

Closed fmalekpour closed 6 years ago

boblemaire commented 6 years ago

Not much here by way of explanation and/or justification for these changes. You may well be right that it is the "proper" way, but I'll need some reference to an authority to understand your intent. I do have a few comments:

I don't have a problem with accepting the http:// in lower case, but I might do it slightly differently: Instead of if(url.toUpperCase().startsWith("HTTP://")) I might use something likeif(url.substring(0,7).equalsIgnoreCase("HTTP://")) The rational is that the URL at this point can be very large and replicating the entire String and converting to upper would use a lot of heap and require a lot of needless processing. Better to lop off the first 7 characters we are interested in.

The changes to buildRequest are a little more problematic. I'm curious about the test case that you used to produce a request that lacked a host header, which should be the first header in all requests, and how the header that you added can work without the colon separator (maybe because it's followed by a correct host header?)

The other thing I'm curious about is what HTTP/1.1 scenario the absoluteURI will not be acceptable.

Kindly cite the applicable RFC standards as I'm not really well versed in this and it would help greatly in understanding this.

fmalekpour commented 6 years ago

I was working on a project that needed HTTP async operation. asyncHTTPrequest was the perfect fit except it didn't work for a url that has query strings like http://www.domain.com/path?arg=value. So I made changes and it works fine now. You can discard the pull request if you think it's not standard or useful.

Ref: https://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html

boblemaire commented 6 years ago

I can understand the problem with the lower case http://, and that change is fine. I don't agree with the general assertion that it doesn't work with query strings. My stuff works with query strings, so there's at least some kind of differentiation. There is no difference in the way the path and query string are assembled in your version. The difference is that you use absolute-path in the request-URI instead of the absolute-URI. Absolute-path is a subset of absolute-URI.

As I understand it, using absolute-URI is required in HTTP/1.1 when using a proxy, and all servers using HTTP/1.1 are required to accept them. So using absolute-URI should work in all cases, proxy or not, while absolute-URI may not work on some proxies.

I'd be willing to look at the failure that you encountered and show you how to use the builtin debug facilities to isolate the problem, and maybe that results in fixing a bug, but your changes clearly have not been researched enough to make this serious change to the request format.