Yeah, I'm pretty sure that doing clearing and flushing around saves is going to cause much *more* strange behavior, not less.
(E.g. fetch a patient, save a new obs for them, then do patient.getPersonAttributes() and suddenly get a LazyInitializationException because the save caused a clearSession.) -Darius On Mon, Nov 28, 2011 at 12:16 PM, Burke Mamlin <[email protected]>wrote: > Unfortunately, we're highly dependent on hibernate magic at this point… it > will probably take a v2.0 or v3.0 of the API to free ourselves from the > hibernate magic. > > Ideally, hibernate magic would only exist under the hood of the API and > not be exposed to clients consuming the API, so we could guarantee that > changing something would only be persisted when/if you saved the change. I > believe in the hibernate world, it would mean detaching objects passed out > from the API and making consumers use the API instead of lazy loading to > get less commonly used properties of objects. > > We could experiment with it and it would be great if it worked well, but > I'm concerned that using flushes to try to coerce hibernate persistence to > behave as if objects are detached outside of the API (when they're really > attached) won't work predictably and will end up reducing hibernate's > efficiency in the process. > > -Burke > > On Mon, Nov 28, 2011 at 2:36 PM, Ben Wolfe <[email protected]> wrote: > >> That is (somewhat) known behavior that just rarely comes up. >> >> The saveOrUpdate calls just add to the objects currently in the session. >> >> Closing a session forces a flush to the db. >> Doing a read forces a flush to the db. (to prevent stale data) >> Closing a session/transaction forces a flush. >> >> Doing a clearSession before any of these flushes will cause all >> previously attached objects to not be persisted. >> >> Perhaps a big change to the API we could make is to put a flushSession >> call after each save method? That makes more sense in my mind. >> >> In fact, we might want to put a clearSession right before it: >> >> clearSession(); >> sessionFactory.saveOrUpdate(object); >> flushSession(); >> >> This would prevent other object changes from flushing when we didn't want >> them to...a long-standing "bug" that we've just lived with. >> >> Ben >> >> >> On Mon, Nov 28, 2011 at 7:38 PM, Mark Goodrich <[email protected]> wrote: >> >>> Burke—**** >>> >>> ** ** >>> >>> Good point… yes, as it turns out it looks like although the reference is >>> being changed, the new timestamp is a clone of the old timestamp.**** >>> >>> ** ** >>> >>> I dug in a little deeper and found something else… reading up on >>> hibernate, it seems that the saveOrUpdate method should do **nothing** >>> to objects that are currently persisted within the current session. It >>> only persists new objects (save) or reattachs objects that have been >>> deattached (update).**** >>> >>> ** ** >>> >>> This would mean in the second test script (I’ll copy it below for >>> convenience), the saveEncounter—which calls the underlying saveOrUpdate via >>> the dao--really shouldn’t do anything.**** >>> >>> ** ** >>> >>> public void editingExistingEncounter_shouldUpdateDateCreated() throws >>> Exception { >>> EncounterService es = Context.getEncounterService(); >>> >>> // First, load an existing Encounter >>> Encounter enc = >>> Context.getEncounterService().getEncounter(3); >>> >>> // confirm that the date changed is null >>> assertNull(enc.getDateChanged()); >>> >>> // Now edit the obs and resave the encounter >>> for (Obs o : enc.getAllObs()) { >>> o.setValueNumeric(75d); >>> } >>> >>> // resave the encounter >>> es.saveEncounter(enc); >>> >>> // PASSES: confirm that the date changed has been set >>> assertNotNull(enc.getDateChanged()); >>> } >>> >>> **** >>> >>> ** ** >>> >>> So I did a little more tracing and it turns out that the two flushes >>> that occur when you call saveEncounter **are not** caused by >>> sessionFactory.getCurrentSession().saveOrUpdate(encounter); in the dao. >>> The flushes occur only occur when DAO queries are made during the >>> execution of the saveEncounter method (they happen during the dao calls >>> made at EncounterServiceImpl:100 and EncounterServiceImpl:102). So it >>> seems that the flush that is caused by calling saveEncounter is >>> coincidental and not intentional.**** >>> >>> ** ** >>> >>> To test this out further I set out to test a save method that that was >>> simpler (and therefore might not coincidentially cause a flush) and see if >>> I could get it to fail.**** >>> >>> ** ** >>> >>> I took the following unit test:**** >>> >>> ** ** >>> >>> *public* *void*saveEncounterType_shouldUpdateAnExistingEncounterTypeName() >>> *throws* Exception {**** >>> >>> EncounterService encounterService = Context.* >>> getEncounterService*();**** >>> >>> **** >>> >>> EncounterType encounterTypeToChange = >>> encounterService.getEncounterType(1);**** >>> >>> **** >>> >>> // change the name of the type**** >>> >>> encounterTypeToChange.setName("another test");**** >>> >>> **** >>> >>> // save the type to the database**** >>> >>> encounterService.saveEncounterType(encounterTypeToChange); >>> **** >>> >>> **** >>> >>> // make sure the encounter type id didn't change**** >>> >>> *assertEquals*(1, >>> encounterTypeToChange.getEncounterTypeId().intValue());**** >>> >>> **** >>> >>> // *refetch* the encounter type from the database**** >>> >>> EncounterType fetchedEncounterType = >>> encounterService.getEncounterType(1);**** >>> >>> *assertTrue*(fetchedEncounterType.getName().equals("another >>> test"));**** >>> >>> }**** >>> >>> ** ** >>> >>> I traced this test and discovered that an onDirtyFlush is never called >>> during its execution.**** >>> >>> ** ** >>> >>> So I tested commenting out the “encounterService.saveEncounterType” >>> line in the above test, and, as I suspected, the test still passed.**** >>> >>> ** ** >>> >>> But, if I keep the saveEncounterType method but add a >>> Context.clearSession() after it **the test fails**.**** >>> >>> ** ** >>> >>> My conclusion here is that in this test the change is never committed to >>> the database (of course we are dealing with a mock db, which may affect >>> things), and the only reason the test passes is because hibernate checks >>> its internal session cache when refetching the encounter. Is this known >>> behavior that I didn’t completely understand, or have we all been missing >>> this?**** >>> >>> ** ** >>> >>> Mark**** >>> >>> ** ** >>> >>> ** ** >>> >>> ** ** >>> >>> ** ** >>> >>> ** ** >>> >>> *From:* [email protected] [mailto:[email protected]] *On Behalf Of *Burke >>> Mamlin >>> *Sent:* Monday, November 28, 2011 9:34 AM >>> >>> *To:* [email protected] >>> *Subject:* Re: [OPENMRS-DEV] Auditable Interceptor functionality**** >>> >>> ** ** >>> >>> Mark,**** >>> >>> ** ** >>> >>> Thanks so much for this investigating. It was a perfect addition to a >>> plate of turkey and a glass of Australian shiraz. :-)**** >>> >>> ** ** >>> >>> At the end, when you say "it is odd that dateCreated is being changed", >>> you mean that the dateCreated *reference* has changed, not the value, >>> right? Presumably because hibernate is doing some cloning or similar >>> trickery under the hood?**** >>> >>> ** ** >>> >>> -Burke**** >>> >>> On Thu, Nov 24, 2011 at 9:39 AM, Mark Goodrich <[email protected]> >>> wrote:**** >>> >>> No, even after flushing the session, the assert still fails. >>> >>> There's only one intercepted method in the AuditableInterceptor... the >>> onDirtyFlush. The interceptor methods are called on an object-by-object >>> basis. I added a dummy onSave method to the interceptor and debugged and >>> found the following: >>> >>> 1 )After the first saveEncounter in the editNewEncounter method: >>> The onSave method is called three times, once for the Encounter, and >>> once for each of the two Obs added >>> The onDirtyFlush method is not called >>> >>> 2) After the second saveEncounter in the editNewEncounter method: >>> The onDirtyFlush method is called four times, twice for each of the two >>> edited Obs >>> The onSave method is not callled >>> >>> 3) After the only saveEncounter in the editExistingEncounter method: >>> The onDirtyFlush method is called six times, twice for the Encounter, >>> and twice for the two edited Obs >>> The onSave method is not called >>> >>> I looked a little deeper into #3 to see exactly why the onDirtyFlush >>> method was being called to hopefully determine why it wasn't being called >>> in case #2. >>> >>> First, during the saveEncounter in the editExisingEncounter method, >>> onDirtyFlush is called with the Encounter, because three encounter fields >>> have changing: >>> encounterDate and dateCreated are both changing from an existing >>> timestamp object to a new timestamp object >>> voidReason is changing from and empty string to null >>> >>> Next, onDirtyFlush is called for each of the two Obs, with the following >>> fields changing: >>> obsDatetime and dateCreated are both changing from an existing timestamp >>> object to a new timestamp object >>> the valueNumeric is updated, as expected >>> the personId is updated from one Integer object to another Integer >>> object (though with the same integer value, thankfully!) >>> >>> Next, onDirtyFlush is called again with the Encounter. Now, six fields >>> have changed... the three fields mentioned previous (encounterDate, >>> dateCreated, and voidReason) all have the same previous and current values >>> as when the last dirty flush was called, and three new fields have changed: >>> voided from null to false >>> dateChanged from null to a timestamp >>> changedBy from null to the current user >>> (So it looks like this caused by the voided value change, or it is a >>> recursive call caused by the changes made in the AuditableInterceptor >>> itself.) >>> >>> Then, finally, onDirtyFlush is called again with the two obs, and >>> checking the current previous states show the exact same changes as the >>> first call. >>> >>> I don't know if I can come to any conclusions here except that it is odd >>> that dateCreated is being changed. One tangential point is that when obs >>> are updated this way (saved via a cascade from an encounter) they are not >>> voided and recreated, but I believe this is a known issue I entered a >>> ticket about at some point? >>> >>> I hope I've provided you all with some fun Thanksgiving reading. >>> >>> Mark >>> >>> >>> >>> ________________________________________ >>> From: [email protected] [[email protected]] On Behalf Of Ben Wolfe [ >>> [email protected]] >>> Sent: Thursday, November 24, 2011 7:40 AM >>> To: [email protected] >>> Subject: Re: [OPENMRS-DEV] Auditable Interceptor functionality**** >>> >>> >>> Do you know which method in the interceptor is called on that first save? >>> >>> Is the dateChanged set at the end of the transaction? If you flush the >>> session right before your failing assert, does the assert then pass? >>> >>> Ben**** >>> >>> On Thu, Nov 24, 2011 at 12:22 AM, Mark Goodrich <[email protected] >>> <mailto:[email protected]>> wrote: >>> Sorry I missed the design call on hibernate interceptors today, but, >>> coincidentally, I was investigating an issue with the Auditable interceptor >>> that I uncovered when running the HFE unit tests against 1.8. Or, rather, >>> something that may be an issue. >>> >>> Take the following unit test, which creates a new encounter, saves it, >>> updates the obs associated with it, and saves it again. Using the old save >>> handlers, the dateChanged of the encounter would be set upon the second >>> save. But, with the hibernate interceptor, this is not the case and so >>> this test will fail: >>> >>> public void editingNewEncounter_shouldUpdateDateCreated() throws >>> Exception { >>> EncounterService es = Context.getEncounterService(); >>> >>> // First, create a new Encounter >>> Encounter enc = new Encounter(); >>> enc.setLocation(new Location(1)); >>> enc.setEncounterType(new EncounterType(1)); >>> enc.setEncounterDatetime(new Date()); >>> enc.setPatient(new Patient(3)); >>> enc.setProvider(new Person(1)); >>> >>> // Now add a couple obs to it >>> Obs newObs = new Obs(); >>> newObs.setConcept(new Concept(1)); >>> newObs.setValueNumeric(50d); >>> enc.addObs(newObs); >>> >>> Obs anotherNewObs = new Obs(); >>> anotherNewObs.setConcept(new Concept(1)); >>> anotherNewObs.setValueNumeric(100d); >>> enc.addObs(anotherNewObs); >>> >>> // now save the encounter >>> es.saveEncounter(enc); >>> >>> // flush the session, just to be safe >>> Context.flushSession(); >>> >>> // confirm that the encounter has been assigned an id, but >>> the date changed is still null >>> assertNotNull(enc.getId()); >>> assertNull(enc.getDateChanged()); >>> >>> // confirm tha the obs have been added to the encounter >>> assertEquals(2, enc.getAllObs().size()); >>> >>> // Now edit the obs and resave the encounter >>> for (Obs o : enc.getAllObs()) { >>> o.setValueNumeric(75d); >>> } >>> >>> // resave the encounter >>> es.saveEncounter(enc); >>> >>> // FAILS: confirm that the date changed has been set >>> assertNotNull(enc.getDateChanged()); >>> } >>> >>> Note, however, that if I load an existing encounter and update the obs, >>> the date changed will be set: >>> >>> public void editingExistingEncounter_shouldUpdateDateCreated() throws >>> Exception { >>> EncounterService es = Context.getEncounterService(); >>> >>> // First, load an existing Encounter >>> Encounter enc = >>> Context.getEncounterService().getEncounter(3); >>> >>> // confirm that the date changed is null >>> assertNull(enc.getDateChanged()); >>> >>> // Now edit the obs and resave the encounter >>> for (Obs o : enc.getAllObs()) { >>> o.setValueNumeric(75d); >>> } >>> >>> // resave the encounter >>> es.saveEncounter(enc); >>> >>> // PASSES: confirm that the date changed has been set >>> assertNotNull(enc.getDateChanged()); >>> } >>> >>> In the first case the encounter is never passed to the interceptor >>> onDirtyFlush method... I would assume because the Encounter is still in >>> the new/save state? Is this a problem? This is a contrived case, and it >>> seems to make sense that if an encounter is saved twice in rapid succession >>> it's date changed isn't set--but this isn't the behavior I would have >>> expected, and it is different from how the old save handlers worked. >>> >>> Thoughts? >>> Mark >>> >>> >>> >>> _________________________________________**** >>> >>> To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to >>> [email protected]<mailto:[email protected]> with >>> "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail. >>> >>> [mailto:[email protected]<mailto:[email protected] >>> >?body=SIGNOFF%20openmrs-devel-l] >>> >>> ________________________________ >>> Click here to >>> unsubscribe<mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l> >>> from OpenMRS Developers' mailing list**** >>> >>> >>> _________________________________________ >>> >>> To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to >>> [email protected] with "SIGNOFF openmrs-devel-l" in the body >>> (not the subject) of your e-mail. >>> >>> [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l]**** >>> >>> ** ** >>> ------------------------------ >>> >>> Click here to >>> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from >>> OpenMRS Developers' mailing list >>> **** >>> >> >> ------------------------------ >> Click here to >> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from >> OpenMRS Developers' mailing list >> > > ------------------------------ > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > _________________________________________ To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to [email protected] with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail. [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l]
