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.95k stars 1.24k forks source link

ODatav4 ODataContext delete does not throw upon 404. #4084

Closed swayongshen closed 3 months ago

swayongshen commented 3 months ago

OpenUI5 version: 1.123.2

Browser/version (+device/version): Chrome MacOS

Steps to reproduce the problem: For the following snippet, when the HTTP DELETE call returns 404, no error is thrown for the rowContext.delete() call and 'wow' is printed to console.

const rows = this.table.getSelectedItems();
const promises = rows.map(async (row) => {
  const rowContext = row.getBindingContext() as ODataContext;
  try {
    await rowContext.delete('$direct');
    console.log('wow');
  } catch (err) {
    console.error('err', err);
  }
});
await promises;

When I tried with this snippet that uses TripPin OData service which throws HTTP 500 when deleting an absent object, an error is thrown as expected.

What is the expected result? The call to .getBindingContext().delete() should throw an error if the API response for the HTTP DELETE is 404.

What happens instead? No error is thrown.

ThomasChadzelek commented 3 months ago

It does what it should. Please consult "If the entity does not exist, we assume it has already been deleted by someone else and report success."

If need be, bRejectIfNotFound might offer a way.

But what is the difference whether your DELETE or someone else's DELETE has removed that entity? Why should a user care?

swayongshen commented 3 months ago

@ThomasChadzelek ok I understand the rationale. However, we are using SAP's CAP Java backend which by default, sends only a single error response in a batch request no matter how many requests were sent, in an event where there is at least one error in the batch request.

When deleting an object from the table, UI5 makes two requests in a single batch, one DELETE to delete the object and one GET to fetch objects except the deleted object.

When the DELETE call fails, the batch returns a single error response which is ignored by the DELETE (so we can't catch the error here), but is treated as an error by the GET so the following shows on the table.

image

One alternative work around other than bRejectIfNotFound is to make CAP JAVA return one response per request in the batch but that require a Prefer: continue-on-error header that we cannot set on an ODataV4Model as it is unsupported by UI5, even though it is in the list of supported OData headers that are listed in the UI5 documentation.

image

Seeking your expertise on this :+1:

ThomasChadzelek commented 3 months ago

OK, I see.

One remark regarding the UI5 documentation: it says _The following headers must not be used: OData V4 requests headers as specified in "8.1 Common Headers" and "8.2 Request Headers" of the specification "OData Version 4.0 Part 1: Protocol"_ and "Prefer" belongs to section 8.2 - which means it is a bit hard to see, but clearly documented. And indeed, we cannot simply allow this header to be changed because we rely on the behaviour in certain places.

Maybe a solution for you is to send the DELETE in a separate $batch? You can easily achieve this by specifying a sGroupId like "$auto.foo" with "foo" replaced by any name that helps you later understand the intention. See Batch Control for more details.

ThomasChadzelek commented 3 months ago

P.S.: What would you do about the failing GET in case you could "catch" the failing DELETE? Maybe it would help to show the exact content of the $batch. When you say "is treated as an error by the GET so the following shows on the table", you refer to the blue (selected) line having no real content, right? I don't get why it looks selected when your code would delete all selected rows...

swayongshen commented 3 months ago

@ThomasChadzelek Sorry for the conclusion. Currently in our code, to make use of batch, the delete line would be await rowContext.delete() instead of await rowContext.delete('$direct') and this will cause the error in the UI.

Unfortunately I misread the UI5 docs to mean that the common headers would be supported. Since they are not supported, then we cannot use the Prefer header.

We have decided to use batch $direct after evaluating our options. Thank you.