diff --git a/changelog/1248.changed.md b/changelog/1248.changed.md new file mode 100644 index 000000000..e102a1ab3 --- /dev/null +++ b/changelog/1248.changed.md @@ -0,0 +1,5 @@ +**Potentially breaking change:** setting a read-only attribute now raises `ReadOnlyAttributeError` instead of silently discarding the value. + +Previously, assigning to a read-only attribute (for example `node.some_readonly_attr.value = "x"`) was accepted and then quietly dropped from the mutation at save time, so the change never reached the server and no error was reported. That assignment now raises `ReadOnlyAttributeError` immediately. Any existing workflow that writes to a read-only attribute and relies on the old silent no-op will now fail and needs to stop setting that attribute. + +Reading and re-querying read-only values are unaffected: values loaded from the API continue to populate normally. Relatedly, `infrahubctl object update` now rejects a read-only field passed via `--set` with a clear error instead of silently ignoring it. diff --git a/infrahub_sdk/ctl/object/update.py b/infrahub_sdk/ctl/object/update.py index cb4c79d7f..64a3e311a 100644 --- a/infrahub_sdk/ctl/object/update.py +++ b/infrahub_sdk/ctl/object/update.py @@ -105,6 +105,11 @@ async def _update_with_set_args( rel_names = schema.relationship_names validate_set_fields(data, attr_names, rel_names) + read_only_keys = sorted(key for key in data if key in attr_names and schema.get_attribute(key).read_only) + if read_only_keys: + console.print(f"[red]Error: cannot update read-only field(s): {', '.join(read_only_keys)}.") + raise typer.Exit(code=1) + node = await resolve_node(client, kind, identifier, schema=schema, branch=branch) prepared = prepare_relationship_data(data, schema) diff --git a/infrahub_sdk/exceptions.py b/infrahub_sdk/exceptions.py index 02111b9ac..5be1ac373 100644 --- a/infrahub_sdk/exceptions.py +++ b/infrahub_sdk/exceptions.py @@ -130,6 +130,15 @@ def __init__(self, message: str | None = None) -> None: super().__init__(self.message) +class ReadOnlyAttributeError(Error): + """Raised when attempting to modify the value of a read-only attribute.""" + + def __init__(self, name: str, message: str | None = None) -> None: + self.name = name + self.message = message or f"The attribute '{name}' is read-only and cannot be modified." + super().__init__(self.message) + + class ResourceNotDefinedError(Error): """Raised when trying to access a resource that hasn't been defined.""" diff --git a/infrahub_sdk/node/attribute.py b/infrahub_sdk/node/attribute.py index 4ada6b9de..653d6cc0b 100644 --- a/infrahub_sdk/node/attribute.py +++ b/infrahub_sdk/node/attribute.py @@ -4,6 +4,7 @@ from collections.abc import Callable from typing import TYPE_CHECKING, Any, NamedTuple, get_args +from ..exceptions import ReadOnlyAttributeError from ..uuidt import UUIDT from .constants import ( ATTRIBUTE_METADATA_OBJECT, @@ -99,8 +100,6 @@ def __init__(self, name: str, schema: AttributeSchemaAPI, data: Any | dict) -> N self._properties_object = PROPERTIES_OBJECT self._properties = self._properties_flag + self._properties_object - self._read_only = ["updated_at", "is_inherited"] - self.id: str | None = data.get("id") self._value: Any | None = data.get("value") @@ -140,6 +139,14 @@ def value(self) -> Any: @value.setter def value(self, value: Any) -> None: + # Read-only attributes are populated from the API (via the constructor and + # _set_value), but users must not change them; loading and re-querying keep working. + if self._schema.read_only: + raise ReadOnlyAttributeError(name=self.name) + self._set_value(value) + + def _set_value(self, value: Any) -> None: + """Set the value bypassing the read-only guard, for internal population.""" self._value = value self.value_has_been_mutated = True diff --git a/infrahub_sdk/node/node.py b/infrahub_sdk/node/node.py index df1ec65b5..1c458aeee 100644 --- a/infrahub_sdk/node/node.py +++ b/infrahub_sdk/node/node.py @@ -1606,7 +1606,7 @@ async def _process_mutation_result( continue # Process allocated resource from a pool and update attribute - attr.value = object_response[attr_name]["value"] + attr._set_value(object_response[attr_name]["value"]) for rel_name in self._relationships: rel = getattr(self, rel_name) @@ -2837,7 +2837,7 @@ def _process_mutation_result( continue # Process allocated resource from a pool and update attribute - attr.value = object_response[attr_name]["value"] + attr._set_value(object_response[attr_name]["value"]) for rel_name in self._relationships: rel = getattr(self, rel_name) diff --git a/tests/unit/ctl/object/test_update.py b/tests/unit/ctl/object/test_update.py index 7082ec045..8060a7a78 100644 --- a/tests/unit/ctl/object/test_update.py +++ b/tests/unit/ctl/object/test_update.py @@ -2,6 +2,7 @@ from __future__ import annotations +from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -39,6 +40,7 @@ def test_update_with_set_args() -> None: mock_schema = MagicMock() mock_schema.attribute_names = ["name", "description"] mock_schema.relationship_names = [] + mock_schema.get_attribute = lambda _name: SimpleNamespace(read_only=False) mock_attr = MagicMock() mock_attr.value = "old-name" @@ -81,6 +83,7 @@ def test_update_with_set_args_attribute_applied() -> None: mock_schema = MagicMock() mock_schema.attribute_names = ["description"] mock_schema.relationship_names = [] + mock_schema.get_attribute = lambda _name: SimpleNamespace(read_only=False) mock_attr = MagicMock() mock_attr.value = "old description" @@ -118,6 +121,7 @@ def test_update_with_set_args_and_branch() -> None: mock_schema = MagicMock() mock_schema.attribute_names = ["name"] mock_schema.relationship_names = [] + mock_schema.get_attribute = lambda _name: SimpleNamespace(read_only=False) mock_attr = MagicMock() mock_attr.value = "old" @@ -168,6 +172,31 @@ def test_update_invalid_field() -> None: assert result.exit_code != 0 +def test_update_read_only_field_rejected() -> None: + """Using --set on a read-only attribute exits non-zero before resolving the node.""" + mock_schema = MagicMock() + mock_schema.attribute_names = ["computed_address"] + mock_schema.relationship_names = [] + mock_schema.get_attribute = lambda _name: SimpleNamespace(read_only=True) + + mock_client = MagicMock() + mock_client.schema = MagicMock() + mock_client.schema.get = AsyncMock(return_value=mock_schema) + + mock_resolve = AsyncMock() + with ( + patch("infrahub_sdk.ctl.object.update.initialize_client", return_value=mock_client), + patch("infrahub_sdk.ctl.object.update.resolve_node", mock_resolve), + ): + result = runner.invoke(app, ["object", "update", "InfraDevice", "abc-123", "--set", "computed_address=x"]) + + assert result.exit_code != 0 + assert "read-only" in result.stdout + assert "computed_address" in result.stdout + # rejected during validation, before the node is fetched or mutated + mock_resolve.assert_not_awaited() + + def test_update_with_file() -> None: """``object update`` with --file delegates to ObjectFile and prints a confirmation.""" mock_file = MagicMock() @@ -256,6 +285,7 @@ def test_update_with_set_args_attribute_noop() -> None: mock_schema = MagicMock() mock_schema.attribute_names = ["description"] mock_schema.relationship_names = [] + mock_schema.get_attribute = lambda _name: SimpleNamespace(read_only=False) mock_attr = MagicMock() mock_attr.value = "same value" diff --git a/tests/unit/sdk/test_file_object.py b/tests/unit/sdk/test_file_object.py index b64d9f093..5c97aaf29 100644 --- a/tests/unit/sdk/test_file_object.py +++ b/tests/unit/sdk/test_file_object.py @@ -20,6 +20,30 @@ FILE_MIME_TYPE = "application/pdf" +def _fetched_file_node( + clients: BothClients, + client_type: str, + schema: NodeSchemaAPI, + *, + checksum: str, + node_id: str | None = "node-1", + existing: bool = False, +) -> InfrahubNode | InfrahubNodeSync: + """Build a file node the way a server fetch delivers it. + + ``checksum`` is read-only, so it is populated through the constructor payload - + the same path a real query uses - rather than assigned, which would raise. + """ + client = getattr(clients, client_type) + node_cls = InfrahubNode if client_type == "standard" else InfrahubNodeSync + node = node_cls(client=client, schema=schema, branch="main", data={"checksum": {"value": checksum}}) + if node_id is not None: + node.id = node_id + if existing: + node._existing = True + return node + + @pytest.fixture def mock_node_create_with_file(httpx_mock: HTTPXMock) -> HTTPXMock: """Mock the HTTP response for node create with file upload.""" @@ -313,13 +337,7 @@ async def test_bytes_match(self, client_type: str, clients: BothClients, file_ob payload = b"matching content" digest = hashlib.sha1(payload, usedforsecurity=False).hexdigest() - client = getattr(clients, client_type) - if client_type == "standard": - node = InfrahubNode(client=client, schema=file_object_schema, branch="main") - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "node-1" - node.checksum.value = digest # type: ignore[attr-defined] + node = _fetched_file_node(clients, client_type, file_object_schema, checksum=digest) if isinstance(node, InfrahubNode): assert await node.matches_local_checksum(payload) is True @@ -329,13 +347,7 @@ async def test_bytes_match(self, client_type: str, clients: BothClients, file_ob async def test_bytes_differ( self, client_type: str, clients: BothClients, file_object_schema: NodeSchemaAPI ) -> None: - client = getattr(clients, client_type) - if client_type == "standard": - node = InfrahubNode(client=client, schema=file_object_schema, branch="main") - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "node-1" - node.checksum.value = "different-digest" # type: ignore[attr-defined] + node = _fetched_file_node(clients, client_type, file_object_schema, checksum="different-digest") if isinstance(node, InfrahubNode): assert await node.matches_local_checksum(b"hello world") is False @@ -354,13 +366,7 @@ async def test_path_source( target.write_bytes(payload) digest = hashlib.sha1(payload, usedforsecurity=False).hexdigest() - client = getattr(clients, client_type) - if client_type == "standard": - node = InfrahubNode(client=client, schema=file_object_schema, branch="main") - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "node-1" - node.checksum.value = digest # type: ignore[attr-defined] + node = _fetched_file_node(clients, client_type, file_object_schema, checksum=digest) if isinstance(node, InfrahubNode): assert await node.matches_local_checksum(target) is True @@ -420,14 +426,9 @@ async def test_skips_when_checksum_matches( payload = b"unchanged content" digest = hashlib.sha1(payload, usedforsecurity=False).hexdigest() - client = getattr(clients, client_type) - if client_type == "standard": - node = InfrahubNode(client=client, schema=file_object_schema, branch="main") - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "already-on-server" - node._existing = True - node.checksum.value = digest # type: ignore[attr-defined, union-attr] + node = _fetched_file_node( + clients, client_type, file_object_schema, checksum=digest, node_id="already-on-server", existing=True + ) if isinstance(node, InfrahubNode): result = await node.upload_if_changed(source=payload, name="f.bin") @@ -450,14 +451,14 @@ async def test_uploads_when_checksum_differs( new_content = b"new content" expected_digest = hashlib.sha1(new_content, usedforsecurity=False).hexdigest() - client = getattr(clients, client_type) - if client_type == "standard": - node = InfrahubNode(client=client, schema=file_object_schema, branch="main") - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "existing-file-node-456" - node._existing = True - node.checksum.value = "old-server-digest" # type: ignore[attr-defined, union-attr] + node = _fetched_file_node( + clients, + client_type, + file_object_schema, + checksum="old-server-digest", + node_id="existing-file-node-456", + existing=True, + ) if isinstance(node, InfrahubNode): result = await node.upload_if_changed(source=new_content, name="f.bin") @@ -513,14 +514,14 @@ async def test_derives_name_from_path( target = tmp_path / "derived-name.bin" target.write_bytes(b"content") - client = getattr(clients, client_type) - if client_type == "standard": - node = InfrahubNode(client=client, schema=file_object_schema, branch="main") - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "existing-file-node-456" - node._existing = True - node.checksum.value = "old-server-digest" # type: ignore[attr-defined, union-attr] + node = _fetched_file_node( + clients, + client_type, + file_object_schema, + checksum="old-server-digest", + node_id="existing-file-node-456", + existing=True, + ) # No explicit name — should derive from target.name internally. if isinstance(node, InfrahubNode): @@ -536,13 +537,7 @@ async def test_requires_name_for_bytes( clients: BothClients, file_object_schema: NodeSchemaAPI, ) -> None: - client = getattr(clients, client_type) - if client_type == "standard": - node = InfrahubNode(client=client, schema=file_object_schema, branch="main") - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "some-id" - node.checksum.value = "x" # type: ignore[attr-defined, union-attr] + node = _fetched_file_node(clients, client_type, file_object_schema, checksum="x", node_id="some-id") if isinstance(node, InfrahubNode): with pytest.raises(ValueError, match=r"name is required"): @@ -592,15 +587,7 @@ async def test_skip_when_local_matches( dest = tmp_path / "local.bin" dest.write_bytes(payload) - client = getattr(clients, client_type) - if client_type == "standard": - node: InfrahubNode | InfrahubNodeSync = InfrahubNode( - client=client, schema=file_object_schema, branch="main" - ) - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "file-node-skip" - node.checksum.value = digest # type: ignore[attr-defined, union-attr] + node = _fetched_file_node(clients, client_type, file_object_schema, checksum=digest, node_id="file-node-skip") if isinstance(node, InfrahubNode): bytes_written = await node.download_file(dest=dest, skip_if_unchanged=True) @@ -623,15 +610,14 @@ async def test_downloads_when_local_differs( dest = tmp_path / "local.bin" dest.write_bytes(b"stale content") # different from FILE_CONTENT - client = getattr(clients, client_type) - if client_type == "standard": - node: InfrahubNode | InfrahubNodeSync = InfrahubNode( - client=client, schema=file_object_schema, branch="main" - ) - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "file-node-stream" # id matches mock_download_file_to_disk - node.checksum.value = "server-digest-different-from-local" # type: ignore[attr-defined, union-attr] + # id matches mock_download_file_to_disk + node = _fetched_file_node( + clients, + client_type, + file_object_schema, + checksum="server-digest-different-from-local", + node_id="file-node-stream", + ) if isinstance(node, InfrahubNode): bytes_written = await node.download_file(dest=dest, skip_if_unchanged=True) @@ -659,15 +645,9 @@ async def test_downloads_when_dest_missing( dest = tmp_path / "missing.bin" # does not exist assert not dest.exists() - client = getattr(clients, client_type) - if client_type == "standard": - node: InfrahubNode | InfrahubNodeSync = InfrahubNode( - client=client, schema=file_object_schema, branch="main" - ) - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "file-node-stream" - node.checksum.value = "any-digest" # type: ignore[attr-defined, union-attr] + node = _fetched_file_node( + clients, client_type, file_object_schema, checksum="any-digest", node_id="file-node-stream" + ) if isinstance(node, InfrahubNode): bytes_written = await node.download_file(dest=dest, skip_if_unchanged=True) @@ -683,15 +663,9 @@ async def test_raises_when_skip_without_dest( clients: BothClients, file_object_schema: NodeSchemaAPI, ) -> None: - client = getattr(clients, client_type) - if client_type == "standard": - node: InfrahubNode | InfrahubNodeSync = InfrahubNode( - client=client, schema=file_object_schema, branch="main" - ) - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") - node.id = "file-node-1" - node.checksum.value = "any-digest" # type: ignore[attr-defined, union-attr] + node = _fetched_file_node( + clients, client_type, file_object_schema, checksum="any-digest", node_id="file-node-1" + ) with pytest.raises(ValueError, match=r"skip_if_unchanged requires dest"): if isinstance(node, InfrahubNode): @@ -739,15 +713,8 @@ async def test_skip_raises_for_unsaved_node( dest = tmp_path / "local.bin" dest.write_bytes(payload) - client = getattr(clients, client_type) - if client_type == "standard": - node: InfrahubNode | InfrahubNodeSync = InfrahubNode( - client=client, schema=file_object_schema, branch="main" - ) - else: - node = InfrahubNodeSync(client=client, schema=file_object_schema, branch="main") # Do NOT set node.id — unsaved. - node.checksum.value = digest # type: ignore[attr-defined, union-attr] + node = _fetched_file_node(clients, client_type, file_object_schema, checksum=digest, node_id=None) with pytest.raises(ValueError, match=r"hasn't been saved yet"): if isinstance(node, InfrahubNode): diff --git a/tests/unit/sdk/test_node.py b/tests/unit/sdk/test_node.py index d8c735637..df946d9bc 100644 --- a/tests/unit/sdk/test_node.py +++ b/tests/unit/sdk/test_node.py @@ -10,7 +10,7 @@ import pytest -from infrahub_sdk.exceptions import FeatureNotSupportedError, NodeNotFoundError +from infrahub_sdk.exceptions import FeatureNotSupportedError, NodeNotFoundError, ReadOnlyAttributeError from infrahub_sdk.node import ( InfrahubNode, InfrahubNodeBase, @@ -2312,8 +2312,25 @@ async def test_read_only_attr( "postal_code": {"is_protected": False, "value": "123ABC"}, }, } + # read-only value is loaded and readable assert address.computed_address.value == "1234 Fake Street 123ABC" + # users cannot change a read-only attribute, via the node or the attribute + with pytest.raises(ReadOnlyAttributeError, match="'computed_address' is read-only"): + address.computed_address = "somewhere else" + with pytest.raises(ReadOnlyAttributeError, match="'computed_address' is read-only"): + address.computed_address.value = "somewhere else" + + # the rejected assignment left the loaded value untouched + assert address.computed_address.value == "1234 Fake Street 123ABC" + + # re-loading the same payload (as a fresh query would) re-populates the read-only value + if client_type == "standard": + reloaded = InfrahubNode(client=client, schema=address_schema, data=address_data) + else: + reloaded = InfrahubNodeSync(client=client, schema=address_schema, data=address_data) + assert reloaded.computed_address.value == "1234 Fake Street 123ABC" + @pytest.mark.parametrize("client_type", client_types) async def test_relationships_excluded_input_data(