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
13.19k stars 1.25k forks source link

query_as! macros do not use FromRow #514

Open icewind1991 opened 4 years ago

icewind1991 commented 4 years ago

instead the query_as! macro's directly build the target struct, while it might be nice not to have to derive FromRow for the struct, this also means that you can't customize the behaviour with a custom FromRow implementation.

abonander commented 4 years ago

We unfortunately have no way to do this right now while keeping the guarantee that output columns and types match the field names and types in the struct. We could possibly do some type-level magic with FromRow to get this information in a way that the macros can use it but leaning too heavily on typechecking produces some really obtuse compiler errors.

I think with the const_if_match and const_panic features we can make this work, although we would also need to be able to loop through a list which isn't currently possible in a const context either, but I think we can work around that.

jplatte commented 4 years ago

would also need to be able to loop through a list which isn't currently possible in a const context either

const_loop also got stabilized for 1.46, same as const_if_match. So that shouldn't be a problem :)

abonander commented 4 years ago

Ah, thanks. const_panic is still a necessary feature for this, though, otherwise we can't control the error message. There's been some promising discussion on the tracking issue so I think I could push it through the stabilization process if I can find the time: https://github.com/rust-lang/rust/issues/51999

Waelwindows commented 4 years ago

Could we at least document this currently for now? As It was not clear for me.

abonander commented 4 years ago

Could we at least document this currently for now? As It was not clear for me.

This is already stated in the documentation: https://github.com/launchbadge/sqlx/blob/master/src/macros.rs#L379

This is also stated in the documentation for 0.3.5, it's just missing the bold formatting: https://github.com/launchbadge/sqlx/blob/v0.3.5/src/macros.rs#L247

If that isn't clear enough then we'd love suggestions for better phrasing.

Waelwindows commented 4 years ago

The beta version is okay as it is, but I think it would be better if it directly mentions the FromRow as it's currently vague by which "trait implementation" it refers to, perhaps update FromRow's docs since they indicate that it's required for query_as, and shouldn't it link to this issue as well?

abonander commented 4 years ago

The documentation also goes on to explain, pretty clearly I think, how the macro maps rows to the struct.

However, "no trait implementations are required" can be misleading because Decode is still required for the individual field types of the struct so the wording should be adjusted anyway.

FromRow is indeed required for the function version of query_as which is why it's mentioned in its docs, which also link to the function to make it clear what it's referring to. In general if the docs are referring to a macro they will include the bang (!) in the item name to disambiguate.

Kinrany commented 3 years ago

This is already stated in the documentation: https://github.com/launchbadge/sqlx/blob/master/src/macros.rs#L379

Can you pin the link to a commit? Otherwise the line number gets outdated.

It would be nice to have a note about this at the top of https://docs.rs/sqlx/0.5.5/sqlx/trait.FromRow.html . That's where I started looking first when using sqlx(rename) didn't work.

jplatte commented 3 years ago

It would be nice to have a note about this at the top of docs.rs/sqlx/0.5.5/sqlx/trait.FromRow.html . That's where I started looking first when using sqlx(rename) didn't work.

I'm sure adding more docs would be appreciated šŸ˜‰

gtsiam commented 2 years ago

Ah, thanks. const_panic is still a necessary feature for this, though, otherwise we can't control the error message.

Just run into this too - Also: Rust 1.57 has stabilized const_panic

nrabulinski commented 2 years ago

Has any work been done on this? If not Iā€™d be willing to try and help contribute this feature.

laralove143 commented 1 year ago

im also waiting on this

rsalmei commented 1 year ago

So, to use nested fields we need FromRow. But to use FromRow we can't use query_as! macro, which dynamically runs queries at compile time, only the query_as func, which doesn't. Is that it? I came looking for how I could split some db fields into another struct, just trying to make sure I understand the current state of things. I love the compile-time query running, and don't want to lose it...

EDIT: Well, I just tried to use FromRow anyway, only to realize query_as func also doesn't accept arguments!! Humm, I'd have to manually format!() the query and handle SQL injections myself (or use the QueryBuilder which I've just found out about in the docs), and then send its &str to it? No way, it is too much of a compromise. Now I can see why this is important... Thank you for your work, sqlx is awesome! I can't stand diesel šŸ˜

laralove143 commented 1 year ago

that deserves its own issue and there probably already is one

saiintbrisson commented 1 year ago

EDIT: Well, I just tried to use FromRow anyway, only to realize query_as func also doesn't accept arguments!! Humm, I'd have to manually format!() the query and handle SQL injections myself (or use the QueryBuilder which I've just found out about in the docs), and then send its &str to it? No way, it is too much of a compromise. Now I can see why this is important...

@rsalmei I think what you are looking for is something like the bind function?

slqx::query_as("SELECT foo FROM bar WHERE name = ? AND age = ?")
    .bind("Harrison Ford")
    .bind(80);
JeppeKlitgaard commented 1 year ago

It seems like the needs rust feature tag should be removed from this issue since the needed features have been stabilised for some now?

abonander commented 1 year ago

It can, but I'm not really happy about how the compiler currently handles const panics: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=33e7700ac709e6581dcf478fb3c3b3b4

I proposed a format that would be more useful to us during the feature's development, but it was brushed off: https://github.com/rust-lang/rust/issues/51999#issuecomment-651022782

We also can't use Type::compatible() since trait methods can't be invoked in const which would necessitate some more codegen hackery, like generating a #[doc(hidden)] pub const fn method in an inherent impl with #[derive(Type)].

abonander commented 1 year ago

There's also the issue of formatting the error messages. We'd like to be able to output something like:

error mapping column `<column name>` to field `<field name>`: 
    expected Rust type <field type>, got incompatible type <column type>

However, panic!() currently only accepts either a string literal or a trivial format-args invocation: panic!("{}", s) where s is a &str.

There's been some relevant groundwork laid, e.g.: https://github.com/rust-lang/rust/issues/78356

But I cannot find evidence of any movement for allowing non-trivial format args in const panic!(). All the issues I can find are either closed or not relevant.

The const_format crate is interesting but it requires some unsafe hackery to work: https://github.com/rodrimati1992/const_format_crates/blob/9a9e1e8d5f477b30a22ea5eeb857d73f8b72682b/const_format/src/macros/fmt_macros.rs#L101

It's basically the same approach as the const-concat crate which has existed unchanged for 5 years, but with a nice API on top.

I suppose knowing that the former exists and is actively maintained is enough, but it is still rather a dumb reason to reach for unsafe on the face of it.

RoDmitry commented 1 year ago

I have managed to modify const-concat crate to run on stable:

pub const unsafe fn union_transmute<From, To>(from: From) -> To
where
    From: Copy,
    To: Copy,
{
    union Transmute<From: Copy, To: Copy> {
        from: From,
        to: To,
    }

    Transmute { from }.to
}

pub const unsafe fn concat<First, Second, Out>(a: &[u8], b: &[u8]) -> Out
where
    First: Copy,
    Second: Copy,
    Out: Copy,
{
    #[derive(Copy, Clone)]
    struct Both<A, B>(A, B);

    let arr: Both<First, Second> = Both(
        *union_transmute::<_, &First>(a),
        *union_transmute::<_, &Second>(b),
    );

    union_transmute(arr)
}

#[macro_export]
macro_rules! const_concat {
    ($a:expr, $b:expr) => {{
        let bytes: &'static [u8] = unsafe {
            &$crate::concat::<
                [u8; $a.len()],
                [u8; $b.len()],
                [u8; $a.len() + $b.len()],
            >($a.as_bytes(), $b.as_bytes())
        };

        unsafe { $crate::union_transmute::<_, &'static str>(bytes) }
    }};
    ($a:expr, $($rest:expr),*) => {{
        const TAIL: &str = const_concat!($($rest),*);
        const_concat!($a, TAIL)
    }};
    ($a:expr, $($rest:expr),*,) => {
        const_concat!($a, $($rest),*);
    };
}

#[cfg(test)]
mod tests {
    #[test]
    fn top_level_constants() {
        const HELLO: &str = "Hello";
        const COMMA: &str = ", ";
        const WORLD: &str = "world";
        const GREETING2: &str = const_concat!(HELLO, WORLD);
        const GREETING3: &str = const_concat!(HELLO, COMMA, WORLD);
        const GREETING4: &str = const_concat!(HELLO, COMMA, WORLD, "!");
        const GREETING6: &str = const_concat!(HELLO, COMMA, WORLD, "!", "1", "2");
        // const GREETING_TRAILING_COMMA: &str = const_concat!(HELLO, COMMA, WORLD, "!",);

        assert_eq!(GREETING2, "Helloworld");
        assert_eq!(GREETING3, "Hello, world");
        assert_eq!(GREETING4, "Hello, world!");
        assert_eq!(GREETING6, "Hello, world!12");
        // assert_eq!(GREETING_TRAILING_COMMA, "Hello, world!");
    }
}

But it fails on GREETING4 with:

assertion left == right failed left: ", world!Hello" right: "Hello, world!"

And it does not matter if you use const_concat!($a, TAIL) or const_concat!(TAIL, $a) which is very weird. I've made it just for fun, guess it's not usable.

Without macros (expanded) version:

fn main() {
    const HELLO: &str = "Hello";
    const COMMA: &str = ", ";
    const WORLD: &str = "world";
    const TAIL: &'static str = {
        const TAIL: &'static str = {
            let bytes: &'static [u8] = unsafe {
                &crate::concat::<[u8; WORLD.len()], [u8; "!".len()], [u8; WORLD.len() + "!".len()]>(
                    WORLD.as_bytes(),
                    "!".as_bytes(),
                )
            };
            unsafe { crate::union_transmute::<_, &'static str>(bytes) }
        };
        {
            let bytes: &'static [u8] = unsafe {
                &crate::concat::<[u8; COMMA.len()], [u8; TAIL.len()], [u8; COMMA.len() + TAIL.len()]>(
                    COMMA.as_bytes(),
                    TAIL.as_bytes(),
                )
            };
            unsafe { crate::union_transmute::<_, &'static str>(bytes) }
        }
    };
    let bytes: [u8; 13] = unsafe {
        crate::concat::<[u8; HELLO.len()], [u8; TAIL.len()], [u8; HELLO.len() + TAIL.len()]>(
            HELLO.as_bytes(),
            TAIL.as_bytes(),
        )
    };
    println!("{}", String::from_utf8(bytes.to_vec()).unwrap()); // ", world!Hello"
}
RoDmitry commented 1 year ago

Looks like const_format::concatcp! works good.

a dumb reason to reach for unsafe on the face of it.

@abonander As far as I understood, you need to panic!() during compilation, and you are worried about unsafe. Is it something to worry about? I mean the panic itself is like the final destination) I guess you are worried about the message being correct?

RoDmitry commented 1 year ago

I've just wrote my own implementation. Is it safe enough? And is it something you was looking for?

frantisek-heca commented 6 months ago

So, to use nested fields we need FromRow. But to use FromRow we can't use query_as! macro, which dynamically runs queries at compile time, only the query_as func, which doesn't. Is that it? I came looking for how I could split some db fields into another struct, just trying to make sure I understand the current state of things. I love the compile-time query running, and don't want to lose it...

Please, I am confused by this @rsalmei previous comment. Talking about "nested structs" - I have seen examples of query_as! macro with selects like (column1, column2, column3) as "address!: Address" posted elsewhere, that says this should work. Well, do I need query_as to map to a nested custom structs or can I do it with query_as! macro easily too, without deriving FromRow at all?

domenkozar commented 2 months ago

What's blocking this one?

abonander commented 2 months ago

@domenkozar the answer is, like pretty much always, "I haven't had time to work on it". I'm fairly certain it's possible now with more stable functionality in const but what the solution should exactly look like isn't forthcoming. I think it'd be a mix of const fn and maybe using traits and #[diagnostic::on_unimplemented] for better error messages, but I haven't had the time or energy to experiment, and there's always a dozen other things that need my attention.

Please do not spam this issue for updates. If there was movement, there'd be a draft PR.

domenkozar commented 2 months ago

@abonander thanks a lot for all your work!

I was wondering why there's no obvious way to sponsor features/bugfixes development? Something worthwhile exploring, I might get a budget to sponsor this!

abonander commented 2 months ago

We were previously discussing a paid offering but those plans have fallen by the wayside as other things got in the way, and we didn't see nearly as much interest in it from the community as we were expecting. From what I'm told, we got basically zero serious inquiries about it.

I'm actually technically not an employee of Launchbadge anymore, but of a spun-off company. We still have a very good working relationship, however, and I essentially have been given creative control here. My current employer actually gives me a little bit of dedicated time to work on SQLx, but I still have my primary duties as a software engineer and a team leader to balance with that.

We've talked about setting up Github Sponsors but it would not be through either company, it'd be paying me directly. The problem is, I would either have to eat into my personal time to work on SQLx (which I do a little bit anyway, but as a hobby, not a second job), or make an agreement with my employer to reduce my obligations (and likely my pay, accordingly) to make room for it, but that wouldn't really work at this time as we're pushing towards a product launch.

If, for whatever reason, I were to leave my current position, I do intend to make a go of it working on SQLx full time, supported by sponsors. It's just not in the cards right now.

laralove143 commented 2 months ago

i'd be interested in working on sqlx in the future, but onboarding such a large project is always time consuming and overwhelming at first, maybe if theres a contribution guide that explains each part of the internal code it'd be easier for new contributors to join

maxmezzomo commented 2 weeks ago

Hi, just wondering; does this mean when using query_file_as macro it is not allowed for the query to return more fields than expected on the type. As opposed to when using query_as function where I suppose it just ignores the additional fields?

I'm trying to explore the practical differences between the macros and functions, I can see the macros are perhaps more comprehensive as they also require a connection to the db to check types.

For the sake of code organization I would like to be able to store queries separate in sql files and simply call them from rust code with similar functionality as the regular query_as function. In my quite limited understanding this would seem like a trimming of functionality rather than adding more but also may not be inline with the goals of the macros feature. As an aside, I would also like to be able to use query_file_as without setting env vars or using cli for cross env dev etc..

Thanks for sqlx, it's very straightforward and non "invasive" and fills a big gap I had, I don't also don't mean for this comment to seam like endless requests, cause I'm also quite limited in my rust ability so couldn't work towards any of this, it's more of a request for information about the rationale between the macros and functions. Thanks

abonander commented 2 weeks ago

query_as works exactly the same as query_file_as; the only thing different is how it gets the SQL string itself. Both will error on extra fields.

maxmezzomo commented 2 weeks ago

Thanks for the very quick response. I had asked cause I got here trying to look into this. I was playing around switching my query_as for query_file_as macro and i am getting an error using the same query.: struct X has no field named y.

But I am using derive FromRow on the struct which may be how this fits into this thread.

Would that difference (macros not using FromRow but functions do) explain that? Or perhaps I am doing some else wrong entirely. Thanks

abonander commented 2 weeks ago

The macros (currently) have no interaction with FromRow whatsoever. They function exactly as described in the documentation: https://docs.rs/sqlx/latest/sqlx/macro.query_as.html

maxmezzomo commented 2 weeks ago

Alright thank you, so as I understand fromRow is the trait that allows deriving structs from rows that may have extra fields, and that does not apply to macros. Also sorry I see these may not be the best questions as the documentation is actually very clear I'm just newly trying to get into rust so even navigating the docs is a bit of a learning process. Cheers for the help!