Xml mdib #401

deichmab-draeger · 2024-11-20T17:44:09Z

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have updated the changelog accordingly.
I have added tests to cover my changes.

codecov · 2024-11-21T13:36:55Z

Codecov Report

Attention: Patch coverage is 91.12534% with 194 lines in your changes missing coverage. Please review.

Project coverage is 90.82%. Comparing base (463564c) to head (24d1c63).

Files with missing lines	Patch %	Lines
src/sdc11073/entity_mdib/entity_consumermdib.py	89.05%	44 Missing ⚠️
src/sdc11073/entity_mdib/entity_transactions.py	91.45%	34 Missing ⚠️
src/sdc11073/mdib/mdibbase.py	69.23%	28 Missing ⚠️
src/sdc11073/entity_mdib/entity_providermdib.py	92.36%	21 Missing ⚠️
src/sdc11073/roles/clockprovider.py	56.25%	14 Missing ⚠️
src/sdc11073/mdib/transactions.py	87.36%	12 Missing ⚠️
...rc/sdc11073/entity_mdib/entity_providermdibxtra.py	94.69%	7 Missing ⚠️
src/sdc11073/entity_mdib/entities.py	97.40%	6 Missing ⚠️
src/sdc11073/roles/alarmprovider.py	96.47%	6 Missing ⚠️
src/sdc11073/roles/audiopauseprovider.py	90.56%	5 Missing ⚠️
... and 9 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #401      +/-   ##
==========================================
- Coverage   90.84%   90.82%   -0.02%     
==========================================
  Files         116      125       +9     
  Lines       12963    14660    +1697     
==========================================
+ Hits        11776    13315    +1539     
- Misses       1187     1345     +158

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/sdc11073/entity_mdib/entities.py

leon1995 · 2024-11-21T11:06:24Z

src/sdc11073/entity_mdib/entities.py

+                f'State handle {state_handle} already exists in {self.__class__.__name__}, handle = {self.handle}')
+        cls = self._mdib.data_model.get_state_container_class(self.descriptor.STATE_QNAME)
+        state = cls(descriptor_container=self.descriptor)
+        state.Handle = state_handle or self.handle


handles should be unique. dont take the descriptor handle

If a context state in a SetContextState operation has Handle == DescriptorHandle, this tells the provider that it shall insert a new context state. The provider changes the Handle to something unique.
This code creates a context state that can directly be used as a new state for SetContextState.
Therefore I think the provided code is correct as it is.

but why does the entity need to know something about operations? shouldnt this just be a representation of data in the mdib?

leon1995 · 2024-11-21T11:14:41Z

src/sdc11073/entity_mdib/entities.py

+                 source: ProviderInternalEntity | ProviderInternalMultiStateEntity,
+                 mdib: EntityProviderMdib):
+        self._mdib = mdib
+        self.descriptor = copy.deepcopy(source.descriptor)


instead of copy use xml_utils.copy... methods. check other occurences

those copy methods only copy lxml nodes, not state/descriptor containers. I implemented a local copy method that creates a deep copy of the container, but without the node element.

use __copy__ or _deepcopy__ instead

src/sdc11073/entity_mdib/entity_consumermdib.py

…ity mdib

leon1995 · 2024-12-02T15:54:39Z

pyproject.toml

@@ -168,11 +168,21 @@ suppress-none-returning = true # https://docs.astral.sh/ruff/settings/#suppress-
 [tool.ruff.lint.flake8-comprehensions]
 allow-dict-calls-with-keyword-arguments = true  # https://docs.astral.sh/ruff/settings/#allow-dict-calls-with-keyword-arguments

+


remove empty line

leon1995 · 2024-12-05T08:32:51Z

src/sdc11073/entity_mdib/entities.py

+        xml_entity = self._mdib.internal_entities.get(self.handle)
+        if xml_entity is None:
+            raise ValueError('entity no longer exists in mdib')
+        if int(xml_entity.descriptor.get('DescriptorVersion', '0')) != self.descriptor.DescriptorVersion:


shouldnt this be < instead of !=? so you update only if there is a newer version?

leon1995 · 2024-12-05T08:37:17Z

src/sdc11073/entity_mdib/entities.py

+                 source: ProviderInternalEntity | ProviderInternalMultiStateEntity,
+                 mdib: EntityProviderMdib):
+        self._mdib = mdib
+        self.descriptor = copy.deepcopy(source.descriptor)


use __copy__ or _deepcopy__ instead

leon1995 · 2024-12-05T08:45:38Z

src/sdc11073/entity_mdib/entities.py

+                f'State handle {state_handle} already exists in {self.__class__.__name__}, handle = {self.handle}')
+        cls = self._mdib.data_model.get_state_container_class(self.descriptor.STATE_QNAME)
+        state = cls(descriptor_container=self.descriptor)
+        state.Handle = state_handle or self.handle


but why does the entity need to know something about operations? shouldnt this just be a representation of data in the mdib?

leon1995 · 2024-12-05T08:47:59Z

src/sdc11073/entity_mdib/entities.py

+        return ProviderMultiStateEntity(self, mdib)
+
+
+def _mk_copy(original: ContainerBase) -> ContainerBase:


typing should be

T = typing.TypeVar('T', bound=ContainerBase) def _mk_copy(original: T) -> T:

leon1995 · 2024-12-05T13:10:54Z

src/sdc11073/entity_mdib/entity_consumermdib.py

+            if tmp_child_node.tag in add_before_q_names:
+                tmp_child_node.addprevious(child_node)
+                return
+        raise RuntimeError('this should not happen')


please provide a more meaningful error message

leon1995 · 2024-12-05T13:11:10Z

src/sdc11073/entity_mdib/entity_consumermdib.py

+            else:
+                self._delete_entity(entity, handles)
+        return handles
+        # Todo: update self._get_mdib_response_node


leon1995 · 2024-12-05T13:16:10Z

src/sdc11073/entity_mdib/entity_consumermdib.py

+            xml_entity.states = state_nodes
+        elif len(state_nodes) != 1:
+            self.logger.error('update descriptor: Expect one state, got %d', len(state_nodes))
+            # Todo: what to do in this case? add entity without state?


leon1995 · 2024-12-05T13:43:24Z

src/sdc11073/entity_mdib/entity_providermdib.py

+        new_internal_entity = self._mdib.entity_factory(descriptor_container, [])
+        if handle in self._mdib.descr_handle_version_lookup:
+            # This handle existed before. Use last descriptor version + 1
+            new_internal_entity.descriptor.DescriptorVersion =  self._mdib.descr_handle_version_lookup[handle] + 1


duplicated whitespace

leon1995 · 2024-12-05T14:10:27Z

src/sdc11073/entity_mdib/entity_providermdib.py

+    def __init__(self,
+                 sdc_definitions: type[BaseDefinitions] | None = None,
+                 log_prefix: str | None = None,
+                 extra_functionality: type | None = None,


leon1995 · 2024-12-19T08:42:10Z

src/sdc11073/entity_mdib/entity_providermdib.py

+    from sdc11073.loghelper import LoggerAdapter
+    from sdc11073.mdib.descriptorcontainers import AbstractDescriptorContainer
+    from sdc11073.mdib.entityprotocol import ProviderEntityGetterProtocol
+    from sdc11073.mdib.mdibbase import MdibVersionGroup


unused import

leon1995 · 2024-12-19T10:14:54Z

src/sdc11073/entity_mdib/entities.py

+        cls = self._mdib.data_model.get_state_container_class(self.descriptor.STATE_QNAME)
+        state = cls(descriptor_container=self.descriptor)
+        state.Handle = state_handle or uuid.uuid4().hex
+        self.states[state.Handle] = state


state should not be added directly, but rather when the provider accepts the state via a notification

leon1995 · 2024-12-19T10:26:23Z

src/sdc11073/entity_mdib/entity_consumermdib.py

+        self._mdib = mdib
+
+    def by_handle(self, handle: str) -> ConsumerEntityType | None:
+        """Return entity with given handle."""


be more specific about the handle you are talking about. write descriptor handle to avoid confusion. check other occurrences

leon1995 · 2024-12-19T10:31:56Z

src/sdc11073/entity_mdib/entities.py

+        self.parent_handle = parent_handle
+        self.source_mds = source_mds
+        self.node_type = node_type  # name of descriptor type
+        self._descriptor = None


instead of None set self._descriptor=descriptor

leon1995 · 2024-12-19T11:21:53Z

src/sdc11073/entity_mdib/entities.py

+        handle = source.descriptor.get('Handle')
+        self.descriptor: AbstractDescriptorContainer = cls(handle, parent_handle=source.parent_handle)
+        self.descriptor.update_from_node(source.descriptor)
+        self.descriptor.set_source_mds(source.source_mds)


source.source_mds can be None but self.descriptor.set_source_mds expects type str

leon1995 · 2024-12-20T08:20:35Z

src/sdc11073/mdib/transactions.py

+            old_state = self._mdib.context_states.handle.get_one(handle, allow_none=True)
+            if state_container is None:
+                # a deleted state : this cannot be communicated via notification.
+                # delete it internal_entity anf that is all


leon1995 · 2024-12-20T08:34:56Z

src/sdc11073/mdib/transactionsprotocol.py

@@ -129,30 +232,44 @@ def mk_context_state(self, descriptor_handle: str,
        """Create a new ContextStateContainer."""


-class StateTransactionManagerProtocol(AbstractTransactionManagerProtocol):
-    """Interface of a TransactionManager that modifies states (except context states)."""
+class StateTransactionManagerProtocol(EntityStateTransactionManagerProtocol): # pragma: no cover


why pragma: no cover?

leon1995 · 2024-12-20T09:29:31Z

src/sdc11073/provider/subscriptionmgr_async.py

            soap_client_pool.async_loop_subscr_mgr = thr
            thr.start()
+            for _i in range(10):
+                if not thr.running:


instead of a bool you could use a threading.Event that is set if its running. Then you can use .wait to wait until its running so you dont need to do such an implementation

leon1995 · 2024-12-20T09:42:29Z

src/sdc11073/roles/alarmprovider.py

+        self._stop_worker.set()
+        self._worker_thread.join()
+
+    def _worker_thread_loop(self):


this method can be reduced to

while not self._stop_worker.wait(timeout=self.WORKER_THREAD_INTERVAL): self._update_alert_system_state_current_alerts()

leon1995 · 2024-12-20T09:54:54Z

tests/test_entity_client_device.py

+            raise
+        sys.stderr.write(f'############### tearDown {self._testMethodName} done ##############\n')
+
+    def add_random_patient(self, count: int = 1) -> [ProviderMultiStateEntity, list]:


deichmab-draeger added 18 commits August 15, 2024 18:33

first concept impl of xml based mdib

0637915

implemented description modification report

bb0aed4

Merge branch 'master' into xml_mdib

2ff9af0

all notifications implemented

cf299e4

typing stuff

c616bd7

reworked entity handling

1f3ac6a

use factory to create entities for flexibility

6876817

provider entity mdib works in most parts

7a32baa

renamings, xml_ prefix replaced with entity_ prefix; tests green

bf811a5

split AlarmProvider class into 3 separate ones for better clarity.

826b478

all tests green

f9d5557

moved state initialization from alarm role provider to mdib.

6110f31

cleanup code, add more protocols for better abstraction.

05ce740

manage version counters in write_entity

a864b20

Merge branch 'master' into xml_mdib

411b625

merge master

adbc6c4

fixed ruff stuff.

c5d5d80

fixed ruff findings

64ca8c1

deichmab-draeger requested a review from leon1995 November 20, 2024 17:44

deichmab-draeger added 2 commits November 21, 2024 10:54

fixed ruff findings in mockstuff.py

b63c2ec

fixed ruff findings in test_client_device.py

0107ce3

relaxed ruff rules for tests

cb0686e

leon1995 requested changes Nov 21, 2024

View reviewed changes

deichmab-draeger added 6 commits November 21, 2024 15:17

fixed ruff stuff

2e637a4

ruff findings, exclude protocol definitions from code coverage

e0cfc41

removed some obsolete code

ea66398

add by_ prefix to entity getter methods; fixed locking problem in ent…

2075091

…ity mdib

fixed ruff findings

85d62d8

fixed ruff findings

ce36701

review remarks; better copy method in entities.py

cdf5002

leon1995 self-requested a review December 2, 2024 15:54

review remark

24d1c63

leon1995 requested changes Dec 5, 2024

View reviewed changes

leon1995 self-requested a review December 17, 2024 12:22

leon1995 requested changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xml mdib #401

Xml mdib #401

deichmab-draeger commented Nov 20, 2024

codecov bot commented Nov 21, 2024 •

edited

Loading

leon1995 Nov 21, 2024

deichmab-draeger Nov 21, 2024

leon1995 Dec 5, 2024

leon1995 Nov 21, 2024

deichmab-draeger Nov 22, 2024 •

edited

Loading

leon1995 Dec 5, 2024

leon1995 Dec 2, 2024

leon1995 Dec 5, 2024

leon1995 Dec 5, 2024

leon1995 Dec 5, 2024

leon1995 Dec 5, 2024

leon1995 Dec 5, 2024

leon1995 Dec 5, 2024

leon1995 Dec 5, 2024

leon1995 Dec 5, 2024

leon1995 Dec 5, 2024

leon1995 Dec 19, 2024

leon1995 Dec 19, 2024

leon1995 Dec 19, 2024

leon1995 Dec 19, 2024

leon1995 Dec 19, 2024

leon1995 Dec 20, 2024

leon1995 Dec 20, 2024

leon1995 Dec 20, 2024

leon1995 Dec 20, 2024

leon1995 Dec 20, 2024

		@@ -168,11 +168,21 @@ suppress-none-returning = true # https://docs.astral.sh/ruff/settings/#suppress-
		[tool.ruff.lint.flake8-comprehensions]
		allow-dict-calls-with-keyword-arguments = true # https://docs.astral.sh/ruff/settings/#allow-dict-calls-with-keyword-arguments

		return ProviderMultiStateEntity(self, mdib)


		def _mk_copy(original: ContainerBase) -> ContainerBase:

Xml mdib #401

Are you sure you want to change the base?

Xml mdib #401

Conversation

deichmab-draeger commented Nov 20, 2024

Types of changes

Checklist:

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deichmab-draeger Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2024 •

edited

Loading

deichmab-draeger Nov 22, 2024 •

edited

Loading