Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Entities inside embedded collections don't track changes #148

Open
valentingoebel opened this issue Oct 5, 2020 · 1 comment
Open

Entities inside embedded collections don't track changes #148

valentingoebel opened this issue Oct 5, 2020 · 1 comment

Comments

@valentingoebel
Copy link

Here is an example project:

https://github.com/valentingoebel/grails-dirtycheckingbug/

application.yml requires following env variables so make sure you fill them.

grails:
    mongodb:
        host: ${MONGODB_HOST}
        port: 27017
        username: ${MONGODB_USER}
        password: ${MONGODB_PASS}
        databaseName: grailsdirtycheckingbug

Here is a summary of the code:

class DBList {

    static constraints = {
    }

    List<DBListEntry> entries = []
    static embedded = ['entries']

    static hasMany = [
        entries: DBListEntry
    ]
}

class DBListEntry {

    String value

    static constraints = {
    }
}

class DBListController {

    def index(Integer max) {
        def dblist = DBList.list()[0]

        dblist.entries.each { it ->
            it.value = it.value + "1"
            render "DBListEntry: This should be dirty but it isn't: ${it.listDirtyPropertyNames()}<br>"
        }

        render "DBList: This is not dirty (optional bug): ${dblist.listDirtyPropertyNames()}<br><br>"

        dblist.entries.each { it ->
            it.trackChanges()
        }

        dblist.entries.each { it ->
            it.value = it.value + "2"
            render "DBListEntry: It works!: ${it.listDirtyPropertyNames()}<br>"
        }

        render "DBList: This is not dirty (optional bug): ${dblist.listDirtyPropertyNames()}<br><br>"

        render "Output: ${dblist.entries*.value}<br>"
    }
}
  1. Calling trackChanges() on each entity inside the embedded list results in the desired outcome. I am already using this workaround in my projects but it required substantial changes in my code because using databinding in controller parameters like def show(DBList list) will fill each field before you even get an opportunity to call trackChanges().

  2. Even with trackChanges() there is still the issue that the list itself does not get marked as dirty. This is fine in the non embedded case because changing an entity would mark all its associated entities dirty which is certainly not what we want but for embedded entities it could make sense because an embedded entity is always contained inside a parent entity. Therefore changing the child could be interpreted as a change in the parent.

I only need the first bug fixed because it prevents me from using databinding in controller parameters. The second one is up for discussion and can be rejected if it doesn't make sense. Marking a list dirty when at least one of its elements has changed is very easy so I'm not really insisting on that change.

@taplar
Copy link

taplar commented Nov 6, 2020

Same issues, same fix. Wrote logic to call trackChanges on the nested entities in lists to make them not think they are dirty. Also wrote a custom isDirty method that recursively goes down an entity tree and "bubbles" the dirty state of nested elements all the way up the tree. The OP included logic to show their version of the issue. Here is mine that is a reduced form of things similar to how we have things structured and configured.

  • Domain Contact.groovy
package myapp.test

import com.mongodb.WriteConcern

class Contact {
    String firstName
    List<Address> addresses

    static embedded = [ 'addresses' ]
    static mapWith = 'mongo'

    static mapping = {
        version false
        collection 'contacts'
        writeConcern WriteConcern.ACKNOWLEDGED
    }

    static constraints = {
        firstName blank: false
    }
}

class Address {
    String address1
    String address2

    static constraints = {
        address1 blank: false
    }
}
  • Controller TestController.groovy
package myapp.test

class TestController {
    def test () {
        def contact = Contact.findByFirstName( 'TEST' )

        if ( contact ) {
            println "Existing Contact is dirty? ${contact.isDirty()}" // returns false [EXPECTED]
            contact.delete()
            contact.discard()
        }

        contact = new Contact( firstName: 'TEST', addresses: [] )
        contact.addresses << new Address( address1: 'TEST STREET 1', address2: 'TEST STREET 1.2' )
        contact.addresses << new Address( address1: 'TEST STREET 2', address2: 'TEST STREET 2.2' )

        println "New Contact is dirty? ${contact.isDirty()}" // returns true [EXPECTED]

        contact.save( flush: true )

        println "Saved Contact is dirty? ${contact.isDirty()}" // returns false [EXPECTED]

        contact.discard()

        contact = Contact.findByFirstName( 'TEST' )

        println "Existing Contact is dirty? ${contact.isDirty()}" // returns false [EXPECTED]
        println "Addresses list is dirty? ${contact.isDirty('addresses')}" // returns false [EXPECTED]
        println "Address 1 is dirty? ${contact.addresses[0].isDirty()}" // returns true [NOT EXPECTED]
        println "Address 2 is dirty? ${contact.addresses[1].isDirty()}" // returns true [NOT EXPECTED]

        render status: response.SC_OK, contentType: 'application/json', text: '{}'
    }
}

I would not expect anything on a freshly pulled domain record to be marked as dirty. And changing anything on an Entity I would expect to mark all parent structures as dirty as well.

I did see the note on https://gorm.grails.org/latest/hibernate/manual/index.html#domainClasses under 6.10.1. isDirty that says "isDirty() does not currently check collection associations, but it does check all other persistent properties and associations." so I assume that the "bubble" up logic isn't currently written to work like that with this sort of situation. However, the children being dirty on a fresh pull, to me, feels like a different thing and should not be happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants