dsmiley commented on code in PR #1239:
URL: https://github.com/apache/solr/pull/1239#discussion_r1062955249
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -238,9 +238,14 @@ public List<String> getAllConfigFiles(String configName)
throws IOException {
@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());
+ Path filePath = configDir.relativize(file);
+ String filePathStr = filePath.toString();
+ filePathStr =
+ filePathStr.replace(
+ filePath.getFileSystem().getSeparator(), "/"); //
normalize slashes
+ // don't include .metadata.json hidden file
+ if (!filePathStr.equals(METADATA_FILE)) {
+ filePaths.add(filePathStr);
Review Comment:
This ought to be documented in the method contract for getAllConfigFiles()
better, although at least the existing docs give examples using `/` as a
separator. I see two users of this method:
* `org.apache.solr.core.backup.BackupManager#downloadConfigToRepo` which
takes these and calls
`org.apache.solr.core.backup.repository.BackupRepository#resolve` which isn't
documented clearly on separators either. Its file system based implementation
assumes it's slash-compatible, which means it'll break if getAllConfigFiles()
were to normalize them.
* `org.apache.solr.handler.configsets.UploadConfigSetAPI#uploadConfigSet`
which will do two things:
* compare the result with a zip entry file name. I dug a little and[ Zip
files should normalize to '/' separator](https://stackoverflow.com/a/44387973).
* it'll call `org.apache.solr.core.ConfigSetService#deleteFilesFromConfig`
which doesn't document this separator matter either.
So it's a bit of a mess. I think getAllConfigFiles() should normalize to
'/' due to the generic nature of the abstraction (isn't specific to a local
file system) and ensure that
`org.apache.solr.core.backup.repository.BackupRepository#resolve` and
`org.apache.solr.core.ConfigSetService#deleteFilesFromConfig` translate a `/`
in its input a file system native separator if there's a difference.
--
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]
