Conversation
This commit refactors the dependencies and adds domain logic for the subscriber module. This includes the creation of a `Subscriber` struct with `email` and `name` fields. It also introduces email and name validation using the `validator` crate. Additionally, the commit updates the `Cargo.toml` file to include the necessary dependencies, such as `validator`, `claims`, `unicode-segmentation`, and `fake`. The `src/domain` and `src/domain/subscriber` modules have been created to provide a structured organization for the domain logic.
This commit refactors the email validation to provide more information and adds a check to the insert query for subscriptions.
WalkthroughAdds a new domain layer with validated Subscriber, SubscriberEmail, and SubscriberName types; wires domain into lib. Updates handler to convert FormData into Subscriber with validation. Replaces model’s create_subscription with create_subscriber. Adds centralized validation error handling (HTTP 400). Expands tests. Introduces new deps for validation and testing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant H as HTTP Handler (subscribe)
participant D as Domain (TryFrom<FormData>)
participant M as ModelManager
participant DB as Database
C->>H: POST /subscriptions (FormData)
H->>D: TryFrom<FormData> -> Subscriber
alt Validation OK
D-->>H: Ok(Subscriber)
H->>M: create_subscriber(Subscriber)
M->>DB: INSERT subscriber(name, email)
DB-->>M: Result
M-->>H: Ok / Error
alt DB Ok
H-->>C: 200 OK
else DB Error
H-->>C: 500 Internal Server Error
end
else Validation Error
D-->>H: Err(ValidationErrors)
H-->>C: 400 Bad Request
end
note over D,H: Domain enforces email/name rules before DB write
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
Cargo.toml (1)
11-11: Moveclaimsto dev-dependencies to avoid pulling it in for non-test builds.
claimsis only used in tests; keep prod dependency graph lean.Apply this diff:
[dependencies] axum = { version = "0.8.4", features = ["tracing"] } -claims = "0.8.0" config = "0.15.14" dotenvy = "0.15.7" @@ [dev-dependencies] http-body-util = "0.1.3" mime = "0.3.17" +claims = "0.8.0" fake = "4.4.0" proptest = "1.7.0"Also applies to: 28-33
src/domain/subscriber/name.rs (2)
4-5: Derive common traits for ergonomics.Add
Clone,PartialEq, andEqto ease comparisons, cloning in tests, and struct reuse.Apply this diff:
-#[derive(Debug)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct SubscriberName(String);
29-35: Also reject ASCII control characters (e.g., newlines, tabs).Currently only a small forbidden set is blocked; control chars can slip through.
Apply this diff:
let forbidden_characters = ['/', '(', ')', '"', '<', '>', '\\', '{', '}']; let contains_forbidden_characters = value.chars().any(|g| forbidden_characters.contains(&g)); if contains_forbidden_characters { return Err(ValidationError::new("INVALID_SUBSCRIBER_NAME") .with_message("subscriber name contains forbidden characters".into())); } + + let contains_control = value.chars().any(|c| c.is_control()); + if contains_control { + return Err(ValidationError::new("INVALID_SUBSCRIBER_NAME") + .with_message("subscriber name contains control characters".into())); + }src/domain/subscriber/email.rs (2)
3-5: Derive common traits for convenience.Consistent with
SubscriberName, deriveClone,PartialEq, andEq.Apply this diff:
-#[derive(Debug)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct SubscriberEmail(String);
60-61: Remove noisydbg!in property test.Avoid cluttering test output for passing cases.
Apply this diff:
- dbg!(&email); - assert_ok!(SubscriberEmail::try_from(email));src/domain/mod.rs (1)
3-3: Optionally re-export the validated wrappers for a flatter API surface.Exposing
SubscriberEmailandSubscriberNameatdomain::*can simplify downstream imports.Apply these diffs:
- pub use subscriber::Subscriber; + pub use subscriber::{Subscriber, SubscriberEmail, SubscriberName};And in
src/domain/subscriber/mod.rsadd:+pub use email::SubscriberEmail; +pub use name::SubscriberName;src/domain/subscriber/mod.rs (1)
37-44: Nit: redundanterrors.is_empty()in theif letchain.If both
nameareSome, there can’t be collected errors. Simplify for clarity.Apply this diff if you keep the current impl:
- if let Some(email) = email - && let Some(name) = name - && errors.is_empty() - { + if let (Some(email), Some(name)) = (email, name) { Ok(Self { email, name }) } else { Err(errors) }tests/tests.rs (1)
140-163: Consider a couple more negative cases to harden validation.
- Name >256 graphemes.
- Name with forbidden chars (e.g.,
<script>).- Email with leading/trailing spaces (should be invalid unless trimmed).
I can add parameterized cases for these.
src/model.rs (2)
29-31: Minor: avoid intermediateStringallocations if Surreal accepts&str.If the client’s
.bindsupports&str, passas_ref()directly.Apply if supported:
- .bind(("name", subscriber.name.as_ref().to_string())) - .bind(("email", subscriber.email.as_ref().to_string())) + .bind(("name", subscriber.name.as_ref())) + .bind(("email", subscriber.email.as_ref()))
25-35: Enforce uniqueness on email at the DB level.Prevent duplicate subscriptions with a UNIQUE index.
Add a migration:
DEFINE INDEX uniq_subscriptions_email ON TABLE subscriptions FIELDS email UNIQUE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(2 hunks)src/domain/mod.rs(1 hunks)src/domain/subscriber/email.rs(1 hunks)src/domain/subscriber/mod.rs(1 hunks)src/domain/subscriber/name.rs(1 hunks)src/errors.rs(1 hunks)src/handlers.rs(1 hunks)src/lib.rs(1 hunks)src/model.rs(2 hunks)tests/tests.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/domain/subscriber/mod.rs (2)
src/domain/subscriber/email.rs (1)
try_from(15-21)src/domain/subscriber/name.rs (1)
try_from(16-38)
src/domain/subscriber/name.rs (2)
src/domain/subscriber/email.rs (3)
as_ref(7-9)try_from(15-21)empty_string_is_rejected(34-37)src/domain/subscriber/mod.rs (1)
try_from(18-45)
src/domain/subscriber/email.rs (2)
src/domain/subscriber/name.rs (3)
as_ref(8-10)try_from(16-38)empty_string_is_rejected(65-68)src/domain/subscriber/mod.rs (1)
try_from(18-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check
🔇 Additional comments (7)
src/lib.rs (1)
2-2: Makedomainmodulepub(crate)
Privatedomainblocks sibling modules (e.g.domain::Subscriberin src/model.rs).-mod domain; +pub(crate) mod domain;Likely an incorrect or invalid review comment.
Cargo.toml (1)
24-27: Deps look appropriate for the new domain validation.
unicode-segmentationandvalidatoralign with the added name/email validators.src/errors.rs (1)
17-19: Good: addValidationErrorvariant to map validator errors.The conversion via
#[from]is correct and keeps call sites clean.src/domain/subscriber/mod.rs (1)
37-39: Verify MSRV for let-chains
No explicitrust-versionin Cargo.toml orrust-toolchainfile; ensure your MSRV is ≥ 1.55 to supportif let … && let …, or refactor to a tuple match.tests/tests.rs (1)
112-138: Good matrix: 422 for missing fields, 400 for invalid/empty.Covers key cases and matches extractor vs domain validation boundaries.
src/handlers.rs (1)
18-18: No action needed – validation errors already map to HTTP 400. TheValidationError(#[from] validator::ValidationErrors)variant insrc/errors.rsis handled in theIntoResponseimpl to returnStatusCode::BAD_REQUEST.src/model.rs (1)
25-35: LGTM: switched to domain type, added.check().Clean hand-off from handler to model using validated
domain::Subscriber.
1 participant
Summary by CodeRabbit