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

Printed schema contains Citext types #1624

Closed
alexfedoseev opened this Issue Apr 7, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@alexfedoseev

alexfedoseev commented Apr 7, 2018

Setup

I'm not sure I understand what Setup section means but I'm using diesel with hyper, r2d2 and number of other packages to build web server.

Versions

  • Rust: rustc 1.26.0-nightly (e5277c145 2018-03-28)
  • Diesel: { version = "=1.1.1", features = ["postgres", "chrono"] }
  • Database: postgres:10.0
  • Operating System macOS 10.12.6

Feature Flags

  • diesel: ["postgres", "chrono"]

Problem Description

I use citext extension in postgres db and when I print schema by running diesel print-schema output contains Citext types. AFAIU this type was added then removed. I'm not sure if it's a bug (that generated output contains Citext type) or should I handle this type in my app somehow? For now I just manually replace it w/ Text.

What are you trying to accomplish?

I'm trying to generate valid schema w/ fields of citext type.

What is the expected output?

I'm not sure :)

What is the actual output?

Printed schema contains Citext types.

Are you seeing any additional errors?

Nope, only errors related to unavailable Citext type.

Steps to reproduce

Run diesel print-schema against postgres db which uses citext extension.

Checklist

  • I have already looked over the issue tracker for similar issues.
@sgrif

This comment has been minimized.

Member

sgrif commented Apr 7, 2018

diesel print-schema will happily output whatever database types you have, regardless of whether they're supported by Diesel or not. There are many extensions to PG which have support added outside of Diesel. You can change the table definition to Text if you want, but you'll run into the same problem that led to the removal of that alias in the first place (see #1215)

@sgrif sgrif closed this Apr 7, 2018

@alexfedoseev

This comment has been minimized.

alexfedoseev commented Apr 8, 2018

@sgrif Yeah, Text is a temporary workaround only to compile the app. Can you give some hints how it should be handled the right way?

@sgrif

This comment has been minimized.

Member

sgrif commented Apr 8, 2018

You'd need to fully implement it as a custom type, like this test

@alexfedoseev

This comment has been minimized.

alexfedoseev commented Apr 8, 2018

Gotcha, thanks!

@Boscop

This comment has been minimized.

Boscop commented Aug 30, 2018

But then how to support case-insensitive string comparison with a custom Citext type?

@sgrif

This comment has been minimized.

Member

sgrif commented Aug 30, 2018

@alexfedoseev

This comment has been minimized.

alexfedoseev commented Sep 17, 2018

Recent activity reminded me about this task so I tried to implement it on the weekend.

AFAIU from the history, the issue w/ this type is that citext_column.eq() performs a case-sensitive comparison. So I implemented ToSql/FromSql using Text type and then tried to impl ExpressionMethods for Citext which gave me an error I couldn't resolve.

@sgrif Can you take a look if what I did makes sense (just requires some adjustments) or I did something stupid and it should be implemented differently? Thanks in advance!

use std::io::Write;

use diesel::deserialize::{self, FromSql};
use diesel::expression::operators::Eq;
use diesel::expression::{AsExpression, Expression};
use diesel::expression_methods::ExpressionMethods;
use diesel::pg::Pg;
use diesel::serialize::{self, Output, ToSql};
use diesel::sql_types::Text;

#[derive(Debug, Clone, Copy, SqlType)]
#[postgres(type_name = "citext")]
pub struct Citext;

impl ToSql<Citext, Pg> for String {
  fn to_sql<W: Write>(&self, out: &mut Output<W, Pg>) -> serialize::Result {
    ToSql::<Text, Pg>::to_sql(self, out)
  }
}

impl ToSql<Citext, Pg> for str {
  fn to_sql<W: Write>(&self, out: &mut Output<W, Pg>) -> serialize::Result {
    ToSql::<Text, Pg>::to_sql(self, out)
  }
}

impl FromSql<Citext, Pg> for String {
  fn from_sql(bytes: Option<&[u8]>) -> deserialize::Result<Self> {
    FromSql::<Text, Pg>::from_sql(bytes)
  }
}

impl Expression for Citext {
  type SqlType = Text;
}

impl ExpressionMethods for Citext {
  // default implementation from current source
  fn eq<T: AsExpression<Self::SqlType>>(self, other: T) -> Eq<Self, T::Expression> {
    Eq::new(self, other.as_expression())
  }
}

But it gives me the following error:

error[E0119]: conflicting implementations of trait `diesel::ExpressionMethods` for type `db::types::Citext`:
  --> server/src/db/types.rs:37:1
   |
37 | impl ExpressionMethods for Citext {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: conflicting implementation in crate `diesel`:
           - impl<T> diesel::ExpressionMethods for T
             where T: diesel::Expression, <T as diesel::Expression>::SqlType: diesel::sql_types::SingleValue;

I'm not sure why it conflicts since Expression is implemented for Citext and SingleValue should be derived for T:SqlType.

My guess is that diesel internally implements ExpressionMethods somehow but then I'm not sure how I can re-implement it for custom type.

@sgrif

This comment has been minimized.

Member

sgrif commented Sep 18, 2018

There's no reason that you'd need to do anything related to ExpressionMethods or expressions, just adding a Citext SQL type is sufficient. I wouldn't couple to any concrete types for ToSql/FromSql impls, just allow anything that impls for Text

@alexfedoseev

This comment has been minimized.

alexfedoseev commented Sep 19, 2018

There's no reason that you'd need to do anything related to ExpressionMethods or expressions, just adding a Citext SQL type is sufficient.

The reason why I get there is that in #1215 you said that citext_column.eq method is problematic and I assumed that this method should be re-defined for Citext. Apparently, I was wrong and I need dig into problem (and diesel domain) deeper to understand why.

I wouldn't couple to any concrete types for ToSql/FromSql impls, just allow anything that impls for Text

I'm not sure I understand this part either. In all examples I've seen (1, 2) there is re-implementation using some base diesel type, e.g.:

impl FromSql<Citext, Pg> for String {
  fn from_sql(bytes: Option<&[u8]>) -> deserialize::Result<Self> {
    FromSql::<Text, Pg>::from_sql(bytes)
  }
}

But you're saying to allow anything that impls for Text and I'm not sure how to do this.

Sorry for lots of questions!

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