Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9719 closed defect (fixed)

JsonRestStore needs better error handling.

Reported by: Jared Jurkiewicz Owned by: Kris Zyp
Priority: high Milestone: 1.4
Component: DojoX Data Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description

The underlying problem is in JsonRest? but I am hitting it while using JsonRestStore?.

From a co-worker:

If you perform a JsonRestStore?.save(), and an error occurs, I was trying to find out how to deal with it. You can pass an onError() callback as part of the args to save(). However, the onError callback is provided no information regarding the error. You don't know what change failed nor how (status) it failed.

The error path in JsonRest? also performs a revert(). Again, this reverts all data not modified except for the one that failed. You end up with a store that has a change but no record of it.

Kris,

Any thoughts on what should be provided?

Change History (10)

comment:1 Changed 10 years ago by Kris Zyp

Is the error object that is passed to the onError callback insufficient? It includes a status property (HTTP status code) and a responseText property. Is there more information that should be included on the error object?

In regards to reverting data on failed saves, it seems to me that the dojo.data write API is designed to be transactional, multiple changes are committed. Traditionally in databases when a failure is encountered while committing a transaction, the entire transaction is rolled back. This convention is followed in JsonRestStore?, and the revert is called to carry out the rollback.

An issue with the dojo.data API that I have complained about before is that revert operations are not supposed to trigger notification events. This seems illogical to me, because it makes it impossible for a user of a store to reliably and consistently monitor an object for changes through notification events. The user receives notifications for all changes, with the exception of when a revert happens. This abnormality results in a change in data without the user being aware of it. JsonRestStore? follows this rule, and you are seeing the effect of it. When a save fails, the data is changed by the revert without any notifications.

Do you have any suggestions for how to deal with this? Should I create an option for having revert trigger notifications and/or have the failure path in save perform a revert that triggers notifications?

comment:2 Changed 10 years ago by Michael Fraenkel

I was the one who wrote the original bug.

The error is a start. I am debating whether the item is also interesting or not.

The issue with revert is the fact that the item that failed is not reverted. In my example I changed 1 item. When revert() was called, there were no dirty items. The resulting Grid still had my change but the Store had no dirty items.

I agree that notifications are a bit of a mess. I was trying to manage a Save button but realized there is no easy way to tell if the store is "dirty". I also cannot tell when the store has been refreshed and now clean. I am debating whether to just hardcode my dependency on the JsonRestStore? and look at the dirty item list. The trick is knowing all the cases when to check.

Obviously the Write API needs to document how to deal with errors so one can do it in a consistent manner. I do agree that having a choice is nice, but doing nothing is probably best. Let the user revert() in their error handler if they so choose.

There also needs to be a document that describes the interaction between Read/Write? and Notifications.

comment:3 Changed 10 years ago by Kris Zyp

The error is a start. I am debating whether the item is also interesting or not.

I could see how adding the item to the error object would be useful, I could certainly add that.

The issue with revert is the fact that the item that failed is not reverted. In my example I changed 1 item. When revert() was called, there were no dirty items. The resulting Grid still had my change but the Store had no dirty items.

The revert probably did happen, you just don't have any visual way of knowing that because the grid relies on notifications, which of course don't happen when the revert takes place, so the grid is still showing the changed data, even though the data in the underlying store was changed back. You could do a getValue on the underlying item to verify that.

I also cannot tell when the store has been refreshed and now clean.

If you call store.isDirty() (with no parameters) it should tell you if there are any dirty items.

I do agree that having a choice is nice, but doing nothing is probably best. Let the user revert() in their error handler if they so choose.

If we do nothing, do we leave the items that we attempted to save in dirty state? If we do, the next widget that calls save() will just get another failed save() attempt unless it knows to revert() or knows how to update the items to meet the demands of the server. Of course if two different widgets are working on the same store, this could be very confusing. If we clear the dirty flag on the items, the situation is even worse. Now we have items that are out of sync with the server and no dirty flag to indicate such. The former is plausible and maybe optional, although I certainly wouldn't think that it should be the default behavior. Traditional DB behavior is most intuitive path here.

In addition the revert that is performed by the error path in save() is not the plain vanilla revert. It only reverts the items that are a part of the current transaction, excluding an items that became dirty between the save() call and the response from the server. Handing that complexity to the user certainly doesn't seem desirable.

It seems the best course is to add an option to revert to trigger notifications and use notifications on the revert that is executed by the save. I could also add an option to not revert on save (still reverting by default though). Does that seem reasonable?

comment:4 in reply to:  3 Changed 10 years ago by Michael Fraenkel

Replying to kzyp:

I could see how adding the item to the error object would be useful, I could certainly add that.

Technically the docs for save() state that an error object is passed. If we want more information, should the docs for save() in Write.js be updated or should it be included in the error object?

The issue with revert is the fact that the item that failed is not reverted. In my example I changed 1 item. When revert() was called, there were no dirty items. The resulting Grid still had my change but the Store had no dirty items.

The revert probably did happen, you just don't have any visual way of knowing that because the grid relies on notifications, which of course don't happen when the revert takes place, so the grid is still showing the changed data, even though the data in the underlying store was changed back. You could do a getValue on the underlying item to verify that.

This would be considered a bug if the Grid and Store or whatever UI and the store are out of sync.

If you call store.isDirty() (with no parameters) it should tell you if there are any dirty items.

Good to know. I see that now in the docs.

I do agree that having a choice is nice, but doing nothing is probably best. Let the user revert() in their error handler if they so choose.

If we do nothing, do we leave the items that we attempted to save in dirty state? If we do, the next widget that calls save() will just get another failed save() attempt unless it knows to revert() or knows how to update the items to meet the demands of the server. Of course if two different widgets are working on the same store, this could be very confusing. If we clear the dirty flag on the items, the situation is even worse. Now we have items that are out of sync with the server and no dirty flag to indicate such. The former is plausible and maybe optional, although I certainly wouldn't think that it should be the default behavior. Traditional DB behavior is most intuitive path here.

Except there doesn't seem to be a notification that corresponds.

In addition the revert that is performed by the error path in save() is not the plain vanilla revert. It only reverts the items that are a part of the current transaction, excluding an items that became dirty between the save() call and the response from the server. Handing that complexity to the user certainly doesn't seem desirable.

What values are you reverting to? Lets say I changed item 1 and perform a save(). Item 1 is changed again, but the save() fails. What are the values of item 1? The original ones? the new one? or some mix of the original and new?

It seems the best course is to add an option to revert to trigger notifications and use notifications on the revert that is executed by the save. I could also add an option to not revert on save (still reverting by default though). Does that seem reasonable?

Reverting seems the right option provided it does the right thing. If you don't revert, what does someone do? Once the object hits the dirty list, how can I update its value?

Just as an aside, I am using Dojo 1.2 right now which has other issues that have been fixed in later releases like isDirty(). It looks like more information is passed on error in 1.3.2 so when I move up that should be fixed.

comment:5 Changed 10 years ago by Kris Zyp

This would be considered a bug if the Grid and Store or whatever UI and the store are out of sync.

But how does the grid have any hope of staying in sync if it doesn't get notifications when the revert takes place? Its not the grid's fault that it is out of sync. The only only way to keep the grid in sync is to use a notification inducing revert operation.

What values are you reverting to? Lets say I changed item 1 and perform a save(). Item 1 is changed again, but the save() fails. What are the values of item 1? The original ones? the new one? or some mix of the original and new?

The original ones, that is the values it had before it became dirty.

comment:6 Changed 10 years ago by Kris Zyp

I think I was mistaken, it actually looks the revert at the JRS level does fire notifications, but the JsonRest? one doesn't it. I'll get that fixed.

comment:7 in reply to:  6 Changed 10 years ago by Michael Fraenkel

I agree that notification has to occur on revert for everything to work. I also believe the documentation for Read/Write/Notification? support needs to be updated to document these interactions.

I am trying to figure out what the right behavior is when you have a form tied to a store and you are creating a new item. The form would create a new item, the store save fails, and now reverts, ie, deletes the new item. The form knows it failed from onError. The form would have to keep the values since it cannot ask the store.

I think allowing me to prevent the revert is still a valid requirement. This should be an option on the save(). In the end, the user of the Store will have to figure out how to deal with errors. Its a hard problem to solve if you allow multiple changes or changes while save() is in progress. But not one with just a single answer.

comment:8 Changed 10 years ago by Kris Zyp

Agreed

comment:9 Changed 10 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

Oops, forgot to reference it in the commit, but I checked in a fix for this.

comment:10 Changed 10 years ago by bill

Milestone: tbd1.4
Note: See TracTickets for help on using tickets.