Skip to content

Refactor ReindexByDoiJob to include Shoryuken::Worker and update perform method #1495

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.

Sign up for GitHub

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

Merged
codycooperross merged 3 commits into from
Mar 12, 2026
+13 −4
Merged

Refactor ReindexByDoiJob to include Shoryuken::Worker and update perform method #1495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99f77b6
Refactor ReindexByDoiJob to include Shoryuken::Worker and update perf…
codycooperross Mar 11, 2026
d1205e4
Update tests
codycooperross Mar 11, 2026
49a46ed
Use hash structure for message data
codycooperross Mar 11, 2026
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
11 changes: 10 additions & 1 deletion app/jobs/reindex_by_doi_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
# frozen_string_literal: true

class ReindexByDoiJob < ApplicationJob
include Shoryuken::Worker

queue_as :lupo_background
shoryuken_options queue: -> { "#{ENV["RAILS_ENV"]}_lupo_background" }, auto_delete: true
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Keep the Shoryuken queue name aligned with the publisher.

Line 7 hard-codes ENV["RAILS_ENV"], but app/services/sqs_message_service.rb:6-7 publishes to ENV["SQS_PREFIX"].presence || Rails.env. In prefixed environments, producers will send to <prefix>_lupo_background while this worker listens on <RAILS_ENV>_lupo_background, so it will never see those messages.

💡 Proposed fix 
-  shoryuken_options queue: -> { "#{ENV["RAILS_ENV"]}_lupo_background" }, auto_delete: true
+  shoryuken_options queue: -> { "#{ENV["SQS_PREFIX"].presence || Rails.env}_lupo_background" }, auto_delete: true
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
shoryuken_options queue: -> { "#{ENV["RAILS_ENV"]}_lupo_background" }, auto_delete: true
shoryuken_options queue: -> { "#{ENV["SQS_PREFIX"].presence || Rails.env}_lupo_background" }, auto_delete: true
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/jobs/reindex_by_doi_job.rb` at line 7, The shoryuken queue name is
hard-coded to ENV["RAILS_ENV"] in the shoryuken_options call; change it to use
the same prefix logic as the publisher (ENV["SQS_PREFIX"].presence || Rails.env)
so the worker listens on the same "<prefix>_lupo_background" queue as producers;
update the lambda passed to shoryuken_options (the shoryuken_options call) to
build the queue name from ENV["SQS_PREFIX"].presence || Rails.env instead of
ENV["RAILS_ENV"].


def perform(sqs_message = nil, data = nil)
parsed = JSON.parse(data)
return unless parsed.is_a?(Hash)

doi_id = parsed["doi"]
return if doi_id.blank?
Comment on lines +9 to +14
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Handle invalid payloads before parsing.

JSON.parse(data) can raise on nil, malformed JSON, or a Hash, so Line 11 never runs. On a queue consumer, that turns one bad payload into an exception path before the worker can decide how to handle it.

💡 Proposed fix 
   def perform(sqs_message = nil, data = nil)
-    parsed = JSON.parse(data)
+    parsed =
+      case data
+      when Hash
+        data
+      when String
+        JSON.parse(data)
+      else
+        return
+      end
     return unless parsed.is_a?(Hash)
 
-    doi_id = parsed["doi"]
+    doi_id = parsed["doi"] || parsed[:doi]
     return if doi_id.blank?
 
     doi = Doi.find_by(doi: doi_id)
     return unless doi.present?
 
     if doi.agency == "datacite"
       DataciteDoiImportInBulkJob.perform_later([doi.id], index: DataciteDoi.active_index)
     else
       OtherDoiImportInBulkJob.perform_later([doi.id], index: OtherDoi.active_index)
     end
+  rescue JSON::ParserError
+    return
   end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def perform(sqs_message = nil, data = nil)
parsed = JSON.parse(data)
return unless parsed.is_a?(Hash)
doi_id = parsed["doi"]
return if doi_id.blank?
def perform(sqs_message = nil, data = nil)
parsed =
case data
when Hash
data
when String
JSON.parse(data)
else
return
end
return unless parsed.is_a?(Hash)
doi_id = parsed["doi"] || parsed[:doi]
return if doi_id.blank?
doi = Doi.find_by(doi: doi_id)
return unless doi.present?
if doi.agency == "datacite"
DataciteDoiImportInBulkJob.perform_later([doi.id], index: DataciteDoi.active_index)
else
OtherDoiImportInBulkJob.perform_later([doi.id], index: OtherDoi.active_index)
end
rescue JSON::ParserError
return
end
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@app/jobs/reindex_by_doi_job.rb` around lines 9 - 14, The perform method calls
JSON.parse(data) directly which will raise for nil or malformed JSON; change
perform to first guard against nil/empty payload (e.g. return unless
data.present?) and then safely parse inside a begin/rescue that catches
JSON::ParserError and TypeError, returning (and optionally logging) when parsing
fails; after the safe parse ensure parsed.is_a?(Hash) before accessing
parsed["doi"] so invalid payloads do not cause exceptions in perform.


def perform(doi_id, _options = {})
doi = Doi.find_by(doi: doi_id)
return unless doi.present?

Expand Down
6 changes: 3 additions & 3 deletions spec/jobs/reindex_by_doi_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
describe ReindexByDoiJob, type: :job do
let(:datacite_doi) { create(:doi, agency: "datacite") }
let(:other_doi) { create(:doi, agency: "crossref") }
subject(:job) { ReindexByDoiJob.perform_later(datacite_doi.doi) }
subject(:job) { ReindexByDoiJob.perform_later(nil, { doi: datacite_doi.doi }.to_json) }

it "queues the job" do
expect { job }.to have_enqueued_job(ReindexByDoiJob).on_queue(
Expand All @@ -14,15 +14,15 @@
end

it "queues DataciteDoiImportInBulkJob for agency 'datacite'" do
ReindexByDoiJob.perform_now(datacite_doi.doi)
ReindexByDoiJob.new.perform(nil, { doi: datacite_doi.doi }.to_json)

enqueued_job = enqueued_jobs.find { |j| j[:job] == DataciteDoiImportInBulkJob }
expect(enqueued_job).to be_present
expect(enqueued_job[:args].first).to eq([datacite_doi.id])
end

it "queues OtherDoiImportInBulkJob for agency 'crossref'" do
ReindexByDoiJob.perform_now(other_doi.doi)
ReindexByDoiJob.new.perform(nil, { doi: other_doi.doi }.to_json)

enqueued_job = enqueued_jobs.find { |j| j[:job] == OtherDoiImportInBulkJob }
expect(enqueued_job).to be_present
Expand Down
Loading