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

sap.ui.getCore().getMessageManager().registerMessageProcessor does not work with useBatch: true #612

Closed DerGuteWolf closed 2 years ago

DerGuteWolf commented 9 years ago

Hi,

when you try sap.ui.getCore().getMessageManager().registerMessageProcessor with an v2.OataModel with useBatch: true , there are never any errors added the error model.

Apparently only the $batch and not the individual requests inside are checked for errors.

I can provide you with files for an hanatrial xsodata bassed app to demonstrate the problem, if needed.

Regards, Wolfgang

Michadelic commented 9 years ago

Hello Wolfgang,

i will contact the data binding experts so they can have a look. Actually batch processing is now enabled by default for performance reasons , so this should always be the case.

It might still be possible however that the error messages are not processed correctly. What would help more than the xsodata service would be the code that you use to attach to the message model and to read the error messages. Could you please provide a JSBin example for this?

This issue is also tracked internally with incident #1580121502

Kind Regards,

Michael

DerGuteWolf commented 9 years ago

this.getView().setModel(sap.ui.getCore().getMessageManager().getMessageModel(), "message");

            this._oMP = new sap.m.MessagePopover({
                items: {
                    path:"message>/",
                    template: new sap.m.MessagePopoverItem({ description: "{message>description}", type: "{message>type}", title: "{message>message}"})
                }
            });
            this.getView().addDependent(this._oMP);

I don't think that this a binding problem, because eg sap.ui.getCore().getMessageManager().getMessageModel().getData() also stays empty with useBatch:true

The error comes from submitChanges and thus belongs to a POST.

See also http://scn.sap.com/message/16261568

sirion commented 9 years ago

Hi Wolfgang,

you do not need to register the v2.ODataModel as it automatically registers itself (it is a MessageProcessor). See: https://github.com/SAP/openui5/blob/master/src/sap.ui.core/src/sap/ui/core/message/MessageProcessor.js#L41

It then creates a MessageParser instance which parses every response from the server. See: https://github.com/SAP/openui5/blob/master/src/sap.ui.core/src/sap/ui/model/odata/v2/ODataModel.js#L4550

We are using batch mode internally almost exclusively and there are no general problems with messages not being shown in batch mode.

Please provide more details about what is not working. Either provide an example or provide the complete content of the network requests, so we can build an example using a fake service.

regards, Jens

DerGuteWolf commented 9 years ago

Well, the documentation is here a bit misleading. https://openui5.hana.ondemand.com/#docs/guide/a90d93df5a024e8bb18826b699c9aaa7.html and https://openui5.hana.ondemand.com/#docs/guide/81c735e69d354de98b0bd139e4bd4e10.html both contain registerMessageProcessor calls.

I have a complete example for HANA XS running on hanatrial. Can I somehow give you access?

I have an V2 ODataModel with one entity set. A sap.m.List is displayed for this entity set. On press of an add-Button I do a createEntity on the model, let the user edit it in a SimpleForm via two-way binding and on pressing a save-Button submitChange is done. The submitted batch contains the POST and a GET to update the List. If the POST request triggers an error, the MessageModel is not updated and stays empty.

DerGuteWolf commented 9 years ago

Ok, I debugged this now. oReponse.body contains

{ "error": { "code": "", "message": { "lang": "en-US", "value": "Service exception: unique constraint violated."},"innererror":{"exception":"exception 1: no.71000301 (ptime\/session\/eapi\/jdbc\/ExternalStatement.cc:927)\n TrexUpdate failed on table '_SYS_BIC:p1941627274trial.hc.demo.data::mymodel.site' with error: unique constraint violation on pos=0 for table _SYS_BIC:p1941627274trial.hc.demo.data::mymodel.siteen, key: PlantID=qwer already exists as udiv=5, rc=55\nNO exception throw location recorded. Stack generation suppressed.\n"}}}

parsing of this results in aMessages.length === 1. mAffectedTargets in ODataMessageParser.prototype._propagateMessages is

"{"":true,"Sites":true}"

Next is the body of the GET parsed:

{"d":{"results":[{"__metadata": {"uri":"https://s9hanaxs.hanatrial.ondemand.com:443/p1941627274trial/hc/demo/services/mymodel.xsodata/Sites('dfgfd')","type":"p1941627274trial.hc.demo.services.mymodel.SitesType"},"PlantID":"dfgfd","CountryKey":"us","ShortDescr":"US","ProdUOM":"TO","FuelUOM":"L"},{"__metadata": {"uri":"https://s9hanaxs.hanatrial.ondemand.com:443/p1941627274trial/hc/demo/services/mymodel.xsodata/Sites('qwer')","type":"p1941627274trial.hc.demo.services.mymodel.SitesType"},"PlantID":"qwer","CountryKey":"fr","ShortDescr":null,"ProdUOM":"TO","FuelUOM":"L"},{"__metadata": {"uri":"https://s9hanaxs.hanatrial.ondemand.com:443/p1941627274trial/hc/demo/services/mymodel.xsodata/Sites('qwewer96')","type":"p1941627274trial.hc.demo.services.mymodel.SitesType"},"PlantID":"qwewer96","CountryKey":"fr","ShortDescr":"adfgfdag0987","ProdUOM":"TO","FuelUOM":"L"},{"__metadata": {"uri":"https://s9hanaxs.hanatrial.ondemand.com:443/p1941627274trial/hc/demo/services/mymodel.xsodata/Sites('ret56')","type":"p1941627274trial.hc.demo.services.mymodel.SitesType"},"PlantID":"ret56","CountryKey":"fr","ShortDescr":"dsfdsaf","ProdUOM":"TO","FuelUOM":"L"},{"__metadata": {"uri":"https://s9hanaxs.hanatrial.ondemand.com:443/p1941627274trial/hc/demo/services/mymodel.xsodata/Sites('tz5676')","type":"p1941627274trial.hc.demo.services.mymodel.SitesType"},"PlantID":"tz5676","CountryKey":"de","ShortDescr":"56de","ProdUOM":"TON","FuelUOM":"L"},{"__metadata": {"uri":"https://s9hanaxs.hanatrial.ondemand.com:443/p1941627274trial/hc/demo/services/mymodel.xsodata/Sites('werert45')","type":"p1941627274trial.hc.demo.services.mymodel.SitesType"},"PlantID":"werert45","CountryKey":"de","ShortDescr":"sdsdf","ProdUOM":"TO","FuelUOM":"L"}],"__count":"6"}}

mAffectedTargets in ODataMessageParser.prototype._propagateMessages is

{"":true,"Sites('dfgfd')":true,"Sites('qwer')":true,"Sites('qwewer96')":true,"Sites('ret56')":true,"Sites('tz5676')":true,"Sites('werert45')":true,"Sites":true}"

which contains Sites:true and thus aRemovedMessages contains the error message from the POST and is thus removed. No new errors, since the GET succeeded -> Error is lost with useBatch=true .

sirion commented 9 years ago

Hi,

this is a known "issue" with the Specification of messages. The behavior is correct. Server-Side messages are held on the server, so as soon as the server tells us that there are no messages for an entity we have to regard that as "the truth".

This leads to the problem that we currently cannot have server-side messages that are not persisted on the server and also reliably shown to the user. The current "workaround" is to show these messages in a Popup - this way the user always sees them even if they are removed directly afterwards.

Btw: This also happens without batch mode if the GET returns after the POST, there is just a bigger chance you get to see the message (for a short time).

We are currently working on the messages specification to account for this use case.

DerGuteWolf commented 9 years ago

Well, without useBatch=false submitRequest triggers first the POST and since that fails, the GET is not done, so the message stays. So, in this problem is really specific for useBatch=true!

With useBatch=true the POST and the GET are in the same $batch!!!!

The workaound means that I can't use MessageModel and co with useBatch=true and I have to rely on parsing the errors by myself!

Since I can't set individual batch group ids for two-way binding changes per request (only per entity) I can't force putting the GET in a second batch group.

So this is either a bug or you should document that MessageModel can't be used reliable with useBatch=true.

sirion commented 9 years ago

No, useBatch is just a special case where it happens in your case - if a refresh on the list is triggered by something else in your application after the POST, the message will be erased without batch mode as well. If you do not trigger a refresh (e.g. by suspending the Binding) on your list, it does not happen with batch mode.

Could you please explain what causes the GET request to be added in batch mode even though you state that it is triggered by the completion of the POST request - that sounds wrong to me.

Also: The MessageModel can be used reliably as defined by the server-side message specification. Messages are held (and kept) on the server. It works as designed and we must wait for a change in the specification before we can change anything, because any change from us would be - by definition - the introduction of a bug.

DerGuteWolf commented 9 years ago

As I said, I have a sap.m.List with binding to the EntitySet. And the SimpleForm with the binding to the Context from the createEntry call. With useBatch=true, the $batch triggered by submitChange also contains the GET for the List besides the POST. So the GET is always done regardless of success or failure of the POST. With useBatch=false, submitChange of cource only does the POST. No GET is triggered afterwards when the POST fails, only when the POST is successful.

with useBatch=false and failure of the POST: POST to /Sites with useBatch=false and succes of the POST: POST to /Sites GET to /Sites?$skip=0&$top=100&$inlinecount=allpages with useBatch=true and failure of the POST: $batch with this body: --batch_ea7c-b979-0880 Content-Type: multipart/mixed; boundary=changeset_da07-9b58-0bcb

--changeset_da07-9b58-0bcb Content-Type: application/http Content-Transfer-Encoding: binary

POST Sites HTTP/1.1 Accept: application/json Accept-Language: de-DE DataServiceVersion: 2.0 MaxDataServiceVersion: 2.0 x-csrf-token: 565A6AFC3C400B44886484167D3E51DC Content-Type: application/json Content-Length: 220

{"PlantID":"qwer","__metadata":{"type":"p1941627274trial.hc.demo.services.mymodel.SitesType","uri":"https://s9hanaxs.hanatrial.ondemand.com/p1941627274trial/hc/demo/services/mymodel.xsodata/Sites('id-1446632548446-7')"}} --changeset_da07-9b58-0bcb--

--batch_ea7c-b979-0880 Content-Type: application/http Content-Transfer-Encoding: binary

GET Sites?$skip=0&$top=100&$inlinecount=allpages HTTP/1.1 Accept: application/json Accept-Language: de-DE DataServiceVersion: 2.0 MaxDataServiceVersion: 2.0 x-csrf-token: 565A6AFC3C400B44886484167D3E51DC

--batch_ea7c-b979-0880--

DerGuteWolf commented 9 years ago

So what should I do with MessageModel with server side messages and useBatch=true? Search for all Bindings and suspend with before submitChange and unsuspend and manually refresh afterwards?

BTW: If you like to see this in action, we can do a short lync session, if you like.

sirion commented 8 years ago

Due to specification changes the following change should at least solve the problem with removing the messages for the collection: https://github.com/SAP/openui5/commit/e90d6d19578dcd614383d7c095969d11dc353291

Please test against the current master and update this thread about whether this fixes the issue for you.

DerGuteWolf commented 8 years ago

The question is wether this // Info: As of 2015-11-12 the "parent" EntitySet should not be part of the affected targets, meaning that // messages for the entire collection should not be deleted just because one entry of that selection // has been requested. // Before this all messages for the parent collection were deleted when an entry returned anything. // This only concerns messages for the EntitySet itself, not for its entities. // Example: // GET /Products(1) used to delete all messages for /Products(1) and /Products // now it only deletes all messages for the single entity /Products(1)

holds also true for the other way round, as I first have a failure for POST to an individual entity and then a GET for the collection which used to remove the error from the failure.

So: does an GET /Products delete a message for /Products(1) ?

If yes, we will still have this problem. If no, you should perhaps ammend the js comment cited above from the commit from the github commmt above.

sirion commented 8 years ago

You are asking the wrong question here. Does your server reply to "/Products" with a response containing "/Products(1)"? If so (and I hope so), the old messages for this entity will be removed and the new ones will be added. This part of the Specification did not change - and it will not as it is the only way to "synchronize" messages between server and client.

DerGuteWolf commented 8 years ago

It depends: If the first was a POST for /Products(x) and failed, /Product will not return /Products(x). If the first was a PUT/MERGE/PATCH for /Products(y), /Product will return /Product(y) (of cource this might also not true in case of filtering). However I still want to point out, that this is caused by SAPUI5 adding the GET /Product call to the batch after the POST/PUT/MERGE/PATCH and parsing and using the result of the GET even if the POST/PUT/MERGE/PATCH failed. Without batch, after the failed POST/PUT/MERGE/PATCH no GET is sent.

DerGuteWolf commented 8 years ago

I currently do not have the time to build the master and test. With 1.34.2 (which should contain the commit mentioned above AFAIK) the error is still there.

codeworrior commented 8 years ago

I can confirm that 1.34.2 contains the above mentioned change (e90d6d1 has been cherry-picked into rel-1.34 as 86e3be5).

sirion commented 8 years ago

Update on this: I guess this is a problem that came from two sources:

  1. The change in the internal specification
  2. The server-centric model of message handling

The internal specification changes have been implemented, we still have issues working with all application scenarios for point 2.

When a write to the server fails, it returns an error without storing it, which means that the next read to the same entity reports "no errors for this entity" - and we simply have to "believe" the server on this.

The workaround for this is to disable the autimatic refresh on the model before triggering the write like this:

oModel.setRefreshAfterChange(false);

And reset it after the write completes or returns an error.

This will however not help against manual refreshes on the model that are triggered somewhere else in the application.

Is this a suitable workaround for your situation?

DerGuteWolf commented 8 years ago

Well, in this specific case i might be able to do oModel.setRefreshAfterChange(false);, I will test this and report here. But this won't be possible with changes coming from two-way binding.

ashalkhakov commented 8 years ago

I had this issue today. I too use batching. Noticed that sometimes errors get abruptly removed (so the user will never notice they were there...).

I implemented a function that is run at the very beginning of submitChanges success handler in order to check if any errors have actually been transferred from the server among batch results, and abort the handler immediately if that is so. Seems to work OK in my scenario (a form for creating new entries).

sirion commented 8 years ago

The current workaround is to send a "wrong" target in cases where the error is not persisted on the server. We are working on a cleaner solution.

Example:

All in the same batch.

If you change the target of the error message to "/#TRANSIENT-[...]" in the server-response, the message will be kept even if /Products(1) is read again without errors.

The disadvantage is that you will have to remove the message by hand after the user has seen it and that it cannot have a real target, meaning it is not shown as ValueState in an Input-Control for example.

In the future we will support this workaround as well as a transient flag.

DerGuteWolf commented 8 years ago

In what handler should I change the target of the error message?

sirion commented 8 years ago

I don't know your back-end. Wherever you generate your errors/exceptions, you can replace the target with the workaround-target.

DerGuteWolf commented 8 years ago

ok, I thought this was a client side solution...we use both HANA XS ODATA and SAP GATEWAY as backends. I think for both a can't easily change the target of an error message...

sirion commented 8 years ago

Even a better (future) solution will require action on the back-end. It has to tell the client in some way or another that this specific error has not been persisted. In the future we are planing (don't quote me on this as it is not finally decided) to use a flag for this, but this flag will also have to be set in the server-response. There is no way around a modification of the back-end, unless you want to filter all messages in the application and then change the target there, which could be done in the way that ashalkhakov describes. I would advise against that.

DerGuteWolf commented 8 years ago

ok, but the I hope that you will communicate with your SAP colleagues responsible for

(ordered in my priority of needing this change...) that they will change their products accordingly (I hope I haven't forgot a SAP made OData producing product... )

Until this happens: Did I understand you right, that I can change the target in the submitChanges success handler? Is it also officially supported to stop processing batch results by aborting this handler (how can I do that?)?

sirion commented 8 years ago

The official workaround is to send other responses from the back-end, which will be needed in any case.

Aborting the processing in the submit handler (by throwing an exception I guess) is not a supported scenario, filtering them manually by accessing the mesage-manager via official API is.

DerGuteWolf commented 8 years ago

@sirion: "In the future we will support this workaround as well as a transient flag." Is this already available in recent OpenUI5 versions/the v4.ODataModel?

sirion commented 8 years ago

Yes, the relevant code can be seen here: https://github.com/SAP/openui5/blob/master/src/sap.ui.core/src/sap/ui/model/odata/ODataMessageParser.js#L328-L333

To keep the MessageManager from deleting the Message automatically, the message from the server must either have a "transient" field set to "true" or a target starting with "/#TRANSIENT#" followed by the real target.

I currently do not know how this will be resolved in the v4 Model.

uhlmannm commented 2 years ago

Hi @DerGuteWolf ,

as already explained in the last reply of @sirion, to solve the issue it is necessary to handle

differently.

This had been realized in OData V4 from the start. See Server Messages in the OData V4 Model for more information.

Best regards Mathias.