Skip to content

Commit

Permalink
#509: Improve some error handling in celix properties.
Browse files Browse the repository at this point in the history 
Also:
  - Add some clarification of when error messaged are pushed to celix err
    in the celix properties header documentation.
  • Loading branch information
@pnoltes
pnoltes committed Nov 10, 2023
1 parent f1beb5f commit cb0a8ba
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 13 deletions.
22 changes: 20 additions & 2 deletions libs/utils/gtest/src/PropertiesErrorInjectionTestSuite.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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{};
Expand All @@ -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);
Expand Down
34 changes: 34 additions & 0 deletions libs/utils/gtest/src/PropertiesTestSuite.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
};


Expand Down Expand Up @@ -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());
}
29 changes: 28 additions & 1 deletion libs/utils/include/celix_properties.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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).
Expand All @@ -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).
Expand All @@ -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).
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down
45 changes: 35 additions & 10 deletions libs/utils/src/properties.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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.");
Expand Down

0 comments on commit cb0a8ba

Please sign in to comment.