From 516174f42988eb232377fe2b231df1c7a517d6f6 Mon Sep 17 00:00:00 2001
From: Mike Smith <grimbough@gmail.com>
Date: Thu, 24 Oct 2024 17:14:09 +0200
Subject: [PATCH] Fix bug in update_zarr_array and add unit test

---
 NEWS.md                          |  3 +++
 R/write_data.R                   |  1 +
 inst/tinytest/test_update_zarr.R | 14 +++++++++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/NEWS.md b/NEWS.md
index f915dc8..f92843b 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -3,6 +3,9 @@
 * Fixed bug when creating an empty array with a floating datatype.  The fill
   value would be interpreted as an integer by `read_metadata()` and create 
   and array of the wrong type.
+* Fixed bug in `update_zarr_array()` when `NULL` was provided to one or more
+  dimensions in the `index` argument.  This was parsed incorrectly and the 
+  underlying zarr was not modified.
 * Added support for the ZarrArray S4 class and the DelayedArray framework.
 * Improvements to read and write performance.
 
diff --git a/R/write_data.R b/R/write_data.R
index 6c373ee..9a81dbb 100644
--- a/R/write_data.R
+++ b/R/write_data.R
@@ -272,6 +272,7 @@ update_zarr_array <- function(zarr_array_path, x, index) {
 
   zarr_array_path <- .normalize_array_path(zarr_array_path)
   metadata <- read_array_metadata(zarr_array_path)
+  index <- check_index(index, metadata = metadata)
 
   data_type <- switch(storage.mode(x),
     "integer" = "<i",
diff --git a/inst/tinytest/test_update_zarr.R b/inst/tinytest/test_update_zarr.R
index 7d39a1a..cd537ec 100644
--- a/inst/tinytest/test_update_zarr.R
+++ b/inst/tinytest/test_update_zarr.R
@@ -14,5 +14,17 @@ expect_identical( list.files(path), "0")
 x <- rep(20L, 5);
 expect_true(update_zarr_array(path, x = x, index = list( c(91, 93, 95, 97, 99)  )))
 expect_identical(read_zarr_array(path)[91:100], array(rep(c(20L, 100L), 5), dim = 10))
-## only a single chunk file should have been created
+## only two chunk files should have been created
 expect_true( length(list.files(path)) == 2 )
+
+
+## Test we can update regions where we provide NULL to the index argument for some dimensions
+path <- tempfile()
+res <- create_empty_zarr_array(zarr_array_path = path, 
+                               dim = c(20, 20), chunk_dim = c(10, 10), data_type = "integer", 
+                               fill_value = 0L)
+x <- matrix(1:40L, ncol = 2)
+index <- list(NULL, 1:2)
+
+expect_true(update_zarr_array(path, x = x, index = index))
+expect_identical(read_zarr_array(path, index = index), x)