dsmiley commented on code in PR #975:
URL: https://github.com/apache/solr/pull/975#discussion_r965280828
##########
solr/core/src/java/org/apache/solr/api/V2HttpCall.java:
##########
@@ -388,33 +335,65 @@ public CompositeApi add(Api api) {
}
}
+ // We don't rely on any of Jersey's authc/z features so we pass in this
empty context for
+ // all requests.
+ public static final SecurityContext DEFAULT_SECURITY_CONTEXT = new
SecurityContext() {
+ public boolean isUserInRole(String role) { return false; }
+ public boolean isSecure() { return false; }
+ public Principal getUserPrincipal() { return null; }
+ public String getAuthenticationScheme() { return null; }
+ };
+
+ private void invokeJerseyRequest(CoreContainer cores, SolrCore core,
ApplicationHandler jerseyHandler, SolrQueryResponse rsp) {
+ try {
+ final ContainerRequest containerRequest =
ContainerRequestUtils.createContainerRequest(req, response,
jerseyHandler.getConfiguration());
+
+ // Set properties that may be used by Jersey filters downstream
+ containerRequest.setProperty(SOLR_QUERY_REQUEST_KEY, solrReq);
+ containerRequest.setProperty(SOLR_QUERY_RESPONSE_KEY, rsp);
+ containerRequest.setProperty(RequestContextConstants.CORE_CONTAINER_KEY,
cores);
Review Comment:
Then let's add a comment to say these are *only* for keys into this map? Or
even better, call this class RequestContextKeys
##########
solr/core/src/java/org/apache/solr/api/V2HttpCall.java:
##########
@@ -462,9 +462,10 @@ private String computeEndpointPath() {
protected void writeResponse(
SolrQueryResponse solrRsp, QueryResponseWriter responseWriter, Method
reqMethod)
throws IOException {
- // JAX-RS has its own code that flushes out the Response to the relevant
output stream, so we no-op here if the
+ // JAX-RS has its own code that flushes out the Response to the relevant
output stream, so we
+ // no-op here if the
// request was already handled via JAX-RS
Review Comment:
reflow this (probably due to spotless)
##########
solr/core/src/test/org/apache/solr/handler/configsets/ListConfigSetsAPITest.java:
##########
@@ -0,0 +1,139 @@
+/*
+ * 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.handler.configsets;
+
+import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.List;
+import javax.inject.Singleton;
+import javax.ws.rs.core.Application;
+import javax.ws.rs.core.Response;
+import org.apache.solr.client.solrj.response.ConfigSetAdminResponse;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.core.ConfigSetService;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.api.V2ApiUtils;
+import org.apache.solr.jersey.CoreContainerFactory;
+import org.apache.solr.jersey.SolrJacksonMapper;
+import org.glassfish.hk2.utilities.binding.AbstractBinder;
+import org.glassfish.jersey.server.ResourceConfig;
+import org.glassfish.jersey.test.JerseyTest;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class ListConfigSetsAPITest extends JerseyTest {
+
+ private CoreContainer mockCoreContainer;
+
+ @BeforeClass
+ public static void ensureWorkingMockito() {
+ assumeWorkingMockito();
+ }
+
+ @Override
+ protected Application configure() {
+ resetMocks();
+ final ResourceConfig config = new ResourceConfig();
+ config.register(ListConfigSetsAPI.class);
+ config.register(SolrJacksonMapper.class);
+ config.register(
+ new AbstractBinder() {
+ @Override
+ protected void configure() {
+ bindFactory(new CoreContainerFactory(mockCoreContainer))
+ .to(CoreContainer.class)
+ .in(Singleton.class);
+ }
+ });
+
+ return config;
+ }
+
+ private void resetMocks() {
+ mockCoreContainer = mock(CoreContainer.class);
+ }
+
+ @Test
+ public void testSuccessfulListConfigsetsRaw() throws Exception {
+ final String expectedJson =
+
"{\"responseHeader\":{\"status\":0,\"QTime\":0},\"configSets\":[\"cs1\",\"cs2\"]}";
+ final ConfigSetService configSetService = mock(ConfigSetService.class);
+ when(mockCoreContainer.getConfigSetService()).thenReturn(configSetService);
+ when(configSetService.listConfigs()).thenReturn(List.of("cs1", "cs2"));
+
+ final Response response = target("/cluster/configs").request().get();
+ final String jsonBody = response.readEntity(String.class);
+
+ assertEquals(200, response.getStatus());
+ assertEquals("application/json",
response.getHeaders().getFirst("Content-type"));
+ assertEquals(1, 1);
+ assertEquals(
+ expectedJson,
+
"{\"responseHeader\":{\"status\":0,\"QTime\":0},\"configSets\":[\"cs1\",\"cs2\"]}");
+ }
+
+ @Test
+ public void testSuccessfulListConfigsetsTyped() throws Exception {
+ final ConfigSetService configSetService = mock(ConfigSetService.class);
+ when(mockCoreContainer.getConfigSetService()).thenReturn(configSetService);
+ when(configSetService.listConfigs()).thenReturn(List.of("cs1", "cs2"));
+
+ final ListConfigsetsResponse response =
+ target("/cluster/configs").request().get(ListConfigsetsResponse.class);
+
+ assertNotNull(response.configSets);
+ assertNull(response.error);
+ assertEquals(2, response.configSets.size());
+ assertTrue(response.configSets.contains("cs1"));
+ assertTrue(response.configSets.contains("cs2"));
+ }
+
+ /**
+ * Test the v2 to v1 response mapping for /cluster/configs
+ *
+ * <p>{@link org.apache.solr.handler.admin.ConfigSetsHandler} uses {@link
ListConfigSetsAPI} (and
+ * its response class {@link ListConfigsetsResponse}) internally to serve
the v1 version of this
+ * functionality. So it's important to make sure that the v2 response stays
compatible with SolrJ
+ * - both because that's important in its own right and because that ensures
we haven't
+ * accidentally changed the v1 response format.
+ */
+ @Test
+ public void testListConfigsetsV1Compatibility() throws Exception {
Review Comment:
I get the trade-offs! Tests have a cost and I'm much more weary than you
are of the quicksand/internals cost of testing Jersey specifics being a barrier
for change. I see this in other code bases too, e.g. testing low-level
token-filters _too much_ and not enough at the high level capturing the
effective impact. I don't mind debugging higher level tests because tests
_generally_ continue to pass forever until you do the one thing that breaks
something, which is a massive clue. Your second argument makes sense; let's
just not repeat this test approach in lots of places for all APIs, please. For
example, this test might be named to reflect this is a how-to or testing Jersey.
BTW when I said test XML/JSON, I should say, test via SolrJ (using Javabin)
is great too especially because of Java deficiencies around multi-line
string-literals (eventually solved in a later Java release). My preference is
generally at the SolrJ level when possible, not actually XML/JSON but these are
very close layers to test at (still fairly integration-y). SolrJ via
EmbeddedSolrServer is fast, skipping Jetty/HTTP.
##########
solr/core/src/java/org/apache/solr/jersey/CatchAllExceptionMapper.java:
##########
@@ -93,10 +98,12 @@ public Response toResponse(Exception exception) {
RequestHandlerBase.processErrorMetricsOnException(normalizedException,
metrics);
}
- // Then, convert the exception into a SolrJerseyResponse (creating one as
necessary if resource was matched, etc.)
- final SolrJerseyResponse response =
containerRequestContext.getProperty(SOLR_JERSEY_RESPONSE_KEY) == null ?
- new SolrJerseyResponse() :
- (SolrJerseyResponse)
containerRequestContext.getProperty(SOLR_JERSEY_RESPONSE_KEY);
+ // Then, convert the exception into a SolrJerseyResponse (creating one as
necessary if resource
+ // was matched, etc.)
Review Comment:
more reflow
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -16,12 +16,51 @@
*/
package org.apache.solr.core;
+import static java.util.Objects.requireNonNull;
Review Comment:
Apparently our import ordering is not yet standardized :-(
##########
solr/core/src/java/org/apache/solr/jersey/CatchAllExceptionMapper.java:
##########
@@ -52,26 +51,33 @@
public class CatchAllExceptionMapper implements ExceptionMapper<Exception> {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- @Context
- public ResourceContext resourceContext;
+ @Context public ResourceContext resourceContext;
@Override
public Response toResponse(Exception exception) {
- final ContainerRequestContext containerRequestContext =
resourceContext.getResource(ContainerRequestContext.class);
-
- // Set the exception on the SolrQueryResponse. Not to affect the actual
response, but as SolrDispatchFiler and
- // HttpSolrCall use the presence of an exception as a marker of
success/failure for AuditLogging, and other logic.
- final SolrQueryResponse solrQueryResponse = (SolrQueryResponse)
containerRequestContext.getProperty(SOLR_QUERY_RESPONSE_KEY);
+ final ContainerRequestContext containerRequestContext =
+ resourceContext.getResource(ContainerRequestContext.class);
+
+ // Set the exception on the SolrQueryResponse. Not to affect the actual
response, but as
+ // SolrDispatchFiler and
Review Comment:
more comment newline reflow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
