Merge changes I2950ea74,Icaa5ff61 * changes: Store for revert commits that they do not contain conflicts CreateChange: Store for empty revisions that they do not contain conflicts
diff --git a/java/com/google/gerrit/entities/converter/ConflictsProtoConverter.java b/java/com/google/gerrit/entities/converter/ConflictsProtoConverter.java index d57a1dd..c7dd9e2 100644 --- a/java/com/google/gerrit/entities/converter/ConflictsProtoConverter.java +++ b/java/com/google/gerrit/entities/converter/ConflictsProtoConverter.java
@@ -46,7 +46,7 @@ proto.hasTheirs() ? Optional.of(objectIdConverter.fromProto(proto.getTheirs())) : Optional.empty(), - proto.getContainsConflicts()); + proto.hasContainsConflicts() ? proto.getContainsConflicts() : false); } @Override
diff --git a/java/com/google/gerrit/httpd/EnableTracingFilter.java b/java/com/google/gerrit/httpd/EnableTracingFilter.java deleted file mode 100644 index 7f90805..0000000 --- a/java/com/google/gerrit/httpd/EnableTracingFilter.java +++ /dev/null
@@ -1,93 +0,0 @@ -// Copyright (C) 2024 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.httpd; - -import static com.google.gerrit.httpd.GerritHeaders.X_GERRIT_TRACE; - -import com.google.common.base.Strings; -import com.google.gerrit.httpd.restapi.ParameterParser; -import com.google.gerrit.server.logging.RequestId; -import com.google.gerrit.server.logging.TraceContext; -import com.google.inject.Singleton; -import java.io.IOException; -import javax.servlet.Filter; -import javax.servlet.FilterChain; -import javax.servlet.FilterConfig; -import javax.servlet.ServletException; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; - -@Singleton -public class EnableTracingFilter implements Filter { - - public static final String REQUEST_TRACE_CONTEXT = "REQUEST_TRACE_CONTEXT"; - - @Override - public void init(FilterConfig filterConfig) throws ServletException {} - - @Override - public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) - throws IOException, ServletException { - - HttpServletRequest req = (HttpServletRequest) request; - HttpServletResponse res = (HttpServletResponse) response; - try (TraceContext traceContext = enableTracing(req, res)) { - request.setAttribute(REQUEST_TRACE_CONTEXT, traceContext); - chain.doFilter(request, response); - } - } - - private TraceContext enableTracing(HttpServletRequest req, HttpServletResponse res) { - // There are 2 ways to enable tracing for REST calls: - // 1. by using the 'trace' or 'trace=<trace-id>' request parameter - // 2. by setting the 'X-Gerrit-Trace:' or 'X-Gerrit-Trace:<trace-id>' header - String traceValueFromHeader = req.getHeader(X_GERRIT_TRACE); - String traceValueFromRequestParam = req.getParameter(ParameterParser.TRACE_PARAMETER); - boolean forceLogging = traceValueFromHeader != null || traceValueFromRequestParam != null; - - // Check whether no trace ID, one trace ID or 2 different trace IDs have been specified. - String traceId1; - String traceId2; - if (!Strings.isNullOrEmpty(traceValueFromHeader)) { - traceId1 = traceValueFromHeader; - if (!Strings.isNullOrEmpty(traceValueFromRequestParam) - && !traceValueFromHeader.equals(traceValueFromRequestParam)) { - traceId2 = traceValueFromRequestParam; - } else { - traceId2 = null; - } - } else { - traceId1 = Strings.emptyToNull(traceValueFromRequestParam); - traceId2 = null; - } - - // Use the first trace ID to start tracing. If this trace ID is null, a trace ID will be - // generated. - TraceContext traceContext = - TraceContext.newTrace( - forceLogging, traceId1, (tagName, traceId) -> res.setHeader(X_GERRIT_TRACE, traceId)); - // If a second trace ID was specified, add a tag for it as well. - if (traceId2 != null) { - traceContext.addTag(RequestId.Type.TRACE_ID, traceId2); - res.addHeader(X_GERRIT_TRACE, traceId2); - } - return traceContext; - } - - @Override - public void destroy() {} -}
diff --git a/java/com/google/gerrit/httpd/GerritHeaders.java b/java/com/google/gerrit/httpd/GerritHeaders.java deleted file mode 100644 index e5be906..0000000 --- a/java/com/google/gerrit/httpd/GerritHeaders.java +++ /dev/null
@@ -1,19 +0,0 @@ -// Copyright (C) 2024 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.httpd; - -public class GerritHeaders { - public static final String X_GERRIT_TRACE = "X-Gerrit-Trace"; -}
diff --git a/java/com/google/gerrit/httpd/HttpRequestTraceModule.java b/java/com/google/gerrit/httpd/HttpRequestTraceModule.java deleted file mode 100644 index ea36fbc..0000000 --- a/java/com/google/gerrit/httpd/HttpRequestTraceModule.java +++ /dev/null
@@ -1,39 +0,0 @@ -// Copyright (C) 2024 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.httpd; - -import static com.google.gerrit.httpd.EnableTracingFilter.REQUEST_TRACE_CONTEXT; - -import com.google.gerrit.server.logging.TraceContext; -import com.google.inject.Provides; -import com.google.inject.name.Named; -import com.google.inject.servlet.RequestScoped; -import com.google.inject.servlet.ServletModule; -import javax.servlet.http.HttpServletRequest; - -public class HttpRequestTraceModule extends ServletModule { - - @Provides - @RequestScoped - @Named(REQUEST_TRACE_CONTEXT) - public TraceContext provideTraceContext(HttpServletRequest req) { - return (TraceContext) req.getAttribute(REQUEST_TRACE_CONTEXT); - } - - @Override - protected void configureServlets() { - filter("/*").through(EnableTracingFilter.class); - } -}
diff --git a/java/com/google/gerrit/httpd/WebModule.java b/java/com/google/gerrit/httpd/WebModule.java index a110a01..e694f77 100644 --- a/java/com/google/gerrit/httpd/WebModule.java +++ b/java/com/google/gerrit/httpd/WebModule.java
@@ -49,8 +49,6 @@ @Override protected void configure() { - install(new HttpRequestTraceModule()); - bind(RequestScopePropagator.class).to(GuiceRequestScopePropagator.class); bind(HttpRequestContext.class);
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java index 1052633..970be10 100644 --- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java +++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -19,7 +19,6 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.net.HttpHeaders.CONTENT_TYPE; -import static com.google.gerrit.httpd.EnableTracingFilter.REQUEST_TRACE_CONTEXT; import static java.math.RoundingMode.CEILING; import static java.nio.charset.StandardCharsets.ISO_8859_1; import static java.nio.charset.StandardCharsets.UTF_8; @@ -143,7 +142,6 @@ import com.google.inject.Injector; import com.google.inject.Provider; import com.google.inject.TypeLiteral; -import com.google.inject.name.Named; import com.google.inject.util.Providers; import java.io.BufferedReader; import java.io.BufferedWriter; @@ -247,7 +245,6 @@ final DeadlineChecker.Factory deadlineCheckerFactory; final CancellationMetrics cancellationMetrics; final AclInfoController aclInfoController; - final Provider<TraceContext> requestTraceContext; @Inject Globals( @@ -268,8 +265,7 @@ DynamicMap<DynamicOptions.DynamicBean> dynamicBeans, DeadlineChecker.Factory deadlineCheckerFactory, CancellationMetrics cancellationMetrics, - AclInfoController aclInfoController, - @Named(REQUEST_TRACE_CONTEXT) Provider<TraceContext> requestTraceContext) { + AclInfoController aclInfoController) { this.currentUser = currentUser; this.webSession = webSession; this.paramParser = paramParser; @@ -289,7 +285,6 @@ this.deadlineCheckerFactory = deadlineCheckerFactory; this.cancellationMetrics = cancellationMetrics; this.aclInfoController = aclInfoController; - this.requestTraceContext = requestTraceContext; } } @@ -334,444 +329,456 @@ RestResource rsrc = TopLevelResource.INSTANCE; ViewData viewData = null; - String requestUri = requestUri(req); + try (TraceContext traceContext = enableTracing(req, res)) { + String requestUri = requestUri(req); - try (PerThreadCache ignored = PerThreadCache.create()) { - List<IdString> path = splitPath(req); - TraceContext traceContext = globals.requestTraceContext.get(); - RequestInfo requestInfo = createRequestInfo(traceContext, req, requestUri, path); - globals.requestListeners.runEach(l -> l.onRequest(requestInfo)); + try (PerThreadCache ignored = PerThreadCache.create()) { + List<IdString> path = splitPath(req); + RequestInfo requestInfo = createRequestInfo(traceContext, req, requestUri, path); + globals.requestListeners.runEach(l -> l.onRequest(requestInfo)); - globals.aclInfoController.enableAclLoggingIfUserCanViewAccess(traceContext); + globals.aclInfoController.enableAclLoggingIfUserCanViewAccess(traceContext); - // It's important that the PerformanceLogContext is closed before the response is sent to - // the client. Only this way it is ensured that the invocation of the PerformanceLogger - // plugins happens before the client sees the response. This is needed for being able to - // test performance logging from an acceptance test (see - // TraceIT#performanceLoggingForRestCall()). - try (RequestStateContext requestStateContext = - RequestStateContext.open() - .addRequestStateProvider( - globals.deadlineCheckerFactory.create( - requestInfo, req.getHeader(X_GERRIT_DEADLINE))); - PerformanceLogContext performanceLogContext = - new PerformanceLogContext(globals.config, globals.performanceLoggers)) { - traceRequestData(req); + // It's important that the PerformanceLogContext is closed before the response is sent to + // the client. Only this way it is ensured that the invocation of the PerformanceLogger + // plugins happens before the client sees the response. This is needed for being able to + // test performance logging from an acceptance test (see + // TraceIT#performanceLoggingForRestCall()). + try (RequestStateContext requestStateContext = + RequestStateContext.open() + .addRequestStateProvider( + globals.deadlineCheckerFactory.create( + requestInfo, req.getHeader(X_GERRIT_DEADLINE))); + PerformanceLogContext performanceLogContext = + new PerformanceLogContext(globals.config, globals.performanceLoggers)) { + traceRequestData(req); - if (corsResponder.filterCorsPreflight(req, res)) { - return; - } - - qp = ParameterParser.getQueryParams(req); - corsResponder.checkCors(req, res, qp.hasXdOverride()); - if (qp.hasXdOverride()) { - req = applyXdOverrides(req, qp); - } - checkUserSession(req); - - RestCollection<RestResource, RestResource> rc = members.get(); - globals - .permissionBackend - .currentUser() - .checkAny(GlobalPermission.fromAnnotation(rc.getClass())); - - viewData = new ViewData(null, null); - - if (path.isEmpty()) { - globals.quotaChecker.enforce(req); - if (rc instanceof NeedsParams) { - ((NeedsParams) rc).setParams(qp.params()); + if (corsResponder.filterCorsPreflight(req, res)) { + return; } - if (isRead(req)) { - viewData = new ViewData(null, rc.list()); - } else if (isPost(req)) { - RestView<RestResource> restCollectionView = - rc.views().get(PluginName.GERRIT, "POST_ON_COLLECTION./"); - if (restCollectionView != null) { - viewData = new ViewData(null, restCollectionView); - } else { - throw methodNotAllowed(req); - } - } else { - // DELETE on root collections is not supported - throw methodNotAllowed(req); + qp = ParameterParser.getQueryParams(req); + corsResponder.checkCors(req, res, qp.hasXdOverride()); + if (qp.hasXdOverride()) { + req = applyXdOverrides(req, qp); } - } else { - IdString id = path.remove(0); - try { - rsrc = parseResourceWithRetry(req, traceContext, viewData.pluginName, rc, rsrc, id); - globals.quotaChecker.enforce(rsrc, req); - if (path.isEmpty()) { - checkPreconditions(req); - } - } catch (ResourceNotFoundException e) { - if (!path.isEmpty()) { - throw e; - } - globals.quotaChecker.enforce(req); + checkUserSession(req); - if (isPost(req) || isPut(req)) { - RestView<RestResource> createView = rc.views().get(PluginName.GERRIT, "CREATE./"); - if (createView != null) { - viewData = new ViewData(null, createView); - path.add(id); - } else { - throw e; - } - } else if (isDelete(req)) { - RestView<RestResource> deleteView = - rc.views().get(PluginName.GERRIT, "DELETE_MISSING./"); - if (deleteView != null) { - viewData = new ViewData(null, deleteView); - path.add(id); - } else { - throw e; - } - } else { - throw e; - } - } - if (viewData.view == null) { - viewData = view(rc, req.getMethod(), path); - } - } - checkRequiresCapability(viewData); + RestCollection<RestResource, RestResource> rc = members.get(); + globals + .permissionBackend + .currentUser() + .checkAny(GlobalPermission.fromAnnotation(rc.getClass())); - while (viewData.view instanceof RestCollection<?, ?>) { - @SuppressWarnings("unchecked") - RestCollection<RestResource, RestResource> c = - (RestCollection<RestResource, RestResource>) viewData.view; + viewData = new ViewData(null, null); if (path.isEmpty()) { + globals.quotaChecker.enforce(req); + if (rc instanceof NeedsParams) { + ((NeedsParams) rc).setParams(qp.params()); + } + if (isRead(req)) { - viewData = new ViewData(null, c.list()); + viewData = new ViewData(null, rc.list()); } else if (isPost(req)) { - // TODO: Here and on other collection methods: There is a bug that binds child views - // with pluginName="gerrit" instead of the real plugin name. This has never worked - // correctly and should be fixed where the binding gets created (DynamicMapProvider) - // and here. RestView<RestResource> restCollectionView = - c.views().get(PluginName.GERRIT, "POST_ON_COLLECTION./"); - if (restCollectionView != null) { - viewData = new ViewData(null, restCollectionView); - } else { - throw methodNotAllowed(req); - } - } else if (isDelete(req)) { - RestView<RestResource> restCollectionView = - c.views().get(PluginName.GERRIT, "DELETE_ON_COLLECTION./"); + rc.views().get(PluginName.GERRIT, "POST_ON_COLLECTION./"); if (restCollectionView != null) { viewData = new ViewData(null, restCollectionView); } else { throw methodNotAllowed(req); } } else { + // DELETE on root collections is not supported throw methodNotAllowed(req); } - break; - } - IdString id = path.remove(0); - try { - rsrc = parseResourceWithRetry(req, traceContext, viewData.pluginName, c, rsrc, id); - checkPreconditions(req); - viewData = new ViewData(null, null); - } catch (ResourceNotFoundException e) { - if (!path.isEmpty()) { - throw e; - } + } else { + IdString id = path.remove(0); + try { + rsrc = parseResourceWithRetry(req, traceContext, viewData.pluginName, rc, rsrc, id); + globals.quotaChecker.enforce(rsrc, req); + if (path.isEmpty()) { + checkPreconditions(req); + } + } catch (ResourceNotFoundException e) { + if (!path.isEmpty()) { + throw e; + } + globals.quotaChecker.enforce(req); - if (isPost(req) || isPut(req)) { - RestView<RestResource> createView = c.views().get(PluginName.GERRIT, "CREATE./"); - if (createView != null) { - viewData = new ViewData(viewData.pluginName, createView); - path.add(id); + if (isPost(req) || isPut(req)) { + RestView<RestResource> createView = rc.views().get(PluginName.GERRIT, "CREATE./"); + if (createView != null) { + viewData = new ViewData(null, createView); + path.add(id); + } else { + throw e; + } + } else if (isDelete(req)) { + RestView<RestResource> deleteView = + rc.views().get(PluginName.GERRIT, "DELETE_MISSING./"); + if (deleteView != null) { + viewData = new ViewData(null, deleteView); + path.add(id); + } else { + throw e; + } } else { throw e; } - } else if (isDelete(req)) { - RestView<RestResource> deleteView = - c.views().get(PluginName.GERRIT, "DELETE_MISSING./"); - if (deleteView != null) { - viewData = new ViewData(viewData.pluginName, deleteView); - path.add(id); - } else { - throw e; - } - } else { - throw e; } - } - if (viewData.view == null) { - viewData = view(c, req.getMethod(), path); + if (viewData.view == null) { + viewData = view(rc, req.getMethod(), path); + } } checkRequiresCapability(viewData); - } - if (notModified(req, rsrc)) { - logger.atFinest().log("REST call succeeded: %d", SC_NOT_MODIFIED); - res.sendError(SC_NOT_MODIFIED); - return; - } + while (viewData.view instanceof RestCollection<?, ?>) { + @SuppressWarnings("unchecked") + RestCollection<RestResource, RestResource> c = + (RestCollection<RestResource, RestResource>) viewData.view; - try (DynamicOptions pluginOptions = - new DynamicOptions(globals.injector, globals.dynamicBeans)) { - if (!globals - .paramParser - .get() - .parse(viewData.view, pluginOptions, qp.params(), req, res)) { + if (path.isEmpty()) { + if (isRead(req)) { + viewData = new ViewData(null, c.list()); + } else if (isPost(req)) { + // TODO: Here and on other collection methods: There is a bug that binds child views + // with pluginName="gerrit" instead of the real plugin name. This has never worked + // correctly and should be fixed where the binding gets created (DynamicMapProvider) + // and here. + RestView<RestResource> restCollectionView = + c.views().get(PluginName.GERRIT, "POST_ON_COLLECTION./"); + if (restCollectionView != null) { + viewData = new ViewData(null, restCollectionView); + } else { + throw methodNotAllowed(req); + } + } else if (isDelete(req)) { + RestView<RestResource> restCollectionView = + c.views().get(PluginName.GERRIT, "DELETE_ON_COLLECTION./"); + if (restCollectionView != null) { + viewData = new ViewData(null, restCollectionView); + } else { + throw methodNotAllowed(req); + } + } else { + throw methodNotAllowed(req); + } + break; + } + IdString id = path.remove(0); + try { + rsrc = parseResourceWithRetry(req, traceContext, viewData.pluginName, c, rsrc, id); + checkPreconditions(req); + viewData = new ViewData(null, null); + } catch (ResourceNotFoundException e) { + if (!path.isEmpty()) { + throw e; + } + + if (isPost(req) || isPut(req)) { + RestView<RestResource> createView = c.views().get(PluginName.GERRIT, "CREATE./"); + if (createView != null) { + viewData = new ViewData(viewData.pluginName, createView); + path.add(id); + } else { + throw e; + } + } else if (isDelete(req)) { + RestView<RestResource> deleteView = + c.views().get(PluginName.GERRIT, "DELETE_MISSING./"); + if (deleteView != null) { + viewData = new ViewData(viewData.pluginName, deleteView); + path.add(id); + } else { + throw e; + } + } else { + throw e; + } + } + if (viewData.view == null) { + viewData = view(c, req.getMethod(), path); + } + checkRequiresCapability(viewData); + } + + if (notModified(req, rsrc)) { + logger.atFinest().log("REST call succeeded: %d", SC_NOT_MODIFIED); + res.sendError(SC_NOT_MODIFIED); return; } - if (viewData.view instanceof RestReadView<?> && isRead(req)) { - response = - invokeRestReadViewWithRetry( - req, traceContext, viewData, (RestReadView<RestResource>) viewData.view, rsrc); - } else if (viewData.view instanceof RestModifyView<?, ?>) { - RestModifyView<RestResource, Object> m = - (RestModifyView<RestResource, Object>) viewData.view; - - Type type = inputType(m); - inputRequestBody = parseRequest(req, type); - response = - invokeRestModifyViewWithRetry( - req, traceContext, viewData, m, rsrc, inputRequestBody); - - if (inputRequestBody instanceof RawInput) { - try (InputStream is = req.getInputStream()) { - ServletUtils.consumeRequestBody(is); - } + try (DynamicOptions pluginOptions = + new DynamicOptions(globals.injector, globals.dynamicBeans)) { + if (!globals + .paramParser + .get() + .parse(viewData.view, pluginOptions, qp.params(), req, res)) { + return; } - } else if (viewData.view instanceof RestCollectionCreateView<?, ?, ?>) { - RestCollectionCreateView<RestResource, RestResource, Object> m = - (RestCollectionCreateView<RestResource, RestResource, Object>) viewData.view; - Type type = inputType(m); - inputRequestBody = parseRequest(req, type); - response = - invokeRestCollectionCreateViewWithRetry( - req, traceContext, viewData, m, rsrc, path.get(0), inputRequestBody); - if (inputRequestBody instanceof RawInput) { - try (InputStream is = req.getInputStream()) { - ServletUtils.consumeRequestBody(is); - } - } - } else if (viewData.view instanceof RestCollectionDeleteMissingView<?, ?, ?>) { - RestCollectionDeleteMissingView<RestResource, RestResource, Object> m = - (RestCollectionDeleteMissingView<RestResource, RestResource, Object>) viewData.view; + if (viewData.view instanceof RestReadView<?> && isRead(req)) { + response = + invokeRestReadViewWithRetry( + req, + traceContext, + viewData, + (RestReadView<RestResource>) viewData.view, + rsrc); + } else if (viewData.view instanceof RestModifyView<?, ?>) { + RestModifyView<RestResource, Object> m = + (RestModifyView<RestResource, Object>) viewData.view; - Type type = inputType(m); - inputRequestBody = parseRequest(req, type); - response = - invokeRestCollectionDeleteMissingViewWithRetry( - req, traceContext, viewData, m, rsrc, path.get(0), inputRequestBody); - if (inputRequestBody instanceof RawInput) { - try (InputStream is = req.getInputStream()) { - ServletUtils.consumeRequestBody(is); - } - } - } else if (viewData.view instanceof RestCollectionModifyView<?, ?, ?>) { - RestCollectionModifyView<RestResource, RestResource, Object> m = - (RestCollectionModifyView<RestResource, RestResource, Object>) viewData.view; + Type type = inputType(m); + inputRequestBody = parseRequest(req, type); + response = + invokeRestModifyViewWithRetry( + req, traceContext, viewData, m, rsrc, inputRequestBody); - Type type = inputType(m); - inputRequestBody = parseRequest(req, type); - response = - invokeRestCollectionModifyViewWithRetry( - req, traceContext, viewData, m, rsrc, inputRequestBody); - if (inputRequestBody instanceof RawInput) { - try (InputStream is = req.getInputStream()) { - ServletUtils.consumeRequestBody(is); + if (inputRequestBody instanceof RawInput) { + try (InputStream is = req.getInputStream()) { + ServletUtils.consumeRequestBody(is); + } } + } else if (viewData.view instanceof RestCollectionCreateView<?, ?, ?>) { + RestCollectionCreateView<RestResource, RestResource, Object> m = + (RestCollectionCreateView<RestResource, RestResource, Object>) viewData.view; + + Type type = inputType(m); + inputRequestBody = parseRequest(req, type); + response = + invokeRestCollectionCreateViewWithRetry( + req, traceContext, viewData, m, rsrc, path.get(0), inputRequestBody); + if (inputRequestBody instanceof RawInput) { + try (InputStream is = req.getInputStream()) { + ServletUtils.consumeRequestBody(is); + } + } + } else if (viewData.view instanceof RestCollectionDeleteMissingView<?, ?, ?>) { + RestCollectionDeleteMissingView<RestResource, RestResource, Object> m = + (RestCollectionDeleteMissingView<RestResource, RestResource, Object>) + viewData.view; + + Type type = inputType(m); + inputRequestBody = parseRequest(req, type); + response = + invokeRestCollectionDeleteMissingViewWithRetry( + req, traceContext, viewData, m, rsrc, path.get(0), inputRequestBody); + if (inputRequestBody instanceof RawInput) { + try (InputStream is = req.getInputStream()) { + ServletUtils.consumeRequestBody(is); + } + } + } else if (viewData.view instanceof RestCollectionModifyView<?, ?, ?>) { + RestCollectionModifyView<RestResource, RestResource, Object> m = + (RestCollectionModifyView<RestResource, RestResource, Object>) viewData.view; + + Type type = inputType(m); + inputRequestBody = parseRequest(req, type); + response = + invokeRestCollectionModifyViewWithRetry( + req, traceContext, viewData, m, rsrc, inputRequestBody); + if (inputRequestBody instanceof RawInput) { + try (InputStream is = req.getInputStream()) { + ServletUtils.consumeRequestBody(is); + } + } + } else { + throw new ResourceNotFoundException(); } - } else { - throw new ResourceNotFoundException(); - } - String isUpdatedRefEnabled = req.getHeader(X_GERRIT_UPDATED_REF_ENABLED); - if (!Strings.isNullOrEmpty(isUpdatedRefEnabled) && Boolean.valueOf(isUpdatedRefEnabled)) { - setXGerritUpdatedRefResponseHeaders(req, res); + String isUpdatedRefEnabled = req.getHeader(X_GERRIT_UPDATED_REF_ENABLED); + if (!Strings.isNullOrEmpty(isUpdatedRefEnabled) + && Boolean.valueOf(isUpdatedRefEnabled)) { + setXGerritUpdatedRefResponseHeaders(req, res); + } + + if (response instanceof Response.Redirect) { + CacheHeaders.setNotCacheable(res); + String location = ((Response.Redirect) response).location(); + res.sendRedirect(location); + logger.atFinest().log("REST call redirected to: %s", location); + return; + } else if (response instanceof Response.Accepted) { + CacheHeaders.setNotCacheable(res); + res.setStatus(response.statusCode()); + res.setHeader(HttpHeaders.LOCATION, ((Response.Accepted) response).location()); + logger.atFinest().log("REST call succeeded: %d", response.statusCode()); + return; + } + + statusCode = response.statusCode(); + response.headers().forEach((k, v) -> res.setHeader(k, v)); + configureCaching(req, res, rsrc, response.caching()); + res.setStatus(statusCode); + logger.atFinest().log("REST call succeeded: %d", statusCode); } - if (response instanceof Response.Redirect) { - CacheHeaders.setNotCacheable(res); - String location = ((Response.Redirect) response).location(); - res.sendRedirect(location); - logger.atFinest().log("REST call redirected to: %s", location); - return; - } else if (response instanceof Response.Accepted) { - CacheHeaders.setNotCacheable(res); - res.setStatus(response.statusCode()); - res.setHeader(HttpHeaders.LOCATION, ((Response.Accepted) response).location()); - logger.atFinest().log("REST call succeeded: %d", response.statusCode()); - return; - } - - statusCode = response.statusCode(); - response.headers().forEach((k, v) -> res.setHeader(k, v)); - configureCaching(req, res, rsrc, response.caching()); - res.setStatus(statusCode); - logger.atFinest().log("REST call succeeded: %d", statusCode); - } - - if (response != Response.none()) { - Object value = Response.unwrap(response); - if (value instanceof BinaryResult) { - responseBytes = replyBinaryResult(req, res, (BinaryResult) value); - } else { - responseBytes = replyJson(req, res, false, qp.config(), value); + if (response != Response.none()) { + Object value = Response.unwrap(response); + if (value instanceof BinaryResult) { + responseBytes = replyBinaryResult(req, res, (BinaryResult) value); + } else { + responseBytes = replyJson(req, res, false, qp.config(), value); + } } } - } - } catch (MalformedJsonException | JsonParseException e) { - cause = Optional.of(e); - logger.atFine().withCause(e).log("REST call failed on JSON parsing"); - responseBytes = - replyError( - req, res, statusCode = SC_BAD_REQUEST, "Invalid " + JSON_TYPE + " in request", e); - } catch (BadRequestException e) { - cause = Optional.of(e); - responseBytes = - replyError( - req, res, statusCode = SC_BAD_REQUEST, messageOr(e, "Bad Request"), e.caching(), e); - } catch (AuthException e) { - cause = Optional.of(e); - - StringBuilder messageBuilder = new StringBuilder(messageOr(e, "Forbidden")); - globals - .aclInfoController - .getAclInfoMessage() - .ifPresent(aclInfo -> messageBuilder.append("\n\n").append(aclInfo)); - - responseBytes = - replyError( - req, res, statusCode = SC_FORBIDDEN, messageBuilder.toString(), e.caching(), e); - } catch (AmbiguousViewException e) { - cause = Optional.of(e); - responseBytes = replyError(req, res, statusCode = SC_NOT_FOUND, messageOr(e, "Ambiguous"), e); - } catch (ResourceNotFoundException e) { - cause = Optional.of(e); - responseBytes = - replyError( - req, res, statusCode = SC_NOT_FOUND, messageOr(e, "Not Found"), e.caching(), e); - } catch (MethodNotAllowedException e) { - cause = Optional.of(e); - responseBytes = - replyError( - req, - res, - statusCode = SC_METHOD_NOT_ALLOWED, - messageOr(e, "Method Not Allowed"), - e.caching(), - e); - } catch (ResourceConflictException e) { - cause = Optional.of(e); - responseBytes = - replyError(req, res, statusCode = SC_CONFLICT, messageOr(e, "Conflict"), e.caching(), e); - } catch (PreconditionFailedException e) { - cause = Optional.of(e); - responseBytes = - replyError( - req, - res, - statusCode = SC_PRECONDITION_FAILED, - messageOr(e, "Precondition Failed"), - e.caching(), - e); - } catch (UnprocessableEntityException e) { - cause = Optional.of(e); - responseBytes = - replyError( - req, - res, - statusCode = SC_UNPROCESSABLE_ENTITY, - messageOr(e, "Unprocessable Entity"), - e.caching(), - e); - } catch (NotImplementedException e) { - cause = Optional.of(e); - logger.atSevere().withCause(e).log("Error in %s %s", req.getMethod(), uriForLogging(req)); - responseBytes = - replyError(req, res, statusCode = SC_NOT_IMPLEMENTED, messageOr(e, "Not Implemented"), e); - } catch (QuotaException e) { - cause = Optional.of(e); - responseBytes = - replyError( - req, - res, - statusCode = SC_TOO_MANY_REQUESTS, - messageOr(e, "Quota limit reached"), - e.caching(), - e); - } catch (InvalidDeadlineException e) { - cause = Optional.of(e); - responseBytes = - replyError(req, res, statusCode = SC_BAD_REQUEST, messageOr(e, "Bad Request"), e); - } catch (Exception e) { - cause = Optional.of(e); - - Optional<RequestCancelledException> requestCancelledException = - RequestCancelledException.getFromCausalChain(e); - if (requestCancelledException.isPresent()) { - RequestStateProvider.Reason cancellationReason = - requestCancelledException.get().getCancellationReason(); - globals.cancellationMetrics.countCancelledRequest( - RequestInfo.RequestType.REST, requestUri, cancellationReason); - statusCode = getCancellationStatusCode(cancellationReason); + } catch (MalformedJsonException | JsonParseException e) { + cause = Optional.of(e); + logger.atFine().withCause(e).log("REST call failed on JSON parsing"); responseBytes = replyError( - req, res, statusCode, getCancellationMessage(requestCancelledException.get()), e); - } else { - statusCode = SC_INTERNAL_SERVER_ERROR; + req, res, statusCode = SC_BAD_REQUEST, "Invalid " + JSON_TYPE + " in request", e); + } catch (BadRequestException e) { + cause = Optional.of(e); + responseBytes = + replyError( + req, res, statusCode = SC_BAD_REQUEST, messageOr(e, "Bad Request"), e.caching(), e); + } catch (AuthException e) { + cause = Optional.of(e); - Optional<ExceptionHook.Status> status = getStatus(e); - statusCode = status.map(ExceptionHook.Status::statusCode).orElse(SC_INTERNAL_SERVER_ERROR); + StringBuilder messageBuilder = new StringBuilder(messageOr(e, "Forbidden")); + globals + .aclInfoController + .getAclInfoMessage() + .ifPresent(aclInfo -> messageBuilder.append("\n\n").append(aclInfo)); - if (res.isCommitted()) { - responseBytes = 0; - if (statusCode == SC_INTERNAL_SERVER_ERROR) { - logger.atSevere().withCause(e).log( - "Error in %s %s, response already committed", req.getMethod(), uriForLogging(req)); - } else { - logger.atWarning().log( - "Response for %s %s already committed, wanted to set status %d", - req.getMethod(), uriForLogging(req), statusCode); - } + responseBytes = + replyError( + req, res, statusCode = SC_FORBIDDEN, messageBuilder.toString(), e.caching(), e); + } catch (AmbiguousViewException e) { + cause = Optional.of(e); + responseBytes = + replyError(req, res, statusCode = SC_NOT_FOUND, messageOr(e, "Ambiguous"), e); + } catch (ResourceNotFoundException e) { + cause = Optional.of(e); + responseBytes = + replyError( + req, res, statusCode = SC_NOT_FOUND, messageOr(e, "Not Found"), e.caching(), e); + } catch (MethodNotAllowedException e) { + cause = Optional.of(e); + responseBytes = + replyError( + req, + res, + statusCode = SC_METHOD_NOT_ALLOWED, + messageOr(e, "Method Not Allowed"), + e.caching(), + e); + } catch (ResourceConflictException e) { + cause = Optional.of(e); + responseBytes = + replyError( + req, res, statusCode = SC_CONFLICT, messageOr(e, "Conflict"), e.caching(), e); + } catch (PreconditionFailedException e) { + cause = Optional.of(e); + responseBytes = + replyError( + req, + res, + statusCode = SC_PRECONDITION_FAILED, + messageOr(e, "Precondition Failed"), + e.caching(), + e); + } catch (UnprocessableEntityException e) { + cause = Optional.of(e); + responseBytes = + replyError( + req, + res, + statusCode = SC_UNPROCESSABLE_ENTITY, + messageOr(e, "Unprocessable Entity"), + e.caching(), + e); + } catch (NotImplementedException e) { + cause = Optional.of(e); + logger.atSevere().withCause(e).log("Error in %s %s", req.getMethod(), uriForLogging(req)); + responseBytes = + replyError( + req, res, statusCode = SC_NOT_IMPLEMENTED, messageOr(e, "Not Implemented"), e); + } catch (QuotaException e) { + cause = Optional.of(e); + responseBytes = + replyError( + req, + res, + statusCode = SC_TOO_MANY_REQUESTS, + messageOr(e, "Quota limit reached"), + e.caching(), + e); + } catch (InvalidDeadlineException e) { + cause = Optional.of(e); + responseBytes = + replyError(req, res, statusCode = SC_BAD_REQUEST, messageOr(e, "Bad Request"), e); + } catch (Exception e) { + cause = Optional.of(e); + + Optional<RequestCancelledException> requestCancelledException = + RequestCancelledException.getFromCausalChain(e); + if (requestCancelledException.isPresent()) { + RequestStateProvider.Reason cancellationReason = + requestCancelledException.get().getCancellationReason(); + globals.cancellationMetrics.countCancelledRequest( + RequestInfo.RequestType.REST, requestUri, cancellationReason); + statusCode = getCancellationStatusCode(cancellationReason); + responseBytes = + replyError( + req, res, statusCode, getCancellationMessage(requestCancelledException.get()), e); } else { - res.reset(); - TraceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId)); + statusCode = SC_INTERNAL_SERVER_ERROR; - if (status.isPresent()) { - responseBytes = reply(req, res, e, status.get(), getUserMessages(e)); + Optional<ExceptionHook.Status> status = getStatus(e); + statusCode = + status.map(ExceptionHook.Status::statusCode).orElse(SC_INTERNAL_SERVER_ERROR); + + if (res.isCommitted()) { + responseBytes = 0; + if (statusCode == SC_INTERNAL_SERVER_ERROR) { + logger.atSevere().withCause(e).log( + "Error in %s %s, response already committed", + req.getMethod(), uriForLogging(req)); + } else { + logger.atWarning().log( + "Response for %s %s already committed, wanted to set status %d", + req.getMethod(), uriForLogging(req), statusCode); + } } else { - responseBytes = - replyInternalServerError(req, res, e, getViewName(viewData), getUserMessages(e)); + res.reset(); + TraceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId)); + + if (status.isPresent()) { + responseBytes = reply(req, res, e, status.get(), getUserMessages(e)); + } else { + responseBytes = + replyInternalServerError(req, res, e, getViewName(viewData), getUserMessages(e)); + } } } + } finally { + String metric = getViewName(viewData); + String formattedCause = cause.map(globals.retryHelper::formatCause).orElse("_none"); + globals.metrics.count.increment(metric); + if (statusCode >= SC_BAD_REQUEST) { + globals.metrics.errorCount.increment(metric, statusCode, formattedCause); + } + if (responseBytes != -1) { + globals.metrics.responseBytes.record(metric, responseBytes); + } + globals.metrics.serverLatency.record( + metric, System.nanoTime() - startNanos, TimeUnit.NANOSECONDS); + globals.auditService.dispatch( + new ExtendedHttpAuditEvent( + globals.webSession.get().getSessionId(), + globals.currentUser.get(), + req, + auditStartTs, + qp != null ? qp.params() : ImmutableListMultimap.of(), + inputRequestBody, + statusCode, + response, + rsrc, + viewData == null ? null : viewData.view)); } - } finally { - String metric = getViewName(viewData); - String formattedCause = cause.map(globals.retryHelper::formatCause).orElse("_none"); - globals.metrics.count.increment(metric); - if (statusCode >= SC_BAD_REQUEST) { - globals.metrics.errorCount.increment(metric, statusCode, formattedCause); - } - if (responseBytes != -1) { - globals.metrics.responseBytes.record(metric, responseBytes); - } - globals.metrics.serverLatency.record( - metric, System.nanoTime() - startNanos, TimeUnit.NANOSECONDS); - globals.auditService.dispatch( - new ExtendedHttpAuditEvent( - globals.webSession.get().getSessionId(), - globals.currentUser.get(), - req, - auditStartTs, - qp != null ? qp.params() : ImmutableListMultimap.of(), - inputRequestBody, - statusCode, - response, - rsrc, - viewData == null ? null : viewData.view)); } } @@ -1566,6 +1573,43 @@ return parameterNames; } + private TraceContext enableTracing(HttpServletRequest req, HttpServletResponse res) { + // There are 2 ways to enable tracing for REST calls: + // 1. by using the 'trace' or 'trace=<trace-id>' request parameter + // 2. by setting the 'X-Gerrit-Trace:' or 'X-Gerrit-Trace:<trace-id>' header + String traceValueFromHeader = req.getHeader(X_GERRIT_TRACE); + String traceValueFromRequestParam = req.getParameter(ParameterParser.TRACE_PARAMETER); + boolean doTrace = traceValueFromHeader != null || traceValueFromRequestParam != null; + + // Check whether no trace ID, one trace ID or 2 different trace IDs have been specified. + String traceId1; + String traceId2; + if (!Strings.isNullOrEmpty(traceValueFromHeader)) { + traceId1 = traceValueFromHeader; + if (!Strings.isNullOrEmpty(traceValueFromRequestParam) + && !traceValueFromHeader.equals(traceValueFromRequestParam)) { + traceId2 = traceValueFromRequestParam; + } else { + traceId2 = null; + } + } else { + traceId1 = Strings.emptyToNull(traceValueFromRequestParam); + traceId2 = null; + } + + // Use the first trace ID to start tracing. If this trace ID is null, a trace ID will be + // generated. + TraceContext traceContext = + TraceContext.newTrace( + doTrace, traceId1, (tagName, traceId) -> res.setHeader(X_GERRIT_TRACE, traceId)); + // If a second trace ID was specified, add a tag for it as well. + if (traceId2 != null) { + traceContext.addTag(RequestId.Type.TRACE_ID, traceId2); + res.addHeader(X_GERRIT_TRACE, traceId2); + } + return traceContext; + } + private RequestInfo createRequestInfo( TraceContext traceContext, HttpServletRequest req, String requestUri, List<IdString> path) { RequestInfo.Builder requestInfo =
diff --git a/java/com/google/gerrit/pgm/http/jetty/HttpLog.java b/java/com/google/gerrit/pgm/http/jetty/HttpLog.java index 1c3126b..e88bb88 100644 --- a/java/com/google/gerrit/pgm/http/jetty/HttpLog.java +++ b/java/com/google/gerrit/pgm/http/jetty/HttpLog.java
@@ -20,7 +20,6 @@ import com.google.gerrit.httpd.GetUserFilter; import com.google.gerrit.httpd.RequestMetricsFilter; import com.google.gerrit.httpd.restapi.LogRedactUtil; -import com.google.gerrit.httpd.restapi.RestApiServlet; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.util.SystemLog; import com.google.gerrit.server.util.time.TimeUtil; @@ -59,7 +58,6 @@ protected static final String P_CPU_USER = "Cpu-User"; protected static final String P_MEMORY = "Memory"; protected static final String P_COMMAND_STATUS = "Command-Status"; - protected static final String P_TRACE_ID = "traceId"; private final AsyncAppender async; @@ -123,10 +121,6 @@ set(event, P_REFERER, req.getHeader("Referer")); set(event, P_USER_AGENT, req.getHeader("User-Agent")); set(event, P_COMMAND_STATUS, rsp.getHeader(GIT_COMMAND_STATUS_HEADER)); - String traceId = rsp.getHeader(RestApiServlet.X_GERRIT_TRACE); - if (traceId != null) { - set(event, P_TRACE_ID, traceId); - } RequestMetricsFilter.Context ctx = (RequestMetricsFilter.Context) req.getAttribute(RequestMetricsFilter.METRICS_CONTEXT);
diff --git a/java/com/google/gerrit/pgm/http/jetty/HttpLogJsonLayout.java b/java/com/google/gerrit/pgm/http/jetty/HttpLogJsonLayout.java index 532ff68..54c587b 100644 --- a/java/com/google/gerrit/pgm/http/jetty/HttpLogJsonLayout.java +++ b/java/com/google/gerrit/pgm/http/jetty/HttpLogJsonLayout.java
@@ -26,7 +26,6 @@ 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_TRACE_ID; import static com.google.gerrit.pgm.http.jetty.HttpLog.P_USER; import static com.google.gerrit.pgm.http.jetty.HttpLog.P_USER_AGENT; @@ -59,7 +58,6 @@ public String referer; public String userAgent; public String commandStatus; - public String traceId; public HttpJsonLogEntry(LoggingEvent event) { this.host = getMdcString(event, P_HOST); @@ -78,7 +76,6 @@ this.referer = getMdcString(event, P_REFERER); this.userAgent = getMdcString(event, P_USER_AGENT); this.commandStatus = getMdcString(event, P_COMMAND_STATUS); - this.traceId = getMdcString(event, P_TRACE_ID); } } }
diff --git a/java/com/google/gerrit/pgm/http/jetty/HttpLogLayout.java b/java/com/google/gerrit/pgm/http/jetty/HttpLogLayout.java index e2a5e12..ddc1b5e 100644 --- a/java/com/google/gerrit/pgm/http/jetty/HttpLogLayout.java +++ b/java/com/google/gerrit/pgm/http/jetty/HttpLogLayout.java
@@ -83,9 +83,6 @@ buf.append(' '); dq_opt(buf, event, HttpLog.P_COMMAND_STATUS); - buf.append(' '); - opt(buf, event, HttpLog.P_TRACE_ID); - buf.append('\n'); return buf.toString(); }
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 9d00aa3..8e7b4d5 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -475,7 +475,7 @@ private boolean newChangeForAllNotInTarget; private boolean setChangeAsPrivate; private Optional<NoteDbPushOption> noteDbPushOption; - private Optional<String> tracePushOption = Optional.empty(); + private Optional<String> tracePushOption; private MessageSender messageSender; private ReceiveCommitsResult.Builder result; @@ -685,7 +685,7 @@ void sendMessages() { try (TraceContext traceContext = TraceContext.newTrace( - tracePushOption.isPresent(), + loggingTags.containsKey(RequestId.Type.TRACE_ID.name()), loggingTags.get(RequestId.Type.TRACE_ID.name()), (tagName, traceId) -> {})) { loggingTags.forEach((tagName, tagValue) -> traceContext.addTag(tagName, tagValue)); @@ -712,11 +712,7 @@ TraceContext.newTrace( tracePushOption.isPresent(), tracePushOption.orElse(null), - (tagName, traceId) -> { - if (tracePushOption.isPresent()) { - addMessage(tagName + ": " + traceId); - } - }); + (tagName, traceId) -> addMessage(tagName + ": " + traceId)); PerformanceLogContext performanceLogContext = new PerformanceLogContext(config, performanceLoggers); TraceTimer traceTimer = @@ -1379,6 +1375,8 @@ List<String> traceValues = pushOptions.get("trace"); if (!traceValues.isEmpty()) { tracePushOption = Optional.of(Iterables.getLast(traceValues)); + } else { + tracePushOption = Optional.empty(); } }
diff --git a/java/com/google/gerrit/server/logging/TraceContext.java b/java/com/google/gerrit/server/logging/TraceContext.java index a07a1eb..3213422 100644 --- a/java/com/google/gerrit/server/logging/TraceContext.java +++ b/java/com/google/gerrit/server/logging/TraceContext.java
@@ -118,7 +118,7 @@ * * <p>No-op if {@code trace} is {@code false}. * - * @param forceLogging whether logging should be forced + * @param trace whether tracing should be started * @param traceId trace ID that should be used for tracing, if {@code null} a trace ID is * generated * @param traceIdConsumer consumer for the trace ID, should be used to return the generated trace @@ -126,23 +126,29 @@ * @return the trace context */ public static TraceContext newTrace( - boolean forceLogging, @Nullable String traceId, TraceIdConsumer traceIdConsumer) { - String effectiveId; - if (!Strings.isNullOrEmpty(traceId)) { - effectiveId = traceId; - } else { - Optional<String> existingTraceId = - LoggingContext.getInstance().getTagsAsMap().get(RequestId.Type.TRACE_ID.name()).stream() - .findAny(); - effectiveId = existingTraceId.orElse(new RequestId().toString()); + boolean trace, @Nullable String traceId, TraceIdConsumer traceIdConsumer) { + if (!trace) { + // Create an empty trace context. + return open(); } - traceIdConsumer.accept(RequestId.Type.TRACE_ID.name(), effectiveId); - TraceContext traceContext = open().addTag(RequestId.Type.TRACE_ID, effectiveId); - if (forceLogging) { - return traceContext.forceLogging(); + if (!Strings.isNullOrEmpty(traceId)) { + traceIdConsumer.accept(RequestId.Type.TRACE_ID.name(), traceId); + return open().addTag(RequestId.Type.TRACE_ID, traceId).forceLogging(); } - return traceContext; + + Optional<String> existingTraceId = + LoggingContext.getInstance().getTagsAsMap().get(RequestId.Type.TRACE_ID.name()).stream() + .findAny(); + if (existingTraceId.isPresent()) { + // request tracing was already started, no need to generate a new trace ID + traceIdConsumer.accept(RequestId.Type.TRACE_ID.name(), existingTraceId.get()); + return open(); + } + + RequestId newTraceId = new RequestId(); + traceIdConsumer.accept(RequestId.Type.TRACE_ID.name(), newTraceId.toString()); + return open().addTag(RequestId.Type.TRACE_ID, newTraceId).forceLogging(); } @FunctionalInterface
diff --git a/java/com/google/gerrit/sshd/BaseCommand.java b/java/com/google/gerrit/sshd/BaseCommand.java index 10eaff3..a77ada4 100644 --- a/java/com/google/gerrit/sshd/BaseCommand.java +++ b/java/com/google/gerrit/sshd/BaseCommand.java
@@ -31,8 +31,6 @@ import com.google.gerrit.server.RequestCleanup; import com.google.gerrit.server.git.ProjectRunnable; import com.google.gerrit.server.git.WorkQueue.CancelableRunnable; -import com.google.gerrit.server.ioutil.HexFormat; -import com.google.gerrit.server.logging.TraceContext; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; @@ -166,18 +164,6 @@ this.argv = argv; } - public void setForceTracing(boolean v) { - context.setForceTracing(v); - } - - public void setTraceId(String id) { - context.setTraceId(id); - } - - public SshSession getSession() { - return context.getSession(); - } - /** * Trim the argument if it is spanning multiple lines. * @@ -519,12 +505,7 @@ } catch (Throwable e) { flushIgnoreException(out); flushIgnoreException(err); - try (TraceContext traceContext = - TraceContext.newTrace( - context.getForceTracing(), context.getTraceId(), (trace, traceId) -> {})) { - traceContext.addTag("SSH_SESSION", HexFormat.fromInt(getSession().getSessionId())); - rc = handleError(e); - } + rc = handleError(e); } finally { try { onExit(rc);
diff --git a/java/com/google/gerrit/sshd/SshCommand.java b/java/com/google/gerrit/sshd/SshCommand.java index 24178eb..7df7f3f 100644 --- a/java/com/google/gerrit/sshd/SshCommand.java +++ b/java/com/google/gerrit/sshd/SshCommand.java
@@ -27,7 +27,6 @@ import com.google.gerrit.server.cancellation.RequestCancelledException; import com.google.gerrit.server.cancellation.RequestStateContext; import com.google.gerrit.server.config.GerritServerConfig; -import com.google.gerrit.server.ioutil.HexFormat; import com.google.gerrit.server.logging.PerformanceLogContext; import com.google.gerrit.server.logging.PerformanceLogger; import com.google.gerrit.server.logging.TraceContext; @@ -63,7 +62,6 @@ @Override public void start(ChannelSession channel, Environment env) throws IOException { - String sessionId = HexFormat.fromInt(getSession().getSessionId()); startThread( () -> { try (PerThreadCache ignored = PerThreadCache.create(); @@ -74,7 +72,6 @@ try (TraceContext traceContext = enableTracing(); PerformanceLogContext performanceLogContext = new PerformanceLogContext(config, performanceLoggers)) { - traceContext.addTag("SSH_SESSION", sessionId); RequestInfo requestInfo = RequestInfo.builder(RequestInfo.RequestType.SSH, getName(), user, traceContext) .build(); @@ -126,12 +123,6 @@ return TraceContext.newTrace( trace, traceId, - (tagName, traceId) -> { - if (trace) { - stderr.println(String.format("%s: %s", tagName, traceId)); - } - setForceTracing(trace); - setTraceId(traceId); - }); + (tagName, traceId) -> stderr.println(String.format("%s: %s", tagName, traceId))); } }
diff --git a/java/com/google/gerrit/sshd/SshLog.java b/java/com/google/gerrit/sshd/SshLog.java index 7c3f871..7c96342 100644 --- a/java/com/google/gerrit/sshd/SshLog.java +++ b/java/com/google/gerrit/sshd/SshLog.java
@@ -50,7 +50,6 @@ protected static final String LOG_NAME = "sshd_log"; protected static final String P_SESSION = "session"; - protected static final String P_TRACE_ID = "traceId"; protected static final String P_USER_NAME = "userName"; protected static final String P_ACCOUNT_ID = "accountId"; protected static final String P_WAIT = "queueWaitTime"; @@ -292,10 +291,6 @@ ); event.setProperty(P_SESSION, id(sd.getSessionId())); - String traceId = context.get().getTraceId(); - if (traceId != null) { - event.setProperty(P_TRACE_ID, context.get().getTraceId()); - } String userName = "-"; String accountId = "-";
diff --git a/java/com/google/gerrit/sshd/SshLogJsonLayout.java b/java/com/google/gerrit/sshd/SshLogJsonLayout.java index 8f28342..321cf56 100644 --- a/java/com/google/gerrit/sshd/SshLogJsonLayout.java +++ b/java/com/google/gerrit/sshd/SshLogJsonLayout.java
@@ -22,7 +22,6 @@ 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_TOTAL_CPU; -import static com.google.gerrit.sshd.SshLog.P_TRACE_ID; import static com.google.gerrit.sshd.SshLog.P_USER_CPU; import static com.google.gerrit.sshd.SshLog.P_USER_NAME; import static com.google.gerrit.sshd.SshLog.P_WAIT; @@ -45,7 +44,6 @@ private class SshJsonLogEntry extends JsonLogEntry { public String timestamp; public String session; - public String traceId; public String thread; public String user; public String accountId; @@ -72,7 +70,6 @@ public SshJsonLogEntry(LoggingEvent event) { this.timestamp = timestampFormatter.format(event.getTimeStamp()); this.session = getMdcString(event, P_SESSION); - this.traceId = getMdcString(event, P_TRACE_ID); this.thread = event.getThreadName(); this.user = getMdcString(event, P_USER_NAME); this.accountId = getMdcString(event, P_ACCOUNT_ID);
diff --git a/java/com/google/gerrit/sshd/SshLogLayout.java b/java/com/google/gerrit/sshd/SshLogLayout.java index 7ba88b0..bb7edfa 100644 --- a/java/com/google/gerrit/sshd/SshLogLayout.java +++ b/java/com/google/gerrit/sshd/SshLogLayout.java
@@ -22,7 +22,6 @@ 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_TOTAL_CPU; -import static com.google.gerrit.sshd.SshLog.P_TRACE_ID; import static com.google.gerrit.sshd.SshLog.P_USER_CPU; import static com.google.gerrit.sshd.SshLog.P_USER_NAME; import static com.google.gerrit.sshd.SshLog.P_WAIT; @@ -72,8 +71,6 @@ req(P_MEMORY, buf, event); } - req(P_TRACE_ID, buf, event); - buf.append('\n'); return buf.toString(); }
diff --git a/java/com/google/gerrit/sshd/SshScope.java b/java/com/google/gerrit/sshd/SshScope.java index b82320b..9ab63a4 100644 --- a/java/com/google/gerrit/sshd/SshScope.java +++ b/java/com/google/gerrit/sshd/SshScope.java
@@ -52,8 +52,6 @@ private volatile long finishedUserCpu; private volatile long startedMemory; private volatile long finishedMemory; - private volatile boolean forceTracing; - private volatile String traceId; private IdentifiedUser identifiedUser; @@ -125,22 +123,6 @@ return session; } - void setForceTracing(boolean v) { - forceTracing = v; - } - - boolean getForceTracing() { - return forceTracing; - } - - void setTraceId(String id) { - traceId = id; - } - - String getTraceId() { - return traceId; - } - @Override public CurrentUser getUser() { CurrentUser user = session.getUser();
diff --git a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java index fcb5301..8f267db 100644 --- a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
@@ -103,8 +103,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new1"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -256,7 +256,7 @@ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo); PushOneCommit.Result r = push.to("refs/heads/master"); r.assertOkStatus(); - assertThat(commitValidationListener.traceId).isNotNull(); + assertThat(commitValidationListener.traceId).isNull(); assertThat(commitValidationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -305,7 +305,7 @@ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo); PushOneCommit.Result r = push.to("refs/for/master"); r.assertOkStatus(); - assertThat(commitValidationListener.traceId).isNotNull(); + assertThat(commitValidationListener.traceId).isNull(); assertThat(commitValidationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -433,8 +433,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new12"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).contains("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isEqualTo("issue123"); assertThat(projectCreationListener.isLoggingForced).isTrue(); assertThat(projectCreationListener.tags.get("project")).containsExactly("new12"); } @@ -449,8 +449,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new13"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).contains("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isEqualTo("issue123"); assertThat(projectCreationListener.isLoggingForced).isTrue(); assertThat(projectCreationListener.tags.get("project")).containsExactly("new13"); } @@ -465,8 +465,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new13"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -483,8 +483,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new14"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -501,8 +501,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new15"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).contains("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isEqualTo("issue123"); assertThat(projectCreationListener.isLoggingForced).isTrue(); assertThat(projectCreationListener.tags.get("project")).containsExactly("new15"); } @@ -517,8 +517,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new16"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -535,8 +535,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new17"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceId).isNotNull(); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -553,8 +553,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new18"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -571,8 +571,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new19"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issues123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -590,8 +590,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new20"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).contains("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isEqualTo("issue123"); assertThat(projectCreationListener.isLoggingForced).isTrue(); assertThat(projectCreationListener.tags.get("project")).containsExactly("new20"); } @@ -607,8 +607,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new21"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -630,7 +630,7 @@ PushOneCommit push = pushFactory.create(admin.newIdent(), tracedRepo); PushOneCommit.Result r = push.to("refs/for/master"); r.assertOkStatus(); - assertThat(commitValidationListener.traceIds).contains("issue123"); + assertThat(commitValidationListener.traceId).isEqualTo("issue123"); assertThat(commitValidationListener.isLoggingForced).isTrue(); assertThat(commitValidationListener.tags.get("project")).containsExactly(tracedProject.get()); } @@ -642,7 +642,7 @@ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo); PushOneCommit.Result r = push.to("refs/for/master"); r.assertOkStatus(); - assertThat(commitValidationListener.traceId).isNotNull(); + assertThat(commitValidationListener.traceId).isNull(); assertThat(commitValidationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -659,8 +659,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new22"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -678,8 +678,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new23"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).contains("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isEqualTo("issue123"); assertThat(projectCreationListener.isLoggingForced).isTrue(); assertThat(projectCreationListener.tags.get("project")).containsExactly("new23"); } @@ -695,8 +695,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new24"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -714,8 +714,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new25"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -732,8 +732,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new26"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).contains("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isEqualTo("issue123"); assertThat(projectCreationListener.isLoggingForced).isTrue(); assertThat(projectCreationListener.tags.get("project")).containsExactly("new26"); } @@ -748,8 +748,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new27"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -766,8 +766,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/new28"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -784,8 +784,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/xyz1"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -802,8 +802,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/xyz2"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -821,8 +821,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/xyz3"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).contains("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isEqualTo("issue123"); assertThat(projectCreationListener.isLoggingForced).isTrue(); assertThat(projectCreationListener.tags.get("project")).containsExactly("xyz3"); } @@ -838,8 +838,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/xyz4"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -857,8 +857,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/xyz5"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).contains("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isEqualTo("issue123"); assertThat(projectCreationListener.isLoggingForced).isTrue(); assertThat(projectCreationListener.tags.get("project")).containsExactly("xyz5"); } @@ -873,8 +873,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { RestResponse response = adminRestSession.put("/projects/xyz6"); assertThat(response.getStatusCode()).isEqualTo(SC_CREATED); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(projectCreationListener.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(projectCreationListener.traceId).isNull(); assertThat(projectCreationListener.isLoggingForced).isFalse(); // The logging tag with the project name is also set if tracing is off. @@ -892,8 +892,8 @@ RestResponse response = adminRestSession.get(String.format("/changes/%s/suggest_reviewers?limit=10", changeId)); assertThat(response.getStatusCode()).isEqualTo(SC_OK); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(reviewerSuggestion.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(reviewerSuggestion.traceId).isNull(); assertThat(reviewerSuggestion.isLoggingForced).isFalse(); } } @@ -909,8 +909,8 @@ RestResponse response = adminRestSession.get(String.format("/changes/%s/suggest_reviewers?limit=10", changeId)); assertThat(response.getStatusCode()).isEqualTo(SC_OK); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(reviewerSuggestion.traceIds).contains("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(reviewerSuggestion.traceId).isEqualTo("issue123"); assertThat(reviewerSuggestion.isLoggingForced).isTrue(); } } @@ -926,8 +926,8 @@ RestResponse response = adminRestSession.get(String.format("/changes/%s/suggest_reviewers?limit=10", changeId)); assertThat(response.getStatusCode()).isEqualTo(SC_OK); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(reviewerSuggestion.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(reviewerSuggestion.traceId).isNull(); assertThat(reviewerSuggestion.isLoggingForced).isFalse(); } } @@ -943,8 +943,8 @@ RestResponse response = adminRestSession.get(String.format("/changes/%s/suggest_reviewers?limit=10", changeId)); assertThat(response.getStatusCode()).isEqualTo(SC_OK); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(reviewerSuggestion.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(reviewerSuggestion.traceId).isNull(); assertThat(reviewerSuggestion.isLoggingForced).isFalse(); } } @@ -963,8 +963,8 @@ new BasicHeader("User-Agent", "foo-bar"), new BasicHeader("Other-Header", "baz")); assertThat(response.getStatusCode()).isEqualTo(SC_OK); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(reviewerSuggestion.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(reviewerSuggestion.traceId).isNull(); assertThat(reviewerSuggestion.isLoggingForced).isFalse(); } } @@ -984,8 +984,8 @@ new BasicHeader("User-Agent", "foo-bar"), new BasicHeader("Other-Header", "baz")); assertThat(response.getStatusCode()).isEqualTo(SC_OK); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(reviewerSuggestion.traceIds).contains("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(reviewerSuggestion.traceId).isEqualTo("issue123"); assertThat(reviewerSuggestion.isLoggingForced).isTrue(); } } @@ -1003,8 +1003,8 @@ new BasicHeader("User-Agent", "foo-bar"), new BasicHeader("Other-Header", "baz")); assertThat(response.getStatusCode()).isEqualTo(SC_OK); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(reviewerSuggestion.traceId).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(reviewerSuggestion.traceId).isNull(); assertThat(reviewerSuggestion.isLoggingForced).isFalse(); } } @@ -1022,8 +1022,8 @@ new BasicHeader("User-Agent", "foo-bar"), new BasicHeader("Other-Header", "baz")); assertThat(response.getStatusCode()).isEqualTo(SC_OK); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(reviewerSuggestion.traceIds).doesNotContain("issue123"); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(reviewerSuggestion.traceId).isNull(); assertThat(reviewerSuggestion.isLoggingForced).isFalse(); } } @@ -1039,13 +1039,8 @@ try (Registration registration = extensionRegistry.newRegistration().add(traceSubmitRule)) { RestResponse response = adminRestSession.post("/changes/" + changeId + "/submit"); assertThat(response.getStatusCode()).isEqualTo(SC_INTERNAL_SERVER_ERROR); - assertThat( - response.getHeaders(RestApiServlet.X_GERRIT_TRACE).stream() - .anyMatch(h -> h.startsWith("retry-on-failure-"))) - .isTrue(); - assertThat( - traceSubmitRule.traceIds.stream().anyMatch(id -> id.startsWith("retry-on-failure-"))) - .isTrue(); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).startsWith("retry-on-failure-"); + assertThat(traceSubmitRule.traceId).startsWith("retry-on-failure-"); assertThat(traceSubmitRule.isLoggingForced).isTrue(); } } @@ -1072,8 +1067,8 @@ })) { RestResponse response = adminRestSession.post("/changes/" + changeId + "/submit"); assertThat(response.getStatusCode()).isEqualTo(SC_INTERNAL_SERVER_ERROR); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(traceSubmitRule.traceId).isNotNull(); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(traceSubmitRule.traceId).isNull(); assertThat(traceSubmitRule.isLoggingForced).isFalse(); } } @@ -1088,8 +1083,8 @@ try (Registration registration = extensionRegistry.newRegistration().add(traceSubmitRule)) { RestResponse response = adminRestSession.post("/changes/" + changeId + "/submit"); assertThat(response.getStatusCode()).isEqualTo(SC_INTERNAL_SERVER_ERROR); - assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull(); - assertThat(traceSubmitRule.traceId).isNotNull(); + assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull(); + assertThat(traceSubmitRule.traceId).isNull(); assertThat(traceSubmitRule.isLoggingForced).isFalse(); } } @@ -1118,7 +1113,6 @@ private static class TraceValidatingCommitValidationListener implements CommitValidationListener { String traceId; - ImmutableSet<String> traceIds; Boolean isLoggingForced; ImmutableSetMultimap<String, String> tags; @@ -1127,7 +1121,6 @@ throws CommitValidationException { this.traceId = Iterables.getFirst(LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID"), null); - this.traceIds = LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID"); this.isLoggingForced = LoggingContext.getInstance().shouldForceLogging(null, null, false); this.tags = LoggingContext.getInstance().getTagsAsMap(); return ImmutableList.of(); @@ -1136,7 +1129,6 @@ private static class TraceReviewerSuggestion implements ReviewerSuggestion { String traceId; - ImmutableSet<String> traceIds; Boolean isLoggingForced; @Override @@ -1147,7 +1139,6 @@ Set<com.google.gerrit.entities.Account.Id> candidates) { this.traceId = Iterables.getFirst(LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID"), null); - this.traceIds = LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID"); this.isLoggingForced = LoggingContext.getInstance().shouldForceLogging(null, null, false); return ImmutableSet.of(); } @@ -1167,7 +1158,6 @@ private static class TraceSubmitRule implements SubmitRule { String traceId; - ImmutableSet<String> traceIds; Boolean isLoggingForced; boolean failOnce; boolean failAlways; @@ -1176,7 +1166,6 @@ public Optional<SubmitRecord> evaluate(ChangeData changeData) { this.traceId = Iterables.getFirst(LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID"), null); - this.traceIds = LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID"); this.isLoggingForced = LoggingContext.getInstance().shouldForceLogging(null, null, false); if (failOnce || failAlways) {
diff --git a/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java b/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java index ed268ce..8153a5d 100644 --- a/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java +++ b/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java
@@ -47,8 +47,8 @@ extensionRegistry.newRegistration().add(projectCreationListener)) { adminSshSession.exec("gerrit create-project new1"); adminSshSession.assertSuccess(); - assertThat(projectCreationListener.traceId).isNotNull(); - assertThat(projectCreationListener.foundTraceId).isTrue(); + assertThat(projectCreationListener.traceId).isNull(); + assertThat(projectCreationListener.foundTraceId).isFalse(); assertThat(projectCreationListener.isLoggingForced).isFalse(); } }
diff --git a/javatests/com/google/gerrit/server/logging/TraceContextTest.java b/javatests/com/google/gerrit/server/logging/TraceContextTest.java index 0478acc..6a3632d 100644 --- a/javatests/com/google/gerrit/server/logging/TraceContextTest.java +++ b/javatests/com/google/gerrit/server/logging/TraceContextTest.java
@@ -15,14 +15,11 @@ package com.google.gerrit.server.logging; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth.assertWithMessage; import static com.google.gerrit.testing.GerritJUnit.assertThrows; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.gerrit.server.logging.TraceContext.TraceIdConsumer; -import java.util.Arrays; -import java.util.List; import java.util.Map; import java.util.Set; import org.junit.After; @@ -183,75 +180,37 @@ } @Test - public void newTraceEnabledWithoutForceLogging() { + public void newTraceDisabled() { TestTraceIdConsumer traceIdConsumer = new TestTraceIdConsumer(); try (TraceContext traceContext = TraceContext.newTrace(false, null, traceIdConsumer)) { assertForceLogging(false); - assertThat(LoggingContext.getInstance().getTagsAsMap().keySet()) - .containsExactly(RequestId.Type.TRACE_ID.name()); + assertTags(ImmutableMap.of()); } - assertThat(traceIdConsumer.tagName).isEqualTo(RequestId.Type.TRACE_ID.name()); - assertThat(traceIdConsumer.traceId).isNotNull(); + assertThat(traceIdConsumer.tagName).isNull(); + assertThat(traceIdConsumer.traceId).isNull(); } @Test - public void newTraceEnabledWithoutForceLoggingWithProvidedTraceId() { + public void newTraceDisabledWithProvidedTraceId() { TestTraceIdConsumer traceIdConsumer = new TestTraceIdConsumer(); try (TraceContext traceContext = TraceContext.newTrace(false, "foo", traceIdConsumer)) { assertForceLogging(false); - assertThat(LoggingContext.getInstance().getTagsAsMap().keySet()) - .containsExactly(RequestId.Type.TRACE_ID.name()); + assertTags(ImmutableMap.of()); } - assertThat(traceIdConsumer.tagName).isEqualTo("TRACE_ID"); - assertThat(traceIdConsumer.traceId).isEqualTo("foo"); + assertThat(traceIdConsumer.tagName).isNull(); + assertThat(traceIdConsumer.traceId).isNull(); } @Test - public void newTraceNestingAndForceLogging() { - // create cartesian product of all possible values for each of the four parameters - for (boolean forceOuter : List.of(false, true)) { - for (String outerId : Arrays.asList(null, "outer")) { - for (boolean forceInner : List.of(false, true)) { - for (String innerId : Arrays.asList(null, "inner")) { - newTraceNesting(forceOuter, outerId, forceInner, innerId); - } - } - } - } - } - - private void newTraceNesting( - boolean forceOuter, String outerId, boolean forceInner, String innerId) { - String message = - String.format("parameters: (%s, %s, %s, %s)", forceOuter, outerId, forceInner, innerId); - try (TraceContext outer = - TraceContext.newTrace(forceOuter, outerId, new TestTraceIdConsumer())) { - assertForceLogging(forceOuter, message); - try (TraceContext nested = - TraceContext.newTrace(forceInner, innerId, new TestTraceIdConsumer())) { - assertForceLogging(forceOuter || forceInner, message); - } - } - } - - @Test - public void onlyOneTraceId() throws InterruptedException { - for (boolean forceOuter : List.of(false, true)) { - for (boolean forceInner : List.of(false, true)) { - onlyOneTraceId(forceOuter, forceInner); - } - } - } - - public void onlyOneTraceId(boolean forceOuter, boolean forceInner) throws InterruptedException { + public void onlyOneTraceId() { TestTraceIdConsumer traceIdConsumer1 = new TestTraceIdConsumer(); - try (TraceContext traceContext1 = TraceContext.newTrace(forceOuter, null, traceIdConsumer1)) { + try (TraceContext traceContext1 = TraceContext.newTrace(true, null, traceIdConsumer1)) { String expectedTraceId = traceIdConsumer1.traceId; assertThat(expectedTraceId).isNotNull(); TestTraceIdConsumer traceIdConsumer2 = new TestTraceIdConsumer(); - Thread.sleep(2); - try (TraceContext traceContext2 = TraceContext.newTrace(forceInner, null, traceIdConsumer2)) { + try (TraceContext traceContext2 = TraceContext.newTrace(true, null, traceIdConsumer2)) { + assertForceLogging(true); assertTags( ImmutableMap.of(RequestId.Type.TRACE_ID.name(), ImmutableSet.of(expectedTraceId))); } @@ -262,21 +221,13 @@ @Test public void multipleTraceIdsIfTraceIdProvided() { - for (boolean forceOuter : List.of(false, true)) { - for (boolean forceInner : List.of(false, true)) { - multipleTraceIdsIfTraceIdProvided(forceOuter, forceInner); - } - } - } - - public void multipleTraceIdsIfTraceIdProvided(boolean forceOuter, boolean forceInner) { String traceId1 = "foo"; try (TraceContext traceContext1 = - TraceContext.newTrace(forceOuter, traceId1, (tagName, traceId) -> {})) { + TraceContext.newTrace(true, traceId1, (tagName, traceId) -> {})) { TestTraceIdConsumer traceIdConsumer = new TestTraceIdConsumer(); String traceId2 = "bar"; - try (TraceContext traceContext2 = - TraceContext.newTrace(forceInner, traceId2, traceIdConsumer)) { + try (TraceContext traceContext2 = TraceContext.newTrace(true, traceId2, traceIdConsumer)) { + assertForceLogging(true); assertTags( ImmutableMap.of(RequestId.Type.TRACE_ID.name(), ImmutableSet.of(traceId1, traceId2))); } @@ -316,12 +267,6 @@ .isEqualTo(expected); } - private void assertForceLogging(boolean expected, String message) { - assertWithMessage(message) - .that(LoggingContext.getInstance().shouldForceLogging(null, null, false)) - .isEqualTo(expected); - } - private static class TestTraceIdConsumer implements TraceIdConsumer { String tagName; String traceId;
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-fix-preview.ts b/polygerrit-ui/app/elements/checks/gr-checks-fix-preview.ts index 89685e6..605b5e7 100644 --- a/polygerrit-ui/app/elements/checks/gr-checks-fix-preview.ts +++ b/polygerrit-ui/app/elements/checks/gr-checks-fix-preview.ts
@@ -61,6 +61,9 @@ @state() loggedIn = false; + @state() + selectedReplacementIdx = 0; + private readonly getChangeModel = resolve(this, changeModelToken); private readonly getUserModel = resolve(this, userModelToken); @@ -120,6 +123,18 @@ border: 1px solid var(--border-color); padding: var(--spacing-xl); } + .fix-picker { + display: flex; + align-items: center; + margin-left: var(--spacing-m); + } + .fix-picker gr-button { + margin: 0 var(--spacing-s); + } + .title { + display: flex; + align-items: center; + } `, ]; } @@ -130,10 +145,38 @@ } private renderHeader() { + const replacementCount = this.fixSuggestionInfo?.replacements?.length ?? 0; return html` <div class="header"> <div class="title"> <span>Attached Fix</span> + ${replacementCount > 1 + ? html` + <div class="fix-picker"> + <span + >${this.selectedReplacementIdx + 1} of + ${replacementCount}</span + > + <gr-button + id="prevFix" + link + @click=${this.onPrevFixClick} + ?disabled=${this.selectedReplacementIdx === 0} + > + <gr-icon icon="chevron_left"></gr-icon> + </gr-button> + <gr-button + id="nextFix" + link + @click=${this.onNextFixClick} + ?disabled=${this.selectedReplacementIdx === + replacementCount - 1} + > + <gr-icon icon="chevron_right"></gr-icon> + </gr-button> + </div> + ` + : nothing} </div> <div> <gr-button @@ -162,9 +205,16 @@ } private renderDiff() { + // Create a new fixSuggestionInfo with just the current replacement + const newFixSuggestionInfo = this.fixSuggestionInfo && { + ...this.fixSuggestionInfo, + replacements: [ + this.fixSuggestionInfo.replacements[this.selectedReplacementIdx], + ], + }; return html` <gr-suggestion-diff-preview - .fixSuggestionInfo=${this.fixSuggestionInfo} + .fixSuggestionInfo=${newFixSuggestionInfo} .patchSet=${this.patchSet} .codeText=${'Loading fix preview ...'} @preview-loaded=${() => (this.previewLoaded = true)} @@ -214,6 +264,26 @@ if (!this.loggedIn) return 'You must be logged in to apply a fix'; return ''; } + + private onPrevFixClick(e: Event) { + if (e) e.stopPropagation(); + if (this.selectedReplacementIdx >= 1) { + this.selectedReplacementIdx -= 1; + this.previewLoaded = false; + } + } + + private onNextFixClick(e: Event) { + if (e) e.stopPropagation(); + if ( + this.fixSuggestionInfo && + this.selectedReplacementIdx < + this.fixSuggestionInfo.replacements.length - 1 + ) { + this.selectedReplacementIdx += 1; + this.previewLoaded = false; + } + } } declare global {
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts index d0bb1ef..8e3963c 100644 --- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts +++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
@@ -108,6 +108,9 @@ @state() loggedIn = false; + @state() + selectedReplacementIdx = 0; + private readonly restApiService = getAppContext().restApiService; private readonly getUserModel = resolve(this, userModelToken); @@ -274,8 +277,11 @@ private renderFooter() { const fixCount = this.fixSuggestions?.length ?? 0; + const replacementCount = this.currentFix?.replacements?.length ?? 0; const reasonForDisabledApplyButton = this.computeTooltip(); - const shouldRenderNav = fixCount >= 2; + const shouldRenderNav = + fixCount >= 2 || + (this.currentFix?.fix_id === PROVIDED_FIX_ID && replacementCount >= 2); const shouldRenderWarning = !!reasonForDisabledApplyButton; if (!shouldRenderNav && !shouldRenderWarning) return nothing; @@ -283,7 +289,7 @@ return html` <div slot="footer" class="fix-picker"> ${when(shouldRenderNav, () => - this.renderNavForMultipleSuggestedFixes(fixCount) + this.renderNavigation(fixCount, replacementCount) )} ${when(shouldRenderWarning, () => this.renderWarning(reasonForDisabledApplyButton) @@ -292,6 +298,37 @@ `; } + private renderNavigation(fixCount: number, replacementCount: number) { + // TODO(b/389011542) Fix apply:fix endpoint to support multiple replacements + // apply:fix endpoint currently supports only one replacement + if (this.currentFix?.fix_id === PROVIDED_FIX_ID && replacementCount > 1) { + return this.renderReplacementNavigation(replacementCount); + } + // TODO(b/227463363) Remove once Robot Comments are deprecated. + return this.renderNavForMultipleSuggestedFixes(fixCount); + } + + private renderReplacementNavigation(replacementCount: number) { + const id = this.selectedReplacementIdx; + return html` + <span>Replacement ${id + 1} of ${replacementCount}</span> + <gr-button + id="prevFix" + @click=${this.onPrevReplacementClick} + ?disabled=${id === 0} + > + <gr-icon icon="chevron_left"></gr-icon> + </gr-button> + <gr-button + id="nextFix" + @click=${this.onNextReplacementClick} + ?disabled=${id === replacementCount - 1} + > + <gr-icon icon="chevron_right"></gr-icon> + </gr-button> + `; + } + private renderNavForMultipleSuggestedFixes(fixCount: number) { const id = this.selectedFixIdx; return html` @@ -352,10 +389,12 @@ let res: FilePathToDiffInfoMap | undefined; try { if (fixSuggestion.fix_id === PROVIDED_FIX_ID) { + const currentReplacement = + fixSuggestion.replacements[this.selectedReplacementIdx]; res = await this.restApiService.getFixPreview( this.changeNum, this.patchNum, - fixSuggestion.replacements + [currentReplacement] ); } else { // TODO(b/227463363) Remove once Robot Comments are deprecated. @@ -414,6 +453,25 @@ } } + private onPrevReplacementClick(e: Event) { + if (e) e.stopPropagation(); + if (this.selectedReplacementIdx >= 1) { + this.selectedReplacementIdx -= 1; + this.fetchFixPreview(this.currentFix!); + } + } + + private onNextReplacementClick(e: Event) { + if (e) e.stopPropagation(); + if ( + this.currentFix?.replacements && + this.selectedReplacementIdx < this.currentFix.replacements.length - 1 + ) { + this.selectedReplacementIdx += 1; + this.fetchFixPreview(this.currentFix); + } + } + private close(fixApplied: boolean) { this.currentFix = undefined; this.currentPreviews = []; @@ -467,10 +525,12 @@ let errorText = ''; let status = ''; try { + const currentReplacement = + this.fixSuggestions[0].replacements[this.selectedReplacementIdx]; res = await this.restApiService.applyFixSuggestion( changeNum, patchNum, - this.fixSuggestions[0].replacements, + [currentReplacement], this.latestPatchNum ); } catch (error) {
diff --git a/proto/entities.proto b/proto/entities.proto index fbffa53..1cf4473 100644 --- a/proto/entities.proto +++ b/proto/entities.proto
@@ -120,7 +120,7 @@ message Conflicts { optional ObjectId ours = 1; optional ObjectId theirs = 2; - required bool containsConflicts = 3; + optional bool containsConflicts = 3; } // Serialized form of com.google.gerrit.extensions.common.MergeInput.