Make client tag IDs consistent in sync fake server

Make client tag IDs consistent in sync fake server

Modifies the sync fake server so it derives the IDs for client-tagged
items based on their model type and unique client tag.

This fixes a race where two clients could simultaneously commit two
client-tagged items with version == 0, and the two committed items would
be assigned different IDs.  The correct behavior, and that of the real
sync server, is to assign the same IDs to items that have the same
client tags.

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

[rlarocque, 2014-07-23 22:59:04]

Patrick, please review.

Nick, we'll need your approval to get past the CQ.

[pval...(no longer on Chromium), 2014-07-23 23:54:16]

https://codereview.chromium.org/414953002/diff/1/sync/test/fake_server/fake_s...
File sync/test/fake_server/fake_server.cc (left):

https://codereview.chromium.org/414953002/diff/1/sync/test/fake_server/fake_s...
sync/test/fake_server/fake_server.cc:339: if
(entities_.find(client_entity.id_string()) != entities_.end()) {
Looking back at this, perhaps I was assuming that client_entity.id_string() was
the client_defined_unique_tag itself.

https://codereview.chromium.org/414953002/diff/1/sync/test/fake_server/unique...
File sync/test/fake_server/unique_client_entity.cc (right):

https://codereview.chromium.org/414953002/diff/1/sync/test/fake_server/unique...
sync/test/fake_server/unique_client_entity.cc:25: FakeServerEntity*
UniqueClientEntity::CreateNew(
CreateNew and CreateUpdatedVersion look pretty similar now. What do you think
about simplifying them down to just one method?

[rlarocque, 2014-07-24 00:18:40]

I've made the update you suggested.

I've also noticed that I seem to have broken item deletion on the FakeServer, so
I'll have to investigate that before committing.

https://codereview.chromium.org/414953002/diff/1/sync/test/fake_server/fake_s...
File sync/test/fake_server/fake_server.cc (left):

https://codereview.chromium.org/414953002/diff/1/sync/test/fake_server/fake_s...
sync/test/fake_server/fake_server.cc:339: if
(entities_.find(client_entity.id_string()) != entities_.end()) {
On 2014/07/23 23:54:15, pvalenzuela wrote:
> Looking back at this, perhaps I was assuming that client_entity.id_string()
was
> the client_defined_unique_tag itself.

Could be.  It's also possible that you implemented it this way because until
recently not all entities were guaranteed to have a ModelType.  Without a
ModelType, it's impossible to derive a unique client tag, so this kind of logic
would have been impossible.

https://codereview.chromium.org/414953002/diff/1/sync/test/fake_server/unique...
File sync/test/fake_server/unique_client_entity.cc (right):

https://codereview.chromium.org/414953002/diff/1/sync/test/fake_server/unique...
sync/test/fake_server/unique_client_entity.cc:25: FakeServerEntity*
UniqueClientEntity::CreateNew(
On 2014/07/23 23:54:15, pvalenzuela wrote:
> CreateNew and CreateUpdatedVersion look pretty similar now. What do you think
> about simplifying them down to just one method?

I hadn't noticed that until now.  Good point.  I'll update this CL.

[rlarocque, 2014-07-24 00:57:32]

I found the problem that was breaking integration tests.  It turns out that the
separator token '/' can appear in our base64'ed unique_client_tags.  If there is
more than one '/' character, then GetModelTypeFromId() will fail.

I'd prefer to not mess with the unique_client_tags, and I'd like to continue
embedding them in the ID.  I'm going to see if there's some other character
besides '/' that could be used as the kIdSeparator.

[rlarocque, 2014-07-24 17:42:02]

I changed the separator to '_'.  According to my reading of the modp_base64
source, this should be a safe choice.

[pval...(no longer on Chromium), 2014-07-24 18:10:18]

lgtm

https://codereview.chromium.org/414953002/diff/40001/sync/test/fake_server/fa...
File sync/test/fake_server/fake_server_entity.cc (right):

https://codereview.chromium.org/414953002/diff/40001/sync/test/fake_server/fa...
sync/test/fake_server/fake_server_entity.cc:29: // The separator used when
formatting IDs.
Can you add a brief comment explaining why _ is chosen here?

[rlarocque, 2014-07-24 18:31:17]

https://codereview.chromium.org/414953002/diff/40001/sync/test/fake_server/fa...
File sync/test/fake_server/fake_server_entity.cc (right):

https://codereview.chromium.org/414953002/diff/40001/sync/test/fake_server/fa...
sync/test/fake_server/fake_server_entity.cc:29: // The separator used when
formatting IDs.
On 2014/07/24 18:10:17, pvalenzuela wrote:
> Can you add a brief comment explaining why _ is chosen here?

Done.

[maniscalco, 2014-07-24 20:34:30]

lgtm

[rlarocque, 2014-07-24 20:35:32]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-24 20:36:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/414953002/60001

[commit-bot: I haz the power, 2014-07-24 23:53:25]

Change committed as 285415