Skip to content
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

Postgres: get_by_any getters #15

Closed
wants to merge 5 commits into from

Conversation

bram209
Copy link
Contributor

@bram209 bram209 commented Jul 26, 2021

Description

This PR adds get_by_any getters. These getters will return all records that match any of the provided values/ids.

Usage

My current use case is a graphql api that utilizes data loaders to avoid N+1 queries:

#[derive(Debug, Clone, SimpleObject, ormx::Table)]
#[ormx(table = "addresses", id = id, insertable)]
pub struct Address {
    #[ormx(get_optional = get_by_id(&Uuid), get_by_any)]
    pub id: Uuid,
    pub address_line1: Option<String>,
    pub address_line2: Option<String>,
    pub city: Option<String>,
    pub province_code: Option<String>,
    pub country_code: Option<String>,
    pub postal_code: Option<String>
}
#[async_trait]
impl<'a> Loader<Uuid> for AddressDataLoader {
    type Value = Address;

    type Error = Arc<sqlx::Error>;

    async fn load(
        &self,
        keys: &[Uuid],
    ) -> Result<std::collections::HashMap<Uuid, Self::Value>, Self::Error> {
        let mut conn = self.0.acquire().await?;
        let addresses = Address::get_by_any_id(&mut conn, keys).await?; // <- generated getter
        Ok(addresses.into_iter().map(|a| (a.id, a)).collect())
    }
}

Implementation

It generates the following SQL, to be type checked by sqlx: SELECT {} FROM {} WHERE {} = ANY($1).
It is only available for postgres since mysql does not support arrays.

@bram209 bram209 marked this pull request as ready for review July 27, 2021 11:27
@bram209
Copy link
Contributor Author

bram209 commented Jul 27, 2021

closes #7

@NyxCode
Copy link
Owner

NyxCode commented Jul 29, 2021

great work!
could you add a small example and note it in the documentation?

@bram209
Copy link
Contributor Author

bram209 commented Jul 30, 2021

yes no problem, will add an example + documentation this weekend : )

@bram209
Copy link
Contributor Author

bram209 commented Jul 30, 2021

On another (related) note, I was thinking...
ormx offloads the heavy lifting to sqlx, so the different backends of ormx in essence just have to come up with the correct SQL statements at compile time.

Would it make sense to refactor the backend trait towards something like:

pub trait Backend: Sized + Clone {
    // ...

    fn insert_statement(table: &Table<Self>) -> String {
        common::insert_statement(table)
    }

    fn get_statement(table: &Table<Self>) -> String {
        common::get_statement(table)
    }

    // ...

    fn get_any_statement(table: &Table<Self>) -> String
}
impl Backend for MySqlBackend {
    
    // ...

    fn get_any_statement(table: &Table<Self>) -> String {
        compile_error!("'get_many' getters are not supported with the MySql driver")
    }
}

This way we would not end up with feature flags throughout the code base

tldr: sqlx's fetch_one, fetch_many, fetch_optional, execute work on all drivers, the only thing that distinguishes them at the API surface is the database-specific SQL statements. Are there any differences in the actual code generation between different ormx drivers except the SQL itself?

@bram209
Copy link
Contributor Author

bram209 commented Aug 2, 2021

@NyxCode added small example and note in docs

@@ -63,6 +63,9 @@ mod utils;
/// **`#[ormx(get_many)]`**:
/// `{pub} async fn get_by_{field_name}(&{field_type}) -> Result<Vec<Self>>`
///
/// **`#[ormx(get_any)]`**:
/// `{pub} async fn get_by_{field_name}(&[{field_type}]) -> Result<Vec<Self>>`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am wondering if get_by_any would be a better name? The default name of the method would become get_by_any_{field_name}.

get_by_any_email(&[String]) is clearer than get_by_email(&[String]) in my opinion

Copy link
Owner

Choose a reason for hiding this comment

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

I agree!
Been inactive here for a while, sry. Do you still want to change this? I think get_by_any_* is a far better name. As soon as you are ready, I can merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem :) will push an update somewhere this week

@Milo123459
Copy link

Anything happening with this PR?

@NyxCode
Copy link
Owner

NyxCode commented Sep 21, 2021

Anything happening with this PR?

Just waiting for @bram209 to rename it to get_to_any 🙂

@NyxCode
Copy link
Owner

NyxCode commented Oct 4, 2021

@bram209 are you still working on this? If not, I'd happily rename the attribute myself and merge it.

@bram209
Copy link
Contributor Author

bram209 commented Oct 4, 2021

@bram209 are you still working on this? If not, I'd happily rename the attribute myself and merge it.

Sorry, this one slipped through : /

You can do it if you want, I am a bit busy rest of the week

@bram209
Copy link
Contributor Author

bram209 commented Nov 12, 2021

Sry have been extremely busy lately :/
@NyxCode did the renaming, should be good to go now

@bram209 bram209 changed the title Postgres: get_any getters Postgres: get_by_any getters Nov 12, 2021
@bram209
Copy link
Contributor Author

bram209 commented Oct 12, 2023

cleaning up my PR list

@bram209 bram209 closed this Oct 12, 2023
@bram209 bram209 deleted the feature/get-any branch October 12, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants