From 394136ca194d3cc4cc9b0cb03aa1987f7ba0884c Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Tue, 24 Aug 2021 16:30:51 -0700 Subject: [PATCH 01/11] Create a ruby worker job for RISC notifications (LG-4972) - Also clean up old eventbridge code (LG-4974) --- Gemfile | 1 - Gemfile.lock | 4 - app/jobs/risc_delivery_job.rb | 51 +++++++ app/services/push_notification/http_push.rb | 90 ++--------- .../push_notification_error.rb | 4 - config/application.yml.default | 1 + lib/identity_config.rb | 2 +- spec/jobs/risc_delivery_job_spec.rb | 67 +++++++++ .../push_notification/http_push_spec.rb | 142 +++--------------- 9 files changed, 153 insertions(+), 209 deletions(-) create mode 100644 app/jobs/risc_delivery_job.rb delete mode 100644 app/services/push_notification/push_notification_error.rb create mode 100644 spec/jobs/risc_delivery_job_spec.rb diff --git a/Gemfile b/Gemfile index a50142a77b9..5f57c861658 100644 --- a/Gemfile +++ b/Gemfile @@ -22,7 +22,6 @@ gem 'ahoy_matey', '~> 3.0' gem 'autoprefixer-rails', '~> 10.0' gem 'aws-sdk-kms', '~> 1.4' gem 'aws-sdk-ses', '~> 1.6' -gem 'aws-sdk-eventbridge' gem 'base32-crockford' gem 'blueprinter', '~> 0.25.3' gem 'device_detector' diff --git a/Gemfile.lock b/Gemfile.lock index 2c27d0334bc..dce059bf720 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -130,9 +130,6 @@ GEM aws-partitions (~> 1, >= 1.239.0) aws-sigv4 (~> 1.1) jmespath (~> 1.0) - aws-sdk-eventbridge (1.18.0) - aws-sdk-core (~> 3, >= 3.109.0) - aws-sigv4 (~> 1.1) aws-sdk-kms (1.40.0) aws-sdk-core (~> 3, >= 3.109.0) aws-sigv4 (~> 1.1) @@ -704,7 +701,6 @@ DEPENDENCIES ahoy_matey (~> 3.0) autoprefixer-rails (~> 10.0) aws-sdk-cloudwatchlogs - aws-sdk-eventbridge aws-sdk-kms (~> 1.4) aws-sdk-ses (~> 1.6) axe-core-rspec (~> 4.2) diff --git a/app/jobs/risc_delivery_job.rb b/app/jobs/risc_delivery_job.rb new file mode 100644 index 00000000000..ee79d46287f --- /dev/null +++ b/app/jobs/risc_delivery_job.rb @@ -0,0 +1,51 @@ +class RiscDeliveryJob < ApplicationJob + queue_as :low + + def perform( + push_notification_url:, + jwt:, + event_type:, + issuer:, + transport: + ) + response = faraday.post( + push_notification_url, + jwt, + 'Accept' => 'application/json', + 'Content-Type' => 'application/secevent+jwt', + ) do |req| + req.options.context = { service_name: 'http_push_direct' } + end + + unless response.success? + Rails.logger.warn( + { + event: 'http_push_error', + transport: transport, + event_type: event_type, + service_provider: issuer, + status: response.status, + }.to_json, + ) + end + rescue Faraday::TimeoutError, Faraday::ConnectionFailed => err + raise err if transport == 'ruby_worker' + + Rails.logger.warn( + { + event: 'http_push_error', + transport: 'direct', + event_type: event_type, + service_provider: issuer, + error: err.message, + }.to_json, + ) + end + + def faraday + Faraday.new do |f| + f.request :instrumentation, name: 'request_log.faraday' + f.adapter :net_http + end + end +end diff --git a/app/services/push_notification/http_push.rb b/app/services/push_notification/http_push.rb index 958b41e09aa..b21cb4154f5 100644 --- a/app/services/push_notification/http_push.rb +++ b/app/services/push_notification/http_push.rb @@ -38,72 +38,23 @@ def url_options def deliver_one(service_provider) deliver_local(service_provider) if IdentityConfig.store.risc_notifications_local_enabled - if IdentityConfig.store.risc_notifications_eventbridge_enabled - deliver_eventbridge(service_provider) - else - deliver_direct(service_provider) - end - end - - def deliver_eventbridge(service_provider) - response = eventbridge_client.put_events( - entries: [ - { - time: now, - source: service_provider.issuer, - detail_type: 'notification', - detail: { jwt: jwt(service_provider) }.to_json, - event_bus_name: "#{Identity::Hostdata.env}-risc-notifications", - }, - ], - ) - - if response.failed_entry_count.to_i > 0 - Rails.logger.warn( - { - event: 'http_push_error', - transport: 'eventbridge', - event_type: event.event_type, - service_provider: service_provider.issuer, - error: response.to_s, - }.to_json, + if IdentityConfig.store.risc_notifications_active_job_enabled + RiscDeliveryJob.perform_later( + push_notification_url: service_provider.push_notification_url, + jwt: jwt(service_provider), + event_type: event.event_type, + issuer: service_provider.issuer, + transport: 'ruby_worker', ) - end - end - - def deliver_direct(service_provider) - response = faraday.post( - service_provider.push_notification_url, - jwt(service_provider), - 'Accept' => 'application/json', - 'Content-Type' => 'application/secevent+jwt', - ) do |req| - req.options.context = { service_name: 'http_push_direct' } - end - - unless response.success? - Rails.logger.warn( - { - event: 'http_push_error', - transport: 'direct', - event_type: event.event_type, - service_provider: service_provider.issuer, - status: response.status, - }.to_json, + else + RiscDeliveryJob.perform_now( + push_notification_url: service_provider.push_notification_url, + jwt: jwt(service_provider), + event_type: event.event_type, + issuer: service_provider.issuer, + transport: 'direct', ) end - rescue Faraday::TimeoutError, - Faraday::ConnectionFailed, - PushNotification::PushNotificationError => err - Rails.logger.warn( - { - event: 'http_push_error', - transport: 'direct', - event_type: event.event_type, - service_provider: service_provider.issuer, - error: err.message, - }.to_json, - ) end def deliver_local(service_provider) @@ -134,13 +85,6 @@ def jwt_payload(service_provider) } end - def faraday - Faraday.new do |f| - f.request :instrumentation, name: 'request_log.faraday' - f.adapter :net_http - end - end - def agency_uuid(service_provider) AgencyIdentity.find_by( user_id: event.user.id, @@ -151,11 +95,5 @@ def agency_uuid(service_provider) service_provider: service_provider.issuer, )&.uuid end - - def eventbridge_client - @eventbridge_client ||= Aws::EventBridge::Client.new( - region: Identity::Hostdata.aws_region, - ) - end end end diff --git a/app/services/push_notification/push_notification_error.rb b/app/services/push_notification/push_notification_error.rb deleted file mode 100644 index d7f682e1106..00000000000 --- a/app/services/push_notification/push_notification_error.rb +++ /dev/null @@ -1,4 +0,0 @@ -module PushNotification - class PushNotificationError < StandardError - end -end diff --git a/config/application.yml.default b/config/application.yml.default index 15db291b02a..c9f211e56f5 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -178,6 +178,7 @@ reset_password_email_max_attempts: '20' reset_password_email_window_in_minutes: '60' risc_notifications_local_enabled: 'false' risc_notifications_eventbridge_enabled: 'false' +risc_notifications_active_job_enabled: 'false' ruby_workers_enabled: 'true' rules_of_use_horizon_years: '6' rules_of_use_updated_at: '2021-05-21T00:00:00Z' diff --git a/lib/identity_config.rb b/lib/identity_config.rb index cb98ad0df69..6b72fa8919c 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -244,7 +244,7 @@ def self.build_store(config_map) config.add(:reset_password_email_max_attempts, type: :integer) config.add(:reset_password_email_window_in_minutes, type: :integer) config.add(:risc_notifications_local_enabled, type: :boolean) - config.add(:risc_notifications_eventbridge_enabled, type: :boolean) + config.add(:risc_notifications_active_job_enabled, type: :boolean) config.add(:ruby_workers_enabled, type: :boolean) config.add(:rules_of_use_horizon_years, type: :integer) config.add(:rules_of_use_updated_at, type: :timestamp) diff --git a/spec/jobs/risc_delivery_job_spec.rb b/spec/jobs/risc_delivery_job_spec.rb new file mode 100644 index 00000000000..bfe47282e75 --- /dev/null +++ b/spec/jobs/risc_delivery_job_spec.rb @@ -0,0 +1,67 @@ +require 'rails_helper' + +RSpec.describe RiscDeliveryJob do + let(:push_notification_url) { 'https://push.example.gov' } + let(:jwt) { JWT.encode({ foo: 'bar' }, 'a') } + let(:event_type) { PushNotification::IdentifierRecycledEvent::EVENT_TYPE } + let(:issuer) { 'issuer1' } + let(:transport) { 'ruby_worker' } + + subject(:job) do + RiscDeliveryJob.new( + push_notification_url: push_notification_url, + jwt: jwt, + event_type: event_type, + issuer: issuer, + transport: transport, + ) + end + + describe '#perform' do + it 'POSTs the jwt to the given URL' do + req = stub_request(:post, push_notification_url). + with( + body: jwt, + headers: { + 'Content-Type' => 'application/secevent+jwt', + 'Accept' => 'application/json', + }, + ) + + job.perform_now + + expect(req).to have_been_requested + end + + context 'network errors' do + before do + stub_request(:post, push_notification_url).to_timeout + end + + context 'when performed inline' do + let(:transport) { 'direct' } + + it 'warns on timeouts' do + expect(Rails.logger).to receive(:warn) do |msg| + payload = JSON.parse(msg, symbolize_names: true) + + expect(payload[:event]).to eq('http_push_error') + expect(payload[:transport]).to eq(transport) + end + + expect { job.perform_now }.to_not raise_error + end + end + + context 'when performed in a worker' do + let(:transport) { 'ruby_worker' } + + it 'raises on timeouts (and retries via ActiveJob)' do + expect(Rails.logger).to_not receive(:warn) + + expect { job.perform_now }.to raise_error(Faraday::ConnectionFailed) + end + end + end + end +end diff --git a/spec/services/push_notification/http_push_spec.rb b/spec/services/push_notification/http_push_spec.rb index 45d24e38958..33baec07c5c 100644 --- a/spec/services/push_notification/http_push_spec.rb +++ b/spec/services/push_notification/http_push_spec.rb @@ -22,20 +22,17 @@ ) end let(:now) { Time.zone.now } - let(:risc_notifications_eventbridge_enabled) { false } + let(:risc_notifications_active_job_enabled) { false } let(:push_notifications_enabled) { true } - let(:eventbridge_client) { Aws::EventBridge::Client.new(stub_responses: true) } subject(:http_push) { PushNotification::HttpPush.new(event, now: now) } before do - allow(IdentityConfig.store).to receive(:risc_notifications_eventbridge_enabled). - and_return(risc_notifications_eventbridge_enabled) + allow(IdentityConfig.store).to receive(:risc_notifications_active_job_enabled). + and_return(risc_notifications_active_job_enabled) allow(Identity::Hostdata).to receive(:env).and_return('dev') allow(IdentityConfig.store).to receive(:push_notifications_enabled). and_return(push_notifications_enabled) - - allow(http_push).to receive(:eventbridge_client).and_return(eventbridge_client) end describe '#deliver' do @@ -51,52 +48,14 @@ end end - context 'when the EventBridge is disabled' do - let(:risc_notifications_eventbridge_enabled) { false } - - it 'makes an HTTP post to service providers with a push_notification_url' do - stub_request(:post, sp_with_push_url.push_notification_url). - with do |request| - expect(request.headers['Content-Type']).to eq('application/secevent+jwt') - expect(request.headers['Accept']).to eq('application/json') - - payload, headers = JWT.decode( - request.body, - AppArtifacts.store.oidc_public_key, - true, - algorithm: 'RS256', - ) - - expect(headers['typ']).to eq('secevent+jwt') - - expect(payload['iss']).to eq(root_url) - expect(payload['iat']).to eq(now.to_i) - expect(payload['exp']).to eq((now + 12.hours).to_i) - expect(payload['aud']).to eq(sp_with_push_url.push_notification_url) - expect(payload['events']).to eq(event.event_type => event.payload.as_json) - end - - deliver - end - end - - context 'when the EventBridge is enabled' do - let(:risc_notifications_eventbridge_enabled) { true } - - it 'posts to the EventBridge' do - expect(eventbridge_client).to receive(:put_events).and_wrap_original do |impl, entries:| - expect(entries.size).to eq(1) - entry = entries.first - - expect(entry[:time]).to eq(now) - expect(entry[:source]).to eq(sp_with_push_url.issuer) - expect(entry[:event_bus_name]).to eq('dev-risc-notifications') - expect(entry[:detail_type]).to eq('notification') - - detail_json = JSON.parse(entry[:detail], symbolize_names: true) + it 'makes an HTTP post to service providers with a push_notification_url' do + stub_request(:post, sp_with_push_url.push_notification_url). + with do |request| + expect(request.headers['Content-Type']).to eq('application/secevent+jwt') + expect(request.headers['Accept']).to eq('application/json') payload, headers = JWT.decode( - detail_json[:jwt], + request.body, AppArtifacts.store.oidc_public_key, true, algorithm: 'RS256', @@ -109,28 +68,9 @@ expect(payload['exp']).to eq((now + 12.hours).to_i) expect(payload['aud']).to eq(sp_with_push_url.push_notification_url) expect(payload['events']).to eq(event.event_type => event.payload.as_json) - - impl.call(entries: entries) - end - - deliver - end - - context 'with an error from eventbridge' do - before do - eventbridge_client.stub_responses( - :put_events, - failed_entry_count: 1, - entries: [{ error_code: 'MalformedDetail', error_message: 'Detail is malformed' }], - ) end - it 'logs a warning' do - expect(Rails.logger).to receive(:warn) - - deliver - end - end + deliver end context 'with an event that sends agency-specific iss_sub' do @@ -138,50 +78,20 @@ let(:agency_uuid) { AgencyIdentityLinker.new(sp_with_push_url_identity).link_identity.uuid } - context 'when the EventBridge is disabled' do - let(:risc_notifications_eventbridge_enabled) { false } - - it 'sends the agency-specific uuid' do - stub_request(:post, sp_with_push_url.push_notification_url). - with do |request| - payload, _headers = JWT.decode( - request.body, - AppArtifacts.store.oidc_public_key, - true, - algorithm: 'RS256', - ) - - expect(payload['events'][event.event_type]['subject']['sub']).to eq(agency_uuid) - end - - deliver - end - end - - context 'when the EventBridge is enabled' do - let(:risc_notifications_eventbridge_enabled) { true } - - it 'sends the agency-specific uuid' do - expect(eventbridge_client).to receive(:put_events).and_wrap_original do |impl, entries:| - expect(entries.size).to eq(1) - entry = entries.first - - detail_json = JSON.parse(entry[:detail], symbolize_names: true) - - payload, headers = JWT.decode( - detail_json[:jwt], + it 'sends the agency-specific uuid' do + stub_request(:post, sp_with_push_url.push_notification_url). + with do |request| + payload, _headers = JWT.decode( + request.body, AppArtifacts.store.oidc_public_key, true, algorithm: 'RS256', ) expect(payload['events'][event.event_type]['subject']['sub']).to eq(agency_uuid) - - impl.call(entries: entries) end - deliver - end + deliver end end @@ -238,24 +148,10 @@ RevokeServiceProviderConsent.new(identity).call end - context 'when the EventBridge is disabled' do - let(:risc_notifications_eventbridge_enabled) { false } - - it 'does not notify that SP' do - deliver - - expect(WebMock).not_to have_requested(:get, sp_with_push_url.push_notification_url) - end - end - - context 'when the EventBridge is enabled' do - let(:risc_notifications_eventbridge_enabled) { true } - - it 'does not notify that SP' do - expect(eventbridge_client).to_not receive(:put_events) + it 'does not notify that SP' do + deliver - deliver - end + expect(WebMock).not_to have_requested(:get, sp_with_push_url.push_notification_url) end end end From 413612725c7c75068b3105a0bd21c05e61f4c89d Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Tue, 24 Aug 2021 16:32:35 -0700 Subject: [PATCH 02/11] retry_on --- app/jobs/risc_delivery_job.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/jobs/risc_delivery_job.rb b/app/jobs/risc_delivery_job.rb index ee79d46287f..88e0b8a677e 100644 --- a/app/jobs/risc_delivery_job.rb +++ b/app/jobs/risc_delivery_job.rb @@ -1,6 +1,8 @@ class RiscDeliveryJob < ApplicationJob queue_as :low + retry_on Faraday::TimeoutError, Faraday::ConnectionFailed + def perform( push_notification_url:, jwt:, From 2499f2960b64b6a65e40187660505dafb7438a10 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Tue, 24 Aug 2021 16:33:40 -0700 Subject: [PATCH 03/11] exponential backoff --- app/jobs/risc_delivery_job.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/jobs/risc_delivery_job.rb b/app/jobs/risc_delivery_job.rb index 88e0b8a677e..c9ad0b9a0a2 100644 --- a/app/jobs/risc_delivery_job.rb +++ b/app/jobs/risc_delivery_job.rb @@ -1,7 +1,7 @@ class RiscDeliveryJob < ApplicationJob queue_as :low - retry_on Faraday::TimeoutError, Faraday::ConnectionFailed + retry_on Faraday::TimeoutError, Faraday::ConnectionFailed, wait: :exponentially_longer def perform( push_notification_url:, From 9ec645e9a43dbb7e76e961af71b18207eb9ae8d8 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Tue, 24 Aug 2021 16:36:53 -0700 Subject: [PATCH 04/11] remove old config --- config/application.yml.default | 1 - 1 file changed, 1 deletion(-) diff --git a/config/application.yml.default b/config/application.yml.default index c9f211e56f5..716bfa60632 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -177,7 +177,6 @@ requests_per_ip_track_only_mode: 'false' reset_password_email_max_attempts: '20' reset_password_email_window_in_minutes: '60' risc_notifications_local_enabled: 'false' -risc_notifications_eventbridge_enabled: 'false' risc_notifications_active_job_enabled: 'false' ruby_workers_enabled: 'true' rules_of_use_horizon_years: '6' From 03a22409a3b24a8e222cddb692e34aad4ee04099 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Tue, 24 Aug 2021 16:59:52 -0700 Subject: [PATCH 05/11] Update specs to call perform directly instead of through adapter **Why**: retry_on exceptions are unhandled and caused errors --- spec/jobs/risc_delivery_job_spec.rb | 36 ++++++++++++++--------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/spec/jobs/risc_delivery_job_spec.rb b/spec/jobs/risc_delivery_job_spec.rb index bfe47282e75..9c1670305d0 100644 --- a/spec/jobs/risc_delivery_job_spec.rb +++ b/spec/jobs/risc_delivery_job_spec.rb @@ -1,23 +1,23 @@ require 'rails_helper' RSpec.describe RiscDeliveryJob do - let(:push_notification_url) { 'https://push.example.gov' } - let(:jwt) { JWT.encode({ foo: 'bar' }, 'a') } - let(:event_type) { PushNotification::IdentifierRecycledEvent::EVENT_TYPE } - let(:issuer) { 'issuer1' } - let(:transport) { 'ruby_worker' } + describe '#perform' do + let(:push_notification_url) { 'https://push.example.gov' } + let(:jwt) { JWT.encode({ foo: 'bar' }, 'a') } + let(:event_type) { PushNotification::IdentifierRecycledEvent::EVENT_TYPE } + let(:issuer) { 'issuer1' } + let(:transport) { 'ruby_worker' } - subject(:job) do - RiscDeliveryJob.new( - push_notification_url: push_notification_url, - jwt: jwt, - event_type: event_type, - issuer: issuer, - transport: transport, - ) - end + subject(:perform) do + RiscDeliveryJob.new.perform( + push_notification_url: push_notification_url, + jwt: jwt, + event_type: event_type, + issuer: issuer, + transport: transport, + ) + end - describe '#perform' do it 'POSTs the jwt to the given URL' do req = stub_request(:post, push_notification_url). with( @@ -28,7 +28,7 @@ }, ) - job.perform_now + perform expect(req).to have_been_requested end @@ -49,7 +49,7 @@ expect(payload[:transport]).to eq(transport) end - expect { job.perform_now }.to_not raise_error + expect { perform }.to_not raise_error end end @@ -59,7 +59,7 @@ it 'raises on timeouts (and retries via ActiveJob)' do expect(Rails.logger).to_not receive(:warn) - expect { job.perform_now }.to raise_error(Faraday::ConnectionFailed) + expect { perform }.to raise_error(Faraday::ConnectionFailed) end end end From 4cb7a0ff91aafa8ae4a5e5da52f7961e7312c373 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Wed, 25 Aug 2021 13:07:38 -0700 Subject: [PATCH 06/11] Add some more spec coverage --- spec/services/push_notification/http_push_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/services/push_notification/http_push_spec.rb b/spec/services/push_notification/http_push_spec.rb index 33baec07c5c..625ba74106c 100644 --- a/spec/services/push_notification/http_push_spec.rb +++ b/spec/services/push_notification/http_push_spec.rb @@ -48,6 +48,17 @@ end end + context 'when risc_notifications_active_job_enabled is enabled' do + let(:risc_notifications_active_job_enabled) { true } + + it 'delivers a notification via background job' do + expect(RiscDeliveryJob).to receive(:perform_later). + with(hash_including(transport: 'ruby_worker')) + + deliver + end + end + it 'makes an HTTP post to service providers with a push_notification_url' do stub_request(:post, sp_with_push_url.push_notification_url). with do |request| From 5693abb35bd9f9614c2caf4ca2dcf2259dcf55c8 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 26 Aug 2021 10:12:44 -0700 Subject: [PATCH 07/11] Set network timeouts for push notification delivery --- app/jobs/risc_delivery_job.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/jobs/risc_delivery_job.rb b/app/jobs/risc_delivery_job.rb index c9ad0b9a0a2..0e09a5b7598 100644 --- a/app/jobs/risc_delivery_job.rb +++ b/app/jobs/risc_delivery_job.rb @@ -48,6 +48,10 @@ def faraday Faraday.new do |f| f.request :instrumentation, name: 'request_log.faraday' f.adapter :net_http + f.options.timeout = 3 + f.options.read_timeout = 3 + f.options.open_timeout = 3 + f.options.write_timeout = 3 end end end From 551ae09ef1461b2d92f43489ab709cbcef4f9399 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 26 Aug 2021 10:25:43 -0700 Subject: [PATCH 08/11] Update service_name for faraday logging --- app/jobs/risc_delivery_job.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/jobs/risc_delivery_job.rb b/app/jobs/risc_delivery_job.rb index 0e09a5b7598..e8f19f07ea4 100644 --- a/app/jobs/risc_delivery_job.rb +++ b/app/jobs/risc_delivery_job.rb @@ -16,7 +16,9 @@ def perform( 'Accept' => 'application/json', 'Content-Type' => 'application/secevent+jwt', ) do |req| - req.options.context = { service_name: 'http_push_direct' } + req.options.context = { + service_name: transport == 'ruby_worker' ? 'risc_http_push_async' : 'risc_http_push_direct' + } end unless response.success? From 403373b0ebb77ad2542bd367bcbddb5dc2fd3732 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 26 Aug 2021 10:36:38 -0700 Subject: [PATCH 09/11] lint --- app/jobs/risc_delivery_job.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/jobs/risc_delivery_job.rb b/app/jobs/risc_delivery_job.rb index e8f19f07ea4..acbe1fad681 100644 --- a/app/jobs/risc_delivery_job.rb +++ b/app/jobs/risc_delivery_job.rb @@ -17,7 +17,7 @@ def perform( 'Content-Type' => 'application/secevent+jwt', ) do |req| req.options.context = { - service_name: transport == 'ruby_worker' ? 'risc_http_push_async' : 'risc_http_push_direct' + service_name: transport == 'ruby_worker' ? 'risc_http_push_async' : 'risc_http_push_direct', } end From 03917be60f360848308e2c2487b9ab741c7882da Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 26 Aug 2021 11:05:24 -0700 Subject: [PATCH 10/11] Clean up thanks to ApplicationJob#queue_adapter --- app/jobs/risc_delivery_job.rb | 13 ++++++++----- app/services/push_notification/http_push.rb | 2 -- spec/jobs/risc_delivery_job_spec.rb | 15 ++++++++------- spec/services/push_notification/http_push_spec.rb | 3 +-- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/app/jobs/risc_delivery_job.rb b/app/jobs/risc_delivery_job.rb index acbe1fad681..e821496b834 100644 --- a/app/jobs/risc_delivery_job.rb +++ b/app/jobs/risc_delivery_job.rb @@ -7,8 +7,7 @@ def perform( push_notification_url:, jwt:, event_type:, - issuer:, - transport: + issuer: ) response = faraday.post( push_notification_url, @@ -17,7 +16,7 @@ def perform( 'Content-Type' => 'application/secevent+jwt', ) do |req| req.options.context = { - service_name: transport == 'ruby_worker' ? 'risc_http_push_async' : 'risc_http_push_direct', + service_name: inline? ? 'risc_http_push_direct' : 'risc_http_push_async', } end @@ -25,7 +24,7 @@ def perform( Rails.logger.warn( { event: 'http_push_error', - transport: transport, + transport: inline? ? 'direct' : 'async', event_type: event_type, service_provider: issuer, status: response.status, @@ -33,7 +32,7 @@ def perform( ) end rescue Faraday::TimeoutError, Faraday::ConnectionFailed => err - raise err if transport == 'ruby_worker' + raise err if !inline? Rails.logger.warn( { @@ -56,4 +55,8 @@ def faraday f.options.write_timeout = 3 end end + + def inline? + queue_adapter.is_a?(ActiveJob::QueueAdapters::InlineAdapter) + end end diff --git a/app/services/push_notification/http_push.rb b/app/services/push_notification/http_push.rb index b21cb4154f5..b58918d70ed 100644 --- a/app/services/push_notification/http_push.rb +++ b/app/services/push_notification/http_push.rb @@ -44,7 +44,6 @@ def deliver_one(service_provider) jwt: jwt(service_provider), event_type: event.event_type, issuer: service_provider.issuer, - transport: 'ruby_worker', ) else RiscDeliveryJob.perform_now( @@ -52,7 +51,6 @@ def deliver_one(service_provider) jwt: jwt(service_provider), event_type: event.event_type, issuer: service_provider.issuer, - transport: 'direct', ) end end diff --git a/spec/jobs/risc_delivery_job_spec.rb b/spec/jobs/risc_delivery_job_spec.rb index 9c1670305d0..c98b76b66b2 100644 --- a/spec/jobs/risc_delivery_job_spec.rb +++ b/spec/jobs/risc_delivery_job_spec.rb @@ -8,13 +8,13 @@ let(:issuer) { 'issuer1' } let(:transport) { 'ruby_worker' } + let(:job) { RiscDeliveryJob.new } subject(:perform) do - RiscDeliveryJob.new.perform( + job.perform( push_notification_url: push_notification_url, jwt: jwt, event_type: event_type, - issuer: issuer, - transport: transport, + issuer: issuer ) end @@ -39,14 +39,12 @@ end context 'when performed inline' do - let(:transport) { 'direct' } - it 'warns on timeouts' do expect(Rails.logger).to receive(:warn) do |msg| payload = JSON.parse(msg, symbolize_names: true) expect(payload[:event]).to eq('http_push_error') - expect(payload[:transport]).to eq(transport) + expect(payload[:transport]).to eq('direct') end expect { perform }.to_not raise_error @@ -54,7 +52,10 @@ end context 'when performed in a worker' do - let(:transport) { 'ruby_worker' } + before do + allow(job).to receive(:queue_adapter). + and_return(ActiveJob::QueueAdapters::GoodJobAdapter.new) + end it 'raises on timeouts (and retries via ActiveJob)' do expect(Rails.logger).to_not receive(:warn) diff --git a/spec/services/push_notification/http_push_spec.rb b/spec/services/push_notification/http_push_spec.rb index 625ba74106c..9c6566b129e 100644 --- a/spec/services/push_notification/http_push_spec.rb +++ b/spec/services/push_notification/http_push_spec.rb @@ -52,8 +52,7 @@ let(:risc_notifications_active_job_enabled) { true } it 'delivers a notification via background job' do - expect(RiscDeliveryJob).to receive(:perform_later). - with(hash_including(transport: 'ruby_worker')) + expect(RiscDeliveryJob).to receive(:perform_later) deliver end From de7c0408fbbb7a96fc924e57b55fba45927458c7 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 26 Aug 2021 11:18:15 -0700 Subject: [PATCH 11/11] another comma --- spec/jobs/risc_delivery_job_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/jobs/risc_delivery_job_spec.rb b/spec/jobs/risc_delivery_job_spec.rb index c98b76b66b2..6ab64350d7b 100644 --- a/spec/jobs/risc_delivery_job_spec.rb +++ b/spec/jobs/risc_delivery_job_spec.rb @@ -14,7 +14,7 @@ push_notification_url: push_notification_url, jwt: jwt, event_type: event_type, - issuer: issuer + issuer: issuer, ) end