Merge pull request #135 from mrc-ide/metadata-no-nl

Don't add a trailing newline to metadata files.

R/location.R

@@ -474,7 +474,7 @@ location_pull_metadata <- function(location_name, root, call) {
                     "Please let us know how this might have happened."),
           i = hint_remove),
         call)
-      writeLines(metadata[[i]], filename[[i]])
+      writeLines(metadata[[i]], filename[[i]], sep = "")
     }
   }
 

R/location_path.R

@@ -92,7 +92,7 @@ location_path_import_metadata <- function(str, hash, root) {
     root$files$get(meta$files$hash, dst, TRUE)
   }
 
-  writeLines(str, file.path(root$path, ".outpack", "metadata", id))
+  writeLines(str, file.path(root$path, ".outpack", "metadata", id), sep = "")
   time <- Sys.time()
   mark_packet_known(id, local, hash, time, root)
 }

R/outpack_insert.R

@@ -34,7 +34,7 @@ outpack_insert_packet <- function(path, json, root = NULL) {
   }
 
   path_meta <- file.path(root$path, ".outpack", "metadata", id)
-  writeLines(json, path_meta)
+  writeLines(json, path_meta, sep = "")
 
   ## TODO: once we get more flexible remotes, this will get moved into
   ## its own thing.

tests/testthat/test-location-path.R

@@ -319,3 +319,34 @@ test_that("Push single packet", {
   files_used <- lapply(id, function(id) client$index$metadata(id)$files$hash)
   expect_setequal(plan$files, unique(unlist(files_used, FALSE, FALSE)))
 })
+
+
+test_that("Can read metadata files with a trailing newline", {
+  # Past versions of orderly2 wrote metadata files with a trailing newline
+  # character, despite the fact that the newline was not included when hashing.
+  #
+  # This has been fixed by not writing the newline anymore, but for
+  # compatibility we need to ensure we can still read those metadata files and
+  # get a correct hash.
+
+  root <- create_temporary_root()
+  id <- create_random_packet(root)
+  path <- file.path(root$path, ".outpack", "metadata", id)
+
+  # Calling writeLines adds the trailing newline and mimicks the old orderly2
+  # behaviour. The size will be one or two bytes bigger than the actual data,
+  # depending on whether the newline is `\n` or `\r\n`.
+  old_size <- file.info(path)$size
+  writeLines(read_string(path), path)
+  expect_gte(file.info(path)$size, old_size + 1)
+
+  # Reading the metadata from a location at that path correctly strips the
+  # newline and hashes correctly.
+  loc <- orderly_location_path$new(root$path)
+  packets <- loc$list()
+  data <- loc$metadata(id)
+  expect_equal(nchar(data), old_size, ignore_attr = TRUE)
+
+  expected_hash <- packets[packets$packet == id]$hash
+  expect_no_error(hash_validate_data(data, expected_hash))
+})

tests/testthat/test-outpack-packet.R

@@ -823,3 +823,15 @@ test_that("can overwrite dependencies", {
     hash_file(file.path(path_src, "data.rds")),
     hash_file(file.path(root$path, "archive", "data", id, "data.rds")))
 })
+
+
+test_that("metadata files match their hash", {
+  root <- create_temporary_root()
+  id <- create_random_packet(root)
+
+  location <- root$index$location(local)
+  expected_hash <- location[location$packet == id]$hash
+
+  path <- file.path(root$path, ".outpack", "metadata", id)
+  expect_no_error(hash_validate_file(path, expected_hash))
+})

tests/testthat/test-util.R

@@ -321,3 +321,16 @@ test_that("can gracefully cope with rds save failure", {
     "some error")
   expect_equal(dir(tmp), character())
 })
+
+
+test_that("read_string strips newlines", {
+  path <- tempfile()
+  writeLines(c("", "12345678"), path)
+
+  # 8 characters, a leading newline and a trailing one
+  # Each newline may be one of two bytes each, depending on the platform.
+  expect_gte(file.info(path)$size, 10)
+
+  result <- expect_silent(read_string(path))
+  expect_equal(result, "12345678")
+})