AbstractRegistry returns reference instead of copy

[triller-telekom]

This commit introduced more checks in this line, i.e. everything behind the elements != null

This leads to a problem with tests such as the one in PR #4008. I think this test method updates and item in the way it is designed. It creates an item, puts it into the registry, fetches it, updates it and while doing so it is expected to create an ItemUpdatedEvent. However due to the above change the event is not fired anymore.

The problem is here. The AbstractRegistry returns a reference to the element instead of a copy. By returning a reference the setLabel on the fetched item changes the item within the registry already, so if the update call happens the newly introduced check elements.contains(oldElement) fails. I think this check is alright.

We should discuss how to deal with this problem, because I think its bad if a registry returns a reference to an inner object so it can be manipulated from the outside without the knowledge of the registry.

The RuleRegistry is returning a copy of a rule in the get method already.

Long story short: The new (ignored) test in PR #4008 should work! :-)

[triller-telekom]

How about implementing a clone() method on every element which lands in the AbstractRegistry, so the get() method can return a copy of the requested object?

Currently all elements which are put inside the AbstractRegistry are Identifieables. So we might need something like an ClonableIdentifieable interface which extends Identifieable?

@maggu2810 @kaikreuzer @htreu @sjka WDYT?

[sjsf]

The new (ignored) test in PR #4008 should work! :-)

I'm not so sure about this - so far, the assumption simply was that the registries return the original entities. Sure, that's because historically there was nothing seriously to be changed anyway.

Still, I'm not convinced that creating copies all over the place is the best idea, considering the crappy hardware that solutions are running on...

[kaikreuzer]

FTR: I also mentioned here that we have different ways to handle it and will need to think about a common solution. https://github.com/eclipse/smarthome/pull/3405 is also related to it.

[maggu2810]

I agree that generating copies in general is not a good thing. But changing the internal hold item and break the update mechanism needs to be solved. A clone method that is used optionally (e.g. different get methods or by the caller itself) could solve it, but don't know if it is the best approach.

[htreu]

But changing the internal hold item and break the update mechanism needs to be solved.

From the looks of the API I always thought to have immutable objects which could only be updated through the update mechanism. To me immutable objects seem to be the solution for not having copies and forbid mutations w/o the knowledge of the registry.

[kaikreuzer]

@htreu In general I would agree, but check the use case in the unit test: If you want to edit an item, you get it, you modify it and you call update on the registry. If it were immutable, you would for the clients to find some way to create an exact copy.

So as a minimum, we would have to provide a "clone()" method on all entities for that solution as well. But additionally, we would have to introduce new interfaces that only have getters and no setters (for Item vs. ActiveItem this is actually the case as that was its original intention - but I fear for Things and Modules we won't be able to easily introduce this).

Although I do not like it, I currently see no way around always returning copies. Any better suggestions are welcome!

[sjsf]

So as a minimum, we would have to provide a "clone()" method on all entities for that solution as well.

...or a builder which takes an entity, has setters to alter it and in the end spits out the modified entity as a new instance. Pretty much like we have it for Things (except that it is lacking a static fromThing(Thing) method). What a pity that exactly they also have setters in their interface 😞

[hsudbrock]

As the ItemRegistry is currently implemented, if one thread changes an item, then other threads who also retrieved that item from the registry will see the change immediately.

If the ItemRegistry would return clones, then other threads would not only not see those changes immediately, they would even not see the changes after an update of the item in the registry.

I think that closely related to the question whether the registries should return references/copies/whateverElse is the question whether updates of an element in the registry should be automatically propagated to all instances of that element that the registry has given out before. And, if that is not the case, how to deal with concurrent updates to that element (should the last of two concurrent updates win? does some kind of optimistic locking strategy makes sense, where the first of two concurrent updates wins and the last only sees an exception? ...).

Is there already some kind of policy which variant registries should implement?

[kaikreuzer]

I had a discussion with @sjka today (see also https://github.com/eclipse/smarthome/pull/4468#issuecomment-376547458) and we think it would be much nicer not to work with copies, but rather return immutable instances instead where possible. The Thing infrastructure would actually break if we introduced copies as the ThingManager would not be able to update status and handler information on Things. Ideally, we should not have any setters on Things and Items, but we will probably have a hard time to remove them without breaking to much - but we at least can ensure that those are not used in a way they are not supposed to.