make edits return fail when aborting the edit changes

make transactions default to "fail=1", which is much more useful
than bailing (for now).

remove auto-grouping of edits in a "group", and require the use
of "transaction"

Clean up the tests so that they pass under the new framework

update documentation to match

diff --git a/doc/antora/modules/reference/nav.adoc b/doc/antora/modules/reference/nav.adoc
index 0291272a1f8564be41cd04b4cd0f1bfcac9f9e64..25df91df3e8294ad2530cd02460094e71b1a8ee6 100644 (file)
--- a/doc/antora/modules/reference/nav.adoc
+++ b/doc/antora/modules/reference/nav.adoc
@@ -29,6 +29,7 @@
 **** xref:unlang/subrequest.adoc[subrequest]
 **** xref:unlang/switch.adoc[switch]
 **** xref:unlang/timeout.adoc[timeout]
+**** xref:unlang/transaction.adoc[transaction]
 **** xref:unlang/update.adoc[update]
 
 *** xref:unlang/module.adoc[Modules]

diff --git a/doc/antora/modules/reference/pages/unlang/group.adoc b/doc/antora/modules/reference/pages/unlang/group.adoc
index a4b6141e4b2f3e68ea896154abc26653235b5cfc..fb92488c0b501421d312238210a02258592ef891 100644 (file)
--- a/doc/antora/modules/reference/pages/unlang/group.adoc
+++ b/doc/antora/modules/reference/pages/unlang/group.adoc
@@ -37,12 +37,5 @@ group {
 
 The last entry in a `group` section can also be an xref:unlang/actions.adoc[actions] subsection.
 
-== Grouping Edits
-
-The `group` keyword can also be used to group multiple
-xref:unlang/edit.adoc[edit] instructions.  When edit instructions are
-grouped, then the edits are "atomic".  That is, either all of the
-edits succeed, or none of them do.
-
 // Copyright (C) 2021 Network RADIUS SAS.  Licenced under CC-by-NC 4.0.
 // This documentation was developed by Network RADIUS SAS.

diff --git a/src/lib/unlang/compile.c b/src/lib/unlang/compile.c
index b3af9d0f81546920175a81251725099d0d7b6567..8fc3fb4230475f8d014678629072444e9ebfcd08 100644 (file)
--- a/src/lib/unlang/compile.c
+++ b/src/lib/unlang/compile.c
@@ -94,7 +94,6 @@ typedef struct {
        rlm_components_t        component;
        char const              *section_name1;
        char const              *section_name2;
-       bool                    all_edits;
        unlang_actions_t        actions;
        tmpl_rules_t const      *rules;
 } unlang_compile_t;
@@ -1443,13 +1442,10 @@ static int unlang_fixup_edit(map_t *map, void *ctx)
 }
 
 /** Compile one edit section.
- *
- *  Edits which are adjacent to one another are automatically merged
- *  into one larger edit transaction.
  */
-static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_t *unlang_ctx, unlang_t **prev, CONF_SECTION *cs)
+static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_t *unlang_ctx, CONF_SECTION *cs)
 {
-       unlang_edit_t           *edit, *edit_free;
+       unlang_edit_t           *edit;
        unlang_t                *c, *out = UNLANG_IGNORE;
        map_t                   *map;
        char const              *name;
@@ -1484,29 +1480,20 @@ static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_t *unlang
        t_rules.attr.allow_unknown = true;
        RULES_VERIFY(&t_rules);
 
-       c = *prev;
-       edit = edit_free = NULL;
-
-       if (c && (c->type == UNLANG_TYPE_EDIT)) {
-               edit = unlang_generic_to_edit(c);
-
-       } else {
-               edit = talloc_zero(parent, unlang_edit_t);
-               if (!edit) return NULL;
+       edit = talloc_zero(parent, unlang_edit_t);
+       if (!edit) return NULL;
 
-               c = out = unlang_edit_to_generic(edit);
-               c->parent = parent;
-               c->next = NULL;
-               c->name = cf_section_name1(cs);
-               c->debug_name = c->name;
-               c->type = UNLANG_TYPE_EDIT;
-               c->ci = CF_TO_ITEM(cs);
+       c = out = unlang_edit_to_generic(edit);
+       c->parent = parent;
+       c->next = NULL;
+       c->name = cf_section_name1(cs);
+       c->debug_name = c->name;
+       c->type = UNLANG_TYPE_EDIT;
+       c->ci = CF_TO_ITEM(cs);
 
-               map_list_init(&edit->maps);
-               edit_free = edit;
+       map_list_init(&edit->maps);
 
-               compile_action_defaults(c, unlang_ctx);
-       }
+       compile_action_defaults(c, unlang_ctx);
 
        /*
         *      Allocate the map and initialize it.
@@ -1522,7 +1509,7 @@ static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_t *unlang
        if (slen <= 0) {
                cf_log_err(cs, "Failed parsing list reference %s - %s", name, fr_strerror());
        fail:
-               talloc_free(edit_free);
+               talloc_free(edit);
                return NULL;
        }
 
@@ -1614,18 +1601,15 @@ static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_t *unlang
 
        map_list_insert_tail(&edit->maps, map);
 
-       *prev = c;
        return out;
 }
 
 /** Compile one edit pair
  *
- *  Edits which are adjacent to one another are automatically merged
- *  into one larger edit transaction.
  */
-static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_t *unlang_ctx, unlang_t **prev, CONF_PAIR *cp)
+static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_t *unlang_ctx, CONF_PAIR *cp)
 {
-       unlang_edit_t           *edit, *edit_free;
+       unlang_edit_t           *edit;
        unlang_t                *c = NULL, *out = UNLANG_IGNORE;
        map_t                   *map;
        int                     num;
@@ -1640,32 +1624,20 @@ static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_t *unlang_ct
        t_rules.attr.allow_unknown = true;
        RULES_VERIFY(&t_rules);
 
-       /*
-        *      Auto-merge pairs only if they're all in a group.
-        */
-       if (unlang_ctx->all_edits) c = *prev;
-       edit = edit_free = NULL;
-
-       if (c && (c->type == UNLANG_TYPE_EDIT)) {
-               edit = unlang_generic_to_edit(c);
+       edit = talloc_zero(parent, unlang_edit_t);
+       if (!edit) return NULL;
 
-       } else {
-               edit = talloc_zero(parent, unlang_edit_t);
-               if (!edit) return NULL;
-
-               c = out = unlang_edit_to_generic(edit);
-               c->parent = parent;
-               c->next = NULL;
-               c->name = cf_pair_attr(cp);
-               c->debug_name = c->name;
-               c->type = UNLANG_TYPE_EDIT;
-               c->ci = CF_TO_ITEM(cp);
+       c = out = unlang_edit_to_generic(edit);
+       c->parent = parent;
+       c->next = NULL;
+       c->name = cf_pair_attr(cp);
+       c->debug_name = c->name;
+       c->type = UNLANG_TYPE_EDIT;
+       c->ci = CF_TO_ITEM(cp);
 
-               map_list_init(&edit->maps);
-               edit_free = edit;
+       map_list_init(&edit->maps);
 
-               compile_action_defaults(c, unlang_ctx);
-       }
+       compile_action_defaults(c, unlang_ctx);
 
        op = cf_pair_operator(cp);
        if ((op == T_OP_CMP_TRUE) || (op == T_OP_CMP_FALSE)) {
@@ -1679,7 +1651,7 @@ static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_t *unlang_ct
         */
        if (map_afrom_cp(edit, &map, map_list_tail(&edit->maps), cp, &t_rules, NULL, true) < 0) {
        fail:
-               talloc_free(edit_free);
+               talloc_free(edit);
                return NULL;
        }
 
@@ -1711,7 +1683,6 @@ static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_t *unlang_ct
 
        map_list_insert_tail(&edit->maps, map);
 
-       *prev = c;
        return out;
 }
 
@@ -2189,7 +2160,6 @@ static unlang_t *compile_children(unlang_group_t *g, unlang_compile_t *unlang_ct
        unlang_t        *c, *single;
        bool            was_if = false;
        char const      *skip_else = NULL;
-       unlang_t        *edit = NULL;
        unlang_compile_t *unlang_ctx;
        unlang_compile_t unlang_ctx2;
        tmpl_rules_t    t_rules;
@@ -2234,7 +2204,7 @@ static unlang_t *compile_children(unlang_group_t *g, unlang_compile_t *unlang_ct
                         *      In-line attribute editing.
                         */
                        if (*name == '&') {
-                               single = compile_edit_section(c, unlang_ctx, &edit, subcs);
+                               single = compile_edit_section(c, unlang_ctx, subcs);
                                if (!single) {
                                        talloc_free(c);
                                        return NULL;
@@ -2243,11 +2213,6 @@ static unlang_t *compile_children(unlang_group_t *g, unlang_compile_t *unlang_ct
                                goto add_child;
                        }
 
-                       /*
-                        *      Only merge edits if they're all in a group { ... }
-                        */
-                       if (!unlang_ctx->all_edits) edit = NULL;
-
                        if (strcmp(name, "actions") == 0) {
                                if (!compile_action_subsection(c, g->cs, subcs)) {
                                        talloc_free(c);
@@ -2316,7 +2281,7 @@ static unlang_t *compile_children(unlang_group_t *g, unlang_compile_t *unlang_ct
                         *      In-line attribute editing is not supported.
                         */
                        if (*attr == '&') {
-                               single = compile_edit_pair(c, unlang_ctx, &edit, cp);
+                               single = compile_edit_pair(c, unlang_ctx, cp);
                                if (!single) {
                                        talloc_free(c);
                                        return NULL;
@@ -2325,8 +2290,6 @@ static unlang_t *compile_children(unlang_group_t *g, unlang_compile_t *unlang_ct
                                goto add_child;
                        }
 
-                       edit = NULL; /* no longer doing implicit merging of edits */
-
                        /*
                         *      Variable definition.
                         */
@@ -2493,10 +2456,6 @@ static unlang_t *compile_section(unlang_t *parent, unlang_compile_t *unlang_ctx,
 
 static unlang_t *compile_group(unlang_t *parent, unlang_compile_t *unlang_ctx, CONF_SECTION *cs)
 {
-       CONF_ITEM *ci = NULL;
-       bool all_edits = true;
-       unlang_compile_t unlang_ctx2;
-
        static unlang_ext_t const group = {
                .type = UNLANG_TYPE_GROUP,
                .len = sizeof(unlang_group_t),
@@ -2505,44 +2464,6 @@ static unlang_t *compile_group(unlang_t *parent, unlang_compile_t *unlang_ctx, C
 
        if (!cf_item_next(cs, NULL)) return UNLANG_IGNORE;
 
-       if (!unlang_ctx->all_edits) {
-               while ((ci = cf_item_next(cs, ci)) != NULL) {
-                       char const *name;
-
-                       if (cf_item_is_section(ci)) {
-                               CONF_SECTION *subcs;
-
-                               subcs = cf_item_to_section(ci);
-                               name = cf_section_name1(subcs);
-
-                       } else if (cf_item_is_pair(ci)) {
-                               CONF_PAIR *cp;
-
-                               cp = cf_item_to_pair(ci);
-                               name = cf_pair_attr(cp);
-
-                       } else {
-                               continue;
-                       }
-
-                       if (*name != '&') {
-                               all_edits = false;
-                               break;
-                       }
-               }
-
-               /*
-                *      The parent wasn't all edits, but we are.  Set
-                *      the "all edits" flag, and recurse.
-                */
-               if (all_edits) {
-                       compile_copy_context(&unlang_ctx2, unlang_ctx, unlang_ctx->component);
-                       unlang_ctx2.all_edits = true;
-
-                       return compile_section(parent, &unlang_ctx2, cs, &group);
-               }
-       }
-
        return compile_section(parent, unlang_ctx, cs, &group);
 }
 
@@ -2564,10 +2485,8 @@ static fr_table_num_sorted_t transaction_keywords[] = {
 static int transaction_keywords_len = NUM_ELEMENTS(transaction_keywords);
 
 /** Limit the operations which can appear in a transaction.
- *
- *  We probably want this to be
  */
-static bool transaction_ok(CONF_SECTION *cs, bool *all_edits)
+static bool transaction_ok(CONF_SECTION *cs)
 {
        CONF_ITEM *ci = NULL;
 
@@ -2589,9 +2508,7 @@ static bool transaction_ok(CONF_SECTION *cs, bool *all_edits)
 
                        if (fr_table_value_by_str(transaction_keywords, name, -1) < 0) goto fail;
 
-                       *all_edits = false;
-
-                       if (!transaction_ok(subcs, all_edits)) return false;
+                       if (!transaction_ok(subcs)) return false;
 
                        continue;
 
@@ -2607,7 +2524,6 @@ static bool transaction_ok(CONF_SECTION *cs, bool *all_edits)
                         *      Allow rcodes via the "always" module.
                         */
                        if (fr_table_value_by_str(mod_rcode_table, name, -1) >= 0) {
-                               *all_edits = false;
                                continue;
                        }
 
@@ -2631,7 +2547,6 @@ static unlang_t *compile_transaction(unlang_t *parent, unlang_compile_t *unlang_
 {
        unlang_group_t *g;
        unlang_t *c;
-       bool all_edits = true;
        unlang_compile_t unlang_ctx2;
 
        static unlang_ext_t const transaction = {
@@ -2644,8 +2559,7 @@ static unlang_t *compile_transaction(unlang_t *parent, unlang_compile_t *unlang_
         *      The transaction is empty, ignore it.
         */
        if (!cf_item_next(cs, NULL)) return UNLANG_IGNORE;
-
-       if (!transaction_ok(cs, &all_edits)) return NULL;
+       if (!transaction_ok(cs)) return NULL;
 
        /*
         *      Any failure is return, not continue.
@@ -2657,8 +2571,6 @@ static unlang_t *compile_transaction(unlang_t *parent, unlang_compile_t *unlang_
        unlang_ctx2.actions.actions[RLM_MODULE_INVALID] = MOD_ACTION_RETURN;
        unlang_ctx2.actions.actions[RLM_MODULE_DISALLOW] = MOD_ACTION_RETURN;
 
-       unlang_ctx2.all_edits = all_edits;
-
        /*
         *      We always create a group, even if the section is empty.
         */

diff --git a/src/lib/unlang/edit.c b/src/lib/unlang/edit.c
index a296cffd0c933f8ec933b0789d745d5ee82d931c..72fe938c298fac503a7d26c33ec873e3b034fd27 100644 (file)
--- a/src/lib/unlang/edit.c
+++ b/src/lib/unlang/edit.c
@@ -1546,7 +1546,7 @@ static unlang_action_t process_edit(rlm_rcode_t *p_result, request_t *request, u
                                if (state->ours) fr_edit_list_abort(state->el);
                                TALLOC_FREE(frame->state);
                                repeatable_clear(frame);
-                               *p_result = RLM_MODULE_NOOP;
+                               *p_result = RLM_MODULE_FAIL;
 
                                return UNLANG_ACTION_CALCULATE_RESULT;
                        }

diff --git a/src/tests/keywords/edit-group b/src/tests/keywords/edit-group
index a5b93f048dc13daf83b014d6b833a0711fa90f29..37f1d2c4bd1b2fa19fddbf5a071abe483e956ff7 100644 (file)
--- a/src/tests/keywords/edit-group
+++ b/src/tests/keywords/edit-group
@@ -6,7 +6,7 @@
 #  ALl of these edits are grouped.  So if one of them fails, all of
 #  them are rolled back.
 #
-group {
+transaction {
        &Tmp-String-0 := "foo"
        &Tmp-String-1 -= "bar" # fails, no existing Tmp-String-1
        &Tmp-String-2 := "bar"
@@ -27,7 +27,7 @@ if (&Tmp-String-2) {
 #
 #  All of these succeed individually, so all of them should succeed.
 #
-group {
+transaction {
        &Tmp-String-0 := "foo"
        &Tmp-String-1 := "yup"
        &Tmp-String-2 := "bar"

diff --git a/src/tests/keywords/ethernet b/src/tests/keywords/ethernet
index c09bbc716d6de585b57c7dcc97f9ed06089921fd..296a5a44f0c1e2ecb094c9b43785fd0d2374f684 100644 (file)
--- a/src/tests/keywords/ethernet
+++ b/src/tests/keywords/ethernet
@@ -22,8 +22,10 @@ if (!(&Tmp-Ethernet-0[2] == 00:11:22:33:44:56)) {
 
 # invalid assignment
 # this will silently fail with a no Module-Failure-Message.
-&request += {
-       &Tmp-Ethernet-0 = %{Tmp-Ethernet-1[42]}
+transaction {
+       &request += {
+               &Tmp-Ethernet-0 = %{Tmp-Ethernet-1[42]}
+       }
 }
 
 if (!(%{request.Tmp-Ethernet-0[#]} == 3)) {

diff --git a/src/tests/keywords/immutable b/src/tests/keywords/immutable
index aa2694131964d0056dbc9e1790cd1af98f0e4c36..82a5c522fa21104319d9dae418ace3cc33fa72cb 100644 (file)
--- a/src/tests/keywords/immutable
+++ b/src/tests/keywords/immutable
@@ -24,9 +24,9 @@ if !(&NAS-Port == 1813) {
 #
 #  Try to edit it.  The value shouldn't change.
 #
-#  @todo - we don't have run-time exceptions... maybe this should return "fail"?
-#
-&NAS-Port += 1
+transaction {
+       &NAS-Port += 1
+}
 if !(&NAS-Port == 1813) {
        test_fail
 }

diff --git a/src/tests/keywords/policy.conf b/src/tests/keywords/policy.conf
index 4a6c9c844d22d5c29c17708113bbf68af17116cd..84516c140a545c5fec39e19c91ddf5fcc07a2fd7 100644 (file)
--- a/src/tests/keywords/policy.conf
+++ b/src/tests/keywords/policy.conf
@@ -47,7 +47,9 @@ with.dots {
 #  Set the test to successful, but only if there are no failures.
 #
 success {
-       &parent.reply.Result-Status = "success"
+#      if (&parent.reply) {
+               &parent.reply.Result-Status = "success"
+#      }
        &reply.Result-Status = "success"
 
        ok