diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e416d1f..82a94f73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +### Fixed + +- A bug where trailing whitespace on otherwise valid JSON would fail to validate. + ## [0.1.5](https://github.com/acquire-project/acquire-driver-zarr/compare/v0.1.3...v0.1.4) - 2023-11-20 ### Added diff --git a/src/zarr.cpp b/src/zarr.cpp index f629e194..a5492f12 100644 --- a/src/zarr.cpp +++ b/src/zarr.cpp @@ -18,14 +18,12 @@ validate_json(const char* str, size_t nbytes) return; } - // Don't do full json validation here, but make sure it at least - // begins and ends with '{' and '}' - EXPECT(nbytes >= 3, - "nbytes (%d) is too small. Expected a null-terminated json string.", - (int)nbytes); - EXPECT(str[nbytes - 1] == '\0', "String must be null-terminated"); - EXPECT(str[0] == '{', "json string must start with \'{\'"); - EXPECT(str[nbytes - 2] == '}', "json string must end with \'}\'"); + json::parse(str, + str + nbytes, + nullptr, // callback + true, // allow exceptions + true // ignore comments + ); } /// \brief Get the filename from a StorageProperties as fs::path. diff --git a/src/zarr.v2.cpp b/src/zarr.v2.cpp index e57a49fe..7179148d 100644 --- a/src/zarr.v2.cpp +++ b/src/zarr.v2.cpp @@ -127,7 +127,15 @@ zarr::ZarrV2::write_external_metadata_() const using json = nlohmann::json; std::string zattrs_path = (dataset_root_ / "0" / ".zattrs").string(); - common::write_string(zattrs_path, external_metadata_json_); + std::string external_metadata = external_metadata_json_.empty() + ? "" + : json::parse(external_metadata_json_, + nullptr, // callback + true, // allow exceptions + true // ignore comments + ) + .dump(); + common::write_string(zattrs_path, external_metadata); } void diff --git a/src/zarr.v3.cpp b/src/zarr.v3.cpp index 7237147d..22950101 100644 --- a/src/zarr.v3.cpp +++ b/src/zarr.v3.cpp @@ -304,7 +304,11 @@ zarr::ZarrV3::write_group_metadata_() const json metadata; metadata["attributes"]["acquire"] = external_metadata_json_.empty() ? "" - : json::parse(external_metadata_json_); + : json::parse(external_metadata_json_, + nullptr, // callback + true, // allow exceptions + true // ignore comments + ); auto path = (dataset_root_ / "meta" / "root.group.json").string(); common::write_string(path, metadata.dump(4)); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 939600b4..b5a853f7 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -16,6 +16,7 @@ else () # set(tests list-devices + external-metadata-with-whitespace-ok get-meta unit-tests multiscale-with-trivial-tile-size diff --git a/tests/external-metadata-with-whitespace-ok.cpp b/tests/external-metadata-with-whitespace-ok.cpp new file mode 100644 index 00000000..912cdbc4 --- /dev/null +++ b/tests/external-metadata-with-whitespace-ok.cpp @@ -0,0 +1,87 @@ +/// @brief Check that setting external_metadata_json with trailing whitespace is +/// fine, actually. The old behavior was to check if the last character was '}', +/// but otherwise didn't validate JSON. This would fail if there was trailing +/// whitespace but otherwise had valid JSON. This test checks the new behavior, +/// which is to use nlohmann::json to parse the metadata. This has the added +/// benefit of actually validating the JSON. + +#include "acquire.h" +#include "device/hal/device.manager.h" +#include "logger.h" + +#include +#include +#include + +void +reporter(int is_error, + const char* file, + int line, + const char* function, + const char* msg) +{ + fprintf(is_error ? stderr : stdout, + "%s%s(%d) - %s: %s\n", + is_error ? "ERROR " : "", + file, + line, + function, + msg); +} + +/// Helper for passing size static strings as function args. +/// For a function: `f(char*,size_t)` use `f(SIZED("hello"))`. +/// Expands to `f("hello",5)`. +#define SIZED(str) str, sizeof(str) + +#define L (aq_logger) +#define LOG(...) L(0, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__) +#define ERR(...) L(1, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__) +#define EXPECT(e, ...) \ + do { \ + if (!(e)) { \ + char buf[1 << 8] = { 0 }; \ + ERR(__VA_ARGS__); \ + snprintf(buf, sizeof(buf) - 1, __VA_ARGS__); \ + throw std::runtime_error(buf); \ + } \ + } while (0) +#define CHECK(e) EXPECT(e, "Expression evaluated as false: %s", #e) +#define DEVOK(e) CHECK(Device_Ok == (e)) +#define OK(e) CHECK(AcquireStatus_Ok == (e)) + +void +setup(AcquireRuntime* runtime) +{ + CHECK(runtime); + auto dm = acquire_device_manager(runtime); + CHECK(dm); + + AcquireProperties props = {}; + OK(acquire_get_configuration(runtime, &props)); + + DEVOK(device_manager_select(dm, + DeviceKind_Camera, + SIZED("simulated.*empty.*") - 1, + &props.video[0].camera.identifier)); + DEVOK(device_manager_select(dm, + DeviceKind_Storage, + SIZED("Zarr") - 1, + &props.video[0].storage.identifier)); + + CHECK(1 == storage_properties_init( + &props.video[0].storage.settings, + 0, + SIZED(TEST ".zarr"), + SIZED(R"({"hello":"world"} )"), // note trailing whitespace + { 0 })); + OK(acquire_configure(runtime, &props)); +} + +int +main() +{ + auto runtime = acquire_init(reporter); + setup(runtime); + return 0; +} \ No newline at end of file