New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Hibernate 5.2 #1871

Merged
merged 7 commits into from Feb 10, 2017

Conversation

Projects
None yet
7 participants
@vvondra
Contributor

vvondra commented Jan 7, 2017

  • fix deprecation notices where type isn't exposed on DW public API (AbstractDAO API doesn't change)
  • upgrade Jadira usertypes to compatible version
  • Hibernate removes hibernate-entitymanager with 5.2 and merges JPA support into core
  • one side effect of this is wrapping previous SQL exceptions in a PersistenceException, tests needed an additional ExceptionMapper to pass

@vvondra vvondra force-pushed the vvondra:hibernate_5.2 branch 4 times, most recently from 6c9f451 to edbee93 Jan 7, 2017

@coveralls

This comment has been minimized.

coveralls commented Jan 8, 2017

Coverage Status

Coverage remained the same at 81.743% when pulling edbee93 on vvondra:hibernate_5.2 into 67075b8 on dropwizard:master.

@arteam

This comment has been minimized.

Member

arteam commented Jan 9, 2017

Thank you for working on this! Great work, but I'm a bit reluctant to merge this pull request to the 1.1.0 branch. Hibernate 5.2 is actually an incompatible version with Hibernate 5.0/5.1, because the user code must adapted to move to this version (many public APIs removed and deprecated, behaviour changes). I think it would better from the Hibernate maintainers to call it Hibernate 6 according to semantic versioning, but I guess they have reasons for their release naming scheme. We have a lot of changes for 1.1.0 already, and I would like to provide an option for Hibernate users to upgrade to this version without spending time to rewrite their Hibernate code. I propose to schedule this change to Dropwizard 1.2 (maybe 2.0?), and for the time being upgrade the Hibernate module to the latest 5.1 version (5.1.3.Final).

@dropwizard/committers, What do you think?

@arteam arteam added the improvement label Jan 9, 2017

@vvondra

This comment has been minimized.

Contributor

vvondra commented Jan 9, 2017

@arteam Agreed on the naming/versioning from Hibernate, though we can't do much about that.

I see your concerns about Dropwizard versioning and I agree, for us updating to 1.0 took some time already and smaller steps are probably better. Feel free to keep this open as for revisiting later when the 1.2.x/2.0.x master branch is open and 1.1 is out or we close it and re-open, both are fine for me.

We've tried to upgrade and it's not that painful. For the main part, Hibernate 5.2 adds a lot of deprecations which have scheduled removal in v6. E.g. the new org.hibernate.query.Query still extends the old org.hibernate.Query for compatibility. This might be an argument to include 5.2 in DW 1.1 or 1.2 and then have Hibernate 6 for DW 2.0

I'll probably set this up as a package/bundle in the meantime on our side.

@jplock

This comment has been minimized.

Member

jplock commented Jan 9, 2017

@arteam I think that is a good idea. We should start creating 1.1.0 release candidates soon anyway.

@ryankennedy

This comment has been minimized.

Member

ryankennedy commented Jan 9, 2017

We've always struggled with how to handle/represent version intersections. Historically with the included modules we've simply said, "dropwizard-core X.Y only ships/works with dropwizard-module-foo X.Y, which uses foo version A.B." This has always meant that if people want to remain on an older version of foo they'd have to stay on an older version of dropwizard-core. Similarly, if they ever wanted to upgrade to a newer foo they'd have to upgrade to a newer version of dropwizard-core.

While this is a nice forcing function for upgrades (both for users to upgrade their dependencies and for us to keep our dependencies up to date), it causes obvious friction. I don't have a good solution for this other than to acknowledge it's a pain for both users and maintainers.

If everyone used semantic versioning properly, we could expand dependency definitions a bit and provide more flexibility for dependency versions. Unfortunately, being in the position we're in as framework maintainers, we pull in a broad swath of dependencies, each with their own feelings about how semantic they want their versioning to be.

Does anyone have examples of modular frameworks they feel are handling this scenario well that we might take inspiration from?

@tburch

This comment has been minimized.

Contributor

tburch commented Feb 2, 2017

Personally, I'd like to see this upgrade in 1.1.0.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Feb 8, 2017

I think one of the strongest arguments for including it in 1.1 is time. Based on previous trends, Dropwizard is on a release cycle of 8-10 months between major versions with bugfixes being released as needed in between. I'm counting dot releases (eg. ".9") as a major release. The time frame is not necessarily bad, as it means an equally as long support cycle. There is no set release schedule for future minor and major releases, so I can only assume minor releases will continue to be released on the same schedule with painful backwards incompatibility reserved for 2.0.

I can see how it would be frustrating to have a code change held back simply because it introduces a tangible amount of backwards incompatibilities. Especially if these backward incompatibilities are minimal. As a user of Dropwizard, I expect to evaluate every minor version (especially because of the infrequency of release) for updates necessary for my code. Dropwizard is glue code -- code that glues together high quality libraries. It would be infeasible to abstract away whole libraries to insulate the user from changes (and consequently features) to be compatible with semantic versioning.

I also find semantic versioning a rule for libraries and more of a guideline for frameworks.

Another library that is approaching a backwards incompatible change is JDBI, and the next version appears to be a major update (and arteam can speak more to this). This this would entail all JDBI users to put forth a relatively large amount of work to upgrade, this would be a good candidate for Dropwizard 2.0 (even if the JDBI code in Dropwizard doesn't change that much, it's the JDBI code in the users of Dropwizard that matters). In the meantime, we could have a community maintained "dropwizard-jdbi3" module for users that want jdbi3 features earlier than Dropwizard 2.0. I don't think it would make sense to have a "dropwizard-hibernate-52" module if the backwards incompatible changes are small.

At the end of the day, while Hibernate 5.2 does technically break backwards compatibility, but if it's a relatively minor change (and it can be documented) Hibernate 5.2 should be included in Dropwizard 1.1.

@vvondra vvondra force-pushed the vvondra:hibernate_5.2 branch 2 times, most recently from 3cedad5 to 9409980 Feb 9, 2017

vvondra added some commits Jan 7, 2017

Upgrade to Hibernate 5.2
 - fix deprecation notices where type isn't exposed on DW public API (AbstractDAO API doesn't change)
 - upgrade Jadira usertypes to compatible version

@vvondra vvondra force-pushed the vvondra:hibernate_5.2 branch from 9409980 to 12cc55a Feb 9, 2017

@vvondra vvondra force-pushed the vvondra:hibernate_5.2 branch from 12cc55a to 0af9d64 Feb 9, 2017

@coveralls

This comment has been minimized.

coveralls commented Feb 9, 2017

Coverage Status

Coverage remained the same at 84.651% when pulling 0af9d64 on vvondra:hibernate_5.2 into ebe6619 on dropwizard:master.

@arteam

arteam approved these changes Feb 9, 2017

Looks good, I've added a couple of minor comments.

@@ -84,7 +84,7 @@ public String persist(String entity) throws HibernateException {

private final SessionFactory factory = mock(SessionFactory.class);
private final Criteria criteria = mock(Criteria.class);
private final Query query = mock(Query.class);
private final org.hibernate.query.Query query = mock(org.hibernate.query.Query.class);

This comment has been minimized.

@arteam

arteam Feb 9, 2017

Member

I think we could also move to import org.hibernate.query.Query in AsbtractDAO instead of deprecated org.hibernate.Query.

@@ -117,11 +117,12 @@ protected Application configure() {
ImmutableList.of(Person.class));

try (Session session = sessionFactory.openSession()) {
session.createSQLQuery("DROP TABLE people IF EXISTS").executeUpdate();
session.createSQLQuery(
session.beginTransaction();

This comment has been minimized.

@arteam

arteam Feb 9, 2017

Member

I think if we open a transaction manually, it makes sense to commit manually too,

environment.jersey().register(new ConstraintViolationExceptionMapper());
}

private void initDatabase(SessionFactory sessionFactory) {
try (Session session = sessionFactory.openSession()) {
session.createSQLQuery(
session.beginTransaction();

This comment has been minimized.

@arteam

arteam Feb 9, 2017

Member

The same comment about the transaction. I think it would be more clear to commit it manually.

@@ -51,9 +52,10 @@ public void setUp() throws Exception {
sessionFactory = new SessionFactoryFactory()
.build(bundle, environment, dataSourceFactory, ImmutableList.of());
try (Session session = sessionFactory.openSession()) {
session.createSQLQuery("create table user_sessions (token varchar(64) primary key, username varchar(16))")
session.beginTransaction();

This comment has been minimized.

@arteam

arteam Feb 9, 2017

Member

The same comment about committing the transaction.

.executeUpdate();
transaction.commit();

This comment has been minimized.

@arteam

arteam Feb 9, 2017

Member

Great!

private static final Logger LOGGER = LoggerFactory.getLogger(DataException.class);

@Context
Providers providers;

This comment has been minimized.

@arteam

arteam Feb 9, 2017

Member

I think we could make the variable private, because it's not exposed outside of the class.

import javax.ws.rs.ext.Providers;

@Provider
public class PersistenceExceptionMapper implements ExceptionMapper<PersistenceException> {

This comment has been minimized.

@arteam

arteam Feb 9, 2017

Member

A pity we can't bundle this class to Dropwizard. The users who will migrate to 5.2 will forced to copy-paste it. But we could mention something about it in the migration docs.

This comment has been minimized.

@nickbabcock

nickbabcock Feb 9, 2017

Contributor

What do you mean by copy-paste? copy-paste the class name to register? Or copy-paste the contents?

If you're worried about informing users to register the exception mapper, maybe we can do something like we've done with other exception mappers (we define default ones that can be overrode)

This comment has been minimized.

@arteam

arteam Feb 9, 2017

Member

Copy-paste the contents. Unfortunately, previous exception mappers become invalid without it, because Hibernate changed its exceptions hierarchy.

@arteam

This comment has been minimized.

Member

arteam commented Feb 9, 2017

@nickbabcock, I think I'm going to agree with you. We always tried to maintain Dropwizard up-to date with the latest dependencies. In the end, pushing users to the latest dependencies reduces the burden of maintenance. Let's hope a move to a new Hibernate version would not be painful for our users.

@arteam

This comment has been minimized.

Member

arteam commented Feb 9, 2017

Regarding JDBI3, most of additional functionality in the jdbi2 module was incorporated into the library and there is no need for a special integration. I actually maintain dropwizard-jdbi3 module, but it's just a dozen lines of code.

@coveralls

This comment has been minimized.

coveralls commented Feb 9, 2017

Coverage Status

Coverage remained the same at 84.651% when pulling c08b811 on vvondra:hibernate_5.2 into ebe6619 on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented Feb 9, 2017

Coverage Status

Coverage remained the same at 84.651% when pulling c08b811 on vvondra:hibernate_5.2 into ebe6619 on dropwizard:master.

@vvondra vvondra force-pushed the vvondra:hibernate_5.2 branch from ca368c0 to 9f1c950 Feb 9, 2017

@vvondra

This comment has been minimized.

Contributor

vvondra commented Feb 9, 2017

@arteam I updated according to your comments + since we support the new generic Query interface, add support for JPA CriteriaBuilder as well.

Hibernate doesn't deprecate the Criteria object itself but the createCriteria methods on the session object. Since the criteria APIs are pretty different, I'd let AbstractDAO support both Criteria and CriteriaQuery<E>. All of it is just in the before last commit.

About the ExceptionMapper, there is one issue with it, that it failed before if the unwrapped exception didn't have a mapper. Jersey has a ExtendedExceptionMapper which allows you to check if you can trigger the mapper. That way the class would be re-usable and packagable among the default framework mappers

@vvondra vvondra force-pushed the vvondra:hibernate_5.2 branch from 9f1c950 to c237e2d Feb 9, 2017

Avoid NPE on PersistenceExceptionMapper
PersistenceExceptionMapper to only get triggered when the unwrapped
exception can be mapped to an error response
@coveralls

This comment has been minimized.

coveralls commented Feb 9, 2017

Coverage Status

Coverage increased (+0.01%) to 84.661% when pulling 4ab6395 on vvondra:hibernate_5.2 into 2675360 on dropwizard:master.

@arteam

This comment has been minimized.

Member

arteam commented Feb 10, 2017

Hibernate doesn't deprecate the Criteria object itself but the createCriteria methods on the session object. Since the criteria APIs are pretty different, I'd let AbstractDAO support both Criteria and CriteriaQuery. All of it is just in the before last commit.

Sounds feasible.

About the ExceptionMapper, there is one issue with it, that it failed before if the unwrapped exception didn't have a mapper. Jersey has a ExtendedExceptionMapper which allows you to check if you can trigger the mapper. That way the class would be re-usable and packagable among the default framework mappers.

Nice. Maybe we can figure out how to ship it within framework.

@arteam arteam merged commit cee6955 into dropwizard:master Feb 10, 2017

@arteam

This comment has been minimized.

Member

arteam commented Feb 10, 2017

@vvondra Thank you very much for your hard work on this issue!

@jplock jplock added this to the 1.1.0 milestone Feb 10, 2017

@vvondra vvondra deleted the vvondra:hibernate_5.2 branch Feb 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment