launchbadge / sqlx

🧰 The Rust SQL Toolkit. An async, pure Rust SQL crate featuring compile-time checked queries without a DSL. Supports PostgreSQL, MySQL, and SQLite.
Apache License 2.0
12.38k stars 1.18k forks source link

Feature request: Cloneable Errors #1364

Open Bajix opened 2 years ago

Bajix commented 2 years ago

I'm planning on splitting my project deque-loader into an ecosystem that includes Diesel, SQLX, Redis, and later Withers, Scylla and Skytable once those are more matured. To do this, I need Error types that are Clone. Either the errors here can be made to be Clone, or else I can write an error kind enum to convert into, and if the case is the latter, would you be keen on contributing error conversions comparable to as I've done with Diesel here? Otherwise I can write the error conversions, but then they maybe end up more generalized and so I want to let you be opinionated should you wish.

Wopple commented 2 years ago

I am also running into library compatibility issues due to sqlx::Error not being Clone. I tried wrapping it in an Arc but that doesn't work for my use case. I need to own the resulting error, but the Arc owns the error.

abonander commented 2 years ago

Errors are not typically Clone, though. Just off the top of my head, none of these errors from popular libraries are Clone:

It sounds like you may be trying to paper over a code architecture issue.

Bajix commented 2 years ago

It sounds like you may be trying to paper over a code architecture issue.

I disagree. As database queries can be grouped together via data loaders, there's a need to be able to split the result and to clone errors as needed. Also some of these you've mentioned, such as redis::RedisError, have clone-able kind enums that can be used instead. Literally every data loader needs Clone errors and it's a major pain, but in general things that aren't used for batch operations don't need Clone errors.

Wopple commented 2 years ago

@abonander I also have an issue open with the offending library to remove the requirement on Clone precisely because so many errors aren't Clone (they have another transitive dependency on Clone so I don't know how likely this is). I can't speak to the best practice, just that this is a pain point that could be alleviated by implementing Clone. It is still understood that cloning is an expensive operation and users should only do so when necessary.

rex-remind101 commented 1 year ago

I am running into the same issue, @Bajix how did you end up solving this?

Bajix commented 1 year ago

@rex-remind101 I don't intend to support SQLX unless they make this change.

Wopple commented 1 year ago

Maybe supporting a feature flag to enable cloneable errors is a good compromise? I don't find "other libraries aren't doing it" to be a convincing argument.

Bajix commented 1 year ago

I think it's a bit much to introduce a feature flag for something so trivial; they should just make this change because it's easy and use cases exist

abonander commented 1 year ago

If it's as easy as you say it is, why not open a PR?