ridp_rba : add subscribers and publisher in FDSH#324
Open
jayreddy519 wants to merge 9 commits intotrunkfrom
Open
ridp_rba : add subscribers and publisher in FDSH#324jayreddy519 wants to merge 9 commits intotrunkfrom
jayreddy519 wants to merge 9 commits intotrunkfrom
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb`:
- Around line 30-36: The rescue block calls publish_failure(job_id,
correlation_id) and assumes job_id and correlation_id were already assigned;
initialize job_id and correlation_id to safe defaults (e.g., nil or a
placeholder string) before the begin/handler scope or ensure they're set via
safe navigation when reading properties, and update the rescue to pass those
defaults to publish_failure and to include them in logger.error; locate the
rescue in determination_requested_subscriber.rb (around the
on_enroll_ridp_rba_determination_requested flow) and ensure
ack(delivery_info.delivery_tag) still runs after safe defaults are used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: feabce58-a893-43df-b31b-d59530065518
📒 Files selected for processing (2)
app/event_source/publishers/fdsh/ridp_rba/ridp_rba_publisher.rbapp/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb (1)
27-29:⚠️ Potential issue | 🟠 MajorInitialize and sanitize IDs before entering the error path.
At Line [29], header access can raise before IDs are safely set. Then rescue calls
publish_failure(job_id, correlation_id)with invalid values, andPublishFailureResponsemay reject them. This can drop the failure response in the exact path where it is most needed.Proposed fix
subscribe(:on_determination_requested) do |delivery_info, properties, payload| - correlation_id = properties.correlation_id - job_id = properties[:headers]['job_id'] + correlation_id = '' + job_id = '' + correlation_id = properties&.correlation_id.to_s + headers = properties.respond_to?(:headers) ? properties.headers : properties&.[](:headers) + job_id = (headers.is_a?(Hash) ? (headers['job_id'] || headers[:job_id]) : nil).to_s result = ::Fdsh::RidpRba::HandleDeterminationRequest.new.call(Also applies to: 47-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb` around lines 27 - 29, The subscriber block in subscribe(:on_determination_requested) reads properties.correlation_id and properties[:headers]['job_id'] directly which can raise before IDs are set, causing publish_failure(job_id, correlation_id) to be called with invalid values; initialize and sanitize correlation_id and job_id up-front (e.g., set correlation_id = properties&.correlation_id rescue nil and job_id = properties&.[](:headers)&.[]('job_id') rescue nil or use safe dig) so they are always defined, and ensure publish_failure and PublishFailureResponse are passed these sanitized values; apply the same fix to the analogous block around the other subscriber (lines 47-49) to avoid dropping failure responses.
🧹 Nitpick comments (1)
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb (1)
63-70: Don’t ignorePublishFailureResponseresult.
publish_failurecurrently discards the monad result. If failure publishing fails, this is silent and hard to diagnose.Proposed fix
def publish_failure(job_id, correlation_id) - ::Fdsh::Jobs::PublishFailureResponse.new.call( + result = ::Fdsh::Jobs::PublishFailureResponse.new.call( { job_id: job_id, event_name: 'events.fdsh.ridp_rba.determined', correlation_id: correlation_id } ) + logger.error("Error: failed to publish RIDP RBA failure response: #{result.failure}") if result.failure? end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb` around lines 63 - 70, The publish_failure method is currently discarding the monad result from ::Fdsh::Jobs::PublishFailureResponse.new.call; update publish_failure to capture the returned result (e.g., result = ::Fdsh::Jobs::PublishFailureResponse.new.call(...)), check result.success?/result.failure? (or equivalent monad API), and on failure either log the error details or raise/propagate the failure so it’s not silent; also return the result from publish_failure so callers can react to the outcome. Use the publish_failure method and ::Fdsh::Jobs::PublishFailureResponse symbol to locate and implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb`:
- Line 43: The error logs in DeterminationRequestedSubscriber (method
on_enroll_ridp_rba_determination_requested) leak verbose/internal data by using
result.inspect and e.inspect/full backtrace; replace those with sanitized,
minimal details: log a concise descriptive message plus non-sensitive fields
(e.g., result.to_s or result.error_message) and for exceptions log e.message and
a limited backtrace snippet (e.backtrace&.first(3) or similar) or omit backtrace
entirely; ensure no use of inspect or printing entire objects in logger.error
calls.
---
Duplicate comments:
In
`@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb`:
- Around line 27-29: The subscriber block in
subscribe(:on_determination_requested) reads properties.correlation_id and
properties[:headers]['job_id'] directly which can raise before IDs are set,
causing publish_failure(job_id, correlation_id) to be called with invalid
values; initialize and sanitize correlation_id and job_id up-front (e.g., set
correlation_id = properties&.correlation_id rescue nil and job_id =
properties&.[](:headers)&.[]('job_id') rescue nil or use safe dig) so they are
always defined, and ensure publish_failure and PublishFailureResponse are passed
these sanitized values; apply the same fix to the analogous block around the
other subscriber (lines 47-49) to avoid dropping failure responses.
---
Nitpick comments:
In
`@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb`:
- Around line 63-70: The publish_failure method is currently discarding the
monad result from ::Fdsh::Jobs::PublishFailureResponse.new.call; update
publish_failure to capture the returned result (e.g., result =
::Fdsh::Jobs::PublishFailureResponse.new.call(...)), check
result.success?/result.failure? (or equivalent monad API), and on failure either
log the error details or raise/propagate the failure so it’s not silent; also
return the result from publish_failure so callers can react to the outcome. Use
the publish_failure method and ::Fdsh::Jobs::PublishFailureResponse symbol to
locate and implement this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2b86a92-a44f-4987-ae1d-52a33038d086
📒 Files selected for processing (1)
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb
Contributor
|
Add event files for publisher and subscriber |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb (1)
56-62: Capture and log failure-response publish outcome.
publish_failurecurrently discards the operation result, so failed failure-publication attempts are invisible. Please log onfailure?for observability.Based on learnings: In `app/operations/fdsh/h36/request/clone_transaction_and_subject.rb`, publish is fire-and-forget, and individual publication failures are expected to be logged.Proposed fix
def publish_failure(job_id, correlation_id) - ::Fdsh::Jobs::PublishFailureResponse.new.call( + result = ::Fdsh::Jobs::PublishFailureResponse.new.call( { job_id: job_id, event_name: 'events.fdsh.ridp_rba.determined', correlation_id: correlation_id } ) + logger.error( + "Error: failed to publish RIDP RBA failure response " \ + "(job_id=#{job_id}, correlation_id=#{correlation_id}, reason=#{result.failure})" + ) if result.failure? + result end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb` around lines 56 - 62, The call to ::Fdsh::Jobs::PublishFailureResponse.new.call(...) currently ignores its return value; capture the result (e.g. result = ::Fdsh::Jobs::PublishFailureResponse.new.call(...)) and check result.failure? and, when true, log the failure details (use an appropriate logger in this subscriber such as Rails.logger or the existing logger) including context like job_id and correlation_id so publication failures are observable; ensure you still return/propagate the original flow after logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb`:
- Around line 22-24: Validate presence of message metadata before processing:
check properties.correlation_id and properties[:headers]['job_id'] (used to set
correlation_id and job_id) and if either is nil/invalid, immediately log and
publish a well-formed failure event that includes safe fallback identifiers (or
return/nack) and skip normal processing; also update the rescue/failure-publish
path to re-check these values before attempting to publish the failure (so it
won't attempt to publish with nil job_id/correlation_id) and ensure ack/nack
semantics are correct (do not ack/ack only after a successful failure publish).
Apply the same guard in the other occurrence around line 42.
- Around line 37-45: Replace payload-rich logging in the
:on_enroll_ridp_rba_determination_requested handling: do not log result.inspect
or the full exception/backtrace; instead log minimal identifiers (job_id and
correlation_id), the exception class and a sanitized error message (or first N
characters of message) and, if needed, at most the first few backtrace lines.
Update the logger.error calls around the result handling and rescue block (the
lines that call logger.error with result.inspect and the large
Exception/backtrace string) to use this minimal, sanitized format and ensure
publish_failure(job_id, correlation_id) remains called and
ack(delivery_info.delivery_tag) behavior is unchanged.
---
Nitpick comments:
In
`@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb`:
- Around line 56-62: The call to
::Fdsh::Jobs::PublishFailureResponse.new.call(...) currently ignores its return
value; capture the result (e.g. result =
::Fdsh::Jobs::PublishFailureResponse.new.call(...)) and check result.failure?
and, when true, log the failure details (use an appropriate logger in this
subscriber such as Rails.logger or the existing logger) including context like
job_id and correlation_id so publication failures are observable; ensure you
still return/propagate the original flow after logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfa14d37-1c47-42d1-a393-8d11e2abbf05
📒 Files selected for processing (4)
app/event_source/events/fdsh/ridp_rba/determined.rbapp/event_source/events/fdsh/ridp_rba/get_record_determined.rbapp/event_source/publishers/fdsh/ridp_rba/publisher.rbapp/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb
Show resolved
Hide resolved
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb (1)
44-45: Logpublish_failureresult when failure publishing itself fails.
publish_failurereturns an operation result, but failures are currently ignored at both call sites. Keep fire-and-forget ack behavior, but log failed publish attempts for traceability.🔍 Proposed fix
- publish_failure(job_id, correlation_id) + publish_result = publish_failure(job_id, correlation_id) + if publish_result.failure? + logger.error( + "Error: failure response publish failed for job_id: #{job_id}, " \ + "correlation_id: #{correlation_id}, reason: #{publish_result.failure}" + ) + end @@ - publish_failure(job_id, correlation_id) if job_id.is_a?(String) && correlation_id.is_a?(String) + if job_id.is_a?(String) && correlation_id.is_a?(String) + publish_result = publish_failure(job_id, correlation_id) + if publish_result.failure? + logger.error( + "Exception path: failure response publish failed for job_id: #{job_id}, " \ + "correlation_id: #{correlation_id}, reason: #{publish_result.failure}" + ) + end + endBased on learnings, fire-and-forget publishing is intentional, so this focuses on preserving that pattern while improving failure visibility.
Also applies to: 53-53, 66-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb` around lines 44 - 45, The call to publish_failure(job_id, correlation_id) is currently fire-and-forget and its result is ignored; change the call sites (the publish_failure invocations around the logger.error usages and the other occurrences at the noted sections) to capture the return value (e.g., result = publish_failure(...)) and if the result indicates failure (result.failure? / !result.success?) log the failure details via logger.error including the operation result or error message so failed publish attempts are recorded while preserving the fire-and-forget ACK behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb`:
- Around line 25-31: The guard currently only checks types, allowing empty
strings; update the validation in the on_enroll_ridp_rba_determination_requested
handling so that correlation_id and job_id are required to be non-empty strings
(e.g., is_a?(String) && !correlation_id.strip.empty? and same for job_id),
preserve the existing logger.error/ack(delivery_info.delivery_tag)/next flow,
and adjust the error message if desired to indicate missing or blank
correlation_id or job_id to make the rejection explicit.
---
Nitpick comments:
In
`@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb`:
- Around line 44-45: The call to publish_failure(job_id, correlation_id) is
currently fire-and-forget and its result is ignored; change the call sites (the
publish_failure invocations around the logger.error usages and the other
occurrences at the noted sections) to capture the return value (e.g., result =
publish_failure(...)) and if the result indicates failure (result.failure? /
!result.success?) log the failure details via logger.error including the
operation result or error message so failed publish attempts are recorded while
preserving the fire-and-forget ACK behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a395202f-5aa2-4c14-a982-83391999fbfe
📒 Files selected for processing (1)
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb (1)
56-64: Consider extracting the metadata validation to reduce duplication.The validation logic at lines 57-58 duplicates the check from lines 25-27. Extracting to a helper would improve maintainability.
Proposed helper extraction
+ def valid_ids?(job_id, correlation_id) + [job_id, correlation_id].all? { |v| v.is_a?(String) && !v.strip.empty? } + end + subscribe(:on_determination_requested) do |delivery_info, properties, payload| # ... - valid_metadata = [correlation_id, job_id].all? do |value| - value.is_a?(String) && !value.strip.empty? - end + valid_metadata = valid_ids?(job_id, correlation_id) # ... rescue StandardError => e - publish_failure(job_id, correlation_id) if job_id.is_a?(String) && !job_id.strip.empty? && - correlation_id.is_a?(String) && !correlation_id.strip.empty? + publish_failure(job_id, correlation_id) if valid_ids?(job_id, correlation_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb` around lines 56 - 64, Duplicate metadata validation for job_id and correlation_id should be extracted to a helper: add a private method (e.g., valid_metadata?(job_id, correlation_id)) that returns true only when both are Strings and not blank after strip, then replace the duplicated checks in DeterminationRequestedSubscriber (the earlier validation around lines 25-27 and the rescue block that calls publish_failure) with calls to that helper; ensure publish_failure invocation and any early-return/flow logic use the helper and that method is named/located so other methods in the class can reuse it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb`:
- Around line 21-23: The subscriber uses safe navigation when reading message
properties which is inconsistent with other determination request subscribers;
update the property access in correlation_id, headers and job_id extraction to
use direct property access (e.g., use properties.correlation_id and
properties[:headers] instead of properties&.correlation_id and
properties&.[](:headers)) so that correlation_id, headers and job_id are
obtained the same way as in other subscribers (leave the existing job_id
fallback logic using headers['job_id'] || headers[:job_id] intact).
---
Nitpick comments:
In
`@app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb`:
- Around line 56-64: Duplicate metadata validation for job_id and correlation_id
should be extracted to a helper: add a private method (e.g.,
valid_metadata?(job_id, correlation_id)) that returns true only when both are
Strings and not blank after strip, then replace the duplicated checks in
DeterminationRequestedSubscriber (the earlier validation around lines 25-27 and
the rescue block that calls publish_failure) with calls to that helper; ensure
publish_failure invocation and any early-return/flow logic use the helper and
that method is named/located so other methods in the class can reuse it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ea31838-c78c-4530-9508-5197218b05c9
📒 Files selected for processing (1)
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb
app/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb
Outdated
Show resolved
Hide resolved
…sted_subscriber.rb Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Jay Reddy <jayanth.rordev@gmail.com>
Contributor
Author
|
Updated changes requested by Hari. Ready to review. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces an AMQP-based event system for FDSH RIDP RBA determination workflows, including a subscriber that processes determination_requested events, invokes a handler, and publishes responses. Adds event classes for determination outcomes and a publisher to route events through the system. Changes
Sequence DiagramsequenceDiagram
actor Queue as AMQP Queue
participant Sub as Determination<br/>Requested Subscriber
participant Handler as Handle<br/>Determination Request
participant Pub as RIDP RBA<br/>Publisher
Queue->>Sub: delivery_info, properties, payload
Sub->>Sub: Extract correlation_id & job_id
Sub->>Sub: Validate metadata
alt Validation Failed
Sub->>Sub: Log error
Sub->>Queue: Acknowledge
else Validation Success
Sub->>Handler: call(correlation_id, job_id, payload)
alt Handler Success
Handler-->>Sub: success?
Sub->>Sub: Log success
else Handler Failure
Handler-->>Sub: failure response
Sub->>Pub: publish_failure(job_id, correlation_id)
Pub->>Queue: Publish failure event
Sub->>Sub: Log error with IDs
end
Sub->>Queue: Acknowledge
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
geetha-1z
approved these changes
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://app.clickup.com/t/868htgrzy
This pull request introduces new publisher and subscriber classes to support the RIDP RBA determination workflow using event-driven architecture. The main changes add AMQP-based event publishing and subscribing for RIDP RBA determination requests and responses.
Event Source Integration for RIDP RBA:
RidpRbaPublisherclass toapp/event_source/publishers/fdsh/ridp_rba/ridp_rba_publisher.rb, enabling the publishing ofdeterminedandget_record_determinedevents on thefdsh.ridp_rbaAMQP exchange.DeterminationRequestedSubscriberclass toapp/event_source/subscribers/fdsh/ridp_rba/determination_requested_subscriber.rb, allowing the system to subscribe toon_determination_requestedevents from theenroll.ridp_rbaAMQP exchange, handle determination requests, and publish failure responses if needed.Summary by CodeRabbit
Release Notes