On second thought... MetadataBuilder is also an interface (unless you are suggesting to change that for some reason), so not sure how a call like this would work:
MetadataBuilder.defineSources() On Wed, Mar 25, 2015 at 6:21 PM, Steve Ebersole <st...@hibernate.org> wrote: > On Wed, Mar 25, 2015 at 4:57 PM, Gunnar Morling <gun...@hibernate.org> > wrote: > >> > 1) What do you think of the split in MetadataSources and >> MetadataBuilder? >> > Does the aplit make sense? Or does it make more sense to combine them >> into >> > one contract? >> >> I think the split makes sense, as I understand that there are two >> different "phases" of configuration here: >> >> * add multiple sources of configuration (XML files, annotated classes) >> * apply further configuration (naming strategy etc.) >> > >> Assuming one first needs to configure all the sources before applying the >> configuration from the second category, it seems useful to express that in >> the API. >> > > MetadataSources is used to collect the sources of mapping information. > However, its really not necessary that the split exists in terms of "this > needs to be set before that". Its more a functional split because they > each serve different roles. One collects the sources of metadata > information. One builds that into a Metadata object (based on some config). > > > >> >> The name "MetadataSources" made me stumble a bit, though. Generally I >> find pluralized class names a bit odd, only really useful for either >> collection-like classes or static helper classes dealing with a specific >> type (e.g. Strings, Collections). What would you think about >> MetadataSources not being a top-level class itself, but rather an inner >> class of MetadataBuilder (e.g. named MetadataBuilderContext) which is >> returned by a static creator method on MetadataBuilder: >> >> MetadataBuilder builder = MetadataBuilder.configure() // returns >> MetadataBuilderContext >> .addFile(...) >> .addAnnotatedClass(...) >> .addResource(...) >> .getBuilder(); >> >> IMO that would help users a bit to find the right entry point. >> > > Not sure about the naming. I think MetadataSources is much better than > MetadataBuilderContext. > That's my take. We can see what others think. > > As far as the API, I am ok with changing that up if others agree. I could > see something like: > > MetadataBuilder.defineSources() > .addFile(...) > .addAnnotatedClass(...) > .addResource(...) > .getBuilder(); > > defineSources (or whatever we call it) needs to be overloaded to be able > to accept a ServiceRegistry: > > MetadataBuilder.defineSources() > > versus: > > MetadataBuilder.defineSources( new > StandardServiceRegistryBuilder()...build() ) > > > > >> Some more questions/thoughts: >> >> * Who is the intended client for the getter methods on MetadataSources? >> Only ORM, or also user code? In case of the former, should the public >> MetadataSources >> contract be an interface with the addXy() methods only, whereas getters are >> only accessible via an internal implementation class? That would narrow >> down the exposed API. >> > > Yes, the intended usage is just Hibernate itself. However, the exposure > is just a natural follow on of the design that MetadataSources is a class > that one instantiates directly. Can't instantiate an interface of course. > An approach such as above addresses that. > > >> * Are MetadataSources#addAttributeConverter(), >> addAuxiliaryDatabaseObject() and addSqlFunction() adding a *source* for >> meta-data really? Somehow it seems they should rather be located on >> MetadataBuilder? >> > > From one perspective yes. Not *sources* per-se, but they are things that > the user supplies that become part of the Metadata; as opposed to simply > options that are used during the process of building a Metadata > (MetadataBuilder). And especially since you want to rename "sources" to > "building context"; imo that makes the argument that these belong here even > stronger. > > > >> * I don't think MetadataSources is intended for concurrent usage from >> several threads, right? If so, it should not be needed to have a >> ConcurrentHashMap as a member >> > > True. Not sure why it stores those in ConcurrentHashMap... > > > >> * MetadataSources#addInputStream() et al.: What schema have passed >> streams to adhere to? Are these orm.xml, hbm.xml or both? Would be nice to >> have this documented >> > > Well same is true of addResources, addFile, add.. I get what you are > saying, but why do you single out addInputStream here? To be honest, I > would personally love to get rid of addInputStream. I think > addInputStreamAccess(InputStreamAccess) is a MUCH better solution there. > > > >> * MetadataBuilder#setSourceProcessOrdering: May be a bit nicer to use if >> modelled with var-args: >> >> sourceTypeProcessingOrder(MetadataSourceType first, >> MetadataSourceType... others) -> used like sourceTypeProcessingOrder( >> CLASS, HBM ); >> > > > +1 > > _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
