Excellent... I am really happy that you validated the effect in full. Breeds confidence.
Thanks On Mon, Jul 27, 2015 at 11:14 PM, Paul Merlin <[email protected]> wrote: > Guys, > > I reviewed JSONEntityState cloning and Tibor catched an obvious mistake. > > The cloneStateIfGlobalStateLoaded() method of JSONEntityState is called > before any change is made to the entity state. > > This method starts with the following check: > > if( isStateNotCloned() ) > { > return; > } > // Do the clone > > And the (private) isStateNotCloned() method returns: > status == EntityStatus.LOADED > > IOW, the code says that it will clone state if status is loaded, but it > does the opposite! > > Corresponding JIRA issue: https://issues.apache.org/jira/browse/ZEST-107. > > > Rewriting the cloneStateIfGlobalStateLoaded() method, as Tibor suggested, > fixes the issue: > > if( status != EntityState.LOADED ) > { > return; > } > // Do the clone > > I removed the isStateNotCloned() method BTW as it was not used elsewhere > and the code is simpler without it. > > > Moreover, I reworded the test case Tibor provided so it fits in > AbstractEntityStoreTest as this: > > @Test > public void > givenEntityStoredLoadedChangedWhenUnitOfWorkDiscardsThenDontStoreState() > throws UnitOfWorkCompletionException > { > UnitOfWork unitOfWork = module.newUnitOfWork(); > try > { > String identity = createEntity( unitOfWork ).identity().get(); > unitOfWork.complete(); > > unitOfWork = module.newUnitOfWork(); > TestEntity entity = unitOfWork.get( TestEntity.class, identity ); > assertThat( entity.intValue().get(), is( 42 ) ); > entity.intValue().set( 23 ); > unitOfWork.discard(); > > unitOfWork = module.newUnitOfWork(); > entity = unitOfWork.get( TestEntity.class, identity ); > assertThat( entity.intValue().get(), is( 42 ) ); // Fail here! > } > finally > { > unitOfWork.discard(); > } > } > > This test pass when no CachePool service is assembled. > > Without the fix described above, and as soon as a CachePool service is > used by (visible to) the EntityStore, this test fail: the cached version is > returned and the discarded change (intValue from 42 to 23) is loaded when > it shouldn't be. > > I then added some AbstractEntityStoreWithCacheTest in core/testsupport to > reuse all tests from AbstractEntityStoreTest adding a MemoryCachePool to > the assembly (overridable). And implemented it for all of our EntityStores > that suppots the Cache SPI (all - [Preferences, SQL]). > > So far so good, all SDK tests passes with theses changes. > > I did that work in a branch, merged into develop as a single commit, "à > la" gitflow, so we can revert all this easily if needed and keep the > complete history. > > Any comments? > > /Paul > > BTW, see https://issues.apache.org/jira/browse/INFRA-8434 for how to > close github pull requests from asf repositories > > > > Tibor Mlynarik a écrit : > > yes, test in pull request. > > > > cheers, > > > > Tibor > > > > > > On Jul 24, 2015, at 7:01 AM, Niclas Hedhman <[email protected]> wrote: > > > >> 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 > > > -- Niclas Hedhman, Software Developer http://zest.apache.org - New Energy for Java
