Conversation
WalkthroughIntroduces email client support and confirmation token flow for subscriptions; restructures handlers; extends configuration (base URLs, email client settings); updates database schema (subscription_tokens table, status/token fields); adjusts startup/state ownership and wiring; adds release profile and dependencies; adds migrations task; updates CI env; adds comprehensive integration tests; replaces/renames config fields. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router
participant SubscriptionHandler as Subscription Handler
participant Model as ModelManager/DB
participant EmailClient
participant MailService as Email Service
Note over User,Router: Subscribe flow
User->>Router: POST /subscriptions (name, email)
Router->>SubscriptionHandler: subscribe(FormData)
SubscriptionHandler->>SubscriptionHandler: generate token
SubscriptionHandler->>Model: create_subscriber(&Subscriber, token)
Model->>Model: begin transaction
Model->>Model: INSERT subscription_tokens(token)
Model->>Model: INSERT subscriptions(email,name,token_ref,status=PENDING)
Model-->>SubscriptionHandler: ok
SubscriptionHandler->>EmailClient: send_email(to, subject, html/text with confirm link)
EmailClient->>MailService: POST / (JSON, auth header)
MailService-->>EmailClient: 200 OK
EmailClient-->>SubscriptionHandler: ok
SubscriptionHandler-->>Router: 201 Created
Router-->>User: 201
Note over User,Router: Confirm flow
User->>Router: GET /subscriptions/confirm?token=...
Router->>SubscriptionHandler: confirm(Query{token})
SubscriptionHandler->>Model: UPDATE subscriptions SET status=CONFIRMED WHERE token matches AND status=PENDING
Model-->>SubscriptionHandler: ok
SubscriptionHandler-->>Router: 200 OK
Router-->>User: 200
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/cd.yml (2)
23-25: Fix missing step ID for Buildx output referenceYou reference steps.setup-buildx.outputs.name but the Buildx step has no id, so the variable will be empty at runtime.
Apply:
- - name: Set up Docker Buildx + - name: Set up Docker Buildx + id: setup-buildx uses: docker/setup-buildx-action@v3Also applies to: 35-37
63-66: Set an ID on the Cloud Run deploy stepsteps.deploy.outputs.url is used later, but the deploy step lacks id: deploy.
Apply:
- - name: Deploy to Google Cloud Run + - name: Deploy to Google Cloud Run + id: deploy uses: google-github-actions/deploy-cloudrun@v2Also applies to: 84-85
🧹 Nitpick comments (33)
.github/workflows/cd.yml (1)
9-19: Avoid overlapping deploymentsOptional: add a concurrency group to serialize deploys on main.
Apply:
jobs: deploy: runs-on: ubuntu-latest environment: production + concurrency: + group: cloudrun-${{ github.ref_name }} + cancel-in-progress: falsesrc/domain/subscriber/email.rs (2)
14-24: Add TryFrom<&str> to avoid temporary allocationsHandy for call sites holding &str.
Apply:
impl TryFrom<String> for SubscriberEmail { @@ } + +impl TryFrom<&str> for SubscriberEmail { + type Error = validator::ValidationError; + fn try_from(value: &str) -> Result<Self, Self::Error> { + Self::try_from(value.to_owned()) + } +}
59-64: Add serde-path testsAdd tests asserting that deserializing an invalid email into SubscriberEmail fails (ensures the above serde wiring stays enforced).
Example (new test):
#[test] fn serde_rejects_invalid_email() { #[derive(Deserialize)] struct T { email: SubscriberEmail } let json = r#"{ "email": "@domain.com" }"#; let err = serde_json::from_str::<T>(json).unwrap_err(); assert!(err.is_data()); }.moon/tasks/rust-backend-application.yml (1)
8-13: Ensure surrealdb-migrations CLI is available in PATHThis task will fail if the CLI isn’t installed on CI/dev machines. Either install it in CI or invoke it via cargo if it’s in your workspace.
Option A (install in CI before running): cargo install surrealdb-migrations-cli (or your chosen distribution).
Option B (invoke via cargo if available as a bin in the workspace):- migrations: - command: surrealdb-migrations + migrations: + command: cargo run -q --bin surrealdb-migrations -- description: An awesome CLI for SurrealDB migrations (provides commands to scaffold, create and apply migrations) local: true options: envFile: .envschemas/subscription_tokens.surql (2)
10-10: Use consistent ON TABLE syntax for index (minor)For consistency with fields, prefer ON TABLE.
Apply:
-DEFINE INDEX OVERWRITE unique_token ON subscription_tokens COLUMNS token UNIQUE; +DEFINE INDEX OVERWRITE unique_token ON TABLE subscription_tokens COLUMNS token UNIQUE;
6-7: Optional: plan for token TTL/cleanupIf tokens should expire, consider an index to support pruning and a scheduled cleanup (or time-based assertions).
src/errors.rs (2)
18-25: Good variant additions; prefer Display over Debug for String to avoid quoted logsUsing
{0:?}onCustom(String)prints quotes/escapes in logs. Switch to{0}for cleaner, consistent formatting.- #[error("{0:?}")] - Custom(String), + #[error("{0}")] + Custom(String),
31-38: Return a helpful body for 400 validation errors (matches your earlier preference to surface the offending email); also tidy log textRight now 400s have an empty body. If you want clients/tests to see the validation message (incl. the email value), return it as the response body. Also fix the log phrasing.
- Self::ValidationErrors(_) | Self::ValidationError(_) => { - tracing::info!("Bad request: - {self:?}"); - StatusCode::BAD_REQUEST.into_response() - } - _ => { - tracing::error!("Internal Server: - {self:?}"); - StatusCode::INTERNAL_SERVER_ERROR.into_response() - } + Self::ValidationErrors(e) => { + tracing::info!("Bad request - {e:?}"); + (StatusCode::BAD_REQUEST, e.to_string()).into_response() + } + Self::ValidationError(e) => { + tracing::info!("Bad request - {e:?}"); + (StatusCode::BAD_REQUEST, e.to_string()).into_response() + } + _ => { + tracing::error!("Internal server error - {self:?}"); + StatusCode::INTERNAL_SERVER_ERROR.into_response() + }If you prefer JSON, I can wire
(StatusCode::BAD_REQUEST, axum::Json(...))instead.src/domain/subscriber/mod.rs (1)
16-46: Decouple domain from handlers (avoid TryFrom in domain).Having domain depend on crate::handlers is an unnecessary layering coupling. Prefer a domain-level conversion (e.g., from raw strings) and keep FormData-to-domain mapping in the handlers layer.
Apply this refactor within this file:
- use crate::handlers::FormData; +// Keep domain independent from the transport layer -impl TryFrom<FormData> for Subscriber { +impl TryFrom<(String, String)> for Subscriber { type Error = validator::ValidationErrors; - fn try_from(value: FormData) -> Result<Self, Self::Error> { + fn try_from((email_raw, name_raw): (String, String)) -> Result<Self, Self::Error> { let mut errors = ValidationErrors::new(); - let email: Option<SubscriberEmail> = match value.email.try_into() { + let email: Option<SubscriberEmail> = match email_raw.try_into() { Ok(email) => Some(email), Err(error) => { errors.add("email", error); None } }; - let name: Option<SubscriberName> = match value.name.try_into() { + let name: Option<SubscriberName> = match name_raw.try_into() { Ok(name) => Some(name), Err(error) => { errors.add("name", error); None } };Then, in handlers, construct with
Subscriber::try_from((form.email, form.name)). This retains your detailed validation errors (per prior preference to include the actual email in messages) while fixing the dependency direction.schemas/subscriptions.surql (2)
9-9: Consider making token explicitly optional.Unless every new row sets token at creation time, model this as optional to avoid accidental type errors during inserts.
-DEFINE FIELD OVERWRITE token ON subscriptions TYPE record<subscription_tokens>; +DEFINE FIELD OVERWRITE token ON subscriptions TYPE option<record<subscription_tokens>>;
6-11: Minor style consistency: use “ON TABLE” uniformly.Cosmetic only; improves readability.
-DEFINE FIELD OVERWRITE email ON subscriptions TYPE string ASSERT string::is::email($value); -DEFINE FIELD OVERWRITE name ON subscriptions TYPE string; -DEFINE FIELD OVERWRITE status ON subscriptions TYPE 'PENDING' | 'CONFIRMED' DEFAULT 'PENDING'; -DEFINE FIELD OVERWRITE token ON subscriptions TYPE record<subscription_tokens>; +DEFINE FIELD OVERWRITE email ON TABLE subscriptions TYPE string ASSERT string::is::email($value); +DEFINE FIELD OVERWRITE name ON TABLE subscriptions TYPE string; +DEFINE FIELD OVERWRITE status ON TABLE subscriptions TYPE 'PENDING' | 'CONFIRMED' DEFAULT 'PENDING'; +DEFINE FIELD OVERWRITE token ON TABLE subscriptions TYPE record<subscription_tokens>;.env.example (1)
6-6: Keep .env.example in sync: order keys and add TIMEOUT for email client.Reorder to satisfy dotenv-linter and include the missing timeout to mirror code/tests.
Apply:
# Subscriptions Application SUBSCRIPTIONS__PORT=1337 -SUBSCRIPTIONS__HOST=0.0.0.0 -SUBSCRIPTIONS__BASE_URL=http://localhost:1337 +SUBSCRIPTIONS__BASE_URL=http://localhost:1337 +SUBSCRIPTIONS__HOST=0.0.0.0 # Subscriptions Database -SUBSCRIPTIONS__DATABASE__USERNAME=subscriptions -SUBSCRIPTIONS__DATABASE__PASSWORD=password -SUBSCRIPTIONS__DATABASE__NAMESPACE=main -SUBSCRIPTIONS__DATABASE__NAME=db +SUBSCRIPTIONS__DATABASE__NAME=db +SUBSCRIPTIONS__DATABASE__NAMESPACE=main +SUBSCRIPTIONS__DATABASE__PASSWORD=password +SUBSCRIPTIONS__DATABASE__USERNAME=subscriptions # Subscriptions Email Client -SUBSCRIPTIONS__EMAIL_CLIENT__SENDER_EMAIL= -SUBSCRIPTIONS__EMAIL_CLIENT__BASE_URL= -SUBSCRIPTIONS__EMAIL_CLIENT__AUTH_TOKEN= +SUBSCRIPTIONS__EMAIL_CLIENT__AUTH_TOKEN= +SUBSCRIPTIONS__EMAIL_CLIENT__BASE_URL= +SUBSCRIPTIONS__EMAIL_CLIENT__SENDER_EMAIL= +SUBSCRIPTIONS__EMAIL_CLIENT__TIMEOUT=10sAlso applies to: 9-13, 15-19
src/handlers/health_check.rs (1)
4-4: Prefer http::StatusCode over reqwest::StatusCode in handlers.Avoid coupling the handler to reqwest; use Axum/http’s StatusCode.
-use reqwest::StatusCode; +use axum::http::StatusCode;src/main.rs (1)
24-29: Avoid cloning Config; capture host/port before moving it into init.Small perf/readability win; no behavior change.
- let (router, _) = subscriptions::init(config.clone()) + let host = config.host.clone(); + let port = config.port; + let (router, _) = subscriptions::init(config) .await .unwrap_or_else(|error| { tracing::error!(%error, "Failed to initialize application"); process::exit(1); }); // Bind address - let listener = TcpListener::bind((config.host.clone(), config.port)) + let listener = TcpListener::bind((host.clone(), port)) .await .unwrap_or_else(|error| { - tracing::error!(%error, "Failed to bind address `{}:{}`", config.host, config.port); + tracing::error!(%error, "Failed to bind address `{}:{}`", host, port); process::exit(1); }); // Start server - tracing::info!("Start listening on: http://{}:{}", config.host, config.port); + tracing::info!("Start listening on: http://{}:{}", host, port);Also applies to: 31-41
.env.test (1)
1-1: Fix lint issues and a typo; quote values and order keys.Quotes prevent parser surprises; ordering silences dotenv-linter; fix “overided” → “overridden”.
-RUST_LOG=debug # https://docs.rs/env_logger/latest/env_logger/#enabling-logging +RUST_LOG="debug" # https://docs.rs/env_logger/latest/env_logger/#enabling-logging # Subscriptions Application -SUBSCRIPTIONS__PORT=1337 # not used by tests -SUBSCRIPTIONS__HOST=0.0.0.0 # not used by tests -SUBSCRIPTIONS__BASE_URL=http://localhost:1337 # not used by tests +SUBSCRIPTIONS__PORT="1337" # not used by tests +SUBSCRIPTIONS__HOST="0.0.0.0" # not used by tests +SUBSCRIPTIONS__BASE_URL="http://localhost:1337" # not used by tests # Subscriptions Database SUBSCRIPTIONS__DATABASE__BASE_URL=mem:// -SUBSCRIPTIONS__DATABASE__USERNAME=subscriptions -SUBSCRIPTIONS__DATABASE__PASSWORD=password -SUBSCRIPTIONS__DATABASE__NAMESPACE=main -SUBSCRIPTIONS__DATABASE__NAME=db +SUBSCRIPTIONS__DATABASE__NAME=db +SUBSCRIPTIONS__DATABASE__NAMESPACE=main +SUBSCRIPTIONS__DATABASE__PASSWORD=password +SUBSCRIPTIONS__DATABASE__USERNAME=subscriptions # Subscriptions Email Client -SUBSCRIPTIONS__EMAIL_CLIENT__SENDER_EMAIL=admin@example.com -SUBSCRIPTIONS__EMAIL_CLIENT__BASE_URL=http://example.com/path # overided with mocked url by tests -SUBSCRIPTIONS__EMAIL_CLIENT__AUTH_TOKEN=token -SUBSCRIPTIONS__EMAIL_CLIENT__TIMEOUT=1s +SUBSCRIPTIONS__EMAIL_CLIENT__AUTH_TOKEN="token" +SUBSCRIPTIONS__EMAIL_CLIENT__BASE_URL="http://example.com/path" # overridden with mocked url by tests +SUBSCRIPTIONS__EMAIL_CLIENT__SENDER_EMAIL="admin@example.com" +SUBSCRIPTIONS__EMAIL_CLIENT__TIMEOUT="1s"Also applies to: 4-6, 10-13, 16-19
tests/api/subscriptions_confirm.rs (2)
20-21: Consistency: terminate the assert with a semicolon.Keep style consistent with the rest of the file.
- assert_eq!(response.status_code(), StatusCode::BAD_REQUEST) + assert_eq!(response.status_code(), StatusCode::BAD_REQUEST);
39-39: Preferexpectoverunwrapand avoid hard indexing.Improves failure messages and avoids panics on empty collections.
- let email_request = &app.email_server.received_requests().await.unwrap()[0]; + let email_request = app + .email_server + .received_requests() + .await + .expect("failed to fetch received requests") + .first() + .expect("email request was not captured");src/config.rs (1)
30-30: UnifySecretStringimport usage.You import
SecretStringalready—use it consistently.- pub password: secrecy::SecretString, + pub password: SecretString,src/state.rs (1)
17-23:asyncnot needed onAppState::new.No awaits inside; make it sync to simplify call sites.
-impl AppState { - pub async fn new(config: Config) -> Result<Self> { +impl AppState { + pub fn new(config: Config) -> Result<Self> { Ok(Self { mm: Arc::new(model::ModelManager::new(config.database.clone())), email_client: Arc::new(EmailClient::new(config.email_client.clone())?), config: Arc::new(config), }) } }Follow-up: update call sites removing
.await.src/model.rs (3)
33-44: Avoid unnecessary String allocations in binds.You can bind &str directly; no need for to_string() on hot paths.
- .bind(("token_val", token.to_string())) - .bind(("email", subscriber.email.as_ref().to_string())) - .bind(("name", subscriber.name.as_ref().to_string())) + .bind(("token_val", token)) + .bind(("email", subscriber.email.as_ref())) + .bind(("name", subscriber.name.as_ref()))Also applies to: 45-46
35-41: Consider returning nothing from the transactional query.If you don't consume results, append RETURN NONE to reduce payload/decoding.
CREATE subscriptions CONTENT { email: $email, name: $name, token: $subscription_token.id }; - COMMIT TRANSACTION; + COMMIT TRANSACTION; + RETURN NONE;
57-61: Quote username in mem bootstrap to avoid identifier issues.Backtick-quote to handle non-alphanumeric usernames in DEFINE USER.
- .query(format!( - "DEFINE USER {} ON DATABASE PASSWORD '{}' ROLES OWNER", - config.username, - config.password.expose_secret() - )) + .query(format!( + "DEFINE USER `{}` ON DATABASE PASSWORD '{}' ROLES OWNER", + config.username, + config.password.expose_secret() + ))Also applies to: 60-70
migrations/definitions/_initial.json (1)
1-1: Add minimal token hardness constraint.Prevent empty/short tokens to reduce accidental duplicates before hashing/encoding.
-DEFINE FIELD OVERWRITE token ON subscription_tokens TYPE string; +DEFINE FIELD OVERWRITE token ON subscription_tokens TYPE string ASSERT string::len($value) > 15;tests/api/subscriptions.rs (2)
145-146: Typos in assertion comment.Clarify comment; no behavior change.
- // the two link should be adentical + // The two links should be identical
150-150: Test name/status mismatch.Name says 200 but you assert 201. Rename for clarity or change expected status.
-pub async fn subscribe_returns_a_200_for_valid_form_data() { +pub async fn subscribe_returns_a_201_for_valid_form_data() {Also applies to: 167-167
src/email_client.rs (4)
36-36: Fix typo: renamerecipeinttorecipient.Keeps naming consistent and prevents future confusion.
- recipeint: &SubscriberEmail, + recipient: &SubscriberEmail, ... - to: recipeint.as_ref(), + to: recipient.as_ref(),Also applies to: 43-43
109-116: Ensure header matcher uses a string name.
wiremock::matchers::header_existsexpects a string; passas_str()to avoid type mismatch.- Mock::given(header_exists(EMAIL_CLIENT_AUTH_HEADER)) + Mock::given(header_exists(EMAIL_CLIENT_AUTH_HEADER.as_str())) .and(header(CONTENT_TYPE, mime::APPLICATION_JSON.as_ref())) .and(method(Method::POST)) .and(SendEmailBodyMatcher)
170-170: Fix test name typo.“too long”, not “to long”.
- async fn send_email_timeout_if_the_server_takes_to_long() { + async fn send_email_timeouts_if_the_server_takes_too_long() {
211-211: Remove redundant.into().
Faker.fake::<String>()already returnsString.- auth_token: SecretString::new(Faker.fake::<String>().into()), + auth_token: SecretString::new(Faker.fake::<String>()),tests/api/helpers.rs (1)
59-65: Guard against missing links in tests.Add an assertion before indexing to make failures clearer.
- let raw_link = links[0].as_str().to_owned(); + assert!( + !links.is_empty(), + "Expected at least one URL in the email body, found none" + ); + let raw_link = links[0].as_str().to_owned();src/handlers/subscription.rs (3)
19-25: Avoid logging PII in spans.Skip
formin theinstrumentmacro to avoid capturing user email/name.-#[tracing::instrument(skip(mm, config, email_client))] +#[tracing::instrument(skip(mm, config, email_client, form))]
9-9: Usehttp::StatusCodeinstead of pulling it fromreqwest.Reduces coupling between handlers and the HTTP client crate.
-use reqwest::StatusCode; +use axum::http::StatusCode;
13-17: Restrict visibility ofFormDatafields.These don’t need to be
pubfor extractors; keep them private unless used externally.#[derive(Debug, Deserialize)] pub struct FormData { - pub email: String, - pub name: String, + email: String, + name: String, }
📜 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 (29)
.env.example(1 hunks).env.test(1 hunks).github/workflows/cd.yml(1 hunks).moon/tasks/rust-backend-application.yml(1 hunks)Cargo.toml(1 hunks)migrations/definitions/_initial.json(1 hunks)schemas/subscription_tokens.surql(1 hunks)schemas/subscriptions.surql(1 hunks)src/config.rs(1 hunks)src/domain/mod.rs(1 hunks)src/domain/subscriber/email.rs(1 hunks)src/domain/subscriber/mod.rs(1 hunks)src/email_client.rs(1 hunks)src/errors.rs(1 hunks)src/handlers.rs(0 hunks)src/handlers/health_check.rs(1 hunks)src/handlers/mod.rs(1 hunks)src/handlers/subscription.rs(1 hunks)src/lib.rs(1 hunks)src/main.rs(1 hunks)src/model.rs(2 hunks)src/startup.rs(3 hunks)src/state.rs(2 hunks)tests/api/health_check.rs(1 hunks)tests/api/helpers.rs(1 hunks)tests/api/main.rs(1 hunks)tests/api/subscriptions.rs(1 hunks)tests/api/subscriptions_confirm.rs(1 hunks)tests/tests.rs(0 hunks)
💤 Files with no reviewable changes (2)
- src/handlers.rs
- tests/tests.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T13:50:40.460Z
Learnt from: TN19N
PR: TN19N/subscriptions#9
File: src/domain/subscriber/email.rs:18-19
Timestamp: 2025-08-28T13:50:40.460Z
Learning: For the SubscriberEmail validation in src/domain/subscriber/email.rs, the user TN19N prefers to include the actual email value in ValidationError messages for debugging purposes, accepting the PII exposure trade-off in favor of detailed error information returned with 400 HTTP status.
Applied to files:
src/domain/mod.rssrc/domain/subscriber/mod.rssrc/domain/subscriber/email.rs
🧬 Code graph analysis (17)
src/handlers/health_check.rs (3)
src/handlers/subscription.rs (1)
mm(47-64)src/handlers.rs (2)
health(25-29)subscribe(14-22)tests/tests.rs (1)
health_works(37-57)
src/lib.rs (1)
src/email_client.rs (1)
email_client(207-215)
tests/api/health_check.rs (3)
tests/api/helpers.rs (1)
new(24-51)tests/tests.rs (2)
health_works(37-57)init(16-34)src/handlers.rs (1)
health(25-29)
src/handlers/mod.rs (1)
src/handlers.rs (2)
subscribe(14-22)health(25-29)
tests/api/main.rs (1)
tests/tests.rs (3)
health_works(37-57)subscribe_works(60-105)subscribe_failed(108-142)
tests/api/subscriptions_confirm.rs (3)
tests/api/subscriptions.rs (2)
app(30-39)app(175-184)src/state.rs (1)
new(17-23)tests/api/helpers.rs (1)
new(24-51)
.env.test (1)
tests/tests.rs (3)
subscribe_works(60-105)subscribe_failed(108-142)dotenvy(20-29)
src/startup.rs (2)
src/handlers/subscription.rs (2)
confirm(43-67)subscribe(20-35)tests/tests.rs (1)
init(16-34)
src/state.rs (3)
src/email_client.rs (2)
email_client(207-215)new(26-32)src/model.rs (1)
new(14-19)tests/api/helpers.rs (1)
new(24-51)
tests/api/helpers.rs (5)
src/email_client.rs (1)
new(26-32)src/state.rs (1)
new(17-23)src/config.rs (1)
load(36-41)src/startup.rs (1)
init(20-43)tests/tests.rs (2)
Result(14-14)init(16-34)
tests/api/subscriptions.rs (4)
tests/api/subscriptions_confirm.rs (1)
app(57-66)src/state.rs (1)
new(17-23)tests/api/helpers.rs (1)
new(24-51)src/handlers.rs (1)
subscribe(14-22)
src/domain/subscriber/mod.rs (1)
src/email_client.rs (1)
src/domain/subscriber/email.rs (2)
tests/tests.rs (1)
subscribe_works(60-105)src/handlers.rs (1)
FormData(8-11)
src/email_client.rs (3)
src/state.rs (1)
new(17-23)tests/api/helpers.rs (1)
new(24-51)src/domain/subscriber/email.rs (1)
try_from(17-23)
src/model.rs (2)
src/handlers.rs (1)
subscribe(14-22)tests/tests.rs (1)
subscribe_works(60-105)
src/handlers/subscription.rs (3)
src/email_client.rs (1)
email_client(207-215)src/handlers.rs (2)
subscribe(14-22)FormData(8-11)tests/tests.rs (1)
subscribe_works(60-105)
src/main.rs (1)
src/startup.rs (1)
init(20-43)
🪛 dotenv-linter (3.3.0)
.env.test
[warning] 1-1: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 4-4: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 5-5: [UnorderedKey] The SUBSCRIPTIONS__HOST key should go before the SUBSCRIPTIONS__PORT key
(UnorderedKey)
[warning] 5-5: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 6-6: [UnorderedKey] The SUBSCRIPTIONS__BASE_URL key should go before the SUBSCRIPTIONS__HOST key
(UnorderedKey)
[warning] 6-6: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 11-11: [UnorderedKey] The SUBSCRIPTIONS__DATABASE__PASSWORD key should go before the SUBSCRIPTIONS__DATABASE__USERNAME key
(UnorderedKey)
[warning] 12-12: [UnorderedKey] The SUBSCRIPTIONS__DATABASE__NAMESPACE key should go before the SUBSCRIPTIONS__DATABASE__PASSWORD key
(UnorderedKey)
[warning] 13-13: [UnorderedKey] The SUBSCRIPTIONS__DATABASE__NAME key should go before the SUBSCRIPTIONS__DATABASE__NAMESPACE key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The SUBSCRIPTIONS__EMAIL_CLIENT__BASE_URL key should go before the SUBSCRIPTIONS__EMAIL_CLIENT__SENDER_EMAIL key
(UnorderedKey)
[warning] 17-17: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 18-18: [UnorderedKey] The SUBSCRIPTIONS__EMAIL_CLIENT__AUTH_TOKEN key should go before the SUBSCRIPTIONS__EMAIL_CLIENT__BASE_URL key
(UnorderedKey)
.env.example
[warning] 6-6: [UnorderedKey] The SUBSCRIPTIONS__BASE_URL key should go before the SUBSCRIPTIONS__HOST key
(UnorderedKey)
[warning] 11-11: [UnorderedKey] The SUBSCRIPTIONS__DATABASE__PASSWORD key should go before the SUBSCRIPTIONS__DATABASE__USERNAME key
(UnorderedKey)
[warning] 12-12: [UnorderedKey] The SUBSCRIPTIONS__DATABASE__NAMESPACE key should go before the SUBSCRIPTIONS__DATABASE__PASSWORD key
(UnorderedKey)
[warning] 13-13: [UnorderedKey] The SUBSCRIPTIONS__DATABASE__NAME key should go before the SUBSCRIPTIONS__DATABASE__NAMESPACE key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The SUBSCRIPTIONS__EMAIL_CLIENT__BASE_URL key should go before the SUBSCRIPTIONS__EMAIL_CLIENT__SENDER_EMAIL key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The SUBSCRIPTIONS__EMAIL_CLIENT__AUTH_TOKEN key should go before the SUBSCRIPTIONS__EMAIL_CLIENT__BASE_URL key
(UnorderedKey)
⏰ 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 (20)
.github/workflows/cd.yml (1)
74-82: Confirm new env var wiring and secrets presenceNew vars look consistent with the config changes. Please verify repo/environ has these defined for the production environment to avoid runtime failures.
src/domain/subscriber/email.rs (1)
20-21: Good: error message includes the offending emailMatches your stated preference for detailed validation errors.
Cargo.toml (2)
9-15: Double-check panic strategypanic = 'abort' reduces size but will kill the process on panic; ensure this aligns with your SLOs on Cloud Run (restarts, cold starts).
38-46: Deps look consistent with the new email client and testsMoving claims to dev-deps and adding wiremock/axum-test/linkify aligns with the new integration tests.
tests/api/health_check.rs (1)
1-16: LGTMTest is concise and asserts both status and empty body correctly using the TestApp harness.
src/domain/mod.rs (1)
4-4: LGTMRe-exporting
SubscriberEmailfrom the domain root improves ergonomics for callers.src/lib.rs (1)
3-3: Private email_client module wiring looks good.Module added without leaking public API; aligns with AppState holding Arc.
tests/api/main.rs (1)
1-4: Test harness module aggregation is clean.Straightforward, discoverable structure for the new integration tests.
src/domain/subscriber/mod.rs (1)
4-5: Public re-export of SubscriberEmail is appropriate.This simplifies downstream imports (crate::domain::SubscriberEmail) without exposing the whole submodule.
src/handlers/mod.rs (1)
1-5: Handlers re-export pattern is tidy.Clear split between health_check and subscription; the glob re-exports keep router wiring concise.
schemas/subscriptions.surql (1)
8-9: Literal enum and record type usage are valid.
- Using a literal union for status is supported: TYPE "PENDING" | "CONFIRMED". (surrealdb.com)
- record<subscription_tokens> is a valid field type pattern. (surrealdb.com)
src/handlers/health_check.rs (1)
7-12: LGTM: DB health check returns 200 with empty body.Implementation is correct and matches tests’ expectations.
src/startup.rs (2)
38-38: LGTM: added GET /subscriptions/confirm route.Route wiring is correct and consistent with the new handler.
20-20: No outdated call sites: allinitcalls updated to takeConfigby value.src/config.rs (2)
36-41: Config loader looks good.Builder + env source +
try_deserialize()is clean.
22-23: No change needed:serde_humantimematches your Cargo.toml
Your Cargo.toml declaresserde-humantime = "0.1.1", and the crate’s docs show using#[serde(with = "serde_humantime")](docs.rs)Likely an incorrect or invalid review comment.
src/state.rs (1)
32-42: Nice use ofFromReffor Axum extractors.Clean dependency injection with
Arcclones.src/model.rs (1)
25-29: Allcreate_subscribercall sites updated
Only usage insrc/handlers/subscription.rsalready callsmm.create_subscriber(&subscriber, &token), matching the new signature.tests/api/subscriptions.rs (1)
56-87: Status codes for invalid payloads: verify contract.Mix of 422 and 400—ensure handlers map validation errors consistently.
If needed, align handler responses or adjust expected codes in these cases.
Also applies to: 89-100
tests/api/helpers.rs (1)
53-53: Renameget_conformation_linkstoget_confirmation_links
tests/api/helpers.rs:53- pub fn get_conformation_links(&self, email_request: &wiremock::Request) -> ConfirmationLinks { + pub fn get_confirmation_links(&self, email_request: &wiremock::Request) -> ConfirmationLinks {Update call sites accordingly:
- tests/api/subscriptions_confirm.rs:40
- tests/api/subscriptions.rs:144
| reqwest = { version = "0.12.23", features = ["json", "rustls-tls"] } | ||
| secrecy = { version = "0.10.3", features = ["serde"] } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make reqwest use only rustls (no accidental default features)
Without default-features = false, reqwest’s defaults may pull in TLS you don’t want.
Apply:
-reqwest = { version = "0.12.23", features = ["json", "rustls-tls"] }
+reqwest = { version = "0.12.23", default-features = false, features = ["json", "rustls-tls"] }🤖 Prompt for AI Agents
In Cargo.toml around lines 20 to 21, the reqwest dependency currently enables
default features which can pull in unwanted TLS/backends; update the reqwest
entry to disable default features and explicitly enable rustls by setting
default-features = false and keeping features = ["json", "rustls-tls"] so only
rustls TLS is used.
| DEFINE FIELD OVERWRITE token ON subscription_tokens TYPE string; | ||
| DEFINE FIELD OVERWRITE created_at ON TABLE subscription_tokens TYPE datetime VALUE time::now() READONLY; | ||
|
|
||
| # --- INDEXES --- | ||
| DEFINE INDEX OVERWRITE unique_token ON subscription_tokens COLUMNS token UNIQUE; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce non-empty tokens at the schema level
Prevent empty-string tokens; uniqueness alone still permits one empty token.
Apply:
-DEFINE FIELD OVERWRITE token ON subscription_tokens TYPE string;
+DEFINE FIELD OVERWRITE token ON subscription_tokens
+ TYPE string
+ ASSERT string::len($value) > 0;📝 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.
| DEFINE FIELD OVERWRITE token ON subscription_tokens TYPE string; | |
| DEFINE FIELD OVERWRITE created_at ON TABLE subscription_tokens TYPE datetime VALUE time::now() READONLY; | |
| # --- INDEXES --- | |
| DEFINE INDEX OVERWRITE unique_token ON subscription_tokens COLUMNS token UNIQUE; | |
| DEFINE FIELD OVERWRITE token ON subscription_tokens | |
| TYPE string | |
| ASSERT string::len($value) > 0; | |
| DEFINE FIELD OVERWRITE created_at ON TABLE subscription_tokens TYPE datetime VALUE time::now() READONLY; | |
| # --- INDEXES --- | |
| DEFINE INDEX OVERWRITE unique_token ON subscription_tokens COLUMNS token UNIQUE; |
🤖 Prompt for AI Agents
In schemas/subscription_tokens.surql around lines 6 to 10, the token field
allows an empty string which the UNIQUE index won't prevent; update the token
field definition to enforce non-empty values at the schema level—add a NOT EMPTY
/ REQUIRED constraint or an explicit CHECK/ASSERT that token != '' or
length(token) > 0 (depending on SurQL syntax) so empty-string tokens are
rejected while keeping the unique index intact.
| use serde::Deserialize; | ||
| use std::convert::TryFrom; | ||
| use validator::{ValidateEmail, ValidationError}; | ||
|
|
||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone, Deserialize)] | ||
| pub struct SubscriberEmail(String); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Route serde deserialization through TryFrom to enforce validation
Deriving Deserialize on the newtype bypasses your TryFrom checks. Make serde call TryFrom so invalid emails are rejected at config/JSON parse time.
Apply:
-use serde::Deserialize;
+use serde::{Deserialize, Serialize};
@@
-#[derive(Debug, Clone, Deserialize)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(try_from = "String", into = "String")]
pub struct SubscriberEmail(String);
@@
impl TryFrom<String> for SubscriberEmail {
@@
}
+
+impl From<SubscriberEmail> for String {
+ fn from(value: SubscriberEmail) -> Self {
+ value.0
+ }
+}Also applies to: 14-24
| r#" | ||
| UPDATE subscriptions | ||
| SET status = 'CONFIRMED' | ||
| WHERE token = ( | ||
| SELECT id | ||
| FROM ONLY subscription_tokens | ||
| WHERE token = $token_val | ||
| ).id | ||
| AND status = 'PENDING'; | ||
| "#, | ||
| ) |
There was a problem hiding this comment.
Fix confirmation UPDATE predicate (wrong column).
You’re comparing subscriptions.token to a selected id. This will never match unless a token column exists and equals an id. Compare id to the selected id.
- UPDATE subscriptions
- SET status = 'CONFIRMED'
- WHERE token = (
- SELECT id
- FROM ONLY subscription_tokens
- WHERE token = $token_val
- ).id
- AND status = 'PENDING';
+ UPDATE subscriptions
+ SET status = 'CONFIRMED'
+ WHERE id = (
+ SELECT id
+ FROM ONLY subscription_tokens
+ WHERE token = $token_val
+ ).id
+ AND status = 'PENDING';📝 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.
| r#" | |
| UPDATE subscriptions | |
| SET status = 'CONFIRMED' | |
| WHERE token = ( | |
| SELECT id | |
| FROM ONLY subscription_tokens | |
| WHERE token = $token_val | |
| ).id | |
| AND status = 'PENDING'; | |
| "#, | |
| ) | |
| r#" | |
| UPDATE subscriptions | |
| SET status = 'CONFIRMED' | |
| WHERE id = ( | |
| SELECT id | |
| FROM ONLY subscription_tokens | |
| WHERE token = $token_val | |
| ).id | |
| AND status = 'PENDING'; | |
| "#, | |
| ) |
🤖 Prompt for AI Agents
In src/handlers/subscription.rs around lines 51 to 61, the UPDATE currently
compares subscriptions.token to the selected id from subscription_tokens which
will never match; change the predicate to compare subscriptions.id to the
selected id (e.g. WHERE id = (SELECT id FROM ONLY subscription_tokens WHERE
token = $token_val) AND status = 'PENDING') so the subscription row is correctly
matched and updated.
| fn get_confirmation_token() -> String { | ||
| let mut rng = rand::rng(); | ||
| std::iter::repeat_with(|| rng.sample(Alphanumeric)) | ||
| .map(char::from) | ||
| .take(25) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm rand API compatibility (rng() and distr::Alphanumeric).
If the crate uses rand 0.8, this won’t compile (needs thread_rng() and distributions::Alphanumeric). Verify the version and adjust if needed.
🏁 Script executed:
#!/bin/bash
# Inspect rand usage/version
rg -n "rand" -C2
fd Cargo.toml | xargs -I{} rg -n "rand\s*=" {}Length of output: 10342
Update rand API usage: Replace rand::rng() with rand::thread_rng() and import Alphanumeric from rand::distributions instead of rand::distr.
--- a/src/handlers/subscription.rs
+++ b/src/handlers/subscription.rs
-use rand::Rng;
-use rand::distr::Alphanumeric;
+use rand::{Rng, thread_rng, distributions::Alphanumeric};
fn get_confirmation_token() -> String {
- let mut rng = rand::rng();
+ let mut rng = thread_rng();
std::iter::repeat_with(|| rng.sample(Alphanumeric))
.map(char::from)
.take(25)
.collect()
}📝 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.
| fn get_confirmation_token() -> String { | |
| let mut rng = rand::rng(); | |
| std::iter::repeat_with(|| rng.sample(Alphanumeric)) | |
| .map(char::from) | |
| .take(25) | |
| .collect() | |
| } | |
| -use rand::Rng; | |
| use rand::{Rng, thread_rng, distributions::Alphanumeric}; | |
| fn get_confirmation_token() -> String { | |
| let mut rng = thread_rng(); | |
| std::iter::repeat_with(|| rng.sample(Alphanumeric)) | |
| .map(char::from) | |
| .take(25) | |
| .collect() | |
| } |
🤖 Prompt for AI Agents
In src/handlers/subscription.rs around lines 69 to 75, update the rand API
usage: replace the call to rand::rng() with rand::thread_rng(), and change the
import for Alphanumeric from rand::distr to rand::distributions::Alphanumeric;
ensure you still have the Rng trait in scope so rng.sample(Alphanumeric) (or
sample_iter) continues to work. Make only these substitutions so token
generation remains the same but uses the correct current rand API.
| @@ -0,0 +1,75 @@ | |||
| use crate::helpers::TestApp; | |||
| use reqwest::{Method, StatusCode}; | |||
There was a problem hiding this comment.
Wiremock matcher and response status type mismatch (won’t compile).
Use string-based method matcher and a u16 status; drop the unused Method import.
Apply:
-use reqwest::{Method, StatusCode};
+use reqwest::StatusCode;
@@
- Mock::given(any())
- .and(method(Method::POST))
- .respond_with(ResponseTemplate::new(StatusCode::OK))
+ Mock::given(any())
+ .and(method("POST"))
+ .respond_with(ResponseTemplate::new(200))
.mount(&app.email_server)
.await;Also applies to: 31-35
🤖 Prompt for AI Agents
In tests/api/subscriptions_confirm.rs around line 2 (and similarly lines 31-35),
the Wiremock setup uses the strongly typed reqwest::Method and an integer
response StatusCode type which causes a type mismatch; remove the unused Method
import, change the Wiremock method matcher to use a string (e.g. "POST" or
"GET") instead of reqwest::Method, and set the mock response status to a u16
(e.g. 200u16) or call .status(200) with a numeric literal so the response uses a
primitive integer status; update any other Wiremock matchers in lines 31-35 the
same way.
| .await; | ||
|
|
||
| // Act | ||
| _ = app.server.post("/subscriptions").form(&body).await; |
There was a problem hiding this comment.
Missing let before _ sink variable.
Current line is invalid Rust; add let.
- _ = app.server.post("/subscriptions").form(&body).await;
+ let _ = app.server.post("/subscriptions").form(&body).await;📝 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.
| _ = app.server.post("/subscriptions").form(&body).await; | |
| let _ = app.server.post("/subscriptions").form(&body).await; |
🤖 Prompt for AI Agents
In tests/api/subscriptions_confirm.rs around line 38, the statement "_ =
app.server.post(\"/subscriptions\").form(&body).await;" is invalid Rust because
the assignment target is missing the "let" binding; update the line to include
"let" (e.g., "let _ = ...") so the await result is properly bound and the code
compiles.
| // Act | ||
| _ = app.server.post("/subscriptions").form(&body).await; | ||
| let email_request = &app.email_server.received_requests().await.unwrap()[0]; | ||
| let confirmation_links = app.get_conformation_links(email_request); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Typo: “conformation” → “confirmation”.
Rename for clarity and consistency with helpers.
If the helper is actually named get_confirmation_links, apply:
- let confirmation_links = app.get_conformation_links(email_request);
+ let confirmation_links = app.get_confirmation_links(email_request);To confirm the helper name, run:
🏁 Script executed:
#!/bin/bash
rg -nP 'fn\s+get_?confi?r?mation_links' -C2 tests/api helpers srcLength of output: 115
🏁 Script executed:
#!/bin/bash
# Search for typo and correct helper definitions/usages across the entire repo
rg -n 'get_conformation_links' -C2 .
rg -n 'get_confirmation_links' -C2 .Length of output: 1349
Rename get_conformation_links to get_confirmation_links across tests/api/
- In tests/api/helpers.rs (line 53), change
-pub fn get_conformation_links(&self, email_request: &wiremock::Request) -> ConfirmationLinks { +pub fn get_confirmation_links(&self, email_request: &wiremock::Request) -> ConfirmationLinks {
- In tests/api/subscriptions_confirm.rs (line 40) and tests/api/subscriptions.rs (line 144), update calls to:
let confirmation_links = app.get_confirmation_links(email_request);
🤖 Prompt for AI Agents
In tests/api/subscriptions_confirm.rs around line 40 the test calls the
misspelled helper get_conformation_links; rename the helper to
get_confirmation_links in tests/api/helpers.rs (line 53) and update all call
sites (including tests/api/subscriptions_confirm.rs line 40 and
tests/api/subscriptions.rs line 144) to use
app.get_confirmation_links(email_request) so names match and tests compile.
| Mock::given(any()) | ||
| .and(method(Method::POST)) | ||
| .respond_with(ResponseTemplate::new(StatusCode::OK)) | ||
| .expect(1) | ||
| .mount(&app.email_server) | ||
| .await; |
There was a problem hiding this comment.
Fix wiremock method matcher type.
matchers::method expects a string; using reqwest::Method won’t compile.
-use reqwest::{Method, StatusCode};
+use reqwest::StatusCode;
...
- Mock::given(any())
- .and(method(Method::POST))
+ Mock::given(any())
+ .and(method("POST"))
.respond_with(ResponseTemplate::new(StatusCode::OK))Apply the same change to all mocks in this file.
Also applies to: 110-116, 132-137, 157-161
🤖 Prompt for AI Agents
In tests/api/subscriptions.rs around lines 17-22 (and similarly at 110-116,
132-137, 157-161), the wiremock matcher call uses reqwest::Method (e.g.
Method::POST) but matchers::method expects a string; replace the Method::XXX
usages with string literals like "POST", "GET", etc. in each
Mock::given(...).and(method(...)) call so the matcher receives &str; apply the
same change to all mentioned mock blocks.
| #[derive(Deserialize)] | ||
| struct QueryResult {} | ||
| let result = app | ||
| .state | ||
| .mm | ||
| .db() | ||
| .await | ||
| .expect("Expected Database to be connected") | ||
| .query("SELECT id FROM ONLY subscriptions WHERE email = 'ursula_le_guin@gmail.com'") | ||
| .await | ||
| .expect("query should be successful") | ||
| .take::<Option<QueryResult>>(0) | ||
| .expect("query result should be valid"); | ||
|
|
There was a problem hiding this comment.
Deserialize the SELECT result correctly.
Empty struct cannot deserialize an object with id; define id and keep Option<>.
- #[derive(Deserialize)]
- struct QueryResult {}
+ #[derive(Deserialize)]
+ struct QueryResult {
+ #[serde(rename = "id")]
+ id: String
+ }
📝 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.
| #[derive(Deserialize)] | |
| struct QueryResult {} | |
| let result = app | |
| .state | |
| .mm | |
| .db() | |
| .await | |
| .expect("Expected Database to be connected") | |
| .query("SELECT id FROM ONLY subscriptions WHERE email = 'ursula_le_guin@gmail.com'") | |
| .await | |
| .expect("query should be successful") | |
| .take::<Option<QueryResult>>(0) | |
| .expect("query result should be valid"); | |
| #[derive(Deserialize)] | |
| struct QueryResult { | |
| #[serde(rename = "id")] | |
| id: String | |
| } |
🤖 Prompt for AI Agents
In tests/api/subscriptions.rs around lines 28 to 41, the test defines an empty
QueryResult struct which cannot deserialize the SELECT result that includes an
id; update the struct to include the id field with the correct type (e.g., id:
i64 or the actual DB id type) and keep the Option<QueryResult> handling so the
query can deserialize successfully; ensure the struct derives Deserialize (and
Debug if helpful).
1 participant
Summary by CodeRabbit