From c3f4ead60088d9cd54da2243c66f18b744f31a39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20Li=C3=A9tar?= Date: Tue, 11 Feb 2025 00:43:26 +0000 Subject: [PATCH] Report an error when pushing conflicting metadata. Our packet IDs are generally assumed to be globally unique, but this is not guaranteed by construction: the ID is not a hash of its contents, and the metadata store is not content-addressed. This means one could try to upload two different packets to the store with the same ID. Previously, outpack server would accept the second request but skip writing to the file. It is better to detect this situation and to return an HTTP error when it occurs. --- src/metadata.rs | 89 ++++++++++++++++++++++++++++++------------------ src/metrics.rs | 14 ++++---- src/responses.rs | 1 + src/utils.rs | 64 ++++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 39 deletions(-) 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"); + } }