Lets step back and be specific about certain endpoints.
DELETE /api/geographicZones/{id}
To answer the question:
“what happens to the facilities that are listed as being in the GeoGraphic zone, when we delete the zone?”
This should have been asked when we implemented the DELETE endpoint. Or when we associated the Facility with the GeographicZone. Right? Was it asked? I could expound on this however we all know the answer, and the answer is that we failed when implementing DELETE /api/geographicZone/{id}. If we didn’t ask the question, why was it even implemented? I can and I will define the current 500 error an indicator that we didn’t probe for and understand the requirements when we implemented the endpoint. I’ll also say I’m glad it does return a 500, as it’s catching a violation of a known requirement: facilities are associated with a geographic zone. Lets not remove that until we know what should be done.
Luckily we’re talking about an edge case of an administrative function, so if 3.0 ships with this we can mark it as our first minor bug and then finally ask the correct “business logic” question - what should be done if we “delete” a geographic zone that has facilities attached.
Lets get to that, however for now lets answer the following:
- How many endpoints implement the DELETE operation?
- How many of those endpoints implement DELETE like /api/geographicZones/{id} ? i.e. lead to 500 level responses?
- Are there other endpoints, which implement DELETE, which likely wouldn’t lead to a 500 level response, which we’re concerned about? What about DELETE /api/requisitions/{id} ?
I did a quick survey of the Reference Data service and feel pretty comfortable saying that all 17 DELETE operations are suspect.
Regardless of the approach for deleting/archiving that we’ll eventually want, I’m pretty certain that 3.0 is going to ship at this point either with DELETE endpoints that hopefully will return HTTP 500 when a known business constraint is violated (e.g. a facility being associated), or we’ll decide to remove these endpoints and address the functional need in a later release.
For reference data, I’m leaning toward removing these endpoints / marking them as “not to be used”. Component leads, please answer these three questions for your service.
Thank you Sebastian for bringing this up. This is important to solve, though of course you can see I have concerns about how we ended up with all these DELETE endpoints - especially in reference data.
Best,
Josh
Josh Zamor|josh.zamor@villagereach.org
*Senior Software Development Engineer, OpenLMIS Architect
Village****Reach ** Starting at the Last Mile*
2900 Eastlake Ave. E, Suite 230, Seattle, WA 98102, USA
DIRECT: 1.206.512.1501 CELL: 1.847.504.7262 FAX: 1 206.860.6972
SKYPE: josh.zamor.vr
www.villagereach.org
Connect on Facebook, **Twitter ** and our** Blog**
···
sbrudzinski@soldevelo.com
On 25.01.2017 18:44, Josh Zamor wrote:
Hi Sebastian. I’m of the opinion that in general DELETE shouldn’t delete anything, but rather mark it as deleted/archived.
There are some simple positives:
- we don’t have the referential integrity issue you described within a service
- AND any client to a service that does a GET /resource/{id} will always return the resource once it’s created - that is it wouldn’t return a 200 on day 1, and on day 2 after it’s deleted suddenly return a 404 to the client. More on this later…
Some cons:
- since we never delete, we always accumulate data. But DELETE is IMO an edge case and storage is cheap.
- We haven’t followed this approach so far, however… It’s true that clients of a GET /resource/{id} might not be looking for some “archived” flag of the resource returned and this change would need to be accounted for. However that change I think is more modest when we think about how clients discover a resource: search. For most of our clients, they’ll be using some search endpoint first to find the resources before the first GET /resource/{id} is ever used. The convention for a search endpoint therefore could be that archived resource items are not returned, unless a query parameter (e.g. ?archived=true) is given.
Leaving it as a 500 error isn’t something we should do. Thoughts?
Best,
Josh
Josh Zamor|josh.zamor@villagereach.org
*Senior Software Development Engineer, OpenLMIS Architect
Village****Reach ** Starting at the Last Mile*
2900 Eastlake Ave. E, Suite 230, Seattle, WA 98102, USA
DIRECT: 1.206.512.1501 CELL: 1.847.504.7262 FAX: 1 206.860.6972
SKYPE: josh.zamor.vr
www.villagereach.org
Connect on Facebook, **Twitter ** and our** Blog**
On Jan 25, 2017, at 7:58 AM, Sebastian Brudziński sbrudzinski@soldevelo.com wrote:
Hello everyone,
Today I’ve been investigating OLMIS-1705 that talks about DELETE endpoints not working. The problem with the delete is that it may affect the not-null constraint in another entity, in which case the resulting SQL query will fail. This is wrapped into the DataIntegrityViolationException. As an example, attempting to delete a geographic zone will fail if there’s any facility that is currently linked to that geographic zone (because Facility requires non-null geographic zone).
I’ve been wondering about the approach we want to take to handle those errors. What currently happens is that the delete throws DataIntegrityViolationException, which is translated into 500 error code. We could try to implement manual validations before the actual delete happens, but we’ve got quite a lot of the not-null constraints scattered across the services and having to check all of the instances of let’s say 10 entities upon delete somehow doesn’t seem like a good solution. Any ideas for other approaches to tackle this? Or should we just leave it be 500 error code?
Best regards,
Sebastian.
–
You received this message because you are subscribed to the Google Groups “OpenLMIS Dev” group.
To unsubscribe from this group and stop receiving emails from it, send an email to
openlmis-dev+unsubscribe@googlegroups.com.
To post to this group, send email to
openlmis-dev@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/openlmis-dev/5888CB0B.6050305%40soldevelo.com.
For more options, visit
https://groups.google.com/d/optout.
–
You received this message because you are subscribed to the Google Groups “OpenLMIS Dev” group.
To unsubscribe from this group and stop receiving emails from it, send an email to
openlmis-dev+unsubscribe@googlegroups.com.
To post to this group, send email to
openlmis-dev@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/openlmis-dev/BDB8ACA4-6F39-4FF7-90F3-F7AE23B39214%40villagereach.org.
For more options, visit
https://groups.google.com/d/optout.
–
<Soldevelo_logo_EPS_CMYK.png>
Sebastian Brudziński
Software Developer / Team Leader
SolDevelo Sp. z o. o. [LLC]
Office: +48 58 782 45 40
/ Fax: +48 58 782 45 41
Al. Zwycięstwa 96/98
81-451, Gdynia
http://www.soldevelo.com
Place of registration: Regional Court for the City of Gdansk
KRS: 0000332728, TAX ID: PL5862240331, REGON: 220828585,
Share capital: 60,000.00 PLN
sbrudzinski@soldevelo.com
