Refactor read_json to accept optioal callback and update related tests and functions

[Karan-Palan]

This PR refactors the read_json function to accept an optional ParseCallback parameter, enhancing the flexibility of JSON parsing. It also updates related function calls and definitions to align with this change as discussed in slack

[jviotti]

Unrelated changes?

[Karan-Palan]

The build fails because read_json has an incompatible function signature, taking an extra ParseCallback parameter while SchemaFlatFileResolver::Reader expects a function that takes only a std::filesystem::path. Hence these changes

[jviotti]

Unrelated changes?

[Karan-Palan]

The build fails because the function definition of SchemaFlatFileResolver::add in resolver.cc uses an rvalue reference (&&) for SchemaVisitorReference, while its declaration in jsonschema_resolver.h passes it by value, causing a mismatch, to avoid breaking changes, added this

[jviotti]

You don't need to explicitly pass it, as you set it to nullptr by default on the signature

[Karan-Palan]

I tried that, however it didn't work

[jviotti]

I left a few comments. The current CI failures are unrelated though. Our benchmark GitHub Action seems to be failing if coming from an external fork... I'll dig into it today.

[Karan-Palan]

Sure! I'll add the accidentally deleted comment after your review to save GitHub Action minutes. I've responded to all comments—the issue stemmed from function signature mismatches: read_json takes an extra ParseCallback parameter while SchemaFlatFileResolver::Reader expects a function with only a std::filesystem::path, and SchemaFlatFileResolver::add had a parameter mismatch between its definition and declaration. I had to update them accordingly. PTAL and let me know if there's a better approach!

[jviotti]

@Karan-Palan Just these changes work for me. I think the rest are indeed all unnecessary:

diff --git a/src/core/json/include/sourcemeta/core/json.h b/src/core/json/include/sourcemeta/core/json.h
index c901afb0..7ab73404 100644
--- a/src/core/json/include/sourcemeta/core/json.h
+++ b/src/core/json/include/sourcemeta/core/json.h
@@ -127,7 +127,8 @@ auto parse_json(const std::basic_string<JSON::Char, JSON::CharTraits> &input,
 ///
 /// If parsing fails, sourcemeta::core::JSONParseError will be thrown.
 SOURCEMETA_CORE_JSON_EXPORT
-auto read_json(const std::filesystem::path &path) -> JSON;
+auto read_json(const std::filesystem::path &path,
+               const JSON::ParseCallback &callback = nullptr) -> JSON;
 
 // TODO: Move this function to a system integration component, as it
 // is not JSON specific
diff --git a/src/core/json/json.cc b/src/core/json/json.cc
index ed50f604..eac8322f 100644
--- a/src/core/json/json.cc
+++ b/src/core/json/json.cc
@@ -50,10 +50,11 @@ auto read_file(const std::filesystem::path &path)
   return stream;
 }
 
-auto read_json(const std::filesystem::path &path) -> JSON {
+auto read_json(const std::filesystem::path &path,
+               const JSON::ParseCallback &callback) -> JSON {
   auto stream{read_file(path)};
   try {
-    return parse_json(stream);
+    return parse_json(stream, callback);
   } catch (const JSONParseError &error) {
     // For producing better error messages
     throw JSONFileParseError(path, error);
diff --git a/src/core/jsonschema/include/sourcemeta/core/jsonschema_resolver.h b/src/core/jsonschema/include/sourcemeta/core/jsonschema_resolver.h
index 582678be..0523ec39 100644
--- a/src/core/jsonschema/include/sourcemeta/core/jsonschema_resolver.h
+++ b/src/core/jsonschema/include/sourcemeta/core/jsonschema_resolver.h
@@ -105,11 +105,13 @@ public:
 
   /// Register a schema to the flat file resolver, returning the detected
   /// identifier for the schema
-  auto add(const std::filesystem::path &path,
-           const std::optional<std::string> &default_dialect = std::nullopt,
-           const std::optional<std::string> &default_id = std::nullopt,
-           const Reader &reader = read_json,
-           SchemaVisitorReference &&reference_visitor = nullptr)
+  auto add(
+      const std::filesystem::path &path,
+      const std::optional<std::string> &default_dialect = std::nullopt,
+      const std::optional<std::string> &default_id = std::nullopt,
+      const Reader &reader =
+          [](const std::filesystem::path &path) { return read_json(path); },
+      SchemaVisitorReference &&reference_visitor = nullptr)
       -> const std::string &;
 
   // Change the identifier of a registered schema
diff --git a/test/jsonschema/jsonschema_flat_file_resolver_test.cc b/test/jsonschema/jsonschema_flat_file_resolver_test.cc
index 21ae91e2..49159ab2 100644
--- a/test/jsonschema/jsonschema_flat_file_resolver_test.cc
+++ b/test/jsonschema/jsonschema_flat_file_resolver_test.cc
@@ -357,7 +357,10 @@ TEST(JSONSchema_SchemaFlatFileResolver, custom_reference_visitor) {
   })JSON");
 
   const auto &identifier{resolver.add(
-      schema_path, std::nullopt, std::nullopt, sourcemeta::core::read_json,
+      schema_path, std::nullopt, std::nullopt,
+      [](const std::filesystem::path &path) {
+        return sourcemeta::core::read_json(path);
+      },
       [](sourcemeta::core::JSON &schema, const sourcemeta::core::URI &,
          const sourcemeta::core::JSON::String &,
          const sourcemeta::core::JSON::String &keyword,

[jviotti]

Also, can you add a tiny test here exercising the new callback variant? https://github.com/sourcemeta/core/blob/db9f42ffcf7f82f7097b3d6d85e09571d702e194/test/json/json_parse_callback_test.cc

[Karan-Palan]

@Karan-Palan Just these changes work for me. I think the rest are indeed all unnecessary:

Done. I also added a test where writes out a small JSON file (in this example just "true") to a temporary location, then calls read_json with a callback that captures the parse “traces,” and finally checks both the resulting JSON and the parse events. PTAL

[jviotti]

I added a commit to make the test case use an existing simple JSON stub we already had in the repo, but other than that, looks good. Thanks!

[Karan-Palan]

I added a commit to make the test case use an existing simple JSON stub we already had in the repo, but other than that, looks good. Thanks!

Ahh I see, it's more maintainable too. Thanks, I'm getting to learn alot about c++ ecosystem and projects whilst contributing!