DOP-2637: Properly report diagnostics on YAML parsing and unmarshaling errors

Author: i80and

snooty/gizaparser/parse.py

@@ -64,10 +64,13 @@ def parse(
         diagnostics.append(parse_diagnostic)
         return ([], text, diagnostics)
 
-    try:
-        parsed = [check_type(ty, data) for data in parsed_yaml]
-        return parsed, text, diagnostics
-    except LoadError as err:
-        mapping = err.bad_data if isinstance(err.bad_data, dict) else {}
-        lineno = mapping._start_line if isinstance(mapping, mapping_dict) else 0
-        return [], text, diagnostics + [UnmarshallingError(str(err), lineno)]
+    parsed: List[_T] = []
+    for data in parsed_yaml:
+        try:
+            parsed.append(check_type(ty, data))
+        except LoadError as err:
+            mapping = err.bad_data if isinstance(err.bad_data, dict) else {}
+            lineno = mapping._start_line if isinstance(mapping, mapping_dict) else 0
+            diagnostics.append(UnmarshallingError(str(err), lineno))
+
+    return parsed, text, diagnostics

snooty/gizaparser/test_extracts.py

@@ -1,7 +1,12 @@
 from pathlib import Path, PurePath
 from typing import Dict, List, Optional, Tuple
 
-from ..diagnostics import Diagnostic, FailedToInheritRef
+from ..diagnostics import (
+    Diagnostic,
+    ErrorParsingYAMLFile,
+    FailedToInheritRef,
+    UnmarshallingError,
+)
 from ..page import Page
 from ..parser import EmbeddedRstParser
 from ..types import FileId, ProjectConfig
@@ -140,3 +145,77 @@ def test_external_cycle() -> None:
             FileId("includes/extracts-test2.yaml"): [FailedToInheritRef],
             FileId("includes/extracts-test1.yaml"): [FailedToInheritRef],
         }
+
+
+def test_partial_unmarshaling_error() -> None:
+    with make_test(
+        {
+            Path(
+                "source/includes/extracts-test1.yaml"
+            ): """
+ref: bypassDocumentValidation-db.collection.aggregate
+inherit:
+  ref: _bypassDocValidation
+  file: extracts-bypassDocumentValidation-base.yaml
+replacement:
+  role: ":method:`db.collection.aggregate()`"
+  interface: "method"
+post: |
+  Document validation only occurs if you are using the
+  :pipeline:`$out` operator in your aggregation operation.
+---
+ref: bypassDocumentValidation-aggregate
+content: "Okay"
+...
+"""
+        }
+    ) as result:
+        assert list(result.pages.keys()) == [
+            FileId("includes/extracts/bypassDocumentValidation-aggregate.rst")
+        ]
+        assert {k: [type(d) for d in v] for k, v in result.diagnostics.items()} == {
+            FileId("includes/extracts-test1.yaml"): [UnmarshallingError]
+        }
+
+
+def test_single_unmarshaling_error() -> None:
+    with make_test(
+        {
+            Path(
+                "source/includes/extracts-test1.yaml"
+            ): """
+ref: bypassDocumentValidation-db.collection.aggregate
+inherit:
+  ref: _bypassDocValidation
+  file: extracts-bypassDocumentValidation-base.yaml
+replacement:
+  role: ":method:`db.collection.aggregate()`"
+  interface: "method"
+post: |
+  Document validation only occurs if you are using the
+  :pipeline:`$out` operator in your aggregation operation.
+...
+"""
+        }
+    ) as result:
+        assert list(result.pages.keys()) == []
+        assert {k: [type(d) for d in v] for k, v in result.diagnostics.items()} == {
+            FileId("includes/extracts-test1.yaml"): [UnmarshallingError]
+        }
+
+
+def test_parse_error() -> None:
+    with make_test(
+        {
+            Path(
+                "source/includes/extracts-test1.yaml"
+            ): """
+ref: bypassDocumentValidation
+content: "not okay
+"""
+        }
+    ) as result:
+        assert list(result.pages.keys()) == []
+        assert {k: [type(d) for d in v] for k, v in result.diagnostics.items()} == {
+            FileId("includes/extracts-test1.yaml"): [ErrorParsingYAMLFile]
+        }

snooty/parser.py

@@ -1105,6 +1105,7 @@ def __init__(self, postprocessor_factory: Callable[[], Postprocessor]) -> None:
 
         self.postprocessor_factory = postprocessor_factory
         self._parsed: Dict[FileId, Tuple[Page, FileId, List[Diagnostic]]] = {}
+        self._orphan_diagnostics: Dict[FileId, List[Diagnostic]] = {}
         self.__cached = PostprocessorResult({}, {}, {}, TargetDatabase())
         self.__changed_pages: Set[FileId] = set()
 
@@ -1137,6 +1138,13 @@ def start(
             Postprocessor, PostprocessorResult
         ] = util.WorkerLauncher("postprocessor", start)
 
+    def set_orphan_diagnostics(self, key: FileId, value: List[Diagnostic]) -> None:
+        """Some diagnostics can't be associated with a parsed Page because of underlying
+        problems like invalid YAML syntax. These are orphan diagnostics, and we need
+        to track them too."""
+        with self._lock:
+            self._orphan_diagnostics[key] = value
+
     def __setitem__(
         self, key: FileId, value: Tuple[Page, FileId, List[Diagnostic]]
     ) -> None:
@@ -1164,7 +1172,15 @@ def __contains__(self, key: FileId) -> bool:
 
     def __delitem__(self, key: FileId) -> None:
         with self._lock:
-            del self._parsed[key]
+            try:
+                del self._parsed[key]
+            except KeyError:
+                pass
+
+            try:
+                del self._orphan_diagnostics[key]
+            except KeyError:
+                pass
 
     def merge_diagnostics(
         self, *others: Dict[FileId, List[Diagnostic]]
@@ -1174,6 +1190,12 @@ def merge_diagnostics(
                 v[1]: list(v[2]) for v in self._parsed.values()
             }
 
+            for key, diagnostics in self._orphan_diagnostics.items():
+                if key in result:
+                    result[key].extend(diagnostics)
+                else:
+                    result[key] = list(diagnostics)
+
         all_keys: Set[FileId] = set()
         for other in others:
             all_keys.update(other.keys())
@@ -1501,11 +1523,13 @@ def build(
         for prefix, giza_category in self.yaml_mapping.items():
             logger.debug("Parsing %s YAML", prefix)
             for path in categorized[prefix]:
-                steps, text, diagnostics = giza_category.parse(path)
-                all_yaml_diagnostics[path] = diagnostics
-                giza_category.add(path, text, steps)
+                artifacts, text, diagnostics = giza_category.parse(path)
+                if diagnostics:
+                    all_yaml_diagnostics[path] = diagnostics
+                giza_category.add(path, text, artifacts)
 
         # Now that all of our YAML files are loaded, generate a page for each one
+        seen_paths: Set[Path] = set()
         for prefix, giza_category in self.yaml_mapping.items():
             logger.debug("Processing %s YAML: %d nodes", prefix, len(giza_category))
             for file_id, giza_node in giza_category.reify_all_files(
@@ -1531,10 +1555,19 @@ def create_page(filename: str) -> Tuple[Page, EmbeddedRstParser]:
                 for page in giza_category.to_pages(
                     giza_node.path, create_page, giza_node.data
                 ):
+                    seen_paths.add(page.source_path)
                     self._page_updated(
                         page, all_yaml_diagnostics.get(page.source_path, [])
                     )
 
+        # Handle parsing and unmarshaling errors that lead to diagnostics not associated with
+        # any page.
+        for key in all_yaml_diagnostics:
+            if key not in seen_paths:
+                self.pages.set_orphan_diagnostics(
+                    self.config.get_fileid(key), all_yaml_diagnostics[key]
+                )
+
         if postprocess:
             postprocessor_result = self.postprocess()