From 19522774df940bfd7bcb82b742f2827301592d04 Mon Sep 17 00:00:00 2001 From: Samuel Giddins Date: Mon, 18 Nov 2024 13:10:07 -0800 Subject: [PATCH] Update linkset homepage from gemspec only after version gets indexed In current implementation, version.reload.indexed? is always false, so gems that only use spec.homepage to specify a homepage will never have a linkset created/updated Signed-off-by: Samuel Giddins --- app/jobs/after_version_write_job.rb | 1 + app/jobs/set_linkset_home_job.rb | 14 ++++++++++++++ app/models/linkset.rb | 6 +----- app/models/pusher.rb | 3 ++- app/models/rubygem.rb | 29 +++-------------------------- app/models/version.rb | 14 +++++++++++++- test/integration/push_test.rb | 2 ++ test/models/rubygem_test.rb | 2 +- 8 files changed, 37 insertions(+), 34 deletions(-) create mode 100644 app/jobs/set_linkset_home_job.rb diff --git a/app/jobs/after_version_write_job.rb b/app/jobs/after_version_write_job.rb index 651af4f5bdf..801168cac86 100644 --- a/app/jobs/after_version_write_job.rb +++ b/app/jobs/after_version_write_job.rb @@ -16,6 +16,7 @@ def perform(version:) version.update!(indexed: true) checksum = GemInfo.new(rubygem.name, cached: false).info_checksum version.update_attribute :info_checksum, checksum + SetLinksetHomeJob.perform_later(version:) end end diff --git a/app/jobs/set_linkset_home_job.rb b/app/jobs/set_linkset_home_job.rb new file mode 100644 index 00000000000..0c60dff0acd --- /dev/null +++ b/app/jobs/set_linkset_home_job.rb @@ -0,0 +1,14 @@ +class SetLinksetHomeJob < ApplicationJob + queue_as :default + + def perform(version:) + return unless version.latest? && version.indexed? + + gem = RubygemFs.instance.get("gems/#{version.gem_file_name}") + package = Gem::Package.new(StringIO.new(gem)) + homepage = package.spec.homepage + + version.rubygem.linkset ||= Linkset.new + version.rubygem.linkset.update!(home: homepage) + end +end diff --git a/app/models/linkset.rb b/app/models/linkset.rb index ccea8016728..559293ae1f7 100644 --- a/app/models/linkset.rb +++ b/app/models/linkset.rb @@ -1,5 +1,5 @@ class Linkset < ApplicationRecord - belongs_to :rubygem + belongs_to :rubygem, inverse_of: :linkset before_save :create_homepage_link_verification, if: :home_changed? @@ -19,10 +19,6 @@ def empty? LINKS.map { |link| attributes[link] }.all?(&:blank?) end - def update_attributes_from_gem_specification!(spec) - update!(home: spec.homepage) - end - def create_homepage_link_verification return if home.blank? rubygem.link_verifications.find_or_create_by!(uri: home).retry_if_needed diff --git a/app/models/pusher.rb b/app/models/pusher.rb index 0f0dab75908..d491e3e7726 100644 --- a/app/models/pusher.rb +++ b/app/models/pusher.rb @@ -246,7 +246,8 @@ def update end true - rescue ActiveRecord::RecordInvalid, ActiveRecord::Rollback, ActiveRecord::RecordNotUnique + rescue ActiveRecord::RecordInvalid, ActiveRecord::Rollback, ActiveRecord::RecordNotUnique => e + logger.info { { message: "Error updating rubygem", exception: e } } false end diff --git a/app/models/rubygem.rb b/app/models/rubygem.rb index 59529105575..2f9f061655e 100644 --- a/app/models/rubygem.rb +++ b/app/models/rubygem.rb @@ -14,7 +14,7 @@ class Rubygem < ApplicationRecord has_many :versions, dependent: :destroy, validate: false has_one :latest_version, -> { latest.order(:position) }, class_name: "Version", inverse_of: :rubygem has_many :web_hooks, dependent: :destroy - has_one :linkset, dependent: :destroy + has_one :linkset, dependent: :destroy, inverse_of: :rubygem has_one :gem_download, -> { where(version_id: 0) }, inverse_of: :rubygem has_many :ownership_calls, -> { opened }, dependent: :destroy, inverse_of: :rubygem has_many :ownership_requests, -> { opened }, dependent: :destroy, inverse_of: :rubygem @@ -273,34 +273,11 @@ def ownership_call ownership_calls.find_by(status: "opened") end - def update_versions!(version, spec) - version.update_attributes_from_gem_specification!(spec) - end - - def update_dependencies!(version, spec) - spec.dependencies.each do |dependency| - version.dependencies.create!(gem_dependency: dependency) - rescue ActiveRecord::RecordInvalid => e - # ActiveRecord can't chain a nested error here, so we have to add and reraise - e.record.errors.errors.each do |error| - errors.import(error, attribute: "dependency.#{error.attribute}") - end - raise - end - end - - def update_linkset!(spec) - self.linkset ||= Linkset.new - self.linkset.update_attributes_from_gem_specification!(spec) - self.linkset.save! - end - def update_attributes_from_gem_specification!(version, spec) Rubygem.transaction do save! - update_versions! version, spec - update_dependencies! version, spec - update_linkset! spec if version.reload.latest? + version.update_attributes_from_gem_specification!(spec) + version.update_dependencies!(spec) end end diff --git a/app/models/version.rb b/app/models/version.rb index f3aa3e75413..9dc57a83c55 100644 --- a/app/models/version.rb +++ b/app/models/version.rb @@ -276,6 +276,18 @@ def update_attributes_from_gem_specification!(spec) ) end + def update_dependencies!(spec) + spec.dependencies.each do |dependency| + dependencies.create!(gem_dependency: dependency) + rescue ActiveRecord::RecordInvalid => e + # ActiveRecord can't chain a nested error here, so we have to add and reraise + e.record.errors.errors.each do |error| + errors.import(error, attribute: "dependency.#{error.attribute}") + end + raise + end + end + def platform_as_number if platformed? 0 @@ -296,7 +308,7 @@ def <=>(other) end def slug - full_name.remove(/^#{rubygem.name}-/) + full_name.delete_prefix("#{rubygem.name}-") end def downloads_count diff --git a/test/integration/push_test.rb b/test/integration/push_test.rb index ae4471a3852..595c34b4f4a 100644 --- a/test/integration/push_test.rb +++ b/test/integration/push_test.rb @@ -76,6 +76,7 @@ class PushTest < ActionDispatch::IntegrationTest push_gem "sandworm-1.0.0.gem" assert_response :success + perform_enqueued_jobs get rubygem_path("sandworm") @@ -83,6 +84,7 @@ class PushTest < ActionDispatch::IntegrationTest assert page.has_content?("sandworm") assert page.has_content?("1.0.0") assert page.has_content?("Pushed by") + assert page.has_link? "Homepage", href: "http://example.com/sandworm" css = %(div.gem__users a[alt=#{@user.handle}]) diff --git a/test/models/rubygem_test.rb b/test/models/rubygem_test.rb index 7b23a1573de..0cb70fec45b 100644 --- a/test/models/rubygem_test.rb +++ b/test/models/rubygem_test.rb @@ -376,7 +376,7 @@ class RubygemTest < ActiveSupport::TestCase @specification.authors = [3] assert_raise ActiveRecord::RecordInvalid do - @rubygem.update_versions!(@version, @specification) + @version.update_attributes_from_gem_specification!(@specification) end assert_equal "Authors must be an Array of Strings", @rubygem.all_errors(@version)