epugh commented on code in PR #2924:
URL: https://github.com/apache/solr/pull/2924#discussion_r1910305906
##########
solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java:
##########
@@ -367,10 +366,7 @@ private void close(CacheValue val) {
}
private static boolean isSubPath(CacheValue cacheValue, CacheValue
otherCacheValue) {
- int one = cacheValue.path.lastIndexOf(File.separatorChar);
- int two = otherCacheValue.path.lastIndexOf(File.separatorChar);
-
- return otherCacheValue.path.startsWith(cacheValue.path +
File.separatorChar) && two > one;
+ return Path.of(otherCacheValue.path).startsWith(Path.of(cacheValue.path));
Review Comment:
love it
##########
solr/core/src/java/org/apache/solr/core/DirectoryFactory.java:
##########
@@ -338,59 +335,66 @@ public String getDataHome(CoreDescriptor cd) throws
IOException {
}
public void cleanupOldIndexDirectories(
- final String dataDirPath, final String currentIndexDirPath, boolean
afterCoreReload) {
+ final String dataDirPath, final String currentIndexDirPath, boolean
afterCoreReload)
+ throws IOException {
- // TODO SOLR-8282 move to PATH
- File dataDir = new File(dataDirPath);
- if (!dataDir.isDirectory()) {
+ Path dataDirFile = Path.of(dataDirPath);
Review Comment:
love seeing a TODO get TODONE'ed!
##########
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:
Can we rename this method to `getRealPath`?
##########
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:
Could we ditch the `FileInputStream`? I asked GPT, and it said:
```
InputStream inputStream = Files.newInputStream(path))
```
The recommended approach is using `Files.newInputStream()` because:
- It's more flexible (supports options)
- Works with any filesystem provider
- Better handles symbolic links
- Part of the modern NIO.2 API
##########
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:
maybe another place where if `isMetaDAtaFile` takes a Path things are easier?
##########
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:
Files.newInputStream?
##########
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:
Would using Path instead of String here make sense for our Distrib File
Store? (Do Paths model distributed file system better than Files did??).
##########
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:
Just wanted to confirm this throw was needed? Maybe because of something
else furthur down?
##########
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:
I saw we have a `SolrPaths.assertNotUnc(dir);`, could it be used here?
##########
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:
maybe just call `admin` `adminFile` in the first place? Now that you fixed
the todo?
##########
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:
wow that's gnarly.. Is there no existing `normalize()` function in any of
our jars that would do this for us? I guess fixing that is orthongonal to
this effort....
##########
solr/core/src/java/org/apache/solr/core/backup/BackupManager.java:
##########
@@ -343,7 +343,7 @@ private void downloadConfigToRepo(ConfigSetService
configSetService, String conf
// getAllConfigFiles always separates file paths with '/'
for (String filePath : filePaths) {
// Replace '/' to ensure that propre file is resolved for writing.
- URI uri = repository.resolve(dir, filePath.replace('/',
File.separatorChar));
+ URI uri = repository.resolve(dir, Path.of(filePath).toString());
Review Comment:
could `repository.resolve()` take a path instead of a string?
##########
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:
can indexDir be a path?
##########
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##########
@@ -1061,35 +1060,37 @@ private void reloadCore() {
}
private void downloadConfFiles(
- List<Map<String, Object>> confFilesToDownload, long latestGeneration)
throws Exception {
+ List<Map<String, Object>> confFilesToDownload, long latestGeneration) {
log.info("Starting download of configuration files from leader: {}",
confFilesToDownload);
confFilesDownloaded = Collections.synchronizedList(new ArrayList<>());
- Path tmpConfPath =
+ Path tmpconfDir =
Review Comment:
maybe `tmpConfDir`?
##########
solr/core/src/java/org/apache/solr/filestore/FileStore.java:
##########
@@ -44,7 +44,7 @@ public interface FileStore {
/** Fetch a resource from another node internal API */
boolean fetch(String path, String from);
- List<FileDetails> list(String path, Predicate<String> predicate);
+ List<FileDetails> list(String path, Predicate<String> predicate) throws
IOException;
Review Comment:
just confirming we DO need this throws.
##########
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:
maybe move thid `path` variable creation down below the check in case we
don't need to do it?
##########
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 =
- "conf" + File.separator + "solrcore.properties";
+ Path.of("conf/solrcore.properties").toString();
Review Comment:
This is a really great benefit of the migration!
##########
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:
`Files.newInputStream()` instead?
##########
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:
can indexPath be a Path? Also, is there an opportunity to align variable
naming between indexPath and the use of indexDir in in
AbstractLuceneSpellChecker?
##########
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:
lets. get rid of this todo.. this code hasn't changed in years, and if
someone wants it they can add it later! we shouldn't litter our code with
todos that are mostly ideas.. a todo really implies hey, someone SHOULD go do
this very soon!
--
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
