feat: basic object merging (#650)

This has a basic recursive merging of sentry_value_t objects. Generally values from the source object have preference, however when the values of a key are objects themselves in both the target and the source they are recursively merged. Co-authored-by: Betty Da <[email protected]>
getsentry · Jan 14, 2022 · 047cf98 · 047cf98
1 parent b6dc581
commit 047cf98
Show file tree

Hide file tree

Showing 6 changed files with 156 additions and 32 deletions.
diff --git a/src/sentry_core.c b/src/sentry_core.c
@@ -735,37 +735,25 @@ sentry_transaction_start(sentry_transaction_context_t *opaque_tx_cxt)
 {
     sentry_value_t tx_cxt = opaque_tx_cxt->inner;
 
-    // TODO: it would be nice if we could just merge tx_cxt into tx.
-    // `sentry_value_new_transaction_event()` is also an option, but risks
-    // causing more confusion as there's already a
-    // `sentry_value_new_transaction`. The ending timestamp is stripped as well
-    // to avoid misleading ourselves later down the line.
+    // If the parent span ID is some empty-ish value, just remove it
+    sentry_value_t parent_span
+        = sentry_value_get_by_key(tx_cxt, "parent_span_id");
+    if (sentry_value_get_length(parent_span) < 1) {
+        sentry_value_remove_by_key(tx_cxt, "parent_span_id");
+    }
+
+    // The ending timestamp is stripped to avoid misleading ourselves later
+    // down the line, as it is the only way to determine whether a transaction
+    // has ended or not.
     sentry_value_t tx = sentry_value_new_event();
     sentry_value_remove_by_key(tx, "timestamp");
 
+    sentry__value_merge_objects(tx, tx_cxt);
+
     bool should_sample = sentry__should_send_transaction(tx_cxt);
     sentry_value_set_by_key(
         tx, "sampled", sentry_value_new_bool(should_sample));
 
-    // Avoid having this show up in the payload at all if it doesn't have a
-    // valid value
-    sentry_value_t parent_span
-        = sentry_value_get_by_key_owned(tx_cxt, "parent_span_id");
-    if (sentry_value_get_length(parent_span) > 0) {
-        sentry_value_set_by_key(tx, "parent_span_id", parent_span);
-    } else {
-        sentry_value_decref(parent_span);
-    }
-    sentry_value_set_by_key(
-        tx, "trace_id", sentry_value_get_by_key_owned(tx_cxt, "trace_id"));
-    sentry_value_set_by_key(
-        tx, "span_id", sentry_value_get_by_key_owned(tx_cxt, "span_id"));
-    sentry_value_set_by_key(tx, "transaction",
-        sentry_value_get_by_key_owned(tx_cxt, "transaction"));
-    sentry_value_set_by_key(
-        tx, "op", sentry_value_get_by_key_owned(tx_cxt, "op"));
-    sentry_value_set_by_key(
-        tx, "status", sentry_value_get_by_key_owned(tx_cxt, "status"));
     sentry_value_set_by_key(tx, "start_timestamp",
         sentry__value_new_string_owned(
             sentry__msec_time_to_iso8601(sentry__msec_time())));

diff --git a/src/sentry_scope.c b/src/sentry_scope.c
@@ -1,4 +1,5 @@
 #include "sentry_scope.h"
+#include "sentry.h"
 #include "sentry_backend.h"
 #include "sentry_core.h"
 #include "sentry_database.h"
@@ -7,6 +8,7 @@
 #include "sentry_string.h"
 #include "sentry_symbolizer.h"
 #include "sentry_sync.h"
+#include "sentry_value.h"
 
 #include <stdlib.h>
 
@@ -297,18 +299,43 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope,
     PLACE_STRING("transaction", scope->transaction);
     PLACE_VALUE("sdk", scope->client_sdk);
 
-    // TODO: these should merge
-    PLACE_CLONED_VALUE("tags", scope->tags);
-    PLACE_CLONED_VALUE("extra", scope->extra);
+    sentry_value_t event_tags = sentry_value_get_by_key(event, "tags");
+    if (sentry_value_is_null(event_tags)) {
+        if (!sentry_value_is_null(scope->tags)) {
+            PLACE_CLONED_VALUE("tags", scope->tags);
+        }
+    } else {
+        sentry__value_merge_objects(event_tags, scope->tags);
+    }
+    sentry_value_t event_extra = sentry_value_get_by_key(event, "extra");
+    if (sentry_value_is_null(event_extra)) {
+        if (!sentry_value_is_null(scope->extra)) {
+            PLACE_CLONED_VALUE("extra", scope->extra);
+        }
+    } else {
+        sentry__value_merge_objects(event_extra, scope->extra);
+    }
 
 #ifdef SENTRY_PERFORMANCE_MONITORING
-    // TODO: better, more thorough deep merging
     sentry_value_t contexts = sentry__value_clone(scope->contexts);
-    sentry_value_t trace = sentry__transaction_get_trace_context(scope->span);
-    if (!sentry_value_is_null(trace)) {
-        sentry_value_set_by_key(contexts, "trace", trace);
+    // prep contexts sourced from scope; data about transaction on scope needs
+    // to be extracted and inserted
+    sentry_value_t scope_trace
+        = sentry__transaction_get_trace_context(scope->span);
+    if (!sentry_value_is_null(scope_trace)) {
+        if (sentry_value_is_null(contexts)) {
+            contexts = sentry_value_new_object();
+        }
+        sentry_value_set_by_key(contexts, "trace", scope_trace);
+    }
+
+    // merge contexts sourced from scope into the event
+    sentry_value_t event_contexts = sentry_value_get_by_key(event, "contexts");
+    if (sentry_value_is_null(event_contexts)) {
+        PLACE_VALUE("contexts", contexts);
+    } else {
+        sentry__value_merge_objects(event_contexts, contexts);
     }
-    PLACE_VALUE("contexts", contexts);
     sentry_value_decref(contexts);
 #endif
 

diff --git a/src/sentry_value.c b/src/sentry_value.c
@@ -806,6 +806,41 @@ sentry_value_is_null(sentry_value_t value)
     return value._bits == CONST_NULL;
 }
 
+int
+sentry__value_merge_objects(sentry_value_t dst, sentry_value_t src)
+{
+    if (sentry_value_is_null(src)) {
+        return 0;
+    }
+    if (sentry_value_get_type(dst) != SENTRY_VALUE_TYPE_OBJECT
+        || sentry_value_get_type(src) != SENTRY_VALUE_TYPE_OBJECT
+        || sentry_value_is_frozen(dst)) {
+        return 1;
+    }
+    thing_t *thing = value_as_thing(src);
+    if (!thing) {
+        return 1;
+    }
+    obj_t *obj = thing->payload._ptr;
+    for (size_t i = 0; i < obj->len; i++) {
+        char *key = obj->pairs[i].k;
+        sentry_value_t src_val = obj->pairs[i].v;
+        sentry_value_t dst_val = sentry_value_get_by_key(dst, key);
+        if (sentry_value_get_type(dst_val) == SENTRY_VALUE_TYPE_OBJECT
+            && sentry_value_get_type(src_val) == SENTRY_VALUE_TYPE_OBJECT) {
+            if (sentry__value_merge_objects(dst_val, src_val) != 0) {
+                return 1;
+            }
+        } else {
+            if (sentry_value_set_by_key(dst, key, src_val) != 0) {
+                return 1;
+            }
+            sentry_value_incref(src_val);
+        }
+    }
+    return 0;
+}
+
 void
 sentry__jsonwriter_write_value(sentry_jsonwriter_t *jw, sentry_value_t value)
 {

diff --git a/src/sentry_value.h b/src/sentry_value.h
@@ -89,6 +89,22 @@ sentry_value_t sentry__value_clone(sentry_value_t value);
 int sentry__value_append_bounded(
     sentry_value_t value, sentry_value_t v, size_t max);
 
+/**
+ * Deep-merges object src into dst.
+ *
+ * For each key-value pair in the src object the same key in the dst object
+ * will be set to the value from src.  If both the dst value and the src value
+ * are objects themselves they are stepped into recursively instead of
+ * overriding the entire dst object.
+ *
+ * If src is null nothing needs to be merged and this is handled gracefully,
+ * otherwise if dst is any other type than an object or src is neither an
+ * object nor null an error is returned.
+ *
+ * Returns 0 on success.
+ */
+int sentry__value_merge_objects(sentry_value_t dst, sentry_value_t src);
+
 /**
  * Parse the given JSON string into a new Value.
  */

diff --git a/tests/unit/test_value.c b/tests/unit/test_value.c
@@ -229,6 +229,62 @@ SENTRY_TEST(value_object)
     sentry_value_decref(val);
 }
 
+SENTRY_TEST(value_object_merge)
+{
+    sentry_value_t dst = sentry_value_new_object();
+    sentry_value_set_by_key(dst, "a", sentry_value_new_int32(1));
+    sentry_value_set_by_key(dst, "b", sentry_value_new_int32(2));
+
+    sentry_value_t src = sentry_value_new_object();
+    sentry_value_set_by_key(src, "b", sentry_value_new_int32(20));
+    sentry_value_set_by_key(src, "c", sentry_value_new_int32(30));
+
+    int rv = sentry__value_merge_objects(dst, src);
+    TEST_CHECK_INT_EQUAL(rv, 0);
+    sentry_value_decref(src);
+
+    sentry_value_t a = sentry_value_get_by_key(dst, "a");
+    sentry_value_t b = sentry_value_get_by_key(dst, "b");
+    sentry_value_t c = sentry_value_get_by_key(dst, "c");
+    TEST_CHECK_INT_EQUAL(sentry_value_as_int32(a), 1);
+    TEST_CHECK_INT_EQUAL(sentry_value_as_int32(b), 20);
+    TEST_CHECK_INT_EQUAL(sentry_value_as_int32(c), 30);
+
+    sentry_value_decref(dst);
+}
+
+SENTRY_TEST(value_object_merge_nested)
+{
+    sentry_value_t dst = sentry_value_new_object();
+    sentry_value_set_by_key(dst, "a", sentry_value_new_int32(1));
+    sentry_value_t dst_nested = sentry_value_new_object();
+    sentry_value_set_by_key(dst_nested, "ba", sentry_value_new_int32(1));
+    sentry_value_set_by_key(dst_nested, "bb", sentry_value_new_int32(2));
+    sentry_value_set_by_key(dst, "b", dst_nested);
+
+    sentry_value_t src = sentry_value_new_object();
+    sentry_value_t src_nested = sentry_value_new_object();
+    sentry_value_set_by_key(src_nested, "bb", sentry_value_new_int32(20));
+    sentry_value_set_by_key(src_nested, "bc", sentry_value_new_int32(30));
+    sentry_value_set_by_key(src, "b", src_nested);
+
+    int rv = sentry__value_merge_objects(dst, src);
+    TEST_CHECK_INT_EQUAL(rv, 0);
+    sentry_value_decref(src);
+
+    sentry_value_t a = sentry_value_get_by_key(dst, "a");
+    sentry_value_t nested = sentry_value_get_by_key(dst, "b");
+    sentry_value_t ba = sentry_value_get_by_key(nested, "ba");
+    sentry_value_t bb = sentry_value_get_by_key(nested, "bb");
+    sentry_value_t bc = sentry_value_get_by_key(nested, "bc");
+    TEST_CHECK_INT_EQUAL(sentry_value_as_int32(a), 1);
+    TEST_CHECK_INT_EQUAL(sentry_value_as_int32(ba), 1);
+    TEST_CHECK_INT_EQUAL(sentry_value_as_int32(bb), 20);
+    TEST_CHECK_INT_EQUAL(sentry_value_as_int32(bc), 30);
+
+    sentry_value_decref(dst);
+}
+
 SENTRY_TEST(value_freezing)
 {
     sentry_value_t val = sentry_value_new_list();

diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc
@@ -79,6 +79,8 @@ XX(value_json_surrogates)
 XX(value_list)
 XX(value_null)
 XX(value_object)
+XX(value_object_merge)
+XX(value_object_merge_nested)
 XX(value_string)
 XX(value_unicode)
 XX(value_wrong_type)