Collections defensive copy


#1

I have a question about defensive copy of input collections and immutability. Are you considering making a copy of collections that are passed to the constructor of data class. I think it would solve mess that appears when you try to enforce it by hand. It will be clear from the following few examples. I hope you’ll have enough patience to read the code :slight_smile: So we have a data class that we are counting on to be immutable and a test that tests that immutability.

data class WannaBeImmutable(
        val id: Long,
        val name: String,
        val age: Int,
        val accountIds: List<String>) {
}

    @Test
    public fun testWannaBeImmutableStandardStuff() {

        val accountIds = ArrayList<String>()
        accountIds.add("1111")
        val wannaBeImmutableUnderTest = WannaBeImmutable(42L, "John", 30, accountIds)

        Assert.assertEquals(1, wannaBeImmutableUnderTest.accountIds.size)

        // This is ok
        val wannaBeImmutableDoe = wannaBeImmutableUnderTest.copy(name = "Doe")
        Assert.assertEquals("John", wannaBeImmutableUnderTest.name)
        Assert.assertEquals("Doe", wannaBeImmutableDoe.name)

        // Also ok
        val wannaBeImmutableRich = wannaBeImmutableDoe.copy(accountIds = listOf("333", "555"))
        Assert.assertEquals(1, wannaBeImmutableDoe.accountIds.size)
        Assert.assertEquals(2, wannaBeImmutableRich.accountIds.size)

        // wannaBeImmutableRich.accountIds.add("444") // Compile error, it's fine
    }

    @Test
    public fun testInputArrayList() {
        val accountIds = ArrayList<String>()
        accountIds.add("1111")
        val wannaBeImmutableUnderTest = WannaBeImmutable(42L, "John", 30, accountIds)

        Assert.assertEquals(1, wannaBeImmutableUnderTest.accountIds.size)

        // Mutate input list
        accountIds.add("2222")
        Assert.assertEquals(1, wannaBeImmutableUnderTest.accountIds.size) // Fails
    }

    @Test
    public fun testOutputArrayList() {
        val accountIds = ArrayList<String>()
        accountIds.add("1111")
        val wannaBeImmutableUnderTest = WannaBeImmutable(42L, "John", 30, accountIds)

        Assert.assertEquals(1, wannaBeImmutableUnderTest.accountIds.size)

        // Mutate output list
        (wannaBeImmutableUnderTest.accountIds as ArrayList).add("22222")
        Assert.assertEquals(1, wannaBeImmutableUnderTest.accountIds.size) // Fails
    }

testInputArrayList and testOutputArrayList fail because although the Kotlin List interface is immutable, the implementation isn’t. It can easily be solved by defensive copy. I’ll paste the test of the RealImmutable class first just to show you what would I expect. Every assertion in this test passes.

    @Test
    public fun testRealImmutable() {

        val accountIds = ArrayList<String>()
        accountIds.add("1111")
        val realImmutableUnderTest = RealImmutable(42L, "John", 30, accountIds)

        Assert.assertEquals(1, realImmutableUnderTest.accountIds.size)

        // This is ok
        val newRealImmutableNamedJoe = realImmutableUnderTest.copy(name = "Doe")
        Assert.assertEquals("John", realImmutableUnderTest.name)
        Assert.assertEquals("Doe", newRealImmutableNamedJoe.name)

        // Also ok
        val realImmutableRich = newRealImmutableNamedJoe.copy(accountIds = listOf("333", "555"))
        Assert.assertEquals(1, newRealImmutableNamedJoe.accountIds.size)
        Assert.assertEquals(2, realImmutableRich.accountIds.size)

        // Mutate input list
        accountIds.add("2222")
        Assert.assertEquals(1, realImmutableUnderTest.accountIds.size) // Ok, because of defensive copy

        // Mutate output list
        (realImmutableUnderTest.accountIds as ArrayList).add("12345")
        Assert.assertEquals(1, realImmutableUnderTest.accountIds.size) // Ok, because of defensive copy

        // Test addAccountId method
        val evenRicherRealImmutable = realImmutableRich.addAccountId("33333")
        Assert.assertEquals(2, realImmutableRich.accountIds.size)
        Assert.assertEquals(3, evenRicherRealImmutable.accountIds.size)
    }

And here is the RealImmutable class. I cannot simply make a defensive copy when using data class. Unfortunately everything falls apart :frowning: I’ve put some comments in the code itself. Does anybody have some advice?

data class RealImmutable(val id: Long,
                         val name: String,
                         val age: Int
) {
    constructor(id: Long,
                name: String,
                age: Int,
                accountIds: List<String>)
    : this(id, name, age) {
        // Have to do defensive copy to be really immutable
        this._accountIds = accountIds.toList()
    }

    private lateinit var _accountIds: List<String>
    public val accountIds: List<String>
        get() = _accountIds.toList() // Defensive copy of outputs

    /*
        Have to make copy method manually because,
        accountIds isn't in the constructor :/
    */
    public fun copy(
            id: Long = this.id,
            name: String = this.name,
            age: Int = this.age,
            accountIds: List<String> = this._accountIds
    ) = RealImmutable(id, name, age, accountIds)

    /*
        Cannot do something like this because overriding messes up data class
        This will evaluate to something like this:
        RealImmutable@5e853265, accountIds=[1111]
     */
    override fun toString(): String {
        return super.toString() + ", accountIds=$accountIds"
    }

    /*
        Nope, messes up data class :/
     */
    override fun hashCode(): Int {
        return super.hashCode() * 31 + accountIds.hashCode()
    }

    /*
        Nope, messes up data class :/
     */
    override fun equals(other: Any?): Boolean {
        return super.equals(other) // and include accountIds
    }

    /*
        I would have to generate toString, equals and hashCode
        every time I change a property which beats the point
        of data class. Also, while in data class Android Studio
        doesn't offer me generate code options for those 3 guys.
        I have to live without them and put a comment that user
        of the class can't count on toString, equals and hashCode
        which is error prone or I have to abandon data class and
        generate the methods my self which is sad :/
     */

    /*
        This could be useful instead of copy method if I want
        more control of accountIds list manipulation.
    */
    public fun addAccountId(accountId: String): RealImmutable =
            if (_accountIds.contains(accountId)) {
                this
            } else {
                RealImmutable(id, name, age, _accountIds + listOf(accountId))
            }
}

#2

You could consider using the type system to help you, e.g.

data class Foo(val x: ImmutableList<String>)

where ImmutableList is for instance, the one from Guava. Or alternatively,

fun List<T>.freeze(): ImmutableList<T> = this.toArrayList()```

#3

Yup, sure, that is always one possible solution. But I just wanted to check if I missed something from the Kotlinland that could help me.