DirtyCheckingSupport check

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

DirtyCheckingSupport check

longwa
Looking for a fix for GRAILS-10984 and came across this in DirtyCheckingSupport:55:

final value = cpf.getPropertyValue(instance, a.name)
if(proxyFactory.isInitialized(value)) {
   if(value instanceof DirtyCheckingSupport) {
       DirtyCheckable dirtyCheckable = (DirtyCheckable) value
       if(dirtyCheckable.hasChanged()) {
          return true
       }
   }
}

It seems like the instanceof check should be "value instance DirtyCheckable" not "value instanceof DirtyCheckingSupport"? DirtyCheckingSupport is just a utility class, right? Probably autocomplete gone wild.

Not sure if that's causing any problems (yet) but thought it seemed kinda strange.

-Aaron
Reply | Threaded
Open this post in threaded view
|

Re: DirtyCheckingSupport check

Lari Hotari

Good catch, please JIRA it.

Lari

      14.01.2014 00:21, Aaron Long wrote:
Looking for a fix for GRAILS-10984 and came across this in DirtyCheckingSupport:55:

final value = cpf.getPropertyValue(instance, a.name)
if(proxyFactory.isInitialized(value)) {
   if(value instanceof DirtyCheckingSupport) {
       DirtyCheckable dirtyCheckable = (DirtyCheckable) value
       if(dirtyCheckable.hasChanged()) {
          return true
       }
   }
}

It seems like the instanceof check should be "value instance DirtyCheckable" not "value instanceof DirtyCheckingSupport"? DirtyCheckingSupport is just a utility class, right? Probably autocomplete gone wild.

Not sure if that's causing any problems (yet) but thought it seemed kinda strange.

-Aaron

Reply | Threaded
Open this post in threaded view
|

Re: DirtyCheckingSupport check

longwa
Lari, take a look at my comment on GRAILS-10984 when you get a chance. This does fix the sample app but it's a little more core than I'm comfortable making without more review. I'm building and running it with our full suite of tests now so I'll no if there's any collateral damage.





On Mon, Jan 13, 2014 at 11:51 PM, Lari Hotari <[hidden email]> wrote:

Good catch, please JIRA it.

Lari


      14.01.2014 00:21, Aaron Long wrote:
Looking for a fix for GRAILS-10984 and came across this in DirtyCheckingSupport:55:

final value = cpf.getPropertyValue(instance, a.name)
if(proxyFactory.isInitialized(value)) {
   if(value instanceof DirtyCheckingSupport) {
       DirtyCheckable dirtyCheckable = (DirtyCheckable) value
       if(dirtyCheckable.hasChanged()) {
          return true
       }
   }
}

It seems like the instanceof check should be "value instance DirtyCheckable" not "value instanceof DirtyCheckingSupport"? DirtyCheckingSupport is just a utility class, right? Probably autocomplete gone wild.

Not sure if that's causing any problems (yet) but thought it seemed kinda strange.

-Aaron


Reply | Threaded
Open this post in threaded view
|

Re: DirtyCheckingSupport check

longwa
Actually, the more I look at it, the more it seems like the call to checkCollectionElements() isn't really needed. I mean, the collection itself will be flagged as dirty if anything is modified in the collection or if the collection is new. If something in the collection is dirty, I'm not sure that really makes the collection property itself dirty.

If I have an Author with many Books and do the following, what is the expectation:

1. Load the Author
2. Get the first Book from the Author.books
3. Change the name of the Book

In this case, is the Author.books property "dirty"? Certainly the Book is dirty, but that doesn't make any collection that has a reference to that book dirty does it? Of course, the Book will be saved when the Author is saved using the default cascading behavior.

So far, I haven't found any issues with removing the checkCollectionElements() call and just using the flag for isDirty on collections.

-Aaron 


On Tue, Jan 14, 2014 at 11:00 AM, Aaron Long <[hidden email]> wrote:
Lari, take a look at my comment on GRAILS-10984 when you get a chance. This does fix the sample app but it's a little more core than I'm comfortable making without more review. I'm building and running it with our full suite of tests now so I'll no if there's any collateral damage.





On Mon, Jan 13, 2014 at 11:51 PM, Lari Hotari <[hidden email]> wrote:

Good catch, please JIRA it.

Lari


      14.01.2014 00:21, Aaron Long wrote:
Looking for a fix for GRAILS-10984 and came across this in DirtyCheckingSupport:55:

final value = cpf.getPropertyValue(instance, a.name)
if(proxyFactory.isInitialized(value)) {
   if(value instanceof DirtyCheckingSupport) {
       DirtyCheckable dirtyCheckable = (DirtyCheckable) value
       if(dirtyCheckable.hasChanged()) {
          return true
       }
   }
}

It seems like the instanceof check should be "value instance DirtyCheckable" not "value instanceof DirtyCheckingSupport"? DirtyCheckingSupport is just a utility class, right? Probably autocomplete gone wild.

Not sure if that's causing any problems (yet) but thought it seemed kinda strange.

-Aaron