summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorYorick Peterse <yorickpeterse@gmail.com>2017-08-09 16:41:51 +0200
committerYorick Peterse <yorickpeterse@gmail.com>2017-08-10 12:45:49 +0200
commit26bb50412ce44be434d7bb86a952397b7983deb5 (patch)
treeae68cd680fc814930299c2c68f414bc2edb70757
parent53ee38053cb924b57447e385dee2a45b8e759ff5 (diff)
downloadgitlab-ce-26bb50412ce44be434d7bb86a952397b7983deb5.tar.gz
Cache Appearance instances in Redisappearances-caching-and-schema
This caches the result of Appearance.first in a similar fashion to how ApplicationSetting instances are cached. We also add some NOT NULL constraints to the table and correct the timestamp types. Fixes gitlab-org/gitlab-ce#36066, fixes gitlab-org/gitlab-ce#31698
-rw-r--r--app/controllers/admin/appearances_controller.rb2
-rw-r--r--app/helpers/appearances_helper.rb2
-rw-r--r--app/models/appearance.rb20
-rw-r--r--changelogs/unreleased/appearances-caching-and-schema.yml4
-rw-r--r--db/migrate/20170809142252_cleanup_appearances_schema.rb33
-rw-r--r--db/schema.rb8
-rw-r--r--spec/models/appearance_spec.rb35
7 files changed, 98 insertions, 6 deletions
diff --git a/app/controllers/admin/appearances_controller.rb b/app/controllers/admin/appearances_controller.rb
index 4b0ec54b3f4..92df1c8dff0 100644
--- a/app/controllers/admin/appearances_controller.rb
+++ b/app/controllers/admin/appearances_controller.rb
@@ -45,7 +45,7 @@ class Admin::AppearancesController < Admin::ApplicationController
# Use callbacks to share common setup or constraints between actions.
def set_appearance
- @appearance = Appearance.last || Appearance.new
+ @appearance = Appearance.current || Appearance.new
end
# Only allow a trusted parameter "white list" through.
diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb
index 16136d02530..cdf5fa5d4b7 100644
--- a/app/helpers/appearances_helper.rb
+++ b/app/helpers/appearances_helper.rb
@@ -20,7 +20,7 @@ module AppearancesHelper
end
def brand_item
- @appearance ||= Appearance.first
+ @appearance ||= Appearance.current
end
def brand_header_logo
diff --git a/app/models/appearance.rb b/app/models/appearance.rb
index f9c48482be7..ff15689ecac 100644
--- a/app/models/appearance.rb
+++ b/app/models/appearance.rb
@@ -8,7 +8,27 @@ class Appearance < ActiveRecord::Base
validates :logo, file_size: { maximum: 1.megabyte }
validates :header_logo, file_size: { maximum: 1.megabyte }
+ validate :single_appearance_row, on: :create
+
mount_uploader :logo, AttachmentUploader
mount_uploader :header_logo, AttachmentUploader
has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+
+ CACHE_KEY = 'current_appearance'.freeze
+
+ after_commit :flush_redis_cache
+
+ def self.current
+ Rails.cache.fetch(CACHE_KEY) { first }
+ end
+
+ def flush_redis_cache
+ Rails.cache.delete(CACHE_KEY)
+ end
+
+ def single_appearance_row
+ if self.class.any?
+ errors.add(:single_appearance_row, 'Only 1 appearances row can exist')
+ end
+ end
end
diff --git a/changelogs/unreleased/appearances-caching-and-schema.yml b/changelogs/unreleased/appearances-caching-and-schema.yml
new file mode 100644
index 00000000000..5743f6e0f2d
--- /dev/null
+++ b/changelogs/unreleased/appearances-caching-and-schema.yml
@@ -0,0 +1,4 @@
+---
+title: Cache Appearance instances in Redis
+merge_request:
+author:
diff --git a/db/migrate/20170809142252_cleanup_appearances_schema.rb b/db/migrate/20170809142252_cleanup_appearances_schema.rb
new file mode 100644
index 00000000000..90d12925ba2
--- /dev/null
+++ b/db/migrate/20170809142252_cleanup_appearances_schema.rb
@@ -0,0 +1,33 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class CleanupAppearancesSchema < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ # Set this constant to true if this migration requires downtime.
+ DOWNTIME = false
+
+ NOT_NULL_COLUMNS = %i[title description description_html created_at updated_at]
+
+ TIME_COLUMNS = %i[created_at updated_at]
+
+ def up
+ NOT_NULL_COLUMNS.each do |column|
+ change_column_null :appearances, column, false
+ end
+
+ TIME_COLUMNS.each do |column|
+ change_column :appearances, column, :datetime_with_timezone
+ end
+ end
+
+ def down
+ NOT_NULL_COLUMNS.each do |column|
+ change_column_null :appearances, column, true
+ end
+
+ TIME_COLUMNS.each do |column|
+ change_column :appearances, column, :datetime # rubocop: disable Migration/Datetime
+ end
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index ed3cf70bcdd..ec92b185be2 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20170807160457) do
+ActiveRecord::Schema.define(version: 20170809142252) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@@ -28,13 +28,13 @@ ActiveRecord::Schema.define(version: 20170807160457) do
end
create_table "appearances", force: :cascade do |t|
- t.string "title"
- t.text "description"
+ t.string "title", null: false
+ t.text "description", null: false
t.string "header_logo"
t.string "logo"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
- t.text "description_html"
+ t.text "description_html", null: false
t.integer "cached_markdown_version"
end
diff --git a/spec/models/appearance_spec.rb b/spec/models/appearance_spec.rb
index 7cd3a84d592..b5d5d58697b 100644
--- a/spec/models/appearance_spec.rb
+++ b/spec/models/appearance_spec.rb
@@ -9,4 +9,39 @@ RSpec.describe Appearance do
it { is_expected.to validate_presence_of(:description) }
it { is_expected.to have_many(:uploads).dependent(:destroy) }
+
+ describe '.current', :use_clean_rails_memory_store_caching do
+ let!(:appearance) { create(:appearance) }
+
+ it 'returns the current appearance row' do
+ expect(described_class.current).to eq(appearance)
+ end
+
+ it 'caches the result' do
+ expect(described_class).to receive(:first).once
+
+ 2.times { described_class.current }
+ end
+ end
+
+ describe '#flush_redis_cache' do
+ it 'flushes the cache in Redis' do
+ appearance = create(:appearance)
+
+ expect(Rails.cache).to receive(:delete).with(described_class::CACHE_KEY)
+
+ appearance.flush_redis_cache
+ end
+ end
+
+ describe '#single_appearance_row' do
+ it 'adds an error when more than 1 row exists' do
+ create(:appearance)
+
+ new_row = build(:appearance)
+ new_row.save
+
+ expect(new_row.valid?).to eq(false)
+ end
+ end
end