RCORE-2210 Remove unused websocket too new/old errors #7917
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build jonathan.reams_3361Details
💛 - Coveralls |
tgoyne
approved these changes
Jul 23, 2024
| @@ -94,6 +94,7 @@ class DefaultWebSocketImpl final : public DefaultWebSocket, public Config { | |||
| { | |||
| WebSocketError error = WebSocketError::websocket_ok; | |||
| bool was_clean = true; | |||
| std::string msg = body.empty() ? ec.message() : std::string{body}; | |||
There was a problem hiding this comment.
This could avoid copying the body if it was inline in the call to websocket_error_and_close_handler() as the temporary from message() would live long enough, but I guess that doesn't actually matter.
michael-wb
approved these changes
Jul 23, 2024
| @@ -9,7 +9,7 @@ | |||
| * None. | |||
|
|
|||
| ### Breaking changes | |||
| * None. | |||
| * The websocket error codes `websocket_client_too_old`, `websocket_client_too_new`, and `websocket_protocol_mismatch` along with their C API constants were removed. These corresponded to errors the legacy C++ server could have sent, but the baas sync server never did. Any platform networking implementations that surfaced these errors can report a `websocket_fatal_error` instead if an unknown error occurs during the websocket handshake. If a client connects that is too old or too new, it will finish the websocket handshake and then receive an in-band sync `ERROR` message that will be handled by the sync error handler. [PR #7917](https://github.com/realm/realm-core/pull/7917) | |||
There was a problem hiding this comment.
I guess it's technically a breaking change, but I doubt any of the SDKs with platform networking care about this error (and I doubt they've ever seen it).
danieltabacaru
approved these changes
Jul 23, 2024
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What, How & Why?
The 403 error message sent by the C++ legacy server that would cause these websocket errors was never sent by baas and we have no tests for this case. So this is just dead code that should be safe to remove.
I put the changelog entry in the Breaking Changes section since this is a slight API change, but maybe it's more an Internal change?
☑️ ToDos
bindgen/spec.yml, if public C++ API changed