[GitHub] [solr] dsmiley commented on a diff in pull request #1239: SOLR-15787: Fix FileSystemConfigSetService test failure

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org