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

Remove types from pre-1.0 crates from the public API, or hide them behind features #771

Open
piodul opened this issue Jul 28, 2023 · 11 comments
Assignees
Labels
API-breaking This might introduce incompatible API changes API-stability Part of the effort to stabilize the API
Milestone

Comments

@piodul
Copy link
Collaborator

piodul commented Jul 28, 2023

Currently, the driver has a number of dependencies on crates that did not commit to the API stability, i.e. their latest releases are pre-1.0. Those crates do sometimes release new major versions and we usually, after some time, update those dependencies as well. However, updating dependencies on crates whose types appear in our public API generates friction for users: they may need to update the dependencies in their projects as well. This can require some effort, depending on how much stuff has been modified in the dependencies. Even if the effort is not large, the fact that we need to do it regularly prevents us from stabilising our API.

We should approach this as follows:

  • "Unstable" types should be removed from the signatures of public functions and should not appear as fields of our public structs/enums, in case when no features are enabled.
  • Each unstable dependency should be hidden behind a feature. There should be a separate feature for each version of the dependency, e.g. num-bigint_03 for version 0.3 and num-bigint_04 for v0.4. This will allow us to introduce support for newer versions without breaking support for the old ones, as we do now. If we feel brave enough, we can consider deprecating support for the old versions and eventually remove them when they are no longer supported by the authors.
    If such a feature is enabled, the API that it brings can, of course, depend on the unstable crate. This also includes trait implementations, e.g. impl FromCqlVal for num_bigint::BigInt.

The biggest offender seems to be the CqlValue. This PR: #745 seems to be a step in the right direction to fixing this type. Of course, there may be more issues and we should vet all current pre-1.0 dependencies.

@piodul piodul added the API-stability Part of the effort to stabilize the API label Jul 28, 2023
@piodul piodul added this to the 1.0.0 milestone Jul 28, 2023
@piodul piodul mentioned this issue Jul 28, 2023
8 tasks
@Lorak-mmk Lorak-mmk self-assigned this Nov 15, 2023
@avelanarius avelanarius assigned muzarski and unassigned Lorak-mmk Dec 8, 2023
@Lorak-mmk Lorak-mmk added the API-breaking This might introduce incompatible API changes label Jan 17, 2024
@Lorak-mmk
Copy link
Collaborator

Why limit this to pre-1.0 crates?
Crates that already released 1.0 may release 2.0 in the future and we will face the same problem (assuming we are on 1.0 too) - either we break SemVer, release 2.0, or force users to use old version of dependent crate.

@piodul
Copy link
Collaborator Author

piodul commented Jan 30, 2024

FWIW, the issue was inspired by this guideline: https://rust-lang.github.io/api-guidelines/necessities.html#public-dependencies-of-a-stable-crate-are-stable-c-stable , and the trick with the features was inspired by what the MongoDB driver did: https://docs.rs/mongodb/2.8.0/mongodb/#all-feature-flags .

Of course, SemVer is just a convention and dependencies are free to release 2.0, 3.0, etc. However, the meaning of pre-1.0 versions is that they explicitly opt-out of API stability, so it is reasonable to expect >=1.0 versions to be more stable and release new major versions less frequently.

We can take a look at >=1.0 dependencies as well, but in general they should release new major versions less frequently and some of them will probably maintain older versions for some time. I don't think we will need features for most, if not all of them.

Ultimately, we are also be free to release new major versions after 1.0 - I think it's perfectly fine, but we just shouldn't do it too frequently because then the implied API stability that comes with >=1.0 goes away.

@piodul
Copy link
Collaborator Author

piodul commented Feb 1, 2024

We did most of the work for the types used for serialization, but we still have some pre-1.0 dependencies that should be taken care of:

  • chrono, time, secrecy - already put behind features, but their names do not have the version of those dependencies in them.
  • histogram - our Metrics type contains a Histogram. We should not expose it publicly but rather reexport its functionality through Metrics' methods.
  • num_enum - it defines a bunch of traits and we implement them for some of our public types (and num_enum::TryFromPrimitiveError appears in our FrameError). We could easily implement the code that the crate generates automatically ourselves.
  • openssl - it's optional right now but the crates themselves are not stable. While those are only bindings to OpenSSL and are compatible with a wide range of versions (in case of OpenSSL they seem to be forwards-compatible), the openssl crate itself might turn out to have some security issues and maybe we will have to bump its version.
  • strum, strum_macros - I believe they have a similar problem to num_enum and the solution is the same for them.
  • async-trait - we use it for some of our public traits (AddressTranslator, AuthenticatorProvider, AuthenticatorSession). We may consider switching to recently stabilized async traits or change the methods to return boxed futures.

@muzarski
Copy link
Contributor

I discussed the openssl matter with @piodul offline.

When it comes to this dependency, we indeed use some of its items in the public - precisely:

  • openssl::SslContext in SessionBuilder::ssl_context public method
  • openssl::error::ErrorStack in CloudConfigError public enum

Notice that these public items are hidden behind the crate features anyway (ssl and cloud features respectively). In addition, @piodul noticed that openssl 0.10 version is pretty stable (0.10.0 introduced around 6 years ago).

I think we can leave this dependency as is. Perhaps, we could only add a version number to the feature name i.e. rename ssl feature to openssl-010 but I'm not sure if this is really necessary.

We should close this topic so it's possible to review the PR introducing rustls dependency. This PR makes some changes related to ssl crate feature.

cc: @Lorak-mmk @avelanarius

@Lorak-mmk
Copy link
Collaborator

Lorak-mmk commented Feb 29, 2024

I started reviewing rustls PR, and I think I'd prefer a different approach, which I'll describe there.
But in short: I think it would be better to make SslConfig an enum, with one variant for openssl and one for rustls. Those variant would exist based on enabled features.
It would allow the features to be additive - they are not right now and I think we should avoid it if possible as it will cause problems in the future.
It would also allow both to be enabled and used at runtime (so you could have a session with openssl and session with rustls). In case openssl breaks compatibility we could add new variant for new version to preserve API compatibility.

@Lorak-mmk
Copy link
Collaborator

Of course an alternative for an enum is trait, this could maybe enable more modularity / extensibility - we'll need to decide on something.

@muzarski
Copy link
Contributor

@wprzytula @Lorak-mmk We can close this.

@wprzytula
Copy link
Collaborator

@wprzytula @Lorak-mmk We can close this.

What about openssl?

@muzarski
Copy link
Contributor

@wprzytula @Lorak-mmk We can close this.

What about openssl?

We already hide the dependency behind ssl/cloud crate features.

@wprzytula
Copy link
Collaborator

@Lorak-mmk if you agree, please close this.

@Lorak-mmk
Copy link
Collaborator

@Lorak-mmk if you agree, please close this.

Those features, and how they are implemented internally don't really allow us to support new versions of those crates or different backends, so I don't think this should be closed.

@wprzytula wprzytula modified the milestones: 0.14.0, 0.15.0 Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-breaking This might introduce incompatible API changes API-stability Part of the effort to stabilize the API
Projects
None yet
Development

No branches or pull requests

5 participants