[hibernate-dev] SessionFactory building APIs

[Steve Ebersole]

On Wed, Mar 25, 2015 at 4:57 PM, Gunnar Morling <gunnar at 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