Consolidate fetch_* methods in a trait

[timvermeulen]

Submitting this PR to see if there is any interest in something like this 🙂 Obviously it's maybe not ideal to have to import a trait to be able to call these methods.

---

Introduces the Fetch trait with required methods fetch_many and fetch_optional (mirroring the Executor trait) that provides fetch, fetch_all, fetch_one, map, and try_map.

The main benefits:

• Gets rid of a lot of code duplication between Query, QueryAs, QueryScalar, and Map
• Allows combining query_as/query_scalar with map, which is slightly more convenient than fetching tuples and mapping the returned Vec

[abonander]

I don't think a trait for the user-facing part of the API is the right direction, precisely because it'll have to be imported everywhere which is going to be a huge headache.

What I had in mind was absorbing all of the existing Query* types into a single type with a Mapper trait governing its output:

pub struct Query<DB, M = Identity> { ... }

pub type QueryAs<DB, T> = Query<DB, mapper::MapFromRow<T>>;

pub type QueryScalar<DB, T> = Query<DB, mapper::Scalar<T>>;

pub type Map<DB, F> = Query<DB, mapper::MapFn<F>>;

pub trait Mapper<DB: Database> {
    type Output;

    fn map(&mut self, row: DB::Row) -> sqlx::Result<Self::Output>;
}

pub mod mapper {
    // `DB::Row` -> `DB::Row`
    pub struct Identity;

    // `DB::Row` -> `T` using `FromRow`
    pub struct MapFromRow<T>(PhantomData<T>);

    // `DB::Row` -> `T` using `Decode`
    pub struct Scalar<T>(PhantomData<T>);

    // `DB::Row` -> `T` where `F: FnMut(DB::Row) -> sqlx::Result<T>`
    pub struct MapFn<F>(pub F);
}

This would even allow custom Mapper implementations which is potentially quite powerful.

What's missing is some sort of typestate that allows .bind() calls on explicitly constructed Query instances but not ones returned by the macros. I think that's really important because otherwise it could result in some really confusing bugs. We currently implement that by just omitting .bind() on Map, but I don't think I would port that directly over to this API.

Maybe the solution for that is as simple as having a const MUTABLE: bool parameter on the Query type and only defining .bind() where MUTABLE = true.

Addendum: on thinking about this more, I think it would be clearer if we used marker types for mutability like MutableArgs and ImmutableArgs. Cause otherwise the type would be rendered in errors as, e.g. Query<Postgres, Identity, true> which isn't helpful.

[abonander]

Also, if we're making breaking changes to the query() API anyway, I'd like to work this change in at the same time: #3364

[abonander]

I also wanted to get rid of the Execute trait and maybe make it a struct instead. That would eliminate some monomorphized code.

Something like:

pub struct Execute<DB> {
    pub sql: SqlStr,
    pub arguments: Option<DB::Arguments>,
    pub persistent: bool,
    pub limit: Option<usize>,
}

[abonander]

@timvermeulen what do you think of the above?

[timvermeulen]

@timvermeulen what do you think of the above?

Your Mapper trait idea looks great — I actually started with something pretty similar except that I made Query generic over an FnMut rather than a custom trait, and that obviously made all of the types unnamable (on stable Rust, anyway) which is why I abandoned that idea. The Mapper trait along with MapFn for lifting an FnMut to a Mapper fixes all of the issues with my initial approach.

I'm now wondering if the main difference between these approaches is whether or not a trait needs to be imported to be able to call the fetch_* methods, or if there is more to it. Just so it's what all the trade-offs are 🙂 Here are some things that come to mind:

• In case one wants to write an abstraction that stores or returns a query, <M: Mapper> Query<DB, M> and <F: Fetch<DB>> F seem about equal in complexity.
• Mapper wouldn't need to be imported in order to call fetch_*, whereas Fetch would.
  • Types that implement Fetch could also add inherent methods that mirror the methods on Fetch (either manually or using something like inherent) though that's obviously not ideal.
  • sqlx::prelude exists, so some might argue that requiring a particular trait to be in scope shouldn't be an issue.
• A Fetch trait might make it easier for people to write custom adaptors, similar to the situation around core::iter::Iterator. Mapper only enables transformations where raw output rows correspond one-to-one with logical output elements.
  • The extra flexibility that a Fetch trait would provide could also be considered a negative, because filtering or aggregation logic is usually best done on the server.

Does that sound like a fair comparison? I think I'm on board with the reasoning that a single Query type that is generic over a Mapper trait is the most appropriate direction here because it adds just enough flexibility to neatly encapsulate the needs of Query, QueryAs, QueryScalar, and Map.

[abonander]

Types that implement Fetch could also add inherent methods that mirror the methods on Fetch (either manually or using something like inherent) though that's obviously not ideal.

Yeah, that's a non-starter.

sqlx::prelude exists, so some might argue that requiring a particular trait to be in scope shouldn't be an issue.

I honestly don't use preludes much myself, so this would still be annoying to me. It's mostly there for people who want it.

And besides, the Mapper approach with typedefs that match the existing struct names should mean that while it's technically a breaking API change, 99% of usages wouldn't have to change.

It'd be really annoying to upgrade to have to go through and add use sqlx::Fetch; to every broken file, especially in very large projects. It's just an unnecessary papercut.

The extra flexibility that a Fetch trait would provide could also be considered a negative, because filtering or aggregation logic is usually best done on the server.

Yeah, there's also not much reason to reinvent the wheel here. The user can get all the flexibility they want out of Stream (or when stable, AsyncIterator) combinators.