[hibernate-dev] SessionFactory building APIs

[Gunnar Morling]

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

> I'm not so convinced that we need this additional level of instantiation
> just to avoid exposing access to the collected sources (getters).
>

Few things are strictly needed in the end :)

IMO limiting the exposed API as much as possible is a good thing, as it a)
reduces complexity on the user (the lesser methods there are, the quicker
they find the ones to deal with) and b) gives you more freedom to alter
things down the road if needed.


> Anyone else have opinions on this?
>

+1 Would like to know that as well :)

On Thu, Mar 26, 2015 at 3:35 AM, Gunnar Morling <gunnar at hibernate.org>
> wrote:
>
>> 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
>>>>
>>>>
>>>
>>
>