CFE-4562: classfilterdata(): added support of object of arrays and object of objects #5850

larsewi · 2025-08-05T14:31:25Z

classfilterdata() now supports object of objects
classfilterdata() now supports object of arrays
classfilterdata(): deduped code for getting class expression
classfilterdata(): added examples for object of arrays/objects

libpromises/evalfunction.c

larsewi · 2025-08-06T09:31:02Z

@cf-bottom Jenkins please :)

The class expression can be stored in either the objects exogenous key (i.e., the key to the object itself) or the value of a specified endogenous key (i.e., a key inside the object). Ticket: CFE-4562 Changelog: Commit Signed-off-by: Lars Erik Wik <[email protected]>

The class expression can be stored in either the objects exogenous key (i.e., the key to the object itself) or the value of a specified endogenous index (i.e., a index inside the object). Changelog: Commit Signed-off-by: Lars Erik Wik <[email protected]>

Signed-off-by: Lars Erik Wik <[email protected]>

cf-bottom · 2025-08-06T11:14:58Z

Alright, I triggered a build:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/12453/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-12453/

…/objects" This commit broke docs because it depends on cfengine/core#5850. Once the dependency is merged, it should be safe to reapply this commit. This reverts commit 3bc6348. Ticket: CFE-4562 Signed-off-by: Lars Erik Wik <[email protected]>

victormlg

Cool!

craigcomstock

looks great! I would generally suggest replacing exogenous and the other term with more plain language that will be easier for maintenance programmers to handle.

craigcomstock · 2025-08-13T14:12:54Z

libpromises/evalfunction.c

@@ -7864,6 +7865,13 @@ static bool ClassFilterDataArray(
    JsonElement *child,
    bool *remove)
 {
+    if (key_or_index == NULL) {
+        Log(LOG_LEVEL_VERBOSE,
+            "Function %s(): At least 3 arguments are required when the data structure is an array of arrays or array of objects",


maybe you could mention the third parameter is key or index so they don't have to check the manual.

craigcomstock · 2025-08-13T15:36:46Z

libpromises/evalfunction.c

+    ARG_UNUSED const char *fn_name,
+    ARG_UNUSED JsonElement *json_array,
+    ARG_UNUSED const char *class_expr_index,
+    ARG_UNUSED const char *exogenous_key,


I wouldn't prefer the word exogenous as it is too specifically linguistically technical. Maybe self_key? 🚲 🏠 (bikeshed todo). Ideally you want to write code that a less smart maintainer can handle :)

craigcomstock · 2025-08-13T15:37:37Z

libpromises/evalfunction.c

+    const char *fn_name,
+    JsonElement *json_object,
+    const char *class_expr_key,
+    const char *exogenous_key,


maybe self_key?

craigcomstock · 2025-08-13T15:39:16Z

libpromises/evalfunction.c

+        if (json_child == NULL)
+        {
+            Log(LOG_LEVEL_VERBOSE,
+                "Function %s(): Bad class expression key '%s': Key not found",


if we aren't validating if it is a "bad class expression" aka somehow not a well formed class expression then this message should only say "Function %s(): Key not found with class expression '%s'".

Do we have a function that can validate a class expression? Should add that to a static checking tool like cf-promises?

ah, I see, asserting that the class expression is not empty, maybe just say "Looking for key in the object itself and found none." or similar.

craigcomstock · 2025-08-13T15:43:54Z

libpromises/evalfunction.c

+                ctx, fn_name, child, key_or_index, exogenous_key, remove);
+        }
+        Log(LOG_LEVEL_VERBOSE,
+            "Function %s(): Expected child element to be of container type array",


would it be helpful to mention the type the item WAS? aka "to be of container type array but was %s", data_structure

craigcomstock · 2025-08-13T15:50:30Z

libpromises/evalfunction.c

+
+            while ((child != NULL))
+            {
+                const char *exogenous_key = JsonIteratorCurrentKey(&iter);


again, I think you should change exogenous_key to self_key or something that is easier to understand.

craigcomstock · 2025-08-13T15:51:17Z

libpromises/evalfunction.c

+                if (remove)
+                {
+                    NDEBUG_UNUSED bool success = JsonObjectRemoveKey(parent, exogenous_key);
+                    assert(success);


why no production-time error handling here?

It fails if the key-value pair you are trying to remove does not exist. However, we already know that it does. Thus, it should never return false.

craigcomstock · 2025-08-13T15:52:53Z

libpromises/evalfunction.c

+            /* Only parent type array and object is supported */
+            Log(LOG_LEVEL_VERBOSE,
+                "Function %s(): Expected parent element to be of container type array or object",


maybe remove the comment since that is in the log message and remove "array or object" and just leave it at "container type"? Or maybe even remove "container type" and just specify "array or object type" since those are more generally understandable than our special use of the term "container".

craigcomstock · 2025-08-13T15:54:41Z

tests/acceptance/01_vars/02_functions/classfilterdata_object_of_objects_endogenous.cf

+{
+  meta:
+    "description" -> { "CFE-4562" }
+      string => "Test for expected results from classfilterdata() with object of objects using endogenous key";


replace endogenous key with something less technical, as I suggested "self_key" for "exogenous_key", maybe this would be "internal_key"?

craigcomstock · 2025-08-13T15:56:33Z

libpromises/evalfunction.c

+        assert(SIZE_MAX >= ULONG_MAX); /* make sure returned value can fit in size_t */
+        if (StringToUlong(class_expr_index, &index) != 0) {
+            Log(LOG_LEVEL_VERBOSE,
+                "Function %s(): Bad class expression index '%s': Failed to parse integer",


is an index a class expression? shouldn't that just be an index, aka integer and specific to this function as an argument.

larsewi force-pushed the classfilterdata-i2 branch from 0e7d1e7 to d96b4fe Compare August 5, 2025 14:35

github-advanced-security bot found potential problems Aug 5, 2025

View reviewed changes

libpromises/evalfunction.c Fixed Show fixed Hide fixed

libpromises/evalfunction.c Fixed Show fixed Hide fixed

libpromises/evalfunction.c Fixed Show fixed Hide fixed

libpromises/evalfunction.c Fixed Show fixed Hide fixed

larsewi force-pushed the classfilterdata-i2 branch 4 times, most recently from 74876c4 to e73693f Compare August 5, 2025 15:51

larsewi marked this pull request as ready for review August 5, 2025 21:02

larsewi requested review from olehermanse and craigcomstock August 5, 2025 21:03

larsewi force-pushed the classfilterdata-i2 branch 2 times, most recently from 96a4953 to b7a2d8d Compare August 6, 2025 09:30

larsewi mentioned this pull request Aug 6, 2025

CFE-4562: classfilterdata(): added docs for object of arrays/objects cfengine/documentation#3483

Merged

larsewi added 2 commits August 6, 2025 12:27

larsewi force-pushed the classfilterdata-i2 branch from b7a2d8d to 9819c78 Compare August 6, 2025 10:27

larsewi added 2 commits August 6, 2025 13:08

classfilterdata(): deduped code for getting class expression

e7dfc98

Signed-off-by: Lars Erik Wik <[email protected]>

classfilterdata(): added examples for object of arrays/objects

1ef4707

Signed-off-by: Lars Erik Wik <[email protected]>

larsewi force-pushed the classfilterdata-i2 branch from 9819c78 to 1ef4707 Compare August 6, 2025 11:08

cfengine deleted a comment from cf-bottom Aug 6, 2025

larsewi mentioned this pull request Aug 11, 2025

Reapply "CFE-4562: classfilterdata(): added docs for object of arrays/objects" cfengine/documentation#3488

Draft

larsewi requested review from jakub-nt and victormlg August 12, 2025 10:55

jakub-nt approved these changes Aug 12, 2025

View reviewed changes

victormlg approved these changes Aug 13, 2025

View reviewed changes

craigcomstock reviewed Aug 13, 2025

View reviewed changes

CFE-4562: classfilterdata(): added support of object of arrays and object of objects #5850

Are you sure you want to change the base?

CFE-4562: classfilterdata(): added support of object of arrays and object of objects #5850

Uh oh!

Conversation

larsewi commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larsewi commented Aug 6, 2025

Uh oh!

cf-bottom commented Aug 6, 2025

Uh oh!

victormlg left a comment

Choose a reason for hiding this comment

Uh oh!

craigcomstock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

larsewi commented Aug 5, 2025 •

edited

Loading