Add unique hashing to sync API.

Add unique hashing to sync API.

TEST=included unit test
BUG=35586
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=39307

[ncarter (slow), 2010-02-13 18:11:11]

Thanks for getting this together so quickly.

http://codereview.chromium.org/593094/diff/1007/8
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/593094/diff/1007/8#newcode396
chrome/browser/sync/engine/syncapi.cc:396: DCHECK(model_type >
syncable::UNSPECIFIED);
This introduces a dependency on the model_type enum.  This worries me a little
bit -- some future refactoring could decide to remove TOP_LEVEL_FOLDER, e.g.

We can easily avoid it by doing like:

sync_pb::EntitySpecifics serialized_type;
syncable::AddDefaultExtensionValue(model_type, &serialized_type);
std::string hash_input;
serialized_type.AppendToString(&hash_input);
hash_input.append(client_tag);

I'm fairly certain that this would be robust against concatenation ambiguities
(e.g. "1" + "12" vs. "11" + 2").  The proof is something like: if you assume the
protobuf contains only a single extended non-repeated field, then when parsing
it, you could know when you're at the end of the field, and thus at the end of
the protobuf message.  Thus, one protobuf message cannot serialize to be a
prefix of another.

http://codereview.chromium.org/593094/diff/1007/8#newcode584
chrome/browser/sync/engine/syncapi.cc:584: GetModelType() ==
syncable::UNSPECIFIED);  // Immutable once set.
This is a good dcheck.

http://codereview.chromium.org/593094/diff/1007/9
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/593094/diff/1007/9#newcode191
chrome/browser/sync/engine/syncapi.h:191: const std::string
GenerateSyncableHash(syncable::ModelType model_type,
Return string, not const string.

http://codereview.chromium.org/593094/diff/1007/10
File chrome/browser/sync/engine/syncapi_unittest.cc (right):

http://codereview.chromium.org/593094/diff/1007/10#newcode116
chrome/browser/sync/engine/syncapi_unittest.cc:116: "collideme"));
It would be prudent to have a couple of canary-type tests that generate a hash
compare the client tag value against a literal; this sort of test we'd expect to
fail if somebody monkeyed with the code without realizing it.

http://codereview.chromium.org/593094/diff/1007/12
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/593094/diff/1007/12#newcode21
chrome/browser/sync/syncable/model_type.h:21: // Don't rearrange the order of
these ever. It is used by a hash function.
For the sake of preventing accidental change and allowing portability, I'd
prefer to keep dependencies that influence the wire-format values documented in
.proto files.  I am open to discussion on this.

[chron_chromium.org, 2010-02-17 01:40:10]

I've added additional unit tests and changed the hash format.

http://codereview.chromium.org/593094/diff/1007/8
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/593094/diff/1007/8#newcode396
chrome/browser/sync/engine/syncapi.cc:396: DCHECK(model_type >
syncable::UNSPECIFIED);
We discussed this and i'm going to go with the serialization you described.

http://codereview.chromium.org/593094/diff/1007/8#newcode584
chrome/browser/sync/engine/syncapi.cc:584: GetModelType() ==
syncable::UNSPECIFIED);  // Immutable once set.
On 2010/02/13 18:11:11, nick wrote:
> This is a good dcheck.

Done.

http://codereview.chromium.org/593094/diff/1007/9
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/593094/diff/1007/9#newcode191
chrome/browser/sync/engine/syncapi.h:191: const std::string
GenerateSyncableHash(syncable::ModelType model_type,
On 2010/02/13 18:11:11, nick wrote:
> Return string, not const string.

Done.

http://codereview.chromium.org/593094/diff/1007/10
File chrome/browser/sync/engine/syncapi_unittest.cc (right):

http://codereview.chromium.org/593094/diff/1007/10#newcode116
chrome/browser/sync/engine/syncapi_unittest.cc:116: "collideme"));
On 2010/02/13 18:11:11, nick wrote:
> It would be prudent to have a couple of canary-type tests that generate a hash
> compare the client tag value against a literal; this sort of test we'd expect
to
> fail if somebody monkeyed with the code without realizing it.

Done.

http://codereview.chromium.org/593094/diff/1007/12
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/593094/diff/1007/12#newcode21
chrome/browser/sync/syncable/model_type.h:21: // Don't rearrange the order of
these ever. It is used by a hash function.
On 2010/02/13 18:11:11, nick wrote:
> For the sake of preventing accidental change and allowing portability, I'd
> prefer to keep dependencies that influence the wire-format values documented
in
> .proto files.  I am open to discussion on this.

Done.

[ncarter (slow), 2010-02-18 01:05:40]

LGTM

http://codereview.chromium.org/593094/diff/5002/5004
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/593094/diff/5002/5004#newcode52
chrome/browser/sync/engine/syncapi.h:52: 
no blank line.

http://codereview.chromium.org/593094/diff/5002/5005
File chrome/browser/sync/engine/syncapi_unittest.cc (right):

http://codereview.chromium.org/593094/diff/5002/5005#newcode83
chrome/browser/sync/engine/syncapi_unittest.cc:83: std::string HashToHex(const
std::string& hash) {
De-indent.

http://codereview.chromium.org/593094/diff/5002/5007
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/593094/diff/5002/5007#newcode1078
chrome/browser/sync/syncable/syncable.cc:1078: 
It's probably okay for these DCHECKs to die -- but they did find a whole bunch
of unit test bugs while they were around.  They might still be appropriate in
the wtrans dtor, but I'm not insisting.

[chron, 2010-02-18 01:09:32]

http://codereview.chromium.org/593094/diff/5002/5004
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/593094/diff/5002/5004#newcode52
chrome/browser/sync/engine/syncapi.h:52: 
On 2010/02/18 01:05:40, nick wrote:
> no blank line.

Done.

http://codereview.chromium.org/593094/diff/5002/5005
File chrome/browser/sync/engine/syncapi_unittest.cc (right):

http://codereview.chromium.org/593094/diff/5002/5005#newcode83
chrome/browser/sync/engine/syncapi_unittest.cc:83: std::string HashToHex(const
std::string& hash) {
On 2010/02/18 01:05:40, nick wrote:
> De-indent.

Done.

http://codereview.chromium.org/593094/diff/5002/5007
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/593094/diff/5002/5007#newcode1078
chrome/browser/sync/syncable/syncable.cc:1078: 
These dchecks weren't really valid since we call GetModelType in different
places now. One such case is if we call it on a local item that we haven't
stuffed any data into, it interferes with the other assert that I think is more
useful. Can't "assert not set" if this is the case.