Increase mutability safety by cloning objects

[KacperKluka]

Due to our current API design, it is possible to create a configuration object (e.g. ClientOptions), pass it to the SDK and modify it later. This is not ideal as those configuration changes while the SDK is running might lead to unexpected behaviours. The same issue is present when the SDK returns a reference to an object which is used internally and the returned reference allows for modifications (e.g. LocalDevice).

To mitigate those issues, we can create and use clones (copies) of such troublesome objects. This will mean that they will become immutable by SDK users after a certain execution point has been reached. For example, we might clone the ClientOptions object once it is passed to the AblyRest constructor and use only the cloned object. In this case, any further changes to the original object won't affect our SDK configuration. The same goes for "output" objects, instead of returning the original object we should create a copy of it and return it instead. Then, even if a user modifies the copy, the original object will be protected and nothing bad will happen.

The aim of this issue is to find all places in which this cloning can improve safety and implement it there. We should be careful and see if we need to do deep copies (e.g. if an object holds a reference to another object).

┆Issue is synchronized with this Jira Task by Unito

[qsdigor]

There are different techniques to do object cloning. We should discuss what is best for us.

1. Use clone() if the class implements Cloneable. This API is a bit flawed in java.

2. Create a clone manually. If there is a constructor that accepts all parameters, it might be simple, e.g new User( user.ID, user.Age, ... ). You might even want a constructor that takes a User: new User( anotherUser ).

3. Implement something to copy from/to a user. Instead of using a constructor, the class may have a method copy( User ). You can then first snapshot the object backupUser.copy( user ) and then restore it user.copy( backupUser ).

4. Use some library to do deep cloning like Cloning or Apache SerializeUtil

My suggestion is 3 or 4.

[KacperKluka]

Thank you for the suggestions :bow: IMO the best choice would be either 2 or modified 3 :thinking: I wouldn't go with 4 as this makes us reliant on a third-party library. Option 1 as you've mentioned is flawed so it isn't acceptable as well.

The cloning should not affect the public API of the SDK and adding a method to those classes like copy() will modify the public API and we will have to maintain them in the future. So if you would prefer to not use the constructors maybe let's create some private static cloning methods that would clone the objects for us but will be hidden from the public API? :wink:

[qsdigor]

Ok, so we would need some kind of manual cloning methods? We can make that but we need to make sure we do a deep copy of the objects. This can be quite tricky if we have objects inside objects as we need to take care of the whole layers of objects.

[qsdigor]

Do we need tests to test the mutability of let's say ClientOptions?

[ikbalkaya]

@KacperKluka I just had a call with @qsdigor. If we want to make an object immutable, isn't the best way to mark final all instances down to object reference hierarchy? Currently all non-final and public fields are exposed and some customers might be using them (even if not intended). Using a copy internally might be misleading if you expose a field directly to customers. I would favour locking the object from start (or from a point in time) using the structure rather than doing a workaround such as a copy. WDYT?

[qsdigor]

@KacperKluka @ikbalkaya If we make object variables final we need to agree on should we lock all of them or maybe leave something to be changed, one variable that sounds changeable is logLevel.

[KacperKluka]

I think that using the final keyword simply means that the object cannot be reassigned but it doesn't mean that you can't modify it, right? This won't work in our case, as I believe we have the potential problem that someone gives us an object and then changes its fields.

For example, let's take a look at ClientOptions, you can create an instance and set clientOptions.echoMessages = true and use it to create an Ably instance. Then, after Ably instance creation, you can call clientOptions.echoMessages = false on that ClientOptions and it will actually modify the value used inside the already created Ably instance.

We need to find a way to secure our SDK from such behaviour, as this could lead to unexpected behaviour.

[qsdigor]

@KacperKluka You are correct but if we make fields final and change the constructor to implement all fields this should be solved. @ikbalkaya Maybe we should consider both approaches as clientId might be mandatory with a few others and the ones that can be changed change but only explicitly in AblyRealtime instance as this ablyRealtime.options.echoMessages = false. We can do that by coping object.

[ikbalkaya]

I think that using the final keyword simply means that the object cannot be reassigned but it doesn't mean that you can't modify it, right?

If you mark references final down to the hierarchy it is effectively immutable.

[KacperKluka]

You are both correct, but the problem here is that we should not change the public API in the 2.0.0 release :wink: And if we were to use those final fields then we would have to change the API and force users to use object constructors (which is not bad itself). Additionally, objects such as ClientOptions that have 20+ fields would have enormous constructors and we would probably have to create builders for them (more API changes). So while it is a good idea on its own it doesn't fit our needs and limitations :disappointed:

[qsdigor]

We still need to discuss if there are some other objects that would benefit the object cloning/copying and as such, I am reopening the issue.