commit b0740eccb22e71b7ec00f67137e062154936f1ec
author Dave Borowitz <dborowitz@google.com> Mon Nov 07 17:02:35 2016 -0500
committer Dave Borowitz <dborowitz@google.com> Tue Nov 08 13:04:00 2016 -0500
tree 9ee625fd7ee2dbb3e753ccb87d4068676949b026
parent c9afaaeeb9d13bfd957c42a0cd48b2e202cf7b0c

ChangeNotesState: Check meta ID before copying state to Change

An implicit assumption when populating the columns of a Change from
ChangeNotesState is that the noteDbState field recorded in the Change
in ReviewDb matches the state in the ChangeNotesState. Make this
assumption explicit with a check in ChangeNotesState#copyColumnsTo.

Normally, this is true because auto-rebuilding of out-of-date changes
happens before loading the ChangeNotesState, so the Change is
generally in sync in advance. However, there is one notable exception:
in I1be7945c we turned off auto-rebuilding when loading a change in
the context of a BatchUpdate. This caused a problem where the
out-of-date state in NoteDb was *not* auto-rebuilt but *was* copied
into the Change, overwriting the newer ReviewDb state. Worse, this
Change entity with inconsistent state was then blindly copied back
into ReviewDb when the BatchUpdate is executed. This is worse than an
inconsistency between NoteDb and ReviewDb: it's an inconsistency
*within* ReviewDb, so rebuilding a change after this point just copies
the inconsistency to NoteDb.

Fortunately, the scope of this problem is somewhat limited, because it
only applies to columns in the Change entity itself, which are
comparatively rarely changed (as opposed to e.g. adding new
PatchSetApprovals or ChangeMessages). However, there was one
particularly bad case, where the ReviewDb update that was lost
happened to be the one that advanced the currentPatchSetId field, even
though the PatchSet entity for the new patch set ID is present.

This change improves safety by failing the BatchUpdate in these cases,
so ReviewDb should not get into an inconsistent state. This is not the
best UX, because we should just be rebuilding the change. I1be7945c
explains why this might be difficult, but we might want to tackle that
anyway.

This issue was not caught by the existing
ChangeRebuilderI#rebuildAutomaticallyWithinBatchUpdate test, since
that test just asserted that the change was up-to-date in NoteDb after
the whole process, and missed the fact that a ReviewDb write was
dropped. Change the test for now to just assert that the update fails,
which is strictly better than continuing to drop the write, until we
get a chance to fix the behavior.

Change-Id: If40663e1c80cba7679b20b4f1d05afad594b6639