Yeah, I guess belt and braces is gonna be what's needed. We could still get rid of the original map that keys on classname, though.
Dan On 31 May 2012 20:44, Robert Matthews <[email protected]> wrote: > Dan > > Tried that out and no exception was thrown! Created a quick repository and > everything was fine. This is a case of the class not being reachable > through the classes that are initially loaded and hence is loaded later. > Looks like we'll never know when everything is really loaded. We could > combine the ideas by doing an initial validation when the classes have been > traversed and then when/if another loaded after that point then just > validate that new class. > > > Rob > > > > On 31/05/12 16:27, Dan Haywood wrote: > >> Hi Rob, >> >> I don't think there is a need to cache by both... but the caching by >> ObjectSpecId is new, and I didn't look into the existing caching to figure >> out if it could be removed. It clearly ought to be possible to do so. >> >> Looking into the code a little more, and to recap... >> >> * the original caching (by class name) is to ensure that the reflecting >> over the metamodel doesn't go into an infinite loop, where A -> B and B >> -> >> A. >> * this cache is built up eagerly, as the metamodel is "discovered" >> * the new caching (by ObjectSpecId) in contrast is done at the end, as >> part >> of the validator logic to ensure that two entities haven't been annotated >> with the same value. >> >> In terms of the defect you've found - where a spec is cached by class name >> but not by ObjectSpecId - it would seem that this could happen if an >> entity >> is somehow missing an ObjectSpecFacet. In >> ObjectReflectorDefault#**validateSpecifications() you'll see: >> >> final ObjectSpecId objectSpecId = objSpec.getSpecId(); >> if (objectSpecId == null) { >> continue; >> } >> >> Looking in turn at ObjectSpecificationAbstract#**getSpecId(), it seems >> that >> this could return null if that facet is missing: >> >> if(specId == null) { >> final ObjectSpecIdFacet facet = >> getFacet(ObjectSpecIdFacet.**class); >> if(facet != null) { >> specId = facet.value(); >> } >> } >> return specId; >> >> To track down this defect, I guess the above could be changed to throw an >> exception, eg: >> >> if(specId == null) { >> final ObjectSpecIdFacet facet = >> getFacet(ObjectSpecIdFacet.**class); >> if(facet == null) { >> throw new IllegalStateException("could not find an >> ObjectSpecIdFacet for " + this.getFullIdentifier()); >> } >> } >> return specId; >> >> >> ~~~ >> In terms of combining the caching, it seems that the ObjectSpecId cache >> could be built up eagerly, ie in SpecificationCacheDefault change: >> >> public void cache(final String className, final ObjectSpecification >> spec) { >> specByClassName.put(className, spec); >> } >> >> to instead be something like: >> >> public void cache(final ObjectSpecification spec) { >> final ObjectSpecIdFacet facet = getFacet(ObjectSpecIdFacet.** >> class); >> >> // check specId exists >> if(facet == null) { >> throw new IllegalStateException("could not find an >> ObjectSpecIdFacet for " + spec.getFullIdentifier()); >> } >> final ObjectSpecId specId = facet.value(); >> >> // check for uniqueness >> final ObjectSpecification existingIfAny = specById.get(specId); >> if (existingIfAny != null&& existingIfAny != spec) { >> >> throw new IllegalStateException("there is already a >> specification " + existingIfAny.**getFullIdentifier() " + " with a spec >> Id of >> " + specId); >> } >> >> specById.put(specId, spec); >> } >> >> >> Does that make sense? >> >> Dan >> >> >> >> On 31 May 2012 15:23, Robert >> Matthews<rmatthews@**nakedobjects.org<[email protected]>> >> wrote: >> >> Dan >>> >>> Can you explain the need for caching specifications by both class and >>> ObjectSpecId. Why do we need both? And, in particular, why might the >>> class >>> entry exist, but not the ObjectSpecId one? >>> >>> While testing out the redesigned code I am failing to get a specification >>> for a known class; while a debug list of specification shows it as >>> loaded. >>> Specifically, I am testing out transient objects being used within the >>> Scimpi interface. >>> >>> Thanks. >>> >>> Rob >>> >>>
