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.

Sign up for GitHub

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

Jump to bottom

issues-336 Third try impl postgres-array #467

Merged
merged 13 commits into from
Oct 16, 2022
Merged

issues-336 Third try impl postgres-array #467

merged 13 commits into from
Oct 16, 2022

Conversation

ikrivosheev
Copy link
Member

@ikrivosheev ikrivosheev commented Oct 9, 2022 •
edited
Loading

PR Info

Other PR:

This is the third try to impl postgres-array in SeaQuery. Here I store type information into Value and then try to convert Vec<Value> into Vec<T>. This look safe.

Adds

  • Support one dimension Postgres Array into sea-query-binder
  • enum ArrayType with all possible types
  • Add Value::except method

Breaking Changes

  • ColumnType::Array store Box<ColumnType> instead string
  • Value::Array store addition field: ArrayType, where store type of array elements

Changes

  • Small improvement Iden::to_string

@ikrivosheev
Copy link
Member Author

@tyt2y3 @billy1624 can you look next my idea?)

@ikrivosheev ikrivosheev mentioned this pull request Oct 9, 2022
Closed
10 tasks
@Sytten
Copy link
Contributor

Sytten commented Oct 9, 2022

For me any solution that cant be extended to support custom types is a no go. That is why the trait based solution is used by all other libraries.

@ikrivosheev
Copy link
Member Author

For me any solution that cant be extended to support custom types is a no go. That is why the trait based solution is used by all other libraries.

I try to implement trait solution, but got a lot of problems. I still don't understand how to solve them all...

tyt2y3
tyt2y3 reviewed Oct 10, 2022
View reviewed changes
sea-query-binder/Cargo.toml
@@ -17,7 +17,7 @@ rust-version = "1.60"
[lib]

[dependencies]
sea-query = { version = "^0", path = ".." }
sea-query = { version = "^0", path = "..", features = ["thread-safe"] }
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 10, 2022 •
edited
Loading

This looks good, especially as an intermediate step. The crate architecture has been revamped in 0.27 that is waiting for a release. What's important now is to roll out the next iteration of SeaORM. And maybe I can give a try on ValueTrait sometime after that.

@ikrivosheev ikrivosheev force-pushed the feature/issues-336_array_support_v3 branch from 835707e to ac92bd1 Compare October 10, 2022 17:31
@ikrivosheev
Copy link
Member Author

@tyt2y3 @billy1624 @Sytten hello! I have done with some part of this PR.

  1. Create new enum: ArrayType
  2. Add support for Value::Array for sqlx/postgres
  3. Extend trait ValueType: fn array_type() -> ArrayType and fn except(v: Value, msg: &str) -> Self

TODO:

  1. Add support for Vec<Option<T>> - don't know how to do it yet

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 11, 2022

Great work! I think now I have an idea as to how ValueTrait would work with SQLx, I am willing to give a try after this settled and we released 0.27

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 12, 2022

This seems to be very nearly done?

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 12, 2022 •
edited
Loading

@ikrivosheev check this 79625e0 , not sure if it really works at downstream, but it compiles

@ikrivosheev
Copy link
Member Author

ikrivosheev commented Oct 12, 2022 •
edited
Loading

This seems to be very nearly done?

I want to add support for Vec<Option<T>>

@ikrivosheev ikrivosheev marked this pull request as ready for review October 12, 2022 17:46
@ikrivosheev
Copy link
Member Author

ikrivosheev commented Oct 12, 2022 •
edited
Loading

This seems to be very nearly done?

I want to add support for Vec<Option<T>>

Or later...

@billy1624 @tyt2y3 I updated the description. Can you review the PR?

tyt2y3
tyt2y3 reviewed Oct 13, 2022
View reviewed changes
sea-query-binder/Cargo.toml
sqlx = { version = "^0.6.1", optional = true }
serde_json = { version = "^1", optional = true }
chrono = { version = "^0.4", default-features = false, features = ["clock"], optional = true }
Copy link
Member

@tyt2y3 tyt2y3 Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

can we pub all these from a special export module in sea-query, then we don't have to specify the dependency twice?

I can imagine the two Cargo.toml might go out of sync in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to make our third-party libraries as part of public interface?

Copy link
Member

Choose a reason for hiding this comment

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

Good point

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 14, 2022

Cool enough. @billy1624 thoughts?

billy1624
billy1624 reviewed Oct 14, 2022
View reviewed changes
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Wow!! That's smart!

src/value.rs Outdated
Comment on lines 221 to 223
fn except(v: Value, msg: &str) -> Self {
Self::try_from(v).expect(msg)
}
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to rename it as expect? Since it's a wrapper method of try_from then expect.

This also align with the API of... few lines above it

fn unwrap(v: Value) -> Self {
    Self::try_from(v).unwrap()
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix!

src/value.rs Outdated
Comment on lines 278 to 283
pub fn except<T>(self, msg: &str) -> T
where
T: ValueType,
{
T::except(self, msg)
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix!

billy1624
billy1624 reviewed Oct 14, 2022
View reviewed changes
sea-query-binder/src/sqlx_postgres.rs Outdated
Comment on lines 134 to 138
ArrayType::Bool => {
let value: Option<Vec<bool>> =
Value::Array(ty, v).except("Invalid type for array value");
args.add(value)
}
Copy link
Member

Choose a reason for hiding this comment

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

I think ordinary user won't see this message (unless someone intentionally construct a vector of boxed value with different value variants in it) but the error message itself can be more specific.

Value::Array(ty, v).except("This Value::Array should consist of Value::Bool");

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

billy1624
billy1624 reviewed Oct 14, 2022
View reviewed changes
src/value.rs Outdated
@@ -222,7 +323,7 @@ macro_rules! type_to_value {
}

macro_rules! type_to_box_value {
( $type: ty, $name: ident, $col_type: expr ) => {
( $type: ty, $name: ident, $col_type: expr, $array_type: expr ) => {
Copy link
Member

@billy1624 billy1624 Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nitpick: a trivial refactoring

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

@billy1624
Copy link
Member

billy1624 commented Oct 14, 2022 •
edited
Loading

Cool enough. @billy1624 thoughts?

Looks good to me except comments above :D

@ikrivosheev
Merge pull request #1 from billy1624/pr/467
21f115e 
Reuse $name ident in type_to_box_value macro for ArrayType variant
@ikrivosheev
issues-336 Fix after review
d5424aa
@ikrivosheev
Copy link
Member Author

@billy1624 thank you for review! I corrected all comments

@ikrivosheev ikrivosheev self-assigned this Oct 14, 2022
billy1624
billy1624 reviewed Oct 15, 2022
View reviewed changes
src/value.rs Outdated
@@ -185,6 +274,13 @@ impl Value {
{
T::unwrap(self)
}

pub fn except<T>(self, msg: &str) -> T
Copy link
Member

Choose a reason for hiding this comment

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

Are we keeping this as except?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! Fix

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 16, 2022

One last thing, can we also update the sqlx-postgres example (with several Array types) and make it part of our CI?

@tyt2y3 tyt2y3 merged commit 9c2a6b2 into SeaQL:master Oct 16, 2022
@ikrivosheev ikrivosheev deleted the feature/issues-336_array_support_v3 branch October 16, 2022 08:33
@kudlatyamroth kudlatyamroth mentioned this pull request Oct 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@billy1624 billy1624 billy1624 left review comments

@tyt2y3 tyt2y3 Awaiting requested review from tyt2y3

Assignees

@ikrivosheev ikrivosheev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Query binders: Add support for postgres arrays
4 participants
@ikrivosheev @Sytten @tyt2y3 @billy1624