This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch branch_10_1 in repository https://gitbox.apache.org/repos/asf/solr.git
commit 83a65e6c4b2bfd33731531955d73bde1fb6a7a39 Author: Eric Pugh <[email protected]> AuthorDate: Wed Sep 30 12:54:43 2026 -0400 SOLR-18324: Fix NPE/RTimer assertion when a V2 request fails before a SolrQueryRequest is attached (#4676) Co-authored-by: Claude Sonnet 5 <[email protected]> (cherry picked from commit c060bee312bed71c52c23ac476ea28a70eba6ff2) (cherry picked from commit 91bf9bad3d2d6579f729b8f9507064ce95f7901d) --- .../SOLR-18324-jersey-null-request-npe.yml | 7 ++ .../apache/solr/handler/RequestHandlerBase.java | 5 +- .../solr/jersey/CatchAllExceptionMapper.java | 6 +- .../solr/jersey/PostRequestDecorationFilter.java | 4 + .../solr/jersey/PostRequestLoggingFilter.java | 4 +- .../apache/solr/jersey/RequestMetricHandling.java | 5 ++ .../solr/handler/RequestHandlerBaseTest.java | 21 ++++++ .../solr/jersey/RequestMetricHandlingTest.java | 87 ++++++++++++++++++++++ 8 files changed, 135 insertions(+), 4 deletions(-) diff --git a/changelog/unreleased/SOLR-18324-jersey-null-request-npe.yml b/changelog/unreleased/SOLR-18324-jersey-null-request-npe.yml new file mode 100644 index 00000000000..e853f4442da --- /dev/null +++ b/changelog/unreleased/SOLR-18324-jersey-null-request-npe.yml @@ -0,0 +1,7 @@ +title: The V2 JAX-RS API, when faced with an error, could throw an NPE masking the true problem. +type: fixed +authors: + - name: Eric Pugh +links: + - name: SOLR-18324 + url: https://issues.apache.org/jira/browse/SOLR-18324 diff --git a/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java b/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java index e7322360ea5..5f59eae2f78 100644 --- a/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java +++ b/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java @@ -321,9 +321,12 @@ public abstract class RequestHandlerBase * <p>If a tragic exception occurred in the index writer, this method also gives up leadership of * the shard, and replaces the index writer with a new one to attempt to get out of a transient * failure (e.g. disk failure). + * + * <p>{@code req} may be null, e.g. when called from Jersey's {@code CatchAllExceptionMapper} for + * a request that failed before a {@link SolrQueryRequest} was attached to the request context. */ public static Exception processReceivedException(SolrQueryRequest req, Exception e) { - SolrCore core = req.getCore(); + SolrCore core = req == null ? null : req.getCore(); if (core != null) { CoreContainer coreContainer = req.getCoreContainer(); assert coreContainer != null; diff --git a/solr/core/src/java/org/apache/solr/jersey/CatchAllExceptionMapper.java b/solr/core/src/java/org/apache/solr/jersey/CatchAllExceptionMapper.java index 506aa0670ac..8302235ecf8 100644 --- a/solr/core/src/java/org/apache/solr/jersey/CatchAllExceptionMapper.java +++ b/solr/core/src/java/org/apache/solr/jersey/CatchAllExceptionMapper.java @@ -117,8 +117,10 @@ public class CatchAllExceptionMapper implements ExceptionMapper<Exception> { shouldHideStackTrace(solrQueryRequest, containerRequestContext)); response.responseHeader.status = response.error.code; final String mediaType = - V2ApiUtils.getMediaTypeFromWtParam( - solrQueryRequest.getParams(), MediaType.APPLICATION_JSON); + solrQueryRequest == null + ? MediaType.APPLICATION_JSON + : V2ApiUtils.getMediaTypeFromWtParam( + solrQueryRequest.getParams(), MediaType.APPLICATION_JSON); return Response.status(response.error.code).type(mediaType).entity(response).build(); } diff --git a/solr/core/src/java/org/apache/solr/jersey/PostRequestDecorationFilter.java b/solr/core/src/java/org/apache/solr/jersey/PostRequestDecorationFilter.java index ea48aeba31d..5fb87de04af 100644 --- a/solr/core/src/java/org/apache/solr/jersey/PostRequestDecorationFilter.java +++ b/solr/core/src/java/org/apache/solr/jersey/PostRequestDecorationFilter.java @@ -55,6 +55,10 @@ public class PostRequestDecorationFilter implements ContainerResponseFilter { } final SolrQueryRequest solrQueryRequest = (SolrQueryRequest) requestContext.getProperty(SOLR_QUERY_REQUEST); + if (solrQueryRequest == null) { + log.trace("Skipping QTime assignment because no SolrQueryRequest was attached"); + return; + } if (!responseContext.hasEntity() || !(responseContext.getEntity() instanceof SolrJerseyResponse)) { log.debug("Skipping QTime assignment because response was not a SolrJerseyResponse"); diff --git a/solr/core/src/java/org/apache/solr/jersey/PostRequestLoggingFilter.java b/solr/core/src/java/org/apache/solr/jersey/PostRequestLoggingFilter.java index b9bed2f8176..a6e798b97d0 100644 --- a/solr/core/src/java/org/apache/solr/jersey/PostRequestLoggingFilter.java +++ b/solr/core/src/java/org/apache/solr/jersey/PostRequestLoggingFilter.java @@ -86,7 +86,9 @@ public class PostRequestLoggingFilter implements ContainerResponseFilter { final SolrQueryRequest solrQueryRequest = (SolrQueryRequest) requestContext.getProperty(SOLR_QUERY_REQUEST); final var solrConfig = - (solrQueryRequest.getCore() != null) ? solrQueryRequest.getCore().getSolrConfig() : null; + (solrQueryRequest != null && solrQueryRequest.getCore() != null) + ? solrQueryRequest.getCore().getSolrConfig() + : null; final Logger requestLogger = (solrConfig != null) ? coreRequestLogger : nonCoreRequestLogger; final String templatedPath = diff --git a/solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java b/solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java index 533f56cd1a3..9dc4fd2934c 100644 --- a/solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java +++ b/solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java @@ -121,8 +121,13 @@ public class RequestMetricHandling { } else { log.debug("Skipping partialResults check because entity was not SolrJerseyResponse"); } + // Jersey can re-invoke response filters a second time when an exception occurs while + // building the first response (e.g. via CatchAllExceptionMapper), so guard against + // double-stopping the same timer. final var timer = (AttributedLongTimer.MetricTimer) requestContext.getProperty(TIMER); + if (timer == null) return; timer.stop(); + requestContext.setProperty(TIMER, null); } } } diff --git a/solr/core/src/test/org/apache/solr/handler/RequestHandlerBaseTest.java b/solr/core/src/test/org/apache/solr/handler/RequestHandlerBaseTest.java index 137879f40e7..fe430c5907d 100644 --- a/solr/core/src/test/org/apache/solr/handler/RequestHandlerBaseTest.java +++ b/solr/core/src/test/org/apache/solr/handler/RequestHandlerBaseTest.java @@ -160,6 +160,27 @@ public class RequestHandlerBaseTest extends SolrTestCaseJ4 { assertEquals(SolrException.ErrorCode.SERVER_ERROR.code, normalizedSolrException.code()); } + @Test + public void testNullRequestDoesNotThrowAndIsNotModified() { + // req can be null when this is called from Jersey's CatchAllExceptionMapper for a request + // that failed before a SolrQueryRequest was attached to the request context. + final Exception e = new RuntimeException("Some generic, non-SolrException"); + + final Exception normalized = RequestHandlerBase.processReceivedException(null, e); + + assertSame(e, normalized); + } + + @Test + public void testNullRequestWithSyntaxErrorIsStillWrappedIn400SolrException() { + final Exception e = new SyntaxError("Some syntax error"); + + final Exception normalized = RequestHandlerBase.processReceivedException(null, e); + + assertEquals(SolrException.class, normalized.getClass()); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ((SolrException) normalized).code()); + } + @Test public void testIsInternalShardRequest() { final SolrQueryRequest solrQueryRequest = diff --git a/solr/core/src/test/org/apache/solr/jersey/RequestMetricHandlingTest.java b/solr/core/src/test/org/apache/solr/jersey/RequestMetricHandlingTest.java new file mode 100644 index 00000000000..df5bd6ac212 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/jersey/RequestMetricHandlingTest.java @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.solr.jersey; + +import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito; +import static org.apache.solr.jersey.RequestContextKeys.HANDLER_METRICS; +import static org.apache.solr.jersey.RequestContextKeys.TIMER; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.metrics.LongCounter; +import io.opentelemetry.api.metrics.LongHistogram; +import jakarta.ws.rs.container.ContainerRequestContext; +import jakarta.ws.rs.container.ContainerResponseContext; +import java.util.Set; +import org.apache.solr.SolrTestCase; +import org.apache.solr.handler.RequestHandlerBase; +import org.apache.solr.metrics.SolrMetricsContext; +import org.junit.BeforeClass; +import org.junit.Test; + +/** Unit tests for {@link RequestMetricHandling} */ +public class RequestMetricHandlingTest extends SolrTestCase { + + @BeforeClass + public static void ensureWorkingMockito() { + assumeWorkingMockito(); + } + + @Test + public void testPostRequestMetricsFilterToleratesBeingInvokedTwice() throws Exception { + // Jersey can re-invoke response filters a second time when an exception occurs while + // building the first response (e.g. via CatchAllExceptionMapper); the second invocation must + // not try to stop an already-stopped timer. + final var mockRequestContext = mock(ContainerRequestContext.class); + final var mockResponseContext = mock(ContainerResponseContext.class); + final RequestHandlerBase.HandlerMetrics metrics = createHandlerMetrics(); + final var timer = metrics.requestTimes.start(); + + when(mockRequestContext.getPropertyNames()).thenReturn(Set.of()); + when(mockRequestContext.getProperty(HANDLER_METRICS)).thenReturn(metrics); + when(mockRequestContext.getProperty(TIMER)).thenReturn(timer); + + final var filter = new RequestMetricHandling.PostRequestMetricsFilter(); + filter.filter(mockRequestContext, mockResponseContext); + + // Simulate Jersey clearing the property after the first stop, as our fix does. + when(mockRequestContext.getProperty(TIMER)).thenReturn(null); + + // A second invocation must not throw (would previously throw AssertionError with + // assertions enabled, from RTimer.stop() being called on an already-stopped timer). + filter.filter(mockRequestContext, mockResponseContext); + } + + private RequestHandlerBase.HandlerMetrics createHandlerMetrics() { + final SolrMetricsContext metricsContext = mock(SolrMetricsContext.class); + final LongCounter mockLongCounter = mock(LongCounter.class); + final LongHistogram mockLongHistogram = mock(LongHistogram.class); + + when(metricsContext.getRegistryName()).thenReturn("solr.core"); + when(metricsContext.longCounter(any(), any())).thenReturn(mockLongCounter); + when(metricsContext.longCounter(any(), any(), any())).thenReturn(mockLongCounter); + when(metricsContext.longHistogram(any(), any())).thenReturn(mockLongHistogram); + when(metricsContext.longHistogram(any(), any(), any())).thenReturn(mockLongHistogram); + + return new RequestHandlerBase.HandlerMetrics( + metricsContext, Attributes.of(AttributeKey.stringKey("source"), "test"), false); + } +}
