Viewer is unresponsive after receiving a model with duplicate ids

[spoenemann]

The command that is trying to install the erroneous model should be rejected.

[JanKoehnlein]

This solution only works if

• the command modifies a copy of the previous root
• the command hasn't modified the root before the exc is thrown

Neither is ensured by any of our API contracts

Von meinem iPhone gesendet

Am 09.08.2017 um 12:01 schrieb Miro Spönemann notifications@github.com:

Closed #161 via c505c85.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[spoenemann]

The change handles the most common case, which is sending a SetModelAction or UpdateModelAction with duplicate ids. Here the error is thrown by the model index while invoking the model factory, so the previous model is still unchanged. In order to handle cases where arbitrary commands manipulate the model and bring it into an inconsistent state, we would need to validate the whole model after each command execution. I'm not sure whether we want that. Should be handled in a separate issue.

[JanKoehnlein]

We should then make clear (at least document) that an exception thrown during command execution will

• not revert changes applied to the original
• not put the command on the stack. A command is responsible for handling everything else itself.

Von meinem iPhone gesendet

Am 09.08.2017 um 16:42 schrieb Miro Spönemann notifications@github.com:

The change handles the most common case, which is sending a SetModelAction or UpdateModelAction with duplicate ids. Here the error is thrown by the model index while invoking the model factory, so the previous model is still unchanged. In order to handle cases where arbitrary commands manipulate the model and bring it into an inconsistent state, we would need to validate the whole model after each command execution. I'm not sure whether we want that. Should be handled in a separate issue.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[JanKoehnlein]

I'd prefer a check or canExecute method in the Command interface to check preconditions hold before actually executing the command. To assume an untyped exception actually refers to that special condition doesn't feel good to me.

Von meinem iPhone gesendet

Am 09.08.2017 um 16:42 schrieb Miro Spönemann notifications@github.com:

The change handles the most common case, which is sending a SetModelAction or UpdateModelAction with duplicate ids. Here the error is thrown by the model index while invoking the model factory, so the previous model is still unchanged. In order to handle cases where arbitrary commands manipulate the model and bring it into an inconsistent state, we would need to validate the whole model after each command execution. I'm not sure whether we want that. Should be handled in a separate issue.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[spoenemann]

The actual issue that led to this report was that the thrown exception caused the promise chain of the command stack to be broken because the last promise was never resolved. My change makes sure that

1. the exception is properly logged with our logger interface,
2. the promise chain is kept intact.

Anyway, such an exception points to a problem that needs to be solved in an application. I don't think we should spend too much effort in making sprotty robust against application errors, but we need to make sure the application developer is notified of what's going on so she can fix the problem.