We should not have methods on our domain objects that remove a collection member

Tags: #<Tag:0x00007f828d69a1b0>

I was having a discussion with @wyclif about https://issues.openmrs.org/browse/TRUNK-4906 and I discovered that we have a convention in our API that I think is terrible: for our domain objects that have collection properties, we typically have a DomainObject.removeXyz(Member) method.

For example we have Concept.removeName(ConceptName) and Person.removeAddress(PersonAddress).

Since these methods remove an element from a hibernate-managed collection, they delete the underlying row from the database.

This is terrible because it allows some innocuous-seeming code to purge data from the database with no audit trail, and bypassing the typical permission check we’d have on a Service.purgeXyz method.

For example

Person person = service.getPerson(7);
PersonAttribute attr = person.getAttributes().iterator().next();
person.removeAttribute(attr);
service.savePerson(person);

…there is now one fewer row in the person_attribute table, with no audit trail, but I only had to pass a permission check for EDIT_PERSONS, and nothing about PURGE.

Our API should make it easy to soft-delete things (e.g. to void/retire them) but make it hard to purge things from the DB. In practice I can always do person.getAttributes().remove(xyz), but let’s not make it even easier and correct-looking to have an explicit method on our domain objects that hard-deletes things without an audit trail.

I propose that we deprecate the removeXyz methods on our domain objects. We should probably add a voidXyz or retireXyz method corresponding to each removeXyz that we’ll deprecate.

Deprecating removeXyz makes a lot of sense to me!

@darius what would be the value of adding voidXyz and retireXyz to the domain objects?

1 Like

IMHO if it’s a weak entity it makes sense to remove it from the collection (and EDIT permission should be enough). If it’s not then I agree, but in this case it shouldn’t be a collection :slight_smile:

Thanks @darius and all for raising this. Like @lluismf, I think we should make a judgement based on the type of data we are talking about. Purging a person attribute is akin to editing a person, isn’t it? Maybe it is a mistake to have a purge person attribute privilege in the first place, and simply use “edit person” privilege for this. I’d be interested to know if there are any practical examples where this level of granularity was a benefit and not a hindrance. It’s not like we have a “change gender” or “change birthdate” privilege. I do see how we would want to improve our audit trail of this, just like we should have a better audit trail of changing a patients gender or birthdate (which doesn’t currently exist). I would think this is a different problem to solve.

I’d also think that perhaps our solution to these issues would be to find a way to work with hibernate (eg. if we are stuck with this hibernate magic and are going to keep it exposed in our domain layer and not limited to our DAO, then work with that), rather than to fight it. We can detect changes via interceptors when hibernate is purging these entities from the database, and presumably we could do permission checks and auditing at that point. Most of our historical AOP (I think) has also already been converted to interceptors for much this reason.

Interested in your and other opinions on this…

Mike

How different is it from when you change a field on domain object which actually results into erasure of the old value from the DB with no audit trail? Much as am not so opposed to removing removeXXX methods from domain objects, I still think the business of voiding/retiring vs purging an entry in a child collection when removed is something that I think we should leave to the discretion of application/UI developers to decide. I think the verb is very direct and more intuitive given that the method is removeXXX(), as an API consumer I don’t want to remove something from a collection on a domain object and instead it gets voided when the verb is clearly saying remove and in any case the services in the API already have voidXXX and retireXX methods that are intended for this behavior and that has been the convention that long term API clients are used to otherwise changing it would lead to confusion. Also I don’t want to decentralize this void/retire logic from the service layer out into other areas in the code base because this is how things get messy.