Skip to content

Commit

Permalink
Look at database consistency issues (#1189)
Browse files Browse the repository at this point in the history
* Lets push these changes out!

* Only one annoucment can be live at a time, so add a index.

* lint
  • Loading branch information
epugh authored Jan 19, 2025
1 parent cdabb82 commit 357b4fa
Show file tree
Hide file tree
Showing 13 changed files with 95 additions and 17 deletions.
31 changes: 22 additions & 9 deletions .database_consistency.todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ Announcement:
enabled: false
live:
MissingUniqueIndexChecker:
enabled: false
enabled: true
ThreeStateBooleanChecker:
enabled: false
text:
Expand Down Expand Up @@ -114,6 +114,9 @@ Case:
scorer:
ForeignKeyChecker:
enabled: false
user_id:
RedundantIndexChecker:
enabled: false
CaseMetadatum:
id:
PrimaryKeyTypeChecker:
Expand Down Expand Up @@ -237,6 +240,9 @@ Score:
user:
ColumnPresenceChecker:
enabled: false
index_case_scores_annotation_id:
UniqueIndexChecker:
enabled: false
Scorer:
code:
LengthConstraintChecker:
Expand Down Expand Up @@ -395,12 +401,12 @@ User:
enabled: false
company:
LengthConstraintChecker:
enabled: false
enabled: true
email:
ColumnPresenceChecker:
enabled: false
LengthConstraintChecker:
enabled: false
enabled: true
id:
PrimaryKeyTypeChecker:
enabled: false
Expand All @@ -412,33 +418,40 @@ User:
enabled: false
invitation_token:
LengthConstraintChecker:
enabled: false
enabled: true
invited_by:
ForeignKeyChecker:
enabled: false
enabled: true
locked:
ThreeStateBooleanChecker:
enabled: false
name:
LengthConstraintChecker:
enabled: false
enabled: true
ColumnPresenceChecker:
enabled: false
owned_scorers:
MissingIndexChecker:
enabled: true
password:
ColumnPresenceChecker:
enabled: false
LengthConstraintChecker:
enabled: false
enabled: true
permissions:
MissingIndexChecker:
enabled: true
profile_pic:
LengthConstraintChecker:
enabled: false
enabled: true
reset_password_token:
LengthConstraintChecker:
enabled: false
enabled: true
stored_raw_invitation_token:
LengthConstraintChecker:
enabled: true

WebRequest:
index_web_requests_on_snapshot_query_id:
UniqueIndexChecker:
enabled: false
1 change: 1 addition & 0 deletions app/models/announcement.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# Indexes
#
# index_announcements_author_id (author_id)
# index_announcements_live (live) UNIQUE
#
class Announcement < ApplicationRecord
belongs_to :author, class_name: 'User'
Expand Down
28 changes: 21 additions & 7 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,15 @@
# Foreign Keys
#
# fk_rails_... (default_scorer_id => scorers.id)
# fk_rails_... (invited_by_id => users.id)
#

class User < ApplicationRecord
# Associations

has_many :api_keys, dependent: :destroy

belongs_to :default_scorer, class_name: 'Scorer', optional: true # for communal scorers there isn't a owner

# has_many :cases,
# dependent: :nullify # sometimes a case belongs to a team, so don't just delete it.
has_many :cases,
class_name: 'Case',
foreign_key: :owner_id,
Expand Down Expand Up @@ -119,29 +117,45 @@ class User < ApplicationRecord

has_many :announcements, foreign_key: 'author_id', dependent: :destroy, inverse_of: :author

# has_many :ai_judges, dependent: :destroy
# has_many :books, through: :ai_judges

# Validations
validates :name,
length: { maximum: 255 }

# https://davidcel.is/posts/stop-validating-email-addresses-with-regex/
validates :email,
presence: true,
uniqueness: true,
format: { with: URI::MailTo::EMAIL_REGEXP },
length: { maximum: 80 },
unless: :ai_judge?

validates :password,
presence: true
presence: true,
length: { maximum: 80 }

validates :password, confirmation: { message: 'should match confirmation' }

validates :reset_password_token,
length: { maximum: 255 }
validates :invitation_token,
length: { maximum: 255 }
validates :stored_raw_invitation_token,
length: { maximum: 255 }

validates :company,
length: { maximum: 255 }
validates :profile_pic,
length: { maximum: 4000 }

validates_with ::DefaultScorerExistsValidator

validates :agreed,
acceptance: { message: 'checkbox must be clicked to signify you agree to the terms and conditions.' },
if: :terms_and_conditions?

validates :openai_key, length: { maximum: 255 }, allow_nil: true
validates :system_prompt, length: { maximum: 4000 }, allow_nil: true

def terms_and_conditions?
Rails.application.config.terms_and_conditions_url.present?
end
Expand Down
8 changes: 8 additions & 0 deletions app/models/web_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@
# updated_at :datetime not null
# snapshot_query_id :integer
#
# Indexes
#
# index_web_requests_on_snapshot_query_id (snapshot_query_id) UNIQUE
#
# Foreign Keys
#
# fk_rails_... (snapshot_query_id => snapshot_queries.id)
#
class WebRequest < ApplicationRecord
belongs_to :snapshot_query, optional: true
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Loaded configurations: .database_consistency.yml, .database_consistency.todo.yml
# UniqueIndexChecker fail Score index_case_scores_annotation_id index is unique in the database but do not have uniqueness validator
# LengthConstraintChecker fail User system_prompt column has limit in the database but do not have length validator
# LengthConstraintChecker fail User openai_key column has limit in the database but do not have length validator
# ForeignKeyChecker fail WebRequest snapshot_query should have foreign key in the database
# MissingIndexChecker fail SnapshotQuery web_request associated model should have proper unique index in the database

class AddUniqueIndexToWebRequestsSnapshotQueryId < ActiveRecord::Migration[8.0]
def change
remove_index :web_requests, :snapshot_query_id if index_exists?(:web_requests, :snapshot_query_id)
add_index :web_requests, :snapshot_query_id, unique: true
add_foreign_key :web_requests, :snapshot_queries
end
end
10 changes: 10 additions & 0 deletions db/migrate/20250118224749_refine_structure.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# RedundantIndexChecker fail Case user_id index is redundant as idx_owner_archived covers it

class RefineStructure < ActiveRecord::Migration[8.0]
def change
if index_exists?(:cases, [:user_id, :archived], name: 'idx_owner_archived')
remove_index :cases, :user_id if index_exists?(:cases, :user_id)
end

end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddUsersIdUsersInvitedByIdFk < ActiveRecord::Migration[8.0]
def change
add_foreign_key :users, :users, column: :invited_by_id, primary_key: :id
end
end
5 changes: 5 additions & 0 deletions db/migrate/20250119130032_add_announcements_live_index.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddAnnouncementsLiveIndex < ActiveRecord::Migration[8.0]
def change
add_index :announcements, :live, name: :index_announcements_live, unique: true
end
end
6 changes: 5 additions & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/fixtures/announcements.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# Indexes
#
# index_announcements_author_id (author_id)
# index_announcements_live (live) UNIQUE
#

live_announcement:
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/users.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
# Foreign Keys
#
# fk_rails_... (default_scorer_id => scorers.id)
# fk_rails_... (invited_by_id => users.id)
#

default: &default
Expand Down
1 change: 1 addition & 0 deletions test/models/announcement_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# Indexes
#
# index_announcements_author_id (author_id)
# index_announcements_live (live) UNIQUE
#
require 'test_helper'

Expand Down
1 change: 1 addition & 0 deletions test/models/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
# Foreign Keys
#
# fk_rails_... (default_scorer_id => scorers.id)
# fk_rails_... (invited_by_id => users.id)
#

# rubocop:disable Layout/LineLength
Expand Down

0 comments on commit 357b4fa

Please sign in to comment.