[hibernate-dev] SessionFactory building APIs

[Gunnar Morling]

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

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

I see. Also if it's not strictly needed, I kind of like the split, as it
enforces some structuring of client code which should help with readability.


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

Ok.

>
>> 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.
>

Yes, it would be nice to hide all those methods from the API which are not
intended for the user.


> * 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?
>

That was only one example, it applies to others as well.


>   To be honest, I would personally love to get rid of addInputStream.  I
> think addInputStreamAccess(InputStreamAccess) is a MUCH better solution
> there.
>

Not sure, that would require to invoke another method/ctor to obtain the
ISA from whatever type the user wishes to add, right? I think it's easier
on the user if they can pass their input type directly.

Speaking of it, I see there is
MetadataSources#addInputStream(InputStreamAccess) already. ISA lives in
*.spi, though. IMO the API should not leak SPI types in signatures.

Btw. could you also add addPackage(Package), so one can do

    addPackage( MyEntity.class.getPackage() )

?

* 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
>
>