Repository: useblocks/sphinx-needs
Pull request #1223 (2024-08-20 11:03:07)

🐛 fix warnings for duplicate needs in parallel builds
Change duplicate need feedback from raising exceptions to emitting Sphinx warnings, e.g.

```
path/to/page.rst:4: WARNING: A need with ID STORY_PAGE_1 already exists, title: 'duplicate'. [needs.duplicate_id]
```

and ensure warning is also emitted during parallel builds

Note, although there was already a parallel build test in the test suite, 
actually it was not running parallel because there were not enough documents in the test project (see https://github.com/sphinx-doc/sphinx/pull/12796),
so one more is added and now the warning is correct.


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1223?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) Report
Attention: Patch coverage is `84.61538%` with `4 lines` in your changes missing coverage. Please review.
> Project coverage is 86.87%. Comparing base [(`2045121`)](https://app.codecov.io/gh/useblocks/sphinx-needs/commit/20451216479b46272121d8ddd7c43a78824aba72?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) to head [(`618c937`)](https://app.codecov.io/gh/useblocks/sphinx-needs/commit/618c9376765ad25eabe22d2c79abed038c1b5cb2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks).

| [Files](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1223?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) | Patch % | Lines |
|---|---|---|
| [sphinx\_needs/api/need.py](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1223?src=pr&el=tree&filepath=sphinx_needs%2Fapi%2Fneed.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks#diff-c3BoaW54X25lZWRzL2FwaS9uZWVkLnB5) | 60.00% | [2 Missing :warning: ](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) |
| [sphinx\_needs/external\_needs.py](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1223?src=pr&el=tree&filepath=sphinx_needs%2Fexternal_needs.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks#diff-c3BoaW54X25lZWRzL2V4dGVybmFsX25lZWRzLnB5) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #1223      +/-   ##
==========================================
+ Coverage   86.44%   86.87%   +0.43%     
==========================================
  Files          56       56              
  Lines        6501     6509       +8     
==========================================
+ Hits         5620     5655      +35     
+ Misses        881      854      -27     
```

| [Flag](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1223/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) | Coverage Δ | |
|---|---|---|
| [pytests](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1223/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks) | `86.87% <84.61%> (+0.43%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks#carryforward-flags-in-the-pull-request-comment) to find out more.

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/useblocks/sphinx-needs/pull/1223?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=useblocks).


Patch:
diff --git a/sphinx_needs/api/exceptions.py b/sphinx_needs/api/exceptions.py
index 1720441a..406e1e50 100644
--- a/sphinx_needs/api/exceptions.py
+++ b/sphinx_needs/api/exceptions.py
@@ -19,10 +19,6 @@ class NeedsNoIdException(SphinxError):
     pass
 
 
-class NeedsDuplicatedId(SphinxError):
-    pass
-
-
 class NeedsStatusNotAllowed(SphinxError):
     pass
 
diff --git a/sphinx_needs/api/need.py b/sphinx_needs/api/need.py
index ccfe6598..8b805443 100644
--- a/sphinx_needs/api/need.py
+++ b/sphinx_needs/api/need.py
@@ -16,7 +16,6 @@ from sphinx.environment import BuildEnvironment
 from sphinx_needs.api.configuration import NEEDS_CONFIG
 from sphinx_needs.api.exceptions import (
     NeedsConstraintNotAllowed,
-    NeedsDuplicatedId,
     NeedsInvalidException,
     NeedsInvalidOption,
     NeedsNoIdException,
@@ -312,19 +311,24 @@ def add_need(
 
     if need_id in SphinxNeedsData(env).get_or_create_needs():
         if id:
-            raise NeedsDuplicatedId(
-                f"A need with ID {need_id} already exists! "
-                f"This is not allowed. Document {docname}[{lineno}] Title: {title}."
-            )
+            message = f"A need with ID {need_id} already exists, " f"title: {title!r}."
         else:  # this is a generated ID
-            raise NeedsDuplicatedId(
+            _title = " ".join(title)
+            message = (
                 "Needs could not generate a unique ID for a need with "
-                "the title '{}' because another need had the same title. "
+                f"the title {_title!r} because another need had the same title. "
                 "Either supply IDs for the requirements or ensure the "
                 "titles are different.  NOTE: If title is being generated "
                 "from the content, then ensure the first sentence of the "
-                "requirements are different.".format(" ".join(title))
+                "requirements are different."
             )
+        logger.warning(
+            message + " [needs.duplicate_id]",
+            type="needs",
+            subtype="duplicate_id",
+            location=(docname, lineno) if docname else None,
+        )
+        return []
 
     # Trim title if it is too long
     max_length = needs_config.max_title_length
diff --git a/sphinx_needs/data.py b/sphinx_needs/data.py
index a97d76c6..b02ba3a5 100644
--- a/sphinx_needs/data.py
+++ b/sphinx_needs/data.py
@@ -6,6 +6,8 @@ from __future__ import annotations
 
 from typing import TYPE_CHECKING, Literal, TypedDict
 
+from sphinx.util.logging import getLogger
+
 if TYPE_CHECKING:
     from docutils.nodes import Element, Text
     from sphinx.application import Sphinx
@@ -15,6 +17,9 @@ if TYPE_CHECKING:
     from sphinx_needs.services.manager import ServiceManager
 
 
+LOGGER = getLogger(__name__)
+
+
 class NeedsFilterType(TypedDict):
     filter: str
     """Filter string."""
@@ -612,7 +617,7 @@ class SphinxNeedsData:
 
 
 def merge_data(
-    _app: Sphinx, env: BuildEnvironment, _docnames: list[str], other: BuildEnvironment
+    _app: Sphinx, env: BuildEnvironment, docnames: list[str], other: BuildEnvironment
 ) -> None:
     """
     Performs data merge of parallel executed workers.
@@ -621,13 +626,32 @@ def merge_data(
     Needs to update env manually for all data Sphinx-Needs collect during read phase
     """
 
-    # Update global needs dict
-    needs = SphinxNeedsData(env).get_or_create_needs()
-    other_needs = SphinxNeedsData(other).get_or_create_needs()
-    needs.update(other_needs)
     if SphinxNeedsData(other).has_export_filters:
         SphinxNeedsData(env).has_export_filters = True
 
+    # Update needs
+    needs = SphinxNeedsData(env).get_or_create_needs()
+    other_needs = SphinxNeedsData(other).get_or_create_needs()
+    for other_id, other_need in other_needs.items():
+        if other_id in needs:
+            # we only want to warn if the need comes from one of the docs parsed in this worker
+            _docname = other_need["docname"]
+            if _docname in docnames:
+                message = (
+                    f"A need with ID {other_id} already exists, "
+                    f"title: {other_need['title']!r}."
+                )
+                LOGGER.warning(
+                    message + " [needs.duplicate_id]",
+                    type="needs",
+                    subtype="duplicate_id",
+                    location=(_docname, other_need["lineno"]) if _docname else None,
+                )
+        else:
+            needs[other_id] = other_need
+
+    # update other data
+
     def _merge(name: str, is_complex_dict: bool = False) -> None:
         # Update global needs dict
         if not hasattr(env, name):
diff --git a/sphinx_needs/external_needs.py b/sphinx_needs/external_needs.py
index eaddfe78..3ecc1f3f 100644
--- a/sphinx_needs/external_needs.py
+++ b/sphinx_needs/external_needs.py
@@ -11,7 +11,6 @@ from sphinx.application import Sphinx
 from sphinx.environment import BuildEnvironment
 
 from sphinx_needs.api import add_external_need, del_need
-from sphinx_needs.api.exceptions import NeedsDuplicatedId
 from sphinx_needs.config import NeedsSphinxConfig
 from sphinx_needs.data import SphinxNeedsData
 from sphinx_needs.logging import get_logger
@@ -30,7 +29,7 @@ def get_target_template(target_url: str) -> Template:
     return mem_template
 
 
-def load_external_needs(app: Sphinx, env: BuildEnvironment, _docname: str) -> None:
+def load_external_needs(app: Sphinx, env: BuildEnvironment, docname: str) -> None:
     """Load needs from configured external sources."""
     needs_config = NeedsSphinxConfig(app.config)
     for source in needs_config.external_needs:
@@ -159,10 +158,14 @@ def load_external_needs(app: Sphinx, env: BuildEnvironment, _docname: str) -> No
                     # delete the already existing external need from api need
                     del_need(app, ext_need_id)
                 else:
-                    raise NeedsDuplicatedId(
-                        f'During external needs handling, an identical ID was detected: {ext_need_id} \
-                            from needs_external_needs url: {source["base_url"]}'
+                    log.warning(
+                        f'During external needs handling, an identical ID was detected: {ext_need_id} '
+                        f'from needs_external_needs url: {source["base_url"]} [needs.duplicate_id]',
+                        type="needs",
+                        subtype="duplicate_id",
+                        location=docname if docname else None,
                     )
+                    return None
 
             add_external_need(app, **need_params)
 


Test patch:
diff --git a/tests/doc_test/parallel_doc/index.rst b/tests/doc_test/parallel_doc/index.rst
index 4c318ed9..8b29080b 100644
--- a/tests/doc_test/parallel_doc/index.rst
+++ b/tests/doc_test/parallel_doc/index.rst
@@ -7,6 +7,7 @@ PARALLEL TEST DOCUMENT
    page_2
    page_3
    page_4
+   page_5
 
 .. spec:: Command line interface
     :id: SP_TOO_001
diff --git a/tests/doc_test/parallel_doc/page_5.rst b/tests/doc_test/parallel_doc/page_5.rst
new file mode 100644
index 00000000..602d47c9
--- /dev/null
+++ b/tests/doc_test/parallel_doc/page_5.rst
@@ -0,0 +1,5 @@
+Page 5
+======
+
+.. story:: duplicate
+   :id: STORY_PAGE_1
diff --git a/tests/test_broken_doc.py b/tests/test_broken_doc.py
index d203541e..1eddf7f6 100644
--- a/tests/test_broken_doc.py
+++ b/tests/test_broken_doc.py
@@ -1,18 +1,26 @@
-import pytest
+import os
 
-from sphinx_needs.api.need import NeedsDuplicatedId
+import pytest
+from sphinx.testing.util import SphinxTestApp
+from sphinx.util.console import strip_colors
 
 
 @pytest.mark.parametrize(
     "test_app",
-    [{"buildername": "html", "srcdir": "doc_test/broken_doc"}],
+    [{"buildername": "html", "srcdir": "doc_test/broken_doc", "no_plantuml": True}],
     indirect=True,
 )
-def test_doc_build_html(test_app):
-    with pytest.raises(NeedsDuplicatedId):
-        app = test_app
-
-        app.build()
-        html = (app.outdir / "index.html").read_text()
-        assert "<h1>BROKEN DOCUMENT" in html
-        assert "SP_TOO_001" in html
+def test_doc_build_html(test_app: SphinxTestApp):
+    test_app.build()
+    warnings = (
+        strip_colors(test_app._warning.getvalue())
+        .replace(str(test_app.srcdir) + os.path.sep, "<srcdir>/")
+        .strip()
+    )
+    assert (
+        warnings
+        == "<srcdir>/index.rst:11: WARNING: A need with ID SP_TOO_001 already exists, title: 'Command line interface'. [needs.duplicate_id]"
+    )
+    html = (test_app.outdir / "index.html").read_text()
+    assert "<h1>BROKEN DOCUMENT" in html
+    assert "SP_TOO_001" in html
diff --git a/tests/test_parallel_execution.py b/tests/test_parallel_execution.py
index 44164ffc..9f2bf042 100644
--- a/tests/test_parallel_execution.py
+++ b/tests/test_parallel_execution.py
@@ -1,31 +1,38 @@
+import os
 from pathlib import Path
 
 import pytest
+from sphinx.util.console import strip_colors
 
 
 @pytest.mark.parametrize(
     "test_app",
-    [{"buildername": "html", "srcdir": "doc_test/parallel_doc", "parallel": 4}],
+    [
+        {
+            "buildername": "html",
+            "srcdir": "doc_test/parallel_doc",
+            "parallel": 4,
+            "no_plantuml": True,
+        }
+    ],
     indirect=True,
 )
 def test_doc_build_html(test_app):
     app = test_app
     app.build()
-    html = Path(app.outdir, "index.html").read_text()
-    assert app.statuscode == 0
-    assert "<h1>PARALLEL TEST DOCUMENT" in html
-    assert "SP_TOO_001" in html
-
+    warnings = (
+        strip_colors(app._warning.getvalue())
+        .replace(str(app.srcdir) + os.path.sep, "<srcdir>/")
+        .strip()
+    )
+    assert (
+        warnings
+        == "<srcdir>/page_5.rst:4: WARNING: A need with ID STORY_PAGE_1 already exists, title: 'duplicate'. [needs.duplicate_id]"
+    )
 
-@pytest.mark.parametrize(
-    "test_app",
-    [{"buildername": "html", "srcdir": "doc_test/parallel_doc", "parallel": 4}],
-    indirect=True,
-)
-def test_doc_parallel_build_html(test_app):
-    app = test_app
-    app.build()
-    assert app.statuscode == 0
+    index_html = Path(app.outdir, "index.html").read_text()
+    assert "<h1>PARALLEL TEST DOCUMENT" in index_html
+    assert "SP_TOO_001" in index_html
 
     page3_html = Path(app.outdir, "page_3.html").read_text()
     assert "Page 3" in page3_html