Use nlohman::json to validate (i.e., try-parse) external metadata. (#173

)

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

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.

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

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));

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

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 <cstdio>
+#include <stdexcept>
+#include <iostream>
+
+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;
+}