Skip to content

Add config delete #1

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.

Sign up for GitHub

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

Open
aws-kevinrickard wants to merge 3 commits into
base: config-empty-object-support
Choose a base branch
from
Open

Add config delete #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fde1102
Add support for empty objects in config (#683)
aws-kevinrickard Dec 5, 2024
8d87515
Fix JSON unicode escape decoding
archigup Dec 5, 2024
8a9ccda
Add config delete
aws-kevinrickard Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
260 changes: 260 additions & 0 deletions configtest/bin/configtest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,50 @@ static void test_get(
}
}

static void test_delete(GglList key_path, GglError expected_result) {
GGL_LOGD(
"test_delete %s, expecting %s",
print_key_path(&key_path),
ggl_strerror(expected_result)
);
GglBuffer server = GGL_STR("gg_config");

GglMap params = GGL_MAP({ GGL_STR("key_path"), GGL_OBJ_LIST(key_path) }, );

GglError remote_error = GGL_ERR_OK;
GglError error = ggl_call(
server, GGL_STR("delete"), params, &remote_error, NULL, NULL
);
if (expected_result != GGL_ERR_OK && error != GGL_ERR_REMOTE) {
GGL_LOGE(
"delete key %s expected result %s but there was not a remote error",
print_key_path(&key_path),
ggl_strerror(expected_result)
);
assert(0);
}
if (expected_result == GGL_ERR_OK && error != GGL_ERR_OK) {
GGL_LOGE(
"delete key %s did not expect error but got error %s and remote "
"error %s",
print_key_path(&key_path),
ggl_strerror(error),
ggl_strerror(remote_error)
);
assert(0);
}
if (remote_error != expected_result) {
GGL_LOGE(
"delete key %s expected result %s but got %s",
print_key_path(&key_path),
ggl_strerror(expected_result),
ggl_strerror(remote_error)
);
assert(0);
return;
}
}

static GglError subscription_callback(
void *ctx, unsigned int handle, GglObject data
) {
Expand Down Expand Up @@ -899,6 +943,222 @@ int main(int argc, char **argv) {
GGL_ERR_OK
);

// Test to ensure a key can be deleted, not affecting its parent
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component13")), GGL_OBJ_BUF(GGL_STR("key"))
),
GGL_OBJ_BUF(GGL_STR("value")),
-1,
GGL_ERR_OK
);
test_delete(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component13")), GGL_OBJ_BUF(GGL_STR("key"))
),
GGL_ERR_OK
);
test_get(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component13")), GGL_OBJ_BUF(GGL_STR("key"))
),
GGL_OBJ_NULL(),
GGL_ERR_NOENTRY
);
test_get(
GGL_LIST(GGL_OBJ_BUF(GGL_STR("component13"))),
GGL_OBJ_MAP(GGL_MAP()),
GGL_ERR_OK
);

// Test to ensure deletes are recursive
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component14")),
GGL_OBJ_BUF(GGL_STR("foo")),
GGL_OBJ_BUF(GGL_STR("bar"))
),
GGL_OBJ_BUF(GGL_STR("value")),
-1,
GGL_ERR_OK
);
test_delete(GGL_LIST(GGL_OBJ_BUF(GGL_STR("component14"))), GGL_ERR_OK);
test_get(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component14")),
GGL_OBJ_BUF(GGL_STR("foo")),
GGL_OBJ_BUF(GGL_STR("bar"))
),
GGL_OBJ_NULL(),
GGL_ERR_NOENTRY
);
test_get(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component14")), GGL_OBJ_BUF(GGL_STR("foo"))
),
GGL_OBJ_NULL(),
GGL_ERR_NOENTRY
);
test_get(
GGL_LIST(GGL_OBJ_BUF(GGL_STR("component14"))),
GGL_OBJ_NULL(),
GGL_ERR_NOENTRY
);

// Test to ensure an empty map can be written and read
test_insert(
GGL_LIST(GGL_OBJ_BUF(GGL_STR("component15"))),
GGL_OBJ_MAP(GGL_MAP()),
-1,
GGL_ERR_OK
);
test_get(
GGL_LIST(GGL_OBJ_BUF(GGL_STR("component15"))),
GGL_OBJ_MAP(GGL_MAP()),
GGL_ERR_OK
);

// Test to ensure an empty map can be merged into an existing empty map
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component16")), GGL_OBJ_BUF(GGL_STR("foo"))
),
GGL_OBJ_MAP(GGL_MAP()),
-1,
GGL_ERR_OK
);
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component16")), GGL_OBJ_BUF(GGL_STR("foo"))
),
GGL_OBJ_MAP(GGL_MAP()),
-1,
GGL_ERR_OK
);
test_get(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component16")), GGL_OBJ_BUF(GGL_STR("foo"))
),
GGL_OBJ_MAP(GGL_MAP()),
GGL_ERR_OK
);

// Test to ensure an empty map can be merged into an existing populated map
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component17")), GGL_OBJ_BUF(GGL_STR("foo"))
),
GGL_OBJ_MAP(GGL_MAP({ GGL_STR("key"), GGL_OBJ_NULL() })),
-1,
GGL_ERR_OK
);
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component17")), GGL_OBJ_BUF(GGL_STR("foo"))
),
GGL_OBJ_MAP(GGL_MAP()),
-1,
GGL_ERR_OK
);
test_get(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component17")), GGL_OBJ_BUF(GGL_STR("foo"))
),
GGL_OBJ_MAP(GGL_MAP({ GGL_STR("key"), GGL_OBJ_NULL() })),
GGL_ERR_OK
);

// Test to ensure an empty map can not be merged into an existing value
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component18")), GGL_OBJ_BUF(GGL_STR("foo"))
),
GGL_OBJ_MAP(GGL_MAP({ GGL_STR("key"), GGL_OBJ_NULL() })),
-1,
GGL_ERR_OK
);
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component18")),
GGL_OBJ_BUF(GGL_STR("foo")),
GGL_OBJ_BUF(GGL_STR("key"))
),
GGL_OBJ_MAP(GGL_MAP()),
-1,
GGL_ERR_FAILURE
);

// Test to ensure an value can not be merged into an existing empty map
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component19")),
GGL_OBJ_BUF(GGL_STR("foo")),
GGL_OBJ_BUF(GGL_STR("key"))
),
GGL_OBJ_MAP(GGL_MAP()),
-1,
GGL_ERR_OK
);
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component19")),
GGL_OBJ_BUF(GGL_STR("foo")),
GGL_OBJ_BUF(GGL_STR("key"))
),
GGL_OBJ_NULL(),
-1,
GGL_ERR_FAILURE
);

// Test to check subscriber behavior on deleted keys
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component20")),
GGL_OBJ_BUF(GGL_STR("foo")),
GGL_OBJ_BUF(GGL_STR("key"))
),
GGL_OBJ_BUF(GGL_STR("value1")),
-1,
GGL_ERR_OK
);
test_subscribe(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component20")),
GGL_OBJ_BUF(GGL_STR("foo")),
GGL_OBJ_BUF(GGL_STR("key"))
),
GGL_ERR_OK
);
test_subscribe(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component20")), GGL_OBJ_BUF(GGL_STR("foo"))
),
GGL_ERR_OK
);
test_delete(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component20")),
GGL_OBJ_BUF(GGL_STR("foo")),
GGL_OBJ_BUF(GGL_STR("key"))
),
GGL_ERR_OK
);
test_insert(
GGL_LIST(
GGL_OBJ_BUF(GGL_STR("component20")),
GGL_OBJ_BUF(GGL_STR("foo")),
GGL_OBJ_BUF(GGL_STR("key"))
),
GGL_OBJ_BUF(GGL_STR("value2")),
-1,
GGL_ERR_OK
); // Should see one `read component20/foo/key` on the callback handle
// created for component20/foo
// Currently, the other subscription callback for component20/foo/key is not
// notified. In the future, it would be good to have that behavior too. See
// the docs/design/ggconfigd.md section "Subscription behavior for keys
// which become deleted" for more info.

// test_insert(
// GGL_LIST(GGL_OBJ_BUF(GGL_STR("component")),
// GGL_OBJ_BUF(GGL_STR("bar"))), GGL_OBJ_MAP(GGL_MAP({ GGL_STR("foo"),
Expand Down
3 changes: 3 additions & 0 deletions core-bus-gg-config/include/ggl/core_bus/gg_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ GglError ggl_gg_config_write(
GglBufList key_path, GglObject value, const int64_t *timestamp
);

/// Wrapper for core-bus `gg_config` `delete`
GglError ggl_gg_config_delete(GglBufList key_path);

/// Wrapper for core-bus `gg_config` `subscribe`
GglError ggl_gg_config_subscribe(
GglBufList key_path,
Expand Down
28 changes: 28 additions & 0 deletions core-bus-gg-config/src/gg_config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,34 @@ GglError ggl_gg_config_read(
return err;
}

GglError ggl_gg_config_delete(GglBufList key_path) {
if (key_path.len > GGL_MAX_OBJECT_DEPTH) {
GGL_LOGE("Key path depth exceeds maximum handled.");
return GGL_ERR_UNSUPPORTED;
}

GglObject path_obj[GGL_MAX_OBJECT_DEPTH] = { 0 };
for (size_t i = 0; i < key_path.len; i++) {
path_obj[i] = GGL_OBJ_BUF(key_path.bufs[i]);
}

GglMap args = GGL_MAP(
{ GGL_STR("key_path"),
GGL_OBJ_LIST((GglList) { .items = path_obj, .len = key_path.len }) },
);

GglError remote_err = GGL_ERR_OK;
GglError err = ggl_call(
GGL_STR("gg_config"), GGL_STR("delete"), args, &remote_err, NULL, NULL
);

if ((err == GGL_ERR_REMOTE) && (remote_err != GGL_ERR_OK)) {
err = remote_err;
}

return err;
}

GglError ggl_gg_config_read_str(GglBufList key_path, GglBuffer *result) {
GglObject result_obj;
GglBumpAlloc alloc = ggl_bump_alloc_init(*result);
Expand Down
34 changes: 34 additions & 0 deletions docs/design/ggconfigd.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,3 +232,37 @@ the future).
non-existent keys or maintained for keys which become non-existent.
- B. Have a temp table for pending subscriptions in addition to the active
subscriptions

### Storing empty maps

Empty maps (or empty objects) are valid JSON, so we need to support them. This
is represented in the database when a key has neither a value nor any children.
In other words, if a key doesn't have a value, it is a map/object, and the
number of children is how many fields are in that map/object, which can be zero.

You can't write/merge a value onto an existing empty map/object or vice versa,
just like you can't write/merge a value onto an existing map/object.

Writing an empty map doesn't trigger any update subscription callbacks, because
no values were written to notify on.

### (Not) considering timestamps when deleting keys

With GG Classic, if deployments that reset (delete) parts of the config are
applied out of order, they can result in a different resulting config state, and
deployment ordering behavior does not apply to configs which were deleted.
Config deletes are always applied at the time of deployment processing. This is
because with GG classic, deleted keys do not have any state (notably a timestamp
of a potential previous deletion) associated with them, and timestamps are not
checked before resetting config keys. Lite replicates this, and also does not
store any state about deleted keys currently.

In the future it would be good to add in this deleted-key state with the
associated timestamp, and check timestamps before deleting keys. This would
result in deployment ordering behavior being preserved and make GG configuration
more consistent, simpler to understand, and less likely to cause application
bugs. And it would also set us up for improving the
[Subscription behavior for keys which become deleted](#subscription-behavior-for-keys-which-become-deleted).
However it may be a breaking change from a Greengrass behavior perspective, and
will need careful consideration in how to make this improvement without
disturbing existing applications which may depend on it for some reason.
8 changes: 5 additions & 3 deletions docs/spec/core-bus-interface/gg_config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,9 @@ The `write` method does not have a response value.

## delete

The `delete` method removes the key and value associated with `key_path`. If the
value is a map with subkeys, everything under it is also deleted recursively.
The `delete` method removes the key and value associated with `key_path`, if
present. If the value is a map with subkeys, everything under it is also deleted
recursively. If the key does not exist, nothing happens.

- [gg-config-delete-1] `delete` can be invoked with call or notify.

Expand All @@ -80,7 +81,8 @@ value is a map with subkeys, everything under it is also deleted recursively.
The `delete` method does not have a response value.

- [gg-config-delete-resp-1] If the method returns without an error, the
configuration has been successfully deleted.
configuration key does not exist, either because it was deleted or it didn't
exist when the call was made.

## subscribe

Expand Down
2 changes: 2 additions & 0 deletions ggconfigd/include/ggconfigd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
GglError ggconfig_write_value_at_key(
GglList *key_path, GglBuffer *value, int64_t timestamp
);
GglError ggconfig_write_empty_map(GglList *key_path);
GglError ggconfig_delete_key(GglList *key_path);
GglError ggconfig_get_value_from_key(GglList *key_path, GglObject *value);
GglError ggconfig_get_key_notification(GglList *key_path, uint32_t handle);
GglError ggconfig_open(void);
Expand Down
Loading