[bug#62298] [PATCH v2 1/8] services: configuration: Add user-defined sanitizer support.

[Maxim Cournoyer]

Hello!

Bruno Victal <mirai@makinata.eu> writes:

> This changes the 'custom-serializer' field into a generic
> 'extra-args' field that can be extended to support new literals.
> With this mechanism, the literals 'sanitizer' allow for user-defined
> sanitizer procedures while the 'serializer' literal is used for
> custom serializer procedures.
> The 'empty-serializer' was also added as a 'literal' and can be used
> just like it was previously.
>
> With the repurposing of 'custom-serializer' into 'extra-args', to prevent
> intolerable confusion, the custom serializer procedures should be
> specified using the new 'literals' approach, with the previous “style”
> being considered deprecated.
>
> * gnu/services/configuration.scm (define-configuration-helper):
> Rename 'custom-serializer' to 'extra-args'.
> Add support for literals 'sanitizer', 'serializer' and 'empty-serializer'.
> Rename procedure 'field-sanitizer' to 'default-field-sanitizer' to avoid 
> syntax clash.
> Only define default field sanitizers if user-defined ones are absent.
> (normalize-extra-args): New procedure.
> (<configuration-field>)[sanitizer]: New field.
>
> * doc/guix.texi (Complex Configurations): Document the newly added literals.
>
> * tests/services/configuration.scm: Add tests for the new literals.

Interesting work!

> ---
>  doc/guix.texi                    |  30 ++++-
>  gnu/services/configuration.scm   |  91 +++++++++++----
>  tests/services/configuration.scm | 185 ++++++++++++++++++++++++++++++-
>  3 files changed, 280 insertions(+), 26 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index dfdb26103a..1609e508ef 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -41216,7 +41216,7 @@ Complex Configurations
>  (@var{field-name}
>   (@var{type} @var{default-value})
>   @var{documentation}
> - @var{serializer})
> + (serializer @var{serializer}))
>  
>  (@var{field-name}
>   (@var{type})
> @@ -41225,7 +41225,18 @@ Complex Configurations
>  (@var{field-name}
>   (@var{type})
>   @var{documentation}
> - @var{serializer})
> + (serializer @var{serializer}))
> +
> +(@var{field-name}
> + (@var{type})
> + @var{documentation}
> + (sanitizer @var{sanitizer})
> +
> +(@var{field-name}
> + (@var{type})
> + @var{documentation}
> + (sanitizer @var{sanitizer})
> + (serializer @var{serializer}))
>  @end example
>  
>  @var{field-name} is an identifier that denotes the name of the field in
> @@ -41248,6 +41259,21 @@ Complex Configurations
>  @var{documentation} is a string formatted with Texinfo syntax which
>  should provide a description of what setting this field does.
>  
> +@var{sanitizer} is the name of a procedure which takes one argument,
> +a user-supplied value, and returns a ``sanitized'' value for the field.
> +If none is specified, the predicate @code{@var{type}?} is automatically
> +used to construct an internal sanitizer that asserts the type correctness
> +of the field value.
> +
> +An example of a sanitizer for a field that accepts both strings and
> +symbols looks like this:
> +@lisp
> +(define (sanitize-foo value)
> +  (cond ((string? value) value)
> +        ((symbol? value) (symbol->string value))
> +        (else (throw 'bad! value))))
> +@end lisp


I'd use the more conventional (error "bad value" value) in the example.

>  @var{serializer} is the name of a procedure which takes two arguments,
>  the first is the name of the field, and the second is the value
>  corresponding to the field.  The procedure should return a string or
> diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
> index 174c2f20d2..409d4cef00 100644
> --- a/gnu/services/configuration.scm
> +++ b/gnu/services/configuration.scm
> @@ -6,6 +6,7 @@
>  ;;; Copyright © 2021, 2022 Maxim Cournoyer <maxim.cournoyer@gmail.com>
>  ;;; Copyright © 2021 Andrew Tropin <andrew@trop.in>
>  ;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
> +;;; Copyright © 2023 Bruno Victal <mirai@makinata.eu>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -28,7 +29,8 @@ (define-module (gnu services configuration)
>    #:use-module (guix gexp)
>    #:use-module ((guix utils) #:select (source-properties->location))
>    #:use-module ((guix diagnostics)
> -                #:select (formatted-message location-file &error-location))
> +                #:select (formatted-message location-file &error-location
> +                          warning))
>    #:use-module ((guix modules) #:select (file-name->module-name))
>    #:use-module (guix i18n)
>    #:autoload   (texinfo) (texi-fragment->stexi)
> @@ -37,6 +39,7 @@ (define-module (gnu services configuration)
>    #:use-module (ice-9 format)
>    #:use-module (ice-9 match)
>    #:use-module (srfi srfi-1)
> +  #:use-module (srfi srfi-26)
>    #:use-module (srfi srfi-34)
>    #:use-module (srfi srfi-35)
>    #:export (configuration-field
> @@ -44,6 +47,7 @@ (define-module (gnu services configuration)
>              configuration-field-type
>              configuration-missing-field
>              configuration-field-error
> +            configuration-field-sanitizer
>              configuration-field-serializer
>              configuration-field-getter
>              configuration-field-default-value-thunk
> @@ -116,6 +120,7 @@ (define-record-type* <configuration-field>
>    (type configuration-field-type)
>    (getter configuration-field-getter)
>    (predicate configuration-field-predicate)
> +  (sanitizer configuration-field-sanitizer)
>    (serializer configuration-field-serializer)
>    (default-value-thunk configuration-field-default-value-thunk)
>    (documentation configuration-field-documentation))
> @@ -181,11 +186,45 @@ (define (normalize-field-type+def s)
>       (values #'(field-type %unset-value)))))
>  
>  (define (define-configuration-helper serialize? serializer-prefix syn)
> +
> +  (define (normalize-extra-args s)

A docstring would help here, explaining what this helper is for.

> +    (let loop ((s s)
> +               (sanitizer* %unset-value)
> +               (serializer* %unset-value))
> +      (syntax-case s (sanitizer serializer empty-serializer)
> +        (((sanitizer proc) tail ...)
> +         (if (maybe-value-set? sanitizer*)
> +             (syntax-violation 'sanitizer "duplicate entry"
> +                               #'proc)
> +             (loop #'(tail ...) #'proc serializer*)))
> +        (((serializer proc) tail ...)
> +         (if (maybe-value-set? serializer*)
> +             (syntax-violation 'serializer "duplicate or conflicting entry"
> +                               #'proc)
> +             (loop #'(tail ...) sanitizer* #'proc)))
> +        ((empty-serializer tail ...)
> +         (if (maybe-value-set? serializer*)
> +             (syntax-violation 'empty-serializer
> +                               "duplicate or conflicting entry" #f)
> +             (loop #'(tail ...) sanitizer* #'empty-serializer)))
> +        (()  ; stop condition
> +         (values (list sanitizer* serializer*)))
> +        ((proc)  ; TODO: deprecated, to be removed
> +         (every (cut eq? <> #f)
> +                (map maybe-value-set?
> +                     (list sanitizer* serializer*)))

This 'every' call result is not acted upon.  Was it supposed to raise a
syntax violation?  If it's useful to keep it (and act on it), I'd use
something like:

--8<---------------cut here---------------start------------->8---
(unless (null? (filter-map maybe-value-set? (list sanitizer* serializer*)))
 (syntax-violation ...))
--8<---------------cut here---------------end--------------->8---

> +         (begin

I think the 'begin' is unnecessary.

> +           (warning #f (G_ "specifying serializers after documentation is \
> +deprecated, use (serializer ~a) instead~%") (syntax->datum #'proc))

I wonder, instead of #f, would it be possible to fill in the correct
source location?  That'd be useful, and it's done elsewhere, though I'm
not too familiar with the mechanism (I think it relies on Guile's source
properties).

[...]

> diff --git a/tests/services/configuration.scm 
> b/tests/services/configuration.scm
> index 4f8a74dc8a..64b7bb1543 100644
> --- a/tests/services/configuration.scm
> +++ b/tests/services/configuration.scm

[...]

> +(test-group "empty-serializer as literal/procedure tests"
> +  ;; empty-serializer as literal

Nitpick; all stand-alone comments starting with ;; should be properly
punctuated, complete sentences.

The rest LGTM, but I'll leave it on the tracker for another week or so
to allow for others to comment since it's a close-to-core change.

-- 
Thanks,
Maxim