Help with Hibernate 6.x upgrade for cascading obs test

Development
, ,
1

Hey all!

I am working on the hibernate 6.x and spring 6.x migration for core. There is one test that is failing that has me stumped so I thought another pair of eyes may help if anyone wants to take a look.

Here is my draft PR - TRUNK-6332: Hibernate 6.x and spring 6.x upgrade by k4pran · Pull Request #5062 · openmrs/openmrs-core · GitHub

The test is ObsServiceTest > shouldSaveUpdateDeleteVoidObsGroupCascades

It fails here with this

Caused by: org.h2.jdbc.JdbcSQLIntegrityConstraintViolationException: Unique index or primary key violation: "PUBLIC.CONSTRAINT_INDEX_13 ON PUBLIC.OBS(PREVIOUS_VERSION NULLS FIRST) VALUES ( /* 85 */ 82 )"; SQL statement:
insert into obs (accession_number,comments,concept_id,creator,date_created,date_voided,encounter_id,form_namespace_and_path,interpretation,location_id,obs_datetime,obs_group_id,order_id,person_id,previous_version,status,uuid,value_coded,value_coded_name_id,value_complex,value_datetime,value_drug,value_group_id,value_modifier,value_numeric,value_text,void_reason,voided,voided_by,obs_id) values (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,default) [23505-220]

Full logs - https://pastebin.com/fe7hGea6

A few observations I made is that after this line gets executed the oParent variable has an id of 82 and 4 group members with ids 83, 84, 85 and 86. In the migrated code obs 82 only has 83 and 84.

I stepped through the code and it looks like both versions are following the same path (from what I can tell), and I can see 85 and 86 being created (and they are even created with obsGroup reference to their parent 82).

When obs 82 hits this line and refreshes the obs, this is when the master code will have all 4 group members, but in the new code it remains only the 2 group members.

I haven’t noticed any docs or hibernate issues that explains it, so any ideas or help would be much appreciated!

1 Like
2

Hi @k4pran, for the failing test particularly, the error is occurring likely because ImmutableObsInterceptor is not being picked up automatically when using Hibernate 6, leading Hibernate to mistakenly attempt inserting a cloned obs (assuming it’s original is immutable while actually some fields prevent this action), with the same previous Obs value/reference (i.e., PREVIOUS_VERSION), violating a unique constraint.

You may want to adjust the relevant backwards compatible configuration to enable interceptors or, or better yet I guess, consider leveraging Hibernate 6 preferred use of event listeners (like PreInsertEventListener).

Have a look at the read below;

Hibernate provides event listener interfaces for intercepting entity lifecycle stages, including PreInsertEventListener and PostInsertEventListener. These listeners can be used instead of the now-deprecated Hibernate interceptors in v6 for intercepting DB operation events. — Hibernate v6 Event Interception

2 Likes
3

So what seems to be happening is something like this.

First, we create this deep tree of Obs that looks something like this (this is what I got running through; the numbers are slightly off yours, but I’ll be using these):

GGGP (#78)
   |
   -> GGP (#79)
       |
       -> GP (#80)
          |
          -> P (#81)
             |
             -> C1 (#82)
             |
             -> C2 (#83)
          -> S (#84)

We then change the text value of #82 and #83 to “testingUpdate” and call os.saveObs(oGGGP, "Updating obs group parent"). During the course of the save procedure, the Obs groups and sibling Obs (which were not changed) get shunted down this code path, but the updated Obs get shunted down this other one, producing Obs #85 and Obs #86, which are the replacements for #82 and #83 respectively.

#85 and #86 appear to get persisted to the database, but crucially, this does not seem to update the memory copy of #81 (or any of it’s grandparents), so what gets returned from the os.saveObs(oGGGP, "Updating obs group parent") is an object that still has the cached in memory data with non-voided copies of #82 and #83 and no knowledge of #85 and #86.

When we then void oGGGP, this simply voids the Obs in the in memory tree and persists them to the database. Then when we try to unvoid them (again using the same in-memory representation), this actually creates a new copy of all of the voided obs (so the whole tree), which leads to the conflict as the newly generated obs to replace #82 conflicts with the entry for obs #85.

All that said, I don’t see anything obvious. I’ve tried:

  • Just always refreshing obs groups after saving the group members, which doesn’t seem to do anything (by defaulting refreshNeeded in saveObsNotDirty to true)
  • Switching to actually using Hibernate cascading (modifying the hbm file)
  • Trying to add the new Obs to the group explicitly in saveExistingObs() (by calling `newObs.getObsGroup().addMember(newObs))
  • Trying to force Hibernate to reload the Obs either by calling ObsService.getObs() instead of returning the obs or calling Context.evictEntity() before saving.

Basically, yeah, this is a very annoying issue.

4

Thanks @ibacher! Nicely explained but there’s a caveat to this I guess. According to ImmutableObsInterceptor, this should not’ve occurred (considering that the actions, (un)voiding affect mutable fields)? And this is where I guess the issue is.

5

Thanks @ruhanga and @ibacher for helping look into this, I won’t get to look into it again for a few days but these are definitely helpful avenues to look into, and yes @ibacher this is the flow I was thinking they had taken so thanks for confirming. A few questions if you know the answers to them

  1. In master after the replacements get persisted (#85 and #86) the parent then has four in its group (#83 - #86). Is the expectation that #83 and #84 would still be part of the group but just voided (rather than completely replaced by (#85 and #86)?
  2. Do you know what mechanism is used to update the parent when its children are updated / replaced? Is it the hibernate cascading configuration or something custom?
  3. @ruhanga I had a quick check in master running the test and set breakpoints across ImmutableObsInterceptor but it never seemed to trigger any of the code in master neither. Do you know where and when it should trigger the interceptor in this specific test?
6

A good starting point might be this line in the code, where the interceptors are registered. It would help to confirm, with help of breakpoints, whether they are actually being registered and, if so, investigate why they might not be getting picked up during tests.

7

It wouldn’t have really helped. The voided field is one the fields that bypasses the immutable check (the list of mutable fields there is the list of fields that can be changed without any issue). And in the cases where we did change an immutable field (the obs value), a new obs is created; it just isn’t getting associated with the group correctly.

I actually don’t think we’d expect the ImmutableObsInterceptor to run in this workflow as it only implements the onFlushDirty() callback, which should only be triggered by implicit saves (i.e., where I’ve modified an object and the Hibernate session flushes it to the database); explicit saves (ultimately sessionFactory.saveOrUpdate()) are intended to be done through saveObs() run through the logic which ensures that if any immutable fields change, we create a new Obs.

Yes, that’s correct. The idea of Obs being “immutable” is so that we have a historical record of what the Obs value was at different points in time. So when voiding an Obs, we don’t want to detach it from the ObsGroup.

By default, we only rely on Hibernate to cascade deletes. Most of the updates to things are handled in either the service methods or the stuff in org.openmrs.aop. That’s why, for example, saveObsNotDirty() tries to cascade changes to Obs group members, etc.

1 Like
8

Oh, I see. Could it be that with Hibernate 6, you now have to explicitly assign the updated entity back to the variable, rather than relying on Hibernate to update it implicitly?

For example:

   oGGGPThatWasUpdated = os.voidObs(oGGGPThatWasUpdated, "testing void cascade");
   assertTrue(oGGGPThatWasUpdated.getVoided());
   // ...
   oGGGPThatWasUpdated = os.unvoidObs(oGGGPThatWasUpdated);
   assertFalse(oGGGPThatWasUpdated.getVoided());
   // ...
   oGGGPThatWasUpdated = os.voidObs(oGGGPThatWasUpdated, "testing void cascade");

Is this now a required pattern due to how entity state is managed in Hibernate 6?

9

@k4pran are all your changes included in that draft pull request? I pulled all the changes in the pull request and compiled successfully without any test failures.

10

I think I disabled that and one other test temporarily, just to avoid noise when I was testing builds etc, but yes my latest changes are pushed

1 Like
11

Just an update, seems that flushing the session before refresh fixes the test

	@Override
	public void refreshEntity(Object obj) {
		sessionFactory.getCurrentSession().flush();
		sessionFactory.getCurrentSession().refresh(obj);
	}

Still need to understand it better though, not sure where the change in behaviour came from

12

Hmm… This may be highlighting a week point in the failing unit test shouldSaveUpdateDeleteVoidObsGroupCascades() — that it is fairly large and involves multiple actions that would otherwise be scoped as their own units of work, which I think Hibernate is being sensitive to. I’'d think of rewriting the test by splitting these actions into smaller, focused test cases which would likely help mitigate the problem.

13

Have you modified FlushMode anywhere? If it’s set to MANUAL or NEVER the refresh will not be preceded by an automatic flush. I’d check getCurrentSession().getFlushMode() before calling refresh.

Regardless there’s nothing wrong with enforcing flush() before refresh() in refreshEntity.

14

No I never changed that, and I actually checked flush mode in master before refresh to compare and in both cases it is set to AUTO, I do think it is some behavioural change inside hibernate but can’t find it documented anywhere. You don’t foresee any side effects from flushing at this point then?

There is a difference in the java docs though

In hibernate 5.6

The Session is sometimes flushed before query execution in order to ensure that queries never return stale state.

In hibernate 6.4

The Session is flushed when EntityTransaction.commit() is called, and is sometimes flushed before query execution in order to ensure that queries never return stale state.

Not sure if that change has any relevance in this case though?

15

I think we should be good. You could add a condition if FlushMode is MANUAL or NEVER to not force flush before refresh to be in line with Hibernate docs. In most cases we do want to flush before refresh so we do not loose changes in a dirty object with old values from DB.

Hibernate 6 somehow decided that refreshing this entity is not in the same scope as changes scheduled for flushing. thus no flush needed in auto mode… I wouldn’t spend more time digging in Hibernate magic :wink:

2 Likes
16

I don’t think an explicit call to flush the session is necessarily a bad thing. I wonder if what we’re running into here is actually an issue in transaction management?

I noticed that Spring says “Hibernate ORM 6.x is only supported as a JPA provider (HibernateJpaVendorAdapter ). Plain SessionFactory setup with the orm.hibernate5 package is not supported anymore. We recommend Hibernate ORM 6.1/6.2 with JPA-style setup for new development projects.”

OpenMRS is pretty heavily and exclusively using the plain SessionFactory setup.

17

When I started this task I did begin going down this path but got into a bit of a mess with sessions and looked to see if we can gradually migrate without needing to switch to the JPA provider.

They did add it in spring 6 for compatibility with hibernate up to 6.1, I guess since this is 6.4 they don’t guarantee compatibility, but it doesn’t seem like the changes between 6.2-6.4 would break anything related to sessions unless its just this edge case?

18

Totally makes sense not to switch to the JPA provider version. That’s a much bigger update that would likely require changes to every DAO in the ecosystem. But it may be a reason that some extra steps are required in some cases.

From the issue you linked, it sounds like they switched to using an internal Hibernate API and I guess those can change without notice.