Don't lose python/name tags values in mkdocs.yml (#7507)

Co-authored-by: Manuel Kaufmann <[email protected]>

Author: pawamoy

readthedocs/config/tests/test_yaml_loader.py

@@ -1,20 +1,38 @@
-import textwrap
-from readthedocs.doc_builder.backends.mkdocs import yaml_load_safely
+from readthedocs.doc_builder.backends.mkdocs import (
+    yaml_dump_safely,
+    yaml_load_safely,
+    ProxyPythonName
+)
+
+content = '''
+int: 3
+float: !!float 3
+function: !!python/name:python_function
+other_function: !!python/name:module.other.function
+unknown: !!python/module:python_module
+'''
 
 
 def test_yaml_load_safely():
     expected = {
         'int': 3,
         'float': 3.0,
+        'function': ProxyPythonName('python_function'),
+        'other_function': ProxyPythonName('module.other.function'),
         'unknown': None,
     }
-
-    content = textwrap.dedent('''
-    int: 3
-    float: !!float 3
-    unknown: !!python/name:unknown_funtion
-    ''')
     data = yaml_load_safely(content)
+
     assert data == expected
     assert type(data['int']) is int
     assert type(data['float']) is float
+    assert type(data['function']) is ProxyPythonName
+    assert type(data['other_function']) is ProxyPythonName
+    assert data['function'].value == 'python_function'
+    assert data['other_function'].value == 'module.other.function'
+
+
+def test_yaml_dump_safely():
+    data = yaml_load_safely(content)
+    assert yaml_load_safely(yaml_dump_safely(data)) == data
+

readthedocs/doc_builder/backends/mkdocs.py

@@ -214,7 +214,7 @@ def append_conf(self):
                 user_config['theme'] = self.DEFAULT_THEME_NAME
 
         # Write the modified mkdocs configuration
-        yaml.safe_dump(
+        yaml_dump_safely(
             user_config,
             open(self.yaml_file, 'w'),
         )
@@ -326,27 +326,67 @@ class MkdocsHTML(BaseMkdocs):
     build_dir = '_build/html'
 
 
-class SafeLoaderIgnoreUnknown(yaml.SafeLoader):  # pylint: disable=too-many-ancestors
+# TODO: find a better way to integrate with MkDocs.
+# See https://github.com/readthedocs/readthedocs.org/issues/7844
+
+
+class ProxyPythonName(yaml.YAMLObject):
+    def __init__(self, value):
+        self.value = value
+
+    def __eq__(self, other):
+        return self.value == other.value
+
+
+class SafeLoader(yaml.SafeLoader):  # pylint: disable=too-many-ancestors
 
     """
-    YAML loader to ignore unknown tags.
+    Safe YAML loader.
+
+    This loader parses special ``!!python/name:`` tags without actually
+    importing or executing code. Every other special tag is ignored.
 
     Borrowed from https://stackoverflow.com/a/57121993
+    Issue https://github.com/readthedocs/readthedocs.org/issues/7461
     """
 
     def ignore_unknown(self, node):  # pylint: disable=no-self-use, unused-argument
         return None
 
+    def construct_python_name(self, suffix, node):  # pylint: disable=no-self-use, unused-argument
+        return ProxyPythonName(suffix)
+
+
+class SafeDumper(yaml.SafeDumper):
 
-SafeLoaderIgnoreUnknown.add_constructor(None, SafeLoaderIgnoreUnknown.ignore_unknown)
+    """
+    Safe YAML dumper.
+
+    This dumper allows to avoid losing values of special tags that
+    were parsed by our safe loader.
+    """
+
+    def represent_name(self, data):
+        return self.represent_scalar("tag:yaml.org,2002:python/name:" + data.value, "")
+
+
+SafeLoader.add_multi_constructor("tag:yaml.org,2002:python/name:", SafeLoader.construct_python_name)
+SafeLoader.add_constructor(None, SafeLoader.ignore_unknown)
+SafeDumper.add_representer(ProxyPythonName, SafeDumper.represent_name)
 
 
 def yaml_load_safely(content):
     """
-    Uses ``SafeLoaderIgnoreUnknown`` loader to skip unknown tags.
+    Uses ``SafeLoader`` loader to skip unknown tags.
 
-    When a YAML contains ``!!python/name:int`` it will complete ignore it an
-    return ``None`` for those fields instead of failing. We need this to avoid
-    executing random code, but still support these YAML files.
+    When a YAML contains ``!!python/name:int`` it will store the ``int``
+    suffix temporarily to be able to re-dump it later. We need this to avoid
+    executing random code, but still support these YAML files without
+    information loss.
     """
-    return yaml.load(content, Loader=SafeLoaderIgnoreUnknown)
+    return yaml.load(content, Loader=SafeLoader)
+
+
+def yaml_dump_safely(content, stream=None):
+    """Uses ``SafeDumper`` dumper to write YAML contents."""
+    return yaml.dump(content, stream=stream, Dumper=SafeDumper)

readthedocs/rtd_tests/tests/test_doc_builder.py

@@ -13,7 +13,7 @@
 
 from readthedocs.builds.constants import EXTERNAL
 from readthedocs.builds.models import Version
-from readthedocs.doc_builder.backends.mkdocs import MkdocsHTML, yaml_load_safely
+from readthedocs.doc_builder.backends.mkdocs import MkdocsHTML, SafeDumper, yaml_load_safely
 from readthedocs.doc_builder.backends.sphinx import (
     BaseSphinx,
     HtmlBuilder,
@@ -392,7 +392,7 @@ def test_get_theme_name_with_feature_flag(self, checkout_path, run):
                 }
                 builder.append_conf()
 
-            mock_yaml.safe_dump.assert_called_once_with(
+            mock_yaml.dump.assert_called_once_with(
                 {
                     'site_name': mock.ANY,
                     'docs_dir': mock.ANY,
@@ -401,7 +401,8 @@ def test_get_theme_name_with_feature_flag(self, checkout_path, run):
                     'google_analytics': mock.ANY,
                     'theme': 'readthedocs',
                 },
-                mock.ANY,
+                stream=mock.ANY,
+                Dumper=SafeDumper,
             )
             mock_yaml.reset_mock()
 
@@ -417,7 +418,7 @@ def test_get_theme_name_with_feature_flag(self, checkout_path, run):
                 }
                 builder.append_conf()
 
-            mock_yaml.safe_dump.assert_called_once_with(
+            mock_yaml.dump.assert_called_once_with(
                 {
                     'site_name': mock.ANY,
                     'docs_dir': mock.ANY,
@@ -426,7 +427,8 @@ def test_get_theme_name_with_feature_flag(self, checkout_path, run):
                     'google_analytics': mock.ANY,
                     'theme': 'customtheme',
                 },
-                mock.ANY,
+                stream=mock.ANY,
+                Dumper=SafeDumper,
             )
 
     @patch('readthedocs.doc_builder.base.BaseBuilder.run')