Make fields of ConnectionSetupRequestError public

[rukai]

Pre-review checklist

• I have split my patch into logically separate commits.
• All commit messages clearly explain what they change and why.
• I added relevant tests for new features and bug fixes.
• All commits compile, pass static checks and pass test.
• PR description sums up the changes and reasons why they should be introduced.
• I have provided docstrings for the public items that I want to introduce.
• I have adjusted the documentation in ./docs/source/.
• I added appropriate Fixes: annotations to PR description.

This seems like an accidental omission. Without these fields being public it is impossible to match on the error type.

[github-actions]

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 9bfd3c2

[muzarski]

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

[muzarski]

Or introduce pub getters instead of pubifying the fields.

[rukai]

Pub getters would prevent matching on errors

[wprzytula]

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?

[rukai]

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.

[Lorak-mmk]

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.

[Lorak-mmk]

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

[muzarski]

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

[rukai]

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

[rukai]

Anything I can do to move this forward?

[wprzytula]

I forgot to post my review, sorry for that @rukai.

[wprzytula]

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?

[Lorak-mmk]

@wprzytula This can land in 0.15.1