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.

Sign up for GitHub

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

Make fields of ConnectionSetupRequestError public #1131

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Make fields of ConnectionSetupRequestError public #1131

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions scylla/src/transport/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,9 +695,10 @@ pub enum TranslationError {
/// It indicates that request needed to initiate a connection failed.
#[derive(Error, Debug, Clone)]
#[error("Failed to perform a connection setup request. Request: {request_kind}, reason: {error}")]
#[non_exhaustive]
pub struct ConnectionSetupRequestError {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe also mark the struct as #[non_exhaustive]? @Lorak-mmk @wprzytula WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or introduce pub getters instead of pubifying the fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pub getters would prevent matching on errors

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pub getters would prevent matching on errors

Well, you still could pattern match down until ConnectionSetupRequestError, and then you could indeed not pattern match deeper. Would it be a serious limitation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, consider this patch to upgrade our project to the latest release of scylla-rust-driver:
https://github.com/shotover/shotover-proxy/pull/1840/files#diff-3c1b291110379020fb6bc7d1644e2e0027997d5243b1c111d1a927a577f9a2c9

The patch uses this PR to scylla-rust-driver that makes the field public allowing this to work:

    match err {
        NewSessionError::ConnectionPoolError(ConnectionPoolError::Broken {
            last_connection_error:
                ConnectionError::ConnectionSetupRequestError(ConnectionSetupRequestError {
                    error: ConnectionSetupRequestErrorKind::DbError(DbError::ServerError, err),
                    ..
                }),
        }) => assert_eq!(format!("{err}"), "expected error"),
        _ => panic!("Unexpected error, was {err:?}"),
    }

Using the latest release of scylla-rust-driver we instead need to break it into two separate matches:

    match err {
        NewSessionError::ConnectionPoolError(ConnectionPoolError::Broken {
            last_connection_error: ConnectionError::ConnectionSetupRequestError(err),
        }) => match err.get_error() {
            ConnectionSetupRequestErrorKind::DbError(DbError::ServerError, err) => {
                assert_eq!(format!("{err}"), "expected error")
            }
            _ => panic!("Unexpected error, was {err:?}"),
        },
        _ => panic!("Unexpected error, was {err:?}"),
    }

So the current state of things is usable and enables breaking changes of the internal fields of the struct in the future.
On the other hand I strongly value being able to write a single match rather than having to break it up into two separate matches.

Its ultimately your call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Imo making the fields public and marking the struct #[non_exhaustive] should be the solution here.
This is consistent with our enums error types, which have tuple variants, so their fields are visible, and so we can add new variants, but not change existing ones.
Here similarly we will be able to add new fields (e.g. we may want to store some context in the future), but won't be able to edit existing fields.

As an exception, I'd keep structs like BrokenConnectionError with private fields. This is because the internal type can't be matched anyway since if first needs to be downcasted, for which we have the API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, unless @wprzytula or @muzarski have strong objections, I'd ask you @rukai to add #[non_exhaustive] and then we'll merge it.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, unless @wprzytula or @muzarski have strong objections, I'd ask you @rukai to add #[non_exhaustive] and then we'll merge it.

I'm fine with this approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that works for me!
I've pushed the changes.

request_kind: CqlRequestKind,
error: ConnectionSetupRequestErrorKind,
pub request_kind: CqlRequestKind,
pub error: ConnectionSetupRequestErrorKind,
}

#[derive(Error, Debug, Clone)]
Expand Down
Loading