Return NotFound for directories in Head and Get (#4230) (#4231)

* Return NotFound for directories in Head and Get (#4230)

* Fix webdav

* Fix error message

object_store/src/azure/client.rs

@@ -257,7 +257,19 @@ impl AzureClient {
                 path: path.as_ref(),
             })?;
 
-        Ok(response)
+        match response.headers().get("x-ms-resource-type") {
+            Some(resource) if resource.as_ref() != b"file" => {
+                Err(crate::Error::NotFound {
+                    path: path.to_string(),
+                    source: format!(
+                        "Not a file, got x-ms-resource-type: {}",
+                        String::from_utf8_lossy(resource.as_ref())
+                    )
+                    .into(),
+                })
+            }
+            _ => Ok(response),
+        }
     }
 
     /// Make an Azure Delete request <https://docs.microsoft.com/en-us/rest/api/storageservices/delete-blob>

object_store/src/http/client.rs

@@ -238,10 +238,13 @@ impl Client {
             .send_retry(&self.retry_config)
             .await
             .map_err(|source| match source.status() {
-                Some(StatusCode::NOT_FOUND) => crate::Error::NotFound {
-                    source: Box::new(source),
-                    path: location.to_string(),
-                },
+                // Some stores return METHOD_NOT_ALLOWED for get on directories
+                Some(StatusCode::NOT_FOUND | StatusCode::METHOD_NOT_ALLOWED) => {
+                    crate::Error::NotFound {
+                        source: Box::new(source),
+                        path: location.to_string(),
+                    }
+                }
                 _ => Error::Request { source }.into(),
             })
     }

object_store/src/http/mod.rs

@@ -60,15 +60,6 @@ enum Error {
         url: String,
     },
 
-    #[snafu(display("Object is a directory"))]
-    IsDirectory,
-
-    #[snafu(display("PROPFIND response contained no valid objects"))]
-    NoObjects,
-
-    #[snafu(display("PROPFIND response contained more than one object"))]
-    MultipleObjects,
-
     #[snafu(display("Request error: {}", source))]
     Reqwest { source: reqwest::Error },
 }
@@ -134,12 +125,17 @@ impl ObjectStore for HttpStore {
                 let response = status.response.into_iter().next().unwrap();
                 response.check_ok()?;
                 match response.is_dir() {
-                    true => Err(Error::IsDirectory.into()),
+                    true => Err(crate::Error::NotFound {
+                        path: location.to_string(),
+                        source: "Is directory".to_string().into(),
+                    }),
                     false => response.object_meta(self.client.base_url()),
                 }
             }
-            0 => Err(Error::NoObjects.into()),
-            _ => Err(Error::MultipleObjects.into()),
+            x => Err(crate::Error::NotFound {
+                path: location.to_string(),
+                source: format!("Expected 1 result, got {x}").into(),
+            }),
         }
     }
 

object_store/src/lib.rs

@@ -880,6 +880,14 @@ mod tests {
         assert_eq!(result.common_prefixes.len(), 1);
         assert_eq!(result.common_prefixes[0], Path::from("test_dir"));
 
+        // Should return not found
+        let err = storage.get(&Path::from("test_dir")).await.unwrap_err();
+        assert!(matches!(err, crate::Error::NotFound { .. }), "{}", err);
+
+        // Should return not found
+        let err = storage.head(&Path::from("test_dir")).await.unwrap_err();
+        assert!(matches!(err, crate::Error::NotFound { .. }), "{}", err);
+
         // List everything starting with a prefix that should return results
         let prefix = Path::from("test_dir");
         let content_list = flatten_list_stream(storage, Some(&prefix)).await.unwrap();

object_store/src/local.rs

@@ -419,18 +419,23 @@ impl ObjectStore for LocalFileSystem {
 
         maybe_spawn_blocking(move || {
             let metadata = match metadata(&path) {
-                Err(e) => Err(if e.kind() == ErrorKind::NotFound {
-                    Error::NotFound {
+                Err(e) => Err(match e.kind() {
+                    ErrorKind::NotFound => Error::NotFound {
                         path: path.clone(),
                         source: e,
-                    }
-                } else {
-                    Error::Metadata {
+                    },
+                    _ => Error::Metadata {
                         source: e.into(),
                         path: location.to_string(),
-                    }
+                    },
                 }),
-                Ok(m) => Ok(m),
+                Ok(m) => match m.is_file() {
+                    true => Ok(m),
+                    false => Err(Error::NotFound {
+                        path,
+                        source: io::Error::new(ErrorKind::NotFound, "is not file"),
+                    }),
+                },
             }?;
             convert_metadata(metadata, location)
         })
@@ -878,19 +883,25 @@ fn read_range(file: &mut File, path: &PathBuf, range: Range<usize>) -> Result<By
 }
 
 fn open_file(path: &PathBuf) -> Result<File> {
-    let file = File::open(path).map_err(|e| {
-        if e.kind() == std::io::ErrorKind::NotFound {
-            Error::NotFound {
+    let file = match File::open(path).and_then(|f| Ok((f.metadata()?, f))) {
+        Err(e) => Err(match e.kind() {
+            ErrorKind::NotFound => Error::NotFound {
                 path: path.clone(),
                 source: e,
-            }
-        } else {
-            Error::UnableToOpenFile {
+            },
+            _ => Error::UnableToOpenFile {
                 path: path.clone(),
                 source: e,
-            }
-        }
-    })?;
+            },
+        }),
+        Ok((metadata, file)) => match metadata.is_file() {
+            true => Ok(file),
+            false => Err(Error::NotFound {
+                path: path.clone(),
+                source: io::Error::new(ErrorKind::NotFound, "not a file"),
+            }),
+        },
+    }?;
     Ok(file)
 }