This is an automated email from the ASF dual-hosted git repository.
pnoltes pushed a commit to branch feature/type_support_for_properties
in repository https://gitbox.apache.org/repos/asf/celix.git
The following commit(s) were added to
refs/heads/feature/type_support_for_properties by this push:
new cb0a8ba9 #509: Improve some error handling in celix properties.
cb0a8ba9 is described below
commit cb0a8ba933113f2098f69bbb1ffb79b330175b3c
Author: Pepijn Noltes <[email protected]>
AuthorDate: Fri Nov 10 14:10:52 2023 +0100
#509: Improve some error handling in celix properties.
Also:
- Add some clarification of when error messaged are pushed to celix err
in the celix properties header documentation.
---
.../gtest/src/PropertiesErrorInjectionTestSuite.cc | 22 ++++++++++-
libs/utils/gtest/src/PropertiesTestSuite.cc | 34 ++++++++++++++++
libs/utils/include/celix_properties.h | 29 +++++++++++++-
libs/utils/src/properties.c | 45 +++++++++++++++++-----
4 files changed, 117 insertions(+), 13 deletions(-)
diff --git a/libs/utils/gtest/src/PropertiesErrorInjectionTestSuite.cc
b/libs/utils/gtest/src/PropertiesErrorInjectionTestSuite.cc
index 7deaa39b..3172a8c2 100644
--- a/libs/utils/gtest/src/PropertiesErrorInjectionTestSuite.cc
+++ b/libs/utils/gtest/src/PropertiesErrorInjectionTestSuite.cc
@@ -37,13 +37,15 @@ class PropertiesErrorInjectionTestSuite : public
::testing::Test {
public:
PropertiesErrorInjectionTestSuite() = default;
~PropertiesErrorInjectionTestSuite() override {
+ celix_err_resetErrors();
celix_ei_expect_malloc(nullptr, 0, nullptr);
- celix_ei_expect_celix_stringHashMap_create(nullptr, 0, nullptr);
+ celix_ei_expect_celix_stringHashMap_createWithOptions(nullptr, 0,
nullptr);
celix_ei_expect_celix_utils_strdup(nullptr, 0, nullptr);
celix_ei_expect_fopen(nullptr, 0, nullptr);
celix_ei_expect_fputc(nullptr, 0, 0);
celix_ei_expect_fseek(nullptr, 0, 0);
celix_ei_expect_celix_utils_strdup(nullptr, 0, nullptr);
+ celix_ei_expect_celix_stringHashMap_put(nullptr, 0, 0);
}
/**
@@ -77,10 +79,26 @@ TEST_F(PropertiesErrorInjectionTestSuite,
CreateFailureTest) {
TEST_F(PropertiesErrorInjectionTestSuite, CopyFailureTest) {
// C API
+
+ //Given a celix properties object with more entries than the optimization
cache
celix_autoptr(celix_properties_t) prop = celix_properties_create();
ASSERT_NE(nullptr, prop);
+ fillOptimizationCache(prop);
+ celix_properties_set(prop, "additionalKey", "value");
+
+ // When a hash map create error injection is set for
celix_properties_create
celix_ei_expect_celix_stringHashMap_createWithOptions((void*)celix_properties_create,
0, nullptr);
+ // Then the celix_properties_copy call fails
ASSERT_EQ(nullptr, celix_properties_copy(prop));
+ ASSERT_EQ(1, celix_err_getErrorCount());
+ celix_err_resetErrors();
+
+ // When a malloc error injection is set for celix_properties_allocEntry
(during set)
+ celix_ei_expect_malloc((void*)celix_properties_allocEntry, 0, nullptr);
+ // Then the celix_properties_copy call fails
+ ASSERT_EQ(nullptr, celix_properties_copy(prop));
+ ASSERT_GE(celix_err_getErrorCount(), 1);
+ celix_err_resetErrors();
// C++ API
const celix::Properties cxxProp{};
@@ -94,7 +112,7 @@ TEST_F(PropertiesErrorInjectionTestSuite, SetFailureTest) {
celix_autoptr(celix_properties_t) props = celix_properties_create();
fillOptimizationCache(props);
- // When a malloc error injection is set for celix_properties_set (during
alloc entry)
+ // When a malloc error injection is set for celix_properties_allocEntry
(during set)
celix_ei_expect_malloc((void*)celix_properties_allocEntry, 0, nullptr);
// Then the celix_properties_set call fails
ASSERT_EQ(celix_properties_set(props, "key", "value"), CELIX_ENOMEM);
diff --git a/libs/utils/gtest/src/PropertiesTestSuite.cc
b/libs/utils/gtest/src/PropertiesTestSuite.cc
index 1a0ae825..675b9086 100644
--- a/libs/utils/gtest/src/PropertiesTestSuite.cc
+++ b/libs/utils/gtest/src/PropertiesTestSuite.cc
@@ -22,11 +22,15 @@
#include "celix_properties.h"
#include "celix_utils.h"
#include "properties.h"
+#include "celix_err.h"
using ::testing::MatchesRegex;
class PropertiesTestSuite : public ::testing::Test {
public:
+ PropertiesTestSuite() {
+ celix_err_resetErrors();
+ }
};
@@ -649,3 +653,33 @@ TEST_F(PropertiesTestSuite, PropertiesEqualsTest) {
celix_properties_setDouble(prop2, "key5", 42.0);
EXPECT_FALSE(celix_properties_equals(prop1, prop2)); //different types
}
+
+TEST_F(PropertiesTestSuite, PropertiesNullArgumentsTest) {
+ //Silently ignore nullptr properties arguments for set* and copy functions
+ EXPECT_EQ(CELIX_SUCCESS, celix_properties_set(nullptr, "key", "value"));
+ EXPECT_EQ(CELIX_SUCCESS, celix_properties_setLong(nullptr, "key", 1));
+ EXPECT_EQ(CELIX_SUCCESS, celix_properties_setDouble(nullptr, "key", 1.0));
+ EXPECT_EQ(CELIX_SUCCESS, celix_properties_setBool(nullptr, "key", true));
+ EXPECT_EQ(CELIX_SUCCESS, celix_properties_setVersion(nullptr, "key",
nullptr));
+ celix_autoptr(celix_properties_t) copy = celix_properties_copy(nullptr);
+ EXPECT_NE(nullptr, copy);
+}
+
+TEST_F(PropertiesTestSuite, InvalidArgumentsTest) {
+ celix_autoptr(celix_properties_t) props = celix_properties_create();
+ celix_autoptr(celix_version_t) version = celix_version_create(1,2,3,
nullptr);
+
+ //Key cannot be nullptr and set functions should fail
+ EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, celix_properties_set(props, nullptr,
"value"));
+ EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, celix_properties_setLong(props, nullptr,
1));
+ EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, celix_properties_setDouble(props,
nullptr, 1.0));
+ EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, celix_properties_setBool(props, nullptr,
true));
+ EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, celix_properties_setVersion(props,
nullptr, version));
+ EXPECT_EQ(5, celix_err_getErrorCount());
+
+ celix_err_resetErrors();
+ //Set without copy should fail if a key or value is nullptr
+ EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, celix_properties_setWithoutCopy(props,
nullptr, strdup("value")));
+ EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, celix_properties_setWithoutCopy(props,
strdup("key"), nullptr));
+ EXPECT_EQ(2, celix_err_getErrorCount());
+}
diff --git a/libs/utils/include/celix_properties.h
b/libs/utils/include/celix_properties.h
index f106b081..edc5a684 100644
--- a/libs/utils/include/celix_properties.h
+++ b/libs/utils/include/celix_properties.h
@@ -117,6 +117,8 @@ typedef struct celix_properties_iterator {
/**
* @brief Create a new empty property set.
*
+ * This function will push a error msg to celix_err if the return value is
NULL.
+ *
* @return A new empty property set.
*/
CELIX_UTILS_EXPORT celix_properties_t* celix_properties_create();
@@ -133,6 +135,8 @@ CELIX_DEFINE_AUTOPTR_CLEANUP_FUNC(celix_properties_t,
celix_properties_destroy)
/**
* @brief Load properties from a file.
*
+ * This function will push a error msg to celix_err if the return value is
NULL.
+ *
* @param[in] filename The name of the file to load properties from.
* @return A property set containing the properties from the file.
* @retval NULL If an error occurred (e.g. file not found).
@@ -142,6 +146,8 @@ CELIX_UTILS_EXPORT celix_properties_t*
celix_properties_load(const char* filenam
/**
* @brief Load properties from a stream.
*
+ * This function will push a error msg to celix_err if the return value is
NULL.
+ *
* @param[in,out] stream The stream to load properties from.
* @return A property set containing the properties from the stream.
* @retval NULL If an error occurred (e.g. invalid format).
@@ -151,6 +157,8 @@ CELIX_UTILS_EXPORT celix_properties_t*
celix_properties_loadWithStream(FILE* str
/**
* @brief Load properties from a string.
*
+ * This function will push a error msg to celix_err if the return value is
NULL.
+ *
* @param[in] input The string to load properties from.
* @return A property set containing the properties from the string.
* @retval NULL If an error occurred (e.g. invalid format).
@@ -162,6 +170,8 @@ CELIX_UTILS_EXPORT celix_properties_t*
celix_properties_loadFromString(const cha
*
* @note Properties values are always stored as string values, regardless of
their actual underlining types.
*
+ * This function will push a error msg to celix_err if the return code is not
CELIX_SUCCESS.
+ *
* @param[in] properties The property set to store.
* @param[in] file The name of the file to store the properties to.
* @param[in] header An optional - single line - header to write to the file
before the properties.
@@ -207,6 +217,8 @@ CELIX_UTILS_EXPORT celix_properties_value_type_e
celix_properties_getType(const
/**
* @brief Set the value of a property.
*
+ * This function will push a error msg to celix_err if the return code is not
CELIX_SUCCESS.
+ *
* @param[in] properties The property set to modify.
* @param[in] key The key of the property to set.
* @param[in] value The value to set the property to.
@@ -219,6 +231,8 @@ CELIX_UTILS_EXPORT celix_status_t
celix_properties_set(celix_properties_t* prope
/**
* @brief Set the value of a property without copying the value string.
*
+ * This function will push a error msg to celix_err if the return code is not
CELIX_SUCCESS.
+ *
* @param[in] properties The property set to modify.
* @param[in] key The key of the property to set. This string will be used
directly, so it must not be freed or
* modified after calling this function.
@@ -233,6 +247,9 @@ CELIX_UTILS_EXPORT celix_status_t
celix_properties_setWithoutCopy(celix_properti
/**
* @brief Set the value of a property based on the provided property entry,
maintaining underlying type.
+ *
+ * This function will push a error msg to celix_err if the return code is not
CELIX_SUCCESS.
+ *
* @param[in] properties The property set to modify.
* @param[in] key The key of the property to set.
* @param[in] entry The entry to set the property to. The entry will be
copied, so it can be freed after calling
@@ -252,7 +269,9 @@ CELIX_UTILS_EXPORT celix_status_t
celix_properties_setEntry(celix_properties_t*
CELIX_UTILS_EXPORT void celix_properties_unset(celix_properties_t* properties,
const char* key);
/**
- * @brief Make a copy of a property set.
+ * @brief Make a copy of a properties set.
+ *
+ * This function will push a error msg to celix_err if the return value is
NULL.
*
* @param[in] properties The property set to copy.
* @return A copy of the given property set.
@@ -276,6 +295,8 @@ celix_properties_getAsLong(const celix_properties_t*
properties, const char* key
/**
* @brief Set the value of a property to a long integer.
*
+ * This function will push a error msg to celix_err if the return code is not
CELIX_SUCCESS.
+ *
* @param[in] properties The property set to modify.
* @param[in] key The key of the property to set.
* @param[in] value The long value to set the property to.
@@ -300,6 +321,8 @@ celix_properties_getAsBool(const celix_properties_t*
properties, const char* key
/**
* @brief Set the value of a property to a boolean.
*
+ * This function will push a error msg to celix_err if the return code is not
CELIX_SUCCESS.
+ *
* @param[in] properties The property set to modify.
* @param[in] key The key of the property to set.
* @param[in] val The boolean value to set the property to.
@@ -310,6 +333,8 @@ CELIX_UTILS_EXPORT celix_status_t
celix_properties_setBool(celix_properties_t* p
/**
* @brief Set the value of a property to a double.
*
+ * This function will push a error msg to celix_err if the return code is not
CELIX_SUCCESS.
+ *
* @param[in] properties The property set to modify.
* @param[in] key The key of the property to set.
* @param[in] val The double value to set the property to.
@@ -337,6 +362,7 @@ celix_properties_getAsDouble(const celix_properties_t*
properties, const char* k
* @brief Set the value of a property as a Celix version.
*
* This function will make a copy of the provided celix_version_t object and
store it in the property set.
+ * This function will push a error msg to celix_err if the return code is not
CELIX_SUCCESS.
*
* @param[in] properties The property set to modify.
* @param[in] key The key of the property to set.
@@ -352,6 +378,7 @@ CELIX_UTILS_EXPORT celix_status_t
celix_properties_setVersion(celix_properties_t
*
* This function will store a reference to the provided celix_version_t object
in the property set and takes
* ownership of the provided version.
+ * This function will push a error msg to celix_err if the return code is not
CELIX_SUCCESS.
*
* @param[in] properties The property set to modify.
* @param[in] key The key of the property to set.
diff --git a/libs/utils/src/properties.c b/libs/utils/src/properties.c
index 0d987bb2..c5381922 100644
--- a/libs/utils/src/properties.c
+++ b/libs/utils/src/properties.c
@@ -265,12 +265,16 @@ static celix_properties_entry_t*
celix_properties_createEntry(celix_properties_t
celix_version_t*
versionValue) {
celix_properties_entry_t* entry = celix_properties_allocEntry(properties);
if (entry == NULL) {
+ celix_err_pushf("Cannot allocate property entry");
+ celix_version_destroy(versionValue);
return NULL;
}
celix_status_t status =
celix_properties_fillEntry(properties, entry, strValue, longValue,
doubleValue, boolValue, versionValue);
if (status != CELIX_SUCCESS) {
+ celix_err_pushf("Cannot fill property entry");
+ celix_version_destroy(versionValue);
if (entry >= properties->entriesBuffer &&
entry <= (properties->entriesBuffer +
CELIX_PROPERTIES_OPTIMIZATION_ENTRIES_BUFFER_SIZE)) {
// entry is part of the properties entries buffer -> nop.
@@ -308,11 +312,14 @@ static celix_status_t
celix_properties_createAndSetEntry(celix_properties_t* pro
const bool* boolValue,
celix_version_t*
versionValue) {
if (!properties) {
+ celix_version_destroy(versionValue);
return CELIX_SUCCESS; // silently ignore
}
+
if (!key) {
- celix_err_push("Cannot set property with NULL key");
- return CELIX_SUCCESS; // print error and ignore
+ celix_version_destroy(versionValue);
+ celix_err_pushf("Cannot set property with NULL key");
+ return CELIX_ILLEGAL_ARGUMENT;
}
celix_properties_entry_t* entry =
@@ -616,25 +623,37 @@ celix_status_t celix_properties_store(celix_properties_t*
properties, const char
celix_properties_t* celix_properties_copy(const celix_properties_t*
properties) {
celix_properties_t* copy = celix_properties_create();
- if (properties == NULL) {
+
+ if (!copy) {
+ celix_err_push("Failed to create properties copy");
+ return NULL;
+ }
+
+ if (!properties) {
return copy;
}
CELIX_PROPERTIES_ITERATE(properties, iter) {
+ celix_status_t status;
if (iter.entry.valueType == CELIX_PROPERTIES_VALUE_TYPE_STRING) {
- celix_properties_set(copy, iter.key, iter.entry.value);
+ status = celix_properties_set(copy, iter.key, iter.entry.value);
} else if (iter.entry.valueType == CELIX_PROPERTIES_VALUE_TYPE_LONG) {
- celix_properties_setLong(copy, iter.key,
iter.entry.typed.longValue);
+ status = celix_properties_setLong(copy, iter.key,
iter.entry.typed.longValue);
} else if (iter.entry.valueType == CELIX_PROPERTIES_VALUE_TYPE_DOUBLE)
{
- celix_properties_setDouble(copy, iter.key,
iter.entry.typed.doubleValue);
+ status = celix_properties_setDouble(copy, iter.key,
iter.entry.typed.doubleValue);
} else if (iter.entry.valueType == CELIX_PROPERTIES_VALUE_TYPE_BOOL) {
- celix_properties_setBool(copy, iter.key,
iter.entry.typed.boolValue);
+ status = celix_properties_setBool(copy, iter.key,
iter.entry.typed.boolValue);
} else /*version*/ {
assert(iter.entry.valueType ==
CELIX_PROPERTIES_VALUE_TYPE_VERSION);
- celix_properties_setVersion(copy, iter.key,
iter.entry.typed.versionValue);
+ status = celix_properties_setVersion(copy, iter.key,
iter.entry.typed.versionValue);
+ }
+ if (status != CELIX_SUCCESS) {
+ celix_err_pushf("Failed to copy property %s", iter.key);
+ celix_properties_destroy(copy);
+ return NULL;
}
}
- return copy;
+ return celix_steal_ptr(copy);
}
celix_properties_value_type_e celix_properties_getType(const
celix_properties_t* properties, const char* key) {
@@ -663,7 +682,13 @@ celix_status_t celix_properties_set(celix_properties_t*
properties, const char*
}
celix_status_t celix_properties_setWithoutCopy(celix_properties_t* properties,
char* key, char* value) {
- if (properties != NULL && key != NULL && value != NULL) {
+ if (properties) {
+ if (!key || !value) {
+ celix_err_push("Failed to set (without copy) property. Key or
value is NULL.");
+ free(key);
+ free(value);
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
celix_properties_entry_t* entry =
celix_properties_createEntryWithNoCopy(properties, value);
if (!entry) {
celix_err_push("Failed to create entry for property.");
