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

Add Support for Unsigned Types in Mysql #1561

Closed

joshleeb wants to merge 4 commits into diesel-rs:master from joshleeb:mysql-unsigned

Contributor

joshleeb commented Feb 17, 2018

Ref. Handling unsigned integer types from MySQL #1181

This functionality was originally added in #782 and then reverted in #912.

joshleeb changed the title ~~Add support for unsigned types in mysql~~ [WIP] Add Support for Unsigned Types in Mysql

Member

sgrif commented Feb 17, 2018

Tests are failing

sgrif reviewed

View reviewed changes

diesel/src/mysql/types/mod.rs Outdated

@@ @@ -12,6 +12,12 @@ use mysql::Mysql; @@
               use serialize::{self, IsNull, Output, ToSql};
               use sql_types;
+              impl sql_types::HasSqlType<sql_types::Unsigned<sql_types::Integer>> for Mysql {

Member

sgrif Feb 17, 2018

This needs to support more than just Integer doesn't it?

Contributor Author

joshleeb Feb 17, 2018

Yep, is it just SmallInt, Integer, and BigInt?

Member

sgrif Feb 17, 2018

This impl can probably just be generic over everything.

Member

Eijebong Feb 17, 2018

Unsigned strings let's go !

Member

sgrif Feb 17, 2018

There wouldn't be a corresponding AsExpression impl, so... :P

There's nothing to prevent you from putting Unsigned<Text> in your table declaration already

sgrif reviewed

View reviewed changes

diesel/src/sql_types/mod.rs Outdated

+                  type QueryId = ();
+              }
+              // impl<ST: NotNull + SingleValue> QueryId for ST {

Member

sgrif Feb 17, 2018

✂️

Member

sgrif commented Feb 17, 2018

You're missing AsExpression and FromSqlRow derives

YetAnotherMinion mentioned this pull request

add support for the Point type on PG. #1566

Closed

Contributor Author

joshleeb commented Feb 25, 2018

@sgrif it seems as though, for the u32_to_sql_integer tests that the newly defined ToSql<Unsigned<Integer>>... implementation isn't being called and I'm not really sure why.

Member

sgrif commented Feb 25, 2018

It's being called, and it's failing

sgrif reviewed

View reviewed changes

diesel/src/sql_types/mod.rs Outdated

@@ @@ -70,7 +72,7 @@ pub struct Tinyint; @@
               /// - [`i16`][i16]
               ///
               /// [i16]: https://doc.rust-lang.org/nightly/std/primitive.i16.html
-              #[derive(Debug, Clone, Copy, Default, QueryId, SqlType)]
+              #[derive(Debug, Clone, Copy, Default, QueryId, SqlType, FromSqlRow)]

Member

sgrif Feb 25, 2018

This type should not be deriving FromSqlRow, nor should any other SQL types.

sgrif reviewed

View reviewed changes

diesel/src/sql_types/mod.rs Outdated

+              /// The unsigned SQL type.
+              /// TODO: Add proper documentation.
+              #[derive(Debug, Clone, Copy, Default, SqlType, FromSqlRow, AsExpression)]
+              pub struct Unsigned<ST: NotNull + SingleValue + QueryId + Expression + AsExpression<ST>>(ST);

Member

sgrif Feb 25, 2018

This should be in the mysql module, it's backend specific.

sgrif reviewed

View reviewed changes

diesel/src/sql_types/mod.rs Outdated

+                  type SqlType = BigInt;
+              }
+              impl QueryId for u16 {

Member

sgrif Feb 25, 2018

None of these should have QueryId implemented.

sgrif reviewed

View reviewed changes

diesel/src/sql_types/mod.rs Outdated

+                  const HAS_STATIC_QUERY_ID: bool = ST::HAS_STATIC_QUERY_ID;
+              }
+              impl Expression for SmallInt {

Member

sgrif Feb 25, 2018

None of these should have Expression implemented.

sgrif reviewed

View reviewed changes

diesel/src/sql_types/mod.rs Outdated

@@ @@ -467,3 +481,37 @@ impl<T: NotNull> IntoNullable for Nullable<T> { @@
               pub trait SingleValue {}
               impl<T: NotNull + SingleValue> SingleValue for Nullable<T> {}
+              impl<ST: NotNull + SingleValue + QueryId + Expression + AsExpression<ST>> QueryId for Unsigned<ST> {
+                  type QueryId = ST::QueryId;

Member

sgrif Feb 25, 2018

This should be Unsigned<ST::QueryId>.

sgrif reviewed

View reviewed changes

diesel/src/type_impls/integers.rs Outdated

@@ @@ -88,3 +88,85 @@ impl<DB: Backend> ToSql<sql_types::BigInt, DB> for i64 { @@
                           .map_err(|e| Box::new(e) as Box<Error + Send + Sync>)
                   }
               }
+              impl<DB: Backend<RawValue = [u8]>> FromSql<sql_types::SmallInt, DB> for u16 {

Member

sgrif Feb 25, 2018

This should only be implemented for Unsigned<SmallInt>.

Member

sgrif Feb 25, 2018

Also all of these implementations are backend specific, they should not be generic like this.

sgrif reviewed

View reviewed changes

diesel/src/type_impls/integers.rs Outdated

+                  }
+              }
+              impl<DB: Backend> ToSql<sql_types::SmallInt, DB> for u16 {

Member

sgrif Feb 25, 2018

This should only be implemented for Unsigned<SmallInt>.

sgrif reviewed

View reviewed changes

diesel/src/type_impls/integers.rs Outdated

+                  }
+              }
+              impl<DB: Backend<RawValue = [u8]>> FromSql<sql_types::Integer, DB> for u32 {

Member

sgrif Feb 25, 2018

This should only be implemented for Unsigned<Integer>.

sgrif reviewed

View reviewed changes

diesel/src/type_impls/integers.rs Outdated

+                  }
+              }
+              impl<DB: Backend> ToSql<sql_types::Integer, DB> for u32 {

Member

sgrif Feb 25, 2018

This should only be implemented for Unsigned<Integer>.

sgrif reviewed

View reviewed changes

diesel/src/type_impls/integers.rs Outdated

+                  }
+              }
+              impl<DB: Backend<RawValue = [u8]>> FromSql<sql_types::BigInt, DB> for u64 {

Member

sgrif Feb 25, 2018

This should only be implemented for Unsigned<BigInt>.

sgrif reviewed

View reviewed changes

diesel/src/type_impls/integers.rs Outdated

+                  }
+              }
+              impl<DB: Backend> ToSql<sql_types::BigInt, DB> for u64 {

Member

sgrif Feb 25, 2018

This should only be implemented for Unsigned<BigInt>.

sgrif reviewed

View reviewed changes

diesel/src/type_impls/unsigned.rs Outdated

+              use query_builder::QueryId;
+              use sql_types::*;
+              impl<T, DB> HasSqlType<Unsigned<T>> for DB

Member

sgrif Feb 25, 2018

This should be MySQL specific.

sgrif reviewed

View reviewed changes

diesel/src/mysql/types/mod.rs Outdated

+              pub struct Unsigned<ST: NotNull + SingleValue + QueryId>(ST);
+              #[doc(hidden)]
+              pub type UInt2 = Unsigned<SmallInt>;

Member

sgrif Mar 4, 2018

Why do we need these types?

Member

sgrif commented Mar 4, 2018

@joshleeb Since CI is still red, and there's still a bunch of debug code in there I'm assuming this is still WIP. I'm hoping to release 1.2 this week, so let me know what I can do to help get this done.

Contributor Author

joshleeb commented Mar 5, 2018 •

edited

Loading

So far the added tests for unsigned types are passing but some documentation still needs to be added.

joshleeb changed the title ~~[WIP] Add Support for Unsigned Types in Mysql~~ Add Support for Unsigned Types in Mysql

Contributor Author

joshleeb commented Mar 6, 2018 •

edited

Loading

@sgrif I'm not sure what's causing the compile-fail tests to fail here.

Member

sgrif commented Mar 7, 2018

Your code has changed the errors that occur. You should look at the differences, if they seem reasonable, update the tests.


          Support unsigned types for mysql

68c0de1

weiznich mentioned this pull request

Implement diesel Expression for more types #80

Closed

sclarson commented Mar 20, 2018

Thanks for doing this. Just started trying diesel out on our existing database today and ran into this almost immediately.

Also it looks like the tests are all passing now 👍

weiznich reviewed

View reviewed changes

diesel/src/mysql/types/mod.rs Outdated

+              #[derive(Debug, Clone, Copy, Default, SqlType)]
+              pub struct Unsigned<ST: NotNull + SingleValue + QueryId>(ST);
+              impl ToSql<Unsigned<Integer>, Mysql> for u16 {

Member

weiznich Mar 20, 2018

Shouldn't this be impl ToSql<Unsigned<SmallInt>, Mysql> for u16?

diesel/src/mysql/types/mod.rs Outdated

+                  }
+              }
+              impl ToSql<Unsigned<Integer>, Mysql> for u64 {

Member

weiznich Mar 20, 2018

Should this be impl ToSql<Unsigned<BigInt>, Mysql> for u64 ?

diesel/src/mysql/types/mod.rs

                   fn from_sql(bytes: Option<&[u8]>) -> deserialize::Result<Self> {
                       Ok(not_none!(bytes).iter().any(|x| *x != 0))
                   }
               }
+              impl<ST> HasSqlType<Unsigned<ST>> for Mysql

Member

weiznich Mar 20, 2018

I'm not sure if this is a good idea, because this will allow also things like Unsigned<Bool> to be valid in some contexts. (Note for @diesel-rs/reviewers)

Contributor Author

joshleeb Mar 21, 2018

@weiznich would it be better to have a HasSqlType for SmallInt, Integer, and BigInt, instead of this more generic version?

Member

sgrif Mar 26, 2018

I don't think the HasSqlType impl being generic is a problem. It doesn't enable anything on its own.

diesel/src/mysql/types/mod.rs Outdated

+                  fn build(row: Self::Row) -> Self {
+                      row
+                  }
+              }

Member

weiznich Mar 20, 2018

Here it seems like the impl for u32 is missing?

sclarson Mar 21, 2018

I went to add this and doing so causes a compilation error. It looks like this implementation is already done by https://github.com/diesel-rs/diesel/blob/master/diesel/src/type_impls/primitives.rs#L38

Contributor Author

joshleeb Mar 21, 2018

I'll add similar structs to /diesel/src/type_impls/primitives.rs for u16 and u64. I imagine that's better than implementing the traits manually here.

diesel/src/mysql/types/mod.rs Outdated

+                  fn build_from_row<T: Row<Mysql>>(row: &mut T) -> deserialize::Result<Self> {
+                      Self::from_sql(row.take())
+                  }
+              }

Member

weiznich Mar 20, 2018

Also: Is the impl for u32 missing?

diesel_infer_schema/infer_schema_internals/src/mysql.rs Outdated

		@@ -101,14 +103,22 @@ fn determine_type_name(sql_type_name: &str) -> Result<&str, Box<Error>> {
		};

		if result.to_lowercase().contains("unsigned") {

Member

weiznich Mar 20, 2018

I think this condition should be replaced with a call to determine_unsigned

sclarson2 and others added 3 commits

March 20, 2018 22:49


          Update u16 impls to smallint

3f4b1d8


          Merge pull request #1 from sclarson/mysql-unsigned

c0c17dc

Update u16 impls to smallint


          Add U16Proxy and U64Proxy to type_impl primitives

aa1907f

sclarson commented Mar 21, 2018

The travis build appears to be breaking on compiling cargo metadata. That seems like it should be unrelated to the change here.

sgrif mentioned this pull request

Rebase of #1561 #1618

Merged

sgrif closed this in #1618

sgrif added a commit that referenced this pull request


          Merge pull request #1618 from diesel-rs/joshleeb-mysql-unsigned

af85985

Rebase of #1561

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment