Return the right error code when trying to move an item that doesn't exist
AcceptedPublic
Actions

Authored by mollekopf on Thu, Sep 19, 10:00 PM.

Details

Reviewers

Group Reviewers

Summary

We used to return 110, which is not a valid code for MoveItems, now we return 1 as we
should.
We now also prefer the "invalid source collection" error code to the
"source and destination are equal" code. This is because if we have
garbage as source and destination, it makes more sense to say that the
id is invalid then that it is equal.

Diff Detail

Repository

rS syncroton

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 50418
Build 18426: arc lint + arc unit

Event Timeline

mollekopf requested review of this revision.Thu, Sep 19, 10:00 PM

mollekopf created this revision.

Harbormaster completed remote builds in B50385: Diff 14178.Thu, Sep 19, 10:00 PM

mollekopf added a reviewer: Syncroton Developers.Thu, Sep 19, 10:01 PM

I see that MoveItems has a global Status (that we do not use) and per-item Status. https://learn.microsoft.com/en-us/openspecs/exchange_server_protocols/ms-ascmd/acae4033-b4f9-4f2a-8d83-51e097eb3b90

I thought that the common status codes can be used everywhere, but the documentation is not clear. https://learn.microsoft.com/en-us/openspecs/exchange_server_protocols/ms-ascmd/95cb9d7c-d33d-4b94-9366-d59911c7a060

As for 110 (server error), it looks like in general it might not be the best default error state as it means "the device SHOULD NOT retry later.". 111 (ServerErrorRetryLater) might be better for most generic error cases.

lib/ext/Syncroton/Command/MoveItems.php
63	That's a debug message, not a warning.
88	Is this the best we can do? Obviously "invalid source" will not always be true. On the other hand we'd have to check the source object existence to do the distinction.
97–102	There are still lines of code outside of try/catch. The chance they will throw a generic Exception is not big, but I'm not sure I like removal of the generic Exception handler. It probably should be kept, but use status 5 or 7.

Addressed comments

We now catch all exceptions again
We try harder to detect a missing item on the move (it's a bit hard to detect because IMAP just reports OK, even if the uid is bogus)

Harbormaster completed remote builds in B50418: Diff 14211.Wed, Sep 25, 10:04 AM

In D4950#61803, @machniak wrote:

I see that MoveItems has a global Status (that we do not use) and per-item Status. https://learn.microsoft.com/en-us/openspecs/exchange_server_protocols/ms-ascmd/acae4033-b4f9-4f2a-8d83-51e097eb3b90

I thought that the common status codes can be used everywhere, but the documentation is not clear. https://learn.microsoft.com/en-us/openspecs/exchange_server_protocols/ms-ascmd/95cb9d7c-d33d-4b94-9366-d59911c7a060

As for 110 (server error), it looks like in general it might not be the best default error state as it means "the device SHOULD NOT retry later.". 111 (ServerErrorRetryLater) might be better for most generic error cases.

Perhaps we should also change the global exception handler in Server.php to return a global status code instead of a 500 error.

machniak accepted this revision.Thu, Oct 3, 1:55 PM

This revision is now accepted and ready to land.Thu, Oct 3, 1:55 PM