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

Replace Value enum with dyn QueryValue #146

Closed
wants to merge 6 commits into from

Conversation

tqwewe
Copy link

@tqwewe tqwewe commented Sep 29, 2021

This PR deprecated the old Value enum, and creates a new struct: Value(Box<dyn QueryValue>).

Any type that implements the new trait QueryValue can be used as a Value.

/// Indicates that a type is supported for use in SQL queries.
pub trait QueryValue: Clone + PartialEq {
    /// Returns the value as an escaped string safe for use in SQL queries.
    fn query_value(&self, query_builder: &dyn QueryBuilder) -> String;
}

Many of the primitive types implement QueryValue, including:
&str, String, i8, i16, i32, i64, u8, u16, u32, u64, f32, f64, bool, () (represents NULL), Vec<u8>, serde_json::Value, chrono::NaiveDate, chrono::NaiveTime, chrono::NaiveDateTime, chrono::DateTime<chrono::FixedOffset>, uuid::Uuid, rust_decimal::Decimal, bigdecimal::BigDecimal.

This means they can be used in queries (as they would previously), and calling .into() where necessary.

The old enum Value has been renamed to PrimitiveValue and has been marked as deprecated. This PR updates sea-query's version to 0.17.0. If this needs to be changed, let me know :)

@tqwewe tqwewe marked this pull request as draft September 29, 2021 05:21
@tyt2y3
Copy link
Member

tyt2y3 commented Sep 29, 2021

Thank you for the effort.

There seems to be a lot of useful ideas!

  1. ().into()
  2. the Default on ColumnSpec
  3. V: Into<Value> on Update::value

And the impact to users is seemingly small.

My only wonder is, what new possibilities does this change bring to users?

@tqwewe
Copy link
Author

tqwewe commented Sep 30, 2021

My only wonder is, what new possibilities does this change bring to users?

I finally can answer this question with the PR in Sea ORM SeaQL/sea-orm#214

It allows support for CAST($1 as type), which lets you cast into custom types, such as enums in Postgres.

PrimitiveValue::BigUnsigned(value) => value.query_value(query_builder),
PrimitiveValue::Float(value) => value.query_value(query_builder),
PrimitiveValue::Double(value) => value.query_value(query_builder),
PrimitiveValue::String(value) => value.as_deref().cloned().query_value(query_builder),
Copy link
Author

Choose a reason for hiding this comment

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

I couldn't find a way to do this without .cloned(). I think some changes need to be made in impl<T> QueryValue for Option<T>, but I had issues getting it to work.

@tqwewe
Copy link
Author

tqwewe commented Oct 17, 2021

With the new issue made SeaQL/sea-orm#252, I wil close this PR for now. If needed it can be reopened.

On a side note, I feel over using dynamic trait objects is tempting but not always the solution.

@tqwewe tqwewe closed this Oct 17, 2021
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.

2 participants