Core: Cannot purge anymore a complex obs that has lost its complex data.

Development
,
1

I’d be curious to get people’s opinion on a case we are stumbling upon.

When one wants to get rid of an obs there are basically two options offered by ObsService:

  1. Voiding the obs.
  2. Purging the obs.

My understanding is that the best practice is to void obs, so that it can be undone. However when it comes to complex obs, voiding does not get rid of the complex data. I’m sure that this was intended so that unvoiding a complex obs would bring things back to where they were before, with the complex data being still available. In VDUI we decided that when the user deletes a visit document, the underlying file should go. The main reason behind this choice is because of the cost of storage. We didn’t want to void complex obs and let the complex_obs dir grow because of ghost files. But then purging can’t be done anymore if the complex data is ‘lost’:

if (!purgeComplexData(obs)) {
 throw new APIException("Obs.error.unable.purge.complex.data", new Object[] { obs });
}

In that case the only option is to void the obs, leading to a mixed situation where most deleted obs are just entirely gone, and where some problematic obs are left voided since they could never be purged. Not a huge deal in the context of our module, but inconsistent nonetheless.

What would help us and let us the flexibility to handle all scenarios, would be to expose purgeComplexData(Obs) as public. Thoughts?

2

I feel that the ability to undo a voided complex obs is so useful that i would not think space is such a big deal, especially looking at the low cost for storage. :slight_smile:

Although the above does not prevent us from fixing the bug where a complex obs that has lots its complex data cannot be purged.

3

Regarding a fix, how would you approach this? I personally would almost need a new purge method that doesn’t look at the non-DAO part of the obs, so basically one flavor of it that goes straight to the DAO layer, like this:

public void purgeObsDao(Obs obs, boolean cascade) throws APIException {
 if (cascade) {
  throw new APIException("Cascading purge of obs not yet implemented");
 }
 dao.deleteObs(obs);
}

public void purgeObs(Obs obs, boolean cascade) throws APIException {
 if (purgeComplexData(obs) == false) {
  throw new APIException("Unable to purge complex data for obs: " + obs);
 }
 purgeObsDao(obs, cascade);
}

Would that be an acceptable solution?

P.S. The cost of storage is high actually, or high enough. I am not talking about physical hard drives themselves but rather about the adequate maintenance work that requires to take care of all sort of redundancies when you are in charge of the actual servicing. Generating many complex obs makes it all quite a lot more complicated and cumbersome. System snapshots and transfers take longer… etc. Also when using something like VDUI, adding files is very simple and friendly, you just drag and drop and it’s done, or you capture images via the device camera on the fly. Believe me, the storage usage increases fast.

4

I created TRUNK-5077 for this: ‘ObsService to allow DAO-only obs purge’. I will submit a PR soon on 1.11.x first.

5

I think purgeComplexData() was meant to return false if anything went wrong to prevent deletion of the complex obs data or file. In cases where the file is already deleted, i think it would be fair enough to just return true, such that the rest of the method continues.

6

I am also unconvinced by TRUNK-5077. There should be a way to do this without exposing so much of the implementation internals. Maybe you’re getting at a “force” flag on the purge method?

Also, why are there “ghost files”? That seems like the actual problem. I would think that if they try to purge a complex obs, which refers to a file that is lost (maybe already deleted?) then we should just allow the purge to proceed.

7

I don’t seem to understand what exactly TRUNK-5077 is trying to solve, I would think that when you purge an Obs, the complex data file needs to be deleted from the file system unlike for voiding, in any case neither the API nor the DB provide a way to undo a purge.

8

I’m with Darius, if the obs file does not exist (somebody has deleted the file from outside OpenMRS) just delete the obs from the DB.

9

Hi all, thanks for your input,

I have reworded and changed the issue to ‘Add a forcePurge parameter to ObsService#purgeObs.’

For some reason AbstractHandler does not increment the name of “same titled files” with a suffix higher than 99 (two digits). See here. Now the nightmare scenario. Imagine a device that sends its captured images always named as ‘image.jpg’. When the 101th picture is sent, AbstractHandler returns an outputfile pointing to ‘image_99.jpg’… for the second time. This confuses the parent handler in believing that it is safe to write there again, leading to a file override and a ghost file for obs #100 (the obs behind the 100th ‘image.jpg’.) I don’t know how many files our client lost like that (maybe 20) before we urge them to stop capturing images with that device. We worked around this by ensuring that VDUI always sends out unique file names from its client-side. We had created swiftly an internal issue about AbstractHandler’s behaviour but ended up pushing a hot fix on VDUI first, then we moved to other things and the actual bug got a little forgotten. I will raise the original issue on TRUNK as well of course. In fact TRUNK-5077 represents an effort to allow VDUI to clean up what was once produced by the original issue.

10

@mksd if i were you, i would not introduce a new method with that forcePurge parameter. I would instead just ensure that the existing purgeComplexData() returns true if the file does not exist. :slight_smile:

11

So you would want purgeComplexData(Obs) to catch children of APIException and act accordingly? Like this:

protected boolean purgeComplexData(Obs obs) throws APIException {
  boolean success = true;

  if (obs.isComplex()) {
    ComplexObsHandler handler = getHandler(obs);
    if (null != handler) {
      
      try {
        success = handler.purgeComplexData(obs);
      }
      catch (ComplexDataFileNotFoundException notFoundEx) {
        success = true;
      }
      catch (APIException apiEx) {
        success = false;
      }
      
    }
    else {
      success = false;
    }
  }
  
  return success;
}

So when catching “acceptable” APIException children that have been singled out as ok cases we would return true, otherwise false.

12

The original code returned true when the handler was not found.

13

Ok :wink:

Anyway, if that’s sufficient, I think I’m just going to change the behaviour of AbstractHandler#purgeComplexData(Obs). I will ensure that it returns true when the file is not found.

14

I reworded the issue as ‘AbstractHandler#purgeComplexData(Obs) should return true when file is missing from disk’. If the file is missing from disk then the purge is already successful before even starting, the handler should then return true.

15

Then the solution is to change the way the file name is built, with a timestamp or a UUID, and forget the counters. Even better would be to use a CMS instead of the file system. There’s even a standard API Content repository API for Java - Wikipedia

16

For everyone’s info I (finally) created this:

TRUNK-5093: ‘AbstractHandler allows blind rewriting of existing complex obs file’.

17

Hi @flobue, if necessary we can discuss TRUNK-5093 here, but overall I think all that is needed is in JIRA. Thanks for looking into it!

18

Hello @mksd! For now I think TRUNK-5093 has enough information for me to start, but as I mentioned, I’d like the other ticket I’ve worked on to be finished first, since it overlaps with this one.

For the time being, I will not assign the ticket to myself in case anybody wants to take it before I get to it. I hope I can start working on it soon though.

1 Like