From: Alan T. DeKok Date: Tue, 14 Oct 2025 13:17:12 +0000 (+0200) Subject: update transaction functionality so it's useful X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3bd36a5a72cde0a0da4b9b1d496a20d3f2f90ab6;p=thirdparty%2Ffreeradius-server.git update transaction functionality so it's useful edits don't fail. So we update the documentation to match. grouped edits are atomic only if they're in a transaction. So we update the documentation to match. But since edit statements never fail, the transaction keyword will never detect that. So we allow if/else/elsif statements inside of a transaction. And update the documentation to match. The example documentation is also update to note that you have to explicitly check that the assignment failed --- diff --git a/doc/antora/modules/reference/pages/unlang/edit.adoc b/doc/antora/modules/reference/pages/unlang/edit.adoc index c47c199da3..471ed4443d 100644 --- a/doc/antora/modules/reference/pages/unlang/edit.adoc +++ b/doc/antora/modules/reference/pages/unlang/edit.adoc @@ -60,16 +60,14 @@ You _cannot_ convert an `update` section to the new syntax simply by removing the `update` keyword. ==== -== Atomic "Transactions" and Errors +== Assignment Failures -The edit process is atomic, in that either all of the attributes are -modified, or none of them are modified. If the edit fails for any -reason, then all of the results are discarded, and the edit does not -affect any attributes. +If the right side of an edit fails, then the assignment does nothing, +and the variable named on the left hand side is not updated. -Note also that the server tracks overflows, underflows, and division -by zero. These issues are caught, and cause the problematic -calculation to fail. +For mathematical expressions, the server tracks overflows, underflows, +and division by zero. These issues are caught, and cause the +problematic calculation to fail. For example, if an attribute is of type `uint8`, then it can only contain 8-bit integers. Any attempt to overflow the value to more @@ -86,11 +84,12 @@ and do not result in any changes. === Grouping Edits -Multiple attributes may be grouped into a set by using the xref:unlang/transaction.adoc[transaction] -keyword. When changes are done in a `transaction`, then either all of the -changes are applied, or none of them are applied. This functionality -is best used to conditionally apply attribute changes, generally when -retrieving data from a database. +Multiple attributes may be grouped into a set by using the +xref:unlang/transaction.adoc[transaction] keyword. When changes are +done in a `transaction`, then either all of the changes are applied, +or none of them are applied. This functionality is best used to +conditionally apply attribute changes, generally when retrieving data +from a database. == List Editing diff --git a/doc/antora/modules/reference/pages/unlang/transaction.adoc b/doc/antora/modules/reference/pages/unlang/transaction.adoc index 4cee695b8f..bd3ae5e176 100644 --- a/doc/antora/modules/reference/pages/unlang/transaction.adoc +++ b/doc/antora/modules/reference/pages/unlang/transaction.adoc @@ -15,37 +15,53 @@ been made will be reverted. [ statements ]:: The `unlang` commands which will be executed. -== Caveats +A `transaction` section should only contain attribute editing which is +done via the xref:unlang/edit.adoc[edit] statements, or conditions +(xref:unlang/edit.adoc[if] / xref:unlang/edit.adoc[else] / +xref:unlang/edit.adoc[elsif]). It can also contain +xref:unlang/return_codes.adoc[rcodes] such as `fail` or `ok`. -The `transaction` sets its own action defaults for return codes -`fail`, `invalid`, and `disallow`. The priority for those return -codes is set to `1`, instead of the default `return`. This behavior -allows for the caller to edit attributes, but continue processing if -something goes wrong. +== Caveats -The main limitation of `transaction` is that it can only revert -attribute editing which is done via the xref:unlang/edit.adoc[edit] -statements. If a module performs attribute editing (e.g. `sql`, -`files`, etc.), then those edits are not reverted. +An Unlang `transaction` cannot undo operations on external databases. +For example, any data which was inserted into `sql` during a +`transaction` statement will remain in `sql` even if the Unlang +`transaction` fails. This is because Unlang transactions are +independent of any database transactions. -Similarly, a `transaction` cannot undo operations on external -databases. For example, any data which was inserted into `sql` during -a `transaction` statement will remain in `sql`. +The `transaction` keyword sets its own return codes for `fail`, +`invalid`, and `disallow` to be set the the priority `1`, instead of +the default `return`. This behavior means that a failed `transaction` +will cause the interpreter to proceed to the next instruction, instead +of returning. .Example In this example, if the SQL `select` statement fails, then the `reply.Framed-IP-Address` attribute is _not_ updated, and the -`transaction` section returns `fail`. +statements inside of the `transaction` section will return `fail`. +The assignment to `reply.Framed-IP-Address` will then be reverted. [source,unlang] ---- transaction { - reply.Filter-Id := %sql("SELECT ...") + string tmp + tmp := %sql("SELECT ...") reply.Framed-IP-Address := 192.0.2.1 + + if !tmp { + fail + } + + reply.Filter-Id := tmp } ---- +Note that xref:unlang/edit.adoc[edit] statements do not fail if the +right hand side expression fails. Instead, you must manually check if +the assignment failed. The example above does this with an +intermediate variable. + The last entry in a `transaction` section can also be an xref:unlang/actions.adoc[actions] subsection. If set, those actions over-ride any previously set defaults. @@ -57,7 +73,7 @@ xref:unlang/edit.adoc[edit] instructions. When edit instructions are grouped, then the edits are done in an atomic transaction. That is, either all of the edits succeed, or none of them do. -For now, the only purpose of `transaction` is to group edits. None of +For now, the main purpose of `transaction` is to group edits. None of the modules support transactions. If a module is used inside of a transaction, the server will return an error, and will not start. diff --git a/src/lib/unlang/transaction.c b/src/lib/unlang/transaction.c index 20265fdd49..92710f9088 100644 --- a/src/lib/unlang/transaction.c +++ b/src/lib/unlang/transaction.c @@ -166,12 +166,15 @@ static bool transaction_ok(CONF_SECTION *cs) subcs = cf_item_to_section(ci); name = cf_section_name1(subcs); - if (strcmp(name, "actions") == 0) continue; - /* - * Definitely an attribute editing thing. + * Allow limited keywords. */ - if (*name == '&') continue; + if ((strcmp(name, "actions") == 0) || + (strcmp(name, "if") == 0) || + (strcmp(name, "else") == 0) || + (strcmp(name, "elsif") == 0)) { + continue; + } if (fr_list_assignment_op[cf_section_name2_quote(cs)]) continue; @@ -195,8 +198,6 @@ static bool transaction_ok(CONF_SECTION *cs) */ if (cf_pair_value(cp)) continue; - if (*name == '&') continue; - /* * Allow rcodes via the "always" module. */ diff --git a/src/tests/keywords/transaction-if b/src/tests/keywords/transaction-if new file mode 100644 index 0000000000..608b4804f0 --- /dev/null +++ b/src/tests/keywords/transaction-if @@ -0,0 +1,35 @@ +# +# PRE: transaction +# +string foo +string bar + +transaction { + # + # Bad argument to base64 decode, so it fails and doesn't return anything. + # + foo := %base64.decode('$$$') + bar := "nope" + + if !foo { + fail + } +} + +# +# This should be rolled back. +# +if bar { + test_fail +} + +# +# This should get rolled back, too! +# +if foo { + test_fail +} else { + ok # force auth success for the test framework +} + +success diff --git a/src/tests/keywords/xlat-alternation-with-func b/src/tests/keywords/xlat-alternation-with-func index fc56ffc4b9..a2a31a16c7 100644 --- a/src/tests/keywords/xlat-alternation-with-func +++ b/src/tests/keywords/xlat-alternation-with-func @@ -8,8 +8,7 @@ string dummy_string test_string1 := "foo" test_string2 := "bar" - -if (!(%{%test.passthrough(%{dummy_string}) || %{test_string2}} == 'bar')) { +if (!(%{%test.passthrough(dummy_string) || test_string2} == 'bar')) { test_fail }