Regarding the "multiple roles" of Module; Yes, this is a well-known design compromise. Way back in time, there were separated UnitOfWorkFactory, ValueBuilderFactory and so on, roles that then later the Module came to fulfill.
So, one *should* (but I guess few does) actually write; @Structure UnitOfWorkFactory uowf; instead of @Structure Module module; which would better allow future design changes without breaking compatibility. We *could* go back and say that the "Module" interface doesn't have all these roles, even if the ModuleInstance implementation under the hood actually does implement them. The downside is a couple of more injections, but perhaps worthwhile, for the sake of tuning internal design. Cheers Niclas On Mon, Jun 29, 2015 at 11:37 PM, Kent Sølvsten <[email protected]> wrote: > Great job! > > Actually I think this change is soooo obviously correct from a design > perspective. Since the "module" used is not the "structural" module > attached to the entitystore, but the "calling module", it simply seems > wrong to couple those together in the EntityStoreUnitOfWork concept. > > I even start to wonder if the concept of EntityStoreUnitOfWork should > simply be scrapped ? > > Another note: In the codebase it is sometimes not very obvious when > seeing a module reference, whether we are considering a > "structurally-attached module" (the one holding the entity store used > for saving the entities) or just looking at a "calling module" (often > used to lookup xxxDescriptors). DCI used the concept of roles - could it > be useful in Qi4J ? Sometimes we are using a module as a > "DescriptorSource" - other times we are using it as an > "EntityStoreHolder" .... or something like that. > > Just toying with ideas here :-) > > /Kent > > > > Den 25-06-2015 kl. 14:29 skrev Niclas Hedhman: > > And YES, the proposed approach and a handful of SPI interface changes > made > > my usecase work as expected. > > > > So, even if I had the wrong approach to my particular issue, the new > > behavior is probably as expected, so I think it should go into the > > codebase, and I think it might as well go in now, rather than later. > > > > EntityStore implementations will break at compile time, but the fix is > > relatively straight forward. > > > > Any other opinions? > > > > // Niclas > > > > On Thu, Jun 25, 2015 at 7:23 PM, Niclas Hedhman <[email protected]> > wrote: > > > >> Kent, > >> I tried to implement the ModuleEntityStoreUnitOfWork idea. It is not as > >> smooth as it first seems, since the inner EntityStoreUnitOfWork needs to > >> call entityStateOf() which needs the module. BUT, by modifying a couple > of > >> SPI interfaces, it is possible to pass the Module along in method calls. > >> Along with removing EntityStoreUnitOfWork in some cases where only > >> something inside the EsUoW is actually wanted, which in some cases > resulted > >> in creating a EsUoW for no practical reason, I have it working.... i.e. > >> doesn't break any existing testcases. I haven't checked whether my case > was > >> solved as well. Will do that shortly. > >> > >> > >> > >> On Thu, Jun 25, 2015 at 5:44 AM, Kent Sølvsten < > [email protected]> > >> wrote: > >> > >>> Hmmm interesting problem . Could it be reproduced with something like > >>> this: > >>> > >>> a. Creation of a UnitOfWork > >>> b. Accessing entity M1 in module M > >>> c. Entity E calls service MS in module M. > >>> d. Service uses entity N1 in module N (N1 has some sort of shared > >>> visibility) > >>> .... (later in the same UOW) > >>> e. Someone uses service NS in module N (some short of shared > visibility). > >>> f. Service NS uses entity N2 in module N (which has only module > >>> visibility) > >>> > >>> I am not sure that using a composite module/entitystore cache key is > >>> such a good idea. Wouldn't that lead to 2 instances of > >>> EntityStoreUnitOfWork, basically splitting uow.complete() into 2 native > >>> transactions in that case? > >>> Could we instead reuse the "ModuleUnitOfWork-idea" ? > >>> > >>> Something like: > >>> > >>> public EntityStoreUnitOfWork getEntityStoreUnitOfWork( EntityStore > >>> store, Module module ) > >>> > >>> { > >>> > >>> EntityStoreUnitOfWork uow = storeUnitOfWork.get( store ); > >>> > >>> if( uow == null ) > >>> > >>> { > >>> > >>> uow = store.newUnitOfWork( usecase, currentTime ); // no > >>> module here! > >>> > >>> storeUnitOfWork.put( store, uow ); > >>> > >>> } > >>> > >>> return new ModuleEntityStoreUnitOfWork(uow, module); > >>> > >>> } > >>> > >>> As a side-note we could one day consider some sort of > >>> entitystore-sharing between modules. If we had 2 modules, separated for > >>> design reasons but sharing the same persistence, it would be nice to be > >>> able to complete a UOW spanning those 2 modules in a single native > >>> transaction. > >>> > >>> Regarding "bug-compatible" the safe option would be to postpone to 3.0 > - > >>> but i guess it is better to just fix the issue? > >>> > >>> /Kent > >>> > >>> > >>> > >>> Den 24-06-2015 kl. 17:40 skrev Niclas Hedhman: > >>>> Follow-up; The case when it gets wrong is more complex than my list, > >>> and I > >>>> am not able to create a testcase for this... > >>>> > >>>> On Wed, Jun 24, 2015 at 10:51 PM, Niclas Hedhman <[email protected]> > >>> wrote: > >>>>> Gang, > >>>>> > >>>>> I have found a fairly sophisticated bug, which is rather severe and > >>>>> possibly hard to fix without breaking applications, that might depend > >>> on > >>>>> wrong behavior... > >>>>> > >>>>> It manifests itself as not having the right Visibility of entities, > and > >>>>> the reason is because the EntityStoreUnitOfWork may be set up > >>> incorrectly > >>>>> sometimes. > >>>>> > >>>>> I think it happens if the following sequence occurs; > >>>>> > >>>>> a. Creation of a UnitOfWork > >>>>> b. Accessing entity E in module M > >>>>> c. Entity E calls service S in module N. > >>>>> d. Service S tries to access entity F in module N. > >>>>> > >>>>> internally, the EntityStoreUnitOfWork will have been initialized with > >>> the > >>>>> Module M, and uses that to find entity F, which is not seen > >>>>> (Visibility.module). > >>>>> > >>>>> This is very similar to a design flaw a long time ago, where the > >>>>> visibility was effectively in the wrong Layer, and that is when the > >>> "split" > >>>>> UnitOfWork concept was introduced (2008?). > >>>>> > >>>>> The issue manifests itself in UnitOfWorkInstance in the following > >>> method; > >>>>> public EntityStoreUnitOfWork getEntityStoreUnitOfWork( EntityStore > >>> store, Module module ) > >>>>> { > >>>>> EntityStoreUnitOfWork uow = storeUnitOfWork.get( store ); > >>>>> if( uow == null ) > >>>>> { > >>>>> uow = store.newUnitOfWork( usecase, module, currentTime ); > >>>>> storeUnitOfWork.put( store, uow ); > >>>>> } > >>>>> return uow; > >>>>> } > >>>>> > >>>>> as the EntityStoreUnitOfWork will have been created with the wrong > >>> module. > >>>>> I think the solution is that the Module must also be part of the > cache > >>>>> key, and that this would solve it. However, I am very anxious that > >>> this may > >>>>> break something else very badly, as it is fairly central to the > entire > >>>>> UnitOfWork flow. > >>>>> > >>>>> The reason I say that it might break applications is that, the called > >>>>> Service has access to the entities in the modules calling it, and > >>> someone > >>>>> might depend on that. But I think, we should be allowed to break bug > >>>>> compatibility... Or?? > >>>>> > >>>>> BTW, this touches on the "multiple entitystore" feature that came up > >>>>> during the discussion of the Messaging abstraction. > >>>>> > >>>>> Cheers > >>>>> -- > >>>>> 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
