cell adding opers with err check

8 files changed, 230 insertions, 64 deletions

diff --git a/include/fieldstat/fieldstat.h b/include/fieldstat/fieldstat.h
index c085017..b9b17bf 100644
--- a/include/fieldstat/fieldstat.h
+++ b/include/fieldstat/fieldstat.h
@@ -103,12 +103,13 @@ int fieldstat_cube_add(struct fieldstat *instance, int cube_id, const struct fie
     * @param metric_id: metric id, previously returned by fieldstat_register_counter.
     * @param cell_id: cell id, previously returned by fieldstat_cube_add.
     * @param increment: increment of the counter metric. Can be negative.
-    * @return 0 if success. -1 if cube_id is invalid. -2 if metric_id is invalid, or the metric is not a METRIC_TYPE_COUNTER. cell_id is not checked, user should guarantee that cell_id is valid.
+    * @return 0 if success. -1 if cube_id is invalid. -2 if metric_id is invalid, or the metric is not a METRIC_TYPE_COUNTER. -3 if cell_id is invalid.
 */
 int fieldstat_counter_incrby(struct fieldstat *instance, int cube_id, int metric_id, int cell_id, long long increment);
 
 /*
     * @brief let the value of counter metric equal to value. Other annotations refer to fieldstat_counter_incrby.
+    * @return 0 if success. -1 if cube_id is invalid. -2 if metric_id is invalid, or the metric is not a METRIC_TYPE_COUNTER. -3 if cell_id is invalid.
 */
 int fieldstat_counter_set(struct fieldstat *instance, int cube_id, int metric_id, int cell_id, long long value);
 
@@ -116,10 +117,15 @@ int fieldstat_counter_set(struct fieldstat *instance, int cube_id, int metric_id
     * @brief add a key to the hll metric of cell_id. HLL approximates the number of distinct elements in a set of `key`s.
     * @param key: key of the hll metric. Cannot be NULL.
     * @param key_len: strlen(key).
-    * @return 0 if success. -1 if cube_id is invalid. -2 if metric_id is invalid, or the metric is not a METRIC_TYPE_HLL. cell_id is not checked, user should guarantee that cell_id is valid.
+    * @return 0 if success. -1 if cube_id is invalid. -2 if metric_id is invalid, or the metric is not a METRIC_TYPE_HLL. -3 if cell_id is invalid.
 */
 int fieldstat_hll_add(struct fieldstat *instance, int cube_id, int metric_id, int cell_id, const char *key, size_t key_len);
 
+/* 
+    * @brief Add a value to the histogram metric of cell_id. Histogram will record the distribution of the values.
+    * @param value: value of the histogram metric.
+    * @return 0 if success. -1 if cube_id is invalid. -2 if metric_id is invalid, or the metric is not a METRIC_TYPE_HISTOGRAM. -3 if cell_id is invalid.
+*/
 int fieldstat_histogram_record(struct fieldstat *instance, int cube_id, int metric_id, int cell_id, long long value);
 
 /*
diff --git a/src/fieldstat.c b/src/fieldstat.c
index 5769011..067893f 100644
--- a/src/fieldstat.c
+++ b/src/fieldstat.c
@@ -371,28 +371,50 @@ int fieldstat_register_histogram(struct fieldstat *instance, int cube_id, const
 /* -------------------------------------------------------------------------- */
 /*                               metric operation                              */
 /* -------------------------------------------------------------------------- */
-struct metric *fieldstat_find_metric(const struct fieldstat *instance, int cube_id, int metric_id)
+static struct metric *fieldstat_find_metric(const struct fieldstat *instance, int cube_id, int metric_id)
 {
+    if (cube_id < 0 || cube_id >= instance->valid_cube_arr_length) {
+        printf("ERR: fieldstat_find_metric cube_id is not correct, input cube id: %d\n", cube_id);
+        return NULL;
+    }
     struct fs_cube *cube = instance->cube[cube_id];
     if (cube == NULL) {
-        printf("ERR: fieldstat_find_metric cube is not registered yet\n");
+        printf("ERR: fieldstat_find_metric cube is not registered yet, input cube id: %d\n", cube_id);
         return NULL;
     }
     return cube->metrics[metric_id];
 }
 
+#define FIELDSTAT_GENERAL_CHECK(instance, cube_id, metric_id, cell_id, metric_type) \
+do { \
+    if ((cube_id) < 0 || (cube_id) >= (instance)->valid_cube_arr_length) {    \
+        printf("ERR: [%s] cube_id is not correct, input cube id: %d\n", __FUNCTION__, (cube_id)); \
+        return -1;  \
+    }   \
+    const struct fs_cube *cube = (instance)->cube[(cube_id)];   \
+    if (cube == NULL) { \
+        printf("ERR: [%s] cube is not registered yet, input cube id: %d\n", __FUNCTION__, (cube_id)); \
+        return -1;  \
+    }   \
+    if ((metric_id) < 0 || (metric_id) >= cube->n_metric) { \
+        printf("ERR: [%s] metric_id is not correct, input metric id: %d\n", __FUNCTION__, (metric_id));   \
+        return -2;  \
+    }   \
+    const struct metric *metric = cube->metrics[(metric_id)]; \
+    if (metric == NULL || metric_get_type(metric) != (metric_type)) { \
+        printf("ERR: [%s] metric is not registered yet, input metric id: %d\n", __FUNCTION__, (metric_id));   \
+        return -2;  \
+    }   \
+    if ((cell_id) < 0 || cell_manager_get_tag_by_cell_id(cube->cell_manager, cell_id) == NULL) { \
+        printf("ERR: [%s] cell_id is not correct, input cell id: %d\n", __FUNCTION__, (cell_id)); \
+        return -3;  \
+    }   \
+} while (0)
+
 int fieldstat_counter_incrby(struct fieldstat *instance, int cube_id, int metric_id, int cell_id, long long increment)
 {
-    struct fs_cube *cube = instance->cube[cube_id];
-    if (cube == NULL) {
-        printf("ERR: fieldstat_counter_incrby cube is not registered yet\n");
-        return -1;
-    }
-    struct metric *metric = cube->metrics[metric_id];
-    if (metric == NULL || metric_get_type(metric) != METRIC_TYPE_COUNTER) {
-        printf("ERR: fieldstat_counter_incrby metrics is not registered yet\n");
-        return -2;
-    }
+    FIELDSTAT_GENERAL_CHECK(instance, cube_id, metric_id, cell_id, METRIC_TYPE_COUNTER);
+    struct metric *metric = instance->cube[cube_id]->metrics[metric_id];
 
     metric_counter_incrby(metric, cell_id, increment);
     return 0;
@@ -400,50 +422,37 @@ int fieldstat_counter_incrby(struct fieldstat *instance, int cube_id, int metric
 
 int fieldstat_counter_set(struct fieldstat *instance, int cube_id, int metric_id, int cell_id, long long value)
 {
-    struct fs_cube *cube = instance->cube[cube_id];
-    if (cube == NULL) {
-        printf("ERR: fieldstat_counter_set cube is not registered yet\n");
-        return -1;
-    }
-    struct metric *metric = cube->metrics[metric_id];
-    if (metric == NULL || metric_get_type(metric) != METRIC_TYPE_COUNTER) {
-        printf("ERR: fieldstat_counter_set metrics is not registered yet\n");
-        return -2;
-    }
+    FIELDSTAT_GENERAL_CHECK(instance, cube_id, metric_id, cell_id, METRIC_TYPE_COUNTER);
+    struct metric *metric = instance->cube[cube_id]->metrics[metric_id];
 
-    return metric_counter_set(metric, cell_id, value);
+    int ret = metric_counter_set(metric, cell_id, value);
+    if (ret < 0) {
+        printf("ERR: fieldstat_counter_set failed");
+        return -4;
+    }
+    return 0;
 }
 
 int fieldstat_hll_add(struct fieldstat *instance, int cube_id, int metric_id, int cell_id, const char *key, size_t key_len)
 {
-    struct fs_cube *cube = instance->cube[cube_id];
-    if (cube == NULL) {
-        printf("ERR: fieldstat_hll_add cube is not registered yet\n");
-        return -1;
-    }
-    struct metric *metric = cube->metrics[metric_id];
-    if (metric == NULL || metric_get_type(metric) != METRIC_TYPE_HLL) {
-        printf("ERR: fieldstat_hll_add metrics is not registered yet\n");
-        return -2;
-    }
+    FIELDSTAT_GENERAL_CHECK(instance, cube_id, metric_id, cell_id, METRIC_TYPE_HLL);
+    struct metric *metric = instance->cube[cube_id]->metrics[metric_id];
 
-    return metric_hll_add(metric, cell_id, key, key_len);
+    metric_hll_add(metric, cell_id, key, key_len);
+    return 0;
 }
 
 int fieldstat_histogram_record(struct fieldstat *instance, int cube_id, int metric_id, int cell_id, long long value)
 {
-    struct fs_cube *cube = instance->cube[cube_id];
-    if (cube == NULL) {
-        printf("ERR: fieldstat_histogram_record cube is not registered yet\n");
-        return -1;
-    }
-    struct metric *metric = cube->metrics[metric_id];
-    if (metric == NULL || metric_get_type(metric) != METRIC_TYPE_HISTOGRAM) {
-        printf("ERR: fieldstat_histogram_record metrics is not registered yet\n");
-        return -2;
-    }
+    FIELDSTAT_GENERAL_CHECK(instance, cube_id, metric_id, cell_id, METRIC_TYPE_HISTOGRAM);
+    struct metric *metric = instance->cube[cube_id]->metrics[metric_id];
 
-    return metric_histogram_record(metric, cell_id, value);
+    int ret = metric_histogram_record(metric, cell_id, value);
+    if (ret < 0) {
+        printf("ERR: fieldstat_histogram_record failed");
+        return -4;
+    }
+    return 0;
 }
 
 
@@ -794,7 +803,6 @@ void fieldstat_get_cells(const struct fieldstat *instance, int cube_id, int metr
 
     size_t n_cell_ret = 0;
     metric_get_cell_ids(metric, cell_ids, &n_cell_ret);
-    printf("fieldstat_get_cells, number of cells in metric: %zu\n", n_cell_ret);
     *n_cell = n_cell_ret;
     if (n_cell_ret == 0) {
         *tag_list = NULL;
diff --git a/src/metrics/metric.c b/src/metrics/metric.c
index eacd4b9..f289634 100644
--- a/src/metrics/metric.c
+++ b/src/metrics/metric.c
@@ -782,13 +782,11 @@ struct metric *metric_counter_new(const char *name, bool is_gauge)
     return pthis;
 }
 
-int metric_counter_incrby(struct metric *pthis, int cell_id, long long value)
+void metric_counter_incrby(struct metric *pthis, int cell_id, long long value)
 {
     struct metric_measure_data *data = metric_find_or_new_cell(pthis, cell_id);
     struct metric_counter_or_gauge *counter = data->counter;
     counter->value += value;
-
-    return 0;
 }
 
 int metric_counter_set(struct metric *pthis, int cell_id, long long value)
@@ -822,12 +820,11 @@ struct metric *metric_hll_new(const char *name, unsigned char precision)
     return pthis;
 }
 
-int metric_hll_add(struct metric *pthis, int cell_id, const char *key, size_t key_len)
+void metric_hll_add(struct metric *pthis, int cell_id, const char *key, size_t key_len)
 {
     struct metric_measure_data *data = metric_find_or_new_cell(pthis, cell_id);
 
     ST_hyperloglog_add(data->hll, key, key_len);
-    return 0;
 }
 
 double metric_hll_get(const struct metric *pthis, int cell_id)
diff --git a/src/metrics/metric.h b/src/metrics/metric.h
index af601e1..bd2d1e5 100644
--- a/src/metrics/metric.h
+++ b/src/metrics/metric.h
@@ -22,12 +22,12 @@ void metric_get_plain_blob(const struct metric *pthis, int cell_id, char **blob,
 void metric_delete_cell(struct metric *pthis, int cell_id);
 
 struct metric *metric_counter_new(const char *name, bool is_gauge);
-int metric_counter_incrby(struct metric *pthis, int cell_id, long long value);
+void metric_counter_incrby(struct metric *pthis, int cell_id, long long value);
 int metric_counter_set(struct metric *pthis, int cell_id, long long value);
 long long metric_counter_get(const struct metric *pthis, int cell_id);
 
 struct metric *metric_hll_new(const char *name, unsigned char precision);
-int metric_hll_add(struct metric *pthis, int cell_id, const char *key, size_t key_len);
+void metric_hll_add(struct metric *pthis, int cell_id, const char *key, size_t key_len);
 double metric_hll_get(const struct metric *pthis, int cell_id);
 
 struct metric *metric_histogram_new(const char *name, long long lowest_trackable_value, long long highest_trackable_value, int significant_figures);
diff --git a/src/tags/cell_manager.c b/src/tags/cell_manager.c
index fe8415b..6134f95 100644
--- a/src/tags/cell_manager.c
+++ b/src/tags/cell_manager.c
@@ -101,12 +101,7 @@ void cell_manager_free(struct cell_manager *pthis)
 
 const struct tag_hash_key *cell_manager_get_tag_by_cell_id(const struct cell_manager *pthis, int cell_id)
 {
-    const struct tag_hash_key *tag_key = pthis->id_tag_array[cell_id];
-    if (tag_key == NULL) {
-        return NULL;
-    }
-
-    return tag_key;
+    return pthis->id_tag_array[cell_id];
 }
 
 void cell_manager_id_tag_array_add(struct cell_manager *pthis, int cell_id, const struct tag_hash_key *tag)
diff --git a/test/test_metric_counter.cpp b/test/test_metric_counter.cpp
index d9069ff..76efbf3 100644
--- a/test/test_metric_counter.cpp
+++ b/test/test_metric_counter.cpp
@@ -278,7 +278,87 @@ TEST(metric_test_counter, topk_add_with_monotonically_increasing_flow_expecting_
         delete tags[i];
 }
 
-// todo: add on wrong cube/metric/cell
+TEST(metric_test_counter, add_or_set_with_wrong_cell_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_TOPK, 10);
+    fieldstat_register_counter(instance, 0, "test", 0);
+
+    int ret = fieldstat_counter_incrby(instance, 0, 0, 1, 1);
+    EXPECT_EQ(ret, -3);
+    ret = fieldstat_counter_incrby(instance, 0, 0, -1, 1);
+    EXPECT_EQ(ret, -3);
+
+    fieldstat_free(instance);
+}
+
+TEST(metric_test_counter, add_with_wrong_cube_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    int cube_id = fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_TOPK, 10);
+
+    int ret = fieldstat_counter_incrby(instance, cube_id + 1, 0, 1, 1);
+    EXPECT_EQ(ret, -1);
+    ret = fieldstat_counter_incrby(instance, -1, 0, 1, 1);
+    EXPECT_EQ(ret, -1);
+
+    fieldstat_free(instance);
+}
+
+TEST(metric_test_counter, add_with_wrong_metric_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    int cube_id = fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_TOPK, 10);
+    int metric_id = fieldstat_register_counter(instance, cube_id, "test", 0);
+
+    int ret = fieldstat_counter_incrby(instance, cube_id, metric_id + 1, 1, 1);
+    EXPECT_EQ(ret, -2);
+    ret = fieldstat_counter_incrby(instance, cube_id, -1, 1, 1);
+    EXPECT_EQ(ret, -2);
+
+    fieldstat_free(instance);
+}
+
+TEST(metric_test_counter, set_with_wrong_cell_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_TOPK, 10);
+    fieldstat_register_counter(instance, 0, "test", 1);
+
+    int ret = fieldstat_counter_set(instance, 0, 0, 1, 1);
+    EXPECT_EQ(ret, -3);
+    ret = fieldstat_counter_set(instance, 0, 0, -1, 1);
+    EXPECT_EQ(ret, -3);
+
+    fieldstat_free(instance);
+}
+
+TEST(metric_test_counter, set_with_wrong_cube_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    int cube_id = fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_TOPK, 10);
+
+    int ret = fieldstat_counter_set(instance, cube_id + 1, 0, 1, 1);
+    EXPECT_EQ(ret, -1);
+    ret = fieldstat_counter_set(instance, -1, 0, 1, 1);
+    EXPECT_EQ(ret, -1);
+
+    fieldstat_free(instance);
+}
+
+TEST(metric_test_counter, set_with_wrong_metric_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    int cube_id = fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_TOPK, 10);
+    int metric_id = fieldstat_register_counter(instance, cube_id, "test", 1);
+
+    int ret = fieldstat_counter_set(instance, cube_id, metric_id + 1, 1, 1);
+    EXPECT_EQ(ret, -2);
+    ret = fieldstat_counter_set(instance, cube_id, -1, 1, 1);
+    EXPECT_EQ(ret, -2);
+
+    fieldstat_free(instance);
+}
 
 int main(int argc, char *argv[]) 
 {
diff --git a/test/test_metric_histogram.cpp b/test/test_metric_histogram.cpp
index 996b1ba..5e3590f 100644
--- a/test/test_metric_histogram.cpp
+++ b/test/test_metric_histogram.cpp
@@ -120,7 +120,46 @@ TEST(metric_test_histogram, serialization_and_merge_twice_with_reset)
     fieldstat_free(instance_total);
 }
 
-// todo: add on wrong cube/metric/cell
+TEST(metric_test_histogram, add_or_set_with_wrong_cell_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    int cube_id = fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_COMPREHENSIVE, 10);
+    int metric_id = fieldstat_register_histogram(instance, cube_id, "czz_test", 1, 600000, 3);
+
+    int ret = fieldstat_histogram_record(instance, cube_id, metric_id, 1, 1234);
+    EXPECT_EQ(ret, -3);
+    ret = fieldstat_histogram_record(instance, cube_id, metric_id, -1, 1234);
+    EXPECT_EQ(ret, -3);
+
+    fieldstat_free(instance);
+}
+
+TEST(metric_test_histogram, add_with_wrong_cube_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    int cube_id = fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_COMPREHENSIVE, 10);
+
+    int ret = fieldstat_histogram_record(instance, cube_id + 1, 0, 1, 1);
+    EXPECT_EQ(ret, -1);
+    ret = fieldstat_histogram_record(instance, -1, 0, 1, 1);
+    EXPECT_EQ(ret, -1);
+
+    fieldstat_free(instance);
+}
+
+TEST(metric_test_histogram, add_with_wrong_metric_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    int cube_id = fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_COMPREHENSIVE, 10);
+    int metric_id = fieldstat_register_histogram(instance, cube_id, "czz_test", 1, 600000, 3);
+
+    int ret = fieldstat_histogram_record(instance, cube_id, metric_id + 1, 1, 1);
+    EXPECT_EQ(ret, -2);
+    ret = fieldstat_histogram_record(instance, cube_id, -1, 1, 1);
+    EXPECT_EQ(ret, -2);
+
+    fieldstat_free(instance);
+}
 
 // todo：测base 64序列化
 
diff --git a/test/test_metric_hll.cpp b/test/test_metric_hll.cpp
index 0441238..cbe6d3c 100644
--- a/test/test_metric_hll.cpp
+++ b/test/test_metric_hll.cpp
@@ -118,8 +118,49 @@ TEST(metric_test_hll, serialization_and_merge_twice_with_reset)
     fieldstat_free(instance_total);
 }
 
+TEST(metric_test_histogram, add_or_set_with_wrong_cell_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    int cube_id = fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_COMPREHENSIVE, 10);
+    int metric_id = fieldstat_register_hll(instance, cube_id, "czz_test hll metric", 10);
+
+    int ret = fieldstat_hll_add(instance, cube_id, metric_id, 1, "hello", 5);
+    EXPECT_EQ(ret, -3);
+    ret = fieldstat_hll_add(instance, cube_id, metric_id, -1, "hello", 5);
+    EXPECT_EQ(ret, -3);
+
+    fieldstat_free(instance);
+}
+
+TEST(metric_test_histogram, add_with_wrong_cube_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    int cube_id = fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_COMPREHENSIVE, 10);
+
+    int ret = fieldstat_hll_add(instance, cube_id + 1, 0, 0, "hello", 5);
+    EXPECT_EQ(ret, -1);
+    ret = fieldstat_hll_add(instance, -1, 0, 0, "hello", 5);
+    EXPECT_EQ(ret, -1);
+
+    fieldstat_free(instance);
+}
+
+TEST(metric_test_histogram, add_with_wrong_metric_id_expecting_fail)
+{
+    struct fieldstat *instance = fieldstat_new();
+    int cube_id = fieldstat_register_cube(instance, &TEST_TAG_INT_collided, 1, SAMPLING_MODE_COMPREHENSIVE, 10);
+    int metric_id = fieldstat_register_hll(instance, cube_id, "czz_test hll metric", 10);
+
+    int ret = fieldstat_hll_add(instance, cube_id, metric_id + 1, 0, "hello", 5);
+    EXPECT_EQ(ret, -2);
+    ret = fieldstat_hll_add(instance, cube_id, -1, 0, "hello", 5);
+    EXPECT_EQ(ret, -2);
+
+    fieldstat_free(instance);
+}
+
+
 // todo：测base 64序列化
-// todo: add on wrong cube/metric/cell
 
 int main(int argc, char *argv[]) 
 {