dsmiley commented on code in PR #146:
URL: https://github.com/apache/solr/pull/146#discussion_r1035241026
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -16,35 +16,54 @@
*/
package org.apache.solr.core;
+import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
+
import java.io.FileNotFoundException;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
+import java.nio.file.FileVisitResult;
import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.Utils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Solr Standalone File System ConfigSetService impl.
+ * Standalone File System ConfigSetService impl.
*
- * <p>Loads a ConfigSet defined by the core's configSet property, looking for
a directory named for
+ * <p>Loads ConfigSet defined by the core's configSet property, looking for a
directory named for
* the configSet property value underneath a base directory. If no configSet
property is set, loads
* the ConfigSet instead from the core's instance directory.
*/
public class FileSystemConfigSetService extends ConfigSetService {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private final Path configSetBase;
+ /** .metadata.json hidden file where metadata is stored */
+ public static final String METADATA_FILE = ".metadata.json";
Review Comment:
Always keep static-final (constants) stuff at the top, above other fields.
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -148,4 +285,8 @@ protected Long getCurrentSchemaModificationVersion(
return null; // debatable; we'll see an error soon if there's a real
problem
}
}
+
+ private Path getConfigDir(String configName) {
+ return configSetBase.toAbsolutePath().resolve(configName);
Review Comment:
Why call toAbsolutePath ?
##########
solr/core/src/test/org/apache/solr/core/TestConfigSetService.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.core;
+
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Supplier;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.ZkConfigSetService;
+import org.apache.solr.cloud.ZkTestServer;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class TestConfigSetService extends SolrTestCaseJ4 {
+
+ private final ConfigSetService configSetService;
+ private static ZkTestServer zkServer;
+ private static SolrZkClient zkClient;
+
+ @BeforeClass
+ public static void startZkServer() throws Exception {
+ zkServer = new ZkTestServer(createTempDir("zkData"));
+ zkServer.run();
+ zkClient = new SolrZkClient(zkServer.getZkAddress("/solr"), 10000);
+ }
+
+ @AfterClass
+ public static void shutdownZkServer() throws IOException,
InterruptedException {
+ zkClient.close();
+ if (null != zkServer) {
+ zkServer.shutdown();
+ }
+ zkServer = null;
+ }
+
+ public TestConfigSetService(Supplier<ConfigSetService> configSetService) {
+ this.configSetService = configSetService.get();
+ }
+
+ @ParametersFactory
+ @SuppressWarnings("rawtypes")
+ public static Iterable<Supplier[]> parameters() {
+ return Arrays.asList(
+ new Supplier[][] {
+ {() -> new ZkConfigSetService(zkClient)},
+ {() -> new FileSystemConfigSetService(createTempDir("configsets"))}
+ });
+ }
+
+ @Test
+ public void testConfigSetServiceOperations() throws IOException {
+ final String configName = "testconfig";
+ byte[] testdata = "test data".getBytes(StandardCharsets.UTF_8);
+
+ Path configDir = createTempDir("testconfig");
+ Files.write(configDir.resolve("file1"), testdata);
+ Files.createFile(configDir.resolve("file2"));
+ Files.createDirectory(configDir.resolve("subdir"));
+ Files.createFile(configDir.resolve("subdir").resolve("file3"));
+
+ configSetService.uploadConfig(configName, configDir);
+
+ assertTrue(configSetService.checkConfigExists(configName));
+ assertFalse(configSetService.checkConfigExists("dummyConfig"));
+
+ byte[] data = "file3 data".getBytes(StandardCharsets.UTF_8);
+ configSetService.uploadFileToConfig(configName, "subdir/file3", data,
true);
+ assertArrayEquals(configSetService.downloadFileFromConfig(configName,
"subdir/file3"), data);
+
+ data = "file4 data".getBytes(StandardCharsets.UTF_8);
+ configSetService.uploadFileToConfig(configName, "subdir/file4", data,
true);
+ assertArrayEquals(configSetService.downloadFileFromConfig(configName,
"subdir/file4"), data);
+
+ Map<String, Object> metadata =
configSetService.getConfigMetadata(configName);
+ assertTrue(metadata.isEmpty());
+ metadata = configSetService.getConfigMetadata("dummyConfig");
+ assertTrue(metadata.isEmpty());
+
+ configSetService.setConfigMetadata(configName,
Collections.singletonMap("trusted", true));
+ metadata = configSetService.getConfigMetadata(configName);
+ assertTrue(metadata.containsKey("trusted"));
+
+ configSetService.setConfigMetadata(configName,
Collections.singletonMap("foo", true));
+
assertFalse(configSetService.getConfigMetadata(configName).containsKey("trusted"));
+
assertTrue(configSetService.getConfigMetadata(configName).containsKey("foo"));
+
+ List<String> configFiles = configSetService.getAllConfigFiles(configName);
+ assertTrue(configFiles.size() == 5);
Review Comment:
can you assert the list contents so we can see the expected strings? I ask
because this method is somewhat quirky / specific about the response, e.g.
sorted & has intermediate dirs
##########
solr/core/src/test/org/apache/solr/core/TestFileSystemConfigSetService.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.core;
+
+import static org.apache.solr.core.FileSystemConfigSetService.METADATA_FILE;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.io.file.PathUtils;
+import org.apache.solr.SolrTestCaseJ4;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class TestFileSystemConfigSetService extends SolrTestCaseJ4 {
+ private static Path configSetBase;
+ private static FileSystemConfigSetService fileSystemConfigSetService;
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ configSetBase = createTempDir();
+ fileSystemConfigSetService = new FileSystemConfigSetService(configSetBase);
+ }
+
+ @AfterClass
+ public static void afterClass() throws Exception {
+ PathUtils.deleteDirectory(configSetBase);
+ fileSystemConfigSetService = null;
+ }
+
+ @Test
+ public void testUploadConfig() throws IOException {
Review Comment:
Please add a test that shows we can neither upload nor download files
outside of the base directory. For example, if the configName supplied were to
start with a "/" or "../" in it, then I think it could be nefariously hacked,
in a sense. Based on my code inspection as indicated in my old review of
FileSystemConfigSetService, I think it's vulnerable.
Note that FileSystemConfigSetService.locateInstanceDir should not be
sandboxed to `configSetBase` because the input (a CoreDescriptor) is written by
a dev/admin type person that could easily deliberately want some path of their
choosing. But other methods could come from an HTTP (external) API where we
don't trust the inputs as much.
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -16,35 +16,54 @@
*/
package org.apache.solr.core;
+import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
+
import java.io.FileNotFoundException;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
+import java.nio.file.FileVisitResult;
import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.Utils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Solr Standalone File System ConfigSetService impl.
+ * Standalone File System ConfigSetService impl.
Review Comment:
Let's remove "Standalone" from here. A user can use this in SolrCloud if
they wish.
Speaking of which, see if the Solr Ref Guide mentions that this is a viable
option (use in SolrCloud mode), especially for immutable configSets but that
isn't enforced.
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -16,35 +16,54 @@
*/
package org.apache.solr.core;
+import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
+
import java.io.FileNotFoundException;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
+import java.nio.file.FileVisitResult;
import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.Utils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Solr Standalone File System ConfigSetService impl.
+ * Standalone File System ConfigSetService impl.
*
- * <p>Loads a ConfigSet defined by the core's configSet property, looking for
a directory named for
+ * <p>Loads ConfigSet defined by the core's configSet property, looking for a
directory named for
Review Comment:
"a" was appropriate.
##########
solr/core/src/test/org/apache/solr/core/TestConfigSetService.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.core;
+
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Supplier;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.ZkConfigSetService;
+import org.apache.solr.cloud.ZkTestServer;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class TestConfigSetService extends SolrTestCaseJ4 {
+
+ private final ConfigSetService configSetService;
+ private static ZkTestServer zkServer;
+ private static SolrZkClient zkClient;
+
+ @BeforeClass
+ public static void startZkServer() throws Exception {
+ zkServer = new ZkTestServer(createTempDir("zkData"));
+ zkServer.run();
+ zkClient = new SolrZkClient(zkServer.getZkAddress("/solr"), 10000);
+ }
+
+ @AfterClass
+ public static void shutdownZkServer() throws IOException,
InterruptedException {
+ zkClient.close();
+ if (null != zkServer) {
+ zkServer.shutdown();
+ }
+ zkServer = null;
+ }
+
+ public TestConfigSetService(Supplier<ConfigSetService> configSetService) {
+ this.configSetService = configSetService.get();
+ }
+
+ @ParametersFactory
+ @SuppressWarnings("rawtypes")
+ public static Iterable<Supplier[]> parameters() {
+ return Arrays.asList(
+ new Supplier[][] {
+ {() -> new ZkConfigSetService(zkClient)},
+ {() -> new FileSystemConfigSetService(createTempDir("configsets"))}
+ });
+ }
+
+ @Test
+ public void testConfigSetServiceOperations() throws IOException {
Review Comment:
awesome generic test
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -60,74 +79,192 @@ public String configSetName(CoreDescriptor cd) {
@Override
public boolean checkConfigExists(String configName) throws IOException {
- Path solrConfigXmlFile =
configSetBase.resolve(configName).resolve("solrconfig.xml");
- return Files.exists(solrConfigXmlFile);
+ return Files.isDirectory(getConfigDir(configName));
}
@Override
public void deleteConfig(String configName) throws IOException {
- throw new UnsupportedOperationException();
+ deleteDir(getConfigDir(configName));
}
@Override
public void deleteFilesFromConfig(String configName, List<String>
filesToDelete)
throws IOException {
- throw new UnsupportedOperationException();
+ Path configDir = getConfigDir(configName);
+ Objects.requireNonNull(filesToDelete);
+ for (String fileName : filesToDelete) {
+ Path file = configDir.resolve(fileName);
+ if (Files.exists(file)) {
+ if (Files.isDirectory(file)) {
+ deleteDir(file);
+ } else {
+ Files.delete(file);
+ }
+ }
+ }
}
@Override
public void copyConfig(String fromConfig, String toConfig) throws
IOException {
- throw new UnsupportedOperationException();
+ Path source = getConfigDir(fromConfig);
+ Path dest = getConfigDir(toConfig);
+ copyRecursively(source, dest);
+ }
+
+ private void deleteDir(Path dir) throws IOException {
+ try {
+ Files.walkFileTree(
+ dir,
+ new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path path, BasicFileAttributes
attrs)
+ throws IOException {
+ Files.delete(path);
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult postVisitDirectory(Path dir, IOException
ioException)
+ throws IOException {
+ Files.delete(dir);
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ } catch (NoSuchFileException e) {
+ // do nothing
+ }
}
@Override
- public void uploadConfig(String configName, Path dir) throws IOException {
- throw new UnsupportedOperationException();
+ public void uploadConfig(String configName, Path source) throws IOException {
+ Path dest = getConfigDir(configName);
+ copyRecursively(source, dest);
}
@Override
public void uploadFileToConfig(
String configName, String fileName, byte[] data, boolean
overwriteOnExists)
throws IOException {
- throw new UnsupportedOperationException();
+ Path filePath = getConfigDir(configName).resolve(fileName);
+ if (!Files.exists(filePath) || overwriteOnExists) {
+ Files.write(filePath, data);
+ }
}
@Override
public void setConfigMetadata(String configName, Map<String, Object> data)
throws IOException {
- throw new UnsupportedOperationException();
+ // store metadata in .metadata.json file
+ Path metadataPath = getConfigDir(configName).resolve(METADATA_FILE);
+ Files.write(metadataPath, Utils.toJSON(data));
+ Files.isHidden(metadataPath);
}
@Override
public Map<String, Object> getConfigMetadata(String configName) throws
IOException {
- throw new UnsupportedOperationException();
+ // get metadata from .metadata.json file
+ Path metadataPath = getConfigDir(configName).resolve(METADATA_FILE);
+ byte[] data = null;
+ try {
+ data = Files.readAllBytes(metadataPath);
+ } catch (NoSuchFileException e) {
+ return new HashMap<>();
+ }
+ if (data == null) {
+ return new HashMap<>();
+ }
+ @SuppressWarnings("unchecked")
+ Map<String, Object> metadata = (Map<String, Object>) Utils.fromJSON(data);
+ return metadata;
}
@Override
- public void downloadConfig(String configName, Path dir) throws IOException {
- throw new UnsupportedOperationException();
+ public void downloadConfig(String configName, Path dest) throws IOException {
+ Path source = getConfigDir(configName);
+ copyRecursively(source, dest);
+ }
+
+ private void copyRecursively(Path source, Path target) throws IOException {
+ try {
+ Files.walkFileTree(
+ source,
+ new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult preVisitDirectory(Path dir,
BasicFileAttributes attrs)
+ throws IOException {
+
Files.createDirectories(target.resolve(source.relativize(dir).toString()));
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes
attrs)
+ throws IOException {
+ Files.copy(
+ file, target.resolve(source.relativize(file).toString()),
REPLACE_EXISTING);
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ } catch (NoSuchFileException e) {
+ // do nothing
+ }
}
@Override
public List<String> listConfigs() throws IOException {
try (Stream<Path> configs = Files.list(configSetBase)) {
- return
configs.map(Path::getFileName).map(Path::toString).collect(Collectors.toList());
+ return configs
+ .map(Path::getFileName)
+ .map(Path::toString)
+ .sorted()
+ .collect(Collectors.toList());
}
}
@Override
- public byte[] downloadFileFromConfig(String configName, String filePath)
throws IOException {
- throw new UnsupportedOperationException();
+ public byte[] downloadFileFromConfig(String configName, String fileName)
throws IOException {
+ Path filePath = getConfigDir(configName).resolve(fileName);
+ byte[] data = null;
+ try {
+ data = Files.readAllBytes(filePath);
+ } catch (NoSuchFileException e) {
+ // do nothing
+ }
+ return data;
}
@Override
public List<String> getAllConfigFiles(String configName) throws IOException {
- throw new UnsupportedOperationException();
+ Path configDir = getConfigDir(configName);
+ List<String> filePaths = new ArrayList<>();
+ Files.walkFileTree(
+ configDir,
+ new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes
attrs)
+ throws IOException {
+ // don't include hidden (.) files
+ if (!Files.isHidden(file)) {
+ filePaths.add(configDir.relativize(file).toString());
+ return FileVisitResult.CONTINUE;
+ }
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult postVisitDirectory(Path dir, IOException
ioException) {
+ if (!dir.getFileName().toString().equals(configName)) {
Review Comment:
if the intermediate directory had a name that coincidentally was the same as
that of the configSet, this would not do what we want. I think you could skip
this check and instead check that the relatived-dir string path is not empty?
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -60,74 +79,192 @@ public String configSetName(CoreDescriptor cd) {
@Override
public boolean checkConfigExists(String configName) throws IOException {
- Path solrConfigXmlFile =
configSetBase.resolve(configName).resolve("solrconfig.xml");
- return Files.exists(solrConfigXmlFile);
+ return Files.isDirectory(getConfigDir(configName));
}
@Override
public void deleteConfig(String configName) throws IOException {
- throw new UnsupportedOperationException();
+ deleteDir(getConfigDir(configName));
}
@Override
public void deleteFilesFromConfig(String configName, List<String>
filesToDelete)
throws IOException {
- throw new UnsupportedOperationException();
+ Path configDir = getConfigDir(configName);
+ Objects.requireNonNull(filesToDelete);
+ for (String fileName : filesToDelete) {
+ Path file = configDir.resolve(fileName);
+ if (Files.exists(file)) {
+ if (Files.isDirectory(file)) {
+ deleteDir(file);
+ } else {
+ Files.delete(file);
+ }
+ }
+ }
}
@Override
public void copyConfig(String fromConfig, String toConfig) throws
IOException {
- throw new UnsupportedOperationException();
+ Path source = getConfigDir(fromConfig);
+ Path dest = getConfigDir(toConfig);
+ copyRecursively(source, dest);
+ }
+
+ private void deleteDir(Path dir) throws IOException {
+ try {
+ Files.walkFileTree(
+ dir,
+ new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path path, BasicFileAttributes
attrs)
+ throws IOException {
+ Files.delete(path);
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult postVisitDirectory(Path dir, IOException
ioException)
+ throws IOException {
+ Files.delete(dir);
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ } catch (NoSuchFileException e) {
+ // do nothing
+ }
}
@Override
- public void uploadConfig(String configName, Path dir) throws IOException {
- throw new UnsupportedOperationException();
+ public void uploadConfig(String configName, Path source) throws IOException {
+ Path dest = getConfigDir(configName);
+ copyRecursively(source, dest);
}
@Override
public void uploadFileToConfig(
String configName, String fileName, byte[] data, boolean
overwriteOnExists)
throws IOException {
- throw new UnsupportedOperationException();
+ Path filePath = getConfigDir(configName).resolve(fileName);
+ if (!Files.exists(filePath) || overwriteOnExists) {
+ Files.write(filePath, data);
+ }
}
@Override
public void setConfigMetadata(String configName, Map<String, Object> data)
throws IOException {
- throw new UnsupportedOperationException();
+ // store metadata in .metadata.json file
+ Path metadataPath = getConfigDir(configName).resolve(METADATA_FILE);
+ Files.write(metadataPath, Utils.toJSON(data));
+ Files.isHidden(metadataPath);
}
@Override
public Map<String, Object> getConfigMetadata(String configName) throws
IOException {
- throw new UnsupportedOperationException();
+ // get metadata from .metadata.json file
+ Path metadataPath = getConfigDir(configName).resolve(METADATA_FILE);
+ byte[] data = null;
+ try {
+ data = Files.readAllBytes(metadataPath);
+ } catch (NoSuchFileException e) {
+ return new HashMap<>();
+ }
+ if (data == null) {
+ return new HashMap<>();
+ }
Review Comment:
Utils.fromJSON already checks for null param and returns an empty map
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -60,74 +79,192 @@ public String configSetName(CoreDescriptor cd) {
@Override
public boolean checkConfigExists(String configName) throws IOException {
- Path solrConfigXmlFile =
configSetBase.resolve(configName).resolve("solrconfig.xml");
- return Files.exists(solrConfigXmlFile);
+ return Files.isDirectory(getConfigDir(configName));
}
@Override
public void deleteConfig(String configName) throws IOException {
- throw new UnsupportedOperationException();
+ deleteDir(getConfigDir(configName));
}
@Override
public void deleteFilesFromConfig(String configName, List<String>
filesToDelete)
throws IOException {
- throw new UnsupportedOperationException();
+ Path configDir = getConfigDir(configName);
+ Objects.requireNonNull(filesToDelete);
+ for (String fileName : filesToDelete) {
+ Path file = configDir.resolve(fileName);
+ if (Files.exists(file)) {
+ if (Files.isDirectory(file)) {
+ deleteDir(file);
+ } else {
+ Files.delete(file);
+ }
+ }
+ }
}
@Override
public void copyConfig(String fromConfig, String toConfig) throws
IOException {
- throw new UnsupportedOperationException();
+ Path source = getConfigDir(fromConfig);
+ Path dest = getConfigDir(toConfig);
+ copyRecursively(source, dest);
+ }
+
+ private void deleteDir(Path dir) throws IOException {
+ try {
+ Files.walkFileTree(
+ dir,
+ new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path path, BasicFileAttributes
attrs)
+ throws IOException {
+ Files.delete(path);
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult postVisitDirectory(Path dir, IOException
ioException)
+ throws IOException {
+ Files.delete(dir);
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ } catch (NoSuchFileException e) {
+ // do nothing
+ }
}
@Override
- public void uploadConfig(String configName, Path dir) throws IOException {
- throw new UnsupportedOperationException();
+ public void uploadConfig(String configName, Path source) throws IOException {
+ Path dest = getConfigDir(configName);
+ copyRecursively(source, dest);
}
@Override
public void uploadFileToConfig(
String configName, String fileName, byte[] data, boolean
overwriteOnExists)
throws IOException {
- throw new UnsupportedOperationException();
+ Path filePath = getConfigDir(configName).resolve(fileName);
+ if (!Files.exists(filePath) || overwriteOnExists) {
+ Files.write(filePath, data);
+ }
}
@Override
public void setConfigMetadata(String configName, Map<String, Object> data)
throws IOException {
- throw new UnsupportedOperationException();
+ // store metadata in .metadata.json file
+ Path metadataPath = getConfigDir(configName).resolve(METADATA_FILE);
+ Files.write(metadataPath, Utils.toJSON(data));
+ Files.isHidden(metadataPath);
Review Comment:
This line does nothing
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -16,35 +16,54 @@
*/
package org.apache.solr.core;
+import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
+
import java.io.FileNotFoundException;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
+import java.nio.file.FileVisitResult;
import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.Utils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Solr Standalone File System ConfigSetService impl.
+ * Standalone File System ConfigSetService impl.
*
- * <p>Loads a ConfigSet defined by the core's configSet property, looking for
a directory named for
+ * <p>Loads ConfigSet defined by the core's configSet property, looking for a
directory named for
* the configSet property value underneath a base directory. If no configSet
property is set, loads
* the ConfigSet instead from the core's instance directory.
*/
public class FileSystemConfigSetService extends ConfigSetService {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private final Path configSetBase;
+ /** .metadata.json hidden file where metadata is stored */
+ public static final String METADATA_FILE = ".metadata.json";
public FileSystemConfigSetService(CoreContainer cc) {
super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
this.configSetBase = cc.getConfig().getConfigSetBaseDirectory();
}
+ /** Testing purpose */
+ public FileSystemConfigSetService(Path configSetBase) {
Review Comment:
make non-public; the tests using it are in the same package
##########
solr/core/src/java/org/apache/solr/core/ConfigSetService.java:
##########
@@ -346,7 +342,7 @@ protected IndexSchema createIndexSchema(
* Returns a modification version for the schema file. Null may be returned
if not known, and if
* so it defeats schema caching.
*/
- protected abstract Long getCurrentSchemaModificationVersion(
+ public abstract Long getCurrentSchemaModificationVersion(
Review Comment:
This was made public so that a custom ConfigSetService can delegate this
method call.
##########
solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java:
##########
@@ -232,15 +230,20 @@ public void setConfigMetadata(String configName,
Map<String, Object> data) throw
@Override
public Map<String, Object> getConfigMetadata(String configName) throws
IOException {
+ byte[] data = null;
try {
- @SuppressWarnings("unchecked")
- Map<String, Object> data =
- (Map<String, Object>)
- Utils.fromJSON(zkClient.getData(CONFIGS_ZKNODE + "/" +
configName, null, null, true));
- return data;
+ data = zkClient.getData(CONFIGS_ZKNODE + "/" + configName, null, null,
true);
+ } catch (KeeperException.NoNodeException e) {
+ return new HashMap<>();
} catch (KeeperException | InterruptedException e) {
throw new IOException("Error getting config metadata",
SolrZkClient.checkInterrupted(e));
}
+ if (data == null) {
+ return new HashMap<>();
+ }
+ @SuppressWarnings("unchecked")
Review Comment:
We have no coding standard apart from the [Google Java
Style](https://google.github.io/styleguide/javaguide.html) which makes no
mentions of multiple returns. FWIW, I like multiple returns!
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -60,74 +79,192 @@ public String configSetName(CoreDescriptor cd) {
@Override
public boolean checkConfigExists(String configName) throws IOException {
- Path solrConfigXmlFile =
configSetBase.resolve(configName).resolve("solrconfig.xml");
- return Files.exists(solrConfigXmlFile);
+ return Files.isDirectory(getConfigDir(configName));
}
@Override
public void deleteConfig(String configName) throws IOException {
- throw new UnsupportedOperationException();
+ deleteDir(getConfigDir(configName));
}
@Override
public void deleteFilesFromConfig(String configName, List<String>
filesToDelete)
throws IOException {
- throw new UnsupportedOperationException();
+ Path configDir = getConfigDir(configName);
+ Objects.requireNonNull(filesToDelete);
+ for (String fileName : filesToDelete) {
+ Path file = configDir.resolve(fileName);
+ if (Files.exists(file)) {
+ if (Files.isDirectory(file)) {
+ deleteDir(file);
+ } else {
+ Files.delete(file);
+ }
+ }
+ }
}
@Override
public void copyConfig(String fromConfig, String toConfig) throws
IOException {
- throw new UnsupportedOperationException();
+ Path source = getConfigDir(fromConfig);
+ Path dest = getConfigDir(toConfig);
+ copyRecursively(source, dest);
+ }
+
+ private void deleteDir(Path dir) throws IOException {
+ try {
+ Files.walkFileTree(
+ dir,
+ new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path path, BasicFileAttributes
attrs)
+ throws IOException {
+ Files.delete(path);
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult postVisitDirectory(Path dir, IOException
ioException)
+ throws IOException {
+ Files.delete(dir);
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ } catch (NoSuchFileException e) {
+ // do nothing
+ }
}
@Override
- public void uploadConfig(String configName, Path dir) throws IOException {
- throw new UnsupportedOperationException();
+ public void uploadConfig(String configName, Path source) throws IOException {
+ Path dest = getConfigDir(configName);
+ copyRecursively(source, dest);
}
@Override
public void uploadFileToConfig(
String configName, String fileName, byte[] data, boolean
overwriteOnExists)
throws IOException {
- throw new UnsupportedOperationException();
+ Path filePath = getConfigDir(configName).resolve(fileName);
+ if (!Files.exists(filePath) || overwriteOnExists) {
+ Files.write(filePath, data);
+ }
}
@Override
public void setConfigMetadata(String configName, Map<String, Object> data)
throws IOException {
- throw new UnsupportedOperationException();
+ // store metadata in .metadata.json file
+ Path metadataPath = getConfigDir(configName).resolve(METADATA_FILE);
+ Files.write(metadataPath, Utils.toJSON(data));
+ Files.isHidden(metadataPath);
}
@Override
public Map<String, Object> getConfigMetadata(String configName) throws
IOException {
- throw new UnsupportedOperationException();
+ // get metadata from .metadata.json file
+ Path metadataPath = getConfigDir(configName).resolve(METADATA_FILE);
+ byte[] data = null;
+ try {
+ data = Files.readAllBytes(metadataPath);
+ } catch (NoSuchFileException e) {
+ return new HashMap<>();
Review Comment:
`Collections.emptyMap()` -- no need for a new mutable map
--
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]
