Missing NOT NULL and uniqueness constraints in DB

Dev
1

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.

2 Likes
2

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.

Yeah we should fix this

Also fine to add an index here.

1 Like