Skip to content

Commit

Permalink
google_apis: Remove a hard-coded URL hack from HttpRequestParser
Browse files Browse the repository at this point in the history
"http://localhost/" was hard-coded in HttpRequestParser, but this was wrong
as the server runs at a different host name and a port.

Along the way, add a test for HttpServer::RegisterDefaultResponse, which was
missing. Also add the request URL to warning messages for 404 requests.

BUG=none
TEST=none

Review URL: https://codereview.chromium.org/11280156

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@169576 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
satorux@chromium.org committed Nov 27, 2012
1 parent 67d0e8a commit 756c6fa
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 8 deletions.
5 changes: 3 additions & 2 deletions chrome/browser/google_apis/test_server/http_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,9 @@ HttpRequestParser::ParseResult HttpRequestParser::ParseHeaders() {
http_request_->method = GetMethodType(StringToLowerASCII(
header_line_tokens[0]));
// Address.
const GURL host = GURL("http://localhost/");
http_request_->url = host.Resolve(header_line_tokens[1]);
// Don't build an absolute URL as the parser does not know (should not
// know) anything about the server address.
http_request_->relative_url = header_line_tokens[1];
// Protocol.
const std::string protocol = StringToLowerASCII(header_line_tokens[2]);
CHECK(protocol == "http/1.0" || protocol == "http/1.1") <<
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/google_apis/test_server/http_request.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ struct HttpRequest {
HttpRequest();
~HttpRequest();

GURL url;
std::string relative_url; // Starts with '/'. Example: "/test?query=foo"
HttpMethod method;
std::map<std::string, std::string> headers;
std::string content;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ TEST(HttpRequestTest, ParseRequest) {
// Fetch the first request and validate it.
{
scoped_ptr<HttpRequest> request = parser.GetRequest();
EXPECT_EQ("http://localhost/foobar.html", request->url.spec());
EXPECT_EQ("/foobar.html", request->relative_url);
EXPECT_EQ(METHOD_POST, request->method);
EXPECT_EQ("1234567890", request->content);
ASSERT_EQ(3u, request->headers.size());
Expand Down
9 changes: 5 additions & 4 deletions chrome/browser/google_apis/test_server/http_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ const int kRetries = 10;
scoped_ptr<HttpResponse> HandleDefaultRequest(const GURL& url,
const HttpResponse& response,
const HttpRequest& request) {
if (url.path() != request.url.path())
const GURL request_url = url.Resolve(request.relative_url);
if (url.path() != request_url.path())
return scoped_ptr<HttpResponse>(NULL);
return scoped_ptr<HttpResponse>(new HttpResponse(response));
}
Expand Down Expand Up @@ -136,7 +137,8 @@ void HttpServer::HandleRequest(HttpConnection* connection,
}
}

LOG(WARNING) << "Request not handled. Returning 404.";
LOG(WARNING) << "Request not handled. Returning 404: "
<< request->relative_url;
scoped_ptr<HttpResponse> not_found_response(new HttpResponse());
not_found_response->set_code(NOT_FOUND);
connection->SendResponse(not_found_response.Pass());
Expand Down Expand Up @@ -169,10 +171,9 @@ void HttpServer::RegisterDefaultResponse(
DCHECK(StartsWithASCII(relative_path, "/", true /* case_sensitive */))
<< relative_path;

GURL request_url = base_url_.Resolve(relative_path);
const HandleRequestCallback callback =
base::Bind(&HandleDefaultRequest,
request_url,
GetURL(relative_path),
default_response);
request_handlers_.push_back(callback);
}
Expand Down
33 changes: 33 additions & 0 deletions chrome/browser/google_apis/test_server/http_server_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,22 @@ class HttpServerTest : public testing::Test,
message_loop_.Run(); // Will be terminated in OnURLFetchComplete().
}

// Handles the request and returns a simple text content. Saves the
// request URL for verification.
scoped_ptr<HttpResponse> HandleRequest(const HttpRequest& request) {
request_relative_url_ = request.relative_url;

scoped_ptr<HttpResponse> http_response(new HttpResponse);
http_response->set_code(SUCCESS);
http_response->set_content("<b>Worked!</b>");
http_response->set_content_type("text/html");
return http_response.Pass();
}

protected:
int num_responses_received_;
int num_responses_expected_;
std::string request_relative_url_;
MessageLoopForUI message_loop_;
content::TestBrowserThread ui_thread_;
content::TestBrowserThread io_thread_;
Expand All @@ -100,6 +113,26 @@ TEST_F(HttpServerTest, GetURL) {
server_.GetURL("/path?query=foo").spec());
}

TEST_F(HttpServerTest, RegisterRequestHandler) {
server_.RegisterRequestHandler(base::Bind(&HttpServerTest::HandleRequest,
base::Unretained(this)));

scoped_ptr<net::URLFetcher> fetcher(
net::URLFetcher::Create(server_.GetURL("/test?q=foo"),
net::URLFetcher::GET,
this));
fetcher->SetRequestContext(request_context_getter_.get());
fetcher->Start();
WaitForResponses(1);

EXPECT_EQ(net::URLRequestStatus::SUCCESS, fetcher->GetStatus().status());
EXPECT_EQ(SUCCESS, fetcher->GetResponseCode());
EXPECT_EQ("<b>Worked!</b>", GetContentFromFetcher(*fetcher));
EXPECT_EQ("text/html", GetContentTypeFromFetcher(*fetcher));

EXPECT_EQ("/test?q=foo", request_relative_url_);
}

TEST_F(HttpServerTest, RegisterDefaultResponse) {
HttpResponse http_response;
// MOVED is chosen here, as it's rather an unusual code.
Expand Down

0 comments on commit 756c6fa

Please sign in to comment.