Skip to content

feat!: needextend effects must be local #689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AlexanderLanin wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat!: needextend effects must be local #689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/how-to/write_docs.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ For further documentation on needextends please `look here <https://sphinx-needs

.. note::

In the future we will enable a check that needextends will only modify needs in the current document.
You can ensure this by adding `c.this_doc()` to the filter string of the need.
Needextends may only modify needs in the current document. Add `c.this_doc()`
to the filter string of the needextend as shown in the examples above.


Requirement ID Feature Part
Expand Down
272 changes: 168 additions & 104 deletions src/extensions/score_metamodel/checks/check_needs_extends.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,15 @@

import sphinx_needs.directives.need
from sphinx_needs.config import NeedsSphinxConfig
from sphinx_needs.data import ExtendType, NeedsExtendType, NeedsMutable
from sphinx_needs.data import (
ExtendType,
NeedsExtendType,
NeedsMutable,
)
from sphinx_needs.directives.needextend import extend_needs_data as original_function
from sphinx_needs.exceptions import NeedsInvalidFilter
from sphinx_needs.filter_common import filter_needs_mutable
from sphinx_needs.logging import get_logger, log_warning
from sphinx_needs.need_item import NeedItem
from sphinx_needs.needs_schema import (
FieldFunctionArray,
FieldLiteralValue,
Expand All @@ -28,114 +32,174 @@

logger = get_logger(__name__)

NeedextendLocation = tuple[str, int]


def _location(needextend: NeedsExtendType) -> NeedextendLocation:
"""Return the source location used for every diagnostic of one directive."""
return needextend["docname"], needextend["lineno"]


def _warn(message: str, location: NeedextendLocation) -> None:
"""Report a needextend violation at the directive, not at its target need."""
log_warning(logger, message, "needextend", location=location)


def _verify_needs_are_in_document(
needs: list[NeedItem], location: NeedextendLocation
) -> None:
"""Report selected needs outside the directive's source document.

Both ID and expression targets resolve to need records. Validating that shared
representation keeps the document-boundary policy identical for both syntaxes.
"""
remote_ids = {need["id"] for need in needs if need["docname"] != location[0]}
if remote_ids:
_warn(
"Needextends may only modify needs in the current document. "
f"Matching needs: {', '.join(sorted(remote_ids))}.",
location,
)


def score_extend_needs_data_func( # noqa: C901
def _fetch_needs(
all_needs: NeedsMutable,
needextend: NeedsExtendType,
needs_config: NeedsSphinxConfig,
) -> list[NeedItem]:
"""Resolve either supported target syntax to the needs it selects."""
location = _location(needextend)

if needextend["filter_is_id"]:
# ``.. needextend:: NEED_ID`` has no expression to constrain, so inspect
# its resolved target in the shared document-boundary check below.
need_id = needextend["filter"]
try:
return [all_needs[need_id]]
except KeyError:
_warn(
f"Provided id {need_id!r} for needextend does not exist.",
location,
)
return []
Comment on lines +76 to +84

else:
need_filter = needextend["filter"]

if "c.this_doc()" not in need_filter:
_warn(
"needextend in S-CORE must always be used per document only. "
"Please add 'c.this_doc()' to the needextend to limit its effects to the correct document. "
"See https://eclipse-score.github.io/docs-as-code/main/how-to/write_docs.html#needextend for more information.",
location,
)

try:
return filter_needs_mutable(
all_needs,
needs_config,
need_filter,
location=location,
origin_docname=location[0],
)
except Exception as e:
_warn(f"Invalid filter {need_filter!r}: {e}", location)
return []
Comment on lines +97 to +107


def _validate_list_modifications(
need: NeedItem,
needextend: NeedsExtendType,
location: NeedextendLocation,
) -> None:
"""Reject destructive changes to link lists, which would erase traceability."""
for _, action, value in needextend["list_modifications"]:
replaces_or_deletes_links = action in {
ExtendType.REPLACE,
ExtendType.DELETE,
} and isinstance(value, LinksLiteralValue | LinksFunctionArray)
if replaces_or_deletes_links:
_warn(
f"Error when extending need: {need['id']}. "
"Replace or Delete action is not allowed via needextends.",
location,
)


def _validate_field_modifications(
need: NeedItem,
needextend: NeedsExtendType,
location: NeedextendLocation,
) -> None:
"""Reject field changes that discard data or append to scalar fields."""
for option_name, action, value in needextend["modifications"]:
is_scalar_append = (
action == ExtendType.APPEND
and isinstance(value, FieldLiteralValue)
and isinstance(value.value, str)
)
is_supported_replacement = action == ExtendType.REPLACE and (
value is None or isinstance(value, FieldLiteralValue | FieldFunctionArray)
)

if action == ExtendType.DELETE:
_warn(
f"Error when extending need: {need['id']}. "
"Delete action is not allowed via needextends.",
location,
)
elif is_scalar_append:
_warn(
f"Error when extending need: {need['id']}. "
"Append action is not allowed via needextends on 'string type options'.",
location,
)
elif is_supported_replacement and need[option_name]:
_warn(
f"Error when extending need: {need['id']}. "
"Replacing of options that are already set is not allowed via needextends.",
location,
)


def _ensure_non_destructive_changes(
need: NeedItem,
needextend: NeedsExtendType,
location: NeedextendLocation,
) -> None:
"""Apply SCORE's non-destructive extension policy to one selected need."""
if need["is_external"]:
_warn(
f"Error when extending need: {need['id']}. "
"It is not allowed to modify external needs via needextend",
location,
)
_validate_list_modifications(need, needextend, location)
_validate_field_modifications(need, needextend, location)


def score_extend_needs_data_func(
all_needs: NeedsMutable,
extends: dict[str, NeedsExtendType],
needs_config: NeedsSphinxConfig,
):
"""Use data gathered from needextend directives to modify fields of existing needs."""
# regardless of parallel build worker completion order.
sorted_extends = sorted(extends.values(), key=lambda x: (x["docname"], x["lineno"]))

current_needextend: NeedsExtendType
for current_needextend in sorted_extends:
need_filter = current_needextend["filter"]
location = (current_needextend["docname"], current_needextend["lineno"])

# ╓ ╖
# ║ This is currently as a grace period still allowed, but ║
# ║ will be forbiden in future releases ║
# ╙ ╜
# if "c.this_doc()" not in need_filter:
# error_msg = "Potentially altering needs outside of the document is not allowed. Please add 'c.this_doc()' to the needextend to limit it to only needs in the same document"
# log_warning(logger, error_msg, "needextend", location=location)

if current_needextend["filter_is_id"]:
try:
found_needs = [all_needs[need_filter]]
except KeyError:
error = f"Provided id {need_filter!r} for needextend does not exist."
if current_needextend["strict"]:
raise NeedsInvalidFilter(error) from KeyError
log_warning(logger, error, "needextend", location=location)
continue
else:
try:
found_needs = filter_needs_mutable(
all_needs,
needs_config,
need_filter,
location=location,
origin_docname=current_needextend["docname"],
)
except Exception as e:
log_warning(
logger,
f"Invalid filter {need_filter!r}: {e}",
"needextend",
location=location,
)
continue
for found_need in found_needs:
if found_need["is_external"]:
log_warning(
logger,
f"Error when extending need: {found_need['id']}. "
+ "It is not allowed to modify external needs via needextend",
"needextend",
location,
)
# Work in the stored needs, not on the search result
need = all_needs[found_need["id"]]

location = (
current_needextend["docname"],
current_needextend["lineno"],
)
"""Validate SCORE's needextend policy, then let Sphinx-Needs apply it.

This wrapper intentionally only reports violations. The unmodified directives
are still passed to Sphinx-Needs so its normal processing and diagnostics remain
authoritative.
"""
# Sphinx-Needs applies extensions in source order as well. Matching that order
# keeps warning output stable and mirrors the later application order.
ordered_extends = sorted(extends.values(), key=_location)

for needextend in ordered_extends:
needs = _fetch_needs(all_needs, needextend, needs_config)
_verify_needs_are_in_document(needs, _location(needextend))

for n in needs:
_ensure_non_destructive_changes(n, needextend, _location(needextend))

for _, etype, link_value in current_needextend["list_modifications"]:
match (etype, link_value):
case (
ExtendType.REPLACE | ExtendType.DELETE,
LinksLiteralValue() | LinksFunctionArray(),
):
# Replacing / Deleting links is not allowed
error_msg = (
f"Error when extending need: {need['id']}. "
"Replace or Delete action is not allowed via needextends."
)
# logger.warning_for_need(current_needextend["id"], error_msg)
log_warning(logger, error_msg, "needextend", location=location)

for option_name, etype, field_value in current_needextend["modifications"]:
if etype == ExtendType.DELETE:
error_msg = (
f"Error when extending need: {need['id']}. "
"Delete action is not allowed via needextends."
)
log_warning(logger, error_msg, "needextend", location=location)
match (etype, field_value):
case (ExtendType.APPEND, FieldLiteralValue()):
if isinstance(field_value.value, str):
error_msg = (
f"Error when extending need: {need['id']}. "
"Append action is not allowed via needextends on 'string type options'."
)
log_warning(
logger, error_msg, "needextend", location=location
)

case (
ExtendType.REPLACE,
None | FieldLiteralValue() | FieldFunctionArray(),
):
if need[option_name]:
error_msg = f"Error when extending need: {need['id']}. Replacing of options that are already set is not allowed via needextends."

log_warning(
logger, error_msg, "needextend", location=location
)
return original_function(all_needs, extends, needs_config)


Expand Down
8 changes: 4 additions & 4 deletions src/extensions/score_metamodel/tests/rst/options/test_need_extends.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@
:expect: Error when extending need: stkh_req__test__need_extends_3. Append action is not allowed via needextends on 'string type options'


.. This will be activated once we have activated the c.this_doc() check aswell
.. #EXPECT[+2]: Potentially altering needs outside of the document is not allowed. Please add 'c.this_doc()' to the needextend to limit it to only needs in the same document
.. A needextend must explicitly be limited to needs in its own document.

.. .. needextend: id == 'stkh_req__test__need_extends_1'
.. :security: QM
.. needextend:: id == 'stkh_req__test__need_extends_1'
:security: QM
:expect: needextend in S-CORE must always be used per document only. Please add 'c.this_doc()' to the needextend to limit its effects to the correct document.
Loading
Loading