dsmiley commented on code in PR #3498:
URL: https://github.com/apache/solr/pull/3498#discussion_r2293771575
##########
solr/core/src/java/org/apache/solr/handler/component/IterativeMergeStrategy.java:
##########
@@ -85,20 +76,16 @@ public boolean handlesMergeFields() {
public void handleMergeFields(ResponseBuilder rb, SolrIndexSearcher
searcher) {}
public class CallBack implements Callable<CallBack> {
- private SolrClient solrClient;
+ private final String shardBaseUrl;
+ private final String shardCoreName;
+
private QueryRequest req;
private QueryResponse response;
private ShardResponse originalShardResponse;
public CallBack(ShardResponse originalShardResponse, QueryRequest req) {
- final String shardBaseUrl =
URLUtil.extractBaseUrl(originalShardResponse.getShardAddress());
- final String shardCoreName =
-
URLUtil.extractCoreFromCoreUrl(originalShardResponse.getShardAddress());
- this.solrClient =
- new Builder(shardBaseUrl)
Review Comment:
don't need to make a new client. we can use
`httpSolrClient.requestWithBaseUrl`
##########
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java:
##########
@@ -557,15 +513,16 @@ private String generateTokenV2(String user) {
return s + " " + base64Signature;
}
- void setHeader(HttpRequest httpRequest) {
+ @VisibleForTesting
+ void setHeader(BiConsumer<String, String> httpRequest) {
Review Comment:
Due to a test, this needed to stay but I made it generic so we don't have a
compile dependency on Apache HttpClient classes (HttpRequest).
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -605,58 +596,10 @@ private synchronized void initializeAuthenticationPlugin(
}
private void setupHttpClientForAuthPlugin(Object authcPlugin) {
- if (authcPlugin instanceof HttpClientBuilderPlugin builderPlugin) {
Review Comment:
this stuff was all tightly linked to org.apache.http, not Jetty.
##########
solr/core/src/java/org/apache/solr/handler/component/IterativeMergeStrategy.java:
##########
@@ -37,31 +33,26 @@
import org.apache.solr.common.util.SolrNamedThreadFactory;
import org.apache.solr.common.util.URLUtil;
import org.apache.solr.search.SolrIndexSearcher;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
public abstract class IterativeMergeStrategy implements MergeStrategy {
protected volatile ExecutorService executorService;
- protected volatile CloseableHttpClient httpClient;
Review Comment:
This was the last remaining real reference to an Apache HttpClient
##########
solr/core/src/java/org/apache/solr/security/SimplePrincipal.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.security;
+
+import java.security.Principal;
+import java.util.Objects;
+
+/** A basic name-only {@link Principal}. */
+public final class SimplePrincipal implements Principal {
+ // TODO use a record. But javadoc tooling complained as it was confused
(Java version issue?)
Review Comment:
I was so hoping to use a record and then got stuck with the javadoc tool
saying it didn't know what to do with it... which was weird since it was using
JDK 21
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java:
##########
@@ -196,7 +196,7 @@ public void testSanityCheckAuth() throws Exception {
assertEquals(
"sanity check of update to X from read only user",
- 500, // should be 403, but CloudSolrClient lies on updates for now:
SOLR-14222
Review Comment:
It was a happy accident that SOLR-14222 can be indirectly resolved due to
this issue (somehow). I have no idea why, honestly.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
