Merge pull request #7658 from ckan/config-declaration-required-adds-v…

…aldiator

`required` flag adds `not_empty` validator to option

changes/7658.feature

@@ -0,0 +1 @@
+Automatically add the ``not_empty`` validator to any config option declared with ``required: true``

ckan/cli/__init__.py

@@ -7,7 +7,7 @@
 import click
 import logging
 from logging.config import fileConfig as loggingFileConfig
-from configparser import ConfigParser, RawConfigParser
+from configparser import ConfigParser, RawConfigParser, NoOptionError
 
 from ckan.exceptions import CkanConfigurationException
 from ckan.types import Config
@@ -74,7 +74,11 @@ def _unwrap_config_chain(self, filename: str) -> list[str]:
 
             parser.read(filename)
             chain.append(filename)
-            use = parser.get(self.section, "use")
+            try:
+                use = parser.get(self.section, "use")
+            except NoOptionError:
+                return chain
+
             if not use:
                 return chain
             try:

ckan/cli/config.py

@@ -68,7 +68,7 @@ def describe(plugins: tuple[str, ...], core: bool, enabled: bool, fmt: str):
     "-m",
     "--minimal",
     is_flag=True,
-    help="Print only mandatory options",
+    help="Print only options with the `required` flag enabled",
 )
 def declaration(
     plugins: tuple[str, ...],

ckan/config/config_declaration.yaml

@@ -155,8 +155,6 @@ groups:
     options:
       - key: use
         placeholder: egg:ckan
-        validators: not_empty
-        required: true
 
       - key: SECRET_KEY
         validators: not_empty
@@ -348,7 +346,6 @@ groups:
 
       - key: beaker.session.samesite
         default: Lax
-        required: True
         validators: OneOf(["Strict","Lax","None"])
         description: |
           Whether or not the session cookie should be marked as SameSite. When marked as SameSite,
@@ -359,7 +356,6 @@ groups:
     options:
       - key: sqlalchemy.url
         placeholder: postgresql://ckan_default:pass@localhost/ckan_default
-        validators: not_empty
         required: true
         example: postgres://tester:pass@localhost/ckantest3
         description: |
@@ -390,7 +386,6 @@ groups:
   - annotation: Site Settings
     options:
       - key: ckan.site_url
-        validators: not_empty
         required: true
         placeholder: http://localhost:5000
         example: http://scotdata.ckan.net
@@ -858,7 +853,6 @@ groups:
 
       - key: solr_url
         placeholder: http://127.0.0.1:8983/solr/ckan
-        validators: not_empty
         required: true
         example: http://solr.okfn.org:8983/solr/ckan-schema-2.0
         description: |
@@ -1017,7 +1011,6 @@ groups:
   - annotation: Plugins Settings
     options:
       - key: ckan.plugins
-        required: true
         type: list
         placeholder: activity
         example: activity scheming_datasets datatables_view datastore xloader

ckan/config/declaration/option.py

@@ -12,6 +12,7 @@
 from ckan.types import Validator, ValidatorFactory
 
 T = TypeVar("T")
+_sentinel = object()
 
 
 class SectionMixin:
@@ -43,7 +44,8 @@ class Flag(enum.Flag):
 
     required: this option cannot be missing/empty. Add such flag to the option
     only if CKAN application won't even start without them and there is no
-    sensible default.
+    sensible default. If option does not have ``not_empty`` validator, it will
+    be added before all other validators.
 
     editable: this option is runtime editable. Technically, every option can be
     modified. This flag means that there is an expectation that option will be
@@ -201,13 +203,16 @@ def __init__(self, default: Optional[T] = None):
         self.default = default
         self.legacy_key = None
 
-    def str_value(self) -> str:
-        """Convert option's default value into the string.
+    def str_value(self, value: T | object = _sentinel) -> str:
+        """Convert value into the string using option's settings.
 
-        If the option has `as_list` validator and default value is represented
-        by the Python's `list` object, result is a space-separated list of all
-        the members of the value. In other cases this method just does naive
-        string conversion.
+        If `value` argument is not present, convert the default value of the
+        option into a string.
+
+        If the option has `as_list` validator and the value is represented by
+        the Python's `list` object, result is a space-separated list of all the
+        members of the value. In other cases this method just does naive string
+        conversion.
 
         If validators are doing complex transformations, for example string ID
         turns into :py:class:`~ckan.model.User` object, this method won't
@@ -224,9 +229,15 @@ def str_value(self) -> str:
 
         """
         as_list = "as_list" in self.get_validators()
-        if isinstance(self.default, list) and as_list:
-            return " ".join(map(str, self.default))
-        return str(self.default) if self.has_default() else ""
+        v = self.default if value is _sentinel else value
+
+        if isinstance(v, list) and as_list:
+            return " ".join(map(str, v))
+
+        if self.has_default() or value is not _sentinel:
+            return str(v)
+
+        return ""
 
     def set_flag(self, flag: Flag) -> Self:
         """Enable specified flag.
@@ -319,7 +330,11 @@ def append_validators(self, validators: str, before: bool = False) -> Self:
     def get_validators(self) -> str:
         """Return the string with current validators.
         """
-        return self.validators
+        validators = self.validators
+        if self.has_flag(Flag.required) and "not_empty" not in validators:
+            validators = f"not_empty {validators}"
+
+        return validators
 
     def experimental(self) -> Self:
         """Enable experimental-flag for value.

ckan/plugins/core.py

@@ -97,10 +97,8 @@ def __iter__(self) -> Iterator[TInterface]:
 
         plugin_lookup = {pf.name: pf for pf in iterator}
 
-        plugins = config.get("ckan.plugins")
-        if plugins is None:
-            plugins = []
-        elif isinstance(plugins, str):
+        plugins = config.get("ckan.plugins", [])
+        if isinstance(plugins, str):
             # this happens when core declarations loaded and validated
             plugins = plugins.split()
 

ckan/tests/cli/test_user.py

@@ -89,7 +89,7 @@ def test_revoke(self, cli):
 
         # tid must be prefixed by --. When it starts with a hyphen it treated
         # as a flag otherwise.
-        args = f'user token revoke -- "{tid}"'
+        args = ["user", "token", "revoke", "--", tid]
         result = cli.invoke(ckan, args)
         assert not result.exit_code, result.output
         assert u"API Token has been revoked" in result.output

ckan/tests/config/declaration/test_declaration.py

@@ -64,6 +64,17 @@ def test_pattern_and_flag_iteration(self):
             key.hey,
         ]
 
+    def test_required_option(self):
+        decl = Declaration()
+        key = "test.required.flag.adds.not_empty"
+        option = decl.declare(key)
+        _, errors = decl.validate({})
+        assert not errors
+
+        option.set_flag(Flag.required)
+        _, errors = decl.validate({})
+        assert key in errors
+
     def test_setup(self, ckan_config):
         decl = Declaration()
         decl.setup()

ckan/tests/config/declaration/test_option.py

@@ -39,3 +39,19 @@ def test_normalize(self):
 
         option.set_validators("default([[],{():None}])")
         assert option.normalize(option.default) == [[], {(): None}]
+
+    def test_str_value(self):
+        option = Option()
+        assert option.str_value() == ""
+        assert option.str_value(1) == "1"
+        assert option.str_value([1, 2]) == "[1, 2]"
+
+        option = Option().set_validators("as_list")
+        assert option.str_value() == ""
+        assert option.str_value(1) == "1"
+        assert option.str_value([1, 2]) == "1 2"
+
+        option = Option([10, 20]).set_validators("as_list")
+        assert option.str_value() == "10 20"
+        assert option.str_value(1) == "1"
+        assert option.str_value([1, 2]) == "1 2"

doc/maintaining/configuration.rst

@@ -183,8 +183,9 @@ only for explanation and you don't need them in the real file::
           # a space-separated list of validators, applied to the value of option.
           validators: not_missing boolean_validator
 
-          # shortcut for the most common option types. It adds validators, so you cannot use it
-          # when `validators` are specified(in this case `type` is silently ignored).
+          # shortcut for the most common option types. It adds type validators to the option.
+          # If both, `type` and `validators` are set, validators from `type` are added first,
+          # then validators from `validators` are appended.
           # Valid types are: bool, int, list, dynamic (see below for more information on dynamic
           # options)
           type: bool