Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing protocol errors #7982

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

### Internals
* Update TestAppSession to allow scope-based usage for restarting the local app resources. ([PR #7672](https://github.com/realm/realm-core/pull/7672))
* Added definitions for missing protocol errors reported by the server. ([PR #7982](https://github.com/realm/realm-core/pull/7982))

----------------------------------------------

Expand Down
2 changes: 2 additions & 0 deletions src/realm/error_codes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,8 @@ static const constexpr MapElem string_to_error_code[] = {
{"SyncConnectFailed", ErrorCodes::SyncConnectFailed},
{"SyncConnectTimeout", ErrorCodes::SyncConnectTimeout},
{"SyncInvalidSchemaChange", ErrorCodes::SyncInvalidSchemaChange},
{"SyncLocalClockBeforeEpoch", ErrorCodes::SyncLocalClockBeforeEpoch},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was never added to the string_to_error_code array.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the new errors be added to this map? and also assign error categories?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am reusing existing ErrorCode values and the two added to this list were already assigned error categories (they were just left off this list - I just verified the list and the only other one missing is UnknownError this seems to be intentional).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this error even mean?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an error I added. We use 2015-01-01 as epoch and count the milliseconds since then for the changesets' timestamp. This error is thrown when the local clock is set before that date.

{"SyncSchemaMigrationError", ErrorCodes::SyncSchemaMigrationError},
{"SyncPermissionDenied", ErrorCodes::SyncPermissionDenied},
{"SyncProtocolInvariantFailed", ErrorCodes::SyncProtocolInvariantFailed},
{"SyncProtocolNegotiationFailed", ErrorCodes::SyncProtocolNegotiationFailed},
Expand Down
7 changes: 7 additions & 0 deletions src/realm/error_codes.h
Original file line number Diff line number Diff line change
Expand Up @@ -266,9 +266,16 @@ typedef enum realm_sync_errno_session {
RLM_SYNC_ERR_SESSION_REVERT_TO_PBS = 234,
RLM_SYNC_ERR_SESSION_BAD_SCHEMA_VERSION = 235,
RLM_SYNC_ERR_SESSION_SCHEMA_VERSION_CHANGED = 236,
RLM_SYNC_ERR_SESSION_FIELD_LVL_PERMS_NOT_SUPPORTED = 238,
RLM_SYNC_ERR_SESSION_SCHEMA_VERSION_FORCE_UPGRADE = 239,
// Error code 299 is reserved as an "unknown session error" in tests
} realm_sync_errno_session_e;

// These errors are intended for the edge server and are an error if received by a sync client
typedef enum realm_sync_errno_edge_e {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should expose errors for the edge server. The only way we could get these is via a bug. I think getting an UnkownError for edge server only errors would be totally acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the edge server and wire protocol errors

RLM_SYNC_ERR_EDGE_REBOOT = 237,
} realm_sync_errno_edge;

typedef enum realm_web_socket_errno {
RLM_ERR_WEBSOCKET_OK = 1000,
RLM_ERR_WEBSOCKET_GOINGAWAY = 1001,
Expand Down
15 changes: 15 additions & 0 deletions src/realm/sync/protocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,15 @@ const char* get_protocol_error_message(int error_code) noexcept
case ProtocolError::schema_version_changed:
return "Client opened a session with a new valid schema version - migrating client to use new schema "
"version (BIND)";
case ProtocolError::edge_reboot:
return "Error used to reboot the Edge Server. This error should only be sent to the Edge Server "
"and no other clients.";
case ProtocolError::field_perms_not_supported:
return "Client tried to make a read or write with a role that contains field-level permissions which "
"are not supported on edge wireprotocol";
case ProtocolError::schema_version_force_upgrade:
return "Server has forcefully bumped client's schema version because it does not support schema "
"versioning";
}
return nullptr;
}
Expand Down Expand Up @@ -164,6 +173,8 @@ Status protocol_error_to_status(ProtocolError error_code, std::string_view msg)
[[fallthrough]];
case ProtocolError::bad_changeset_size:
[[fallthrough]];
case ProtocolError::field_perms_not_supported:
[[fallthrough]];
case ProtocolError::bad_message_order:
return ErrorCodes::SyncProtocolInvariantFailed;
case ProtocolError::bad_decompression:
Expand Down Expand Up @@ -222,6 +233,8 @@ Status protocol_error_to_status(ProtocolError error_code, std::string_view msg)
case ProtocolError::bad_schema_version:
[[fallthrough]];
case ProtocolError::schema_version_changed:
[[fallthrough]];
case ProtocolError::schema_version_force_upgrade:
return ErrorCodes::SyncSchemaMigrationError;

case ProtocolError::limits_exceeded:
Expand All @@ -247,6 +260,8 @@ Status protocol_error_to_status(ProtocolError error_code, std::string_view msg)
case ProtocolError::user_blacklisted:
[[fallthrough]];
case ProtocolError::transact_before_upload:
[[fallthrough]];
case ProtocolError::edge_reboot:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very clearly not unreachable as we reach this point if the server sends us this error code. "Unreachable" specifically means that it's a bug in our code if things ever get to that point.

REALM_UNREACHABLE();
}
return ErrorCodes::UnknownError;
Expand Down
5 changes: 5 additions & 0 deletions src/realm/sync/protocol.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,11 @@ enum class ProtocolError {
revert_to_pbs = RLM_SYNC_ERR_SESSION_REVERT_TO_PBS, // Server rolled back to PBS after FLX migration - revert FLX client migration (BIND)
bad_schema_version = RLM_SYNC_ERR_SESSION_BAD_SCHEMA_VERSION, // Client tried to open a session with an invalid schema version (BIND)
schema_version_changed = RLM_SYNC_ERR_SESSION_SCHEMA_VERSION_CHANGED, // Client opened a session with a new valid schema version - migrate client to use new schema version (BIND)
field_perms_not_supported = RLM_SYNC_ERR_SESSION_FIELD_LVL_PERMS_NOT_SUPPORTED, // Client tried to make a read or write with a role that contains field-level permissions which are not supported on edge wireprotocol.
schema_version_force_upgrade = RLM_SYNC_ERR_SESSION_SCHEMA_VERSION_FORCE_UPGRADE, // Server has forcefully bumped client's schema version because it does not support schema versioning

// Edge Server errors - not supported
edge_reboot = RLM_SYNC_ERR_EDGE_REBOOT, // Error used to reboot the Edge Server. This error should only be sent to the Edge Server and no other clients.

// clang-format on
};
Expand Down
Loading