From 9c0ced9ccfcbcaed3d9691042aee1e79112a06be Mon Sep 17 00:00:00 2001 From: maniscalco Date: Thu, 11 Dec 2014 09:45:32 -0800 Subject: Include model type in attachment uploads and downloads Pass model type using EntitySpecifics field number in HTTP header on attachment uploads and downloads. We use the field number because that's a sync-standard way of communicating model type to the server. Update comment for GetSpecificsFieldNumberFromModelType to reflect the fact that this method is use outside of test code. BUG=440928 Review URL: https://codereview.chromium.org/785063005 Cr-Commit-Position: refs/heads/master@{#307923} --- .../attachments/attachment_downloader.cc | 5 ++-- .../attachments/attachment_downloader_impl.cc | 8 ++++-- .../attachment_downloader_impl_unittest.cc | 4 ++- .../attachments/attachment_uploader_impl.cc | 32 ++++++++++++++++------ .../attachment_uploader_impl_unittest.cc | 11 +++++++- .../public/attachments/attachment_downloader.h | 6 +++- .../attachments/attachment_downloader_impl.h | 8 +++++- .../public/attachments/attachment_uploader_impl.h | 8 +++++- sync/internal_api/public/base/model_type.h | 2 -- 9 files changed, 63 insertions(+), 21 deletions(-) (limited to 'sync') diff --git a/sync/internal_api/attachments/attachment_downloader.cc b/sync/internal_api/attachments/attachment_downloader.cc index fb60201..5b52eb0 100644 --- a/sync/internal_api/attachments/attachment_downloader.cc +++ b/sync/internal_api/attachments/attachment_downloader.cc @@ -22,10 +22,11 @@ scoped_ptr AttachmentDownloader::Create( const OAuth2TokenService::ScopeSet scopes, const scoped_refptr& token_service_provider, - const std::string& store_birthday) { + const std::string& store_birthday, + ModelType model_type) { return scoped_ptr(new AttachmentDownloaderImpl( sync_service_url, url_request_context_getter, account_id, scopes, - token_service_provider, store_birthday)); + token_service_provider, store_birthday, model_type)); } } // namespace syncer diff --git a/sync/internal_api/attachments/attachment_downloader_impl.cc b/sync/internal_api/attachments/attachment_downloader_impl.cc index 9e6c488..33ac885 100644 --- a/sync/internal_api/attachments/attachment_downloader_impl.cc +++ b/sync/internal_api/attachments/attachment_downloader_impl.cc @@ -51,14 +51,16 @@ AttachmentDownloaderImpl::AttachmentDownloaderImpl( const OAuth2TokenService::ScopeSet& scopes, const scoped_refptr& token_service_provider, - const std::string& store_birthday) + const std::string& store_birthday, + ModelType model_type) : OAuth2TokenService::Consumer("attachment-downloader-impl"), 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), - raw_store_birthday_(store_birthday) { + raw_store_birthday_(store_birthday), + model_type_(model_type) { DCHECK(url_request_context_getter_.get()); DCHECK(!account_id.empty()); DCHECK(!scopes.empty()); @@ -194,7 +196,7 @@ scoped_ptr AttachmentDownloaderImpl::CreateFetcher( scoped_ptr url_fetcher( net::URLFetcher::Create(GURL(url), net::URLFetcher::GET, this)); AttachmentUploaderImpl::ConfigureURLFetcherCommon( - url_fetcher.get(), access_token, raw_store_birthday_, + url_fetcher.get(), access_token, raw_store_birthday_, model_type_, url_request_context_getter_.get()); return url_fetcher.Pass(); } diff --git a/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc b/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc index 501e01f..96ea5ea 100644 --- a/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc +++ b/sync/internal_api/attachments/attachment_downloader_impl_unittest.cc @@ -18,6 +18,7 @@ #include "sync/api/attachments/attachment.h" #include "sync/internal_api/public/attachments/attachment_uploader_impl.h" #include "sync/internal_api/public/attachments/attachment_util.h" +#include "sync/internal_api/public/base/model_type.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/leveldatabase/src/util/crc32c.h" @@ -30,6 +31,7 @@ const char kAccessToken[] = "access.token"; const char kAttachmentServerUrl[] = "http://attachments.com/"; const char kAttachmentContent[] = "attachment.content"; const char kStoreBirthday[] = "z00000000-0000-007b-0000-0000000004d2"; +const syncer::ModelType kModelType = syncer::ModelType::ARTICLES; // MockOAuth2TokenService remembers last request for access token and verifies // that only one request is active at a time. @@ -206,7 +208,7 @@ void AttachmentDownloaderImplTest::SetUp() { scopes.insert(GaiaConstants::kChromeSyncOAuth2Scope); attachment_downloader_ = AttachmentDownloader::Create( GURL(kAttachmentServerUrl), url_request_context_getter_, kAccountId, - scopes, token_service_provider, std::string(kStoreBirthday)); + scopes, token_service_provider, std::string(kStoreBirthday), kModelType); } void AttachmentDownloaderImplTest::TearDown() { diff --git a/sync/internal_api/attachments/attachment_uploader_impl.cc b/sync/internal_api/attachments/attachment_uploader_impl.cc index 3819fec..e66330c 100644 --- a/sync/internal_api/attachments/attachment_uploader_impl.cc +++ b/sync/internal_api/attachments/attachment_uploader_impl.cc @@ -29,6 +29,7 @@ namespace { const char kContentType[] = "application/octet-stream"; const char kAttachments[] = "attachments/"; const char kSyncStoreBirthday[] = "X-Sync-Store-Birthday"; +const char kSyncDataTypeId[] = "X-Sync-Data-Type-Id"; } // namespace @@ -57,7 +58,8 @@ class AttachmentUploaderImpl::UploadState : public net::URLFetcherDelegate, const OAuth2TokenService::ScopeSet& scopes, OAuth2TokenServiceRequest::TokenServiceProvider* token_service_provider, const std::string& raw_store_birthday, - const base::WeakPtr& owner); + const base::WeakPtr& owner, + ModelType model_type); ~UploadState() override; @@ -108,6 +110,7 @@ class AttachmentUploaderImpl::UploadState : public net::URLFetcherDelegate, // Pointer to the AttachmentUploaderImpl that owns this object. base::WeakPtr owner_; scoped_ptr access_token_request_; + ModelType model_type_; DISALLOW_COPY_AND_ASSIGN(UploadState); }; @@ -122,7 +125,8 @@ AttachmentUploaderImpl::UploadState::UploadState( const OAuth2TokenService::ScopeSet& scopes, OAuth2TokenServiceRequest::TokenServiceProvider* token_service_provider, const std::string& raw_store_birthday, - const base::WeakPtr& owner) + const base::WeakPtr& owner, + ModelType model_type) : OAuth2TokenService::Consumer("attachment-uploader-impl"), is_stopped_(false), upload_url_(upload_url), @@ -133,7 +137,8 @@ AttachmentUploaderImpl::UploadState::UploadState( scopes_(scopes), raw_store_birthday_(raw_store_birthday), token_service_provider_(token_service_provider), - owner_(owner) { + owner_(owner), + model_type_(model_type) { DCHECK(upload_url_.is_valid()); DCHECK(url_request_context_getter_.get()); DCHECK(!account_id_.empty()); @@ -208,7 +213,7 @@ void AttachmentUploaderImpl::UploadState::OnGetTokenSuccess( fetcher_.reset( net::URLFetcher::Create(upload_url_, net::URLFetcher::POST, this)); ConfigureURLFetcherCommon(fetcher_.get(), access_token_, raw_store_birthday_, - url_request_context_getter_.get()); + model_type_, url_request_context_getter_.get()); const uint32_t crc32c = attachment_.GetCrc32c(); fetcher_->AddExtraRequestHeader(base::StringPrintf( @@ -273,13 +278,15 @@ AttachmentUploaderImpl::AttachmentUploaderImpl( const OAuth2TokenService::ScopeSet& scopes, const scoped_refptr& token_service_provider, - const std::string& store_birthday) + const std::string& store_birthday, + ModelType model_type) : sync_service_url_(sync_service_url), url_request_context_getter_(url_request_context_getter), account_id_(account_id), scopes_(scopes), token_service_provider_(token_service_provider), raw_store_birthday_(store_birthday), + model_type_(model_type), weak_ptr_factory_(this) { DCHECK(CalledOnValidThread()); DCHECK(!account_id.empty()); @@ -314,10 +321,10 @@ void AttachmentUploaderImpl::UploadAttachment(const Attachment& attachment, } const GURL url = GetURLForAttachmentId(sync_service_url_, attachment_id); - scoped_ptr upload_state( - new UploadState(url, url_request_context_getter_, attachment, callback, - account_id_, scopes_, token_service_provider_.get(), - raw_store_birthday_, weak_ptr_factory_.GetWeakPtr())); + scoped_ptr upload_state(new UploadState( + url, url_request_context_getter_, attachment, callback, account_id_, + scopes_, token_service_provider_.get(), raw_store_birthday_, + weak_ptr_factory_.GetWeakPtr(), model_type_)); state_map_.add(unique_id, upload_state.Pass()); } @@ -360,6 +367,7 @@ void AttachmentUploaderImpl::ConfigureURLFetcherCommon( net::URLFetcher* fetcher, const std::string& access_token, const std::string& raw_store_birthday, + ModelType model_type, net::URLRequestContextGetter* request_context_getter) { DCHECK(request_context_getter); DCHECK(fetcher); @@ -377,6 +385,12 @@ void AttachmentUploaderImpl::ConfigureURLFetcherCommon( Base64URLSafeEncode(raw_store_birthday, &encoded_store_birthday); fetcher->AddExtraRequestHeader(base::StringPrintf( "%s: %s", kSyncStoreBirthday, encoded_store_birthday.c_str())); + + // Use field number to pass ModelType because it's stable and we have server + // code to decode it. + const int field_number = GetSpecificsFieldNumberFromModelType(model_type); + fetcher->AddExtraRequestHeader( + base::StringPrintf("%s: %d", kSyncDataTypeId, field_number)); } void AttachmentUploaderImpl::Base64URLSafeEncode(const std::string& input, diff --git a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc index 0c5c45f..2daff32 100644 --- a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc +++ b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc @@ -11,6 +11,7 @@ #include "base/memory/ref_counted_memory.h" #include "base/message_loop/message_loop.h" #include "base/run_loop.h" +#include "base/strings/string_number_conversions.h" #include "base/strings/stringprintf.h" #include "base/synchronization/lock.h" #include "base/test/histogram_tester.h" @@ -27,6 +28,7 @@ #include "net/url_request/url_request_test_util.h" #include "sync/api/attachments/attachment.h" #include "sync/internal_api/public/attachments/attachment_util.h" +#include "sync/internal_api/public/base/model_type.h" #include "sync/protocol/sync.pb.h" #include "testing/gmock/include/gmock/gmock-matchers.h" #include "testing/gtest/include/gtest/gtest.h" @@ -46,6 +48,8 @@ const char kStoreBirthday[] = const char kBase64URLSafeStoreBirthday[] = "Rv_d4HQ6SP2dBpM8YY6lcAlZmQVhRiFhGwPTAmDNQVX-JhXXDA"; const char kSyncStoreBirthdayHeader[] = "X-Sync-Store-Birthday"; +const char kSyncDataTypeIdHeader[] = "X-Sync-Data-Type-Id"; +const syncer::ModelType kModelType = syncer::ModelType::ARTICLES; } // namespace @@ -293,7 +297,7 @@ void AttachmentUploaderImplTest::SetUp() { scopes.insert(GaiaConstants::kChromeSyncOAuth2Scope); uploader().reset(new AttachmentUploaderImpl( url, url_request_context_getter_, kAccountId, scopes, - token_service_provider, std::string(kStoreBirthday))); + token_service_provider, std::string(kStoreBirthday), kModelType)); upload_callback_ = base::Bind(&AttachmentUploaderImplTest::UploadDone, base::Unretained(this)); @@ -506,6 +510,11 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_Headers) { EXPECT_THAT(http_request.headers, testing::Contains(testing::Pair(kSyncStoreBirthdayHeader, kBase64URLSafeStoreBirthday))); + EXPECT_THAT(http_request.headers, + testing::Contains(testing::Pair( + kSyncDataTypeIdHeader, + base::IntToString( + GetSpecificsFieldNumberFromModelType(kModelType))))); } // Verify two overlapping calls to upload the same attachment result in only one diff --git a/sync/internal_api/public/attachments/attachment_downloader.h b/sync/internal_api/public/attachments/attachment_downloader.h index 8b7180c..1b2e922 100644 --- a/sync/internal_api/public/attachments/attachment_downloader.h +++ b/sync/internal_api/public/attachments/attachment_downloader.h @@ -10,6 +10,7 @@ #include "google_apis/gaia/oauth2_token_service_request.h" #include "sync/api/attachments/attachment.h" #include "sync/base/sync_export.h" +#include "sync/internal_api/public/base/model_type.h" namespace net { class URLRequestContextGetter; @@ -53,6 +54,8 @@ class SYNC_EXPORT AttachmentDownloader { // |token_service_provider| provides an OAuth2 token service. // // |store_birthday| is the raw, sync store birthday. + // + // |model_type| is the model type this downloader is used with. static scoped_ptr Create( const GURL& sync_service_url, const scoped_refptr& @@ -61,7 +64,8 @@ class SYNC_EXPORT AttachmentDownloader { const OAuth2TokenService::ScopeSet scopes, const scoped_refptr& token_service_provider, - const std::string& store_birthday); + const std::string& store_birthday, + ModelType model_type); }; } // namespace syncer diff --git a/sync/internal_api/public/attachments/attachment_downloader_impl.h b/sync/internal_api/public/attachments/attachment_downloader_impl.h index d6fa533..4e68f27 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/internal_api/public/attachments/attachment_downloader.h" +#include "sync/internal_api/public/base/model_type.h" #include "url/gurl.h" namespace base { @@ -40,6 +41,8 @@ class AttachmentDownloaderImpl : public AttachmentDownloader, // |token_service_provider| provides an OAuth2 token service. // // |store_birthday| is the raw, sync store birthday. + // + // |model_type| is the model type this downloader is used with. AttachmentDownloaderImpl( const GURL& sync_service_url, const scoped_refptr& @@ -48,7 +51,8 @@ class AttachmentDownloaderImpl : public AttachmentDownloader, const OAuth2TokenService::ScopeSet& scopes, const scoped_refptr& token_service_provider, - const std::string& store_birthday); + const std::string& store_birthday, + ModelType model_type); ~AttachmentDownloaderImpl() override; // AttachmentDownloader implementation. @@ -112,6 +116,8 @@ class AttachmentDownloaderImpl : public AttachmentDownloader, // lifetime. StateList requests_waiting_for_access_token_; + ModelType model_type_; + DISALLOW_COPY_AND_ASSIGN(AttachmentDownloaderImpl); }; diff --git a/sync/internal_api/public/attachments/attachment_uploader_impl.h b/sync/internal_api/public/attachments/attachment_uploader_impl.h index fe77379..1f85bc6 100644 --- a/sync/internal_api/public/attachments/attachment_uploader_impl.h +++ b/sync/internal_api/public/attachments/attachment_uploader_impl.h @@ -11,6 +11,7 @@ #include "google_apis/gaia/oauth2_token_service_request.h" #include "net/url_request/url_request_context_getter.h" #include "sync/internal_api/public/attachments/attachment_uploader.h" +#include "sync/internal_api/public/base/model_type.h" class GURL; @@ -35,6 +36,8 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader, // |token_service_provider| provides an OAuth2 token service. // // |store_birthday| is the raw, sync store birthday. + // + // |model_type| is the model type this uploader is used with. AttachmentUploaderImpl( const GURL& sync_service_url, const scoped_refptr& @@ -43,7 +46,8 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader, const OAuth2TokenService::ScopeSet& scopes, const scoped_refptr& token_service_provider, - const std::string& store_birthday); + const std::string& store_birthday, + ModelType model_type); ~AttachmentUploaderImpl() override; // AttachmentUploader implementation. @@ -67,6 +71,7 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader, net::URLFetcher* fetcher, const std::string& auth_token, const std::string& raw_store_birthday, + ModelType model_type, net::URLRequestContextGetter* request_context_getter); private: @@ -89,6 +94,7 @@ class SYNC_EXPORT AttachmentUploaderImpl : public AttachmentUploader, token_service_provider_; std::string raw_store_birthday_; StateMap state_map_; + ModelType model_type_; // Must be last data member. base::WeakPtrFactory weak_ptr_factory_; diff --git a/sync/internal_api/public/base/model_type.h b/sync/internal_api/public/base/model_type.h index 716bece..13c7373 100644 --- a/sync/internal_api/public/base/model_type.h +++ b/sync/internal_api/public/base/model_type.h @@ -251,8 +251,6 @@ SYNC_EXPORT_PRIVATE ModelType GetModelTypeFromSpecificsFieldNumber( // Return the field number of the EntitySpecifics field associated with // a model type. -// -// Used by tests outside of sync. SYNC_EXPORT int GetSpecificsFieldNumberFromModelType( ModelType model_type); -- cgit v1.1