model.base: Copy Qualifier and Extension data in update_nss_from by sankalpsthakur · Pull Request #624 · eclipse-basyx/basyx-python-sdk

sankalpsthakur · 2026-09-05T15:26:57Z

Matching Qualifier and Extension objects in
NamespaceSet.update_nss_from() were looked up and then left unchanged,
so a later copy of the same object silently kept the old value.
Referable already copies in place.

Qualifier.update_from() and Extension.update_from() now copy all
fields, including type and name, onto the object that already lives
in the target set. The type and name setters no longer count the
object itself as a duplicate, a KeyError raised while updating reaches
the caller instead of being read as a missing object, and the old
supplemental semantic IDs are cleared before semantic_id is replaced
(AASd-118).

Fixes #602

AI/LLM disclosure

AI coding tools assisted with the change and this description.

`NamespaceSet.update_nss_from()` is how backends refresh a namespace from a newer copy of the same objects. Matching Referable objects get their attributes copied in place. Matching Qualifier and Extension objects were only looked up; their value and the rest of the payload stayed on the old object. A PUT of a Submodel with the same qualifier type therefore kept the previous value. Backends already treat a Qualifier of the same type and an Extension of the same name as the same object, so those fields have to be copied onto the object that already lives in the target set. Fixes eclipse-basyx#602

`Extension.update_from()` and `Qualifier.update_from()` left `name` and `type` as they were, so they did not produce a full copy of the other version of the object. The setters of both attributes reject a value that is already present in the parent namespace, which includes the object's own current value. As `update_nss_from()` matches the objects by exactly that attribute, it is only assigned if it actually differs. `Extension.update_from()` also took over the `refers_to` set of the other object, so that both extensions shared the same set instance afterwards.

sankalpsthakur · 2026-10-12T01:42:29Z

@zrgt addressed in 0b16e07 (name copied, tests merged into the existing cases). Could you re-review?

zrgt

Reviewed both commits. The direction is right — #602 is a real bug and update_nss_from silently dropping qualifier/extension data needs fixing — but two defects in the new code should be addressed before merge, and the second commit's approach treats a symptom rather than the cause.

Blocking

update_from() is now called inside the except KeyError guard (line 2447). A KeyError from the type/name setter is read as "object not present", the element is removed from other and destroyed, and the caller gets an unrelated AASd-021. Reproduced below; develop handles the same input cleanly.
semantic_id is assigned before supplemental_semantic_id (lines 1972 and 1797), so any update that clears semantic_id raises AASd-118 and leaves the object half-updated. Reproduced below.

Approach

The if self.type != other.type / if self.name != other.name guards from the second commit are dead code for every namespace in the SDK (all keys are case-sensitive) and raise KeyError in the only case where they do execute. The cause is that Qualifier.type and Extension.name lack the no-op early return Referable._set_id_short has, and their duplicate check does not exclude self. Fixing the setters removes the need for both guards and makes renames work.

Tests cover neither bug: no differing type/name case, and no assertion on supplemental_semantic_id or value_id.

I also left one comment on the # Todo: Implement function including tests removal — four pre-existing defects remain in the removal loop (one of them a reproducible KeyError on any multi-key NamespaceSet), so removing the marker asserts more than currently holds.

Verified on the PR head: python -m unittest, ruff check, mypy basyx test and the Sphinx build with -n -W all pass; copyright years and commit format match CONTRIBUTING.md. Every repro above was run against the PR head and compared with develop.

Generated by Claude Code

zrgt · 2026-10-22T01:35:48Z

+                    extension.update_from(other_object)  # type: ignore
                else:
                    raise TypeError("Type not implemented")
            except KeyError:


Enabling update_from() here puts it inside this try, so a KeyError raised inside update_from is now misread as "object is not in NamespaceSet" — and the element gets destroyed.

class Q(model.Qualifiable): def __init__(self, values=()): super().__init__() self.set1 = model.NamespaceSet(self, [("type", False)], values) a = Q([model.Qualifier("type1", model.datatypes.Int, 1)]) b = Q([model.Qualifier("TYPE1", model.datatypes.Int, 99)]) a.set1.update_nss_from(b.set1)

The case-insensitive lookup matches, update_from runs self.type = other.type, and the type setter raises KeyError("Object with type 'TYPE1' is already present in the parent Namespace"). It is swallowed, the object lands in objects_to_add, other.remove(obj) succeeds and self.add(obj) raises:

AASConstraintViolation: Object with attribute (name='type', value='TYPE1') is already present in this set of objects (Constraint AASd-021) a.set1 = [('type1', 1)] # stale b.set1 = [] # qualifier gone

On develop the same input is a clean no-op. The try should cover only the backend lookup — use backend.get(...) and call update_from() outside it. Same applies to the pre-existing referable.update_from() path.

Generated by Claude Code

zrgt · 2026-10-22T01:35:48Z

+        self.value = other.value
+        self.value_id = other.value_id
+        self.kind = other.kind
+        self.semantic_id = other.semantic_id


Ordering bug: semantic_id is assigned before supplemental_semantic_id, so an update that clears semantic_id hits the AASd-118 guard in the setter (base.py:1660).

q = model.Qualifier("q", model.datatypes.String, "old", semantic_id=S, supplemental_semantic_id=[T]) model.Submodel("urn:sm").add_qualifier(q) q.update_from(model.Qualifier("q", model.datatypes.String, "new", semantic_id=None)) # AASConstraintViolation: semantic_id can not be removed while there is at least one # supplemental_semantic_id (Constraint AASd-118)

self.value is already "new" when it raises, so the object is left half-updated. Clear supplemental_semantic_id first, then assign semantic_id, then the new supplemental list. Same at line 1797 in Extension.update_from.

Generated by Claude Code

zrgt · 2026-10-22T01:35:48Z

+        :param other: The object to update from
+        """
+        # The type setter rejects the current type as a duplicate, so only assign a changed type
+        if self.type != other.type:


This guard is dead code in the SDK, and broken in the one case where it does run.

Every production set is case-sensitive (submodel.py:95,96,176,178 use ("type", True) / ("name", True)) and update_nss_from looks the object up by exactly that key, so the found qualifier's type always equals other.type — the branch can never be taken. In a case-insensitive set it is taken, and raises KeyError (see my comment on line 2447).

The actual defect is in the setter: Qualifier.type (line 1936) and Extension.name lack the no-op short-circuit that Referable._set_id_short has —

if id_short == self.id_short: return

— and their set_.contains_id("type", type_) check does not exclude self. Fixing the setters makes renames work and lets both guards go away.

Generated by Claude Code

zrgt · 2026-10-22T01:35:48Z

+
+        :param other: The object to update from
+        """
+        # The name setter rejects the current name as a duplicate, so only assign a changed name


This comment states an intentional contract where there is a setter bug: the setter does not "reject the current name", it simply fails to exclude self from set_.contains_id("name", name). Contrast Referable._set_id_short, which returns early on a no-op.

Worth rewording (same text at line 1964 for type), otherwise whoever fixes the setters later will not notice these two guards become dead.

Generated by Claude Code

zrgt · 2026-10-22T01:35:48Z

+        Internal function to update the object's attributes from a different version of the exact same object.
+
+        This function should not be used directly. It is typically used by backend implementations (database adapters,
+        protocol clients, etc.) to update the object's data, after ``update_nss_from()`` has been called.


"after update_nss_from() has been called" inverts the call direction: update_nss_from (line 2409) is the only caller of this method (lines 2436 and 2444), so it runs during update_nss_from, not after it.

The sentence is copied from Referable.update_from, where it is equally wrong, but these are new Sphinx-published docstrings — worth getting right here. Same text at line 1960.

Generated by Claude Code

zrgt · 2026-10-22T01:35:48Z

        # Redundant to the line above. However, this way, we make sure that we really update the _name
        self._name = name

+    def update_from(self, other: "Extension") -> None:


Extension.update_from and Qualifier.update_from share four verbatim lines (value_type, value, semantic_id, supplemental_semantic_id), identical comments included. The AASd-118 ordering fix has to be applied in both places and will drift.

HasSemantics is their common base — a HasSemantics.update_from copying semantic_id/supplemental_semantic_id, overridden to add the subtype-specific fields, removes the duplication and also covers other HasSemantics members of NamespaceSets, which get no implementation at all today.

Generated by Claude Code

zrgt · 2026-10-22T01:35:48Z

-                    # extension.update_from(other_object) # TODO: What should happend here? Remove noqa when done
+                    extension.update_from(other_object)  # type: ignore
                else:
                    raise TypeError("Type not implemented")


_NSO is bound to Union[Referable, Qualifier, HasSemantics, Extension] (line 571), and UniqueSemanticIdNamespace._get_object(HasSemantics, "semantic_id", ...) treats sets as holding bare HasSemantics — so this raises TypeError("Type not implemented") for a legitimate member type, aborting Referable.update_from on its owner.

Not introduced by this PR, but the removed # Todo: Implement function including tests marker was covering it. Either handle the generic HasSemantics case or narrow _NSO.

Generated by Claude Code

zrgt · 2026-10-22T01:35:48Z

        )

-    # Todo: Implement function including tests
    def update_nss_from(self, other: "NamespaceSet"):


Dropping the # Todo: Implement function including tests marker asserts this function is complete and tested, but four defects remain in the removal loop below (all pre-existing, none introduced here):

Line 2461 — item appended once per backend key, then removed twice. Reproduced on this branch and on develop:

ns = NamespaceSet(self, [("id_short", False), ("semantic_id", True)], [Property("Prop1", Int, semantic_id=S)]) ns.update_nss_from(empty_set) # KeyError: 'PROP1'

Both keys match, Prop1 is appended to objects_to_remove twice, and the second self.remove(item) raises. Dedup objects_to_remove or break after the first matching key.

Line 2458 — case_sensitive_other is unpacked at 2453 and never used. The backend_other lookup uses self's flag. With self keyed ("type", True) and other keyed ("type", False), every lookup misses and every element of self is deleted. ruff does not catch it because it is a tuple-unpacking target.

Line 2455 — if attr_name is attr_name_other compares dict-key strings by identity. It works only because all current call sites pass interned literals; a dynamically built attribute name makes the whole removal pass silently never run.

Line 2457 — if not backend_other.get(...) tests the found element's truthiness instead of is None, so any member with a falsy __bool__/__len__ is deleted although it exists in other. Latent today, but is None is what is meant.

Suggestion: either fix these in this PR, or keep a narrower Todo so the gap stays visible.

Generated by Claude Code

zrgt · 2026-10-22T01:35:48Z

+        # ExampleNamespaceReferable also indexes semantic_id, so Qualifier/Extension
+        # objects belong in namespaces that only key on type/name.
+        qualifier_ns1 = self._namespace_class_qualifier()
+        qualifier1 = model.Qualifier(


These tests do not cover either of the two bugs above.

Every qualifier/extension pair here already has an identical type/name, so the if self.type != other.type branch (line 1965) and if self.name != other.name (1790) have zero coverage — which is why they ship green despite raising KeyError when taken.

self.supplemental_semantic_id = other.supplemental_semantic_id (1797/1973) and self.value_id = other.value_id (1970) are never asserted, which is why the AASd-118 ordering defect ships green.

A case with Qualifier(semantic_id=S, supplemental_semantic_id=[T]) updated from a qualifier with semantic_id=None, and one with a genuinely differing type/name, both fail today.

Generated by Claude Code

zrgt · 2026-10-22T01:35:48Z

        self.set1 = model.NamespaceSet(self, [("type", False)], values)


+class ExampleNamespaceExtension(model.HasExtension):


ExampleNamespaceExtension is instantiated directly in the test body rather than exposed as _namespace_class_extension, the way _namespace_class_qualifier already is.

ModelOrderedNamespaceTest overrides only _namespace_class, so the inherited run replays the same ~80 lines of qualifier/extension assertions against plain NamespaceSets a second time — runtime spent for no extra coverage, while OrderedNamespaceSet qualifier/extension behaviour stays untested. Adding _namespace_class_extension and overriding both in the ordered subclass follows the existing pattern.

Generated by Claude Code

`NamespaceSet.update_nss_from()` called `update_from()` of the matching object inside the `try` block that reads a `KeyError` as "the object is not in this set". A `KeyError` raised by `update_from()` itself was therefore taken for a missing object: the other object was removed from its set and added a second time, the caller got an unrelated AASd-021 (or -022, -077) violation, and the object in the other set was gone. An error raised while updating a matching object has to reach the caller and leave both sets as they were, so only the lookup is guarded. The Todo above `update_nss_from()` is back in a narrower form. The removal of objects that are missing in the other set still has known defects, which are independent of eclipse-basyx#602.

`Qualifier.update_from()` and `Extension.update_from()` assigned the `semantic_id` before the `supplemental_semantic_id`. An update from an object without a `semantic_id` therefore ran into AASd-118 (the semantic ID must not be removed while there are supplemental ones) and stopped half way through, after other attributes had already been copied. The old supplemental semantic IDs have to go before the semantic ID is replaced, and the new ones are set last. Both classes share that order in `HasSemantics._update_semantic_ids_from()`, so it cannot drift apart. It is private on purpose: `Referable` precedes `HasSemantics` in the MRO of every class that is both, so a public `update_from()` on `HasSemantics` would never run for them, and `update_nss_from()` has no branch for plain `HasSemantics` objects. The docstrings said that `update_from()` is used after `update_nss_from()` has been called. It is called by `update_nss_from()` for the objects that both sets have.

The `Qualifier.type` and `Extension.name` setters raise a `KeyError` if the value is already present in the parent Namespace. The object itself counts as present, so the setters rejected the value the object already had and, in a NamespaceSet that ignores the case, a rename that only changes the case. `Referable.id_short` returns early for its current value, these two did not. `update_from()` worked around that by assigning `type` and `name` only if they differ. That check never ran for the NamespaceSets of the SDK, which all match case-sensitively, and raised a `KeyError` in the one case where it did run. The setters now leave the current value alone and compare a duplicate with the object itself, so `update_from()` assigns `type` and `name` unconditionally. A value that belongs to another object is still rejected, and since that is the first step of `update_from()`, nothing else is changed in that case.

sankalpsthakur · 2026-10-24T19:47:49Z

addressed all review comments and added tests, ptal

zrgt self-requested a review October 6, 2026 13:19

zrgt requested changes Oct 7, 2026

View reviewed changes

zrgt reviewed Oct 22, 2026

View reviewed changes

sankalpsthakur added 3 commits October 22, 2026 19:03

sankalpsthakur force-pushed the fix/602-namespaceset-update-nss branch from 39cbfe5 to 2a37c9f Compare October 26, 2026 20:31

		self.set1 = model.NamespaceSet(self, [("type", False)], values)


		class ExampleNamespaceExtension(model.HasExtension):

Conversation

sankalpsthakur commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI/LLM disclosure

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sankalpsthakur commented Oct 12, 2026

Uh oh!

zrgt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sankalpsthakur commented Oct 24, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sankalpsthakur commented Sep 5, 2026 •

edited

Loading