Error handling for deletes causing constraint violations

20 views
Skip to first unread message

Sebastian Brudziński

unread,
Jan 25, 2017, 10:58:17 AM1/25/17
to openlm...@googlegroups.com
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.

--

Sebastian Brudziński
Software Developer / Team Leader
sbrud...@soldevelo.com

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

Josh Zamor

unread,
Jan 25, 2017, 12:44:57 PM1/25/17
to Sebastian Brudziński, openlm...@googlegroups.com
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 lose data
- 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....@villagereach.org 
Senior Software Development Engineer, OpenLMIS Architect 

VillageReach 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 <sbrud...@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.

Sebastian Brudziński
Software Developer / Team Leader
sbrud...@soldevelo.com

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


--
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...@googlegroups.com.
To post to this group, send email to openlm...@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.

Paweł Gesek

unread,
Jan 25, 2017, 1:58:30 PM1/25/17
to openlm...@googlegroups.com

Hello,

although it's true that we won't have foreign key issues in this case, I think we would still don't want archived geographic zones being referenced by active facilities? There might be no hard database integrity error, however we might get a logical integrity error in this case. Do you think this is something we should consider?


Regards,
Paweł

For more options, visit https://groups.google.com/d/optout.

--

Paweł Gesek


Software Developer / Team Leader

Sebastian Brudziński

unread,
Jan 25, 2017, 2:11:36 PM1/25/17
to openlm...@googlegroups.com

Thanks for the answer Josh.

From what you are saying, the DELETE should be more of an "archive", rather than actual remove from the system. Does that mean we do not want to support complete removal at all? I think that raises some more questions.

What does it mean that a facility is associated with a geographic zone that is marked as deleted? Does this mean that the facility does no longer has got that association? If so, it would be simpler to just remove all non-null constraints and deletes would work just fine as well. However, I believe we actually want to have those checks in place? Or would we treat them as an association that still exists but with an archived resource? If so, do we still consider that association valid? Is it kind of - it doesn't matter that this resource is archived now, it existsed in the past so it's OK?

About discovering the resource via "/search", this is probably correct when creating new resources. However all the existing instances will simply look for a resource using UUIDs (especially cross-service relationships). In that case they would just fetch the "archived" resources they are linked to. Is that what we want to happen for all of our cross-service associations?

From what I understand, this approach means revisiting all our searches and introducing the checks for the deleted/archived flag + an option to search for archived resources. We will probably need to revisit our unique constraints as well (we may have duplications if we keep archived instances). Is that refactor something that we want to do in 3.0 for all of our services?

Best regards,
Sebastian.

--

Sebastian Brudziński


Software Developer / Team Leader
sbrud...@soldevelo.com

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


On 25.01.2017 18:44, Josh Zamor wrote:

Josh Zamor

unread,
Jan 25, 2017, 3:38:02 PM1/25/17
to Sebastian Brudziński, openlm...@googlegroups.com
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....@villagereach.org 
Senior Software Development Engineer, OpenLMIS Architect 

VillageReach 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 11:11 AM, Sebastian Brudziński <sbrud...@soldevelo.com> wrote:

Thanks for the answer Josh.

From what you are saying, the DELETE should be more of an "archive", rather than actual remove from the system. Does that mean we do not want to support complete removal at all? I think that raises some more questions.

What does it mean that a facility is associated with a geographic zone that is marked as deleted? Does this mean that the facility does no longer has got that association? If so, it would be simpler to just remove all non-null constraints and deletes would work just fine as well. However, I believe we actually want to have those checks in place? Or would we treat them as an association that still exists but with an archived resource? If so, do we still consider that association valid? Is it kind of - it doesn't matter that this resource is archived now, it existsed in the past so it's OK?

About discovering the resource via "/search", this is probably correct when creating new resources. However all the existing instances will simply look for a resource using UUIDs (especially cross-service relationships). In that case they would just fetch the "archived" resources they are linked to. Is that what we want to happen for all of our cross-service associations?

From what I understand, this approach means revisiting all our searches and introducing the checks for the deleted/archived flag + an option to search for archived resources. We will probably need to revisit our unique constraints as well (we may have duplications if we keep archived instances). Is that refactor something that we want to do in 3.0 for all of our services?

Best regards,
Sebastian.

--

Jake Watson

unread,
Jan 25, 2017, 4:05:14 PM1/25/17
to Josh Zamor, Sebastian Brudziński, openlm...@googlegroups.com
It also raises questions about RBAC, no?
Having a true delete (with referential integrity) is indeed useful assuming we have audit logging, but probably only for a very, very few users.

Office: +48 58 782 45 40/ Fax: +48 58 782 45 41Al. Zwycięstwa 96/9881-451, Gdynia
http://www.soldevelo.com

Place of registration: Regional Court for the City of GdanskKRS: 0000332728, TAX ID: PL5862240331, REGON: 220828585,Share capital: 60,000.00 PLN

Office: +48 58 782 45 40/ Fax: +48 58 782 45 41Al. Zwycięstwa 96/9881-451, Gdynia
http://www.soldevelo.com

Place of registration: Regional Court for the City of GdanskKRS: 0000332728, TAX ID: PL5862240331, REGON: 220828585,Share capital: 60,000.00 PLN


--
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...@googlegroups.com.
To post to this group, send email to openlm...@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...@googlegroups.com.
To post to this group, send email to openlm...@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.

--
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...@googlegroups.com.
To post to this group, send email to openlm...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/a620c1fb-56b0-fb4e-e69d-318a048aab05%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...@googlegroups.com.
To post to this group, send email to openlm...@googlegroups.com.

Sebastian Brudziński

unread,
Jan 27, 2017, 9:41:10 AM1/27/17
to openlm...@googlegroups.com
I've taken a look at the requisition and fulfillment services. The requisition service has currently got 3 DELETE endpoints and the fulfillment service has got 4 of them. It looks like both of them have got only 1 endpoint that may be problematic (that is result in 500 error).

For requisition service this is RequisitionTemplate. Requisition is linked to a RequisitionTemplate and it cannot be deleted as long as there's a requisition that's linked to it. Deleting requisition should be handled properly and this is currently even supported on the UI. The users are allowed to delete a requisition as long as it's in initiated state and deleting it means that a new requisition needs to be initiated for that period. The remaining delete is for JasperTemplates and it doesn't directly affect other entities.

For fulfillment service, deleting an Order may be problematic because ProofOfDelivery is linked with an Order that is required. Other delete endpoints include ProofOfDelivery, JasperTemplates and TransferProperties. I'd say there's no problem with the templates and properties, but I'm not sure how much sense does it make to allow deleting proof of deliveries (even though they shouldn't cause 500).

That being said I'd recommend deleting/marking as "not to be used" delete endpoints for requisitionTemplate and order. Endpoint for proofOfDelivery can be considered as well. Thoughts, component leads?

Also, how would you recommend marking the endpoints as "not to be used"?. Should we remove them from RAML completely? (Endpoints that are not present in RAML cannot be used at all and we don't have to remove them from the codebase completely). A milder approach is just leaving a note in the description in RAML and possibly javadocs that they are not to be used, but that may be hardly noticeable for implementers.

Regards,
Sebastian.

--

Sebastian Brudziński
Software Developer / Team Leader
sbrud...@soldevelo.com

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

Brandon Bowersox-Johnson

unread,
Jan 29, 2017, 6:00:42 PM1/29/17
to Sebastian Brudziński, openlm...@googlegroups.com

Hi Sebastian and Josh,

 

For the RequisitionTemplate DELETE endpoint, I suggest we simply prohibit deletion of any RequisitionTemplate that has 1 or more Requisitions. That should involve adding a simple check before deletion and responding with a 400 status code* and an error message.

 

For RequisitionTemplate I don’t think we need to do the complex thing of marking a deleted item as “Archived”. If there are zero Requisitions, then we truly allow the user to delete the RequisitionTemplate altogether, and that is sufficient. This is not one of our high-value endpoints where we want to archive old records or store a long audit trail. I don’t think an “Archived” status for RequisitionTemplate data is worth the complexity.

 

*HTTP 422 could also appropriate. OpenLMIS service style guide points to this: http://www.vinaysahni.com/best-practices-for-a-pragmatic-restful-api#http-status . I don’t think we have any consistency yet about what to return when DELETEs are not allowed. So I’m fine with 400 or 422.

 

Sebastian, this ticket 1705 seems to touch a lot of components and it has a lot of different DELETE endpoints to address. If we should split the ticket to address different services/components on their own, that’s fine. Also, if it would help to have a Skype meeting to speed up our decision-making, feel free to schedule.

 

-Brandon

brandon.bowersox-johnson

unread,
Jan 30, 2017, 8:01:37 PM1/30/17
to OpenLMIS Dev, sbrud...@soldevelo.com
Hi Sebastian,

Regarding the RequisitionTemplate, I want to add one more thing: Adding this simple DELETE ability on a RequisitionTemplate should not change what you folks did in ticket OLMIS-1360*. Specifically, that ticket means that each time a requisition is created, it uses the current template at that time. Even if the template changes later, that does not break the existing requisitions. 

I mention that here because hopefully even if a Requisition Template that has existing Requisitions gets deleted, that probably should not break the Requisition any way.

In summary, for the RequisitionTemplate DELETE endpoint, I will be happy if we do the simple solution for deleting that I suggested in my previous message below, AND as long as we make sure 1360 still works.


-Brandon


Senior Software Development Engineer, OpenLMIS Architect 

VillageReach 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 <sbrud...@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.

--

<Soldevelo_logo_EPS_CMYK.png>

Sebastian Brudziński
Software Developer / Team Leader
sbrud...@soldevelo.com

SolDevelo Sp. z o. o. [LLC]
Office: +48 58 782 45 40/ Fax: +48 58 782 45 41Al. Zwycięstwa 96/9881-451, Gdynia
http://www.soldevelo.com

Place of registration: Regional Court for the City of GdanskKRS: 0000332728, TAX ID: PL5862240331, REGON: 220828585,Share capital: 60,000.00 PLN

 

--
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 openlm...@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.

--
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.

--
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.

--
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.

Josh Zamor

unread,
Jan 31, 2017, 1:30:03 AM1/31/17
to OpenLMIS Dev, sbrud...@soldevelo.com
I share Brandon's thoughts on the Requisition Template.

Thanks Sebastian for tracking those down.  I think from that list, we'll proceed by treating those endpoints separately from the DELETE endpoints of Reference Data.

For Reference Data, which appears to have by far the most DELETE endpoints, I'd be fine with simply adding to the RAML not to use the DELETE operation for each.  However I could also see a value to removing access to the endpoint altogether to safe guard against accidental usage.  Sebastian, since SolDevelo will be the first organization to implement 3.0, I'd like to hear your opinion:  Would you prefer to remove them to be safe, or can we do the least-effort and simply recommend against their usage in the API documentation?

Best,
Josh

Sebastian Brudziński

unread,
Jan 31, 2017, 7:32:52 AM1/31/17
to openlm...@googlegroups.com
Thanks for the answers,

checking for requisitions that may be using the requisition templates sounds good. Brandon, this should not affect what we have done in OLMIS-1360, since in the first place we will be checking whether the template is in use (associated with a requisition) and if it is - we won't touch it. So this won't have any effect on this ticket, but we will of course make sure to verify that. Josh, I'd personally choose less restrictive approach, that is having the DELETE endpoints working and only annotating that they completely remove the instance and should be used with care or not used at all. Since we have the constraints in place, the instance will not be deleted if that would break any required associations. I don't think that someone would accidentally issue a DELETE request and it may turn out that deleting something is crucial for some case, but not possible.


Regards,
Sebastian.

--

Sebastian Brudziński
Software Developer / Team Leader
sbrud...@soldevelo.com

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



To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev...@googlegroups.com.

To post to this group, send email to openlm...@googlegroups.com.
Reply all
Reply to author
Forward
0 new messages