[hibernate-dev] Redefining the API/SPI/implementation split

[Sanne Grinovero]

On 8 December 2014 at 10:13, Hardy Ferentschik <hardy at hibernate.org> wrote:
> Hi,
>
>> One of today's issues for Hibernate Search had the goal to move this class:
>>
>> org.hibernate.search.engine.spi.SearchFactoryImplementor
>
> Well, for me there is more to this than just moving. There are two issues imo.
>
> 1. The location. org.hibernate.search.engine.spi implies after our definition that it is
>    and public SPI. However, according to Sanne it should not be private and the main/sole
>    purpose of the class is to integrate the Search engine module with the orm module.

s/ it should not be private/ it should be private/

> 2. The name. SearchFactoryImplementor is something which implements SearchFactory. However,
>    one of the latest changes was to make SearchFactory a stand alone class of the orm module.
>    SearchFactory is now only available in the orm module and has not inheritance link anymore
>    to SearchFactoryImplementor or SearchIntegrator. This is awesome, since now we are able
>    to evolve the engine code in the direction for "free form" entities without affecting the
>    API for the users using Search in combination with Hibernate ORM. However, it also means
>    that the engine module should now be agnostic of the orm module. Having a SearchFactoryImplementor
>    in the engine module, but the SearchFactory defined in the orm module seems wrong.

Remember this still is "the implementor" for the SearchFactory API so
the name is not wrong; what is wrong is the position in terms of
modules.
So while I disagree on a rename I agree on moving its role: in fact
I've been working up to late tonight to move some things across
modules;
I had to back-off though as it's going to take me longer, and as long
as we're clear this is not an SPI we an change it at any point.

>
> So, what can be done? Similar to Emmanuel, I am against a internal/friend package. I think we should
> look at other things we can do.
>
> First off, I find the separation between SearchIntegrator (former SearchFactoryIntegrator) and
> SearchFactoryImplementor quite arbitrary. For example, the integrator contains getIndexBindings, whereas
> SearchFactoryImplemtor has a getIndexBindings. Why could the latter not be part of SearchIntegrator?
>
> If we move all methods of SearchFactoryImplementor into SearchIntegrator, and an integrator cannot
> implement all of them we could throw an OperationNotSupportedException.
>
> Other things we could do is to work with services. We could have something like a ORMInterationService which
> contains methods which are part of SearchFactoryImplemtor and which we don't want to move to
> SearchIntegrator.
>
> We could also keep SearchFactoryImplementor, but move it to the orm module. During bootstrapping in
> HibernateSearchSessionFactoryObserver we would create the SearchIntegrator and then wrapp/add the
> additional functionality required for the SearchFactoryImplementor.

+1 Makes a lot of sense but we need to postpone such internal
refactorings by a couple of weeks.

> I think we would have a whole range of possibilities to address this. Just moving the class it imo
> not one of them. If we say that we don't have time to a full fledged refactoring (which would imo
> be a shame, since that was one of the targets of Search 5), I'd rather just rename the class for now
> and do a refactoring later. We could rename and deprecate the class to make clear that it is not an
> SPI.

Exactly I just want to rename things to "unclassify" it from SPI.

>
>> So it seemed a straight-forward decision to move it to:
>>
>> org.hibernate.search.engine.impl.SearchFactoryImplementor
>
>> However then we need to patch the OSGi descriptor to export this package:
>> org.hibernate.search.engine.impl
>
> No, you cannot just move it to org.hibernate.search.engine.impl. If anything you would need to
> move it into a standalone impl package which does not contains any other class, say
> org.hibernate.search.engine.orm.impl. If you want to move the class, I'd rather do it this way
> than creating a 'friend' package.
>
> --Hardy