diff --git a/src/metadata.rs b/src/metadata.rs index 89d3a4a..5815baa 100644 --- a/src/metadata.rs +++ b/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(); diff --git a/src/metrics.rs b/src/metrics.rs index 2ef4214..4308b4a 100644 --- a/src/metrics.rs +++ b/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] diff --git a/src/responses.rs b/src/responses.rs index 78cee1b..20e8781 100644 --- a/src/responses.rs +++ b/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, }; diff --git a/src/utils.rs b/src/utils.rs index 5fa6868..e152bf1 100644 --- a/src/utils.rs +++ b/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"); + } }