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
Merged

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

Show file tree
Hide file tree
Changes from 2 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
5 changes: 4 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,12 @@
# 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(doi_id, _options = {})
def perform(sqs_message = nil, doi_id = nil)
doi = Doi.find_by(doi: doi_id)
return unless doi.present?
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 | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Find all places where ReindexByDoiJob is invoked to verify argument compatibility

echo "=== Direct invocations of ReindexByDoiJob ==="
rg -n -C3 'ReindexByDoiJob\.(perform_later|perform_now|perform_async|set)' --type ruby

echo ""
echo "=== Test file contents ==="
cat spec/jobs/reindex_by_doi_job_spec.rb 2>/dev/null || echo "Test file not found"

Repository: datacite/lupo

Length of output: 2860

🏁 Script executed:

cat -n app/jobs/reindex_by_doi_job.rb

Repository: datacite/lupo

Length of output: 720

🏁 Script executed:

rg -n 'ReindexByDoiJob' --type ruby -B2 -A2

Repository: datacite/lupo

Length of output: 1931

Critical: Method signature breaks all existing perform_later and perform_now invocations.

The current signature def perform(sqs_message = nil, doi_id = nil) causes all single-argument invocations to fail. When called with perform_later(doi_string) or perform_now(doi_string), the DOI value is assigned to the sqs_message parameter, leaving doi_id as nil. This causes Doi.find_by(doi: nil) to return nil, and the job silently exits without processing.

All three test invocations in spec/jobs/reindex_by_doi_job_spec.rb are broken:

  • Line 8: ReindexByDoiJob.perform_later(datacite_doi.doi)
  • Line 17: ReindexByDoiJob.perform_now(datacite_doi.doi)
  • Line 25: ReindexByDoiJob.perform_now(other_doi.doi)
Proposed fix to support both ActiveJob and Shoryuken

If this job needs to support both ActiveJob (perform_later) and direct Shoryuken invocation, detect how it was invoked:

  def perform(sqs_message = nil, doi_id = nil)
+   # When called via perform_later, sqs_message contains the doi_id
+   # When called directly by Shoryuken, doi_id is the second argument
+   resolved_doi_id = doi_id || sqs_message
-   doi = Doi.find_by(doi: doi_id)
+   doi = Doi.find_by(doi: resolved_doi_id)

Alternatively, if this job is only invoked via Shoryuken, update all callers to pass the second argument:

-  subject(:job) { ReindexByDoiJob.perform_later(datacite_doi.doi) }
+  subject(:job) { ReindexByDoiJob.perform_later(nil, datacite_doi.doi) }

and

-  ReindexByDoiJob.perform_now(datacite_doi.doi)
+  ReindexByDoiJob.perform_now(nil, datacite_doi.doi)
📝 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, doi_id = nil)
doi = Doi.find_by(doi: doi_id)
return unless doi.present?
def perform(sqs_message = nil, doi_id = nil)
# When called via perform_later, sqs_message contains the doi_id
# When called directly by Shoryuken, doi_id is the second argument
resolved_doi_id = doi_id || sqs_message
doi = Doi.find_by(doi: resolved_doi_id)
return unless doi.present?
🤖 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 8 - 10, The perform signature
currently def perform(sqs_message = nil, doi_id = nil) causes single-argument
calls (perform_later/perform_now) to place the DOI into sqs_message and leave
doi_id nil; update perform in ReindexByDoiJob so it normalizes inputs: if doi_id
is nil then detect whether sqs_message is the DOI string (e.g., String) or a
Shoryuken/SQS payload (e.g., Hash with "body" or :body) and set doi_id
accordingly before calling Doi.find_by(doi: doi_id); this preserves support for
perform_later/perform_now and direct Shoryuken invocation while keeping calls to
Doi.find_by unchanged.


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, datacite_doi.doi) }

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, datacite_doi.doi)

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, other_doi.doi)

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