opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.15'

Browse Source 
* stable-2.15:
  Skip migrating inline comments on missing patch set parents
  Temporarily increase heap size of NoteDb migration tests
  Log the reason why a project cannot be found
  Change kind cache: short-circuit on root commits
  Document that gitweb.type must be set
  Tidy up config-gitweb
  Change kind cache: short-circuit on root commits
  Do not abort indexing if < 50% projects failed
  Improve documentation of `index.maxLimit` for Elasticsearch
  InitIndex: Set Elasticsearch index config under elasticsearch section
  Link to hashtag intro docs from more places
  user-upload.txt: Document setting hashtags on push
  intro-user.txt: Document hashtags
  user-search.txt: Document hashtag operator
  intro-user.txt: Mention that topics may affect submission
  Add NoteDb migration test for change with no patch set refs
  NoteDbMigrator: Totally skip changes with no patch sets
  Add more tests for rebuilding changes missing some entities
  Fix Change-Id in revert email
  Widen set of My Drafts menus that are automatically removed
  Migrate old My Drafts menus in refs/users/default

This partially reverts commit e518d9dacc
because Schema_159_to_160_Test references PREFERENCES which was made
private, and uses the forDefault method which was removed. This commit
makes PREFERENCES package visible and re-adds the forDefault method as
a package visible method.

Change-Id: Ifba662a47197b3a5f17988fc69896cdca1ff853b
This commit is contained in:
David Pursehouse
2018-03-17 10:05:58 +09:00
parent 4505ccfa33 6e0601e762
commit 3c62aa0d0b
29 changed files with 613 additions and 122 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
Documentation/access-control.txt
View File

@@ -859,8 +859,8 @@ edited on open changes.
[[category_edit_hashtags]]
=== Edit Hashtags
This category permits users to add or remove hashtags on a change that
is uploaded for review.
This category permits users to add or remove
link:intro-user.html#hashtags[hashtags] on a change that is uploaded for review.
The change owner, branch owners, project owners, and site administrators
can always edit or remove hashtags (even without having the `Edit Hashtags`

3
Documentation/cmd-stream-events.txt
View File

@@ -151,7 +151,8 @@ type:: "dropped-output"
=== Hashtags Changed
Sent when the hashtags have been added to or removed from a change.
Sent when the link:intro-user.html#hashtags[hashtags] have been added to or
removed from a change.
type:: "hashtags-changed"

4
Documentation/config-gerrit.txt
View File

@@ -2702,6 +2702,10 @@ Maximum limit to allow for search queries. Requesting results above this
limit will truncate the list (but will still set `_more_changes` on
result lists). Set to 0 for no limit.
+
When `index.type` is set to `ELASTICSEARCH`, this value should not exceed
the `index.max_result_window` value configured on the Elasticsearch
server.
+
Defaults to no limit.
[[index.maxPages]]index.maxPages::

43
Documentation/config-gitweb.txt
View File

@@ -17,8 +17,9 @@ which is a common installation path for the 'gitweb' package on
Linux distributions.
----
git config --file $site_path/etc/gerrit.config gitweb.cgi /usr/lib/cgi-bin/gitweb.cgi
git config --file $site_path/etc/gerrit.config --unset gitweb.url
git config -f $site_path/etc/gerrit.config gitweb.type gitweb
git config -f $site_path/etc/gerrit.config gitweb.cgi /usr/lib/cgi-bin/gitweb.cgi
git config -f $site_path/etc/gerrit.config --unset gitweb.url
----
Alternatively, if Gerrit is served behind reverse proxy, it can
@@ -28,8 +29,9 @@ for serving gitweb under a different URL than the Gerrit instance.
To enable this feature, set both: `gitweb.cgi` and `gitweb.url`.
----
git config --file $site_path/etc/gerrit.config gitweb.cgi /usr/lib/cgi-bin/gitweb.cgi
git config --file $site_path/etc/gerrit.config gitweb.url /pretty/path/to/gitweb
git config -f $site_path/etc/gerrit.config gitweb.type gitweb
git config -f $site_path/etc/gerrit.config gitweb.cgi /usr/lib/cgi-bin/gitweb.cgi
git config -f $site_path/etc/gerrit.config gitweb.url /pretty/path/to/gitweb
----
After updating `'$site_path'/etc/gerrit.config`, the Gerrit server must
@@ -77,13 +79,13 @@ provides configuration parameters for integration with gitweb.
On Ubuntu:
----
$ sudo apt-get install gitweb
sudo apt-get install gitweb
----
With Yum:
----
$ yum install gitweb
yum install gitweb
----
===== Configure Gitweb
@@ -125,14 +127,14 @@ $favicon = "git-favicon.png";
Link gitweb to `/var/www/gitweb`, check `/etc/gitweb.conf` if unsure of paths:
----
$ sudo ln -s /usr/share/gitweb /var/www/gitweb
sudo ln -s /usr/share/gitweb /var/www/gitweb
----
Add the gitweb directory to the Apache configuration by creating a "gitweb"
file inside the Apache conf.d directory:
----
$ touch /etc/apache/conf.d/gitweb
touch /etc/apache/conf.d/gitweb
----
Add the following to /etc/apache/conf.d/gitweb:
@@ -152,7 +154,7 @@ is.
===== Restart the Apache Web Server
----
$ sudo /etc/init.d/apache2 restart
sudo /etc/init.d/apache2 restart
----
Now you should be able to view your repository projects online:
@@ -184,7 +186,7 @@ verify by checking for perl modules. From an msys console, execute the
following to check:
----
$ perl -mCGI -mEncode -mFcntl -mFile::Find -mFile::Basename -e ""
perl -mCGI -mEncode -mFcntl -mFile::Find -mFile::Basename -e ""
----
You may encounter the following exception:
@@ -220,21 +222,22 @@ being used, ensure it uses a full mirror, so the `+refs/changes/*+`
namespace is available.
----
$ git config -f $site_path/etc/gerrit.config --unset gitweb.cgi
$ git config -f $site_path/etc/gerrit.config gitweb.url https://gitweb.corporation.com
git config -f $site_path/etc/gerrit.config gitweb.type gitweb
git config -f $site_path/etc/gerrit.config --unset gitweb.cgi
git config -f $site_path/etc/gerrit.config gitweb.url https://gitweb.corporation.com
----
If you're not following the traditional \{projectName\}.git project naming conventions,
If you're not following the traditional `\{projectName\}.git` project naming conventions,
you will want to customize Gerrit to read them. Add the following:
----
$ git config -f $site_path/etc/gerrit.config gitweb.type custom
$ git config -f $site_path/etc/gerrit.config gitweb.project ?p=\${project}\;a=summary
$ git config -f $site_path/etc/gerrit.config gitweb.revision ?p=\${project}\;a=commit\;h=\${commit}
$ git config -f $site_path/etc/gerrit.config gitweb.branch ?p=\${project}\;a=shortlog\;h=\${branch}
$ git config -f $site_path/etc/gerrit.config gitweb.roottree ?p=\${project}\;a=tree\;hb=\${commit}
$ git config -f $site_path/etc/gerrit.config gitweb.file ?p=\${project}\;hb=\${commit}\;f=\${file}
$ git config -f $site_path/etc/gerrit.config gitweb.filehistory ?p=\${project}\;a=history\;hb=\${branch}\;f=\${file}
git config -f $site_path/etc/gerrit.config gitweb.type custom
git config -f $site_path/etc/gerrit.config gitweb.project ?p=\${project}\;a=summary
git config -f $site_path/etc/gerrit.config gitweb.revision ?p=\${project}\;a=commit\;h=\${commit}
git config -f $site_path/etc/gerrit.config gitweb.branch ?p=\${project}\;a=shortlog\;h=\${branch}
git config -f $site_path/etc/gerrit.config gitweb.roottree ?p=\${project}\;a=tree\;hb=\${commit}
git config -f $site_path/etc/gerrit.config gitweb.file ?p=\${project}\;hb=\${commit}\;f=\${file}
git config -f $site_path/etc/gerrit.config gitweb.filehistory ?p=\${project}\;a=history\;hb=\${branch}\;f=\${file}
----
After updating `'$site_path'/etc/gerrit.config`, the Gerrit server must

28
Documentation/intro-user.txt
View File

@@ -472,6 +472,10 @@ push], either by appending `%topic=...` to the ref name or through
the use of the command line flag `--push-option`, aliased to `-o`,
followed by `topic=...`.
Gerrit may be link:config-gerrit.html#change.submitWholeTopic[configured] to
submit all changes in a topic together with a single click, even when topics
span multiple projects.
.Set Topic on Push
----
$ git push origin HEAD:refs/for/master%topic=multi-master
@@ -480,6 +484,30 @@ followed by `topic=...`.
$ git push origin HEAD:refs/heads/master -o topic=multi-master
----
[[hashtags]]
== Using Hashtags
Hashtags are freeform strings associated with a change, like on social media
platforms. In Gerrit, you explicitly associate hashtags with changes using a
dedicated area of the UI; they are not parsed from commit messages or comments.
Similar to topics, hashtags can be used to group related changes together, and
to search using the link:user-search.html#hashtag[`hashtag:`] operator. Unlike
topics, a change can have multiple hashtags, and they are only used for
informational grouping; changes with the same hashtags are not necessarily
submitted together.
The hashtag feature is only available when running under
link:note-db.html[NoteDb].
.Set Hashtag on Push
----
$ git push origin HEAD:refs/for/master%t=stable-bugfix
// this is the same as:
$ git push origin HEAD:refs/heads/master -o t=stable-bugfix
----
[[wip]]
== Work-in-Progress Changes

5
Documentation/user-search.txt
View File

@@ -191,6 +191,11 @@ Changes whose designated topic matches 'TOPIC' exactly. This is
often combined with 'branch:' and 'project:' operators to select
all related changes in a series.
[[hashtag]]
hashtag:'HASHTAG'::
+
Changes whose link:intro-user.html#hashtags[hashtag] matches 'HASHTAG' exactly.
[[ref]]
ref:'REF'::
+

25
Documentation/user-upload.txt
View File

@@ -243,12 +243,11 @@ change.
[[topic]]
==== Topic
To include a short tag associated with all of the changes in the
same group, such as the local topic branch name, append it after
the destination branch name or add it with the command line flag
`--push-option`, aliased to `-o`. In this example the short topic
tag 'driver/i42' will be saved on each change this push creates or
updates:
To include a short link:intro-user.html#topics[topic] associated with all
of the changes in the same group, such as the local topic branch name,
append it after the destination branch name or add it with the command line
flag `--push-option`, aliased to `-o`. In this example the short topic name
'driver/i42' will be saved on each change this push creates or updates:
----
git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%topic=driver/i42
@@ -257,6 +256,20 @@ updates:
git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental -o topic=driver/i42
----
[[hashtag]]
==== Hashtag
To include a link:intro-user.html#hashtags[hashtag] associated with all of the
changes in the same group, use the `hashtag` or `t` option:
----
// these are all equivalent
git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%hashtag=stable-fix
git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%t=stable-fix
git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental -o hashtag=stable-fix
git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental -o t=stable-fix
----
[[private]]
==== Private Changes

15
java/com/google/gerrit/pgm/init/InitIndex.java
View File

@@ -40,6 +40,7 @@ class InitIndex implements InitStep {
private final SitePaths site;
private final InitFlags initFlags;
private final Section gerrit;
private final Section.Factory sections;
@Inject
InitIndex(ConsoleUI ui, Section.Factory sections, SitePaths site, InitFlags initFlags) {
@@ -48,6 +49,7 @@ class InitIndex implements InitStep {
this.gerrit = sections.get("gerrit", null);
this.site = site;
this.initFlags = initFlags;
this.sections = sections;
}
@Override
@@ -59,10 +61,15 @@ class InitIndex implements InitStep {
}
if (type == IndexType.ELASTICSEARCH) {
index.select("Transport protocol", "protocol", "http", Sets.newHashSet("http", "https"));
index.string("Hostname", "hostname", "localhost");
index.string("Port", "port", "9200");
index.string("Index Name", "name", "gerrit");
Section elasticsearch = sections.get("elasticsearch", null);
elasticsearch.string("Index Prefix", "prefix", "gerrit");
String name = ui.readString("default", "Server Name");
Section defaultServer = sections.get("elasticsearch", name);
defaultServer.select(
"Transport protocol", "protocol", "http", Sets.newHashSet("http", "https"));
defaultServer.string("Hostname", "hostname", "localhost");
defaultServer.string("Port", "port", "9200");
}
if ((site.isNew || isEmptySite()) && type == IndexType.LUCENE) {

24
java/com/google/gerrit/server/CommentsUtil.java
View File

@@ -491,25 +491,21 @@ public class CommentsUtil {
}
public static void setCommentRevId(Comment c, PatchListCache cache, Change change, PatchSet ps)
throws OrmException {
throws PatchListNotAvailableException {
checkArgument(
c.key.patchSetId == ps.getId().get(),
"cannot set RevId for patch set %s on comment %s",
ps.getId(),
c);
if (c.revId == null) {
try {
if (Side.fromShort(c.side) == Side.PARENT) {
if (c.side < 0) {
c.revId = ObjectId.toString(cache.getOldId(change, ps, -c.side));
} else {
c.revId = ObjectId.toString(cache.getOldId(change, ps, null));
}
if (Side.fromShort(c.side) == Side.PARENT) {
if (c.side < 0) {
c.revId = ObjectId.toString(cache.getOldId(change, ps, -c.side));
} else {
c.revId = ps.getRevision().get();
c.revId = ObjectId.toString(cache.getOldId(change, ps, null));
}
} catch (PatchListNotAvailableException e) {
throw new OrmException(e);
} else {
c.revId = ps.getRevision().get();
}
}
}
@@ -576,7 +572,11 @@ public class CommentsUtil {
// Draft may have been created by a different real user; copy the current real user. (Only
// applies to X-Gerrit-RunAs, since modifying drafts via on_behalf_of is not allowed.)
ctx.getUser().updateRealAccountId(d::setRealAuthor);
setCommentRevId(d, patchListCache, notes.getChange(), ps);
try {
setCommentRevId(d, patchListCache, notes.getChange(), ps);
} catch (PatchListNotAvailableException e) {
throw new OrmException(e);
}
}
putComments(ctx.getDb(), ctx.getUpdate(psId), PUBLISHED, drafts);
}

11
java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
View File

@@ -241,8 +241,15 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
return ChangeKind.NO_CHANGE;
}
if ((prior.getParentCount() != 1 || next.getParentCount() != 1)
&& (!onlyFirstParentChanged(prior, next) || prior.getParentCount() == 0)) {
if (prior.getParentCount() == 0 || next.getParentCount() == 0) {
// At this point we have considered all the kinds that could be applicable to root
// commits; the remainder of the checks in this method all assume that both commits have
// at least one parent.
return ChangeKind.REWORK;
}
if ((prior.getParentCount() > 1 || next.getParentCount() > 1)
&& !onlyFirstParentChanged(prior, next)) {
// Trivial rebases done by machine only work well on 1 parent.
return ChangeKind.REWORK;
}

10
java/com/google/gerrit/server/index/change/AllChangesIndexer.java
View File

@@ -111,20 +111,24 @@ public class AllChangesIndexer extends SiteIndexer<Change.Id, ChangeData, Change
SortedSet<ProjectHolder> projects = new TreeSet<>();
int changeCount = 0;
Stopwatch sw = Stopwatch.createStarted();
int projectsFailed = 0;
for (Project.NameKey name : projectCache.all()) {
try (Repository repo = repoManager.openRepository(name)) {
long size = estimateSize(repo);
changeCount += size;
projects.add(new ProjectHolder(name, size));
} catch (IOException e) {
log.error("Error collecting projects", e);
return new Result(sw, false, 0, 0);
log.error("Error collecting project {}", name, e);
projectsFailed++;
if (projectsFailed > projects.size() / 2) {
log.error("Over 50% of the projects could not be collected: aborted");
return new Result(sw, false, 0, 0);
}
}
pm.update(1);
}
pm.endTask();
setTotalWork(changeCount);
return indexAll(index, projects);
}

5
java/com/google/gerrit/server/mail/receive/MailProcessor.java
View File

@@ -46,6 +46,7 @@ import com.google.gerrit.server.mail.MailFilter;
import com.google.gerrit.server.mail.send.InboundEmailRejectionSender;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.update.BatchUpdate;
@@ -274,7 +275,7 @@ public class MailProcessor {
@Override
public boolean updateChange(ChangeContext ctx)
throws OrmException, UnprocessableEntityException {
throws OrmException, UnprocessableEntityException, PatchListNotAvailableException {
patchSet = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
notes = ctx.getNotes();
if (patchSet == null) {
@@ -371,7 +372,7 @@ public class MailProcessor {
private Comment persistentCommentFromMailComment(
ChangeContext ctx, MailComment mailComment, PatchSet patchSetForComment)
throws OrmException, UnprocessableEntityException {
throws OrmException, UnprocessableEntityException, PatchListNotAvailableException {
String fileName;
// The patch set that this comment is based on is different if this
// comment was sent in reply to a comment on a previous patch set.

20
java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java
View File

@@ -80,6 +80,18 @@ import org.slf4j.LoggerFactory;
public class PrimaryStorageMigrator {
private static final Logger log = LoggerFactory.getLogger(PrimaryStorageMigrator.class);
/**
* Exception thrown during migration if the change has no {@code noteDbState} field at the
* beginning of the migration.
*/
public static class NoNoteDbStateException extends RuntimeException {
private static final long serialVersionUID = 1L;
private NoNoteDbStateException(Change.Id id) {
super("change " + id + " has no note_db_state; rebuild it first");
}
}
private final AllUsersName allUsers;
private final ChangeNotes.Factory changeNotesFactory;
private final ChangeRebuilder rebuilder;
@@ -280,9 +292,11 @@ public class PrimaryStorageMigrator {
NoteDbChangeState state = NoteDbChangeState.parse(change);
if (state == null) {
// Could rebuild the change here, but that's more complexity, and this
// really shouldn't happen.
throw new OrmRuntimeException(
"change " + id + " has no note_db_state; rebuild it first");
// normally shouldn't happen.
//
// Known cases where this happens are described in and handled by
// NoteDbMigrator#canSkipPrimaryStorageMigration.
throw new NoNoteDbStateException(id);
}
// If the change is already read-only, then the lease is held by another
// (likely failed) migrator thread. Fail early, as we can't take over

3
java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
View File

@@ -522,8 +522,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
}
private void flushEventsToDraftUpdate(
NoteDbUpdateManager manager, EventList<DraftCommentEvent> events, Change change)
throws OrmException {
NoteDbUpdateManager manager, EventList<DraftCommentEvent> events, Change change) {
if (events.isEmpty()) {
return;
}

19
java/com/google/gerrit/server/notedb/rebuild/CommentEvent.java
View File

@@ -24,9 +24,13 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gwtorm.server.OrmException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
class CommentEvent extends Event {
private static final Logger log = LoggerFactory.getLogger(CommentEvent.class);
public final Comment c;
private final Change change;
private final PatchSet ps;
@@ -57,10 +61,19 @@ class CommentEvent extends Event {
}
@Override
void apply(ChangeUpdate update) throws OrmException {
void apply(ChangeUpdate update) {
checkUpdate(update);
if (c.revId == null) {
setCommentRevId(c, cache, change, ps);
try {
setCommentRevId(c, cache, change, ps);
} catch (PatchListNotAvailableException e) {
log.warn(
"Unable to determine parent commit of patch set {} ({}); omitting inline comment",
ps.getId(),
ps.getRevision(),
c);
return;
}
}
update.putComment(PatchLineComment.Status.PUBLISHED, c);
}

19
java/com/google/gerrit/server/notedb/rebuild/DraftCommentEvent.java
View File

@@ -24,9 +24,13 @@ import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.notedb.ChangeDraftUpdate;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gwtorm.server.OrmException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
class DraftCommentEvent extends Event {
private static final Logger log = LoggerFactory.getLogger(DraftCommentEvent.class);
public final Comment c;
private final Change change;
private final PatchSet ps;
@@ -56,9 +60,18 @@ class DraftCommentEvent extends Event {
throw new UnsupportedOperationException();
}
void applyDraft(ChangeDraftUpdate draftUpdate) throws OrmException {
void applyDraft(ChangeDraftUpdate draftUpdate) {
if (c.revId == null) {
setCommentRevId(c, cache, change, ps);
try {
setCommentRevId(c, cache, change, ps);
} catch (PatchListNotAvailableException e) {
log.warn(
"Unable to determine parent commit of patch set {} ({}); omitting draft inline comment",
ps.getId(),
ps.getRevision(),
c);
return;
}
}
draftUpdate.putComment(c);
}

48
java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
View File

@@ -33,6 +33,7 @@ import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Stopwatch;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.Iterables;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.Ordering;
import com.google.common.collect.SetMultimap;
@@ -66,6 +67,7 @@ import com.google.gerrit.server.notedb.NoteDbTable;
import com.google.gerrit.server.notedb.NoteDbUpdateManager;
import com.google.gerrit.server.notedb.NotesMigrationState;
import com.google.gerrit.server.notedb.PrimaryStorageMigrator;
import com.google.gerrit.server.notedb.PrimaryStorageMigrator.NoNoteDbStateException;
import com.google.gerrit.server.notedb.RepoSequence;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException;
import com.google.gerrit.server.project.NoSuchChangeException;
@@ -605,7 +607,19 @@ public class NoteDbMigrator implements AutoCloseable {
executor.submit(
() -> {
try (ManualRequestContext ctx = contextHelper.open()) {
primaryStorageMigrator.migrateToNoteDbPrimary(id);
try {
primaryStorageMigrator.migrateToNoteDbPrimary(id);
} catch (NoNoteDbStateException e) {
if (canSkipPrimaryStorageMigration(
ctx.getReviewDbProvider().get(), id)) {
log.warn(
"Change {} previously failed to rebuild;"
+ " skipping primary storage migration",
e);
} else {
throw e;
}
}
return true;
} catch (Exception e) {
log.error("Error migrating primary storage for " + id, e);
@@ -628,6 +642,38 @@ public class NoteDbMigrator implements AutoCloseable {
return disableReviewDb(prev);
}
/**
* Checks whether a change is so corrupt that it can be completely skipped by the primary storage
* migration step.
*
* <p>To get to the point where this method is called from {@link #setNoteDbPrimary}, it means we
* attempted to rebuild it, and encountered an error that was then caught in {@link
* #rebuildProject} and skipped. As a result, there is no {@code noteDbState} field in the change
* by the time we get to {@link #setNoteDbPrimary}, so {@code migrateToNoteDbPrimary} throws an
* exception.
*
* <p>We have to do this hacky double-checking because we don't have a way for the rebuilding
* phase to communicate to the primary storage migration phase that the change is skippable. It
* would be possible to store this info in some field in this class, but there is no guarantee
* that the rebuild and primary storage migration phases are run in the same JVM invocation.
*
* <p>In an ideal world, we could do this through the {@link
* com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage} enum, having a separate value
* for errors. However, that would be an invasive change touching many non-migration-related parts
* of the NoteDb migration code, which is too risky to attempt in the stable branch where this bug
* had to be fixed.
*
* <p>As of this writing, the only case where this happens is when a change has no patch sets.
*/
private static boolean canSkipPrimaryStorageMigration(ReviewDb db, Change.Id id) {
try {
return Iterables.isEmpty(db.patchSets().byChange(id));
} catch (Exception e) {
log.error("Error checking if change " + id + " is corrupt, assuming yes", e);
return true;
}
}
private NotesMigrationState disableReviewDb(NotesMigrationState prev) throws IOException {
return saveState(prev, NOTE_DB, c -> setAutoMigrate(c, false));
}

1
java/com/google/gerrit/server/project/ProjectCacheImpl.java
View File

@@ -155,6 +155,7 @@ public class ProjectCacheImpl implements ProjectCache {
Throwables.throwIfInstanceOf(e.getCause(), IOException.class);
throw new IOException(e);
}
log.warn("Cannot find project {}", projectName.get(), e);
return null;
}
}

4
java/com/google/gerrit/server/restapi/change/CreateDraftComment.java
View File

@@ -34,6 +34,7 @@ import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -108,7 +109,8 @@ public class CreateDraftComment
@Override
public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, OrmException, UnprocessableEntityException {
throws ResourceNotFoundException, OrmException, UnprocessableEntityException,
PatchListNotAvailableException {
PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
if (ps == null) {
throw new ResourceNotFoundException("patch set not found: " + psId);

4
java/com/google/gerrit/server/restapi/change/DeleteDraftComment.java
View File

@@ -29,6 +29,7 @@ import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.DraftCommentResource;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -87,7 +88,8 @@ public class DeleteDraftComment
}
@Override
public boolean updateChange(ChangeContext ctx) throws ResourceNotFoundException, OrmException {
public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, OrmException, PatchListNotAvailableException {
Optional<Comment> maybeComment =
commentsUtil.getDraft(ctx.getDb(), ctx.getNotes(), ctx.getIdentifiedUser(), key);
if (!maybeComment.isPresent()) {

14
java/com/google/gerrit/server/restapi/change/PostReview.java
View File

@@ -839,7 +839,8 @@ public class PostReview
@Override
public boolean updateChange(ChangeContext ctx)
throws OrmException, ResourceConflictException, UnprocessableEntityException, IOException {
throws OrmException, ResourceConflictException, UnprocessableEntityException, IOException,
PatchListNotAvailableException {
user = ctx.getIdentifiedUser();
notes = ctx.getNotes();
ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
@@ -881,7 +882,7 @@ public class PostReview
}
private boolean insertComments(ChangeContext ctx)
throws OrmException, UnprocessableEntityException {
throws OrmException, UnprocessableEntityException, PatchListNotAvailableException {
Map<String, List<CommentInput>> map = in.comments;
if (map == null) {
map = Collections.emptyMap();
@@ -941,7 +942,8 @@ public class PostReview
return !toPublish.isEmpty();
}
private boolean insertRobotComments(ChangeContext ctx) throws OrmException {
private boolean insertRobotComments(ChangeContext ctx)
throws OrmException, PatchListNotAvailableException {
if (in.robotComments == null) {
return false;
}
@@ -952,7 +954,8 @@ public class PostReview
return !newRobotComments.isEmpty();
}
private List<RobotComment> getNewRobotComments(ChangeContext ctx) throws OrmException {
private List<RobotComment> getNewRobotComments(ChangeContext ctx)
throws OrmException, PatchListNotAvailableException {
List<RobotComment> toAdd = new ArrayList<>(in.robotComments.size());
Set<CommentSetEntry> existingIds =
@@ -972,7 +975,8 @@ public class PostReview
}
private RobotComment createRobotCommentFromInput(
ChangeContext ctx, String path, RobotCommentInput robotCommentInput) throws OrmException {
ChangeContext ctx, String path, RobotCommentInput robotCommentInput)
throws PatchListNotAvailableException {
RobotComment robotComment =
commentsUtil.newRobotComment(
ctx,

4
java/com/google/gerrit/server/restapi/change/PutDraftComment.java
View File

@@ -33,6 +33,7 @@ import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.DraftCommentResource;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -113,7 +114,8 @@ public class PutDraftComment
}
@Override
public boolean updateChange(ChangeContext ctx) throws ResourceNotFoundException, OrmException {
public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, OrmException, PatchListNotAvailableException {
Optional<Comment> maybeComment =
commentsUtil.getDraft(ctx.getDb(), ctx.getNotes(), ctx.getIdentifiedUser(), key);
if (!maybeComment.isPresent()) {

48
java/com/google/gerrit/server/schema/Schema_160.java
View File

@@ -18,6 +18,7 @@ import static com.google.gerrit.server.git.UserConfigSections.KEY_URL;
import static com.google.gerrit.server.git.UserConfigSections.MY;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -40,17 +41,34 @@ import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.lib.TextProgressMonitor;
/**
* Remove "My Drafts" menu items for all users.
* Remove "My Drafts" menu items for all users and server-wide default preferences.
*
* <p>Since draft changes no longer exist, these menu items are obsolete.
*
* <p>Only matches menu items (with any name) where the URL exactly matches the <a
* <p>Only matches menu items (with any name) where the URL exactly matches one of the following,
* with or without leading {@code #}:
*
* <ul>
* <li>/q/is:draft
* <li>/q/owner:self+is:draft
* </ul>
*
* In particular, this includes the <a
* href="https://gerrit.googlesource.com/gerrit/+/v2.14.4/gerrit-server/src/main/java/com/google/gerrit/server/account/GeneralPreferencesLoader.java#144">default
* version from 2.14 and earlier</a>. Other menus containing {@code is:draft} in other positions are
* not affected; this is still a valid predicate that matches no changes.
* from version 2.14 and earlier</a>.
*
* <p>Other menus containing {@code is:draft} in other positions are not affected; this is still a
* valid predicate that matches no changes.
*/
public class Schema_160 extends SchemaVersion {
@VisibleForTesting static final String DEFAULT_DRAFT_ITEM = "#/q/owner:self+is:draft";
@VisibleForTesting static final ImmutableList<String> DEFAULT_DRAFT_ITEMS;
static {
String ownerSelfIsDraft = "/q/owner:self+is:draft";
String isDraft = "/q/is:draft";
DEFAULT_DRAFT_ITEMS =
ImmutableList.of(ownerSelfIsDraft, '#' + ownerSelfIsDraft, isDraft, '#' + isDraft);
}
private final GitRepositoryManager repoManager;
private final AllUsersName allUsersName;
@@ -75,10 +93,9 @@ public class Schema_160 extends SchemaVersion {
ProgressMonitor pm = new TextProgressMonitor();
pm.beginTask("Removing \"My Drafts\" menu items", ProgressMonitor.UNKNOWN);
for (Account.Id id : (Iterable<Account.Id>) Accounts.readUserRefs(repo)::iterator) {
if (removeMyDrafts(repo, id)) {
pm.update(1);
}
removeMyDrafts(repo, RefNames.refsUsers(id), pm);
}
removeMyDrafts(repo, RefNames.REFS_USERS_DEFAULT, pm);
pm.endTask();
}
} catch (IOException | ConfigInvalidException e) {
@@ -86,24 +103,26 @@ public class Schema_160 extends SchemaVersion {
}
}
private boolean removeMyDrafts(Repository repo, Account.Id id)
private void removeMyDrafts(Repository repo, String ref, ProgressMonitor pm)
throws IOException, ConfigInvalidException {
MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsersName, repo);
PersonIdent ident = serverIdent.get();
md.getCommitBuilder().setAuthor(ident);
md.getCommitBuilder().setCommitter(ident);
Prefs prefs = new Prefs(id);
Prefs prefs = new Prefs(ref);
prefs.load(repo);
prefs.removeMyDrafts();
prefs.commit(md);
return prefs.dirty();
if (prefs.dirty()) {
pm.update(1);
}
}
private static class Prefs extends VersionedAccountPreferences {
private boolean dirty;
Prefs(Account.Id id) {
super(RefNames.refsUsers(id));
Prefs(String ref) {
super(ref);
}
@Override
@@ -118,7 +137,8 @@ public class Schema_160 extends SchemaVersion {
void removeMyDrafts() {
Config cfg = getConfig();
for (String item : cfg.getSubsections(MY)) {
if (DEFAULT_DRAFT_ITEM.equals(cfg.getString(MY, item, KEY_URL))) {
String value = cfg.getString(MY, item, KEY_URL);
if (DEFAULT_DRAFT_ITEMS.contains(value)) {
cfg.unsetSection(MY, item);
dirty = true;
}

6
java/com/google/gerrit/server/schema/VersionedAccountPreferences.java
View File

@@ -25,12 +25,16 @@ import org.eclipse.jgit.lib.Config;
/** Preferences for user accounts during schema migrations. */
class VersionedAccountPreferences extends VersionedMetaData {
private static final String PREFERENCES = "preferences.config";
static final String PREFERENCES = "preferences.config";
static VersionedAccountPreferences forUser(Account.Id id) {
return new VersionedAccountPreferences(RefNames.refsUsers(id));
}
static VersionedAccountPreferences forDefault() {
return new VersionedAccountPreferences(RefNames.REFS_USERS_DEFAULT);
}
private final String ref;
private Config cfg;

35
java/com/google/gerrit/testing/NoteDbChecker.java
View File

@@ -19,6 +19,8 @@ import static java.util.Comparator.comparing;
import static java.util.stream.Collectors.toList;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
@@ -78,14 +80,17 @@ public class NoteDbChecker {
}
public void rebuildAndCheckAllChanges() throws Exception {
rebuildAndCheckChanges(getUnwrappedDb().changes().all().toList().stream().map(Change::getId));
rebuildAndCheckChanges(
getUnwrappedDb().changes().all().toList().stream().map(Change::getId),
ImmutableListMultimap.of());
}
public void rebuildAndCheckChanges(Change.Id... changeIds) throws Exception {
rebuildAndCheckChanges(Arrays.stream(changeIds));
rebuildAndCheckChanges(Arrays.stream(changeIds), ImmutableListMultimap.of());
}
private void rebuildAndCheckChanges(Stream<Change.Id> changeIds) throws Exception {
private void rebuildAndCheckChanges(
Stream<Change.Id> changeIds, ListMultimap<Change.Id, String> expectedDiffs) throws Exception {
ReviewDb db = getUnwrappedDb();
List<ChangeBundle> allExpected = readExpected(changeIds);
@@ -105,7 +110,7 @@ public class NoteDbChecker {
}
}
checkActual(allExpected, msgs);
checkActual(allExpected, expectedDiffs, msgs);
} finally {
notesMigration.setReadChanges(oldRead);
notesMigration.setWriteChanges(oldWrite);
@@ -113,7 +118,14 @@ public class NoteDbChecker {
}
public void checkChanges(Change.Id... changeIds) throws Exception {
checkActual(readExpected(Arrays.stream(changeIds)), new ArrayList<>());
checkActual(
readExpected(Arrays.stream(changeIds)), ImmutableListMultimap.of(), new ArrayList<>());
}
public void rebuildAndCheckChange(Change.Id changeId, String... expectedDiff) throws Exception {
ImmutableListMultimap.Builder<Change.Id, String> b = ImmutableListMultimap.builder();
b.putAll(changeId, Arrays.asList(expectedDiff));
rebuildAndCheckChanges(Stream.of(changeId), b.build());
}
public void assertNoChangeRef(Project.NameKey project, Change.Id changeId) throws Exception {
@@ -160,7 +172,11 @@ public class NoteDbChecker {
}
}
private void checkActual(List<ChangeBundle> allExpected, List<String> msgs) throws Exception {
private void checkActual(
List<ChangeBundle> allExpected,
ListMultimap<Change.Id, String> expectedDiffs,
List<String> msgs)
throws Exception {
ReviewDb db = getUnwrappedDb();
boolean oldRead = notesMigration.readChanges();
boolean oldWrite = notesMigration.rawWriteChangesSetting();
@@ -181,9 +197,14 @@ public class NoteDbChecker {
continue;
}
List<String> diff = expected.differencesFrom(actual);
if (!diff.isEmpty()) {
List<String> expectedDiff = expectedDiffs.get(c.getId());
if (!diff.equals(expectedDiff)) {
msgs.add("Differences between ReviewDb and NoteDb for " + c + ":");
msgs.addAll(diff);
if (!expectedDiff.isEmpty()) {
msgs.add("Expected differences:");
msgs.addAll(expectedDiff);
}
msgs.add("");
} else {
System.err.println("NoteDb conversion of change " + c.getId() + " successful");

3
javatests/com/google/gerrit/acceptance/server/notedb/BUILD
View File

@@ -7,5 +7,8 @@ acceptance_tests(
"notedb",
"server",
],
# TODO(dborowitz): Fix leaks in local disk tests so we can reduce heap size.
# http://crbug.com/gerrit/8567
vm_args = ["-Xmx1024m"],
deps = ["//java/com/google/gerrit/server/schema"],
)

127
javatests/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.server.notedb;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assert_;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
@@ -23,6 +24,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.concurrent.TimeUnit.DAYS;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
@@ -53,6 +55,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.ChangeUtil;
@@ -69,6 +72,8 @@ import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.NoteDbUpdateManager;
import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.project.testing.Util;
@@ -95,6 +100,7 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;
import org.apache.http.Header;
import org.apache.http.message.BasicHeader;
import org.eclipse.jgit.junit.TestRepository;
@@ -145,6 +151,8 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
@Inject private PatchSetInfoFactory patchSetInfoFactory;
@Inject private PatchListCache patchListCache;
@Before
public void setUp() throws Exception {
assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.OFF);
@@ -1017,8 +1025,43 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
db.rollback();
}
exception.expect(NoPatchSetsException.class);
checker.rebuildAndCheckChanges(id);
try {
checker.rebuildAndCheckChanges(id);
assert_().fail("expected NoPatchSetsException");
} catch (NoPatchSetsException e) {
// Expected.
}
Change c = db.changes().get(id);
assertThat(c.getNoteDbState()).isNull();
checker.assertNoChangeRef(project, id);
}
@Test
public void rebuildChangeWithNoEntitiesOtherThanChange() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey();
db.changes().beginTransaction(id);
try {
db.changeMessages().delete(db.changeMessages().byChange(id));
db.patchSets().delete(db.patchSets().byChange(id));
db.patchSetApprovals().delete(db.patchSetApprovals().byChange(id));
db.patchComments().delete(db.patchComments().byChange(id));
db.commit();
} finally {
db.rollback();
}
try {
checker.rebuildAndCheckChanges(id);
assert_().fail("expected NoPatchSetsException");
} catch (NoPatchSetsException e) {
// Expected.
}
Change c = db.changes().get(id);
assertThat(c.getNoteDbState()).isNull();
checker.assertNoChangeRef(project, id);
}
@Test
@@ -1339,6 +1382,67 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
assertChangeUpToDate(true, id);
}
@Test
public void missingPatchSetCommitOkForCommentsNotOnParentSide() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();
putDraft(user, id, 1, "draft comment", null, Side.REVISION);
putComment(user, id, 1, "published comment", null, Side.REVISION);
ReviewDb db = getUnwrappedDb();
PatchSet ps = db.patchSets().get(new PatchSet.Id(id, 1));
ps.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
db.patchSets().update(Collections.singleton(ps));
try {
patchListCache.getOldId(db.changes().get(id), ps, null);
assert_().fail("Expected PatchListNotAvailableException");
} catch (PatchListNotAvailableException e) {
// Expected.
}
checker.rebuildAndCheckChanges(id);
}
@Test
public void missingPatchSetCommitOmitsCommentsOnParentSide() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();
CommentInfo draftInfo = putDraft(user, id, 1, "draft comment", null, Side.PARENT);
putComment(user, id, 1, "published comment", null, Side.PARENT);
CommentInfo commentInfo =
gApi.changes()
.id(id.get())
.comments()
.values()
.stream()
.flatMap(List::stream)
.findFirst()
.get();
ReviewDb db = getUnwrappedDb();
PatchSet ps = db.patchSets().get(new PatchSet.Id(id, 1));
ps.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
db.patchSets().update(Collections.singleton(ps));
try {
patchListCache.getOldId(db.changes().get(id), ps, null);
assert_().fail("Expected PatchListNotAvailableException");
} catch (PatchListNotAvailableException e) {
// Expected.
}
checker.rebuildAndCheckChange(
id,
Stream.of(draftInfo.id, commentInfo.id)
.sorted()
.map(c -> id + ",1," + PushOneCommit.FILE_NAME + "," + c)
.collect(
joining(", ", "PatchLineComment.Key sets differ: [", "] only in A; [] only in B")));
}
private void assertChangesReadOnly(RestApiException e) throws Exception {
Throwable cause = e.getCause();
assertThat(cause).isInstanceOf(UpdateException.class);
@@ -1388,16 +1492,24 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
}
}
private void putDraft(TestAccount account, Change.Id id, int line, String msg, Boolean unresolved)
private CommentInfo putDraft(
TestAccount account, Change.Id id, int line, String msg, Boolean unresolved)
throws Exception {
return putDraft(account, id, line, msg, unresolved, Side.REVISION);
}
private CommentInfo putDraft(
TestAccount account, Change.Id id, int line, String msg, Boolean unresolved, Side side)
throws Exception {
DraftInput in = new DraftInput();
in.side = side;
in.line = line;
in.message = msg;
in.path = PushOneCommit.FILE_NAME;
in.unresolved = unresolved;
AcceptanceTestRequestScope.Context old = setApiUser(account);
try {
gApi.changes().id(id.get()).current().createDraft(in);
return gApi.changes().id(id.get()).current().createDraft(in).get();
} finally {
atrScope.set(old);
}
@@ -1405,7 +1517,14 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
private void putComment(TestAccount account, Change.Id id, int line, String msg, String inReplyTo)
throws Exception {
putComment(account, id, line, msg, inReplyTo, Side.REVISION);
}
private void putComment(
TestAccount account, Change.Id id, int line, String msg, String inReplyTo, Side side)
throws Exception {
CommentInput in = new CommentInput();
in.side = side;
in.line = line;
in.message = msg;
in.inReplyTo = inReplyTo;

73
javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
View File

@@ -18,6 +18,7 @@ import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.server.notedb.NoteDbChangeState.NOTE_DB_PRIMARY_STATE;
import static com.google.gerrit.server.notedb.NotesMigrationState.NOTE_DB;
import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_NO_SEQUENCE;
import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY;
@@ -43,11 +44,15 @@ import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.notedb.ChangeBundle;
import com.google.gerrit.server.notedb.ChangeBundleReader;
import com.google.gerrit.server.notedb.NoteDbChangeState;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.NoteDbChangeState.RefState;
@@ -76,6 +81,7 @@ import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.storage.file.FileBasedConfig;
import org.eclipse.jgit.util.FS;
@@ -101,10 +107,12 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
// migration state may result in various kinds of wrappers showing up unexpectedly.
@Inject @ReviewDbFactory private SchemaFactory<ReviewDb> schemaFactory;
@Inject private SitePaths sitePaths;
@Inject private ChangeBundleReader changeBundleReader;
@Inject private CommentsUtil commentsUtil;
@Inject private DynamicSet<NotesMigrationStateListener> listeners;
@Inject private Provider<NoteDbMigrator.Builder> migratorBuilderProvider;
@Inject private Sequences sequences;
@Inject private DynamicSet<NotesMigrationStateListener> listeners;
@Inject private SitePaths sitePaths;
private FileBasedConfig noteDbConfig;
private List<RegistrationHandle> addedListeners;
@@ -432,6 +440,67 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
}
}
@Test
public void fullMigrationOneChangeWithNoPatchSets() throws Exception {
PushOneCommit.Result r1 = createChange();
PushOneCommit.Result r2 = createChange();
Change.Id id1 = r1.getChange().getId();
Change.Id id2 = r2.getChange().getId();
db.changes().beginTransaction(id2);
try {
db.patchSets().delete(db.patchSets().byChange(id2));
db.commit();
} finally {
db.rollback();
}
migrate(b -> b);
assertNotesMigrationState(NOTE_DB, false, false);
try (ReviewDb db = schemaFactory.open();
Repository repo = repoManager.openRepository(project)) {
assertThat(repo.exactRef(RefNames.changeMetaRef(id1))).isNotNull();
assertThat(db.changes().get(id1).getNoteDbState()).isEqualTo(NOTE_DB_PRIMARY_STATE);
// A change with no patch sets is so corrupt that it is completely skipped by the migration
// process.
assertThat(repo.exactRef(RefNames.changeMetaRef(id2))).isNull();
assertThat(db.changes().get(id2).getNoteDbState()).isNull();
}
}
@Test
public void fullMigrationMissingPatchSetRefs() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();
try (Repository repo = repoManager.openRepository(project)) {
RefUpdate u = repo.updateRef(new PatchSet.Id(id, 1).toRefName());
u.setForceUpdate(true);
assertThat(u.delete()).isEqualTo(RefUpdate.Result.FORCED);
}
ChangeBundle reviewDbBundle;
try (ReviewDb db = schemaFactory.open()) {
reviewDbBundle = changeBundleReader.fromReviewDb(db, id);
}
migrate(b -> b);
assertNotesMigrationState(NOTE_DB, false, false);
try (ReviewDb db = schemaFactory.open();
Repository repo = repoManager.openRepository(project)) {
// Change migrated successfully even though it was missing patch set refs.
assertThat(repo.exactRef(RefNames.changeMetaRef(id))).isNotNull();
assertThat(db.changes().get(id).getNoteDbState()).isEqualTo(NOTE_DB_PRIMARY_STATE);
ChangeBundle noteDbBundle =
ChangeBundle.fromNotes(commentsUtil, notesFactory.createChecked(db, project, id));
assertThat(noteDbBundle.differencesFrom(reviewDbBundle)).isEmpty();
}
}
@Test
public void autoMigrationConfig() throws Exception {
createChange();

130
javatests/com/google/gerrit/server/schema/Schema_159_to_160_Test.java
View File

@@ -14,14 +14,19 @@
package com.google.gerrit.server.schema;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS_DEFAULT;
import static com.google.gerrit.server.git.UserConfigSections.KEY_URL;
import static com.google.gerrit.server.git.UserConfigSections.MY;
import static com.google.gerrit.server.schema.Schema_160.DEFAULT_DRAFT_ITEM;
import static com.google.gerrit.server.schema.Schema_160.DEFAULT_DRAFT_ITEMS;
import static com.google.gerrit.server.schema.VersionedAccountPreferences.PREFERENCES;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
import com.google.gerrit.extensions.client.MenuItem;
@@ -38,8 +43,13 @@ import com.google.gerrit.testing.TestUpdateUI;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.List;
import java.util.Optional;
import java.util.function.Supplier;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.BlobBasedConfig;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.junit.Before;
import org.junit.Rule;
@@ -67,8 +77,12 @@ public class Schema_159_to_160_Test {
@Test
public void skipUnmodified() throws Exception {
ObjectId oldMetaId = metaRef(accountId);
assertThat(myMenusFromNoteDb(accountId).values()).doesNotContain(DEFAULT_DRAFT_ITEM);
assertThat(myMenusFromApi(accountId).values()).doesNotContain(DEFAULT_DRAFT_ITEM);
ImmutableSet<String> fromNoteDb = myMenusFromNoteDb(accountId);
ImmutableSet<String> fromApi = myMenusFromApi(accountId);
for (String item : DEFAULT_DRAFT_ITEMS) {
assertThat(fromNoteDb).doesNotContain(item);
assertThat(fromApi).doesNotContain(item);
}
schema160.migrateData(db, new TestUpdateUI());
@@ -78,22 +92,25 @@ public class Schema_159_to_160_Test {
@Test
public void deleteItems() throws Exception {
ObjectId oldMetaId = metaRef(accountId);
List<String> defaultNames = ImmutableList.copyOf(myMenusFromApi(accountId).keySet());
ImmutableSet<String> defaultNames = myMenusFromApi(accountId);
GeneralPreferencesInfo prefs = gApi.accounts().id(accountId.get()).getPreferences();
prefs.my.add(0, new MenuItem("Something else", DEFAULT_DRAFT_ITEM + "+is:mergeable"));
prefs.my.add(new MenuItem("Drafts", DEFAULT_DRAFT_ITEM));
prefs.my.add(new MenuItem("Totally not drafts", DEFAULT_DRAFT_ITEM));
prefs.my.add(0, new MenuItem("Something else", DEFAULT_DRAFT_ITEMS.get(0) + "+is:mergeable"));
for (int i = 0; i < DEFAULT_DRAFT_ITEMS.size(); i++) {
prefs.my.add(new MenuItem("Draft entry " + i, DEFAULT_DRAFT_ITEMS.get(i)));
}
gApi.accounts().id(accountId.get()).setPreferences(prefs);
List<String> oldNames =
ImmutableList.<String>builder()
.add("Something else")
.addAll(defaultNames)
.add("Drafts")
.add("Totally not drafts")
.add("Draft entry 0")
.add("Draft entry 1")
.add("Draft entry 2")
.add("Draft entry 3")
.build();
assertThat(myMenusFromApi(accountId).keySet()).containsExactlyElementsIn(oldNames).inOrder();
assertThat(myMenusFromApi(accountId)).containsExactlyElementsIn(oldNames).inOrder();
schema160.migrateData(db, new TestUpdateUI());
accountCache.evict(accountId);
@@ -104,14 +121,74 @@ public class Schema_159_to_160_Test {
List<String> newNames =
ImmutableList.<String>builder().add("Something else").addAll(defaultNames).build();
assertThat(myMenusFromNoteDb(accountId).keySet()).containsExactlyElementsIn(newNames).inOrder();
assertThat(myMenusFromApi(accountId).keySet()).containsExactlyElementsIn(newNames).inOrder();
assertThat(myMenusFromNoteDb(accountId)).containsExactlyElementsIn(newNames).inOrder();
assertThat(myMenusFromApi(accountId)).containsExactlyElementsIn(newNames).inOrder();
}
@Test
public void skipNonExistentRefsUsersDefault() throws Exception {
assertThat(readRef(REFS_USERS_DEFAULT)).isEmpty();
schema160.migrateData(db, new TestUpdateUI());
assertThat(readRef(REFS_USERS_DEFAULT)).isEmpty();
}
@Test
public void deleteDefaultItem() throws Exception {
assertThat(readRef(REFS_USERS_DEFAULT)).isEmpty();
ImmutableSet<String> defaultNames = defaultMenusFromApi();
// Setting *any* preference causes preferences.config to contain the full set of "my" sections.
// This mimics real-world behavior prior to the 2.15 upgrade; see Issue 8439 for details.
GeneralPreferencesInfo prefs = gApi.config().server().getDefaultPreferences();
prefs.signedOffBy = !firstNonNull(prefs.signedOffBy, false);
gApi.config().server().setDefaultPreferences(prefs);
try (Repository repo = repoManager.openRepository(allUsersName)) {
Config cfg = new BlobBasedConfig(null, repo, readRef(REFS_USERS_DEFAULT).get(), PREFERENCES);
assertThat(cfg.getSubsections("my")).containsExactlyElementsIn(defaultNames).inOrder();
// Add more defaults directly in git, the SetPreferences endpoint doesn't respect the "my"
// field in the input in 2.15 and earlier.
cfg.setString("my", "Drafts", "url", "#/q/owner:self+is:draft");
cfg.setString("my", "Something else", "url", "#/q/owner:self+is:draft+is:mergeable");
cfg.setString("my", "Totally not drafts", "url", "#/q/owner:self+is:draft");
new TestRepository<>(repo)
.branch(REFS_USERS_DEFAULT)
.commit()
.add(PREFERENCES, cfg.toText())
.create();
}
List<String> oldNames =
ImmutableList.<String>builder()
.addAll(defaultNames)
.add("Drafts")
.add("Something else")
.add("Totally not drafts")
.build();
assertThat(defaultMenusFromApi()).containsExactlyElementsIn(oldNames).inOrder();
schema160.migrateData(db, new TestUpdateUI());
assertThat(readRef(REFS_USERS_DEFAULT)).isPresent();
List<String> newNames =
ImmutableList.<String>builder().addAll(defaultNames).add("Something else").build();
assertThat(myMenusFromNoteDb(VersionedAccountPreferences::forDefault).keySet())
.containsExactlyElementsIn(newNames)
.inOrder();
assertThat(defaultMenusFromApi()).containsExactlyElementsIn(newNames).inOrder();
}
private ImmutableSet<String> myMenusFromNoteDb(Account.Id id) throws Exception {
return myMenusFromNoteDb(() -> VersionedAccountPreferences.forUser(id)).keySet();
}
// Raw config values, bypassing the defaults set by PreferencesConfig.
private ImmutableMap<String, String> myMenusFromNoteDb(Account.Id id) throws Exception {
private ImmutableMap<String, String> myMenusFromNoteDb(
Supplier<VersionedAccountPreferences> prefsSupplier) throws Exception {
try (Repository repo = repoManager.openRepository(allUsersName)) {
VersionedAccountPreferences prefs = VersionedAccountPreferences.forUser(id);
VersionedAccountPreferences prefs = prefsSupplier.get();
prefs.load(repo);
Config cfg = prefs.getConfig();
return cfg.getSubsections(MY)
@@ -120,18 +197,27 @@ public class Schema_159_to_160_Test {
}
}
private ImmutableMap<String, String> myMenusFromApi(Account.Id id) throws Exception {
return gApi.accounts()
.id(id.get())
.getPreferences()
.my
.stream()
.collect(toImmutableMap(i -> i.name, i -> i.url));
private ImmutableSet<String> myMenusFromApi(Account.Id id) throws Exception {
return myMenus(gApi.accounts().id(id.get()).getPreferences()).keySet();
}
private ImmutableSet<String> defaultMenusFromApi() throws Exception {
return myMenus(gApi.config().server().getDefaultPreferences()).keySet();
}
private static ImmutableMap<String, String> myMenus(GeneralPreferencesInfo prefs) {
return prefs.my.stream().collect(toImmutableMap(i -> i.name, i -> i.url));
}
private ObjectId metaRef(Account.Id id) throws Exception {
return readRef(RefNames.refsUsers(id))
.orElseThrow(() -> new AssertionError("missing ref for account " + id));
}
private Optional<ObjectId> readRef(String ref) throws Exception {
try (Repository repo = repoManager.openRepository(allUsersName)) {
return repo.exactRef(RefNames.refsUsers(id)).getObjectId();
return Optional.ofNullable(repo.exactRef(ref)).map(Ref::getObjectId);
}
}
}