[hibernate-dev] Various expectation changes in hibernate-core after consolidating hibernate-entitymanager

Mon Apr 25 11:36:33 EDT 2016

On 25 April 2016 at 16:28, Steve Ebersole <steve at hibernate.org> wrote:
> I agree with that to an extent.  However there is also a balance - clearly
> any added complexity has a direct correlation to likelihood of errors/bugs.
> We just have to find that balance :)

Of course :)
Just pointing this out as there was mention of "clean room" design.

>
> On Mon, Apr 25, 2016 at 9:52 AM Sanne Grinovero <sanne at hibernate.org> wrote:
>>
>> N.B. that chancing the exception types thrown by our native methods is
>> an API change in my opinion.
>>
>> So while I think you can have some Hibernate exceptions now extend
>> some of the types expected by JPA, a fully "clean room" approach is
>> not something I'd do in a minor release.
>>
>> On 25 April 2016 at 15:25, Steve Ebersole <steve at hibernate.org> wrote:
>> > I agree with Gunnar in that the underlying question here is in how we
>> > see
>> > the "Hibernate API" long term.  However I would consider the following
>> > categories:
>> >
>> > Methods which are Hibernate-only
>> > Methods which Hibernate had historically which share a signature with
>> > JPA
>> > Methods which we added to Hibernate specifically for JPA.
>> >
>> >
>> > Personally, in retrospect, I would say that group (3) unequivocally
>> > should
>> > have been handled by just throwing the JPA exceptions.  In a clean-room
>> > impl
>> > that's what I would do.
>> >
>> > So I guess the question is whether we want to look at this as a
>> > clean-room
>> > impl or if we want to constrain ourselves to the legacy exception types.
>> > I
>> > do wonder if a hybrid approach may be the best, as touched on before.
>> > Namely, where JPA expects an IllegalArgumentException,
>> > IllegalStateException, etc we throw those; for the rest we'd have
>> > Hibernate
>> > exception extend PersistenceException.  I think that's a great
>> > compromise.
>> >
>> > I'd also mention that in the IllegalArgumentException,
>> > IllegalStateException, etc cases there are a few places where
>> > HibernateExceptions cover the JPA case already.  E.g. JPA says that if
>> > there
>> > is a problem with the JPQL query (String) used to create a Query an
>> > IllegalArgumentException should be thrown; Hibernate currently handles
>> > this
>> > by throwing a QuerySyntaxException - we could just make
>> > QuerySyntaxException
>> > extend from IllegalArgumentException rather than Hibernate's
>> > HibernateException hierarchy.
>> >
>> > I think the bottom line is that having Session and EntityManager split
>> > before caused all kinds of difficult-to-debug complexity that I'd rather
>> > avoid as we consolidate them together.
>> >
>> >
>> > On Mon, Apr 25, 2016 at 5:37 AM Sanne Grinovero <sanne at hibernate.org>
>> > wrote:
>> >>
>> >> Gunnar's words seem wise to me: users will need to have the JPA API on
>> >> classpath anyway, so I don't see why we should have - and maintain -
>> >> strategies for different kind of exceptions.
>> >>
>> >> This might have been useful in the past, but looking forward?
>> >>
>> >> If the reasoning is that Hibernate - having more features - could
>> >> throw more specific and informative exceptions, we could have some
>> >> Hibernate exceptions to subclass the JPA ones?
>> >>
>> >> Would be nice to avoid breaking the expected exception types in a
>> >> minor, so I'm not sure if you can do that in all cases by subclassing
>> >> the JPA ones, but I suspect it can get you a long way.
>> >>
>> >>
>> >> On 25 April 2016 at 10:13, Gunnar Morling <gunnar at hibernate.org> wrote:
>> >> > The strategy approach sounds nice on first thought, but it also adds
>> >> > complexity.
>> >> >
>> >> > I think the underlying question is: What's the long-term strategy
>> >> > around
>> >> > the "Classic API"? Should it remain in place for all times as a
>> >> > complete
>> >> > alternative to the JPA API?
>> >> >
>> >> > Or should we begin to deprecate it and narrow it down in favour of
>> >> > the
>> >> > corresponding functionality standardized in JPA, and only
>> >> > functionality
>> >> > not
>> >> > present in JPA would be exposed through some kind of unwrap()?
>> >> >
>> >> > Without having thought about all the implications too much, I'd lean
>> >> > towards the latter, in which case I vote for "1. Just move to JPA
>> >> > expected
>> >> > exceptions" as part of such larger effort.
>> >> >
>> >> > It'd be interesting to run a poll to see some figures of people using
>> >> > classic vs. JPA.
>> >> >
>> >> > --Gunnar
>> >> >
>> >> >
>> >> >
>> >> > 2016-04-25 10:47 GMT+02:00 andrea boriero <andrea at hibernate.org>:
>> >> >
>> >> >> Having a Strategy gives us more flexibility  so +1.
>> >> >>
>> >> >> About the expectations I think what Vlad says is reasonable.
>> >> >>
>> >> >> On 25 April 2016 at 06:04, Vlad Mihalcea <mihalcea.vlad at gmail.com>
>> >> >> wrote:
>> >> >>
>> >> >> > Your second email summarizes my thoughts as well. If we can
>> >> >> > separate
>> >> >> > the
>> >> >> > exception handling in two separate strategies that are defined
>> >> >> > during
>> >> >> > bootstrap (JPA vs Hibernate),
>> >> >> > I think that's the way to go.
>> >> >> >
>> >> >> > There so many projects out there that rely on the exception type
>> >> >> > being
>> >> >> > thrown, and changing those would make it very difficult for them
>> >> >> > to
>> >> >> migrate
>> >> >> > to this new version.
>> >> >> > But that only affects Hibernate-native projects since, for those
>> >> >> > who
>> >> >> > have
>> >> >> > been using JPA, they already expect the JPA exceptions anyway.
>> >> >> >
>> >> >> > As for the other behavior discrepancies:
>> >> >> >
>> >> >> > 1. "calling EntityManager#close on a closed EntityManager should
>> >> >> > result
>> >> >> in
>> >> >> > an
>> >> >> > exception;" - that's a reasonable default and shouldn't cause too
>> >> >> > much
>> >> >> > trouble.
>> >> >> > 2. "Another change in expectation is in regards to operations
>> >> >> > outside
>> >> >> > of
>> >> >> a
>> >> >> > transaction" - in JPA we can execute queries outside a
>> >> >> > transaction,
>> >> >> > but
>> >> >> any
>> >> >> > write will fail if there is no transactional context, which is
>> >> >> > reasonable
>> >> >> > for me too. If Hibernate allows writes outside of a transactional
>> >> >> context,
>> >> >> > that's definitely a thing we should not support anyway.
>> >> >> > 3. "Asking a Session if is contains
>> >> >> > (Session/EntityManager#contains)
>> >> >> > a
>> >> >> >  non-entity" - we can handle this with the separate exception
>> >> >> > handler
>> >> >> > strategies to retain both JPA and Hibernate behaviors.
>> >> >> > 4. "Accessing Session/EntityManager#getTransaction.  JPA says that
>> >> >> > is
>> >> >> > only allowed
>> >> >> > for JDBC transactions.  Hibernate always allows it." - I'd choose
>> >> >> > the
>> >> >> > Hibernate behavior because I don;t see how it can cause any issue
>> >> >> > and
>> >> >> it's
>> >> >> > an enhancement as well.
>> >> >> >
>> >> >> > Vlad
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > On Sat, Apr 23, 2016 at 5:29 PM, Steve Ebersole
>> >> >> > <steve at hibernate.org>
>> >> >> > wrote:
>> >> >> >
>> >> >> > > Just realized that I should have mentioned an important plan
>> >> >> > > that
>> >> >> > > helps
>> >> >> > > understand the idea behind the "exception handling strategy"
>> >> >> > > route.
>> >> >> > > I
>> >> >> > plan
>> >> >> > > to keep track of how a SessionFactory was bootstrapped in some
>> >> >> > > fashion.
>> >> >> > So
>> >> >> > > when it was bootstrapped from EntityManagerFactoryBuilder (which
>> >> >> > > JPA
>> >> >> > > bootstrap methods leverage) we'd select the "JPA exception
>> >> >> > > handling"
>> >> >> > > strategy impl.  When not, we'd use the "legacy Hibernate
>> >> >> > > exception
>> >> >> > > handling" strategy.
>> >> >> > >
>> >> >> > > On Sat, Apr 23, 2016 at 9:21 AM Steve Ebersole
>> >> >> > > <steve at hibernate.org>
>> >> >> > > wrote:
>> >> >> > >
>> >> >> > > > There are a number of "expectation changes" that come about
>> >> >> > > > from
>> >> >> > > > consolidating hibernate-entitymanger into hibernate-core.
>> >> >> > > > Some
>> >> >> > > > we
>> >> >> have
>> >> >> > > > discussed; some we have not.  Hopefully we can come to a
>> >> >> > > > consensus
>> >> >> > > regards
>> >> >> > > > how to deal with these changes...
>> >> >> > > >
>> >> >> > > > The first one is different exception types.  We have discussed
>> >> >> > > > this
>> >> >> > > > before.  For now, in an effort to fix test failures and move
>> >> >> > > > forward
>> >> >> > with
>> >> >> > > > developing, I simply changed failing tests to expect the JPA
>> >> >> > > > defined
>> >> >> > > > exceptions.  I had mentioned, where possible, to to throw a
>> >> >> combination
>> >> >> > > of
>> >> >> > > > the 2 expected exceptions.  Generally this falls into 2
>> >> >> > > > discrete
>> >> >> > > categories:
>> >> >> > > >
>> >> >> > > >
>> >> >> > > >    1. JPA expects a PersistenceException and we have
>> >> >> > > > historically
>> >> >> > thrown
>> >> >> > > >    a HibernateException
>> >> >> > > >    2. JPA expects some form of JDK RuntimeException
>> >> >> > > >    (IllegalArgumentException, IllegalStateException, etc) and
>> >> >> > > > we
>> >> >> > > > have
>> >> >> > > >    historically thrown a HibernateException
>> >> >> > > >
>> >> >> > > > It is unfortunate that Java does not allow exceptions to be
>> >> >> > > > defined
>> >> >> by
>> >> >> > > > means of interfaces; that's the only "clean" way I see to do
>> >> >> > > > this
>> >> >> > > > -
>> >> >> > that
>> >> >> > > > would have allowed us to define concrete exception classes
>> >> >> > > > that
>> >> >> extend
>> >> >> > > > PersistenceException, IllegalArgumentException, etc and that
>> >> >> implement
>> >> >> > > HibernateException.
>> >> >> > > >
>> >> >> > > >
>> >> >> > > > So I see 3 potential solutions (feel free to bring up others).
>> >> >> > > >
>> >> >> > > >    1. Just move to JPA expected exceptions.
>> >> >> > > >    2. Have HibernateException extend PersistenceException and
>> >> >> > > > just
>> >> >> not
>> >> >> > > >    worry about the change in expectation in regards to that
>> >> >> > > > second
>> >> >> > > category.
>> >> >> > > >    3. Push exception handling behind a strategy.  This would
>> >> >> > > > have
>> >> >> > > > to
>> >> >> > be a
>> >> >> > > >    pretty specific strategy for very specific cases.
>> >> >> > > >
>> >> >> > > > The first and second options are pretty self-explanatory and
>> >> >> > > > straight-forward so I won't go into detail there.  Just
>> >> >> > > > realize
>> >> >> > > > that
>> >> >> > > these
>> >> >> > > > change the expectation for the user.  They'd have to change
>> >> >> > > > their
>> >> >> code
>> >> >> > to
>> >> >> > > > catch these JPA-defined exceptions.
>> >> >> > > > The other option, I see, is to h
>> >> >> > > >
>> >> >> > > > The third option is perfect in theory, but it is very tedious.
>> >> >> > > > For
>> >> >> > > > example, take the case of trying to perform some operation on
>> >> >> > > > a
>> >> >> closed
>> >> >> > > > Session/EntityManager.  Hibernate historically threw a
>> >> >> > HibernateException
>> >> >> > > > here.  JPA says that should result in an
>> >> >> > > > IllegalStateException.
>> >> >> > > > So
>> >> >> in
>> >> >> > > > SessionImpl#checkOpen, when the Session/EntityManager is
>> >> >> > > > closed,
>> >> >> > > > we'd
>> >> >> > > > call out to the strategy to handle that condition.  Even more,
>> >> >> > Hibernate
>> >> >> > > > (historically) and JPA disagree about which methods getting
>> >> >> > > > called
>> >> >> on a
>> >> >> > > > closed Session/EntityManager should lead to an exception.  For
>> >> >> example,
>> >> >> > > > JPA says calling EntityManager#close on a closed EntityManager
>> >> >> > > > should
>> >> >> > > > result in an exception; Hibernate historically did not care if
>> >> >> > > > you
>> >> >> > called
>> >> >> > > > Session#close on a closed Session.  So that is a special case,
>> >> >> > > > and
>> >> >> > every
>> >> >> > > > one of those special cases would have to be exposed and
>> >> >> > > > handled
>> >> >> > > > in
>> >> >> the
>> >> >> > > > exception handling strategy in additional to the general
>> >> >> > > > cases.
>> >> >> > > >
>> >> >> > > > Another change in expectation is in regards to operations
>> >> >> > > > outside
>> >> >> > > > of
>> >> >> a
>> >> >> > > > transaction, which I consider a questionable pattern anyway.
>> >> >> Hibernate
>> >> >> > > > historically allowed that; JPA explicitly disallows it.  In a
>> >> >> > > > way
>> >> >> this
>> >> >> > > > could fall under the exception discussion above, meaning we
>> >> >> > > > could
>> >> >> push
>> >> >> > > that
>> >> >> > > > distinction behind the exception handling strategy.  Or we
>> >> >> > > > could
>> >> >> decide
>> >> >> > > > that we are going to stop supporting that.
>> >> >> > > >
>> >> >> > > > There are a lot of other highly questionable things I have
>> >> >> > > > seen
>> >> >> > > > in
>> >> >> the
>> >> >> > > > tests that JPA explicitly disallows that I think we ought to
>> >> >> > > > just
>> >> >> stop
>> >> >> > > > supporting and opt for the JPA way, although I am open to
>> >> >> > > > discussing
>> >> >> > them
>> >> >> > > > if any feels strongly about them.  Some of these include:
>> >> >> > > >
>> >> >> > > >    - Asking a Session if is contains
>> >> >> (Session/EntityManager#contains) a
>> >> >> > > >    non-entity.  Hibernate historically would just return
>> >> >> > > > false.
>> >> >> > > > JPA
>> >> >> > > states
>> >> >> > > >    that should be an exception.
>> >> >> > > >    - Accessing Session/EntityManager#getTransaction.  JPA says
>> >> >> > > > that
>> >> >> is
>> >> >> > > >    only allowed for JDBC transactions.  Hibernate always
>> >> >> > > > allows
>> >> >> > > > it.
>> >> >> > > >
>> >> >> > > > If we go the route of an "exception handling strategy" a lot
>> >> >> > > > of
>> >> >> > > > the
>> >> >> > other
>> >> >> > > > points I mentioned above could just be pushed behind that
>> >> >> > > > strategy.
>> >> >> > But
>> >> >> > > I
>> >> >> > > > really want to start looking critically at what we support
>> >> >> > > > today
>> >> >> > > > that
>> >> >> > we
>> >> >> > > > maybe really should not be.
>> >> >> > > >
>> >> >> > > _______________________________________________
>> >> >> > > hibernate-dev mailing list
>> >> >> > > hibernate-dev at lists.jboss.org
>> >> >> > > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>> >> >> > >
>> >> >> > _______________________________________________
>> >> >> > hibernate-dev mailing list
>> >> >> > hibernate-dev at lists.jboss.org
>> >> >> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>> >> >> >
>> >> >> _______________________________________________
>> >> >> hibernate-dev mailing list
>> >> >> hibernate-dev at lists.jboss.org
>> >> >> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>> >> >>
>> >> > _______________________________________________
>> >> > hibernate-dev mailing list
>> >> > hibernate-dev at lists.jboss.org
>> >> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>> >> _______________________________________________
>> >> hibernate-dev mailing list
>> >> hibernate-dev at lists.jboss.org
>> >> https://lists.jboss.org/mailman/listinfo/hibernate-dev