Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update linkset homepage from gemspec only after version gets indexed #5249

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/jobs/after_version_write_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
14 changes: 14 additions & 0 deletions app/jobs/set_linkset_home_job.rb
Original file line number Diff line number Diff line change
@@ -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}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only job that redownloads the gem besides the content storage job. Could it just grab this info on push?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now we don't pass anything besides the version itself to the "after write" set of jobs, so I kept in keeping with that

package = Gem::Package.new(StringIO.new(gem))
homepage = package.spec.homepage

version.rubygem.linkset ||= Linkset.new
version.rubygem.linkset.update!(home: homepage)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinemde this is where we update the linkset

end
end
6 changes: 1 addition & 5 deletions app/models/linkset.rb
Original file line number Diff line number Diff line change
@@ -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?

Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion app/models/pusher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
29 changes: 3 additions & 26 deletions app/models/rubygem.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we never update the linkset now. Is that expected?

version.update_attributes_from_gem_specification!(spec)
version.update_dependencies!(spec)
end
end

Expand Down
14 changes: 13 additions & 1 deletion app/models/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -296,7 +308,7 @@ def <=>(other)
end

def slug
full_name.remove(/^#{rubygem.name}-/)
full_name.delete_prefix("#{rubygem.name}-")
end

def downloads_count
Expand Down
2 changes: 2 additions & 0 deletions test/integration/push_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,15 @@ class PushTest < ActionDispatch::IntegrationTest
push_gem "sandworm-1.0.0.gem"

assert_response :success
perform_enqueued_jobs

get rubygem_path("sandworm")

assert_response :success
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}])

Expand Down
2 changes: 1 addition & 1 deletion test/models/rubygem_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading