2015-03-26 0:21 GMT+01:00 Steve Ebersole <steve(a)hibernate.org>:
On Wed, Mar 25, 2015 at 4:57 PM, Gunnar Morling
<gunnar(a)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