Skip to content

Fix OrderedNamespaceSet.__setitem__: int remove-before-add and slice exhausted iterator#517

Open
zrgt wants to merge 4 commits intoeclipse-basyx:developfrom
rwth-iat:fix/b5-ordered-namespaceset-setitem-int
Open

Fix OrderedNamespaceSet.__setitem__: int remove-before-add and slice exhausted iterator#517
zrgt wants to merge 4 commits intoeclipse-basyx:developfrom
rwth-iat:fix/b5-ordered-namespaceset-setitem-int

Conversation

@zrgt
Copy link
Copy Markdown
Contributor

@zrgt zrgt commented May 5, 2026

Fixes #498
Fixes #494

Changes

__setitem__ int (index replacement)

  • Remove old item from backends before adding new item — prevents false AASConstraintViolation when new item shares an attribute (e.g. id_short) with the item being replaced
  • Rollback: re-adds old item if the subsequent add fails

__setitem__ slice

  • Replace self._order[s] = new_items with self._order[s] = successful_new_itemsnew_items is an exhausted islice iterator; assigning it back to the list slice wrote an empty sequence, silently discarding all new items

Tests

  • test_ordered_namespaceset_int_setitem_preserves_index: builds [p0, old, p2], replaces index 1 with new (same id_short), asserts result is [p0, new, p2] — no index shift, no AASConstraintViolation
  • test_ordered_namespaceset_slice_setitem_preserves_order: replaces a 2-item slice, verifies new items appear at correct positions

zrgt added 2 commits May 5, 2026 17:12
slice branch stored exhausted islice iterator back into _order instead
of the materialized successful_new_items list, resulting in an empty
slice assignment that discarded all newly added items.

Fixes eclipse-basyx#494
…ng new

add-before-remove caused false AASConstraintViolation when new item
shares an attribute (e.g. id_short) with the item being replaced.
Fix removes old item first, rolls back if the subsequent add fails.

Fixes eclipse-basyx#498
@s-heppner
Copy link
Copy Markdown
Member

Doesn't the new (and old) implementation change the item's index? Doesn't __setitem__ usually keep the index?

I think the method __set_item__ should actually make use of Referable.update_from(), which makes sure they don't accidentally change immutable attributes.

Add test_ordered_namespaceset_int_setitem_preserves_index: build
[p0, old, p2], replace index 1 with new (same id_short as old), assert
result is [p0, new, p2] — no index shift and no AASConstraintViolation.

Addresses review feedback on eclipse-basyx#517
@zrgt
Copy link
Copy Markdown
Contributor Author

zrgt commented May 7, 2026

On index preservation: The index is preserved — self._order[s] = o is a list assignment that places the new item exactly at position s, leaving all surrounding elements unchanged. Added test_ordered_namespaceset_int_setitem_preserves_index to make this explicit: builds [p0, old, p2], replaces index 1 with new (same id_short as old), and asserts the result is [p0, new, p2].

On update_from(): Using update_from() would change the semantics — the caller passes a new object o intending it to be the item at index s, but update_from() mutates the existing object in place and discards o. If the caller holds a reference to o expecting it to be in the collection after ns[1] = o, they would be surprised. Standard Python __setitem__ semantics (e.g. list.__setitem__) replace the item at the index with the new object, which is what we implement here.

Both int and slice __setitem__ fixes now in one branch.
Keep both tests: test_ordered_namespaceset_int_setitem_preserves_index
and test_ordered_namespaceset_slice_setitem_preserves_order.

Closes eclipse-basyx#494
@zrgt zrgt changed the title Fix OrderedNamespaceSet.__setitem__ int: remove-before-add Fix OrderedNamespaceSet.__setitem__: int remove-before-add and slice exhausted iterator May 7, 2026
@zrgt
Copy link
Copy Markdown
Contributor Author

zrgt commented May 7, 2026

Merged #516 (slice fix: exhausted iterator) into this PR. Both int and slice fixes are now covered here. PR #516 has been closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants