fix: Array parsing in new config (#1896)

Improving array parsing in config:
- Allow null values in arrays for optional fields
- Allow empty array even for required field
- Allow to not put an empty array in config even if array contains
required fields

tests/unit/LoggerTests.cpp

@@ -80,8 +80,8 @@ using util::config::ConfigValue;
 struct LoggerInitTest : LoggerTest {
 protected:
     util::config::ClioConfigDefinition config_{
-        {"log_channels.[].channel", Array{ConfigValue{ConfigType::String}.optional()}},
-        {"log_channels.[].log_level", Array{ConfigValue{ConfigType::String}.optional()}},
+        {"log_channels.[].channel", Array{ConfigValue{ConfigType::String}}},
+        {"log_channels.[].log_level", Array{ConfigValue{ConfigType::String}}},
 
         {"log_level", ConfigValue{ConfigType::String}.defaultValue("info")},
 
@@ -181,9 +181,15 @@ TEST_F(LoggerInitTest, LogSizeAndHourRotationCannotBeZero)
         "log_rotation_hour_interval", "log_directory_max_size", "log_rotation_size"
     };
 
+    auto const jsonStr = fmt::format(R"json({{
+        "{}": 0,
+        "{}": 0,
+        "{}": 0
+    }})json", keys[0], keys[1], keys[2]);
+
     auto const parsingErrors =
-        config_.parse(ConfigFileJson{boost::json::object{{keys[0], 0}, {keys[1], 0}, {keys[2], 0}}});
-    ASSERT_TRUE(parsingErrors->size() == 3);
+        config_.parse(ConfigFileJson{boost::json::parse(jsonStr).as_object()});
+    ASSERT_EQ(parsingErrors->size(), 3);
     for (std::size_t i = 0; i < parsingErrors->size(); ++i) {
         EXPECT_EQ(
             (*parsingErrors)[i].error,

tests/unit/util/newconfig/ArrayTests.cpp

@@ -46,16 +46,16 @@ TEST(ArrayDeathTest, prefix)
 TEST(ArrayTest, addSingleValue)
 {
     auto arr = Array{ConfigValue{ConfigType::Double}};
-    arr.addValue(111.11);
+    ASSERT_FALSE(arr.addValue(111.11));
     EXPECT_EQ(arr.size(), 1);
 }
 
 TEST(ArrayTest, addAndCheckMultipleValues)
 {
     auto arr = Array{ConfigValue{ConfigType::Double}};
-    arr.addValue(111.11);
-    arr.addValue(222.22);
-    arr.addValue(333.33);
+    ASSERT_FALSE(arr.addValue(111.11));
+    ASSERT_FALSE(arr.addValue(222.22));
+    ASSERT_FALSE(arr.addValue(333.33));
     EXPECT_EQ(arr.size(), 3);
 
     auto const cv = arr.at(0);
@@ -67,7 +67,7 @@ TEST(ArrayTest, addAndCheckMultipleValues)
     EXPECT_EQ(vv2.asDouble(), 222.22);
 
     EXPECT_EQ(arr.size(), 3);
-    arr.addValue(444.44);
+    ASSERT_FALSE(arr.addValue(444.44));
 
     EXPECT_EQ(arr.size(), 4);
     auto const cv4 = arr.at(3);
@@ -88,7 +88,7 @@ TEST(ArrayTest, iterateValueArray)
     std::vector<int64_t> const expected{543, 123, 909};
 
     for (auto const num : expected)
-        arr.addValue(num);
+        ASSERT_FALSE(arr.addValue(num));
 
     std::vector<int64_t> actual;
     for (auto it = arr.begin(); it != arr.end(); ++it)
@@ -96,3 +96,34 @@ TEST(ArrayTest, iterateValueArray)
 
     EXPECT_TRUE(std::ranges::equal(expected, actual));
 }
+
+TEST(ArrayTest, addNullOptional)
+{
+    Array arr{ConfigValue{ConfigType::Integer}.optional()};
+    ASSERT_FALSE(arr.addNull());
+    ASSERT_FALSE(arr.addValue(1));
+
+    ASSERT_EQ(arr.size(), 2);
+    EXPECT_FALSE(arr.at(0).hasValue());
+    EXPECT_TRUE(arr.at(1).hasValue());
+    EXPECT_EQ(std::get<int64_t>(arr.at(1).getValue()), 1);
+}
+
+TEST(ArrayTest, addNullDefault)
+{
+    Array arr{ConfigValue{ConfigType::Integer}.defaultValue(42)};
+    ASSERT_FALSE(arr.addNull());
+    ASSERT_FALSE(arr.addValue(1));
+
+    ASSERT_EQ(arr.size(), 2);
+    EXPECT_EQ(std::get<int64_t>(arr.at(0).getValue()), 42);
+    EXPECT_EQ(std::get<int64_t>(arr.at(1).getValue()), 1);
+}
+
+TEST(ArrayTest, addNullRequired)
+{
+    Array arr{ConfigValue{ConfigType::Integer}};
+    auto const error = arr.addNull();
+    EXPECT_TRUE(error.has_value());
+}
+

tests/unit/util/newconfig/ClioConfigDefinitionTests.cpp

@@ -17,10 +17,13 @@
 */
 //==============================================================================
 
+#include "util/LoggerFixtures.hpp"
+#include "util/newconfig/Array.hpp"
 #include "util/newconfig/ArrayView.hpp"
 #include "util/newconfig/ConfigDefinition.hpp"
 #include "util/newconfig/ConfigDescription.hpp"
 #include "util/newconfig/ConfigFileJson.hpp"
+#include "util/newconfig/ConfigValue.hpp"
 #include "util/newconfig/FakeConfigData.hpp"
 #include "util/newconfig/Types.hpp"
 #include "util/newconfig/ValueView.hpp"
@@ -29,10 +32,12 @@
 #include <boost/json/parse.hpp>
 #include <boost/json/value.hpp>
 #include <gtest/gtest.h>
+#include <gmock/gmock.h>
 
 #include <algorithm>
 #include <cstdint>
 #include <optional>
+#include <set>
 #include <string>
 #include <string_view>
 #include <unordered_set>
@@ -304,19 +309,144 @@ TEST_F(IncorrectOverrideValues, InvalidJsonErrors)
     EXPECT_TRUE(errors.has_value());
 
     // Expected error messages
-    std::unordered_set<std::string_view> const expectedErrors{
+    std::set<std::string_view> const expectedErrors{
         "dosguard.whitelist.[] value does not match type string",
-        "higher.[].low.section key is required in user Config",
-        "higher.[].low.admin key is required in user Config",
-        "array.[].sub key is required in user Config",
         "header.port value does not match type integer",
         "header.admin value does not match type boolean",
         "optional.withDefault value does not match type double"
     };
 
-    std::unordered_set<std::string_view> actualErrors;
+    std::set<std::string_view> actualErrors;
     for (auto const& error : errors.value()) {
         actualErrors.insert(error.error);
     }
     EXPECT_EQ(expectedErrors, actualErrors);
 }
+
+struct ClioConfigDefinitionParseArrayTest : NoLoggerFixture {
+    ClioConfigDefinition config{
+        {"array.[].int", Array{ConfigValue{ConfigType::Integer}}},
+        {"array.[].string", Array{ConfigValue{ConfigType::String}.optional()}}
+    };
+};
+
+TEST_F(ClioConfigDefinitionParseArrayTest, emptyArray)
+{
+    auto const configJson = boost::json::parse(R"json({
+        "array": []
+    })json").as_object();
+
+    auto const result = config.parse(ConfigFileJson{configJson});
+    EXPECT_FALSE(result.has_value());
+}
+
+TEST_F(ClioConfigDefinitionParseArrayTest, emptyJson)
+{
+    auto const configJson = boost::json::object{};
+
+    auto const result = config.parse(ConfigFileJson{configJson});
+    EXPECT_FALSE(result.has_value());
+}
+
+TEST_F(ClioConfigDefinitionParseArrayTest, fullArray)
+{
+    auto const configJson = boost::json::parse(R"json({
+        "array": [
+            {"int": 1, "string": "one"},
+            {"int": 2, "string": "two"}
+        ]
+    })json").as_object();
+
+    auto const result = config.parse(ConfigFileJson{configJson});
+    EXPECT_FALSE(result.has_value());
+    EXPECT_EQ(config.arraySize("array.[]"), 2);
+}
+
+TEST_F(ClioConfigDefinitionParseArrayTest, onlyRequiredFields) {
+    auto const configJson = boost::json::parse(R"json({
+        "array": [
+            {"int": 1},
+            {"int": 2}
+        ]
+    })json").as_object();
+
+    auto const configFile = ConfigFileJson{configJson};
+    auto const result = config.parse(configFile);
+    ASSERT_FALSE(result.has_value());
+    EXPECT_EQ(config.arraySize("array.[]"), 2);
+
+    EXPECT_EQ(config.getArray("array.[].int").valueAt(0).asIntType<int>(), 1);
+    EXPECT_EQ(config.getArray("array.[].int").valueAt(1).asIntType<int>(), 2);
+    EXPECT_FALSE(config.getArray("array.[].string").valueAt(0).hasValue());
+    EXPECT_FALSE(config.getArray("array.[].string").valueAt(1).hasValue());
+}
+
+TEST_F(ClioConfigDefinitionParseArrayTest, someOptionalFieldsMissing)
+{
+    auto const configJson = boost::json::parse(R"json({
+        "array": [
+            {"int": 1, "string": "one"},
+            {"int": 2}
+        ]
+    })json").as_object();
+
+    auto const configFile = ConfigFileJson{configJson};
+    auto const result = config.parse(configFile);
+    ASSERT_FALSE(result.has_value());
+    EXPECT_EQ(config.arraySize("array.[]"), 2);
+
+    EXPECT_EQ(config.getArray("array.[].int").valueAt(0).asIntType<int>(), 1);
+    EXPECT_EQ(config.getArray("array.[].int").valueAt(1).asIntType<int>(), 2);
+    EXPECT_EQ(config.getArray("array.[].string").valueAt(0).asString(), "one");
+    EXPECT_FALSE(config.getArray("array.[].string").valueAt(1).hasValue());
+}
+
+TEST_F(ClioConfigDefinitionParseArrayTest, optionalFieldMissingAtFirstPosition) {
+    auto const configJson = boost::json::parse(R"json({
+        "array": [
+            {"int": 1},
+            {"int": 2, "string": "two"}
+        ]
+    })json").as_object();
+
+    auto const configFile = ConfigFileJson{configJson};
+    auto const result = config.parse(configFile);
+    ASSERT_FALSE(result.has_value());
+    EXPECT_EQ(config.arraySize("array.[]"), 2);
+
+    EXPECT_EQ(config.getArray("array.[].int").valueAt(0).asIntType<int>(), 1);
+    EXPECT_EQ(config.getArray("array.[].int").valueAt(1).asIntType<int>(), 2);
+
+    EXPECT_FALSE(config.getArray("array.[].string").valueAt(0).hasValue());
+    EXPECT_EQ(config.getArray("array.[].string").valueAt(1).asString(), "two");
+}
+
+TEST_F(ClioConfigDefinitionParseArrayTest, missingRequiredFields) {
+    auto const configJson = boost::json::parse(R"json({
+        "array": [
+            {"int": 1},
+            {"string": "two"}
+        ]
+    })json").as_object();
+
+    auto const configFile = ConfigFileJson{configJson};
+    auto const result = config.parse(configFile);
+    ASSERT_TRUE(result.has_value());
+    EXPECT_EQ(result->size(), 1);
+    EXPECT_THAT(result->at(0).error, testing::StartsWith("array.[].int"));
+}
+
+TEST_F(ClioConfigDefinitionParseArrayTest, missingAllRequiredFields) {
+    auto const configJson = boost::json::parse(R"json({
+        "array": [
+            {"string": "one"},
+            {"string": "two"}
+        ]
+    })json").as_object();
+
+    auto const configFile = ConfigFileJson{configJson};
+    auto const result = config.parse(configFile);
+    ASSERT_TRUE(result.has_value());
+    EXPECT_EQ(result->size(), 1);
+    EXPECT_THAT(result->at(0).error, testing::StartsWith("array.[].int"));
+}

tests/unit/util/newconfig/ConfigFileJsonTests.cpp

@@ -22,7 +22,6 @@
 #include "util/OverloadSet.hpp"
 #include "util/TmpFile.hpp"
 #include "util/newconfig/ConfigFileJson.hpp"
-#include "util/newconfig/Types.hpp"
 
 #include <boost/json/array.hpp>
 #include <boost/json/object.hpp>
@@ -316,8 +315,7 @@ TEST_F(ConfigFileJsonTest, getValue)
         "int": 42,
         "object": { "string": "some string" },
         "bool": true,
-        "double": 123.456,
-        "null": null
+        "double": 123.456
     })json";
     auto const jsonFileObj = ConfigFileJson{boost::json::parse(jsonStr).as_object()};
 
@@ -337,9 +335,6 @@ TEST_F(ConfigFileJsonTest, getValue)
     ASSERT_TRUE(std::holds_alternative<double>(doubleValue));
     EXPECT_NEAR(std::get<double>(doubleValue), 123.456, kEPS);
 
-    auto const nullValue = jsonFileObj.getValue("null");
-    EXPECT_TRUE(std::holds_alternative<NullType>(nullValue));
-
     EXPECT_FALSE(jsonFileObj.containsKey("object.int"));
 }
 
@@ -360,6 +355,15 @@ TEST_F(ConfigFileJsonDeathTest, getValueOfArray)
     EXPECT_DEATH([[maybe_unused]] auto a = jsonFileObj.getValue("array"), ".*");
 }
 
+TEST_F(ConfigFileJsonDeathTest, nullIsNotSupported)
+{
+    auto const jsonStr = R"json({
+        "null": null
+    })json";
+    auto const jsonFileObj = ConfigFileJson{boost::json::parse(jsonStr).as_object()};
+    EXPECT_DEATH([[maybe_unused]] auto a = jsonFileObj.getValue("null"), ".*");
+}
+
 TEST_F(ConfigFileJsonTest, getArray)
 {
     auto const jsonStr = R"json({
@@ -370,19 +374,27 @@ TEST_F(ConfigFileJsonTest, getArray)
 
     auto const array = jsonFileObj.getArray("array.[]");
     ASSERT_EQ(array.size(), 4);
-    ASSERT_TRUE(std::holds_alternative<int64_t>(array.at(0)));
-    EXPECT_EQ(std::get<int64_t>(array.at(0)), 1);
-    ASSERT_TRUE(std::holds_alternative<std::string>(array.at(1)));
-    EXPECT_EQ(std::get<std::string>(array.at(1)), "2");
-    ASSERT_TRUE(std::holds_alternative<double>(array.at(2)));
-    EXPECT_NEAR(std::get<double>(array.at(2)), 3.14, kEPS);
-    ASSERT_TRUE(std::holds_alternative<bool>(array.at(3)));
-    EXPECT_EQ(std::get<bool>(array.at(3)), true);
+
+    auto const value0 = array.at(0).value();
+    ASSERT_TRUE(std::holds_alternative<int64_t>(value0));
+    EXPECT_EQ(std::get<int64_t>(value0), 1);
+
+    auto const value1 = array.at(1).value();
+    ASSERT_TRUE(std::holds_alternative<std::string>(value1));
+    EXPECT_EQ(std::get<std::string>(value1), "2");
+
+    auto const value2 = array.at(2).value();
+    ASSERT_TRUE(std::holds_alternative<double>(value2));
+    EXPECT_NEAR(std::get<double>(value2), 3.14, kEPS);
+
+    auto const value3 = array.at(3).value();
+    ASSERT_TRUE(std::holds_alternative<bool>(value3));
+    EXPECT_EQ(std::get<bool>(value3), true);
 
     auto const arrayFromObject = jsonFileObj.getArray("object.array.[]");
     ASSERT_EQ(arrayFromObject.size(), 2);
-    EXPECT_EQ(std::get<int64_t>(arrayFromObject.at(0)), 3);
-    EXPECT_EQ(std::get<int64_t>(arrayFromObject.at(1)), 4);
+    EXPECT_EQ(std::get<int64_t>(arrayFromObject.at(0).value()), 3);
+    EXPECT_EQ(std::get<int64_t>(arrayFromObject.at(1).value()), 4);
 }
 
 TEST_F(ConfigFileJsonTest, getArrayObjectInArray)
@@ -397,15 +409,38 @@ TEST_F(ConfigFileJsonTest, getArrayObjectInArray)
 
     auto const ints = jsonFileObj.getArray("array.[].int");
     ASSERT_EQ(ints.size(), 2);
-    ASSERT_TRUE(std::holds_alternative<int64_t>(ints.at(0)));
-    EXPECT_EQ(std::get<int64_t>(ints.at(0)), 42);
-    EXPECT_TRUE(std::holds_alternative<NullType>(ints.at(1)));
+    ASSERT_TRUE(std::holds_alternative<int64_t>(ints.at(0).value()));
+    EXPECT_EQ(std::get<int64_t>(ints.at(0).value()), 42);
+    EXPECT_FALSE(ints.at(1).has_value());
 
     auto const strings = jsonFileObj.getArray("array.[].string");
     ASSERT_EQ(strings.size(), 2);
-    EXPECT_TRUE(std::holds_alternative<NullType>(strings.at(0)));
-    ASSERT_TRUE(std::holds_alternative<std::string>(strings.at(1)));
-    EXPECT_EQ(std::get<std::string>(strings.at(1)), "some string");
+    EXPECT_FALSE(strings.at(0).has_value());
+    ASSERT_TRUE(std::holds_alternative<std::string>(strings.at(1).value()));
+    EXPECT_EQ(std::get<std::string>(strings.at(1).value()), "some string");
+}
+
+TEST_F(ConfigFileJsonTest, getArrayOptionalInArray) {
+    auto const jsonStr = R"json({
+        "array": [
+            { "int": 42 },
+            { "int": 24, "bool": true }
+        ]
+    })json";
+    auto const jsonFileObj = ConfigFileJson{boost::json::parse(jsonStr).as_object()};
+
+    auto const ints = jsonFileObj.getArray("array.[].int");
+    ASSERT_EQ(ints.size(), 2);
+    ASSERT_TRUE(std::holds_alternative<int64_t>(ints.at(0).value()));
+    EXPECT_EQ(std::get<int64_t>(ints.at(0).value()), 42);
+    ASSERT_TRUE(std::holds_alternative<int64_t>(ints.at(1).value()));
+    EXPECT_EQ(std::get<int64_t>(ints.at(1).value()), 24);
+
+    auto const bools = jsonFileObj.getArray("array.[].bool");
+    ASSERT_EQ(bools.size(), 2);
+    EXPECT_FALSE(bools.at(0).has_value());
+    ASSERT_TRUE(std::holds_alternative<bool>(bools.at(1).value()));
+    EXPECT_EQ(std::get<bool>(bools.at(1).value()), true);
 }
 
 TEST_F(ConfigFileJsonDeathTest, getArrayInvalidKey)

tests/unit/util/newconfig/ConfigValueTests.cpp

@@ -66,28 +66,6 @@ TEST_F(ConfigValueDeathTest, invalidDefaultValue)
     EXPECT_DEATH({ [[maybe_unused]] auto const a = ConfigValue{ConfigType::String}.defaultValue(33); }, ".*");
 }
 
-TEST_F(ConfigValueTest, setValueNull)
-{
-    auto cv = ConfigValue{ConfigType::Integer};
-    auto const err = cv.setValue(NullType{});
-    EXPECT_TRUE(err.has_value());
-}
-
-TEST_F(ConfigValueTest, setValueNullOptional)
-{
-    auto cv = ConfigValue{ConfigType::Integer}.optional();
-    auto const err = cv.setValue(NullType{});
-    EXPECT_FALSE(err.has_value());
-}
-
-TEST_F(ConfigValueTest, setValueNullDefault)
-{
-    auto cv = ConfigValue{ConfigType::Integer}.defaultValue(123);
-    auto const err = cv.setValue(NullType{});
-    EXPECT_FALSE(err.has_value());
-    EXPECT_EQ(cv.getValue(), Value{123});
-}
-
 TEST_F(ConfigValueTest, setValueWrongType)
 {
     auto cv = ConfigValue{ConfigType::Integer};