feat: Add sentry_value_get_key(value, index)

[limbonaut]

This PR introduces the sentry_value_get_key(value, index) function.

Rationale

There is currently no way to get the keys of a map object. We need such a function to build a proper representation in upstream SDKs (like sentry-godot). We can potentialy improve existing code in other SDKs if we add this function. It can also be used to find which contexts or tags are added to a sentry event and present such data through SentryEvent class.

sentry_value_get_length(value) is already set up to return the number of elements in a map. This PR simply adds the last missing piece - the sentry_value_get_key(value, index) function.

How we will use it

WIP upstream SDK code that benefits from these changes:

• https://github.com/getsentry/sentry-godot/blob/2fae26ef6151ef983b45d5c97280f66545c3b64d/src/sentry/native/native_breadcrumb.cpp#L47-L54
• https://github.com/getsentry/sentry-godot/blob/2fae26ef6151ef983b45d5c97280f66545c3b64d/src/sentry/native/native_util.cpp#L85-L95

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against d105f15

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.65%. Comparing base (03143a8) to head (d105f15).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1142      +/-   ##
==========================================
- Coverage   82.66%   82.65%   -0.02%     
==========================================
  Files          53       53              
  Lines        7927     7937      +10     
  Branches     1239     1241       +2     
==========================================
+ Hits         6553     6560       +7     
- Misses       1263     1266       +3     
  Partials      111      111              

[supervacuus]

Hi @limbonaut! Providing a way to iterate the items (key and value) of the object value type is very useful. However, I would prefer not to expose implementation details or keep compatibility with an index-based lookup if we change internal structures in the future since the index is meaningless in the object's context, and it might give clients weird ideas about the stability of such an index.

Since the primary use-case seems to be iteration, using the key to retrieve the value, I suggest having a stateful iterator design instead of an index-based lookup. Something like this:

for (item_iter_t iter = item_iter(obj); is_item_iter_valid(iter); item_iter_next(&iter)) {
    const char* key = item_iter_get_key(iter);
    value_t value = item_iter_get_value(iter); // Borrowed (== ref-count stays the same, the client can always `inc` if they want to hold on)
}

This is undoubtedly more upfront effort but a sensible mid- to long-term solution. The iterator can be an opaque pointer hiding any details of the implementation. In the first version, the iterator wouldn't provide guarantees if the object is modified concurrently (i.e., any modification to the object invalidates any iterator, similar to how the index lookup cannot offer any guarantees), which we could later improve.

[limbonaut]

Hey @supervacuus! Indeed, the main use case is iteration, and I'm glad this functionality is considered useful. Your suggestions sound so much better than what I proposed. I'll try to present a new PR using the stateful iterator approach in a bit. Thanks for looking into it!

[supervacuus]

I quickly hacked up a basic implementation yesterday for you to play with.

sentry.h

struct sentry_item_iter_s;
typedef struct sentry_item_iter_s sentry_item_iter_t;

SENTRY_API sentry_item_iter_t *sentry_value_new_item_iter(sentry_value_t value);
SENTRY_API void sentry_value_item_iter_next(sentry_item_iter_t *item_iter);
SENTRY_API int sentry_value_item_iter_valid(sentry_item_iter_t *item_iter);
SENTRY_API const char *sentry_value_item_iter_get_key(
    sentry_item_iter_t *item_iter);
SENTRY_API sentry_value_t sentry_value_item_iter_get_value(
    sentry_item_iter_t *item_iter);

sentry_value.c

struct sentry_item_iter_s {
    size_t len;
    obj_pair_t *pairs;
    size_t index;
};

sentry_item_iter_t *
sentry_value_new_item_iter(sentry_value_t value)
{
    const thing_t *thing = value_as_thing(value);
    if (thing && thing_get_type(thing) == THING_TYPE_OBJECT) {
        obj_t *o = thing->payload._ptr;
        sentry_item_iter_t *item_iter = SENTRY_MAKE(sentry_item_iter_t);
        item_iter->len = o->len;
        item_iter->pairs = o->pairs;
        item_iter->index = 0;
        return item_iter;
    }
    return NULL;
}

void sentry_value_item_iter_next(sentry_item_iter_t *item_iter)
{
    item_iter->index++;
}

const char *
sentry_value_item_iter_get_key(sentry_item_iter_t *item_iter)
{
    if (item_iter->index >= item_iter->len) {
        return NULL;
    }
    return item_iter->pairs[item_iter->index].k;
}

sentry_value_t
sentry_value_item_iter_get_value(sentry_item_iter_t *item_iter)
{
    if (item_iter->index >= item_iter->len) {
        return sentry_value_new_null();
    }
    return item_iter->pairs[item_iter->index].v;
}

int
sentry_value_item_iter_valid(sentry_item_iter_t *item_iter)
{
    return item_iter->index < item_iter->len && item_iter->pairs != NULL;
}

Which can be used like this:

sentry_item_iter_t *item_iter = sentry_value_new_item_iter(val);
for (; sentry_value_item_iter_valid(item_iter);
         sentry_value_item_iter_next(item_iter)) {
    const char *item_key = sentry_value_item_iter_get_key(item_iter);
    sentry_value_t item_value = sentry_value_item_iter_get_value(item_iter);
    // ...
}
sentry_free(item_iter);

As you can see, there are still open design/implementation questions. For instance, I was defensive in the get_key() and get_value() functions. One could require the client to check validity before the accessor; in that case, we could remove the check. In any case, you can pick up or ignore it.

[limbonaut]

@supervacuus
I've tried your code and it works great. I also measured it with and without the defensive guards: for traversing event.contexts there was no tangible difference, at least for our use case. Having such safety measures is probably for the best. I've added a branch for the new code, feel free to commit directly to it: #1143

I'm trialing it with my PR branch which wants this functionality over here: getsentry/sentry-godot#83

I'm contemplating adding sentry_value_iter_item_erase(). It seems useful for filtering. How do you feel about it?

[limbonaut]

Closed in favor of #1143