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

Select into tuple #1311

Merged
merged 12 commits into from
Jan 10, 2023
Merged

Select into tuple #1311

merged 12 commits into from
Jan 10, 2023

Conversation

billy1624
Copy link
Member

@billy1624 billy1624 commented Dec 15, 2022

New Features

  • Added Select::into_tuple()

Breaking Changes

  • Added TryGetable::try_get_by_index()
  • Added TryGetableMany::try_get_many_by_index()

@billy1624 billy1624 self-assigned this Dec 15, 2022
@billy1624 billy1624 marked this pull request as ready for review December 20, 2022 07:43
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

I like the overall API 👍

However I think there is too much code duplication, and I think it is possible to refactor try_get_by_index and try_get with a common try_get_by<T>

@billy1624
Copy link
Member Author

Hey @tyt2y3, I'm thinking something like this. What do you think?

pub enum TryGetColumn {
    Name(String),
    Index(usize),
}

pub trait IntoTryGetColumn {
    fn into_try_get_column() -> TryGetColumn;
}

impl IntoTryGetColumn for &str { ... }
impl IntoTryGetColumn for (&str, &str) { ... }
impl IntoTryGetColumn for String { ... }
impl IntoTryGetColumn for (String, String) { ... }
impl IntoTryGetColumn for usize { ... }

pub trait TryGetable: Sized {
    fn try_get<T>(res: &QueryResult, col: T) -> Result<Self, TryGetError>
    where
        T: IntoTryGetColumn;
}

impl sqlx::ColumnIndex<MySqlRow> for TryGetColumn { ... }
impl sqlx::ColumnIndex<SqliteRow> for TryGetColumn { ... }
impl sqlx::ColumnIndex<PgRow> for TryGetColumn { ... }

@tyt2y3
Copy link
Member

tyt2y3 commented Jan 5, 2023

I think the original sqlx try_get is generic over usize and str as index, so we probably don't need the TryGetColumn enum?

The SQLx type system looks like the following:

mod private_column_index {
    pub trait Sealed {}

    impl Sealed for usize {}
    impl Sealed for str {}
    impl<T> Sealed for &'_ T where T: Sealed + ?Sized {}
}

Wouldn't the following work?

fn try_get_by<I: Index>(res: &QueryResult, idx: I) -> Result<Self, TryGetError> {

@billy1624
Copy link
Member Author

I think it won't work :(

fn try_get_by<I: Index>(res: &QueryResult, idx: I) -> Result<Self, TryGetError> { ... }

SQLx try_get takes &str / usize but here the compiler only knows the type implemented Index trait.

Also, I just found that we cannot implement sqlx::ColumnIndex in foreign crate because it's being sealed. https://docs.rs/sqlx/latest/sqlx/trait.ColumnIndex.html

// Not possible
impl sqlx::ColumnIndex<MySqlRow> for TryGetColumn { ... }
impl sqlx::ColumnIndex<SqliteRow> for TryGetColumn { ... }
impl sqlx::ColumnIndex<PgRow> for TryGetColumn { ... }

Then, we have no choice but to store String / usize and supply that to SQLx's try_get. So, I afraid this is the "only" way?

pub enum TryGetColumn {
    Name(String),
    Index(usize),
}

pub trait IntoTryGetColumn {
    fn into_try_get_column() -> TryGetColumn;
}

@tyt2y3
Copy link
Member

tyt2y3 commented Jan 9, 2023

Well, no and yes and no

Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Self approve

@tyt2y3 tyt2y3 merged commit 4210526 into master Jan 10, 2023
@tyt2y3 tyt2y3 deleted the select-into-tuple branch January 10, 2023 07:20
@billy1624 billy1624 mentioned this pull request Jan 11, 2023
@billy1624 billy1624 added this to the 0.11.x milestone Jan 31, 2023
billy1624 added a commit to SeaQL/seaql.github.io that referenced this pull request Feb 2, 2023
tyt2y3 added a commit to SeaQL/seaql.github.io that referenced this pull request Feb 3, 2023
* Update 02-writing-migration.md

* Update SeaORM/docs/03-migration/02-writing-migration.md

* Support various UUID formats that are available in `uuid::fmt` module (SeaQL/sea-orm#1325)

* Casting columns as a different data type on select, insert and update (SeaQL/sea-orm#1304)

* Methods of `ActiveModelBehavior` receive db connection as a parameter (SeaQL/sea-orm#1145, SeaQL/sea-orm#1328)

* Added `execute_unprepared` method to `DatabaseConnection` and `DatabaseTransaction` (SeaQL/sea-orm#1327)

* Added `Select::into_tuple` to select rows as tuples (instead of defining a custom Model) (SeaQL/sea-orm#1311)

* Generate `#[serde(skip)]` for hidden columns (SeaQL/sea-orm#1171, SeaQL/sea-orm#1320)

* Generate entity with extra derives and attributes for model struct (SeaQL/sea-orm#1124, SeaQL/sea-orm#1321)

* Generate entity with extra derives and attributes for model struct (SeaQL/sea-orm#1124, SeaQL/sea-orm#1321)

* async_trait

* Migrations are now performed inside a transaction for Postgres (SeaQL/sea-orm#1379)

* `MockDatabase::append_exec_results()`, `MockDatabase::append_query_results()`, `MockDatabase::append_exec_errors()` and `MockDatabase::append_query_errors()` take any types implemented `IntoIterator` trait (SeaQL/sea-orm#1367)

* Cleanup the use of `vec!` macros

* Added `DatabaseConnection::close` (SeaQL/sea-orm#1236)

* Added `ActiveValue::reset` to convert `Unchanged` into `Set` (SeaQL/sea-orm#1177)

* Added `QueryTrait::apply_if` to optionally apply a filter (SeaQL/sea-orm#1415)

* Added the `sea-orm-internal` feature flag to expose some SQLx types (SeaQL/sea-orm#1297, SeaQL/sea-orm#1434)

* Add `QuerySelect::columns` method - select multiple columns (SeaQL/sea-orm#1264)

* Edit

* Update SeaORM/docs/02-install-and-config/02-connection.md

Co-authored-by: Chris Tsang <chris.2y3@outlook.com>

* Update SeaORM/docs/05-basic-crud/03-insert.md

Co-authored-by: Chris Tsang <chris.2y3@outlook.com>

* fmt

* Edit

---------

Co-authored-by: Chris Tsang <chris.2y3@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants