Support citext column type

[rmarronnier]

Sometimes a case insensitive text column type might be useful : luckyframework/authentic#51

citext is exactly that.

I was thinking about a CIString or Citext crystal type to map a citext column, but maybe an option (case_sensitive: false) could be better.

[jwoertink]

I can see how that could be useful, but I'm not sure if this is something we could support directly right now. There's a TON of things that postgres can do, and being able to be 1-1 with postgres while trying to maintain type-safety and all that is a pretty big undertaking. One of my big worries is that crystal itself doesn't have a String like object that can be treated as case-insensitive which means that anywhere == would happen (i.e. validation or whatnot), we have to take in to consideration of calling .downcase on both the column value and the param value. So it wouldn't be just the database column, but really the whole app. This could make for potentially weird side affects in Operations.

Instead, I think we should put some focus into #497. This would allow you to roll your own custom type and then it would be up to you on how you handle it.

Right now you can make a custom type, but it's just a bit of work. It would look something like this:

class Citext
  def blank?
    false
  end

  module Lucky
    alias ColumnType = String
    include Avram::Type

    def parse(value : String)
      if !value.blank?
        SuccessfulCast(Citext).new(parsed_value)
      else
        FailedCast.new
      end
    end

    def to_db(value : String)
      value
    end

    # Essentially just need to do what String does https://github.com/luckyframework/avram/blob/master/src/avram/charms/string_extensions.cr
    # but change whatever needs to change for a citext column
    class Criteria(T, V) < Avram::Criteria(T, V)
    end
  end
end

 # only needed if you wanted to use `add field : Citext` in your migrations
# Otherwise you'd use raw SQL
module Avram::Migrator::Columns
  class CitextColumn(T) < Base
    @default : T | Nil = nil

    def initialize(@name, @nilable, @default)
    end

    def column_type : String
      "citext"
    end
  end
end

Then in your model:

class User < BaseModel
  table do
    column username : Citext
  end
end

What are your thoughts on that?

[rmarronnier]

Thank you very much for your comprehensive answer and code examples.

Before going further in the discussion, I have several assumptions about the Lucky ecosystem which are maybe mistaken :

• Lucky shares with Rails the convention over configuration philosophy (a common problem should have one solution provided by the framework). Worded differently, Lucky is opiniated.

• Avram is a good example for this : it's an ORM, but only works with Postgresql. Lucky expects a project to use Avram. You can use a different ORM, but you'll have to fight conventions to make it work with another ORM.

With these assumptions, it feels more coherent/efficient to solve lucky issues as upstream as possible. The original issue (luckyframework/authentic#9) which led me here is/will be present in all shards/projects that implement email as login. So if we create for example a Citext custom type in Authentic, Shield users and future authentification shards will have to do the same or implement a different fix.

There's a TON of things that postgres can do, and being able to be 1-1 with postgres while trying to maintain type-safety and all that is a pretty big undertaking.

I trust you 💯 on this. Designing an ORM is way over my skills.

One of my big worries is that crystal itself doesn't have a String like object that can be treated as case-insensitive which means that anywhere == would happen (i.e. validation or whatnot), we have to take in to consideration of calling .downcase on both the column value and the param value. So it wouldn't be just the database column, but really the whole app. This could make for potentially weird side affects in Operations.

You're right, it doesn't only involve SELECT and INSERT db operations. Maybe creating a Citext custom type is a maintenance nightmare on Avram side, so what about a case_insensitive option that only affect db migrations ? From a Avram/Crystal point of view, the column type will be treated as a String and all non-db based operations / error handling / side effects will be handled by the shard / downstream developer ?

All I want to say : this kind of decision is technically way over my head, but I can't help thinking that not implementing a part of Citext support upstream in Avram is a missed opportunity.

[jwoertink]

Lucky shares with Rails the convention over configuration philosophy

You're not far off. Rails is like "do things how we do them, and everything just magically works". Lucky is more like "we recommend you do things our way because it's the most type-safe; however, here's an escape hatch so you can do things your own way". So we do have a convention, but that convention is based on a specific configuration.

One thing in rails is that if you break convention, then your code becomes 10x more complicated. We are currently doing the same thing, but I have a goal of not doing that. Breaking from the normal set shouldn't increase your dev time and complexity (like with this citext).

I'll have to think about the case_insensitive option. That might be an easy hook, and then just let the developer handle side affects from it. Or maybe we open it even more and allow a hook in to the column generation so you can add your own additional bits 🤔

# just thinking out loud here...
class Migration < V1

def change
  create table_for(User) do
    add username : String, modifier: CitexModifier
  end
end

end

module CitexModifier
  include Lucky::ColumnModifier
  def column_type : String
    "citext"
  end
end

Maybe you want varchar(50), or to add some other bits? I'm still not sure on how the model and operations and query objects would be affected in this case, but I like the discussion. I appreciate your suggestion on this!

[rmarronnier]

One thing in rails is that if you break convention, then your code becomes 10x more complicated. We are currently doing the same thing, but I have a goal of not doing that.

It's tough doing that, but documentation and code examples help tremendously with that (I succeeded doing a Lucky API project with no database thanks to the wizard and lucky website source code).

As for your CitextModifier suggestion, it got me thinking.

I just need a non-raw SQL way to declare a Citext column as an immediate improvement.

it would be great if there was an unsafe but generic way (à la raw_html) to set the column type, like this :

class Migration < V1

def change
enable_extension "citext"
  create table_for(User) do
    add username : String, column_type: "citext"
  end
end

end

The column_type argument would override the default and be useful for other specific pg column type configurations.

Maybe you want varchar(50), or to add some other bits?
citextis already a datatype (there is no civarchar :-) )

The more I think about it, implementing a novel Citext custom type could be avoided. Most of the outside db logic has to be implemented anyway. I like the discussion too :-)

[matthewmcgarvey]

A problem with supporting citext right now is will/crystal-pg#43

[matthewmcgarvey]

This is blocked until will/crystal-pg#221 is merged and released