Repository navigation
Increase mutability safety by cloning objects #771
Description
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).
Activity
qsdigor commentedon Jul 13, 2022
There are different techniques to do object cloning. We should discuss what is best for us.
Use clone() if the class implements Cloneable. This API is a bit flawed in java.
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 ).
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 ).
Use some library to do deep cloning like Cloning or Apache SerializeUtil
My suggestion is 3 or 4.
KacperKluka commentedon Jul 13, 2022
Thank you for the suggestions 🙇 IMO the best choice would be either 2 or modified 3 🤔
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? 😉qsdigor commentedon Jul 13, 2022
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 commentedon Jul 18, 2022
Do we need tests to test the mutability of let's say ClientOptions?
ikbalkaya commentedon Jul 18, 2022
@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 commentedon Jul 18, 2022
@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 commentedon Jul 18, 2022
I think that using the
finalkeyword 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 setclientOptions.echoMessages = trueand use it to create an Ably instance. Then, after Ably instance creation, you can callclientOptions.echoMessages = falseon 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 commentedon Jul 18, 2022
@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 commentedon Jul 18, 2022
If you mark references
finaldown to the hierarchy it is effectively immutable.KacperKluka commentedon Jul 19, 2022
You are both correct, but the problem here is that we should not change the public API in the 2.0.0 release 😉 And if we were to use those
finalfields then we would have to change the API and force users to use object constructors (which is not bad itself). Additionally, objects such asClientOptionsthat 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 😞qsdigor commentedon Aug 1, 2022
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.