SAP / openui5

OpenUI5 lets you build enterprise-ready web applications, responsive to all devices, running on almost any browser of your choice.
http://openui5.org
Apache License 2.0
2.96k stars 1.24k forks source link

Server error code 404 gets treated as successful on delete in OData4 #2545

Closed vxmn closed 10 months ago

vxmn commented 5 years ago

OpenUI5 version: 1.66.1 Browser/version (+device/version):

Any other tested browsers/devices(OK/FAIL):

URL (minimal example if possible): https://openui5.hana.ondemand.com/test-resources/sap/ui/core/demokit/tutorial/odatav4/07/webapp/index.html But with modified mockserver.js: https://github.com/vxmn/openui5/commit/6f07e4ec57c116957cab8aa873258f6e9cd0a97a

User/password (if required and possible - do not post any confidential information here):

Steps to reproduce the problem:

  1. use the ODatav4 Demo
  2. modify mockserver.js to return a 404 for delete requests
  3. try to delete a user

When deleting an entity from a list, a server status code of 404 gets treated the same way a 200 is treated: the entity is deleted from cache.

What is the expected result? An error is being displayed that the user could not be deleted.

What happens instead? The entity is deleted from cache, even though the server indicated that the request was not successful.

Any other information? (attach screenshot if possible) The issue seems to be inside _Cache.js https://github.com/SAP/openui5/blob/0c268c1e4c52776dafa0b2f3bec9a5b88d24e3e8/src/sap.ui.core/src/sap/ui/model/odata/v4/lib/_Cache.js#L168-L173

ThomasChadzelek commented 5 years ago

Hello @vxmn !

This is intentionally (you see that comment: "map 404 to 200"). We thought that if you want to delete something and it is actually already gone (maybe because someone else deleted it first), that should be OK with you. After all, it's gone and that's what you wanted to achieve.

Best regards, Thomas

vxmn commented 5 years ago

Hello @ThomasChadzelek,

thank you for your quick reply.

I would appreciate if you would consider putting this exception into the documentation and/or log this odd edge case. I think swallowing errors this way is dangerous. Even for your mentioned case it might be a very useful peace of information you could show the user to make him aware, that his view might be out of date. E.g. use this information to trigger a reload of the view

vxmn commented 5 years ago

I think documenting and logging this case would not be enough. I would expect that this delete request failed and let the developer decide how he wants to handle this kind of error.

ThomasChadzelek commented 5 years ago

Hello @vmnx!

I'm not sure I get your point here. You say "show the user to make him aware, that his view might be out of date". What does that mean? If the user deletes data, it is gone from the UI, no matter if it was gone from the backend long time ago or just now. What is "out of date" on the UI?

Best regards, Thomas

vxmn commented 5 years ago

Hello @ThomasChadzelek,

as a developer I would expect that the framework I am relying on to tell me, when something went wrong and let me at least consider whether I want to handle this special case at all. Yes, it might be alright, that the entity has been deleted by someone else. But a 404 might also be an indication for a different problem.

Consider the case where user A has a view of details to an entity X including a list with children. In the meantime user B deletes the entity X. Now A tries to delete some child entities from X. All actions seem to be going fine, since the server responds to each delete request with 404. However, the parent entity X does not even exist anymore. All actions towards its children will result in a 404. User A is doing pointless work by deleting non existent items. He/she might even think, entity X and all the children he/she did not delete, still exist, even though the backend could have provided an updated view of the model after the first interaction.

Another case might be, that the endpoint is temporarily unavailable, e.g. because the container in which the OData server is being deployed, is not yet loaded after a restart. Most web servers that allow live deployment of such containers would just serve a 404, when no container is registered to the path. The point being: A 404 is no reason to believe that the action the user intended actually went through.

ThomasChadzelek commented 5 years ago

Hello @vxmn !

Regarding your question about documentation, I can confirm that sap.ui.model.odata.v4.Context#delete already states "If the entity does not exist, we assume it has already been deleted by someone else and report success."

We will consider your suggestion and think about an enhancement, but we need to stay compatible. What do you think about some way to report that "success was actually 404, not 200"? In this way, developers like you which care, can react. Other people who don't care, do not need to react at all.

Best regards, Thomas

vxmn commented 5 years ago

Hello @ThomasChadzelek,

you could either resolve the promise with an result object or add an option to delete() to reject the promise for all status codes except 2xx. Both changes can be implemented without breaking existing code.

flovogt commented 10 months ago

With UI5 1.103 v4.ODataModel#methods/delete offers the parameter bRejectIfNotFound to reject the returned promise.