Merge ~ack/maas:1949963-api-description-stable-sorting into maas:master

[Alberto Donato]

LP:1949963 ensure form choices are formatted in order in doc text

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1949963-api-description-stable-sorting lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/11469/console
COMMIT: 84aaeaa007c67b1431d8e254293dbd8b4951d876

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1949963-api-description-stable-sorting lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 6b5ab5590e107aa65deaada1e0e63fb050a6ce4b

review: Approve

[Björn Tillenius (bjornt)]

+1

review: Approve

diff --git a/src/maasserver/api/doc.py b/src/maasserver/api/doc.py
index b22251f..3b777aa 100644
--- a/src/maasserver/api/doc.py
+++ b/src/maasserver/api/doc.py
@@ -19,6 +19,7 @@ from inspect import getdoc
 from io import StringIO
 from itertools import zip_longest
 import json
+from operator import itemgetter
 from threading import RLock
 
 from django.urls import get_resolver
@@ -44,25 +45,23 @@ def accumulate_api_resources(resolver, accumulator):
     :rtype: Generator, yielding handlers.
     """
 
-    def p_has_resource_uri(resource):
-        return getattr(resource.handler, "resource_uri", None) is not None
-
-    def p_is_not_hidden(resource):
-        return getattr(resource.handler, "hidden", False)
+    def visible(handler):
+        return bool(
+            getattr(handler, "resource_uri", None)
+            and not getattr(handler, "hidden", False)
+        )
 
     for pattern in resolver.url_patterns:
         if isinstance(pattern, URLResolver):
             accumulate_api_resources(pattern, accumulator)
         elif isinstance(pattern, URLPattern):
-            if isinstance(pattern.callback, Resource):
-                resource = pattern.callback
-                if p_has_resource_uri(resource) and not p_is_not_hidden(
-                    resource
-                ):
-                    accumulator.add(resource)
+            if isinstance(pattern.callback, Resource) and visible(
+                pattern.callback.handler
+            ):
+                accumulator.add(pattern.callback)
         else:
             raise AssertionError(
-                "Not a recognised pattern or resolver: %r" % (pattern,)
+                f"Not a recognised pattern or resolver: {pattern!r}"
             )
 
 
@@ -228,10 +227,18 @@ def describe_actions(handler):
       restful: Indicates if this is a CRUD/ReSTful action.
 
     """
-    from maasserver.api import support  # Circular import.
+    from maasserver.api import support
 
     getname = support.OperationsResource.crudmap.get
-    for signature, function in handler.exports.items():
+
+    actions = []
+
+    # ensure stable sorting, accounting for operation being None
+    exports = sorted(
+        handler.exports.items(),
+        key=lambda item: (item[0][0], item[0][1] or ""),
+    )
+    for signature, function in exports:
         http_method, operation = signature
         name = getname(http_method) if operation is None else operation
 
@@ -276,13 +283,16 @@ def describe_actions(handler):
                         )
                         doc += ":type %s: %s\n\n " % (pname, param["type"])
 
-        yield dict(
-            method=http_method,
-            name=name,
-            doc=doc,
-            op=operation,
-            restful=(operation is None),
+        actions.append(
+            {
+                "method": http_method,
+                "name": name,
+                "doc": doc,
+                "op": operation,
+                "restful": operation is None,
+            }
         )
+    return sorted(actions, key=itemgetter("name"))
 
 
 def describe_handler(handler):
@@ -306,7 +316,7 @@ def describe_handler(handler):
     )
 
     return {
-        "actions": list(describe_actions(handler)),
+        "actions": describe_actions(handler),
         "doc": getdoc(handler),
         "name": handler.__name__,
         "params": tuple(uri_params),
@@ -345,26 +355,30 @@ def describe_api():
     """
     from maasserver import urls_api as urlconf
 
+    resources = sorted(
+        (
+            describe_resource(resource)
+            for resource in find_api_resources(urlconf)
+        ),
+        key=itemgetter("name"),
+    )
+
     # This is the core of it:
     description = {
         "doc": "MAAS API",
-        "resources": [
-            describe_resource(resource)
-            for resource in find_api_resources(urlconf)
-        ],
+        "resources": resources,
     }
-
     # However, for backward compatibility, add "handlers" as an alias for all
     # not-None anon and auth handlers in "resources".
     description["handlers"] = []
     description["handlers"].extend(
         resource["anon"]
-        for resource in description["resources"]
+        for resource in resources
         if resource["anon"] is not None
     )
     description["handlers"].extend(
         resource["auth"]
-        for resource in description["resources"]
+        for resource in resources
         if resource["auth"] is not None
     )
 
@@ -537,11 +551,7 @@ def hash_canonical(description):
     into a new `hashlib.sha1` object.
     """
     description = describe_canonical(description)
-    description_as_json = json.dumps(description)
-    # Python 3's json.dumps returns a `str`, so encode if necessary.
-    if not isinstance(description_as_json, bytes):
-        description_as_json = description_as_json.encode("ascii")
-    # We /could/ instead pass a hashing object in and call .update()...
+    description_as_json = json.dumps(description).encode("ascii")
     return hashlib.sha1(description_as_json)
 
 
diff --git a/src/maasserver/api/doc_handler.py b/src/maasserver/api/doc_handler.py
index 9b332c7..e9e1f4a 100644
--- a/src/maasserver/api/doc_handler.py
+++ b/src/maasserver/api/doc_handler.py
@@ -235,11 +235,12 @@ def describe(request):
     # The handler URIs returned by describe_resource() are relative paths.
     absolute = partial(build_absolute_uri, request)
     for resource in description["resources"]:
-        for handler_type in "anon", "auth":
+        for handler_type in ("anon", "auth"):
             handler = resource[handler_type]
             if handler is not None:
                 handler["uri"] = absolute(handler["path"])
     # Return as a JSON document.
     return HttpResponse(
-        json.dumps(description), content_type="application/json"
+        json.dumps(description, sort_keys=True),
+        content_type="application/json",
     )
diff --git a/src/maasserver/forms/settings.py b/src/maasserver/forms/settings.py
index 10ab368..5a662a2 100644
--- a/src/maasserver/forms/settings.py
+++ b/src/maasserver/forms/settings.py
@@ -75,7 +75,7 @@ def make_default_osystem_field(*args, **kwargs):
                 "osystem", os_choices
             )
         },
-        **kwargs
+        **kwargs,
     )
     return field
 
@@ -154,7 +154,7 @@ def make_default_distro_series_field(*args, **kwargs):
                 "release", release_choices
             )
         },
-        **kwargs
+        **kwargs,
     )
     return field
 
@@ -186,7 +186,7 @@ def make_default_min_hwe_kernel_field(*args, **kwargs):
                 "default_min_hwe_kernel", kernel_choices
             )
         },
-        **kwargs
+        **kwargs,
     )
     return field
 
@@ -206,7 +206,7 @@ def make_commissioning_distro_series_field(*args, **kwargs):
                 "commissioning_distro_series", commissioning_choices
             )
         },
-        **kwargs
+        **kwargs,
     )
     return field
 
@@ -221,7 +221,7 @@ def make_dnssec_validation_field(*args, **kwargs):
                 "dnssec_validation", DNSSEC_VALIDATION_CHOICES
             )
         },
-        **kwargs
+        **kwargs,
     )
     return field
 
@@ -236,7 +236,7 @@ def make_network_discovery_field(*args, **kwargs):
                 "network_discovery", NETWORK_DISCOVERY_CHOICES
             )
         },
-        **kwargs
+        **kwargs,
     )
     return field
 
@@ -251,7 +251,7 @@ def make_active_discovery_interval_field(*args, **kwargs):
                 "active_discovery_interval", ACTIVE_DISCOVERY_INTERVAL_CHOICES
             )
         },
-        **kwargs
+        **kwargs,
     )
     return field
 
@@ -998,17 +998,12 @@ def get_config_form(config_name, data=None):
     return form
 
 
-def describe_choices(choices):
-    """Describe the items in an enumeration of Django form choices."""
-    return ", ".join(
-        "'%s' (%s)" % (value, meaning) for value, meaning in choices
-    )
-
-
-def get_config_doc(indentation=0):
+def get_config_doc(config_items=None, indentation=0):
     """Return the documentation about the available configuration settings."""
+    if config_items is None:
+        config_items = CONFIG_ITEMS
     doc = ["Available configuration items:\n\n"]
-    for config_name, config_details in sorted(CONFIG_ITEMS.items()):
+    for config_name, config_details in sorted(config_items.items()):
         form_details = config_details["form_kwargs"]
         doc.append(":" + config_name + ": " + form_details["label"] + ". ")
         # Append help text if present.
@@ -1018,7 +1013,9 @@ def get_config_doc(indentation=0):
         # Append list of possible choices if present.
         choices = form_details.get("choices")
         if choices is not None:
-            choice_descr = describe_choices(choices)
+            choice_descr = ", ".join(
+                f"'{value}' ({meaning})" for value, meaning in sorted(choices)
+            )
             doc.append("Available choices are: %s." % choice_descr)
         doc.append("\n")
     full_text = (" " * indentation).join(doc)
diff --git a/src/maasserver/forms/tests/test_settings.py b/src/maasserver/forms/tests/test_settings.py
index a1dcc23..f0cd075 100644
--- a/src/maasserver/forms/tests/test_settings.py
+++ b/src/maasserver/forms/tests/test_settings.py
@@ -1,8 +1,7 @@
 # Copyright 2013-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Test forms settings."""
-
+from textwrap import dedent
 
 from django import forms
 from django.core.exceptions import ValidationError
@@ -41,9 +40,31 @@ class TestGetConfigForm(MAASServerTestCase):
 
 class TestGetConfigDoc(MAASServerTestCase):
     def test_get_config_doc(self):
-        doc = get_config_doc()
-        # Just make sure that the doc looks okay.
-        self.assertIn("maas_name", doc)
+        config_items = {
+            "testitem": {
+                "default": "foo",
+                "form": forms.CharField,
+                "form_kwargs": {
+                    "label": "bar",
+                    "choices": [
+                        ("b", "B"),
+                        ("a", "A"),
+                    ],
+                },
+            },
+        }
+        doc = get_config_doc(config_items=config_items)
+        # choices are returned in the correct order
+        self.assertEqual(
+            doc,
+            dedent(
+                """\
+                Available configuration items:
+
+                :testitem: bar. Available choices are: 'a' (A), 'b' (B).
+                """
+            ),
+        )
 
 
 class TestSpecificConfigSettings(MAASServerTestCase):
diff --git a/src/maasserver/urls_api.py b/src/maasserver/urls_api.py
index ff6bf95..bbf76a1 100644
--- a/src/maasserver/urls_api.py
+++ b/src/maasserver/urls_api.py
@@ -566,7 +566,7 @@ urlpatterns += [
     ),
     url(r"^vlans/(?P<vlan_id>[^/]+)/$", vlan_handler, name="vlanid_handler"),
     url(
-        r"fabrics/(?P<fabric_id>[^/]+)/vlans/(?P<vid>[^/]+)/$",
+        r"^fabrics/(?P<fabric_id>[^/]+)/vlans/(?P<vid>[^/]+)/$",
         vlan_handler,
         name="vlan_handler",
     ),