mlbiscoc commented on code in PR #2924:
URL: https://github.com/apache/solr/pull/2924#discussion_r1918615357
##########
solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java:
##########
@@ -353,12 +353,14 @@ public ClassLoader getClassLoader() {
*/
@Override
public InputStream openResource(String resource) throws IOException {
- if (resource.trim().startsWith("\\\\")) { // Always disallow UNC paths
- throw new SolrResourceNotFoundException("Resource '" + resource + "'
could not be loaded.");
+ String pathResource = Paths.get(resource).normalize().toString();
+ if (pathResource.trim().startsWith("\\\\")) { // Always disallow UNC paths
Review Comment:
Thanks for the find, Eric. I don't know why I overcomplicated this line with
the normalize. Just passing the resource here instead so not much should have
changed.
Also switched to `assertNotUnc` but the function checked both the path and
the systems OS. I made a tiny modification to it which I am not sure if we
should do but a test that was asserting the message was modified as well.
##########
solr/core/src/java/org/apache/solr/core/SolrPaths.java:
##########
@@ -43,9 +42,7 @@ private SolrPaths() {} // don't create this
/** Ensures a directory name always ends with a '/'. */
public static String normalizeDir(String path) {
- return (path != null && (!(path.endsWith("/") || path.endsWith("\\"))))
- ? path + File.separator
- : path;
+ return (path != null && (!(path.endsWith("/") || path.endsWith("\\")))) ?
path + "/" : path;
Review Comment:
I was thinking, we shouldn't need to call
`FileSystems.getDefault().getSeparator()` assuming callers of this eventually
take this string and put it into a Path, but I don't think that is necessarily
true. Seeing something like [this
line](https://github.com/apache/solr/blob/82083ea13ad2b2114d4311439be2f90ed868fac1/solr/core/src/java/org/apache/solr/update/UpdateLog.java#L363)
it's comparing strings 1:1 but my slash would probably break it on a different
OS that uses a different delimiter. Directory strings should probably be
checked through `Path.equal()`, not strings directly. I just put it back with
`FileSystems.getDefault().getSeparator()`
##########
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java:
##########
@@ -195,54 +195,69 @@ private void showFromZooKeeper(
}
// Return the file indicated (or the directory listing) from the local file
system.
- private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
{
+ private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
throws IOException {
Path admin = getAdminFileFromFileSystem(req, rsp, hiddenFiles);
if (admin == null) { // exception already recorded
return;
}
- // TODO SOLR-8282 move to PATH
- File adminFile = admin.toFile();
+ Path adminFile = admin;
Review Comment:
yes, that was kind of a 🤦 moment after looking at this. Thanks for catching!
##########
solr/core/src/java/org/apache/solr/util/VersionedFile.java:
##########
@@ -41,44 +41,47 @@ public class VersionedFile {
*/
public static InputStream getLatestFile(String dirName, String fileName)
throws FileNotFoundException {
- // TODO SOLR-8282 move to PATH
- Collection<File> oldFiles = null;
+ Collection<Path> oldFiles = null;
final String prefix = fileName + '.';
- File f = new File(dirName, fileName);
+ Path f = Path.of(dirName, fileName);
InputStream is = null;
// there can be a race between checking for a file and opening it...
// the user may have just put a new version in and deleted an old version.
// try multiple times in a row.
for (int retry = 0; retry < 10 && is == null; retry++) {
try {
- if (!f.exists()) {
- File dir = new File(dirName);
- String[] names =
- dir.list(
- new FilenameFilter() {
- @Override
- public boolean accept(File dir, String name) {
- return name.startsWith(prefix);
- }
- });
- Arrays.sort(names);
- f = new File(dir, names[names.length - 1]);
+ if (!Files.exists(f)) {
+ Path dir = Path.of(dirName);
+ List<Path> fileList;
+
+ try (Stream<Path> files = Files.list(dir)) {
+ fileList =
+ files
+ .filter((file) ->
file.getFileName().toString().startsWith(prefix))
+ .sorted()
+ .toList();
+ } catch (IOException e) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Unable to list files in
" + dir, e);
+ }
+
+ f = Path.of(dir.toString(),
fileList.getLast().getFileName().toString());
oldFiles = new ArrayList<>();
- for (int i = 0; i < names.length - 1; i++) {
- oldFiles.add(new File(dir, names[i]));
+ for (int i = 0; i < fileList.size() - 1; i++) {
+ oldFiles.add(Path.of(dir.toString(), fileList.get(i).toString()));
}
}
- is = new FileInputStream(f);
+ is = new FileInputStream(f.toString());
Review Comment:
Added TODO. Might either make another story or just PR to move them all. I
see about 34 instances of this used including inside of tests (which is a whole
different monster 😵💫) but at least after this PR, there should be no uses of
File in Solr itself.
##########
solr/core/src/java/org/apache/solr/core/CoreDescriptor.java:
##########
@@ -64,7 +63,7 @@ public class CoreDescriptor {
public static final String SOLR_CORE_PROP_PREFIX = "solr.core.";
public static final String DEFAULT_EXTERNAL_PROPERTIES_FILE =
Review Comment:
This only had 1 usage used as a string. Easy to move to Path and call
toString().
##########
solr/core/src/java/org/apache/solr/util/VersionedFile.java:
##########
@@ -41,44 +41,47 @@ public class VersionedFile {
*/
public static InputStream getLatestFile(String dirName, String fileName)
throws FileNotFoundException {
- // TODO SOLR-8282 move to PATH
- Collection<File> oldFiles = null;
+ Collection<Path> oldFiles = null;
final String prefix = fileName + '.';
- File f = new File(dirName, fileName);
+ Path f = Path.of(dirName, fileName);
InputStream is = null;
// there can be a race between checking for a file and opening it...
// the user may have just put a new version in and deleted an old version.
// try multiple times in a row.
for (int retry = 0; retry < 10 && is == null; retry++) {
try {
- if (!f.exists()) {
- File dir = new File(dirName);
- String[] names =
- dir.list(
- new FilenameFilter() {
- @Override
- public boolean accept(File dir, String name) {
- return name.startsWith(prefix);
- }
- });
- Arrays.sort(names);
- f = new File(dir, names[names.length - 1]);
+ if (!Files.exists(f)) {
+ Path dir = Path.of(dirName);
+ List<Path> fileList;
+
+ try (Stream<Path> files = Files.list(dir)) {
+ fileList =
+ files
+ .filter((file) ->
file.getFileName().toString().startsWith(prefix))
+ .sorted()
+ .toList();
+ } catch (IOException e) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Unable to list files in
" + dir, e);
+ }
+
+ f = Path.of(dir.toString(),
fileList.getLast().getFileName().toString());
oldFiles = new ArrayList<>();
- for (int i = 0; i < names.length - 1; i++) {
- oldFiles.add(new File(dir, names[i]));
+ for (int i = 0; i < fileList.size() - 1; i++) {
+ oldFiles.add(Path.of(dir.toString(), fileList.get(i).toString()));
}
}
- is = new FileInputStream(f);
+ is = new FileInputStream(f.toString());
} catch (Exception e) {
// swallow exception for now
}
}
// allow exception to be thrown from the final try.
if (is == null) {
- is = new FileInputStream(f);
+ is = new FileInputStream(f.toString());
Review Comment:
See comment above
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1124,6 +1121,9 @@ protected SolrCore(
this.snapshotMgr = initSnapshotMetaDataManager();
this.solrDelPolicy = initDeletionPolicy(delPolicy);
+ // initialize core metrics
+ initializeMetrics(solrMetricsContext, null);
Review Comment:
This was a mistake. I forgot to revert. This has to do with metrics being
pre-computed with File disk usage and was trying to get the data directory
initialized before the metrics registered. I eventually stopped and forgot to
move it back
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -85,14 +85,14 @@ public Path getRealpath(String path) {
}
private static Path _getRealPath(String path, Path solrHome) {
- if (File.separatorChar == '\\') {
- path = path.replace('/', File.separatorChar);
- }
- SolrPaths.assertNotUnc(Path.of(path));
- while (path.startsWith(File.separator)) { // Trim all leading slashes
+ Path dir = Path.of(path);
+ SolrPaths.assertNotUnc(dir);
+
+ while (path.startsWith("/")) {
path = path.substring(1);
}
Review Comment:
Added in a comment. I'll see if I can add a check if the Path is absolute
and throw an illegal argument exception. Maybe tests will catch something and
is easily fixable, otherwise maybe a TODO.
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -428,10 +428,10 @@ public boolean fetch(String path, String from) {
@Override
public void get(String path, Consumer<FileEntry> consumer, boolean
fetchmissing)
throws IOException {
- File file = getRealpath(path).toFile();
- String simpleName = file.getName();
+ Path file = getRealpath(path);
+ String simpleName = file.getFileName().toString();
if (isMetaDataFile(simpleName)) {
- try (InputStream is = new FileInputStream(file)) {
+ try (InputStream is = new FileInputStream(file.toString())) {
Review Comment:
Thanks Eric for finding that, I missed it. Definitely should move towards
that. Added this to TODO and will come back in another PR. Took a look and
wasn't 1:1 change
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/NodeFileStoreApis.java:
##########
@@ -63,5 +64,6 @@ SolrJerseyResponse getFile(
String getFrom,
@Parameter(description = "Indicates that (only) file metadata should be
fetched")
@QueryParam("meta")
- Boolean meta);
+ Boolean meta)
+ throws IOException;
Review Comment:
Actually no it isn't needed. It was due to my changes on `List<FileDetails>
list()` method under `DistribFileStore` so I threw a SolrException instead
which seems correct.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1354,16 +1354,29 @@ public void initializeMetrics(SolrMetricsContext
parentContext, String scope) {
Category.CORE.toString());
}
- // TODO SOLR-8282 move to PATH
// initialize disk total / free metrics
- Path dataDirPath = Paths.get(dataDir);
- File dataDirFile = dataDirPath.toFile();
- parentContext.gauge(
- () -> dataDirFile.getTotalSpace(), true, "totalSpace",
Category.CORE.toString(), "fs");
+ Path dataDirFile = Paths.get(dataDir);
+ long totalSpace;
+ long useableSpace;
+
+ try {
+ totalSpace = Files.getFileStore(dataDirFile).getTotalSpace();
+ useableSpace = Files.getFileStore(dataDirFile).getUsableSpace();
+ } catch (Exception e) {
+ // TODO Metrics used to default to 0 with java.io.File even if directory
didn't exist
+ // Should throw an exception and initialize data directory before metrics
+ totalSpace = 0L;
+ useableSpace = 0L;
+ }
Review Comment:
Just initialized the gauges to 0 and added the TODO. Could be worth moving
metric initialization to after the directory is created if we want that data
point of the disk usage.
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -458,25 +458,25 @@ public void syncToAllNodes(String path) throws
IOException {
}
@Override
- public List<FileDetails> list(String path, Predicate<String> predicate) {
- File file = getRealpath(path).toFile();
+ public List<FileDetails> list(String path, Predicate<String> predicate)
throws IOException {
+ Path file = getRealpath(path);
List<FileDetails> fileDetails = new ArrayList<>();
FileType type = getType(path, false);
if (type == FileType.DIRECTORY) {
- file.list(
- (dir, name) -> {
- if (predicate == null || predicate.test(name)) {
- if (!isMetaDataFile(name)) {
- fileDetails.add(new FileInfo(path + "/" + name).getDetails());
+ try (Stream<Path> fileStream = Files.list(file)) {
+ fileStream.forEach(
+ (f) -> {
+ String fileName = f.getFileName().toString();
+ if (predicate == null || predicate.test(fileName)) {
+ if (!isMetaDataFile(fileName)) {
Review Comment:
Just for clarification, you want me to remove the String of fileName here
and change DistribFileStore to just use Path in all it's functions? If so, I
think this is definitely doable and probably much better that passing string
directories to be converted later. If this is what you mean, I can definitely
give it a shot.
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -458,25 +458,25 @@ public void syncToAllNodes(String path) throws
IOException {
}
@Override
- public List<FileDetails> list(String path, Predicate<String> predicate) {
- File file = getRealpath(path).toFile();
+ public List<FileDetails> list(String path, Predicate<String> predicate)
throws IOException {
+ Path file = getRealpath(path);
Review Comment:
Renamed to `getRealPath`. Thanks to the creator of find/replace 🙌
##########
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java:
##########
@@ -195,54 +195,69 @@ private void showFromZooKeeper(
}
// Return the file indicated (or the directory listing) from the local file
system.
- private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
{
+ private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
throws IOException {
Path admin = getAdminFileFromFileSystem(req, rsp, hiddenFiles);
if (admin == null) { // exception already recorded
return;
}
- // TODO SOLR-8282 move to PATH
- File adminFile = admin.toFile();
+ Path adminFile = admin;
// Make sure the file exists, is readable and is not a hidden file
- if (!adminFile.exists()) {
- log.error("Can not find: {} [{}]", adminFile.getName(),
adminFile.getAbsolutePath());
+ if (!Files.exists(adminFile)) {
+ log.error("Can not find: {} [{}]", adminFile.getFileName(),
adminFile.toAbsolutePath());
rsp.setException(
new SolrException(
ErrorCode.NOT_FOUND,
- "Can not find: " + adminFile.getName() + " [" +
adminFile.getAbsolutePath() + "]"));
+ "Can not find: "
+ + adminFile.getFileName()
+ + " ["
+ + adminFile.toAbsolutePath()
+ + "]"));
return;
}
- if (!adminFile.canRead() || adminFile.isHidden()) {
- log.error("Can not show: {} [{}]", adminFile.getName(),
adminFile.getAbsolutePath());
+ if (!Files.isReadable(adminFile) || Files.isHidden(adminFile)) {
+ log.error("Can not show: {} [{}]", adminFile.getFileName(),
adminFile.toAbsolutePath());
rsp.setException(
new SolrException(
ErrorCode.NOT_FOUND,
- "Can not show: " + adminFile.getName() + " [" +
adminFile.getAbsolutePath() + "]"));
+ "Can not show: "
+ + adminFile.getFileName()
+ + " ["
+ + adminFile.toAbsolutePath()
+ + "]"));
return;
}
// Show a directory listing
- if (adminFile.isDirectory()) {
+ if (Files.isDirectory(adminFile)) {
// it's really a directory, just go for it.
- int basePath = adminFile.getAbsolutePath().length() + 1;
NamedList<SimpleOrderedMap<Object>> files = new SimpleOrderedMap<>();
- for (File f : adminFile.listFiles()) {
- String path = f.getAbsolutePath().substring(basePath);
- path = path.replace('\\', '/'); // normalize slashes
-
- if (isHiddenFile(req, rsp, f.getName().replace('\\', '/'), false,
hiddenFiles)) {
- continue;
- }
-
- SimpleOrderedMap<Object> fileInfo = new SimpleOrderedMap<>();
- files.add(path, fileInfo);
- if (f.isDirectory()) {
- fileInfo.add("directory", true);
- } else {
- // TODO? content type
- fileInfo.add("size", f.length());
- }
- fileInfo.add("modified", new Date(f.lastModified()));
+ try (Stream<Path> directoryFiles = Files.list(adminFile)) {
+ directoryFiles.forEach(
+ (f) -> {
+ String path = f.getFileName().toString();
+
+ if (isHiddenFile(req, rsp, f.getFileName().toString(), false,
hiddenFiles)) {
+ return;
+ }
+
+ SimpleOrderedMap<Object> fileInfo = new SimpleOrderedMap<>();
+ files.add(path, fileInfo);
+ if (Files.isDirectory(f)) {
+ fileInfo.add("directory", true);
+ } else {
+ // TODO? content type
Review Comment:
Got it. Removed it. Mostly trying to just untouch code unless needed so was
just porting over anything including TODO's. Let me know if there is more that
should be removed.
##########
solr/core/src/java/org/apache/solr/filestore/DistribFileStore.java:
##########
@@ -550,19 +550,19 @@ public void refresh(String path) {
@Override
public FileType getType(String path, boolean fetchMissing) {
- File file = getRealpath(path).toFile();
- if (!file.exists() && fetchMissing) {
+ Path file = getRealpath(path);
+ if (!Files.exists(file) && fetchMissing) {
if (fetch(path, null)) {
- file = getRealpath(path).toFile();
+ file = getRealpath(path);
}
}
return _getFileType(file);
}
- public static FileType _getFileType(File file) {
- if (!file.exists()) return FileType.NOFILE;
- if (file.isDirectory()) return FileType.DIRECTORY;
- return isMetaDataFile(file.getName()) ? FileType.METADATA : FileType.FILE;
+ public static FileType _getFileType(Path file) {
+ if (!Files.exists(file)) return FileType.NOFILE;
+ if (Files.isDirectory(file)) return FileType.DIRECTORY;
+ return isMetaDataFile(file.getFileName().toString()) ? FileType.METADATA :
FileType.FILE;
Review Comment:
See me comment above
##########
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java:
##########
@@ -195,54 +195,69 @@ private void showFromZooKeeper(
}
// Return the file indicated (or the directory listing) from the local file
system.
- private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
{
+ private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
throws IOException {
Path admin = getAdminFileFromFileSystem(req, rsp, hiddenFiles);
if (admin == null) { // exception already recorded
return;
}
- // TODO SOLR-8282 move to PATH
- File adminFile = admin.toFile();
+ Path adminFile = admin;
// Make sure the file exists, is readable and is not a hidden file
- if (!adminFile.exists()) {
- log.error("Can not find: {} [{}]", adminFile.getName(),
adminFile.getAbsolutePath());
+ if (!Files.exists(adminFile)) {
+ log.error("Can not find: {} [{}]", adminFile.getFileName(),
adminFile.toAbsolutePath());
rsp.setException(
new SolrException(
ErrorCode.NOT_FOUND,
- "Can not find: " + adminFile.getName() + " [" +
adminFile.getAbsolutePath() + "]"));
+ "Can not find: "
+ + adminFile.getFileName()
+ + " ["
+ + adminFile.toAbsolutePath()
+ + "]"));
return;
}
- if (!adminFile.canRead() || adminFile.isHidden()) {
- log.error("Can not show: {} [{}]", adminFile.getName(),
adminFile.getAbsolutePath());
+ if (!Files.isReadable(adminFile) || Files.isHidden(adminFile)) {
+ log.error("Can not show: {} [{}]", adminFile.getFileName(),
adminFile.toAbsolutePath());
rsp.setException(
new SolrException(
ErrorCode.NOT_FOUND,
- "Can not show: " + adminFile.getName() + " [" +
adminFile.getAbsolutePath() + "]"));
+ "Can not show: "
+ + adminFile.getFileName()
+ + " ["
+ + adminFile.toAbsolutePath()
+ + "]"));
return;
}
// Show a directory listing
- if (adminFile.isDirectory()) {
+ if (Files.isDirectory(adminFile)) {
// it's really a directory, just go for it.
- int basePath = adminFile.getAbsolutePath().length() + 1;
NamedList<SimpleOrderedMap<Object>> files = new SimpleOrderedMap<>();
- for (File f : adminFile.listFiles()) {
- String path = f.getAbsolutePath().substring(basePath);
- path = path.replace('\\', '/'); // normalize slashes
-
- if (isHiddenFile(req, rsp, f.getName().replace('\\', '/'), false,
hiddenFiles)) {
- continue;
- }
-
- SimpleOrderedMap<Object> fileInfo = new SimpleOrderedMap<>();
- files.add(path, fileInfo);
- if (f.isDirectory()) {
- fileInfo.add("directory", true);
- } else {
- // TODO? content type
- fileInfo.add("size", f.length());
- }
- fileInfo.add("modified", new Date(f.lastModified()));
+ try (Stream<Path> directoryFiles = Files.list(adminFile)) {
+ directoryFiles.forEach(
+ (f) -> {
+ String path = f.getFileName().toString();
Review Comment:
Yeah, makes sense. Moved it below
##########
solr/core/src/java/org/apache/solr/spelling/suggest/fst/AnalyzingInfixLookupFactory.java:
##########
@@ -89,7 +88,7 @@ public Lookup create(NamedList<?> params, SolrCore core) {
String indexPath =
params.get(INDEX_PATH) != null ? params.get(INDEX_PATH).toString() :
DEFAULT_INDEX_PATH;
if (!Path.of(indexPath).isAbsolute()) {
- indexPath = core.getDataDir() + File.separator + indexPath;
+ indexPath = Path.of(core.getDataDir(), indexPath).toString();
Review Comment:
Yes good catch. Much better than calling Path.of everywhere.
##########
solr/core/src/java/org/apache/solr/core/CoreDescriptor.java:
##########
@@ -96,7 +95,7 @@ public Properties getPersistableUserProperties() {
CORE_CONFIG, "solrconfig.xml",
CORE_SCHEMA, "schema.xml",
CORE_CONFIGSET_PROPERTIES, ConfigSetProperties.DEFAULT_FILENAME,
- CORE_DATADIR, "data" + File.separator,
+ CORE_DATADIR, Path.of("data/").toString(),
Review Comment:
Removed
##########
solr/core/src/java/org/apache/solr/spelling/AbstractLuceneSpellChecker.java:
##########
@@ -84,7 +83,7 @@ public String init(NamedList<?> config, SolrCore core) {
// If indexDir is relative then create index inside core.getDataDir()
if (indexDir != null) {
if (!Path.of(indexDir).isAbsolute()) {
- indexDir = core.getDataDir() + File.separator + indexDir;
+ indexDir = Path.of(core.getDataDir(), indexDir).toString();
Review Comment:
Yes! Changed it as Path.
##########
solr/core/src/java/org/apache/solr/cloud/ZkSolrResourceLoader.java:
##########
@@ -121,7 +120,7 @@ public InputStream openResource(String resource) throws
IOException {
try {
// delegate to the class loader (looking into $INSTANCE_DIR/lib jars)
- is =
classLoader.getResourceAsStream(resource.replace(File.separatorChar, '/'));
+ is = classLoader.getResourceAsStream(Path.of(resource).toString());
Review Comment:
Added
##########
solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java:
##########
@@ -404,8 +406,7 @@ public String resourceLocation(String resource) {
return inInstanceDir.normalize().toString();
}
- try (InputStream is =
- classLoader.getResourceAsStream(resource.replace(File.separatorChar,
'/'))) {
+ try (InputStream is =
classLoader.getResourceAsStream(Path.of(resource).toString())) {
Review Comment:
You're correct, bad that I removed the replace. Brought it back but changed
to `FileSystems.getDefault().getSeparator()`
##########
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java:
##########
@@ -195,54 +195,69 @@ private void showFromZooKeeper(
}
// Return the file indicated (or the directory listing) from the local file
system.
- private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
{
+ private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
throws IOException {
Path admin = getAdminFileFromFileSystem(req, rsp, hiddenFiles);
if (admin == null) { // exception already recorded
return;
}
- // TODO SOLR-8282 move to PATH
- File adminFile = admin.toFile();
+ Path adminFile = admin;
// Make sure the file exists, is readable and is not a hidden file
- if (!adminFile.exists()) {
- log.error("Can not find: {} [{}]", adminFile.getName(),
adminFile.getAbsolutePath());
+ if (!Files.exists(adminFile)) {
+ log.error("Can not find: {} [{}]", adminFile.getFileName(),
adminFile.toAbsolutePath());
rsp.setException(
new SolrException(
ErrorCode.NOT_FOUND,
- "Can not find: " + adminFile.getName() + " [" +
adminFile.getAbsolutePath() + "]"));
+ "Can not find: "
+ + adminFile.getFileName()
+ + " ["
+ + adminFile.toAbsolutePath()
+ + "]"));
return;
}
- if (!adminFile.canRead() || adminFile.isHidden()) {
- log.error("Can not show: {} [{}]", adminFile.getName(),
adminFile.getAbsolutePath());
+ if (!Files.isReadable(adminFile) || Files.isHidden(adminFile)) {
+ log.error("Can not show: {} [{}]", adminFile.getFileName(),
adminFile.toAbsolutePath());
rsp.setException(
new SolrException(
ErrorCode.NOT_FOUND,
- "Can not show: " + adminFile.getName() + " [" +
adminFile.getAbsolutePath() + "]"));
+ "Can not show: "
+ + adminFile.getFileName()
+ + " ["
+ + adminFile.toAbsolutePath()
+ + "]"));
return;
}
// Show a directory listing
- if (adminFile.isDirectory()) {
+ if (Files.isDirectory(adminFile)) {
// it's really a directory, just go for it.
- int basePath = adminFile.getAbsolutePath().length() + 1;
NamedList<SimpleOrderedMap<Object>> files = new SimpleOrderedMap<>();
- for (File f : adminFile.listFiles()) {
- String path = f.getAbsolutePath().substring(basePath);
- path = path.replace('\\', '/'); // normalize slashes
Review Comment:
For this method itself, I don't think it should need it, but you just want
the method itself in Solr for other use-cases?
I created a static `toForwardSlashPathString` method in this class. Maybe it
belongs or we could put it in `FileUtils.java`?
What are scenarios are does Solr need this method?
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminOperation.java:
##########
@@ -357,7 +356,7 @@ public static NamedList<Object> getCoreStatus(
if (core != null) {
info.add(NAME, core.getName());
info.add("instanceDir", core.getInstancePath().toString());
- info.add("dataDir", normalizePath(core.getDataDir()));
+ info.add("dataDir", Path.of(core.getDataDir()).toString());
Review Comment:
I completely removed that `normalizePath` because I thought Path would do
this for us already, so the utility wasn't needed. Happy to bring it back
though then I guess keep the code as it was?
##########
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java:
##########
@@ -195,54 +195,69 @@ private void showFromZooKeeper(
}
// Return the file indicated (or the directory listing) from the local file
system.
- private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
{
+ private void showFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp)
throws IOException {
Path admin = getAdminFileFromFileSystem(req, rsp, hiddenFiles);
if (admin == null) { // exception already recorded
return;
}
- // TODO SOLR-8282 move to PATH
- File adminFile = admin.toFile();
+ Path adminFile = admin;
// Make sure the file exists, is readable and is not a hidden file
- if (!adminFile.exists()) {
- log.error("Can not find: {} [{}]", adminFile.getName(),
adminFile.getAbsolutePath());
+ if (!Files.exists(adminFile)) {
+ log.error("Can not find: {} [{}]", adminFile.getFileName(),
adminFile.toAbsolutePath());
rsp.setException(
new SolrException(
ErrorCode.NOT_FOUND,
- "Can not find: " + adminFile.getName() + " [" +
adminFile.getAbsolutePath() + "]"));
+ "Can not find: "
+ + adminFile.getFileName()
+ + " ["
+ + adminFile.toAbsolutePath()
+ + "]"));
return;
}
- if (!adminFile.canRead() || adminFile.isHidden()) {
- log.error("Can not show: {} [{}]", adminFile.getName(),
adminFile.getAbsolutePath());
+ if (!Files.isReadable(adminFile) || Files.isHidden(adminFile)) {
+ log.error("Can not show: {} [{}]", adminFile.getFileName(),
adminFile.toAbsolutePath());
rsp.setException(
new SolrException(
ErrorCode.NOT_FOUND,
- "Can not show: " + adminFile.getName() + " [" +
adminFile.getAbsolutePath() + "]"));
+ "Can not show: "
+ + adminFile.getFileName()
+ + " ["
+ + adminFile.toAbsolutePath()
+ + "]"));
return;
}
// Show a directory listing
- if (adminFile.isDirectory()) {
+ if (Files.isDirectory(adminFile)) {
// it's really a directory, just go for it.
- int basePath = adminFile.getAbsolutePath().length() + 1;
NamedList<SimpleOrderedMap<Object>> files = new SimpleOrderedMap<>();
- for (File f : adminFile.listFiles()) {
- String path = f.getAbsolutePath().substring(basePath);
- path = path.replace('\\', '/'); // normalize slashes
-
- if (isHiddenFile(req, rsp, f.getName().replace('\\', '/'), false,
hiddenFiles)) {
- continue;
- }
-
- SimpleOrderedMap<Object> fileInfo = new SimpleOrderedMap<>();
- files.add(path, fileInfo);
- if (f.isDirectory()) {
- fileInfo.add("directory", true);
- } else {
- // TODO? content type
- fileInfo.add("size", f.length());
- }
- fileInfo.add("modified", new Date(f.lastModified()));
+ try (Stream<Path> directoryFiles = Files.list(adminFile)) {
+ directoryFiles.forEach(
Review Comment:
Wouldn't we need to close the `directoryFiles` stream after then?
##########
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##########
@@ -1484,28 +1484,27 @@ private void copyTmpConfFiles2Conf(Path tmpconfDir) {
int numTempPathElements = tmpconfDir.getNameCount();
for (Path path : makeTmpConfDirFileList(tmpconfDir)) {
Path oldPath = confPath.resolve(path.subpath(numTempPathElements,
path.getNameCount()));
+
try {
Files.createDirectories(oldPath.getParent());
} catch (IOException e) {
throw new SolrException(
ErrorCode.SERVER_ERROR, "Unable to mkdirs: " +
oldPath.getParent(), e);
}
if (Files.exists(oldPath)) {
- File oldFile = oldPath.toFile(); // TODO SOLR-8282 move to PATH
- File backupFile =
- new File(oldFile.getPath() + "." + getDateAsStr(new
Date(oldFile.lastModified())));
- if (!backupFile.getParentFile().exists()) {
- status = backupFile.getParentFile().mkdirs();
- if (!status) {
- throw new SolrException(
- ErrorCode.SERVER_ERROR, "Unable to mkdirs: " +
backupFile.getParentFile());
+ try {
+ Path backupFile =
+ Path.of(
+ oldPath
+ + "."
+ + getDateAsStr(new
Date(Files.getLastModifiedTime(oldPath).toMillis())));
+ if (!Files.exists(backupFile.getParent())) {
+ Files.createDirectories(backupFile.getParent());
}
Review Comment:
Cool, removed that `!Files.exists(backupFile.getParent())`
--
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
