kaleidos / grails-postgresql-extensions

Grails plugin to use postgresql native elements such as arrays, hstores,...
Apache License 2.0
78 stars 63 forks source link

fix dirty check for empty hstore #104

Closed jglapa closed 7 years ago

jglapa commented 7 years ago

for issue https://github.com/kaleidos/grails-postgresql-extensions/issues/96

After debugging hibernate I have found the root cause. Here's a short explanation:

GORM to find dirty properties compares the actual object instance with an entry (this is I assume is the initial object without any values) https://github.com/grails/gorm-hibernate5/blob/master/grails-datastore-gorm-hibernate5/src/main/groovy/org/grails/orm/hibernate/HibernateGormInstanceApi.groovy#L53-L61 the crucial thing is here that def entry = findEntityEntry(instance, session) method

If you dive deep enough into this method you will end up in TwoPhaseLoad class and a line where deepCopy happens: https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/engine/internal/TwoPhaseLoad.java#L267-L274 the hydratedState is an array of all object properties that later on are transformed into loadedState. But what's important here is this: TypeHelper.deepCopy iterates over all types and when it encounters the UserDefined HstoreMapType it uses its deepCopy method.

The main problem here is the groovy truth. null and empty Map is not the same when in comes to equals method that's being used by isDirty later on (https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/type/AbstractType.java#L70-L72).

Doing a deepCopy for empty map or null will yield the same result: null.

Initially Hibernate created an empty map for that hstore type but if you deepCopy it you will get a null. The isDirty equal call will fail and hence the object will be dirty

So to fix this we just need to be consistent in deepCopy. Return null if it's a null or copy the map even if it's empty.

PS. I'm more interested in back-porting this into v4. This should be trivial. How should that be done? A PR with cherry-pick?

jglapa commented 7 years ago

not sure what's wrong with those travis builds, bintrayUser shouldn't be needed when running a PR check??

ilopmar commented 7 years ago

I've fixed the bintray credentials for the local build and I've also merged the PR and published version 5.0.1 with the fix.

Thanks for your contribution!

ilopmar commented 7 years ago

BTW, I've backported the changes to 4.x branch and I've published the version 4.6.9 with the fix.