Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Map item iterators #1143

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@
**Breaking changes**:

- Return type of `sentry_capture_minidump()` and `sentry_capture_minidump_n()` changed from `void` to `sentry_uuid_t` to retrieve the event-id for a successful minidump upload. ([#1138](https://github.com/getsentry/sentry-native/pull/1138))


**Features**:

- Add object item iterators. ([#1143](https://github.com/getsentry/sentry-native/pull/1143))

**Fixes**:

- Ensure that `sentry_capture_minidump()` fails if the provided minidump path cannot be attached, instead of sending a crash event without minidump. ([#1138](https://github.com/getsentry/sentry-native/pull/1138))
Expand Down
48 changes: 48 additions & 0 deletions include/sentry.h
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,54 @@ SENTRY_API int sentry_value_remove_by_key(sentry_value_t value, const char *k);
SENTRY_API int sentry_value_remove_by_key_n(
sentry_value_t value, const char *k, size_t k_len);

/**
* Object item iterator.
*
* It's used to iterate over the key-value pairs of an object.
*/
struct sentry_item_iter_s;
typedef struct sentry_item_iter_s sentry_item_iter_t;

/**
* Creates a new object item iterator.
*
* Returns `NULL` if the given value is not an object.
*/
SENTRY_API sentry_item_iter_t *sentry_value_new_item_iter(sentry_value_t value);

/**
* Advances the item iterator to the next item.
*/
SENTRY_API void sentry_value_item_iter_next(sentry_item_iter_t *item_iter);

/**
* Returns true if the item iterator is valid.
*/
SENTRY_API int sentry_value_item_iter_valid(sentry_item_iter_t *item_iter);

/**
* Returns the key to the current item.
*
* Returns a `NULL` pointer if the iterator is invalid.
*/
SENTRY_API const char *sentry_value_item_iter_get_key(
sentry_item_iter_t *item_iter);

/**
* Returns the value of the current item.
*
* Returns a null value if the iterator is invalid.
*/
SENTRY_API sentry_value_t sentry_value_item_iter_get_value(
sentry_item_iter_t *item_iter);

/**
* Erases the current item and advances the iterator to the next item.
*
* Returns 1 if the iterator is exhausted or the object is frozen.
*/
SENTRY_API int sentry_value_item_iter_erase(sentry_item_iter_t *item_iter);

/**
* Appends a value to a list.
*
Expand Down
69 changes: 69 additions & 0 deletions src/sentry_value.c
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,75 @@ sentry_value_get_by_key_owned(sentry_value_t value, const char *k)
return rv;
}

struct sentry_item_iter_s {
size_t *len; // Pointer to length!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having len in the iterator and the actual data structure be independent was a choice since it clarifies the immutability of the iterated object.

If we introduce a pointer here, its validity becomes part of the iterator invariant and must be checked accordingly.

I am not against introducing in-place erasure, but as you can see, it complicates the user guarantees, which you can't change without breaking the code, even if you keep the interfaces the same.

To be clear, you can achieve the same filtering by populating a new object with only the unfiltered items. The cost will be negligible in most cases (considering that practically anything you interact with is a pointer or a thing the size of a pointer), and you will have to make fewer decisions regarding the API and its guarantees.

We can still introduce in-place erasure later if someone finds "copy filtering" too costly (either due to required memory size or allocation cost). I also wonder if most scenarios want to remove items of the original object they are iterating. Would you happen to have an example?

Again, I am not entirely against introducing this; we should just re-evaluate the need vs the cost.

obj_pair_t *pairs;
size_t index;
int frozen;
};

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;
item_iter->frozen = thing_is_frozen(thing);
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If item_iter->len is ever NULL at this point, the process will crash.

return NULL;
}
return item_iter->pairs[item_iter->index].k;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If item_iter->pairs is ever NULL at this point, the process will crash.

}

sentry_value_t
sentry_value_item_iter_get_value(sentry_item_iter_t *item_iter)
{
if (item_iter->index >= *item_iter->len) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

return sentry_value_new_null();
}
return item_iter->pairs[item_iter->index].v;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we check pairs here, we should check len too.

}

int
sentry_value_item_iter_erase(sentry_item_iter_t *item_iter)
{
if (item_iter->frozen || item_iter->index >= *item_iter->len) {
return 1;
}
obj_pair_t *pair = &item_iter->pairs[item_iter->index];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, do we assume that pairs and len are valid pointers in this case?

If so, why? Because valid() does the check? If so, we must decide whether users must check for validity every time either the iterator or the object changed and erase(), get_key() and get_value() assume valid iterators or add the checks in all of those functions.

Whichever way we choose, we should be consistent regarding the guarantees of calling valid(), the accessors, and mutators (since adding new keys will then also be used if iterators allow object mutation).

This demonstrates how easily checks (and thus guarantees) can become inconsistent quickly when an in-place erasure is introduced. If you introduce the guarantees for each getter and mutator (making valid() solely a function for a loop-invariant), then you can't change that without breaking code. OTOH, adding those guarantees later won't break any code.

sentry_free(pair->k);
sentry_value_decref(pair->v);
memmove(item_iter->pairs + item_iter->index,
item_iter->pairs + item_iter->index + 1,
(*item_iter->len - item_iter->index - 1) * sizeof(item_iter->pairs[0]));
(*item_iter->len)--;
return 0;
}

sentry_value_t
sentry_value_get_by_index(sentry_value_t value, size_t index)
{
Expand Down
88 changes: 88 additions & 0 deletions tests/unit/test_value.c
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,94 @@ SENTRY_TEST(value_object)
sentry_value_decref(val);
}

SENTRY_TEST(value_object_iteration)
{
sentry_value_t obj = sentry_value_new_object();

// Populate.
for (size_t i = 0; i < 10; i++) {
char key[100];
sprintf(key, "key%d", (int)i);
sentry_value_set_by_key(obj, key, sentry_value_new_int32((int32_t)i));
}

// Iterate over items.
{
sentry_item_iter_t *it = sentry_value_new_item_iter(obj);
size_t count = 0;
TEST_CHECK(it != NULL);
for (; sentry_value_item_iter_valid(it);
sentry_value_item_iter_next(it)) {
const char *key = sentry_value_item_iter_get_key(it);
sentry_value_t value = sentry_value_item_iter_get_value(it);

TEST_CHECK(key != NULL);
TEST_CHECK(sentry_value_get_type(value) == SENTRY_VALUE_TYPE_INT32);

int32_t key_idx;
sscanf(key, "key%d", &key_idx);
TEST_CHECK_INT_EQUAL(key_idx, sentry_value_as_int32(value));

count++;
}
TEST_CHECK_INT_EQUAL(count, 10);
sentry_free(it);
}

// Erase even-numbered items.
{
sentry_item_iter_t *it = sentry_value_new_item_iter(obj);
TEST_CHECK(it != NULL);
size_t count = 0;
const char *prev_key = "";
size_t i = 0;
while (sentry_value_item_iter_valid(it)) {
TEST_CHECK(
strcmp(prev_key, sentry_value_item_iter_get_key(it)) != 0);
prev_key = sentry_value_item_iter_get_key(it);
if (i % 2 == 0) {
int err = sentry_value_item_iter_erase(it);
TEST_CHECK_INT_EQUAL(err, 0);
} else {
sentry_value_item_iter_next(it);
count++;
}
i++;
}
TEST_CHECK_INT_EQUAL(sentry_value_get_length(obj), 5);
TEST_CHECK_INT_EQUAL(count, 5);
sentry_free(it);
}

// Verify if the right items were removed.
{
sentry_item_iter_t *it = sentry_value_new_item_iter(obj);
for (; sentry_value_item_iter_valid(it);
sentry_value_item_iter_next(it)) {
const char *key = sentry_value_item_iter_get_key(it);
int32_t key_idx;
sscanf(key, "key%d", &key_idx);
TEST_CHECK(key_idx % 2 != 0);
}
sentry_free(it);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep the mutating iterator, please also add a loop that adds keys or documents that this is forbidden.

// Erase the rest of the items.
{
sentry_item_iter_t *it = sentry_value_new_item_iter(obj);
size_t count = 0;
TEST_CHECK(it != NULL);
while (sentry_value_item_iter_erase(it) == 0) {
count++;
}
TEST_CHECK_INT_EQUAL(sentry_value_get_length(obj), 0);
TEST_CHECK_INT_EQUAL(count, 5);
sentry_free(it);
}

sentry_value_decref(obj);
}

SENTRY_TEST(value_object_merge)
{
sentry_value_t dst = sentry_value_new_object();
Expand Down
Loading