Handle unknown protocol errors gracefully in the sync client #6885
Conversation
| @@ -259,6 +260,7 @@ typedef enum realm_sync_errno_session { | |||
| RLM_SYNC_ERR_SESSION_MIGRATE_TO_FLX = 232, | |||
| RLM_SYNC_ERR_SESSION_BAD_PROGRESS = 233, | |||
| RLM_SYNC_ERR_SESSION_REVERT_TO_PBS = 234, | |||
| // Error code 299 is reserved as an "unknown session error" in tests | |||
There was a problem hiding this comment.
I'm assuming you let the server team know about these two reserved values?
There was a problem hiding this comment.
Yup, I coordinated with @kmorkos about this.
| @@ -488,7 +490,7 @@ class ClientProtocol { | |||
| if (auto action_it = mapping.find(action_string); action_it != mapping.end()) { | |||
| return action_it->second; | |||
| } | |||
| return action::ApplicationBug; | |||
| return action::NoAction; | |||
There was a problem hiding this comment.
So we should treat any new actions sent by the server that the client doesn't recognize as NoAction?
There was a problem hiding this comment.
hmm, going back to the scope, we do call out that it should be ApplicationBug. I'll change this back to the way it was and adjust the tests.
| {"action", "ApplicationBug"}, | ||
| }; | ||
| nlohmann::json test_command = {{"command", "ECHO_ERROR"}, | ||
| {"args", nlohmann::json{{"errorCode", 299}, {"errorBody", error_body}}}}; |
There was a problem hiding this comment.
Maybe create a REALM_SYNC_ERR_... constant for error code 199/299?
There was a problem hiding this comment.
Then they wouldn't be unknown though? I'm comfortable with just commenting on them as being reserved values. We definitely don't need to expose these error codes that will never be used via the C API, IMO.
There was a problem hiding this comment.
"Then they wouldn't be unknown though?" 😂
I agree with your response
| .get(); | ||
| REQUIRE(test_cmd_res == "{}"); | ||
| auto error = wait_for_future(std::move(error_future)).get(); | ||
| REQUIRE(error.status == ErrorCodes::UnknownError); |
There was a problem hiding this comment.
Add one more check to verify the message value from the JSON error is passed through to the status object:
REQUIRE(error.status.reason() == "fake error");
| @@ -279,6 +280,105 @@ static auto make_client_reset_handler() | |||
| return std::make_pair(std::move(reset_future), std::move(fn)); | |||
| } | |||
|
|
|||
|
|
|||
| TEST_CASE("app: error handling integration test", "[sync][flx][baas]") { | |||
There was a problem hiding this comment.
Create a new section with an unrecognized "action" value to verify the NoAction response.
| {"action", "ApplicationBug"}, | ||
| }; | ||
| nlohmann::json test_command = {{"command", "ECHO_ERROR"}, | ||
| {"args", nlohmann::json{{"errorCode", 299}, {"errorBody", error_body}}}}; |
There was a problem hiding this comment.
"Then they wouldn't be unknown though?" 😂
I agree with your response
What, How & Why?
This makes it so if the server sends a json error message with an action, the sync client won't crash if it doesn't know about the error code sent. This way we don't need to bump the protocol version every time we add an error code.
For legacy ERROR messages - which are still sent by the C++ test server, we ignore the action value.
There's also a new integration test that gets baas to send us a json error message with various unknown errors to validate that the error handling works as expected.
☑️ ToDos