tmiyamon / acts-as-taggable-array-on

A simple and high performance tagging gem for Rails using PostgreSQL array.
MIT License
125 stars 26 forks source link

Adding support for ::text typed array #12

Closed skatkov closed 6 years ago

skatkov commented 6 years ago

I've ran into issues, my setup includes:

If have following model:

class Product < ApplicationRecord
  acts_as_taggable_array_on :categories
end

Migration is as recommended in README. But then I use Product.withoutany{tags} i ran into casting issue. Example:

2.5.0 :001 > Product.all_categories
   (1.2ms)  SELECT tag FROM (SELECT DISTINCT unnest(products.categories) as tag FROM "products") subquery /*application:Amzstore*/
 => ["testqa"]
2.5.0 :002 > Product.without_any_categories('testqa')
  Product Load (1.2ms)  SELECT  "products".* FROM "products" WHERE NOT (categories && ARRAY['testqa']::varchar[]) LIMIT $1 /*application:Amzstore*/  [["LIMIT", 11]]
Traceback (most recent call last):
ActiveRecord::StatementInvalid (PG::UndefinedFunction: ERROR:  operator does not exist: text[] && character varying[]
LINE 1: ...products".* FROM "products" WHERE NOT (categories && ARRAY['...
                                                             ^
HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.
: SELECT  "products".* FROM "products" WHERE NOT (categories && ARRAY['testqa']::varchar[]) LIMIT $1 /*application:Amzstore*/)

Following documentation refers to two two possible types in PG ARRAY -- text and integer. https://www.postgresql.org/docs/9.6/static/arrays.html

Varchar is not listed as supported type. So I changed current implementation and it works for my case (i'm not sure if it work for older PG version though)

skatkov commented 6 years ago

I double checked migration again and here is what I have: add_column :products, :categories, :text, array:true, nil: false, default: []

But readme proposes to use :string

skatkov commented 6 years ago

If we would look into this article: https://www.depesz.com/2010/03/02/charx-vs-varcharx-vs-varchar-vs-text/

To sum it all up:
    char(n) – takes too much space when dealing with values shorter than n, and can lead to subtle errors because of adding trailing spaces, plus it is problematic to change the limit
    varchar(n) – it's problematic to change the limit in live environment
    varchar – just like text
    text – for me a winner – over (n) data types because it lacks their problems, and over varchar – because it has distinct name
  1. There is no point for 'acts-as-taggable-array-on' to enforce varchar over text type. it's basically the same.
  2. There is not more sense in using varchar(n) -- no performance benefits, no problems with limitation after. So there might me more reason to propose ::text typed array as a reasonably safe default.
  3. There still need to be a compatibility for both cases.

what if we will add additional param as a requirement:

class Product < ApplicationRecord
  acts_as_taggable_array_on :categories, as: :<column_type>
end

And make sure that it's included? (raise an error, if it's not) @tmiyamon what do you think?

tmiyamon commented 6 years ago

Thank you for your suggestion. I agree not to stick to specific type in this gem. It's good to add required type option but I think auto detection is more useful such as using type info of columns_hash['tag'].type.

skatkov commented 6 years ago

@tmiyamon thanks for a tip. I managed to do a quick fix that should address this. I'd appreciate if you can review it

skatkov commented 6 years ago

Sorry, to bother you. Are there any issues @tmiyamon ?

tmiyamon commented 6 years ago

Great work! Thank you for your contribution!