The "this testcase" is referring to the pull request? I'll take a look a bit later.... Checking the "Release" at the moment.
Cheers Niclas On Fri, Jul 24, 2015 at 12:14 PM, Tibor Mlynarik <[email protected]> wrote: > Hi Niclas, > > > > > The "isStateNotCloned()" fix seems legit and a testcase for that would be > > awesome (can't figure one out in my at this early hour) > > this testcase should be showing that "isStateNotCloned()" is broken. > Change to entity in uow3, which is discarded, is visible to others, as > cloning is not working. > > account3.balance().set( BigDecimal.TEN); > uow3.discard() ; > > on other side, clone is executed with each set to entity, what is probably > only unnecessary performance penalty. > > > As for the testcase, the bug that it exposes is that it doesn't prevent > you > > from using the "Account account1" references outside the UoW it was > created > > in. > > > I see your point that accessing entity fetched from other uow ( closed > one) should be prohibited. > But maybe accessing just identity value should be allowed. > But this is not what I wanted to report. > > thanks , > > Tibor > > On Jul 24, 2015, at 3:50 AM, Niclas Hedhman <[email protected]> wrote: > > > I have not checked the code yet, but the clone is supposed to be there to > > provide UoW isolation, i.e. a value pulled out of the cache, and modified > > (any of the set methods) must not be visible into the cached state until > > the UoW completes, at which time I presume the entity state is replaced > in > > the the cache. > > > > As for the testcase, the bug that it exposes is that it doesn't prevent > you > > from using the "Account account1" references outside the UoW it was > created > > in. That is the bug, and I would be happy to look into that (perhaps for > > 3.0 just in case you depend on this fault behavior). If identity is used, > > it is more likely working as expected. > > > > > > The "isStateNotCloned()" fix seems legit and a testcase for that would be > > awesome (can't figure one out in my at this early hour) > > > > > > Thanks, Tibor > > > > > > On Fri, Jul 24, 2015 at 12:31 AM, Tibor Mlynarik < > [email protected]> > > wrote: > > > >> Hi Paul, > >> > >> I have prepared test case to demonstrate issue. > >> Problem is if there is enabled cache in json based entity store. > >> Entities could be in wrong state. > >> > >> see here : > >> https://github.com/apache/zest-qi4j/pull/2 > >> > >> to make test success, fix : > >> > >> JSONEntityState > >> void cloneStateIfGlobalStateLoaded() > >> { > >> if( ! isStateNotCloned() ) > >> > >> > >> there is one more fix needed in JSONMapEntityStoreMixin#fetchCachedState > >> method . > >> > >> Note: I was not able to configure entity store with cache service in one > >> layer , > >> probably because of circular dependency: store uses cache, cache need > >> store for config . > >> > >> cheers, > >> > >> Tibor > >> > >> > >> On Jul 23, 2015, at 1:49 PM, Paul Merlin <[email protected]> wrote: > >> > >>> Hi Tibor, > >>> > >>> Tibor Mlynarik a écrit : > >>>> Hi, > >>>> > >>>> I have noticed code in JSONEntityState that seems suspicious for me. > >>>> > >>>> In method JSONEntityState#cloneStateIfGlobalStateLoaded > >>>> if parts are swapped. > >>>> > >>>> I suppose that by global state is meant copy taken from cache that is > >> shared across UoWs. > >>>> And before first entity change own copy for UoW should be cloned. > >>>> > >>>> But as it is now, cloning is not done at first change and redundantly > >> executed with each entity change. > >>>> Also maybe whole cloning could be avoided if cache is not used. > >>>> > >>>> Or am I wrong in understanding of purpose of this state clone ? > >>> IIRC this is the sole purpose of this state clone. > >>> As you pointed out there may be room for improvements here. > >>> > >>> Niclas, WDYT? > >>> > >>> /Paul > >>> > >> > >> > > > > > > -- > > Niclas Hedhman, Software Developer > > http://zest.apache.org - New Energy for Java > > -- Niclas Hedhman, Software Developer http://zest.apache.org - New Energy for Java
