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
For example we have
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.
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.