Missing NOT NULL and uniqueness constraints in DB

[Utsav]

Hi,

Wanted to make a report about a few validations/constraints that exist in models but are missing in DB.

Missing not null requirements

PostDetail - key, value are required to be present in model but are nullable in DB.

1. Required in model (app/models/post_detail.rb):

   validates_presence_of   :key, :value
   

   …But nullable in DB

   discourse_development=# \d post_details
                                           Table "public.post_details"
      Column   |            Type             | Collation | Nullable |                 Default
   ------------+-----------------------------+-----------+----------+------------------------------------------
    ...
    key        | character varying           |           |          |
    value      | character varying           |           |          |
   

   If we add a NOT NULL constraint to the DB, then it’s still possible an empty string gets through, but I think having not null will at least be a little safer

Missing unique constraints

1. TagGroup - name. Unique in model (app/models/tag_group.rb)

   validates_uniqueness_of :name, case_sensitive: false
   

   But unique index missing from DB

   discourse_development=# \d tag_groups
   ...
   Indexes:
       "tag_groups_pkey" PRIMARY KEY, btree (id)
   

2. Likewise, WatchedWord - word. (app/models/watched_word.rb)

   validates :word,   presence: true, uniqueness: true, length: { maximum: 50 }
   

   DB has a unique index on (action, word) but not on (word) alone:

   discourse_development=# \d watched_words
   ...
   Indexes:
       "watched_words_pkey" PRIMARY KEY, btree (id)
       "index_watched_words_on_action_and_word" UNIQUE, btree (action, word)
   

3. Finally, WebHookEventType - name. (app/models/web_hook_event_type.rb):

   validates :name, presence: true, uniqueness: true
   

   Missing unique constraint in DB:

   discourse_development=# \d web_hook_event_types
   ...
   Indexes:
       "web_hook_event_types_pkey" PRIMARY KEY, btree (id)
   

Perhaps someone might be able to confirm if these seem reasonable. It’s possible they will help prevent a bug before it happens. Also, if so, the fix seems like it would be pretty straightforward, and I’m happy to create a pull request if that will be helpful.

Thanks.

[sam (Sam Saffron)]

@Utsav

validates_uniqueness_of :name, case_sensitive: false

Case insensitive unique constraints are a bit tricky, I am uneasy adding the citext extension for this and we would be forced then to go name_lower and dupe the column just to add the index.

@Utsav

validates :word, presence: true, uniqueness: true, length: { maximum: 50 }

Yeah we should fix this

@Utsav

Finally, WebHookEventType - name . ( app/models/web_hook_event_type.rb ):

Also fine to add an index here.