Repository: octodns/octodns

Issue:
The sorting algorithm can cause friction when adding new records
We manage all of our DNS records using OctoDNS, including DNS verification records. These records tend to include a random sequence of alphanumeric characters. 

E.g:
```
_b9ab05b3cefa9cb63dcd2a98846zfd46.my.domain
```

We may have many of these in our various domain configuration files.

We understand that OcotoDNS uses a sort algorithm by padding integers with 0s to maintain natural order (as described in this [issue](https://github.com/octodns/octodns/issues/43)). 

Friction is encountered when we need to add a similar record, often times requiring trial and error before it is added to the right location because the sort order is not obvious just by eyeballing the value. 

We still want to enforce ordering, but since the sort order imposed by OctoDNS is not standard, it's difficult to use existing tools (short of building our own) to validate the order. 

Do you have suggestions on how we can improve our process here?

Hints from issue comments:
@ross: > Do you have suggestions on how we can improve our process here?

As I assume you've already found, enforcing order is optional with the `enforce_order`

https://github.com/octodns/octodns/blob/6f8454ef7bb0a18b30a89b51de7e1933c2377576/octodns/provider/yaml.py#L36.

that's really the only option in the short term. 

Otherwise nothing prevents adding support for non-natural simplistic alphabetical sort order if that's what you're saying would work better, it's just really annoying in the most common case, e.g.

```yaml
---
foo-1:
   ...
foo-10:
   ...
foo-11:
   ...
foo-12:
   ...
foo-2:
   ...
foo-20:
   ...
foo-3:
   ...
...
```

I can poke around and see what that option would look like.



Solution patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1e644bf..665db90 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,8 @@
+## v1.?.? - 2024-??-?? - ???
+
+* Add YamlProvider.order_mode to allow picking between natural (human)
+  the default when enforce_order=True and simple `sort`.
+
 ## v1.10.0 - 2024-10-06 - Lots of little stuff
 
 * Zone name validation checking for double dots, and throwing InvalidNameError
diff --git a/octodns/cmds/args.py b/octodns/cmds/args.py
index 1aefbea..7e0ae7d 100644
--- a/octodns/cmds/args.py
+++ b/octodns/cmds/args.py
@@ -8,9 +8,8 @@ from logging.config import dictConfig
 from logging.handlers import SysLogHandler
 from sys import stderr, stdout
 
-from yaml import safe_load
-
 from octodns import __version__
+from octodns.yaml import safe_load
 
 
 class ArgumentParser(_Base):
@@ -68,7 +67,7 @@ class ArgumentParser(_Base):
     def _setup_logging(self, args, default_log_level):
         if args.logging_config:
             with open(args.logging_config) as fh:
-                config = safe_load(fh.read())
+                config = safe_load(fh.read(), enforce_order=False)
             dictConfig(config)
             # if we're provided a logging_config we won't do any of our normal
             # configuration
diff --git a/octodns/provider/yaml.py b/octodns/provider/yaml.py
index 3fc6102..b5fd3d7 100644
--- a/octodns/provider/yaml.py
+++ b/octodns/provider/yaml.py
@@ -34,6 +34,11 @@ class YamlProvider(BaseProvider):
         # Whether or not to enforce sorting order when loading yaml
         # (optional, default True)
         enforce_order: true
+        # What sort mode to employ when enforcing order
+        # - simple: `sort`
+        # - natural: https://pypi.org/project/natsort/
+        # (optional, default natural)
+        order_mode: natural
 
         # Whether duplicate records should replace rather than error
         # (optional, default False)
@@ -174,6 +179,7 @@ class YamlProvider(BaseProvider):
         directory,
         default_ttl=3600,
         enforce_order=True,
+        order_mode='natural',
         populate_should_replace=False,
         supports_root_ns=True,
         split_extension=False,
@@ -186,11 +192,12 @@ class YamlProvider(BaseProvider):
         klass = self.__class__.__name__
         self.log = logging.getLogger(f'{klass}[{id}]')
         self.log.debug(
-            '__init__: id=%s, directory=%s, default_ttl=%d, enforce_order=%d, populate_should_replace=%s, supports_root_ns=%s, split_extension=%s, split_catchall=%s, shared_filename=%s, disable_zonefile=%s',
+            '__init__: id=%s, directory=%s, default_ttl=%d, enforce_order=%d, order_mode=%s, populate_should_replace=%s, supports_root_ns=%s, split_extension=%s, split_catchall=%s, shared_filename=%s, disable_zonefile=%s',
             id,
             directory,
             default_ttl,
             enforce_order,
+            order_mode,
             populate_should_replace,
             supports_root_ns,
             split_extension,
@@ -202,6 +209,7 @@ class YamlProvider(BaseProvider):
         self.directory = directory
         self.default_ttl = default_ttl
         self.enforce_order = enforce_order
+        self.order_mode = order_mode
         self.populate_should_replace = populate_should_replace
         self.supports_root_ns = supports_root_ns
         self.split_extension = split_extension
@@ -304,7 +312,9 @@ class YamlProvider(BaseProvider):
 
     def _populate_from_file(self, filename, zone, lenient):
         with open(filename, 'r') as fh:
-            yaml_data = safe_load(fh, enforce_order=self.enforce_order)
+            yaml_data = safe_load(
+                fh, enforce_order=self.enforce_order, order_mode=self.order_mode
+            )
             if yaml_data:
                 for name, data in yaml_data.items():
                     if not isinstance(data, list):
@@ -420,7 +430,7 @@ class YamlProvider(BaseProvider):
 
                 with open(filename, 'w') as fh:
                     record_data = {record: config}
-                    safe_dump(record_data, fh)
+                    safe_dump(record_data, fh, order_mode=self.order_mode)
 
             if catchall:
                 # Scrub the trailing . to make filenames more sane.
@@ -429,14 +439,19 @@ class YamlProvider(BaseProvider):
                     '_apply:   writing catchall filename=%s', filename
                 )
                 with open(filename, 'w') as fh:
-                    safe_dump(catchall, fh)
+                    safe_dump(catchall, fh, order_mode=self.order_mode)
 
         else:
             # single large file
             filename = join(self.directory, f'{desired.decoded_name}yaml')
             self.log.debug('_apply:   writing filename=%s', filename)
             with open(filename, 'w') as fh:
-                safe_dump(dict(data), fh, allow_unicode=True)
+                safe_dump(
+                    dict(data),
+                    fh,
+                    allow_unicode=True,
+                    order_mode=self.order_mode,
+                )
 
 
 class SplitYamlProvider(YamlProvider):
diff --git a/octodns/yaml.py b/octodns/yaml.py
index 433486a..71e7d05 100644
--- a/octodns/yaml.py
+++ b/octodns/yaml.py
@@ -11,7 +11,9 @@ from yaml.representer import SafeRepresenter
 
 from .context import ContextDict
 
-_natsort_key = natsort_keygen()
+# as of python 3.13 functools.partial is a method descriptor and must be wrapped
+# in staticmethod() to preserve the behavior natsort is expecting it to have
+_natsort_key = staticmethod(natsort_keygen())
 
 
 class ContextLoader(SafeLoader):
@@ -44,11 +46,12 @@ ContextLoader.add_constructor(
 # Found http://stackoverflow.com/a/21912744 which guided me on how to hook in
 # here
 class SortEnforcingLoader(ContextLoader):
+
     def _construct(self, node):
         ret, pairs, context = self._pairs(node)
 
         keys = [d[0] for d in pairs]
-        keys_sorted = sorted(keys, key=_natsort_key)
+        keys_sorted = sorted(keys, key=self.KEYGEN)
         for key in keys:
             expected = keys_sorted.pop(0)
             if key != expected:
@@ -62,13 +65,51 @@ class SortEnforcingLoader(ContextLoader):
         return ret
 
 
-SortEnforcingLoader.add_constructor(
-    SortEnforcingLoader.DEFAULT_MAPPING_TAG, SortEnforcingLoader._construct
+class NaturalSortEnforcingLoader(SortEnforcingLoader):
+    KEYGEN = _natsort_key
+
+
+NaturalSortEnforcingLoader.add_constructor(
+    SortEnforcingLoader.DEFAULT_MAPPING_TAG,
+    NaturalSortEnforcingLoader._construct,
+)
+
+
+class SimpleSortEnforcingLoader(SortEnforcingLoader):
+    KEYGEN = lambda _, s: s
+
+
+SimpleSortEnforcingLoader.add_constructor(
+    SortEnforcingLoader.DEFAULT_MAPPING_TAG,
+    SimpleSortEnforcingLoader._construct,
 )
 
 
-def safe_load(stream, enforce_order=True):
-    return load(stream, SortEnforcingLoader if enforce_order else ContextLoader)
+_loaders = {
+    'natural': NaturalSortEnforcingLoader,
+    'simple': SimpleSortEnforcingLoader,
+}
+
+
+class InvalidOrder(Exception):
+
+    def __init__(self, order_mode):
+        options = '", "'.join(_loaders.keys())
+        super().__init__(
+            f'Invalid order_mode, "{order_mode}", options are "{options}"'
+        )
+
+
+def safe_load(stream, enforce_order=True, order_mode='natural'):
+    if enforce_order:
+        try:
+            loader = _loaders[order_mode]
+        except KeyError as e:
+            raise InvalidOrder(order_mode) from e
+    else:
+        loader = ContextLoader
+
+    return load(stream, loader)
 
 
 class SortingDumper(SafeDumper):
@@ -81,7 +122,7 @@ class SortingDumper(SafeDumper):
     '''
 
     def _representer(self, data):
-        data = sorted(data.items(), key=lambda d: _natsort_key(d[0]))
+        data = sorted(data.items(), key=self.KEYGEN)
         return self.represent_mapping(self.DEFAULT_MAPPING_TAG, data)
 
 
@@ -92,7 +133,18 @@ SortingDumper.add_multi_representer(str, SafeRepresenter.represent_str)
 SortingDumper.add_multi_representer(dict, SortingDumper._representer)
 
 
-def safe_dump(data, fh, **options):
+class NaturalSortingDumper(SortingDumper):
+    KEYGEN = _natsort_key
+
+
+class SimpleSortingDumper(SortingDumper):
+    KEYGEN = lambda _, s: s
+
+
+_dumpers = {'natural': NaturalSortingDumper, 'simple': SimpleSortingDumper}
+
+
+def safe_dump(data, fh, order_mode='natural', **options):
     kwargs = {
         'canonical': False,
         'indent': 2,
@@ -101,4 +153,8 @@ def safe_dump(data, fh, **options):
         'explicit_start': True,
     }
     kwargs.update(options)
-    dump(data, fh, SortingDumper, **kwargs)
+    try:
+        dumper = _dumpers[order_mode]
+    except KeyError as e:
+        raise InvalidOrder(order_mode) from e
+    dump(data, fh, dumper, **kwargs)


Test patch:
diff --git a/tests/test_octodns_yaml.py b/tests/test_octodns_yaml.py
index 396d59c..a05d28d 100644
--- a/tests/test_octodns_yaml.py
+++ b/tests/test_octodns_yaml.py
@@ -7,7 +7,7 @@ from unittest import TestCase
 
 from yaml.constructor import ConstructorError
 
-from octodns.yaml import safe_dump, safe_load
+from octodns.yaml import InvalidOrder, safe_dump, safe_load
 
 
 class TestYaml(TestCase):
@@ -86,3 +86,60 @@ class TestYaml(TestCase):
                 "[Errno 2] No such file or directory: 'tests/config/include/does-not-exist.yaml'",
                 str(ctx.exception),
             )
+
+    def test_order_mode(self):
+        data = {'*.1.2': 'a', '*.10.1': 'c', '*.11.2': 'd', '*.2.2': 'b'}
+        natural = '''---
+'*.1.2': a
+'*.2.2': b
+'*.10.1': c
+'*.11.2': d
+'''
+        simple = '''---
+'*.1.2': a
+'*.10.1': c
+'*.11.2': d
+'*.2.2': b
+'''
+
+        ## natural
+        # correct order
+        self.assertEqual(data, safe_load(natural))
+        # wrong order
+        with self.assertRaises(ConstructorError) as ctx:
+            safe_load(simple)
+        problem = ctx.exception.problem.split(' at')[0]
+        self.assertEqual(
+            'keys out of order: expected *.2.2 got *.10.1', problem
+        )
+        # dump
+        buf = StringIO()
+        safe_dump(data, buf)
+        self.assertEqual(natural, buf.getvalue())
+
+        ## simple
+        # correct order
+        self.assertEqual(data, safe_load(simple, order_mode='simple'))
+        # wrong order
+        with self.assertRaises(ConstructorError) as ctx:
+            safe_load(natural, order_mode='simple')
+        problem = ctx.exception.problem.split(' at')[0]
+        self.assertEqual(
+            'keys out of order: expected *.10.1 got *.2.2', problem
+        )
+        buf = StringIO()
+        safe_dump(data, buf, order_mode='simple')
+        self.assertEqual(simple, buf.getvalue())
+
+        with self.assertRaises(InvalidOrder) as ctx:
+            safe_load(None, order_mode='bad')
+        self.assertEqual(
+            'Invalid order_mode, "bad", options are "natural", "simple"',
+            str(ctx.exception),
+        )
+        with self.assertRaises(InvalidOrder) as ctx:
+            safe_dump(None, None, order_mode='bad2')
+        self.assertEqual(
+            'Invalid order_mode, "bad2", options are "natural", "simple"',
+            str(ctx.exception),
+        )