Consolidate attachment URL construction.

Fix bug where attachment URLs could end up with too many slashes.

Relanding issue 355093002

BUG=

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@280384 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 80 insertions, 38 deletions

diff --git a/sync/api/attachments/attachment_downloader.cc b/sync/api/attachments/attachment_downloader.cc
index 7fd32cd..8b07ab9 100644
--- a/sync/api/attachments/attachment_downloader.cc
+++ b/sync/api/attachments/attachment_downloader.cc
@@ -15,7 +15,7 @@ AttachmentDownloader::~AttachmentDownloader() {
 // It is introduced to avoid SYNC_EXPORT-ing AttachmentDownloaderImpl since it
 // inherits from OAuth2TokenService::Consumer which is not exported.
 scoped_ptr<AttachmentDownloader> AttachmentDownloader::Create(
-    const std::string& url_prefix,
+    const GURL& sync_service_url,
     const scoped_refptr<net::URLRequestContextGetter>&
         url_request_context_getter,
     const std::string& account_id,
@@ -23,7 +23,7 @@ scoped_ptr<AttachmentDownloader> AttachmentDownloader::Create(
     scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
         token_service_provider) {
   return scoped_ptr<AttachmentDownloader>(
-      new AttachmentDownloaderImpl(url_prefix,
+      new AttachmentDownloaderImpl(sync_service_url,
                                    url_request_context_getter,
                                    account_id,
                                    scopes,
diff --git a/sync/api/attachments/attachment_downloader.h b/sync/api/attachments/attachment_downloader.h
index 718284c..b2b2037 100644
--- a/sync/api/attachments/attachment_downloader.h
+++ b/sync/api/attachments/attachment_downloader.h
@@ -41,8 +41,7 @@ class SYNC_EXPORT AttachmentDownloader {
                                   const DownloadCallback& callback) = 0;
 
   // Create instance of AttachmentDownloaderImpl.
-  // |url_prefix| is the URL prefix (including trailing slash) to be used when
-  // downloading attachments.
+  // |sync_service_url| is the URL of the sync service.
   //
   // |url_request_context_getter| provides a URLRequestContext.
   //
@@ -52,7 +51,7 @@ class SYNC_EXPORT AttachmentDownloader {
   //
   // |token_service_provider| provides an OAuth2 token service.
   static scoped_ptr<AttachmentDownloader> Create(
-      const std::string& url_prefix,
+      const GURL& sync_service_url,
       const scoped_refptr<net::URLRequestContextGetter>&
           url_request_context_getter,
       const std::string& account_id,
diff --git a/sync/internal_api/attachments/attachment_downloader_impl.cc b/sync/internal_api/attachments/attachment_downloader_impl.cc
index 18c49d8..740c4fa 100644
--- a/sync/internal_api/attachments/attachment_downloader_impl.cc
+++ b/sync/internal_api/attachments/attachment_downloader_impl.cc
@@ -9,6 +9,7 @@
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_fetcher.h"
+#include "sync/internal_api/public/attachments/attachment_uploader_impl.h"
 #include "sync/protocol/sync.pb.h"
 #include "url/gurl.h"
 
@@ -35,7 +36,7 @@ AttachmentDownloaderImpl::DownloadState::DownloadState(
 }
 
 AttachmentDownloaderImpl::AttachmentDownloaderImpl(
-    const std::string& url_prefix,
+    const GURL& sync_service_url,
     const scoped_refptr<net::URLRequestContextGetter>&
         url_request_context_getter,
     const std::string& account_id,
@@ -43,14 +44,13 @@ AttachmentDownloaderImpl::AttachmentDownloaderImpl(
     scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
         token_service_provider)
     : OAuth2TokenService::Consumer("attachment-downloader-impl"),
-      url_prefix_(url_prefix),
+      sync_service_url_(sync_service_url),
       url_request_context_getter_(url_request_context_getter),
       account_id_(account_id),
       oauth2_scopes_(scopes),
       token_service_provider_(token_service_provider.Pass()) {
   DCHECK(token_service_provider_);
   DCHECK(url_request_context_getter_);
-  DCHECK(!url_prefix_.empty());
 }
 
 AttachmentDownloaderImpl::~AttachmentDownloaderImpl() {
@@ -61,7 +61,8 @@ void AttachmentDownloaderImpl::DownloadAttachment(
     const DownloadCallback& callback) {
   DCHECK(CalledOnValidThread());
 
-  AttachmentUrl url = GetAttachmentUrl(attachment_id);
+  AttachmentUrl url = AttachmentUploaderImpl::GetURLForAttachmentId(
+                          sync_service_url_, attachment_id).spec();
 
   StateMap::iterator iter = state_map_.find(url);
   if (iter == state_map_.end()) {
@@ -151,11 +152,6 @@ void AttachmentDownloaderImpl::OnURLFetchComplete(
   state_map_.erase(iter);
 }
 
-AttachmentDownloaderImpl::AttachmentUrl
-AttachmentDownloaderImpl::GetAttachmentUrl(const AttachmentId& attachment_id) {
-  return url_prefix_ + attachment_id.GetProto().unique_id();
-}
-
 scoped_ptr<net::URLFetcher> AttachmentDownloaderImpl::CreateFetcher(
     const AttachmentUrl& url,
     const std::string& access_token) {
diff --git a/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc b/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc
index f8d1393..b8938cd 100644
--- a/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc
+++ b/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc
@@ -187,7 +187,7 @@ void AttachmentDownloaderImplTest::SetUp() {
   OAuth2TokenService::ScopeSet scopes;
   scopes.insert(GaiaConstants::kChromeSyncOAuth2Scope);
   attachment_downloader_ =
-      AttachmentDownloader::Create(kAttachmentServerUrl,
+      AttachmentDownloader::Create(GURL(kAttachmentServerUrl),
                                    url_request_context_getter_,
                                    kAccountId,
                                    scopes,
diff --git a/sync/internal_api/attachments/attachment_uploader_impl.cc b/sync/internal_api/attachments/attachment_uploader_impl.cc
index 90c620e5..9048b20 100644
--- a/sync/internal_api/attachments/attachment_uploader_impl.cc
+++ b/sync/internal_api/attachments/attachment_uploader_impl.cc
@@ -14,11 +14,11 @@
 #include "net/url_request/url_fetcher_delegate.h"
 #include "sync/api/attachments/attachment.h"
 #include "sync/protocol/sync.pb.h"
-#include "url/gurl.h"
 
 namespace {
 
 const char kContentType[] = "application/octet-stream";
+const char kAttachments[] = "attachments/";
 
 }  // namespace
 
@@ -209,14 +209,14 @@ void AttachmentUploaderImpl::UploadState::ReportResult(
 }
 
 AttachmentUploaderImpl::AttachmentUploaderImpl(
-    const std::string& url_prefix,
+    const GURL& sync_service_url,
     const scoped_refptr<net::URLRequestContextGetter>&
         url_request_context_getter,
     const std::string& account_id,
     const OAuth2TokenService::ScopeSet& scopes,
     scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
         token_service_provider)
-    : url_prefix_(url_prefix),
+    : sync_service_url_(sync_service_url),
       url_request_context_getter_(url_request_context_getter),
       account_id_(account_id),
       scopes_(scopes),
@@ -237,7 +237,7 @@ void AttachmentUploaderImpl::UploadAttachment(const Attachment& attachment,
   DCHECK(!unique_id.empty());
   StateMap::iterator iter = state_map_.find(unique_id);
   if (iter == state_map_.end()) {
-    const GURL url = GetUploadURLForAttachmentId(attachment_id);
+    const GURL url = GetURLForAttachmentId(sync_service_url_, attachment_id);
     scoped_ptr<UploadState> upload_state(
         new UploadState(url,
                         url_request_context_getter_,
@@ -256,9 +256,19 @@ void AttachmentUploaderImpl::UploadAttachment(const Attachment& attachment,
   }
 }
 
-GURL AttachmentUploaderImpl::GetUploadURLForAttachmentId(
-    const AttachmentId& attachment_id) const {
-  return GURL(url_prefix_ + attachment_id.GetProto().unique_id());
+// Static.
+GURL AttachmentUploaderImpl::GetURLForAttachmentId(
+    const GURL& sync_service_url,
+    const AttachmentId& attachment_id) {
+  std::string path = sync_service_url.path();
+  if (path.empty() || *path.rbegin() != '/') {
+    path += '/';
+  }
+  path += kAttachments;
+  path += attachment_id.GetProto().unique_id();
+  GURL::Replacements replacements;
+  replacements.SetPathStr(path);
+  return sync_service_url.ReplaceComponents(replacements);
 }
 
 void AttachmentUploaderImpl::DeleteUploadStateFor(const UniqueId& unique_id) {
diff --git a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
index 3e3c67e..90cb07d 100644
--- a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
+++ b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
@@ -33,6 +33,7 @@ const char kAttachmentData[] = "some data";
 const char kAccountId[] = "some-account-id";
 const char kAccessToken[] = "some-access-token";
 const char kAuthorization[] = "Authorization";
+const char kAttachments[] = "/attachments/";
 
 }  // namespace
 
@@ -264,8 +265,7 @@ void AttachmentUploaderImplTest::SetUp() {
       base::Bind(&RequestHandler::HandleRequest,
                  base::Unretained(request_handler_.get())));
 
-  std::string url_prefix(
-      base::StringPrintf("http://localhost:%d/uploads/", server_.port()));
+  GURL url(base::StringPrintf("http://localhost:%d/", server_.port()));
 
   token_service_.reset(new MockOAuth2TokenService);
   scoped_ptr<OAuth2TokenServiceRequest::TokenServiceProvider>
@@ -273,7 +273,7 @@ void AttachmentUploaderImplTest::SetUp() {
 
   OAuth2TokenService::ScopeSet scopes;
   scopes.insert(GaiaConstants::kChromeSyncOAuth2Scope);
-  uploader().reset(new AttachmentUploaderImpl(url_prefix,
+  uploader().reset(new AttachmentUploaderImpl(url,
                                               url_request_context_getter_,
                                               kAccountId,
                                               scopes,
@@ -377,6 +377,42 @@ net::HttpStatusCode RequestHandler::GetStatusCode() const {
   return status_code_;
 }
 
+TEST_F(AttachmentUploaderImplTest, GetURLForAttachmentId_NoPath) {
+  AttachmentId id = AttachmentId::Create();
+  std::string unique_id = id.GetProto().unique_id();
+  GURL sync_service_url("https://example.com");
+  EXPECT_EQ("https://example.com/attachments/" + unique_id,
+            AttachmentUploaderImpl::GetURLForAttachmentId(sync_service_url, id)
+                .spec());
+}
+
+TEST_F(AttachmentUploaderImplTest, GetURLForAttachmentId_JustSlash) {
+  AttachmentId id = AttachmentId::Create();
+  std::string unique_id = id.GetProto().unique_id();
+  GURL sync_service_url("https://example.com/");
+  EXPECT_EQ("https://example.com/attachments/" + unique_id,
+            AttachmentUploaderImpl::GetURLForAttachmentId(sync_service_url, id)
+                .spec());
+}
+
+TEST_F(AttachmentUploaderImplTest, GetURLForAttachmentId_Path) {
+  AttachmentId id = AttachmentId::Create();
+  std::string unique_id = id.GetProto().unique_id();
+  GURL sync_service_url("https://example.com/service");
+  EXPECT_EQ("https://example.com/service/attachments/" + unique_id,
+            AttachmentUploaderImpl::GetURLForAttachmentId(sync_service_url, id)
+                .spec());
+}
+
+TEST_F(AttachmentUploaderImplTest, GetURLForAttachmentId_PathAndSlash) {
+  AttachmentId id = AttachmentId::Create();
+  std::string unique_id = id.GetProto().unique_id();
+  GURL sync_service_url("https://example.com/service/");
+  EXPECT_EQ("https://example.com/service/attachments/" + unique_id,
+            AttachmentUploaderImpl::GetURLForAttachmentId(sync_service_url, id)
+                .spec());
+}
+
 // Verify the "happy case" of uploading an attachment.
 //
 // Token is requested, token is returned, HTTP request is made, attachment is
@@ -403,7 +439,7 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_HappyCase) {
   ASSERT_EQ(1U, http_requests_received().size());
   const HttpRequest& http_request = http_requests_received().front();
   EXPECT_EQ(net::test_server::METHOD_POST, http_request.method);
-  std::string expected_relative_url("/uploads/" +
+  std::string expected_relative_url(kAttachments +
                                     attachment.GetId().GetProto().unique_id());
   EXPECT_EQ(expected_relative_url, http_request.relative_url);
   EXPECT_TRUE(http_request.has_content);
@@ -506,7 +542,7 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_ServiceUnavilable) {
   ASSERT_EQ(1U, http_requests_received().size());
   const HttpRequest& http_request = http_requests_received().front();
   EXPECT_EQ(net::test_server::METHOD_POST, http_request.method);
-  std::string expected_relative_url("/uploads/" +
+  std::string expected_relative_url(kAttachments +
                                     attachment.GetId().GetProto().unique_id());
   EXPECT_EQ(expected_relative_url, http_request.relative_url);
   EXPECT_TRUE(http_request.has_content);
@@ -544,7 +580,7 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_BadToken) {
   ASSERT_EQ(1U, http_requests_received().size());
   const HttpRequest& http_request = http_requests_received().front();
   EXPECT_EQ(net::test_server::METHOD_POST, http_request.method);
-  std::string expected_relative_url("/uploads/" +
+  std::string expected_relative_url(kAttachments +
                                     attachment.GetId().GetProto().unique_id());
   EXPECT_EQ(expected_relative_url, http_request.relative_url);
   EXPECT_TRUE(http_request.has_content);
diff --git a/sync/internal_api/public/attachments/attachment_downloader_impl.h b/sync/internal_api/public/attachments/attachment_downloader_impl.h
index e3fee399e..3a125c0 100644
--- a/sync/internal_api/public/attachments/attachment_downloader_impl.h
+++ b/sync/internal_api/public/attachments/attachment_downloader_impl.h
@@ -11,6 +11,7 @@
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "sync/api/attachments/attachment_downloader.h"
+#include "url/gurl.h"
 
 namespace syncer {
 
@@ -20,8 +21,7 @@ class AttachmentDownloaderImpl : public AttachmentDownloader,
                                  public net::URLFetcherDelegate,
                                  public base::NonThreadSafe {
  public:
-  // |url_prefix| is the URL prefix (including trailing slash) to be used when
-  // downloading attachments.
+  // |sync_service_url| is the URL of the sync service.
   //
   // |url_request_context_getter| provides a URLRequestContext.
   //
@@ -31,7 +31,7 @@ class AttachmentDownloaderImpl : public AttachmentDownloader,
   //
   // |token_service_provider| provides an OAuth2 token service.
   AttachmentDownloaderImpl(
-      const std::string& url_prefix,
+      const GURL& sync_service_url,
       const scoped_refptr<net::URLRequestContextGetter>&
           url_request_context_getter,
       const std::string& account_id,
@@ -60,7 +60,6 @@ class AttachmentDownloaderImpl : public AttachmentDownloader,
   typedef base::ScopedPtrHashMap<AttachmentUrl, DownloadState> StateMap;
   typedef std::vector<DownloadState*> StateList;
 
-  AttachmentUrl GetAttachmentUrl(const AttachmentId& attachment_id);
   scoped_ptr<net::URLFetcher> CreateFetcher(const AttachmentUrl& url,
                                             const std::string& access_token);
   void RequestAccessToken(DownloadState* download_state);
@@ -69,7 +68,7 @@ class AttachmentDownloaderImpl : public AttachmentDownloader,
       const DownloadResult& result,
       const scoped_refptr<base::RefCountedString>& attachment_data);
 
-  std::string url_prefix_;
+  GURL sync_service_url_;
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
 
   std::string account_id_;
diff --git a/sync/internal_api/public/attachments/attachment_uploader_impl.h b/sync/internal_api/public/attachments/attachment_uploader_impl.h
index 8604415..25394fc 100644
--- a/sync/internal_api/public/attachments/attachment_uploader_impl.h
+++ b/sync/internal_api/public/attachments/attachment_uploader_impl.h
@@ -23,8 +23,7 @@ namespace syncer {
 class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader,
                                            public base::NonThreadSafe {
  public:
-  // |url_prefix| is the URL prefix (including trailing slash) to be used when
-  // uploading attachments.
+  // |sync_service_url| is the URL of the sync service.
   //
   // |url_request_context_getter| provides a URLRequestContext.
   //
@@ -34,7 +33,7 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader,
   //
   // |token_service_provider| provides an OAuth2 token service.
   AttachmentUploaderImpl(
-      const std::string& url_prefix,
+      const GURL& sync_service_url,
       const scoped_refptr<net::URLRequestContextGetter>&
           url_request_context_getter,
       const std::string& account_id,
@@ -47,15 +46,18 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader,
   virtual void UploadAttachment(const Attachment& attachment,
                                 const UploadCallback& callback) OVERRIDE;
 
+  // Return the URL for the given |sync_service_url| and |attachment_id|.
+  static GURL GetURLForAttachmentId(const GURL& sync_service_url,
+                                    const AttachmentId& attachment_id);
+
  private:
   class UploadState;
   typedef std::string UniqueId;
   typedef base::ScopedPtrHashMap<UniqueId, UploadState> StateMap;
 
-  GURL GetUploadURLForAttachmentId(const AttachmentId& attachment_id) const;
   void DeleteUploadStateFor(const UniqueId& unique_id);
 
-  std::string url_prefix_;
+  GURL sync_service_url_;
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
   std::string account_id_;
   OAuth2TokenService::ScopeSet scopes_;