Optimistic locking allows ReactiveMongoTemplate to modify immutable objects [DATAMONGO-1842]

[spring-projects-issues]

Martin Drozdik opened DATAMONGO-1842 and commented

Steps to reproduce

A minimal working example is here:

https://github.com/martin-drozdik/reactive-mongo-bug-demo

Given an immutable class:

data class Person
(
    val id: String,
    @Version val version: Long? = null
)

It is nevertheless possible to modify its instances, just by calling ReactiveMongoTemplate::save

@Component
class PersonHandler(val template: ReactiveMongoTemplate)
{
    fun initialize()
    {
        val jim = Person("Jim")
        require(jim.version == null)

        template.save<Person>(jim).block()
        require(jim.version == null) // This require will fail
        {
            "Immutable object has been modified"
        }
    }
}

If this is by design, it could be beneficial to add a warning, or just a simple mention in the documentation. Otherwise I find it counterintuitive that a save method could break a fundamental principle of the language

---

Affects: 2.0.2 (Kay SR2)

Reference URL: https://github.com/martin-drozdik/reactive-mongo-bug-demo

Issue Links:

• DATACMNS-1322 Add support for immutable objects in PersistentPropertyAccessor

[spring-projects-issues]

Oliver Drotbohm commented

While I agree that this is non-intuitive, I wonder how – given the way your class is designed – you expect that class to be used by the framework to return a version value other than null except modifying the field via reflection. You initialize it to null, there's not constructor to create instance with a different version, there's no wither-method.

Yes, the current implementation (btw. the reactive and the non-reactive version do the same here in terms of how to set the version on the object) assumes mutability but without a detectable way to create new instances with a different (dedicated) version, there's just no way we can make optimistic locking work on the instance level

[spring-projects-issues]

Martin Drozdik commented

Thank you for the follow up!

??I wonder how – given the way your class is designed – you expect that class to be used by the framework to return a version value other than null except modifying the field via reflection.??

Simple, I will just throw away the saved object and instantiate a new one from the database with findById. The framework should give me a new instance with a properly initialized version. The framework does not need reflection for this, just pass the new version to the constructor.

However, now I see your point. Going to the database each time can be indeed wasteful, but this is the cost you pay for immutable objects (there are of course benefits too).

I discovered the behavior while writing unit tests:

val jim = Person("jim")

// Do something test-worthy
reactiveTemplate.save<Person>(jim).block() // Put Jim inside
reactiveTemplate.remove<Person>(Query(where("id").isEqualTo("jim")))  // remove Jim

// Do something test-worthy
reactiveTemplate.save<Person>(jim).block() // OptimisticLockingFailureException

Anyway, just a simple sentence like "save increments the version of the argument" could spare some of your users some bafflement, but that is up to you to decide

[spring-projects-issues]

Oliver Drotbohm commented

There is no constructor taking a value for the version property as you directly initialize the immutable property to null. We can't simply read the object from the database as we need to initialize the version property on first storage. Optimistic locking in MongoDB is not a database feature, it's implemented by Spring Data. On subsequent calls to save(…) we also need to tweak the version property before the document makes it into the store. All of that either requires changing the state of the object handed to us or means to detect that we deal with a value type (immutable) and API exposed, so that instead of changing the current object's state, we can create a fresh instance with an updated version, e.g. a Person withVersion(…) or the like.

We currently implement the former as there are no means to detect and implement the latter right now. I've filed DATACMNS-1239 to investigate options here as we'd like to be a good citizen to immutable types and not get in the way of users trying to use them

[spring-projects-issues]

Martin Drozdik commented

I see where the confusion can come from. The code I posted:

data class Person
(
    val id: String,
    @Version val version: Long? = null
)

means that the class has one constructor with a default parameter. We may instantiate the class like this:

val version = 197
val jim = Person("Jim", version)

But thanks to your comment I understand better how Spring works. Thanks for your valuable feedback and for maintaining Spring! I will learn to be aware of this design :)

[spring-projects-issues]

Oliver Drotbohm commented

We've just fixed DATACMNS-1322 and you should Spring Data MongoDB do the right thing™ now