Merge branch 'stable-2.16' into stable-3.0

* stable-2.16:
  ChangeIdHandler: Make assumption on number of query results explicit
  ChangeIdHandler: Remove unnecessary 'final' modifiers
  Update git submodules
  AbstractQueryChangesTest: Add method to create change as private
  AbstractQueryChangesTest: Fix comment in byPrivate
  AbstractQueryChangesTest: Use overloaded newChange method where possible
  AbstractQueryChangesTest: Extend testing of visibleto predicate
  ChangeQueryBuilder: Rename status_open to statusOpen
  ChangeQueryBuilder: Rename is_visible to isVisible
  Extract duplicated code of {Ssh|Http}LogJsonLayout
  Add option to log SSH events in JSON format
  Remove duplicated constants storing key names of Ssh logs
  Add option to log HTTP logging events in JSON format

Change-Id: Ib4de5b560a75ea85ca15581d73dde24baf83c1b4

Documentation/config-gerrit.txt

@@ -3464,7 +3464,8 @@ By default unset.
 
 [[log.jsonLogging]]log.jsonLogging::
 +
-If set to true, enables error logging in JSON format (file name: "logs/error_log.json").
+If set to true, enables error, ssh and http logging in JSON format (file name:
+"logs/{error|sshd|httpd}_log.json").
 +
 Defaults to false.
 

java/com/google/gerrit/pgm/http/jetty/BUILD

@@ -10,9 +10,11 @@ java_library(
         "//java/com/google/gerrit/lifecycle",
         "//java/com/google/gerrit/metrics",
         "//java/com/google/gerrit/server",
+        "//java/com/google/gerrit/server/logging",
         "//java/com/google/gerrit/server/util/time",
         "//java/com/google/gerrit/sshd",
         "//java/com/google/gerrit/util/http",
+        "//lib:gson",
         "//lib:guava",
         "//lib:servlet-api-3_1",
         "//lib/flogger:api",

java/com/google/gerrit/pgm/http/jetty/HttpLog.java

@@ -17,6 +17,7 @@ package com.google.gerrit.pgm.http.jetty;
 import com.google.common.base.Strings;
 import com.google.gerrit.httpd.GetUserFilter;
 import com.google.gerrit.httpd.restapi.LogRedactUtil;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.util.SystemLog;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.inject.Inject;
@@ -28,11 +29,13 @@ import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.RequestLog;
 import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.util.component.AbstractLifeCycle;
+import org.eclipse.jgit.lib.Config;
 
 /** Writes the {@code httpd_log} file with per-request data. */
 class HttpLog extends AbstractLifeCycle implements RequestLog {
   private static final Logger log = Logger.getLogger(HttpLog.class);
   private static final String LOG_NAME = "httpd_log";
+  private static final String JSON_SUFFIX = ".json";
 
   interface HttpLogFactory {
     HttpLog get();
@@ -52,8 +55,20 @@ class HttpLog extends AbstractLifeCycle implements RequestLog {
   private final AsyncAppender async;
 
   @Inject
-  HttpLog(SystemLog systemLog) {
-    async = systemLog.createAsyncAppender(LOG_NAME, new HttpLogLayout());
+  HttpLog(SystemLog systemLog, @GerritServerConfig Config config) {
+    boolean json = config.getBoolean("log", "jsonLogging", false);
+    boolean text = config.getBoolean("log", "textLogging", true) || !json;
+
+    async = new AsyncAppender();
+
+    if (text) {
+      async.addAppender(systemLog.createAsyncAppender(LOG_NAME, new HttpLogLayout()));
+    }
+
+    if (json) {
+      async.addAppender(
+          systemLog.createAsyncAppender(LOG_NAME + JSON_SUFFIX, new HttpLogJsonLayout()));
+    }
   }
 
   @Override

java/com/google/gerrit/pgm/http/jetty/HttpLogJsonLayout.java

@@ -0,0 +1,72 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.pgm.http.jetty;
+
+import static com.google.gerrit.pgm.http.jetty.HttpLog.P_CONTENT_LENGTH;
+import static com.google.gerrit.pgm.http.jetty.HttpLog.P_HOST;
+import static com.google.gerrit.pgm.http.jetty.HttpLog.P_METHOD;
+import static com.google.gerrit.pgm.http.jetty.HttpLog.P_PROTOCOL;
+import static com.google.gerrit.pgm.http.jetty.HttpLog.P_REFERER;
+import static com.google.gerrit.pgm.http.jetty.HttpLog.P_RESOURCE;
+import static com.google.gerrit.pgm.http.jetty.HttpLog.P_STATUS;
+import static com.google.gerrit.pgm.http.jetty.HttpLog.P_USER;
+import static com.google.gerrit.pgm.http.jetty.HttpLog.P_USER_AGENT;
+
+import com.google.gerrit.server.logging.JsonLayout;
+import com.google.gerrit.server.logging.JsonLogEntry;
+import java.time.format.DateTimeFormatter;
+import org.apache.log4j.spi.LoggingEvent;
+
+public class HttpLogJsonLayout extends JsonLayout {
+
+  @Override
+  public DateTimeFormatter createDateTimeFormatter() {
+    return DateTimeFormatter.ofPattern("dd/MMM/yyyy:HH:mm:ss,SSS Z");
+  }
+
+  @Override
+  public JsonLogEntry toJsonLogEntry(LoggingEvent event) {
+    return new HttpJsonLogEntry(event);
+  }
+
+  @SuppressWarnings("unused")
+  private class HttpJsonLogEntry extends JsonLogEntry {
+    public String host;
+    public String thread;
+    public String user;
+    public String timestamp;
+    public String method;
+    public String resource;
+    public String protocol;
+    public String status;
+    public String contentLength;
+    public String referer;
+    public String userAgent;
+
+    public HttpJsonLogEntry(LoggingEvent event) {
+      this.host = getMdcString(event, P_HOST);
+      this.thread = event.getThreadName();
+      this.user = getMdcString(event, P_USER);
+      this.timestamp = formatDate(event.getTimeStamp());
+      this.method = getMdcString(event, P_METHOD);
+      this.resource = getMdcString(event, P_RESOURCE);
+      this.protocol = getMdcString(event, P_PROTOCOL);
+      this.status = getMdcString(event, P_STATUS);
+      this.contentLength = getMdcString(event, P_CONTENT_LENGTH);
+      this.referer = getMdcString(event, P_REFERER);
+      this.userAgent = getMdcString(event, P_USER_AGENT);
+    }
+  }
+}

java/com/google/gerrit/server/args4j/ChangeIdHandler.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.args4j;
 import static com.google.gerrit.util.cli.Localizable.localizable;
 
 import com.google.common.base.Splitter;
+import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
@@ -35,34 +36,42 @@ import org.kohsuke.args4j.spi.Parameters;
 import org.kohsuke.args4j.spi.Setter;
 
 public class ChangeIdHandler extends OptionHandler<Change.Id> {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   private final Provider<InternalChangeQuery> queryProvider;
 
   @Inject
   public ChangeIdHandler(
       // TODO(dborowitz): Not sure whether this is injectable here.
       Provider<InternalChangeQuery> queryProvider,
-      @Assisted final CmdLineParser parser,
-      @Assisted final OptionDef option,
-      @Assisted final Setter<Change.Id> setter) {
+      @Assisted CmdLineParser parser,
+      @Assisted OptionDef option,
+      @Assisted Setter<Change.Id> setter) {
     super(parser, option, setter);
     this.queryProvider = queryProvider;
   }
 
   @Override
   public final int parseArguments(Parameters params) throws CmdLineException {
-    final String token = params.getParameter(0);
-    final List<String> tokens = Splitter.on(',').splitToList(token);
+    String token = params.getParameter(0);
+    List<String> tokens = Splitter.on(',').splitToList(token);
     if (tokens.size() != 3) {
       throw new CmdLineException(
           owner, localizable("change should be specified as <project>,<branch>,<change-id>"));
     }
 
     try {
-      final Change.Key key = Change.Key.parse(tokens.get(2));
-      final Project.NameKey project = new Project.NameKey(tokens.get(0));
-      final Branch.NameKey branch = new Branch.NameKey(project, tokens.get(1));
-      for (ChangeData cd : queryProvider.get().byBranchKey(branch, key)) {
-        setter.addValue(cd.getId());
+      Change.Key key = Change.Key.parse(tokens.get(2));
+      Project.NameKey project = new Project.NameKey(tokens.get(0));
+      Branch.NameKey branch = new Branch.NameKey(project, tokens.get(1));
+      List<ChangeData> changes = queryProvider.get().byBranchKey(branch, key);
+      if (!changes.isEmpty()) {
+        if (changes.size() > 1) {
+          String msg = "\"%s\": resolves to multiple changes";
+          logger.atSevere().log(msg, token);
+          throw new CmdLineException(owner, localizable(msg), token);
+        }
+        setter.addValue(changes.get(0).getId());
         return 1;
       }
     } catch (IllegalArgumentException e) {

java/com/google/gerrit/server/logging/BUILD

@@ -9,9 +9,11 @@ java_library(
     deps = [
         "//java/com/google/gerrit/common:annotations",
         "//java/com/google/gerrit/server/util/time",
+        "//lib:gson",
         "//lib:guava",
         "//lib/auto:auto-value",
         "//lib/auto:auto-value-annotations",
         "//lib/flogger:api",
+        "//lib/log:log4j",
     ],
 )

java/com/google/gerrit/server/logging/JsonLayout.java

@@ -0,0 +1,72 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.logging;
+
+import com.google.gson.FieldNamingPolicy;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+import java.time.Instant;
+import java.time.LocalDateTime;
+import java.time.OffsetDateTime;
+import java.time.ZoneId;
+import java.time.ZoneOffset;
+import java.time.ZonedDateTime;
+import java.time.format.DateTimeFormatter;
+import org.apache.log4j.Layout;
+import org.apache.log4j.spi.LoggingEvent;
+
+public abstract class JsonLayout extends Layout {
+  private final DateTimeFormatter dateFormatter;
+  private final Gson gson;
+  private final ZoneOffset timeOffset;
+
+  public JsonLayout() {
+    dateFormatter = createDateTimeFormatter();
+    timeOffset = OffsetDateTime.now().getOffset();
+
+    gson = newGson();
+  }
+
+  public abstract DateTimeFormatter createDateTimeFormatter();
+
+  public abstract JsonLogEntry toJsonLogEntry(LoggingEvent event);
+
+  @Override
+  public String format(LoggingEvent event) {
+    return gson.toJson(toJsonLogEntry(event)) + "\n";
+  }
+
+  private static Gson newGson() {
+    GsonBuilder gb =
+        new GsonBuilder()
+            .setFieldNamingPolicy(FieldNamingPolicy.LOWER_CASE_WITH_UNDERSCORES)
+            .disableHtmlEscaping();
+    return gb.create();
+  }
+
+  public String formatDate(long now) {
+    return ZonedDateTime.of(
+            LocalDateTime.ofInstant(Instant.ofEpochMilli(now), timeOffset), ZoneId.systemDefault())
+        .format(dateFormatter);
+  }
+
+  @Override
+  public void activateOptions() {}
+
+  @Override
+  public boolean ignoresThrowable() {
+    return false;
+  }
+}

java/com/google/gerrit/server/logging/JsonLogEntry.java

@@ -0,0 +1,23 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.logging;
+
+import org.apache.log4j.spi.LoggingEvent;
+
+public abstract class JsonLogEntry {
+  public String getMdcString(LoggingEvent event, String key) {
+    return (String) event.getMDC(key);
+  }
+}

java/com/google/gerrit/server/mail/send/ProjectWatch.java

@@ -226,7 +226,7 @@ public class ProjectWatch {
       qb = args.queryBuilder.asUser(args.anonymousUser);
     } else {
       qb = args.queryBuilder.asUser(user);
-      p = qb.is_visible();
+      p = qb.isVisible();
     }
 
     if (filter != null) {

java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java

@@ -488,7 +488,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
     return ChangeStatusPredicate.parse(statusName);
   }
 
-  public Predicate<ChangeData> status_open() {
+  public Predicate<ChangeData> statusOpen() {
     return ChangeStatusPredicate.open();
   }
 
@@ -537,7 +537,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
     }
 
     if ("visible".equalsIgnoreCase(value)) {
-      return is_visible();
+      return isVisible();
     }
 
     if ("reviewed".equalsIgnoreCase(value)) {
@@ -955,7 +955,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
   public Predicate<ChangeData> visibleto(String who)
       throws QueryParseException, IOException, ConfigInvalidException {
     if (isSelf(who)) {
-      return is_visible();
+      return isVisible();
     }
     try {
       return Predicate.or(
@@ -990,7 +990,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
         args.anonymousUserProvider);
   }
 
-  public Predicate<ChangeData> is_visible() throws QueryParseException {
+  public Predicate<ChangeData> isVisible() throws QueryParseException {
     return visibleto(args.getUser());
   }
 

java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java

@@ -77,13 +77,13 @@ public class IsWatchedByPredicate extends AndPredicate<ChangeData> {
       } else if (f != null) {
         r.add(f);
       } else {
-        r.add(builder.status_open());
+        r.add(builder.statusOpen());
       }
     }
     if (r.isEmpty()) {
       return ImmutableList.of(ChangeIndexPredicate.none());
     } else if (checkIsVisible) {
-      return ImmutableList.of(or(r), builder.is_visible());
+      return ImmutableList.of(or(r), builder.isVisible());
     } else {
       return ImmutableList.of(or(r));
     }

java/com/google/gerrit/sshd/SshLog.java

@@ -45,15 +45,18 @@ import org.eclipse.jgit.lib.Config;
 @Singleton
 class SshLog implements LifecycleListener, GerritConfigListener {
   private static final Logger log = Logger.getLogger(SshLog.class);
-  private static final String LOG_NAME = "sshd_log";
-  private static final String P_SESSION = "session";
-  private static final String P_USER_NAME = "userName";
-  private static final String P_ACCOUNT_ID = "accountId";
-  private static final String P_WAIT = "queueWaitTime";
-  private static final String P_EXEC = "executionTime";
-  private static final String P_STATUS = "status";
-  private static final String P_AGENT = "agent";
-  private static final String P_MESSAGE = "message";
+
+  private static final String JSON_SUFFIX = ".json";
+
+  protected static final String LOG_NAME = "sshd_log";
+  protected static final String P_SESSION = "session";
+  protected static final String P_USER_NAME = "userName";
+  protected static final String P_ACCOUNT_ID = "accountId";
+  protected static final String P_WAIT = "queueWaitTime";
+  protected static final String P_EXEC = "executionTime";
+  protected static final String P_STATUS = "status";
+  protected static final String P_AGENT = "agent";
+  protected static final String P_MESSAGE = "message";
 
   private final Provider<SshSession> session;
   private final Provider<Context> context;
@@ -61,6 +64,9 @@ class SshLog implements LifecycleListener, GerritConfigListener {
   private final GroupAuditService auditService;
   private final SystemLog systemLog;
 
+  private final boolean json;
+  private final boolean text;
+
   private final Object lock = new Object();
 
   @Inject
@@ -75,6 +81,9 @@ class SshLog implements LifecycleListener, GerritConfigListener {
     this.auditService = auditService;
     this.systemLog = systemLog;
 
+    this.json = config.getBoolean("log", "jsonLogging", false);
+    this.text = config.getBoolean("log", "textLogging", true) || !json;
+
     if (config.getBoolean("sshd", "requestLog", true)) {
       enableLogging();
     }
@@ -84,7 +93,16 @@ class SshLog implements LifecycleListener, GerritConfigListener {
   public boolean enableLogging() {
     synchronized (lock) {
       if (async == null) {
-        async = systemLog.createAsyncAppender(LOG_NAME, new SshLogLayout());
+        async = new AsyncAppender();
+
+        if (text) {
+          async.addAppender(systemLog.createAsyncAppender(LOG_NAME, new SshLogLayout()));
+        }
+
+        if (json) {
+          async.addAppender(
+              systemLog.createAsyncAppender(LOG_NAME + JSON_SUFFIX, new SshLogJsonLayout()));
+        }
         return true;
       }
       return false;

java/com/google/gerrit/sshd/SshLogJsonLayout.java

@@ -0,0 +1,96 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.sshd;
+
+import static com.google.gerrit.sshd.SshLog.P_ACCOUNT_ID;
+import static com.google.gerrit.sshd.SshLog.P_AGENT;
+import static com.google.gerrit.sshd.SshLog.P_EXEC;
+import static com.google.gerrit.sshd.SshLog.P_MESSAGE;
+import static com.google.gerrit.sshd.SshLog.P_SESSION;
+import static com.google.gerrit.sshd.SshLog.P_STATUS;
+import static com.google.gerrit.sshd.SshLog.P_USER_NAME;
+import static com.google.gerrit.sshd.SshLog.P_WAIT;
+
+import com.google.gerrit.server.logging.JsonLayout;
+import com.google.gerrit.server.logging.JsonLogEntry;
+import java.time.format.DateTimeFormatter;
+import org.apache.log4j.spi.LoggingEvent;
+
+public class SshLogJsonLayout extends JsonLayout {
+
+  @Override
+  public DateTimeFormatter createDateTimeFormatter() {
+    return DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss,SSS Z");
+  }
+
+  @Override
+  public JsonLogEntry toJsonLogEntry(LoggingEvent event) {
+    return new SshJsonLogEntry(event);
+  }
+
+  @SuppressWarnings("unused")
+  private class SshJsonLogEntry extends JsonLogEntry {
+    public String timestamp;
+    public String session;
+    public String thread;
+    public String user;
+    public String accountId;
+    public String message;
+    public String waitTime;
+    public String execTime;
+    public String status;
+    public String agent;
+    public String timeNegotiating;
+    public String timeSearchReuse;
+    public String timeSearchSizes;
+    public String timeCounting;
+    public String timeCompressing;
+    public String timeWriting;
+    public String timeTotal;
+    public String bitmapIndexMisses;
+    public String deltasTotal;
+    public String objectsTotal;
+    public String bytesTotal;
+
+    public SshJsonLogEntry(LoggingEvent event) {
+      this.timestamp = formatDate(event.getTimeStamp());
+      this.session = getMdcString(event, P_SESSION);
+      this.thread = event.getThreadName();
+      this.user = getMdcString(event, P_USER_NAME);
+      this.accountId = getMdcString(event, P_ACCOUNT_ID);
+      this.message = (String) event.getMessage();
+      this.waitTime = getMdcString(event, P_WAIT);
+      this.execTime = getMdcString(event, P_EXEC);
+      this.status = getMdcString(event, P_STATUS);
+      this.agent = getMdcString(event, P_AGENT);
+
+      String metricString = getMdcString(event, P_MESSAGE);
+      if (metricString != null && !metricString.isEmpty()) {
+        String[] ssh_metrics = metricString.split(" ");
+        this.timeNegotiating = ssh_metrics[0];
+        this.timeSearchReuse = ssh_metrics[1];
+        this.timeSearchSizes = ssh_metrics[2];
+        this.timeCounting = ssh_metrics[3];
+        this.timeCompressing = ssh_metrics[4];
+        this.timeWriting = ssh_metrics[5];
+        this.timeTotal = ssh_metrics[6];
+        this.bitmapIndexMisses = ssh_metrics[7];
+        this.deltasTotal = ssh_metrics[8];
+        this.objectsTotal = ssh_metrics[9];
+        this.bytesTotal = ssh_metrics[10];
+      }
+    }
+  }
+}

java/com/google/gerrit/sshd/SshLogLayout.java

@@ -14,6 +14,15 @@
 
 package com.google.gerrit.sshd;
 
+import static com.google.gerrit.sshd.SshLog.P_ACCOUNT_ID;
+import static com.google.gerrit.sshd.SshLog.P_AGENT;
+import static com.google.gerrit.sshd.SshLog.P_EXEC;
+import static com.google.gerrit.sshd.SshLog.P_MESSAGE;
+import static com.google.gerrit.sshd.SshLog.P_SESSION;
+import static com.google.gerrit.sshd.SshLog.P_STATUS;
+import static com.google.gerrit.sshd.SshLog.P_USER_NAME;
+import static com.google.gerrit.sshd.SshLog.P_WAIT;
+
 import java.text.SimpleDateFormat;
 import java.util.Calendar;
 import java.util.TimeZone;
@@ -23,15 +32,6 @@ import org.eclipse.jgit.util.QuotedString;
 
 public final class SshLogLayout extends Layout {
 
-  private static final String P_SESSION = "session";
-  private static final String P_USER_NAME = "userName";
-  private static final String P_ACCOUNT_ID = "accountId";
-  private static final String P_WAIT = "queueWaitTime";
-  private static final String P_EXEC = "executionTime";
-  private static final String P_STATUS = "status";
-  private static final String P_AGENT = "agent";
-  private static final String P_MESSAGE = "message";
-
   private final Calendar calendar;
   private long lastTimeMillis;
   private final char[] lastTimeString = new char[20];

javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java

@@ -427,7 +427,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
     gApi.changes().id(change1.getChangeId()).setPrivate(true, null);
 
-    // Change1 is not private, but should be still visible to its owner.
+    // Change1 is private, but should be still visible to its owner.
     assertQuery("is:open", change1, change2);
     assertQuery("is:private", change1);
 
@@ -961,11 +961,11 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
   public void byLabel() throws Exception {
     accountManager.authenticate(AuthRequest.forUser("anotheruser"));
     TestRepository<Repo> repo = createProject("repo");
-    ChangeInserter ins = newChange(repo, null, null, null, null, false);
-    ChangeInserter ins2 = newChange(repo, null, null, null, null, false);
-    ChangeInserter ins3 = newChange(repo, null, null, null, null, false);
-    ChangeInserter ins4 = newChange(repo, null, null, null, null, false);
-    ChangeInserter ins5 = newChange(repo, null, null, null, null, false);
+    ChangeInserter ins = newChange(repo);
+    ChangeInserter ins2 = newChange(repo);
+    ChangeInserter ins3 = newChange(repo);
+    ChangeInserter ins4 = newChange(repo);
+    ChangeInserter ins5 = newChange(repo);
 
     Change reviewMinus2Change = insert(repo, ins);
     gApi.changes().id(reviewMinus2Change.getId().get()).current().review(ReviewInput.reject());
@@ -1062,11 +1062,11 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     projectCache.evict(cfg.getProject());
 
     ReviewInput reviewVerified = new ReviewInput().label("Verified", 1);
-    ChangeInserter ins = newChange(repo, null, null, null, null, false);
-    ChangeInserter ins2 = newChange(repo, null, null, null, null, false);
-    ChangeInserter ins3 = newChange(repo, null, null, null, null, false);
-    ChangeInserter ins4 = newChange(repo, null, null, null, null, false);
-    ChangeInserter ins5 = newChange(repo, null, null, null, null, false);
+    ChangeInserter ins = newChange(repo);
+    ChangeInserter ins2 = newChange(repo);
+    ChangeInserter ins3 = newChange(repo);
+    ChangeInserter ins4 = newChange(repo);
+    ChangeInserter ins5 = newChange(repo);
 
     // CR+1
     Change reviewCRplus1 = insert(repo, ins);
@@ -1107,7 +1107,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
   @Test
   public void byLabelNotOwner() throws Exception {
     TestRepository<Repo> repo = createProject("repo");
-    ChangeInserter ins = newChange(repo, null, null, null, null, false);
+    ChangeInserter ins = newChange(repo);
     Account.Id user1 = createAccount("user1");
 
     Change reviewPlus1Change = insert(repo, ins);
@@ -1841,25 +1841,27 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
   public void visible() throws Exception {
     TestRepository<Repo> repo = createProject("repo");
     Change change1 = insert(repo, newChange(repo));
-    Change change2 = insert(repo, newChange(repo));
-
-    gApi.changes().id(change2.getChangeId()).setPrivate(true, "private");
+    Change change2 = insert(repo, newChangePrivate(repo));
 
     String q = "project:repo";
-    assertQuery(q, change2, change1);
 
-    // Second user cannot see first user's private change.
-    Account.Id user2 =
-        accountManager.authenticate(AuthRequest.forUser("anotheruser")).getAccountId();
+    // Bad request for query with non-existent user
+    assertThatQueryException(q + " visibleto:notexisting");
+
+    // Current user can see all changes
+    assertQuery(q, change2, change1);
+    assertQuery(q + " visibleto:self", change2, change1);
+
+    // Second user cannot see first user's private change
+    Account.Id user2 = createAccount("anotheruser");
     assertQuery(q + " visibleto:" + user2.get(), change1);
+    assertQuery(q + " visibleto:anotheruser", change1);
 
     String g1 = createGroup("group1", "Administrators");
     gApi.groups().id(g1).addMembers("anotheruser");
     assertQuery(q + " visibleto:" + g1, change1);
 
-    requestContext.setContext(
-        newRequestContext(
-            accountManager.authenticate(AuthRequest.forUser("anotheruser")).getAccountId()));
+    requestContext.setContext(newRequestContext(user2));
     assertQuery("is:visible", change1);
   }
 
@@ -3161,12 +3163,12 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
   }
 
   protected ChangeInserter newChange(TestRepository<Repo> repo) throws Exception {
-    return newChange(repo, null, null, null, null, false);
+    return newChange(repo, null, null, null, null, false, false);
   }
 
   protected ChangeInserter newChangeForCommit(TestRepository<Repo> repo, RevCommit commit)
       throws Exception {
-    return newChange(repo, commit, null, null, null, false);
+    return newChange(repo, commit, null, null, null, false, false);
   }
 
   protected ChangeInserter newChangeWithFiles(TestRepository<Repo> repo, String... paths)
@@ -3180,21 +3182,25 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
   protected ChangeInserter newChangeForBranch(TestRepository<Repo> repo, String branch)
       throws Exception {
-    return newChange(repo, null, branch, null, null, false);
+    return newChange(repo, null, branch, null, null, false, false);
   }
 
   protected ChangeInserter newChangeWithStatus(TestRepository<Repo> repo, Change.Status status)
       throws Exception {
-    return newChange(repo, null, null, status, null, false);
+    return newChange(repo, null, null, status, null, false, false);
   }
 
   protected ChangeInserter newChangeWithTopic(TestRepository<Repo> repo, String topic)
       throws Exception {
-    return newChange(repo, null, null, null, topic, false);
+    return newChange(repo, null, null, null, topic, false, false);
   }
 
   protected ChangeInserter newChangeWorkInProgress(TestRepository<Repo> repo) throws Exception {
-    return newChange(repo, null, null, null, null, true);
+    return newChange(repo, null, null, null, null, true, false);
+  }
+
+  protected ChangeInserter newChangePrivate(TestRepository<Repo> repo) throws Exception {
+    return newChange(repo, null, null, null, null, false, true);
   }
 
   protected ChangeInserter newChange(
@@ -3203,7 +3209,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
       @Nullable String branch,
       @Nullable Change.Status status,
       @Nullable String topic,
-      boolean workInProgress)
+      boolean workInProgress,
+      boolean isPrivate)
       throws Exception {
     if (commit == null) {
       commit = repo.parseBody(repo.commit().message("message").create());
@@ -3221,7 +3228,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
             .setValidate(false)
             .setStatus(status)
             .setTopic(topic)
-            .setWorkInProgress(workInProgress);
+            .setWorkInProgress(workInProgress)
+            .setPrivate(isPrivate);
     return ins;
   }