refactor(gateway): extract source plugin trait and resolve_common()#161
refactor(gateway): extract source plugin trait and resolve_common()#161enilsen16 wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR implements a gateway-internal plugin abstraction to replace per-source conditional nesting and duplicated NATS field validation. A new ChangesPlugin System Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
PR SummaryMedium Risk Overview Extracts repeated NATS-related config parsing/validation ( Reviewed by Cursor Bugbot for commit dc07fc2. Bugbot is set up for automated code reviews on this repo. Configure here. |
…dation Eliminates ~400 lines of duplication across http.rs, streams.rs, and the 10 resolve_* functions in config.rs by introducing: - A gateway-internal SourcePlugin trait with enum dispatch (unit structs + monomorphised entry points, zero vtables) replacing the 9-arm if-let chains in http.rs and streams.rs. - A shared resolve_common() helper in config.rs extracting the four repeated validation blocks (subject_prefix, stream_name, nats_ack_timeout, stream_max_age) that appeared in every per-source resolver. - Nine thin wrapper impls (GithubPlugin, SlackPlugin, etc.) that delegate to the existing source crate routers and provision fns. No source crate code or dependency changes needed. Discord is intentionally excluded from SourcePlugin: its webhook-less primary path is handled by the gateway runner spawned in main.rs, which stays untouched. All 606+ workspace tests pass unchanged.
enilsen16
force-pushed
the
plugin-system-dev
branch
from
a9ebf3e to
dc07fc2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-gateway/src/source_plugin.rs (1)
435-435: 💤 Low valueRemove unnecessary clone on the final mount_one call.
The last call to
mount_oneclonespublisher, but since this is the final use, you can move it instead. This avoids an unnecessary clone operation.♻️ Proposed fix
- mount_one(&SentryPlugin, router, publisher.clone(), resolved) + mount_one(&SentryPlugin, router, publisher, resolved)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-gateway/src/source_plugin.rs` at line 435, The final call to mount_one(&SentryPlugin, router, publisher.clone(), resolved) performs an unnecessary clone of publisher; change this call to move the publisher (mount_one(&SentryPlugin, router, publisher, resolved)) since this is the last use—ensure no subsequent code expects publisher afterwards, and if earlier mounts needed clones keep those as-is (only remove .clone() on the final mount_one invocation).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 481-531: The resolve_common function bundles four distinct
conversions (subject_prefix via NatsToken::new, stream_name via NatsToken::new,
nats_ack_timeout via NonZeroDuration::from_secs, and stream_max_age via
StreamMaxAge::from_secs) and should be split into small per-field resolver
functions (e.g., resolve_subject_prefix, resolve_stream_name,
resolve_nats_ack_timeout, resolve_stream_max_age) that each return Result<... ,
ConfigValidationError> or Option<...> and push errors into the shared errors
vec; then have resolve_common (or the callers of ResolvedCommon) compose those
per-field resolvers to construct ResolvedCommon so validation is isolated by
type and easier to test/maintain.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-gateway/src/source_plugin.rs`:
- Line 435: The final call to mount_one(&SentryPlugin, router,
publisher.clone(), resolved) performs an unnecessary clone of publisher; change
this call to move the publisher (mount_one(&SentryPlugin, router, publisher,
resolved)) since this is the last use—ensure no subsequent code expects
publisher afterwards, and if earlier mounts needed clones keep those as-is (only
remove .clone() on the final mount_one invocation).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d13776bb-abb5-4e2f-b89b-a838d2651d70
📒 Files selected for processing (6)
PLUGIN_SYSTEM_PLAN.mdrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/http.rsrsworkspace/crates/trogon-gateway/src/main.rsrsworkspace/crates/trogon-gateway/src/source_plugin.rsrsworkspace/crates/trogon-gateway/src/streams.rs
| fn resolve_common( | ||
| source: &'static str, | ||
| subject_prefix: String, | ||
| stream_name: String, | ||
| nats_ack_timeout_secs: u64, | ||
| stream_max_age_secs: u64, | ||
| errors: &mut Vec<ConfigValidationError>, | ||
| ) -> Option<ResolvedCommon> { | ||
| let subject_prefix = match NatsToken::new(subject_prefix) { | ||
| Ok(t) => t, | ||
| Err(e) => { | ||
| errors.push(ConfigValidationError::invalid_subject_token( | ||
| "github", | ||
| source, | ||
| "subject_prefix", | ||
| e, | ||
| )); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| let stream_name = match NatsToken::new(section.stream_name) { | ||
| let stream_name = match NatsToken::new(stream_name) { | ||
| Ok(t) => t, | ||
| Err(e) => { | ||
| errors.push(ConfigValidationError::invalid_subject_token("github", "stream_name", e)); | ||
| errors.push(ConfigValidationError::invalid_subject_token(source, "stream_name", e)); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| let nats_ack_timeout = match NonZeroDuration::from_secs(section.nats_ack_timeout_secs) { | ||
| let nats_ack_timeout = match NonZeroDuration::from_secs(nats_ack_timeout_secs) { | ||
| Ok(d) => d, | ||
| Err(err) => { | ||
| errors.push(ConfigValidationError::invalid("github", "nats_ack_timeout_secs", err)); | ||
| errors.push(ConfigValidationError::invalid(source, "nats_ack_timeout_secs", err)); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| let stream_max_age = match StreamMaxAge::from_secs(section.stream_max_age_secs) { | ||
| let stream_max_age = match StreamMaxAge::from_secs(stream_max_age_secs) { | ||
| Ok(age) => age, | ||
| Err(err) => { | ||
| errors.push(ConfigValidationError::invalid("github", "stream_max_age_secs", err)); | ||
| errors.push(ConfigValidationError::invalid(source, "stream_max_age_secs", err)); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| Some(trogon_source_github::GithubConfig { | ||
| webhook_secret, | ||
| Some(ResolvedCommon { | ||
| subject_prefix, | ||
| stream_name, | ||
| stream_max_age, | ||
| nats_ack_timeout, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Split resolve_common into per-field resolvers to keep validation boundaries type-focused.
This helper bundles four unrelated validations/conversions into one aggregate resolver. Consider small per-field resolvers (or per-type constructors at module boundaries) and compose them in each resolve_* function to keep type validation isolated.
As per coding guidelines, "Validate per-type, not per-aggregate: avoid validating unrelated fields together in a single constructor."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-gateway/src/config.rs` around lines 481 - 531, The
resolve_common function bundles four distinct conversions (subject_prefix via
NatsToken::new, stream_name via NatsToken::new, nats_ack_timeout via
NonZeroDuration::from_secs, and stream_max_age via StreamMaxAge::from_secs) and
should be split into small per-field resolver functions (e.g.,
resolve_subject_prefix, resolve_stream_name, resolve_nats_ack_timeout,
resolve_stream_max_age) that each return Result<... , ConfigValidationError> or
Option<...> and push errors into the shared errors vec; then have resolve_common
(or the callers of ResolvedCommon) compose those per-field resolvers to
construct ResolvedCommon so validation is isolated by type and easier to
test/maintain.
1 participant
refactor(gateway): extract source plugin trait and common config validation
Eliminates ~400 lines of duplication across http.rs, streams.rs, and
the 10 resolve_* functions in config.rs by introducing:
A gateway-internal SourcePlugin trait with enum dispatch (unit
structs + monomorphised entry points, zero vtables) replacing
the 9-arm if-let chains in http.rs and streams.rs.
A shared resolve_common() helper in config.rs extracting the
four repeated validation blocks (subject_prefix, stream_name,
nats_ack_timeout, stream_max_age) that appeared in every
per-source resolver.
Nine thin wrapper impls (GithubPlugin, SlackPlugin, etc.) that
delegate to the existing source crate routers and provision
fns. No source crate code or dependency changes needed.
Discord is intentionally excluded from SourcePlugin: its
webhook-less primary path is handled by the gateway runner
spawned in main.rs, which stays untouched.
All 606+ workspace tests pass unchanged.