Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

upgrade to 1.64 and use workspace dependencies #265

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

justinmmott
Copy link

TODO: get db.rs to compile current error is tripping me up

If anyone can help with this error that would be amazing. https://www.reddit.com/r/rust/comments/xmdz7s/how_to_import_a_struct_that_impls_a_trait_into_a/

TODO: get db.rs to compile current error is tripping me up
@augustuswm
Copy link
Contributor

I can take a look at this. I believe I ran into a similar (maybe the same) issue when initially looking at updating to the Diesel 2.0 RCs

@justinmmott
Copy link
Author

I can take a look at this. I believe I ran into a similar (maybe the same) issue when initially looking at updating to the Diesel 2.0 RCs

That would be awesome! Let me know if I can help in any way. I really can't seem to figure out why it doesn't work. My only thought was that I used LoadConnection on R2D2 instead of LoadConnection<T> in diesel-sentey/lib.rs. I'm not super clear of what difference that makes for the types it will accept and I couldn't seem to get it to work with the Connection impl unless it didn't have the <T>

@augustuswm
Copy link
Contributor

augustuswm commented Sep 26, 2022

On initial look there is a dependency conflict being hidden. There are two versions of bb8 (0.7.1 and 0.8.0), and two versions of diesel (confusingly both 2.0.0) being used.

In the root Cargo.toml try adding bb8 = "0.8.0" and replacing diesel with diesel = { version = "2.0.0" }. In cio/Cargo.toml replace the direct bb8 dependency with bb8 = { workspace = true }.

This should make sure that the versions pulled in across the workspace match those in async-bb8-diesel.

These should get the connection working at least. This then presents other compilation failures related to the diesel update, but I haven't looked at them yet.

Edit: Actually the remaining compilation issue was just a change to the ToSql::to_sql signature. Also this line shouldn't be needed:

// Need this so that ConnectionManager can impl ManageConnection
unsafe impl<C: Connection> Send for SentryConnection<C> {} 

@justinmmott justinmmott marked this pull request as ready for review September 26, 2022 20:39
@justinmmott
Copy link
Author

On initial look there is a dependency conflict being hidden. There are two versions of bb8 (0.7.1 and 0.8.0), and two versions of diesel (confusingly both 2.0.0) being used.

In the root Cargo.toml try adding bb8 = "0.8.0" and replacing diesel with diesel = { version = "2.0.0" }. In cio/Cargo.toml replace the direct bb8 dependency with bb8 = { workspace = true }.

This should make sure that the versions pulled in across the workspace match those in async-bb8-diesel.

These should get the connection working at least. This then presents other compilation failures related to the diesel update, but I haven't looked at them yet.

Edit: Actually the remaining compilation issue was just a change to the ToSql::to_sql signature. Also this line shouldn't be needed:

// Need this so that ConnectionManager can impl ManageConnection
unsafe impl<C: Connection> Send for SentryConnection<C> {} 

Thanks! I was able to get it to compile as well and all of the test passed!

Comment on lines -152 to -169
fn execute(&mut self, query: &str) -> QueryResult<usize> {
let mut txn = start_sentry_db_transaction("sql.query", query);

let result = self.inner.execute(query);
txn.finish();
result
}

#[tracing::instrument(
fields(
db.name=%self.info.current_database,
db.system="postgresql",
db.version=%self.info.version,
db.statement=tracing::field::Empty,
otel.kind="client",
),
skip(self, source),
)]
Copy link
Author

@justinmmott justinmmott Sep 26, 2022

Choose a reason for hiding this comment

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

I couldn't find any references to this function so I removed it since it is no longer part of the Connection trait. Not sure where it was supposed to be.

@justinmmott
Copy link
Author

justinmmott commented Sep 26, 2022

The original purpose of this PR was just to use the new 1.65 workspace dependencies. However, I ran into some issues getting that to compile which required upgrading to diesel 2.0 to fix. So this PR ended up being a lot bigger than I originally thought it would be.

Only other thing I wanted to note about this PR is that clippy wanted me to add Eq as a derived trait on a lot of things that used PartialEq. I tried doing that at first but it seems like the db macro was conflicting with it in some way so I ignored it for now.

@justinmmott
Copy link
Author

https://crates.io/crates/diesel-async now that this is a thing maybe it should be used

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants