SyncFS: Convert WAPI ResourceID to DriveAPI FileID

SyncFS: Convert WAPI ResourceID to DriveAPI FileID

To migrate SyncFS from WAPI to DriveAPI, this change adds utility functions to
convert from/to WAPI ResourceID to/from DriveAPI FileID as below, and replaces
WAPI ResourceIDs stored in DriveMetadataDB with DriveAPI FileIDs using
DB migration mechanism (DB schema version: 1 -> 2).

(WAPI) "file:xxxx" <=> "xxxx" (DriveAPI)
(WAPI) "folder:yyyy" <=> "yyyy" (DriveAPI)

When using WAPI (i.e. IsDriveAPIEnabled() returns false), DriveMetadataDB
passes ResourceIDs re-converted from FileIDs to DriveMetadataStore so that the
SyncFS can use migrated DB contents on WAPI.


BUG=234557
TEST=unit_tests --gtest_filter=\*DriveMetadataDBMigrationUtilTest\*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202858

[nhiroki, 2013-05-27 06:29:53]

Hi, can you review this?
I'll add tests for v1-v2 DB migration soon.

Thanks!

[nhiroki, 2013-05-27 07:45:25]

Oops... sorry, I forgot to migrate the SyncRootDir, Origins, ...
I'll update the patch set.

[tzik, 2013-05-27 08:04:42]

looks good

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:1024: //   key:
"VERSION" (new)
drop (new)?

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:1033: //   key:
"METADATA: " + <Origin and URL> (changed)
drop (changed)?

[kinuko, 2013-05-27 08:25:16]

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:956: SyncStatusCode
DriveMetadataDB::MigrateFromVersion0Database() {
Can we rename this: MigrateFromVersion0To1Database

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:1022: SyncStatusCode
DriveMetadataDB::MigrateFromVersion1Database() {
Can we rename this: MigrateFromVersion1To2Database

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:1044: //
I think we could drop comments for Version 1 here? (line 1023-1044)

[kinuko, 2013-05-27 08:26:57]

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:1044: //
On 2013/05/27 08:25:16, kinuko wrote:
> I think we could drop comments for Version 1 here? (line 1023-1044)

I mean, because we have the same text right above in 0to1 migration method.

[nhiroki, 2013-05-27 09:13:03]

Updated. Thanks!

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:956: SyncStatusCode
DriveMetadataDB::MigrateFromVersion0Database() {
On 2013/05/27 08:25:16, kinuko wrote:
> Can we rename this: MigrateFromVersion0To1Database

Done.

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:1022: SyncStatusCode
DriveMetadataDB::MigrateFromVersion1Database() {
On 2013/05/27 08:25:16, kinuko wrote:
> Can we rename this: MigrateFromVersion1To2Database

Done.

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:1024: //   key:
"VERSION" (new)
On 2013/05/27 08:04:42, tzik wrote:
> drop (new)?

Done.

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:1033: //   key:
"METADATA: " + <Origin and URL> (changed)
On 2013/05/27 08:04:42, tzik wrote:
> drop (changed)?

Done.

https://codereview.chromium.org/15808002/diff/22001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:1044: //
On 2013/05/27 08:26:57, kinuko wrote:
> On 2013/05/27 08:25:16, kinuko wrote:
> > I think we could drop comments for Version 1 here? (line 1023-1044)
> 
> I mean, because we have the same text right above in 0to1 migration method.

Done.

[nhiroki, 2013-05-27 09:17:56]

https://codereview.chromium.org/15808002/diff/34001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store_unittest.cc (right):

https://codereview.chromium.org/15808002/diff/34001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store_unittest.cc:604: {
Sorry, I uploaded wrong commit. This migration test has not updated yet (just
copied). Please ignore for now...

[nhiroki, 2013-05-28 10:04:41]

Updated. Can you take another look?

PatchSet-3 factors out V1-V2 migration function and prefix manipulations into
metadata_db_migration_util.{cc,h}. V0-V1 migration function will be moved in a
separate CL if this change looks good to you.

Thanks!

[kinuko, 2013-05-28 15:09:19]

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:24:
StartsWithASCII(resource_id, kWapiFolderIdPrefix, true))
Is this ok?  Does this mean we get 'folder:xxxx' resource Id while expecting a
file?

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:56:
resource_id.end());
nit: please put { } if body spans multiple line (for both if blocks)

nit: can we make these patterns a local (template) function like 
RemovePrefix(string, prefix) and use it here and for all leveldb strip-prefix
operations?

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
File
chrome/browser/sync_file_system/drive/metadata_db_migration_util_unittest.cc
(right):

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util_unittest.cc:52:
EXPECT_EQ("zzz", RemoveWapiIdPrefix("folder:zzz"));
nit: should we also test something like "foo:xxx" -> "foo:xxx" (while it
wouldn't have much meaning in a real situation)

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util_unittest.cc:151:
// Verify incremental sync origins.
incremental -> disabled

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:997:
metadata.set_resource_id(
nit: indent

[nhiroki, 2013-05-29 04:10:18]

Updated, thanks!

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:24:
StartsWithASCII(resource_id, kWapiFolderIdPrefix, true))
On 2013/05/28 15:09:20, kinuko wrote:
> Is this ok?  Does this mean we get 'folder:xxxx' resource Id while expecting a
> file?

Yes, I mean it. Do you prefer to return empty string or something else?

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:56:
resource_id.end());
On 2013/05/28 15:09:20, kinuko wrote:
> nit: please put { } if body spans multiple line (for both if blocks)
> 
> nit: can we make these patterns a local (template) function like 
> RemovePrefix(string, prefix) and use it here and for all leveldb strip-prefix
> operations?

Done.

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
File
chrome/browser/sync_file_system/drive/metadata_db_migration_util_unittest.cc
(right):

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util_unittest.cc:52:
EXPECT_EQ("zzz", RemoveWapiIdPrefix("folder:zzz"));
On 2013/05/28 15:09:20, kinuko wrote:
> nit: should we also test something like "foo:xxx" -> "foo:xxx" (while it
> wouldn't have much meaning in a real situation)

Done.

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util_unittest.cc:151:
// Verify incremental sync origins.
On 2013/05/28 15:09:20, kinuko wrote:
> incremental -> disabled

Done.

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:997:
metadata.set_resource_id(
On 2013/05/28 15:09:20, kinuko wrote:
> nit: indent

Done.

[tzik, 2013-05-29 04:30:24]

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:52: default:
Could you drop "default:" and move this case out of the switch?
That will detect enum value adding on the compile time.

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_file_sync_service_sync_unittest.cc
(right):

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_file_sync_service_sync_unittest.cc:210:
default:
ditto

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:113: return
std::string(str.begin() + prefix.size(), str.end());
str.substr(prefix.size())?

[kinuko, 2013-05-29 04:44:36]

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:24:
StartsWithASCII(resource_id, kWapiFolderIdPrefix, true))
On 2013/05/29 04:10:18, nhiroki wrote:
> On 2013/05/28 15:09:20, kinuko wrote:
> > Is this ok?  Does this mean we get 'folder:xxxx' resource Id while expecting
a
> > file?
> 
> Yes, I mean it. Do you prefer to return empty string or something else?

When does this happen? If your answer to my first question is yes (i.e. it's
totally ok and it doesn't mean anything wrong/error) this code's fine for me
too.

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:103:
RemoveWapiIdPrefix(sync_root_resource_id));
I wonder if writing this in a single for could be simpler / makes sense as we
seem to be rewriting most key-value pairs:

for (itr->SeekToFirst(); itr->Valid(); itr->Next()) {
  std::string key = ...;
  if (StartsWithASCII(key, kSyncRootDirectory...)) {
    ...
    continue;
  }
  if (StartsWithASCII(key, kDriveMetadata...)) {
    ...
    continue;
  }
  if (StartsWithASCII(key, kDriveIncremental...)) {
    ...
    continue;
  }
  ...
}

[nhiroki, 2013-05-29 06:31:03]

PTAL

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:24:
StartsWithASCII(resource_id, kWapiFolderIdPrefix, true))
On 2013/05/29 04:44:36, kinuko wrote:
> On 2013/05/29 04:10:18, nhiroki wrote:
> > On 2013/05/28 15:09:20, kinuko wrote:
> > > Is this ok?  Does this mean we get 'folder:xxxx' resource Id while
expecting
> a
> > > file?
> > 
> > Yes, I mean it. Do you prefer to return empty string or something else?
> 
> When does this happen? If your answer to my first question is yes (i.e. it's
> totally ok and it doesn't mean anything wrong/error) this code's fine for me
> too.

For example:

a) metadata.type() == TYPE_FILE, but metadata.resource_id() == "folder:xxx"
b) origin_root_resource_id == "file:yyy"

This never happen as long as all stored metadata are consistent. And probably
this conditional branch is not necessary since |resource_id| must be stripped
and not empty in an actual situation. Even if happens, WAPI API could just be
failed (e.g. UpdateExistingFile with folder resource id).

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:52: default:
On 2013/05/29 04:30:24, tzik wrote:
> Could you drop "default:" and move this case out of the switch?
> That will detect enum value adding on the compile time.

Done.

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:103:
RemoveWapiIdPrefix(sync_root_resource_id));
On 2013/05/29 04:44:36, kinuko wrote:
> I wonder if writing this in a single for could be simpler / makes sense as we
> seem to be rewriting most key-value pairs:
> 
> for (itr->SeekToFirst(); itr->Valid(); itr->Next()) {
>   std::string key = ...;
>   if (StartsWithASCII(key, kSyncRootDirectory...)) {
>     ...
>     continue;
>   }
>   if (StartsWithASCII(key, kDriveMetadata...)) {
>     ...
>     continue;
>   }
>   if (StartsWithASCII(key, kDriveIncremental...)) {
>     ...
>     continue;
>   }
>   ...
> }

Done.

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_file_sync_service_sync_unittest.cc
(right):

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_file_sync_service_sync_unittest.cc:210:
default:
On 2013/05/29 04:30:24, tzik wrote:
> ditto

Done.

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15808002/diff/52001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_metadata_store.cc:113: return
std::string(str.begin() + prefix.size(), str.end());
On 2013/05/29 04:30:24, tzik wrote:
> str.substr(prefix.size())?

Done.

[kinuko, 2013-05-29 06:41:13]

lgtm with one comment

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:24:
StartsWithASCII(resource_id, kWapiFolderIdPrefix, true))
On 2013/05/29 06:31:03, nhiroki wrote:
> On 2013/05/29 04:44:36, kinuko wrote:
> > On 2013/05/29 04:10:18, nhiroki wrote:
> > > On 2013/05/28 15:09:20, kinuko wrote:
> > > > Is this ok?  Does this mean we get 'folder:xxxx' resource Id while
> expecting
> > a
> > > > file?
> > > 
> > > Yes, I mean it. Do you prefer to return empty string or something else?
> > 
> > When does this happen? If your answer to my first question is yes (i.e. it's
> > totally ok and it doesn't mean anything wrong/error) this code's fine for me
> > too.
> 
> For example:
> 
> a) metadata.type() == TYPE_FILE, but metadata.resource_id() == "folder:xxx"
> b) origin_root_resource_id == "file:yyy"
> 
> This never happen as long as all stored metadata are consistent. And probably
> this conditional branch is not necessary since |resource_id| must be stripped
> and not empty in an actual situation. Even if happens, WAPI API could just be
> failed (e.g. UpdateExistingFile with folder resource id).

Thanks for the details.
If it never happens or it's not expected can we have a stricter debug-only check
(e.g. DCHECK)?  This code confused me.

[nhiroki, 2013-05-29 07:49:16]

Updated!

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/42001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:24:
StartsWithASCII(resource_id, kWapiFolderIdPrefix, true))
On 2013/05/29 06:41:13, kinuko wrote:
> On 2013/05/29 06:31:03, nhiroki wrote:
> > On 2013/05/29 04:44:36, kinuko wrote:
> > > On 2013/05/29 04:10:18, nhiroki wrote:
> > > > On 2013/05/28 15:09:20, kinuko wrote:
> > > > > Is this ok?  Does this mean we get 'folder:xxxx' resource Id while
> > expecting
> > > a
> > > > > file?
> > > > 
> > > > Yes, I mean it. Do you prefer to return empty string or something else?
> > > 
> > > When does this happen? If your answer to my first question is yes (i.e.
it's
> > > totally ok and it doesn't mean anything wrong/error) this code's fine for
me
> > > too.
> > 
> > For example:
> > 
> > a) metadata.type() == TYPE_FILE, but metadata.resource_id() == "folder:xxx"
> > b) origin_root_resource_id == "file:yyy"
> > 
> > This never happen as long as all stored metadata are consistent. And
probably
> > this conditional branch is not necessary since |resource_id| must be
stripped
> > and not empty in an actual situation. Even if happens, WAPI API could just
be
> > failed (e.g. UpdateExistingFile with folder resource id).
> 
> Thanks for the details.
> If it never happens or it's not expected can we have a stricter debug-only
check
> (e.g. DCHECK)?  This code confused me.

Sorry, empty resource id is valid here since to-be-fetched change does not have
resource id. Anyway added DCHECKs.

[kinuko, 2013-05-29 07:52:34]

still lgtm

https://codereview.chromium.org/15808002/diff/93001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/93001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:30:
DCHECK(!StartsWithASCII(resource_id, kWapiFileIdPrefix, true));
We can probably remove this DCHECK, it's not very surprising to return the input
as is if it already starts with the prefix

https://codereview.chromium.org/15808002/diff/93001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:42:
DCHECK(!StartsWithASCII(resource_id, kWapiFolderIdPrefix, true));
ditto

[nhiroki, 2013-05-29 07:58:58]

https://codereview.chromium.org/15808002/diff/93001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/93001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:30:
DCHECK(!StartsWithASCII(resource_id, kWapiFileIdPrefix, true));
On 2013/05/29 07:52:34, kinuko wrote:
> We can probably remove this DCHECK, it's not very surprising to return the
input
> as is if it already starts with the prefix

Hmm... if resource ID with prefix reaches here, it means this patch has
leakages...

[tzik, 2013-05-29 08:09:18]

lgtm

[kinuko, 2013-05-29 08:14:53]

https://codereview.chromium.org/15808002/diff/93001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15808002/diff/93001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:30:
DCHECK(!StartsWithASCII(resource_id, kWapiFileIdPrefix, true));
On 2013/05/29 07:58:59, nhiroki wrote:
> On 2013/05/29 07:52:34, kinuko wrote:
> > We can probably remove this DCHECK, it's not very surprising to return the
> input
> > as is if it already starts with the prefix
> 
> Hmm... if resource ID with prefix reaches here, it means this patch has
> leakages...

Ok then please keep them.

[commit-bot: I haz the power, 2013-05-29 12:13:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/15808002/93001

[commit-bot: I haz the power, 2013-05-29 13:51:51]

Change committed as 202858

[nhiroki, 2013-05-30 06:50:06]

+thakis@

Hi Nico, can you give me an owner stamp for chrome_tests_unit.gypi?
I forgot to receive it before submitting...

Thanks!

[kinuko, 2013-05-30 06:58:52]

On 2013/05/30 06:50:06, nhiroki wrote:
> +thakis@
> 
> Hi Nico, can you give me an owner stamp for chrome_tests_unit.gypi?
> I forgot to receive it before submitting...
> 
> Thanks!

Fyi, TBR should be ok in this case. (You still should have added an owner to the
reviewers before submitting though)