madrob commented on a change in pull request #585:
URL: https://github.com/apache/solr/pull/585#discussion_r807527032
##########
File path:
solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
##########
@@ -246,7 +266,7 @@ private void doRandomAccessTest(String content, int
position) throws Exception {
try (S3BackupRepository repo = getRepository()) {
// Open an index input on a file
- pushObject("/my-repo/content", content);
+ pushObject("my-repo/content", content);
Review comment:
@HoustonPutman I recall you fighting with S3 and Mock-S3 and how it
handled directories...
##########
File path: gradle/ant-compat/force-versions.gradle
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.
+ */
+
+
+// Force versions of certain components to align them with ant build.
Review comment:
Can you add more comments explaining why we need this, since earlier we
felt like it would have been safe to remove?
##########
File path:
solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java
##########
@@ -91,7 +91,7 @@ public void testDirectoryLength() throws Exception {
"Getting length on a dir should throw exception",
S3Exception.class,
() -> client.length("/directory"));
- assertEquals("Path is Directory", exception.getMessage());
+ assertTrue(exception.getMessage(), exception.getMessage().contains("Path
is Directory"));
Review comment:
nit: `assertThat` would get us a nice error message for free
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/util/SolrBasicAuthentication.java
##########
@@ -35,7 +35,7 @@
private final String value;
public SolrBasicAuthentication(String user, String password) {
- this.value = "Basic " + B64Code.encode(user + ":" + password,
StandardCharsets.ISO_8859_1);
+ this.value = "Basic " + Base64.getEncoder().encodeToString((user + ":" +
password + "").getBytes(StandardCharsets.ISO_8859_1));
Review comment:
nit: extra `+ ""` in here
##########
File path: solr/core/src/java/org/apache/solr/request/json/RequestUtil.java
##########
@@ -68,8 +69,12 @@ public static void processParams(SolrRequestHandler handler,
SolrQueryRequest re
String[] jsonFromParams = map.remove(JSON); // params from the query
string should come after (and hence override) JSON content streams
for (ContentStream cs : req.getContentStreams()) {
+ // if BinaryResponseParser.BINARY_CONTENT_TYPE, let the following fail
below - we may have adjusted the content without updating the content type
+ // problem in this case happens in a few tests, one seems to happen
with kerberos and remote node query (HttpSolrCall's request proxy)
+
String contentType = cs.getContentType();
- if (contentType==null || !contentType.contains("/json")) {
+ if (contentType==null || (!contentType.contains("/json") &&
!contentType.contains(
+ BinaryResponseParser.BINARY_CONTENT_TYPE))) {
Review comment:
I saw that you switched a bunch of places away from BINARY_CONTENT_TYPE
to multi-part -- does Jetty 10 not like octet-stream? What's the motivation?
##########
File path:
solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
##########
@@ -320,17 +340,113 @@ protected URI getBaseUri() throws URISyntaxException {
}
private void pushObject(String path, String content) {
- try (S3Client s3 = S3_MOCK_RULE.createS3ClientV2()) {
+ try (S3Client s3 = createS3ClientV2()) {
s3.putObject(b -> b.bucket(BUCKET_NAME).key(path),
RequestBody.fromString(content));
}
}
private File pullObject(String path) throws IOException {
- try (S3Client s3 = S3_MOCK_RULE.createS3ClientV2()) {
+ try (S3Client s3 = createS3ClientV2()) {
File file = temporaryFolder.newFile();
InputStream input = s3.getObject(b -> b.bucket(BUCKET_NAME).key(path));
FileUtils.copyInputStreamToFile(input, file);
return file;
}
}
+
+ public S3Client createS3ClientV2() {
+ return S3Client.builder()
+ .region(Region.of("us-east-1"))
+ .credentialsProvider(
+ StaticCredentialsProvider.create(AwsBasicCredentials.create("foo",
"bar")))
+ .endpointOverride(URI.create(S3_MOCK_RULE.getServiceEndpoint()))
+ .httpClient(
+ UrlConnectionHttpClient.builder()
+ .buildWithDefaults(
+ AttributeMap.builder()
+ .put(
+ new HttpConfigurationOption("secureConnection",
Boolean.class),
+ Boolean.FALSE)
+ .build()))
+ .build();
+ }
+
+ public ClientConfiguration configureClientToIgnoreInvalidSslCertificates(
+ final ClientConfiguration clientConfiguration) {
+
+ clientConfiguration
+ .getApacheHttpClientConfig()
+ .withSslSocketFactory(
+ new SSLConnectionSocketFactory(
+ createBlindlyTrustingSslContext(),
NoopHostnameVerifier.INSTANCE));
+
+ return clientConfiguration;
+ }
+
+ private SSLContext createBlindlyTrustingSslContext() {
+ try {
+ final SSLContext sc = SSLContext.getInstance("TLS");
+
+ sc.init(
+ null,
+ new TrustManager[] {
+ new X509ExtendedTrustManager() {
+ @Override
+ public java.security.cert.X509Certificate[] getAcceptedIssuers()
{
+ return null;
+ }
+
+ @Override
+ public void checkClientTrusted(
+ final X509Certificate[] arg0, final String arg1, final
Socket arg2) {
+ // no-op
+ }
+
+ @Override
+ public void checkClientTrusted(
+ final X509Certificate[] arg0, final String arg1, final
SSLEngine arg2) {
+ // no-op
+ }
+
+ @Override
+ public void checkClientTrusted(final X509Certificate[] certs,
final String authType) {
+ // no-op
+ }
+
+ @Override
+ public void checkServerTrusted(final X509Certificate[] certs,
final String authType) {
+ // no-op
+ }
+
+ @Override
+ public void checkServerTrusted(
+ final X509Certificate[] arg0, final String arg1, final
Socket arg2) {
+ // no-op
+ }
+
+ @Override
+ public void checkServerTrusted(
+ final X509Certificate[] arg0, final String arg1, final
SSLEngine arg2) {
+ // no-op
+ }
+ }
+ },
+ new java.security.SecureRandom());
Review comment:
Should we use NotSecurePseudoRandom here?
##########
File path:
solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
##########
@@ -320,17 +340,113 @@ protected URI getBaseUri() throws URISyntaxException {
}
private void pushObject(String path, String content) {
- try (S3Client s3 = S3_MOCK_RULE.createS3ClientV2()) {
+ try (S3Client s3 = createS3ClientV2()) {
s3.putObject(b -> b.bucket(BUCKET_NAME).key(path),
RequestBody.fromString(content));
}
}
private File pullObject(String path) throws IOException {
- try (S3Client s3 = S3_MOCK_RULE.createS3ClientV2()) {
+ try (S3Client s3 = createS3ClientV2()) {
File file = temporaryFolder.newFile();
InputStream input = s3.getObject(b -> b.bucket(BUCKET_NAME).key(path));
FileUtils.copyInputStreamToFile(input, file);
return file;
}
}
+
+ public S3Client createS3ClientV2() {
Review comment:
Is this copied directly from adobe's S3MockStarter? If so, I'd like to
see it in a separate file that we can properly attribute (and keep up to date
periodically as needed)
##########
File path:
solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java
##########
@@ -103,6 +104,11 @@ public void testCanDistinguishBetweenFilesAndDirectories()
throws Exception {
repo.createDirectory(nonEmptyDirUri);
addFile(repo, nestedFileUri);
+ String[] list = repo.listAll(repo.resolve(nonEmptyDirUri));
Review comment:
left over from debugging?
--
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
