diff --git a/Gemfile b/Gemfile index 0573e98a75..f7fd884b60 100644 --- a/Gemfile +++ b/Gemfile @@ -41,7 +41,7 @@ gem 'kaminari' gem 'oj', '~> 3.14.0' # Setup dependencies -gem 'exercism-config', '>= 0.136.0' +gem 'exercism-config', '>= 0.137.0' # gem 'exercism-config', path: '../config' # Model-level dependencies diff --git a/Gemfile.lock b/Gemfile.lock index 907791f402..43a425da30 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -195,7 +195,7 @@ GEM et-orbi (1.2.11) tzinfo event_stream_parser (1.0.0) - exercism-config (0.136.0) + exercism-config (0.137.0) aws-sdk-dynamodb (~> 1.0) aws-sdk-secretsmanager (~> 1.0) mandate @@ -684,7 +684,7 @@ DEPENDENCIES devise (~> 4.7) discourse_api doorkeeper (~> 5.8) - exercism-config (>= 0.136.0) + exercism-config (>= 0.137.0) factory_bot_rails friendly_id (~> 5.4.0) geocoder (~> 1.8) diff --git a/app/commands/icons/check_exists.rb b/app/commands/icons/check_exists.rb new file mode 100644 index 0000000000..6c7fea980a --- /dev/null +++ b/app/commands/icons/check_exists.rb @@ -0,0 +1,18 @@ +# Checks whether an icon actually exists in the icons bucket. +class Icons::CheckExists + include Mandate + + initialize_with :path + + def call + # An empty manifest means we couldn't retrieve it, so assume the icon exists + # and leave the browser's onerror handler to fall back if it doesn't. + return true if manifest.empty? + + manifest.include?(path) + end + + private + memoize + def manifest = Icons::RetrieveManifest.() +end diff --git a/app/commands/icons/determine_url_for.rb b/app/commands/icons/determine_url_for.rb new file mode 100644 index 0000000000..4b49f652ee --- /dev/null +++ b/app/commands/icons/determine_url_for.rb @@ -0,0 +1,27 @@ +# Builds the URL for an icon hosted in the icons bucket. +# +# Lots of exercises and tracks have never had an icon drawn for them. Without a +# check here we'd emit a URL for a file that doesn't exist, the browser would +# request it and get a 403, and only then would our onerror handlers swap in the +# fallback. S3 errors aren't cacheable at the CDN, so every page view hits the +# origin again: those missing icons were generating millions of requests a week. +# +# So we point straight at the local fallback asset when there isn't an icon. +class Icons::DetermineUrlFor + include Mandate + include Propshaft::Helper + + MISSING_EXERCISE_ICON = "graphics/missing-exercise.svg".freeze + MISSING_TRACK_ICON = "graphics/missing-track.svg".freeze + + initialize_with :path, :fallback + + def call + return fallback_url unless Icons::CheckExists.(path) + + "#{Exercism.config.website_icons_host}/#{path}" + end + + private + def fallback_url = "#{Rails.application.config.action_controller.asset_host}#{compute_asset_path(fallback)}" +end diff --git a/app/commands/icons/retrieve_manifest.rb b/app/commands/icons/retrieve_manifest.rb new file mode 100644 index 0000000000..abb53a4744 --- /dev/null +++ b/app/commands/icons/retrieve_manifest.rb @@ -0,0 +1,48 @@ +# Retrieves the set of icon paths that actually exist in the icons bucket. +# +# The manifest is published to the bucket by the sync workflow in the +# exercism/website-icons repo, so it updates whenever icons are added or +# removed, without needing anything deployed here. +# +# This fails open: if we can't retrieve the manifest we return an empty set, +# which callers treat as "assume everything exists". That keeps us on the old +# behaviour (request the icon, let the browser's onerror handler fall back) +# rather than rendering the fallback for every icon on the site. +class Icons::RetrieveManifest + include Mandate + + def call + cached = Rails.cache.read(CACHE_KEY) + return cached if cached + + # Don't hold onto a failure for the full expiry. A blip would otherwise + # leave us checking nothing for an hour. + Rails.cache.write(CACHE_KEY, paths, expires_in: paths.empty? ? FAILURE_CACHE_EXPIRY : CACHE_EXPIRY) + paths + end + + private + memoize + def paths + parsed = JSON.parse(manifest) + raise TypeError, "Icons manifest is not an array" unless parsed.is_a?(Array) + + parsed.to_set + rescue StandardError => e + Sentry.capture_exception(e) + Set.new + end + + def manifest + Exercism.s3_client.get_object( + bucket: Exercism.config.aws_icons_bucket, + key: MANIFEST_KEY + ).body.read + end + + CACHE_KEY = "Icons::RetrieveManifest".freeze + MANIFEST_KEY = "manifest.json".freeze + CACHE_EXPIRY = 1.hour.freeze + FAILURE_CACHE_EXPIRY = 1.minute.freeze + private_constant :CACHE_KEY, :MANIFEST_KEY, :CACHE_EXPIRY, :FAILURE_CACHE_EXPIRY +end diff --git a/app/models/exercise.rb b/app/models/exercise.rb index 996989fb49..e599809fd5 100644 --- a/app/models/exercise.rb +++ b/app/models/exercise.rb @@ -167,7 +167,7 @@ def difficulty_category end end - def icon_url = "#{Exercism.config.website_icons_host}/exercises/#{icon_name}.svg" + def icon_url = Icons::DetermineUrlFor.("exercises/#{icon_name}.svg", Icons::DetermineUrlFor::MISSING_EXERCISE_ICON) memoize def mentoring_notes diff --git a/app/models/generic_exercise.rb b/app/models/generic_exercise.rb index 5131395f37..41729ba495 100644 --- a/app/models/generic_exercise.rb +++ b/app/models/generic_exercise.rb @@ -11,7 +11,7 @@ def status = super.to_sym def git = Git::ProblemSpecifications::Exercise.new(slug) def url = "https://github.com/exercism/problem-specifications/tree/main/exercises/#{slug}" - def icon_url = "#{Exercism.config.website_icons_host}/exercises/#{slug}.svg" + def icon_url = Icons::DetermineUrlFor.("exercises/#{slug}.svg", Icons::DetermineUrlFor::MISSING_EXERCISE_ICON) def self.for!(slug) = find_by!(slug:) diff --git a/app/models/track.rb b/app/models/track.rb index 728a707f83..7290377cc9 100644 --- a/app/models/track.rb +++ b/app/models/track.rb @@ -118,7 +118,7 @@ def trophies Track::Trophy.for_track(self) end - def icon_url = "#{Exercism.config.website_icons_host}/tracks/#{slug}.svg" + def icon_url = Icons::DetermineUrlFor.("tracks/#{slug}.svg", Icons::DetermineUrlFor::MISSING_TRACK_ICON) def highlightjs_language super || slug diff --git a/test/commands/icons/check_exists_test.rb b/test/commands/icons/check_exists_test.rb new file mode 100644 index 0000000000..37e6f86acc --- /dev/null +++ b/test/commands/icons/check_exists_test.rb @@ -0,0 +1,21 @@ +require 'test_helper' + +class Icons::CheckExistsTest < ActiveSupport::TestCase + test "true when the icon is in the manifest" do + setup_s3_icons_manifest!(["exercises/bob.svg"]) + + assert Icons::CheckExists.("exercises/bob.svg") + end + + test "false when the icon isn't in the manifest" do + setup_s3_icons_manifest!(["exercises/bob.svg"]) + + refute Icons::CheckExists.("exercises/flower-field.svg") + end + + test "true for everything when the manifest is empty" do + setup_s3_icons_manifest!([]) + + assert Icons::CheckExists.("exercises/flower-field.svg") + end +end diff --git a/test/commands/icons/determine_url_for_test.rb b/test/commands/icons/determine_url_for_test.rb new file mode 100644 index 0000000000..c3a685d866 --- /dev/null +++ b/test/commands/icons/determine_url_for_test.rb @@ -0,0 +1,33 @@ +require 'test_helper' + +class Icons::DetermineUrlForTest < ActiveSupport::TestCase + test "returns the bucket url when the icon exists" do + setup_s3_icons_manifest!(["exercises/bob.svg"]) + + assert_equal "https://assets.exercism.org/exercises/bob.svg", + Icons::DetermineUrlFor.("exercises/bob.svg", Icons::DetermineUrlFor::MISSING_EXERCISE_ICON) + end + + test "returns the fallback when the icon doesn't exist" do + setup_s3_icons_manifest!(["exercises/bob.svg"]) + + url = Icons::DetermineUrlFor.("exercises/flower-field.svg", Icons::DetermineUrlFor::MISSING_EXERCISE_ICON) + assert_includes url, "missing-exercise" + refute_includes url, "flower-field" + end + + test "assumes the icon exists when the manifest is unavailable" do + assert_equal "https://assets.exercism.org/exercises/flower-field.svg", + Icons::DetermineUrlFor.("exercises/flower-field.svg", Icons::DetermineUrlFor::MISSING_EXERCISE_ICON) + end + + test "exercise and track icon urls use the manifest" do + setup_s3_icons_manifest!(["tracks/ruby.svg"]) + + track = create :track, slug: :ruby + exercise = create :practice_exercise, track:, slug: :flower_field, icon_name: 'flower-field' + + assert_equal "https://assets.exercism.org/tracks/ruby.svg", track.icon_url + assert_includes exercise.icon_url, "missing-exercise" + end +end diff --git a/test/commands/icons/retrieve_manifest_test.rb b/test/commands/icons/retrieve_manifest_test.rb new file mode 100644 index 0000000000..3edf5c9ea0 --- /dev/null +++ b/test/commands/icons/retrieve_manifest_test.rb @@ -0,0 +1,32 @@ +require 'test_helper' + +class Icons::RetrieveManifestTest < ActiveSupport::TestCase + test "retrieves and caches the manifest" do + setup_s3_icons_manifest!(["exercises/bob.svg", "tracks/ruby.svg"]) + + assert_equal Set["exercises/bob.svg", "tracks/ruby.svg"], Icons::RetrieveManifest.() + + # The second call should come from the cache, not the bucket + setup_s3_icons_manifest!(["exercises/leap.svg"]) + assert_equal Set["exercises/bob.svg", "tracks/ruby.svg"], Icons::RetrieveManifest.() + end + + test "returns an empty set when there's no manifest" do + assert_empty Icons::RetrieveManifest.() + end + + test "returns an empty set when the manifest isn't an array" do + setup_s3_icons_manifest!({ exercises: [] }) + + assert_empty Icons::RetrieveManifest.() + end + + test "retries sooner after a failure" do + assert_empty Icons::RetrieveManifest.() + + travel 2.minutes do + setup_s3_icons_manifest!(["exercises/bob.svg"]) + assert_equal Set["exercises/bob.svg"], Icons::RetrieveManifest.() + end + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 3952333868..f981e0139f 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -187,6 +187,7 @@ class ActiveSupport::TestCase teardown do reset_opensearch! + remove_s3_icons_manifest! Bullet.perform_out_of_channel_notifications if Bullet.notification? Bullet.end_request @@ -295,6 +296,33 @@ def create_tooling_job!(submission, type, params = {}) ) end + # The icons bucket and its manifest live in production. Tests that care about + # icon urls create the bucket in LocalStack and upload their own manifest. + # Everything else gets a NoSuchBucket, which the command treats as "assume + # every icon exists" and so leaves urls untouched. + def setup_s3_icons_manifest!(paths) + @__uploaded_s3_icons_manifest__ = true + + begin + Exercism.s3_client.create_bucket(bucket: Exercism.config.aws_icons_bucket) + rescue Aws::S3::Errors::BucketAlreadyOwnedByYou, Aws::S3::Errors::BucketAlreadyExists + # Already there from a previous test + end + + upload_to_s3(Exercism.config.aws_icons_bucket, ICONS_MANIFEST_KEY, paths.to_json) + end + + # S3 isn't rolled back between tests the way the database is, so a manifest + # left behind would change icon urls in every test that ran after it. + def remove_s3_icons_manifest! + return unless @__uploaded_s3_icons_manifest__ + + Exercism.s3_client.delete_object(bucket: Exercism.config.aws_icons_bucket, key: ICONS_MANIFEST_KEY) + @__uploaded_s3_icons_manifest__ = false + end + + ICONS_MANIFEST_KEY = "manifest.json".freeze + # The cache bucket is created by terraform in production. Locally the # config key doesn't exist, so tests set it and create the bucket in # LocalStack on demand.