[hibernate-dev] SessionFactory building APIs

[Gunnar Morling]

2015-03-26 3:56 GMT+01:00 Steve Ebersole <steve at hibernate.org>:

> 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()
>

Ah, I forgot you cannot rely on Java 8 yet ;)

So a static method somewhere else would be needed indeed, maybe something
like:

    Bootstrap.configureMetadataBuilderSources()
        .addFile(...)
        .getMetadataBuilder();

On Wed, Mar 25, 2015 at 6:21 PM, Steve Ebersole <steve at hibernate.org> wrote:
>
>> 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
>>
>>
>