Repository: sanic-org/sanic
Pull request #2330 (2021-12-02 22:23:49)

Env custom type casting
Inspired by the pattern from @Varriount in #2321 and `app.router.register_pattern`, this PR adds the ability to register custom converters to cast values loaded from environment variables into a custom type.

Example:

```python
class Foo:
    def __init__(self, name) -> None:
        self.name = name


config = Config(converters=[Foo])
app = Sanic(__name__, config=config)

@app.before_server_start
async def display(app, _):
    assert isinstance(app.config.FOO, Foo)
```

prryplatypus: Was just thinking about this... I think it would be a good idea to pass a "key" parameter to the converter, to make it possible to convert only a specific environment variable without converting others that would also be possible to convert.
ahopkins: > Was just thinking about this... I think it would be a good idea to pass a "key" parameter to the converter, to make it possible to convert only a specific environment variable without converting others that would also be possible to convert.

Can you provide a snippet of what that would look like as a dev? At that point, why not just convert yourself?
prryplatypus: Sure! That's what I thought this PR was meant to avoid, so this came to mind.

In one of my codebases I currently have the following method, which is called every time the application is started. It's only use is to coerce values.
```py
## config.py
from base64 import b64decode
from sanic import Sanic

def init(app: Sanic) -> None:
    """This is simply used to coerce the config values
    into the correct type where necessary. Use environment
    variables to set config values.
    """
    config = app.config
    if isinstance(config.JWT_SECRET, str):
        config.JWT_SECRET = b64decode(config.JWT_SECRET)
```

After thinking about it, I realised that passing the key may not be possible as there'd be no way to indicate "no, I don't want to coerce this", but the following implementation came to mind to replace the example above, where the value can be any callable:
```py
config = Config(coercers={"JWT_SECRET": b64decode})
app = Sanic(__name__, config=config)

@app.before_server_start
async def display(app, _):
    assert isinstance(app.config.JWT_SECRET, bytes)
```
ahopkins: Hmm.... :thinking:  two thoughts:

1. I would think another name other than `coercer`. It might be confusing to have converter & coercer. Will have to think on that.
2. It probably should be reversed, with the type being the key so it could be used on multiple:

```python
Config(something={b64decode: "JWT_SECRET", foobar: ["FOO", "BAR"]})
```

Thinking more, maybe the answer is to just combine both concepts.

```python
class Foo:
    def __init__(self, name) -> None:
        self.name = name


config = Config(converters=[
    Foo,
    (b64decode, "JWT_SECRET"),
    (foobar, "FOO", "BAR"),
])

# OR

config = Config(converters=[
    Foo,
    (b64decode, "JWT_SECRET"),
    (foobar, ("FOO", "BAR")),
])

# OR

config = Config(converters={
    Foo: None,
    b64decode: "JWT_SECRET",
    foobar: ("FOO", "BAR"),
})
```

I think the third option may be the winner. Pass a dict (maybe we also allow a simple list like in the original if NONE of them have key constraints) where:

- if value is `None`, then apply to all keys and see what matches (current implementation)
- if value is string, then apply to single key
- if value is list, then apply to those keys only
prryplatypus: I did think about the tuple option, but then the question about what to do if a key was defined in more than one place came to mind, so figured it could be avoided all together by just making it a mapping of Key-Coercer 🤷 . Aaaand we could just make a `None: Foo` key to match all?
ahopkins: > What about if a key appears in more than one place though?

I think it executes in the order they are listed. If there are multiple things that run on it, they would each run in turn.


Varriount: Looking at the code, it appears that converters are (more or less) global - `Config.register_type`, despite being an instance method, modifies an instance of "CastRegistry" that is stored in a class attribute. I feel that this is very misleading, as I do not normally expect instance methods to modify class-level or global data. It would probably be better to just have a `registered_converters` instance attribute. 
ahopkins: > it would probably be better to just have a `registered_converters` instance attribute

That makes total sense. Nice call out.
Varriount: (thinking on it, the attribute would probably be better named as just `converters` or some such)

Some other thoughts:
  - If the logic is changed to use an instance attribute, a decision needs to be made on how users are going to manipulate that attribute. It's tempting to just expose it directly, since this would give users the most control (for example, by inserting a converter at a specific point), however it may pose a problem for any future changes.
  - I think a decision may eventually need to be made on how "complex" the built-in configuration logic is going to be. To be clear, I _do_ think that this PR is a good addition, as it lets users load types like dates, json, etc. from the environment. Configuration loading, however, is one of those mechanisms that can become quite extensive, and as such it's important to make good architecture decisions early on.
  - One of the most flexible configuration systems I've ever used was the one supported by [Spring Boot](https://docs.spring.io/spring-boot/docs/2.1.13.RELEASE/reference/html/boot-features-external-config.html), though it was also frustrating too - primarily because it was difficult to tell where it was looking for data, and where certain configuration values had originated.
ahopkins: > * however it may pose a problem for any future changes.

It really needs to be done upfront since environment variables are loaded when the application instance is instantiated. After that, there really is not so much use to them.

This could be a possibility, but it is verbose.

```python
config = Config()
config.converters = [...]
```

> * it lets users load types like dates, json, etc

`datetime` and `UUID` are the two I have in mind most. Originally I was thinking these should just be added, but that would be a breaking change for sure. For someone that wants this, it seems easy enough to do this:

```python
app = Sanic(..., config=Config(converters=[datetime, UUID]))
```

That was what I figured the most common use case would be.

> * Configuration loading, however, is one of those mechanisms that can become quite extensive, and as such it's important to make good architecture decisions early on.

:100: 

> * was the one supported by Spring Boot

Now I need to go dust off my skills and clear the cobwebs from my brain. Its been a while. Will need to go see how they do it. Something in particular that sticks out for you?

Patch:
diff --git a/sanic/config.py b/sanic/config.py
index 6994e605..d4057a71 100644
--- a/sanic/config.py
+++ b/sanic/config.py
@@ -3,7 +3,7 @@ from __future__ import annotations
 from inspect import getmembers, isclass, isdatadescriptor
 from os import environ
 from pathlib import Path
-from typing import Any, Dict, Optional, Union
+from typing import Any, Callable, Dict, Optional, Sequence, Union
 from warnings import warn
 
 from sanic.errorpages import DEFAULT_FORMAT, check_error_format
@@ -43,6 +43,10 @@ DEFAULT_CONFIG = {
     "WEBSOCKET_PING_TIMEOUT": 20,
 }
 
+# These values will be removed from the Config object in v22.6 and moved
+# to the application state
+DEPRECATED_CONFIG = ("SERVER_RUNNING", "RELOADER_PROCESS", "RELOADED_FILES")
+
 
 class DescriptorMeta(type):
     def __init__(cls, *_):
@@ -85,12 +89,19 @@ class Config(dict, metaclass=DescriptorMeta):
         load_env: Optional[Union[bool, str]] = True,
         env_prefix: Optional[str] = SANIC_PREFIX,
         keep_alive: Optional[bool] = None,
+        *,
+        converters: Optional[Sequence[Callable[[str], Any]]] = None,
     ):
         defaults = defaults or {}
         super().__init__({**DEFAULT_CONFIG, **defaults})
 
+        self._converters = [str, str_to_bool, float, int]
         self._LOGO = ""
 
+        if converters:
+            for converter in converters:
+                self.register_type(converter)
+
         if keep_alive is not None:
             self.KEEP_ALIVE = keep_alive
 
@@ -199,7 +210,23 @@ class Config(dict, metaclass=DescriptorMeta):
         - ``float``
         - ``bool``
 
-        Anything else will be imported as a ``str``.
+        Anything else will be imported as a ``str``. If you would like to add
+        additional types to this list, you can use
+        :meth:`sanic.config.Config.register_type`. Just make sure that they
+        are registered before you instantiate your application.
+
+        .. code-block:: python
+
+            class Foo:
+                def __init__(self, name) -> None:
+                    self.name = name
+
+
+            config = Config(converters=[Foo])
+            app = Sanic(__name__, config=config)
+
+        `See user guide re: config
+        <https://sanicframework.org/guide/deployment/configuration.html>`__
         """
         for key, value in environ.items():
             if not key.startswith(prefix):
@@ -207,7 +234,7 @@ class Config(dict, metaclass=DescriptorMeta):
 
             _, config_key = key.split(prefix, 1)
 
-            for converter in (int, float, str_to_bool, str):
+            for converter in reversed(self._converters):
                 try:
                     self[config_key] = converter(value)
                     break
@@ -282,3 +309,17 @@ class Config(dict, metaclass=DescriptorMeta):
         self.update(config)
 
     load = update_config
+
+    def register_type(self, converter: Callable[[str], Any]) -> None:
+        """
+        Allows for adding custom function to cast from a string value to any
+        other type. The function should raise ValueError if it is not the
+        correct type.
+        """
+        if converter in self._converters:
+            error_logger.warning(
+                f"Configuration value converter '{converter.__name__}' has "
+                "already been registered"
+            )
+            return
+        self._converters.append(converter)


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index 67324f1e..7c5bab05 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -1,3 +1,5 @@
+import logging
+
 from contextlib import contextmanager
 from os import environ
 from pathlib import Path
@@ -32,6 +34,11 @@ class ConfigTest:
         return self.not_for_config
 
 
+class UltimateAnswer:
+    def __init__(self, answer):
+        self.answer = int(answer)
+
+
 def test_load_from_object(app):
     app.config.load(ConfigTest)
     assert "CONFIG_VALUE" in app.config
@@ -137,6 +144,32 @@ def test_env_prefix_string_value():
     del environ["MYAPP_TEST_TOKEN"]
 
 
+def test_env_w_custom_converter():
+    environ["SANIC_TEST_ANSWER"] = "42"
+
+    config = Config(converters=[UltimateAnswer])
+    app = Sanic(name=__name__, config=config)
+    assert isinstance(app.config.TEST_ANSWER, UltimateAnswer)
+    assert app.config.TEST_ANSWER.answer == 42
+    del environ["SANIC_TEST_ANSWER"]
+
+
+def test_add_converter_multiple_times(caplog):
+    def converter():
+        ...
+
+    message = (
+        "Configuration value converter 'converter' has already been registered"
+    )
+    config = Config()
+    config.register_type(converter)
+    with caplog.at_level(logging.WARNING):
+        config.register_type(converter)
+
+    assert ("sanic.error", logging.WARNING, message) in caplog.record_tuples
+    assert len(config._converters) == 5
+
+
 def test_load_from_file(app):
     config = dedent(
         """