Skip to content

query_as! macros do not use FromRow #514

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

Open
icewind1991 opened this issue Jul 13, 2020 · 32 comments
Open

query_as! macros do not use FromRow #514

icewind1991 opened this issue Jul 13, 2020 · 32 comments
Labels
enhancement New feature or request macros

Comments

@icewind1991
Copy link

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
Copy link
Collaborator

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.

@abonander abonander added enhancement New feature or request macros needs rust feature This issue or PR needs a Rust feature to be stabilized before it can be merged into master labels Jul 13, 2020
@jplatte
Copy link
Contributor

jplatte commented Jul 14, 2020

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
Copy link
Collaborator

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: rust-lang/rust#51999

@Waelwindows
Copy link

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

@abonander
Copy link
Collaborator

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
Copy link

Waelwindows commented Aug 12, 2020

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
Copy link
Collaborator

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
Copy link

Kinrany commented Jun 18, 2021

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
Copy link
Contributor

jplatte commented Jun 18, 2021

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
Copy link

gtsiam commented Dec 15, 2021

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
Copy link

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

@laralove143
Copy link

im also waiting on this

@rsalmei
Copy link

rsalmei commented Mar 11, 2023

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
Copy link

that deserves its own issue and there probably already is one

@saiintbrisson
Copy link
Contributor

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
Copy link

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 abonander removed the needs rust feature This issue or PR needs a Rust feature to be stabilized before it can be merged into master label Oct 3, 2023
@abonander
Copy link
Collaborator

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: rust-lang/rust#51999 (comment)

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
Copy link
Collaborator

abonander commented Oct 3, 2023

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.: rust-lang/rust#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
Copy link

RoDmitry commented Oct 3, 2023

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
Copy link

RoDmitry commented Oct 3, 2023

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
Copy link

RoDmitry commented Oct 3, 2023

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

@frantisek-heca
Copy link

frantisek-heca commented Mar 20, 2024

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
Copy link

What's blocking this one?

@abonander
Copy link
Collaborator

@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
Copy link

@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
Copy link
Collaborator

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
Copy link

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
Copy link

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

abonander commented Sep 22, 2024

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
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request macros
Projects
None yet
Development

No branches or pull requests