Improve BasicHttpFetcher uri handling

BasicHttpFetcher use apache HttpClient.
Which doesn't handle cleanly uris, for example underscores in url.

The change pass the url already parsed to HttpClient.

[zhoresh, 2010-02-10 18:48:19]

Fix error handling and add test cases

[zhoresh, 2010-02-10 18:52:40]

The change didn't change return for errors, just handle better few cases.

But is 500 the right error here, or should it be a user error?
Also should the error message be sent back as well instead of empty message?

[zhoresh, 2010-02-11 00:23:54]

Improve html error code handling

[zhoresh, 2010-02-11 00:28:21]

Last change separate shindig error (Exception) from external error (error
respond) in the fetcher.
Also it handle error code of "internal error" from external system, and convert
to gateway error before sending to user.

The point is that we will be able to track in our log what are really shindig
errors verses external system errors.

[johnfargo, 2010-02-11 23:25:58]

Rietveld is giving me all sorts of problems, so I'll paste my comments here.

1. [extremely minor] Rename gmodules.com to apache.org or example.com in
tests.
2. Does the new HttpFetcher pipeline work when
StringUtils.isEmpty(uri.getScheme()) ie. for schemaless requests? I'd be
interested in a quick test.

Past that, it's looking great. I'll apply ASAP after hearing about #2.

--j

On Wed, Feb 10, 2010 at 4:28 PM, <zhoresh@gmail.com> wrote:

> Last change separate shindig error (Exception) from external error
> (error respond) in the fetcher.
> Also it handle error code of "internal error" from external system, and
> convert to gateway error before sending to user.
>
> The point is that we will be able to track in our log what are really
> shindig errors verses external system errors.
>
>
>
>
> http://codereview.appspot.com/206057/show
>

[zhoresh, 2010-02-12 00:19:30]

Add check for null schema

[zhoresh, 2010-02-12 00:21:44]

The original check would return 500 (catch NPE in HttpGet constructor). So I
added a check and a test.

On 2010/02/11 23:25:58, johnfargo wrote:
> 1. [extremely minor] Rename http://gmodules.com to http://apache.org or
http://example.com in
> tests.
> 2. Does the new HttpFetcher pipeline work when
> StringUtils.isEmpty(uri.getScheme()) ie. for schemaless requests? I'd be
> interested in a quick test.

[zhoresh, 2010-02-12 00:49:05]

Merge code with the latest

[johnfargo, 2010-02-12 00:56:53]

Patch committed.

On Thu, Feb 11, 2010 at 4:49 PM, <zhoresh@gmail.com> wrote:

> Merge code with the latest
>
>
> http://codereview.appspot.com/206057/show
>