Implement a base delegator ot help maintenance of out of tree implemento...

[Sanne]

...rs of Session

@emmanuelbernard, @sebersole, would you agree on adding something like this in ORM ?

It would greatly simplify the code of FullTextSessionImpl by isolating the simple delegate methods from those we actually need to override with specific logic.

Also, in case ORM changes some of the other 100 methods this would not affect Search compatibility.

Impact on Search would be as in:
Sanne/hibernate-search@012cae2
(proof of concept, but not working because of some classloading problem in AS7)

[emmanuelbernard]

@hferentschik yes it has to be in ORM because a new method added needs to be implemented in the delegator. The alternative is a separate project entirely but that seems overkill.

We can still do it in a few ways:

• add the class in hbernate orm core under spi or something and be done with it
• create a new module in hibernate orm which would contain one class

[sebersole]

In general I am not a fan of adding code to hibernate-orm that we KNOW we wont be using there, for lots of reasons. I'd say we break this down into different discussions:

1. Best way to allow swapping of custom Session implementations.
2. Breaking Session up into series of delegations.

Gavin and I started on #2 way, way back in the day. I think there is still room for more work there. Same (more so in fact) for SessionFactory. Even if we do not end up exposing the delegation through Session, we could do it in AbstractSessionImpl/SessionImpl. This may or may not help with #1. The reason I say it may not is that if you end up having to swap multiple delegations its probably more cumbersome in the end

I personally have a huge preference for delegation over inheritance, especially for customization....

[sebersole]

@emmanuelbernard My concern is not with where the class lives in terms of java package. Nor do I think that was Hardy's concern either. Its a question of writing and including a class in hibernate-orm that we know will never be USED IN hibernate-orm (read: never get properly tested within the scope of hibernate-orm).

[sebersole]

@Sanne lets start at a more basic level... why do you think this will make maintenance easier/better? I understand the delegation versus inheritance argument. Trust me, I get that. But at the end of the day you still have the same issue. Actually perhaps even a worse situation because it goes silently goes unnoticed. Take 4.0->4.1. We added new features to Session to handle loading by natural ids. For OGM at least you absolutely would need to override this behavior (for Search, maybe you can get away with not). And the worst part is you will never know that until much later if you miss it up front.

[Sanne]

silently unnoticed:

That's the whole point for Search, we add additional methods, and only a couple of the ORM methods need to be "adjusted" by overriding them.
For OGM we might not need it or decide not to use it.
If silently unnoticed is not desirable, then we need to always release in tandem. Maybe even dump all the source in the same repository and do releases together.

delegation

We do that in Search, and because of that we need to re-spin a release for every and each minor change in the SPI of ORM. I think we all appreciate delegation generally, and that's what my class does: if you take out the IDE-generated code, there is only a trivial constructor and two fields.

test coverage

The fact that the project compiles correctly means that all methods defined in the API are implemented. Having the Override annotation will get your attention also for removed methods.
Also, Search uses it so some functionality is covered by us: if it breaks in our tests, we'll have to make changes, but that's better to need to do changes every time: this is not to guarantee upgrades will always be flawless, just make it less likely to break compatibility Search v ORM.. so I see no reason to need tests as a bug in it won't affect your (direct) users.

Consider it part of Search if you want, more like a binary which is shipped by ORM to provide cross-version compatibility.

And the worst part is you will never know that until much later if you miss it up front.

Search has pretty good test coverage as well, we certainly would notice some mess-up if it's relevant to Search - if not, it's delegating straight to Core classes and we would not need to override, nor care to even know about the change ;)

maintenance vs runtime

This is not just to simplify our maintenance, which is not very complex in this regard. The real world problem is that people combine versions of ORM and Search which where not released on the same day, and even for support they'll have a different lifecycle. We should make sure that runtime compatibility is more stable than what it is.. this patch won't fix the current problem of Search 4.1 not working with both ORM 4.1.x and 4.0.x but should improve the odds in future.

[hferentschik]

My concern is not with where the class lives in terms of java package. Nor do I think that was Hardy's concern either. Its a > question of writing and including a class in hibernate-orm that we know will never be USED IN hibernate-orm (read: never > get properly tested within the scope of hibernate-orm).

+1 that's one concern. I am, however, also concerned about the name and location of the class. Neither name nor location imo give you a hint on what this class does.

I think Steve also has a very good argument regarding that with this delegation class you might get unnoticed problems.

Maybe we should first talk about other solutions (aka the two discussions Steve brought up) and generally release compatibility. Should the users be able to really mix and match versions. How far do we want to support this?

Also, how often do these contract really change? I think we are atm in a phase were a lot of things changed (and still might change) in ORM, but I think there will be an end to it soon (hopefully :-))

[Sanne]

Also, how often do these contract really change? I think we are atm in a phase were a lot of things changed (and still might change) in ORM, but I think there will be an end to it soon (hopefully :-))

Every time these classes change, you introduce a tight dependency between Search and ORM. That makes life for your users pretty complex, especially as you know Search is not distributed with the AS, while ORM is. Also people might want to get a minor upgrade of Search because of some issue, without needing mayor upgrades in ORM. Finally, we can't keep pace with maintaining a compatibility branch for each ORM version, that would be very time consuming for us.

As opposing to all your arguments, what does it cost ORM to have this class? I'd say almost nothing, it's trivial to maintain.

[emmanuelbernard]

On 12 mars 2012, at 14:17, Sanne Grinovero wrote:

Also, how often do these contract really change? I think we are atm in a phase were a lot of things changed (and still might change) in ORM, but I think there will be an end to it soon (hopefully :-))

Every time these classes change, you introduce a tight dependency between Search and ORM. That makes life for your users pretty complex, especially as you know Search is not distributed with the AS, while ORM is. Also people might want to get a minor upgrade of Search because of some issue, without needing mayor upgrades in ORM. Finally, we can't keep pace with maintaining a compatibility branch for each ORM version, that would be very time consuming for us.

As opposing to all your arguments, what does it cost ORM to have this class? I'd say almost nothing, it's trivial to maintain.

Right, it's not so much for users but we (the Hibernate team and some RHT guys) have spent a cumulated good 10 hours in the last 7 days and I'm sure the counter will increase by at least as much. We had similar headache in the past on the same issues and I expect it to happen in the future as well.

[stliu]

I would agree with @emmanuelbernard

[sebersole]

I think you are asking for just as much trouble with missing implementing methods that you actually should be implementing. Again, going back to Search and the notion of loading by natural-id, pretty sure that is functionality that could easy be hooked up into Search indexes. Yes apps will continue to work without Search hooking in there.

I guess the warning there is that you are missing out on the inherent opportunity to evaluate how new ORM Session features should be integrated (or maybe not at all) into Search. Instead all new features would silently get delegated. But if thats what y'all want, I'll include this. But I dont think it makes sense to integrate anyway prior to the initial 5.0 pre-releases.

[Sanne]

thanks Steve, I understand now why we were having this different view, there was a big misconception to be cleared: Hibernate Search does never attempt to "customize" or "extend" the Hibernate Session methods with custom behaviour.

In other words, If it would extend the Session (implementation) it would not override any method. That's not the way we expect it to work: we want users to be very clear on what they want to do, and only if invoking any Search specific method explicitly Search will "kick in". All the integration on ORM events is done via event listeners.

Hope you see now why we suggested this approach: we want to transparently miss any new method you might add: that's the purpose of this pull request.
If we would have been interested in evaluating carefully the need for changes in our code on Session (API) changes, then this approach I'm proposing would be very bad, I agree.

Of course I can't predict if in future we might not want/need to override a method, but it seems pretty unlikely and we never needed before; in case, I'll hope to notice. As noted before, this is not meant as the ultimate integration patch but just to make it more likely for different versions to work together.

Please integrate it whenever you like; the sooner the better, but it's not a problem to postpone to 5 if you prefer.

[sebersole]

See https://hibernate.onjira.com/browse/HHH-7992