Report an error when pushing conflicting metadata.

src/metadata.rs

@@ -251,34 +251,22 @@ fn check_missing_dependencies(root: &Path, packet: &Packet) -> Result<(), io::Er
     Ok(())
 }
 
-fn add_parsed_metadata(root: &Path, data: &str, packet: &Packet, hash: &str) -> io::Result<()> {
-    hash::validate_hash_data(data.as_bytes(), hash).map_err(hash::hash_error_to_io_error)?;
-    let path = get_path(root, &packet.id);
-    if !path.exists() {
-        fs::File::create(&path)?;
-        fs::write(path, data)?;
-    }
-    Ok(())
-}
-
-/// Add metadata to the repository.
-#[cfg(test)] // Only used from tests at the moment.
-pub fn add_metadata(root: &Path, data: &str, hash: &hash::Hash) -> io::Result<()> {
-    let packet: Packet = serde_json::from_str(data)?;
-    add_parsed_metadata(root, data, &packet, &hash.to_string())
-}
-
 /// Add a packet to the repository.
 ///
 /// The packet's files and dependencies must already be present in the repository.
+///
+/// Returns an error if a packet with the same ID but different metadata hash exists in the
+/// repository.
 pub fn add_packet(root: &Path, data: &str, hash: &hash::Hash) -> io::Result<()> {
     let packet: Packet = serde_json::from_str(data)?;
     let hash_str = hash.to_string();
 
+    hash::validate_hash_data(data.as_bytes(), &hash_str).map_err(hash::hash_error_to_io_error)?;
+
     check_missing_files(root, &packet)?;
     check_missing_dependencies(root, &packet)?;
 
-    add_parsed_metadata(root, data, &packet, &hash.to_string())?;
+    utils::write_file_idempotent(&get_path(root, &packet.id), data.as_bytes())?;
 
     let time = SystemTime::now();
     location::mark_packet_known(&packet.id, "local", &hash_str, time, root)?;
@@ -485,6 +473,56 @@ mod tests {
         add_packet(&root, data, &hash).unwrap();
     }
 
+    #[test]
+    fn add_packet_detects_conflict() {
+        // Same contents, but different timestamps
+        let data1 = r#"{
+            "schema_version": "0.0.1",
+            "name": "computed-resource",
+            "id": "20230427-150828-68772cee",
+            "time": {
+              "start": 1680000000.0000,
+              "end": 1680001000.0000
+            },
+            "parameters": null,
+            "files": [],
+            "depends": [],
+            "script": [
+              "orderly.R"
+            ]
+        }"#;
+        let data2 = r#"{
+            "schema_version": "0.0.1",
+            "name": "computed-resource",
+            "id": "20230427-150828-68772cee",
+            "time": {
+              "start": 1690000000.0000,
+              "end": 1690001000.0000
+            },
+            "parameters": null,
+            "files": [],
+            "depends": [],
+            "script": [
+              "orderly.R"
+            ]
+        }"#;
+
+        let hash1 = hash::hash_data(data1.as_bytes(), hash::HashAlgorithm::Sha256);
+        let hash2 = hash::hash_data(data2.as_bytes(), hash::HashAlgorithm::Sha256);
+
+        assert_ne!(hash1, hash2);
+
+        let root = get_temp_outpack_root();
+        add_packet(&root, data1, &hash1).unwrap();
+
+        let error = add_packet(&root, data2, &hash2).unwrap_err();
+        assert_eq!(error.kind(), io::ErrorKind::AlreadyExists);
+
+        // Check that the metadata did not get overwritten.
+        let packet = get_metadata_text(&root, "20230427-150828-68772cee").unwrap();
+        assert_eq!(packet, data1);
+    }
+
     #[test]
     fn imported_metadata_is_added_to_local_location() {
         let data = r#"{
@@ -521,21 +559,6 @@ mod tests {
         assert!(entry.time >= time_as_num(now));
     }
 
-    #[test]
-    fn can_add_metadata_with_missing_files() {
-        let root = get_temp_outpack_root();
-
-        let file_hash = "sha256:c7b512b2d14a7caae8968830760cb95980a98e18ca2c2991b87c71529e223164";
-
-        assert!(!file_exists(&root, file_hash).unwrap());
-
-        let (_, metadata, hash) = start_packet("data")
-            .add_file("data.csv", file_hash, 51)
-            .finish();
-
-        add_metadata(&root, &metadata, &hash).unwrap();
-    }
-
     #[test]
     fn cannot_add_packet_with_missing_files() {
         let root = get_temp_outpack_root();

src/metrics.rs

@@ -259,7 +259,7 @@ mod tests {
     use super::*;
     use crate::hash::hash_data;
     use crate::hash::HashAlgorithm;
-    use crate::metadata::{add_metadata, add_packet};
+    use crate::metadata::add_packet;
     use crate::store::put_file;
     use crate::test_utils::tests::{get_empty_outpack_root, start_packet};
 
@@ -307,18 +307,20 @@ mod tests {
         let root = get_empty_outpack_root();
         let collector = RepositoryMetrics::new(&root);
 
-        // Create two different packets.
-        // One of them is actually added to the repository.
-        // We have the metadata for the second one, but it is missing from the repo.
         let (_, packet1, hash1) = start_packet("hello").finish();
         let (_, packet2, hash2) = start_packet("hello").finish();
 
         add_packet(&root, &packet1, &hash1).unwrap();
-        add_metadata(&root, &packet2, &hash2).unwrap();
 
         collector.update().unwrap();
-        assert_eq!(collector.metadata_total.get(), 2);
+        assert_eq!(collector.metadata_total.get(), 1);
         assert_eq!(collector.packets_total.get(), 1);
+
+        add_packet(&root, &packet2, &hash2).unwrap();
+
+        collector.update().unwrap();
+        assert_eq!(collector.metadata_total.get(), 2);
+        assert_eq!(collector.packets_total.get(), 2);
     }
 
     #[tokio::test]

src/responses.rs

@@ -97,6 +97,7 @@ impl axum::response::IntoResponse for OutpackError {
             Some(ErrorKind::NotFound) => StatusCode::NOT_FOUND,
             Some(ErrorKind::InvalidInput) => StatusCode::BAD_REQUEST,
             Some(ErrorKind::UnexpectedEof) => StatusCode::BAD_REQUEST,
+            Some(ErrorKind::AlreadyExists) => StatusCode::CONFLICT,
             _ => StatusCode::INTERNAL_SERVER_ERROR,
         };
 

src/utils.rs

@@ -2,7 +2,9 @@ use cached::instant::SystemTime;
 use lazy_static::lazy_static;
 use regex::Regex;
 use std::ffi::OsString;
+use std::path::Path;
 use std::time::UNIX_EPOCH;
+use std::{fs, io, io::Write};
 
 lazy_static! {
     static ref ID_REG: Regex = Regex::new(r"^([0-9]{8}-[0-9]{6}-[[:xdigit:]]{8})$").unwrap();
@@ -20,10 +22,34 @@ pub fn time_as_num(time: SystemTime) -> f64 {
     (time.duration_since(UNIX_EPOCH).unwrap().as_millis() as f64) / 1000.0
 }
 
+/// Write a byte slice to disk.
+///
+/// Succeeds if the file already exists with identical contents.
+/// On the other hand, an AlreadyExists error is returned if the file exists with different contents.
+pub fn write_file_idempotent(path: &Path, contents: &[u8]) -> io::Result<()> {
+    match fs::OpenOptions::new()
+        .write(true)
+        .create_new(true)
+        .open(path)
+    {
+        Ok(mut f) => {
+            f.write_all(contents)?;
+        }
+        Err(err) if err.kind() == io::ErrorKind::AlreadyExists => {
+            if fs::read(path)? != contents {
+                return Err(err);
+            }
+        }
+        Err(err) => return Err(err),
+    }
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
     use std::time::Duration;
+    use tempfile::tempdir;
 
     #[test]
     fn can_detect_packet_id() {
@@ -39,4 +65,42 @@ mod tests {
         let res = time_as_num(time);
         assert_eq!(res, 1688033668.123);
     }
+
+    #[test]
+    fn write_file_idempotent_writes_to_disk() {
+        let base = tempdir().unwrap();
+        let path = base.path().join("hello.txt");
+
+        write_file_idempotent(&path, b"Hello").unwrap();
+
+        let contents = fs::read(path).unwrap();
+        assert_eq!(contents, b"Hello");
+    }
+
+    #[test]
+    fn write_file_idempotent_succeeds_if_content_is_identical() {
+        let base = tempdir().unwrap();
+        let path = base.path().join("hello.txt");
+
+        fs::write(&path, b"Hello").unwrap();
+
+        write_file_idempotent(&path, b"Hello").unwrap();
+
+        let contents = fs::read(path).unwrap();
+        assert_eq!(contents, b"Hello");
+    }
+
+    #[test]
+    fn write_file_idempotent_succeeds_if_content_is_different() {
+        let base = tempdir().unwrap();
+        let path = base.path().join("hello.txt");
+
+        fs::write(&path, b"Hello").unwrap();
+
+        let error = write_file_idempotent(&path, b"World").unwrap_err();
+        assert_eq!(error.kind(), io::ErrorKind::AlreadyExists);
+
+        let contents = fs::read(path).unwrap();
+        assert_eq!(contents, b"Hello");
+    }
 }