🐛 Fix rendering of needextract needs (#1249)

This commit fixes two key issues of `needextract` nodes:

1. The call to `BuildEnvironment.resolve_references` was already converting the need node to its final representation (since the calls `process_need_nodes`), and so the later call to `build_need` didn't do anything.
   This is why the requested layout was not being created.

2. A `needextract` can generate multiple need representation, but all of them were being given the same `id` of the original `NeedExtract` node.
   All nodes are now wrapped in a containing node, that is set with this `id`

Additionally, warnings are now emitted for unfound needs, rather than raise exceptions

Also, `build_need` is renamed to `build_need_repr`, to better describe its purpose, and its signature is slightly changed, to allow for the new logic.

docs/directives/needextract.rst

@@ -23,12 +23,17 @@ For instance, a supplier could get a copy of requirements but would not see all
       :style: green_border
 
 
-.. note:: 
+.. caution::
+
+   ``needextract`` supports correct rendering of most, but not all, content coming from the original need.
+   Be careful when using complex content in the original need, like custom roles or directives
+   that require any sphinx transforms.
 
-   **needextract** supports the full filtering possibilities of **Sphinx-Needs**.
-   Please read :ref:`filter` for more information.
+``needextract`` supports the full filtering possibilities of sphinx-needs.
+Please read :ref:`filter` for more information.
 
-``needextract`` supports also arguments as filter string. It works like the option `filter`, but also
+It also supports arguments as filter string,
+which works like the option `filter`, but also
 supports need ID as filter argument.
 
 .. need-example::

sphinx_needs/directives/need.py

@@ -25,7 +25,7 @@
     resolve_variants_options,
 )
 from sphinx_needs.functions.functions import check_and_get_content
-from sphinx_needs.layout import build_need
+from sphinx_needs.layout import build_need_repr
 from sphinx_needs.logging import get_logger, log_warning
 from sphinx_needs.need_constraints import process_constraints
 from sphinx_needs.nodes import Need
@@ -434,15 +434,18 @@ def format_need_nodes(
         need_id = node_need.attributes["ids"][0]
         need_data = needs[need_id]
 
+        if need_data["hide"]:
+            remove_node_from_tree(node_need)
+            continue
+
         find_and_replace_node_content(node_need, env, need_data)
         for index, attribute in enumerate(node_need.attributes["classes"]):
             node_need.attributes["classes"][index] = check_and_get_content(
                 attribute, need_data, env
             )
 
-        layout = need_data["layout"] or NeedsSphinxConfig(app.config).default_layout
-
-        build_need(layout, node_need, app, fromdocname=fromdocname)
+        rendered_node = build_need_repr(node_need, need_data, app, docname=fromdocname)
+        node_need.parent.replace(node_need, rendered_node)
 
 
 def check_links(needs: dict[str, NeedsInfoType], config: NeedsSphinxConfig) -> None:

sphinx_needs/directives/needextract.py

@@ -1,26 +1,29 @@
 from __future__ import annotations
 
 import re
-from typing import Sequence, cast
+from typing import Sequence
 
 from docutils import nodes
 from docutils.parsers.rst import directives
 from docutils.transforms.references import Substitutions
 from sphinx.application import Sphinx
 from sphinx.environment.collectors.asset import DownloadFileCollector, ImageCollector
+from sphinx.util.logging import getLogger
 
-from sphinx_needs.api.exceptions import NeedsInvalidFilter
 from sphinx_needs.config import NeedsSphinxConfig
-from sphinx_needs.data import NeedsExtractType, SphinxNeedsData
+from sphinx_needs.data import NeedsExtractType, NeedsInfoType, SphinxNeedsData
 from sphinx_needs.debug import measure_time
 from sphinx_needs.directives.utils import (
     no_needs_found_paragraph,
     used_filter_paragraph,
 )
 from sphinx_needs.filter_common import FilterBase, process_filters
-from sphinx_needs.layout import SphinxNeedLayoutException, build_need
+from sphinx_needs.layout import build_need_repr
+from sphinx_needs.logging import log_warning
 from sphinx_needs.utils import add_doc, remove_node_from_tree
 
+LOGGER = getLogger(__name__)
+
 
 class Needextract(nodes.General, nodes.Element):
     pass
@@ -82,30 +85,42 @@ def process_needextract(
     env = app.env
     needs_config = NeedsSphinxConfig(app.config)
 
-    for node in found_nodes:
+    node: Needextract
+    for node in found_nodes:  # type: ignore[assignment]
         if not needs_config.include_needs:
             remove_node_from_tree(node)
             continue
 
         current_needextract: NeedsExtractType = node.attributes
         all_needs = SphinxNeedsData(env).get_or_create_needs()
-        content: list[nodes.Element] = []
+        content = nodes.container()
+        content.attributes["ids"] = [current_needextract["target_id"]]
 
         # check if filter argument and option filter both exist
         need_filter_arg = current_needextract["filter_arg"]
         if need_filter_arg and current_needextract["filter"]:
-            raise NeedsInvalidFilter(
-                "Needextract can't have filter arguments and option filter at the same time."
+            log_warning(
+                LOGGER,
+                "filter arguments and option filter at the same time are disallowed.",
+                "needextract",
+                location=node,
             )
+            remove_node_from_tree(node)
+            continue
         elif need_filter_arg:
             # check if given filter argument is need-id
             if need_filter_arg in all_needs:
                 need_filter_arg = f'id == "{need_filter_arg}"'
             elif re.fullmatch(needs_config.id_regex, need_filter_arg):
                 # check if given filter argument is need-id, but not exists
-                raise NeedsInvalidFilter(
-                    f"Provided id {need_filter_arg} for needextract does not exist."
+                log_warning(
+                    LOGGER,
+                    f"Requested need {need_filter_arg!r} not found.",
+                    "needextract",
+                    location=node,
                 )
+                remove_node_from_tree(node)
+                continue
             current_needextract["filter"] = need_filter_arg
 
         found_needs = process_filters(
@@ -119,18 +134,15 @@ def process_needextract(
         for need_info in found_needs:
             # filter out need_part from found_needs, in order to generate
             # copies of filtered needs with custom layout and style
-            if need_info["is_need"] and not need_info["is_part"]:
-                need_extract = _build_needextract(
-                    need_info["id"],
-                    app,
-                    layout=current_needextract["layout"],
-                    style=current_needextract["style"],
-                    docname=current_needextract["docname"],
+            if (
+                need_info["is_need"]
+                and not need_info["is_part"]
+                and (
+                    need_extract := _build_needextract(
+                        app, node, need_info, current_needextract
+                    )
                 )
-
-                # Add lineno to node
-                need_extract.line = current_needextract["lineno"]
-
+            ):
                 content.append(need_extract)
 
         if len(content) == 0:
@@ -152,87 +164,54 @@ def process_needextract(
 
 @measure_time("build_needextract")
 def _build_needextract(
-    need_id: str,
     app: Sphinx,
-    layout: str | None = None,
-    style: str | None = None,
-    docname: str | None = None,
-) -> nodes.container:
-    """
-    Creates a new need-node for a given layout.
-
-    Need must already exist in internal dictionary.
-    This creates a new representation only.
-    :param need_id: need id
-    :param app: sphinx application
-    :param layout: layout to use, overrides layout set by need itself
-    :param style: style to use, overrides styles set by need itself
-    :param docname: Needed for calculating references
-    :return:
-    """
+    extract_node: Needextract,
+    need_data: NeedsInfoType,
+    extract_data: NeedsExtractType,
+) -> nodes.container | None:
+    """Creates a new need representation."""
     env = app.env
-    needs = SphinxNeedsData(env).get_or_create_needs()
-
-    if need_id not in needs.keys():
-        raise SphinxNeedLayoutException(f"Given need id {need_id} does not exist.")
-
-    need_data = needs[need_id]
-
-    # Resolve internal references.
-    # This is done for original need content automatically.
-    # But as we are working on  a copy, we have to trigger this on our own.
-    if docname is None:
-        # needed to calculate relative references
-        # TODO ideally we should not cast here:
-        # the docname can still be None, if the need is external, although practically these are not rendered
-        docname = cast(str, needs[need_id]["docname"])
 
-    node_container = nodes.container()
-    # node_container += needs[need_id]["need_node"].children
-
-    node_inner = SphinxNeedsData(env).get_need_node(need_id)
-    assert node_inner is not None, f"Need {need_id} has no content node."
+    if (need_node := SphinxNeedsData(env).get_need_node(need_data["id"])) is None:
+        log_warning(
+            LOGGER,
+            f"Content for requested need {need_data['id']!r} not found.",
+            "needextract",
+            location=extract_node,
+        )
+        return None
 
-    # Rerun some important Sphinx collectors for need-content coming from "needsexternal".
-    # This is needed, as Sphinx needs to know images and download paths.
-    # Normally this gets done much earlier in the process, so that for the copied need-content this
-    # handling was and will not be done by Sphinx itself anymore.
+    dummy_need = nodes.container()
+    dummy_need.source, dummy_need.line = extract_node.source, extract_node.line
 
-    # Overwrite the docname, which must be the original one from the reused need, as all used paths are relative
-    # to the original location, not to the current document.
+    # Try to implement Sphinx transforms that would have already been done if the need was in the original document.
+    # Note, this is very hacky and can not possibly account for all transforms.
     env.temp_data["docname"] = need_data[
         "docname"
-    ]  # Dirty, as in this phase normally no docname is set anymore in env
-    ImageCollector().process_doc(app, node_inner)  # type: ignore[arg-type]
-    DownloadFileCollector().process_doc(app, node_inner)  # type: ignore[arg-type]
-
+    ]  # this is normally set in the read phase
+    ImageCollector().process_doc(app, need_node)  # type: ignore[arg-type]
+    DownloadFileCollector().process_doc(app, need_node)  # type: ignore[arg-type]
     del env.temp_data["docname"]  # Be sure our env is as it was before
 
-    node_container.append(node_inner)
+    dummy_need.extend(need_node.children)
 
     # resolve_references() ignores the given docname and takes the docname from the pending_xref node.
     # Therefore, we need to manipulate this first, before we can ask Sphinx to perform the normal
     # reference handling for us.
-    _replace_pending_xref_refdoc(node_container, docname)
-    env.resolve_references(node_container, docname, env.app.builder)  # type: ignore[arg-type]
-
-    node_container.attributes["ids"].append(need_id)
-
-    needs_config = NeedsSphinxConfig(app.config)
-    layout = layout or need_data["layout"] or needs_config.default_layout
-    style = style or need_data["style"] or needs_config.default_style
-
-    build_need(layout, node_container, app, style, docname)
-
-    # set the layout and style for the new need
-    node_container[0].attributes = node_container.parent.children[0].attributes  # type: ignore
-    node_container[0].children[0].attributes = (  # type: ignore
-        node_container.parent.children[0].children[0].attributes  # type: ignore
+    _replace_pending_xref_refdoc(dummy_need, extract_data["docname"])
+    env.resolve_references(dummy_need, extract_data["docname"], app.builder)  # type: ignore[arg-type]
+
+    dummy_need.attributes["ids"].append(need_data["id"])
+    rendered_node = build_need_repr(
+        dummy_need,  # type: ignore[arg-type]
+        need_data,
+        app,
+        layout=extract_data["layout"],
+        style=extract_data["style"],
+        docname=extract_data["docname"],
     )
 
-    node_container.attributes["ids"] = []
-
-    return node_container
+    return rendered_node
 
 
 def _replace_pending_xref_refdoc(node: nodes.Element, new_refdoc: str) -> None: