Implement DB-backed user types with tests (3.5.3)#322
Merged
Conversation
…eplacing fixture-backed user types with DB-backed adapters Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Reviewer's GuideImplements DB-backed adapters for user profile and interests, wires them into server startup with config-based DB/fixture switching, adds integration and BDD tests for startup modes and validation/error behavior, updates architecture and roadmap docs, and introduces a small new frontend dependency and formatting-normalization in tests. Sequence diagram for startup wiring of profile and interests adapterssequenceDiagram
participant Main as main
participant ServerConfig as ServerConfig
participant StateBuilders as state_builders
participant HttpState as HttpState
participant ProfilePort as UserProfileQuery
participant InterestsPort as UserInterestsCommand
participant DieselProfile as DieselUserProfileQuery
participant DieselInterests as DieselUserInterestsCommand
participant FixtureProfile as FixtureUserProfileQuery
participant FixtureInterests as FixtureUserInterestsCommand
Main->>ServerConfig: load()
Main->>StateBuilders: build_http_state(config)
activate StateBuilders
StateBuilders->>StateBuilders: build_login_users_pair(config)
StateBuilders->>StateBuilders: build_profile_interests_pair(config)
alt db_pool present
StateBuilders->>DieselProfile: new(DieselUserRepository)
StateBuilders->>DieselInterests: new(DieselUserPreferencesRepository)
StateBuilders-->>ProfilePort: Arc~DieselUserProfileQuery~
StateBuilders-->>InterestsPort: Arc~DieselUserInterestsCommand~
else db_pool absent
StateBuilders->>FixtureProfile: construct
StateBuilders->>FixtureInterests: construct
StateBuilders-->>ProfilePort: Arc~FixtureUserProfileQuery~
StateBuilders-->>InterestsPort: Arc~FixtureUserInterestsCommand~
end
StateBuilders-->>HttpState: HttpStatePorts{login,users,profile,interests,...}
deactivate StateBuilders
Main-->>HttpState: start_http_server(HttpState)
Class diagram for new Diesel user profile and interests adaptersclassDiagram
class UserProfileQuery {
<<interface>>
+fetch_profile(user_id: UserId) Result~User, Error~
}
class UserInterestsCommand {
<<interface>>
+set_interests(user_id: UserId, interest_theme_ids: Vec~InterestThemeId~) Result~UserInterests, Error~
}
class UserRepository {
<<interface>>
+upsert(user: User) Result~(), UserPersistenceError~
+find_by_id(id: UserId) Result~Option~User~, UserPersistenceError~
}
class UserPreferencesRepository {
<<interface>>
+find_by_user_id(user_id: UserId) Result~Option~UserPreferences~, UserPreferencesRepositoryError~
+save(preferences: UserPreferences, expected_revision: Option~u32~) Result~(), UserPreferencesRepositoryError~
}
class DieselUserRepository {
+new(pool: DbPool)
}
class DieselUserPreferencesRepository {
+new(pool: DbPool)
}
class DieselUserProfileQuery {
-user_repository: Arc~UserRepository~
+new(user_repository: DieselUserRepository)
+fetch_profile(user_id: UserId) Result~User, Error~
}
class DieselUserInterestsCommand {
-preferences_repository: Arc~UserPreferencesRepository~
+new(preferences_repository: DieselUserPreferencesRepository)
+set_interests(user_id: UserId, interest_theme_ids: Vec~InterestThemeId~) Result~UserInterests, Error~
}
class UserPreferencesRepositoryError {
<<enum>>
Connection
Query
RevisionMismatch
}
class UserPersistenceError {
<<enum>>
Connection
Query
}
class Error {
+code() ErrorCode
+message() String
+service_unavailable(message: String) Error
+internal(message: String) Error
}
class ErrorCode {
<<enum>>
ServiceUnavailable
InternalError
}
class User {
+id() UserId
}
class UserId {
+as_ref() &str
}
class InterestThemeId {
+as_uuid() &Uuid
}
class UserInterests {
+new(user_id: UserId, interest_theme_ids: Vec~InterestThemeId~) UserInterests
+user_id() &UserId
+interest_theme_ids() &Vec~InterestThemeId~
}
class UserPreferences {
+user_id: UserId
+interest_theme_ids: Vec~Uuid~
+safety_toggle_ids: Vec~Uuid~
+unit_system: UnitSystem
+revision: u32
+builder(user_id: UserId) UserPreferencesBuilder
}
class UserPreferencesBuilder {
+interest_theme_ids(ids: Vec~Uuid~) UserPreferencesBuilder
+safety_toggle_ids(ids: Vec~Uuid~) UserPreferencesBuilder
+unit_system(unit_system: UnitSystem) UserPreferencesBuilder
+revision(revision: u32) UserPreferencesBuilder
+build() UserPreferences
}
class UnitSystem {
<<enum>>
Metric
Imperial
}
UserProfileQuery <|.. DieselUserProfileQuery
UserInterestsCommand <|.. DieselUserInterestsCommand
UserRepository <|.. DieselUserRepository
UserPreferencesRepository <|.. DieselUserPreferencesRepository
DieselUserProfileQuery --> UserRepository : uses
DieselUserInterestsCommand --> UserPreferencesRepository : uses
UserPreferencesRepositoryError --> Error : mapped_by
UserPersistenceError --> Error : mapped_by
UserPreferences --> UnitSystem
UserPreferences --> UserId
UserPreferences --> "*" Uuid
UserInterests --> UserId
UserInterests --> InterestThemeId
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbitRelease Notes
WalkthroughAdd Diesel-backed UserProfileQuery and UserInterestsCommand, wire them into HTTP state when a DB pool exists, implement optimistic-revision insert/update with retry and new repository errors, add non‑negative revision migrations, and add unit, integration and BDD tests plus test helpers and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as HTTP Server
participant State as AppState
participant ProfileQ as DieselUserProfileQuery
participant InterestsC as DieselUserInterestsCommand
participant PrefRepo as UserPreferencesRepository
participant DB as Database
Client->>Server: POST /api/v1/login (credentials)
Server->>State: authenticate user, create session
Client->>Server: GET /api/v1/users/me
Server->>State: fetch_profile(user_id)
State->>ProfileQ: fetch_profile(user_id)
ProfileQ->>DB: SELECT users WHERE id=...
DB-->>ProfileQ: row / none / error
ProfileQ-->>State: User / error
Client->>Server: PUT /api/v1/users/me/interests (theme IDs)
Server->>State: set_interests(user_id, themes)
State->>InterestsC: set_interests(user_id, themes)
InterestsC->>PrefRepo: find_by_user_id(user_id)
PrefRepo->>DB: SELECT user_preferences ...
DB-->>PrefRepo: preferences / none / error
PrefRepo-->>InterestsC: Option<UserPreferences> / error
InterestsC->>InterestsC: build_preferences_for_interest_update(...)
InterestsC->>PrefRepo: save(updated_preferences, expected_revision?)
PrefRepo->>DB: INSERT ... ON CONFLICT DO NOTHING / UPDATE ... WHERE revision = expected
DB-->>PrefRepo: success / zero-rows / constraint violation
PrefRepo-->>InterestsC: ok / RevisionMismatch(current_revision) / MissingForUpdate / ConcurrentWriteConflict / error
InterestsC->>InterestsC: retry loop on retryable errors (up to MAX_CONCURRENT_WRITE_ATTEMPTS)
InterestsC-->>State: UserInterests (on success) / Error
State-->>Client: updated interests JSON / error response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
Implement roadmap item 3.5.3 by replacing fixture-only profile and\ninterests wiring with explicit Diesel-backed adapters in DB-present\nstartup mode, while preserving fixture fallback with no DB pool.\n\nAdd dedicated outbound adapters for UserProfileQuery and\nUserInterestsCommand, wire them through state_builders, and export\nthem from the persistence module.\n\nExpand regression coverage with startup-mode integration and BDD suites\ncovering fixture fallback, DB-backed behaviour, schema-loss stability,\nand validation-envelope stability.\n\nUpdate architecture and roadmap docs to record the dedicated-adapter\ndecision and mark 3.5.3 complete.\n\nInclude formatting-only updates required by repository check-fmt gates.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: backend/tests/user_state_profile_interests_startup_modes_bdd.rs Comment on file fn run_profile_interests_flow(world: &mut World) {
if is_skipped(world) {
return;
}
let db_pool = world.db.as_ref().expect("db context").pool.clone();
let bind_addr = SocketAddr::from(([127, 0, 0, 1], 0));
let config =
ServerConfig::new(Key::generate(), false, SameSite::Lax, bind_addr).with_db_pool(db_pool);
let state = state_builders::build_http_state(
&config,
Arc::new(FixtureRouteSubmissionService) as Arc<dyn RouteSubmissionService>,
);
let payload = InterestsRequest {
interest_theme_ids: world.interests_payload.interest_theme_ids.clone(),
};
let (login_snapshot, profile_snapshot, interests_snapshot) = run_async(async move {
let session = SessionMiddleware::builder(CookieSessionStore::default(), Key::generate())
.cookie_name("session".to_owned())
.cookie_path("/".to_owned())
.cookie_secure(false)
.cookie_http_only(true)
.cookie_content_security(CookieContentSecurity::Private)
.cookie_same_site(SameSite::Lax)
.session_lifecycle(PersistentSession::default().session_ttl(CookieDuration::hours(2)))
.build();
let app = actix_test::init_service(
App::new().app_data(state).wrap(backend::Trace).service(
web::scope("/api/v1")
.wrap(session)
.service(login)
.service(current_user)
.service(update_interests),
),
)
.await;
let login_req = actix_test::TestRequest::post()
.uri("/api/v1/login")
.set_json(&LoginRequest {
username: "admin".to_owned(),
password: "password".to_owned(),
})
.to_request();
let login_res = actix_test::call_service(&app, login_req).await;
let login_snapshot = Snapshot {
status: login_res.status().as_u16(),
trace_id: login_res
.headers()
.get(TRACE_ID_HEADER)
.and_then(|value| value.to_str().ok())
.map(ToOwned::to_owned),
session_cookie: login_res
.response()
.cookies()
.find(|cookie| cookie.name() == "session")
.map(|cookie| cookie.into_owned()),
body: parse_json_body(actix_test::read_body(login_res).await.as_ref()),
};
let Some(cookie) = login_snapshot.session_cookie.clone() else {
return (login_snapshot, None, None);
};
let profile_req = actix_test::TestRequest::get()
.uri("/api/v1/users/me")
.cookie(cookie.clone())
.to_request();
let profile_res = actix_test::call_service(&app, profile_req).await;
let profile_snapshot = Snapshot {
status: profile_res.status().as_u16(),
trace_id: profile_res
.headers()
.get(TRACE_ID_HEADER)
.and_then(|value| value.to_str().ok())
.map(ToOwned::to_owned),
session_cookie: None,
body: parse_json_body(actix_test::read_body(profile_res).await.as_ref()),
};
let interests_req = actix_test::TestRequest::put()
.uri("/api/v1/users/me/interests")
.cookie(cookie)
.set_json(payload)
.to_request();
let interests_res = actix_test::call_service(&app, interests_req).await;
let interests_snapshot = Snapshot {
status: interests_res.status().as_u16(),
trace_id: interests_res
.headers()
.get(TRACE_ID_HEADER)
.and_then(|value| value.to_str().ok())
.map(ToOwned::to_owned),
session_cookie: None,
body: parse_json_body(actix_test::read_body(interests_res).await.as_ref()),
};
(
login_snapshot,
Some(profile_snapshot),
Some(interests_snapshot),
)
});
world.login = Some(login_snapshot);
world.profile = profile_snapshot;
world.interests = interests_snapshot;
}❌ New issue: Large Method |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: backend/tests/diesel_profile_interests_adapters.rs Comment on file //! Focused startup-mode coverage for profile/interests routes.
❌ New issue: String Heavy Function Arguments |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: backend/tests/diesel_profile_interests_adapters.rs Comment on file async fn run_flow(
state: web::Data<HttpState>,
username: &str,
password: &str,
interests_payload: &InterestsRequest,
) -> (Snapshot, Option<Snapshot>, Option<Snapshot>) {
let session = SessionMiddleware::builder(CookieSessionStore::default(), Key::generate())
.cookie_name("session".to_owned())
.cookie_path("/".to_owned())
.cookie_secure(false)
.cookie_http_only(true)
.cookie_content_security(CookieContentSecurity::Private)
.cookie_same_site(SameSite::Lax)
.session_lifecycle(PersistentSession::default().session_ttl(CookieDuration::hours(2)))
.build();
let app = actix_test::init_service(
App::new().app_data(state).wrap(backend::Trace).service(
web::scope("/api/v1")
.wrap(session)
.service(login)
.service(current_user)
.service(update_interests),
),
)
.await;
let login_req = actix_test::TestRequest::post()
.uri("/api/v1/login")
.set_json(&LoginRequest {
username: username.to_owned(),
password: password.to_owned(),
})
.to_request();
let login_res = actix_test::call_service(&app, login_req).await;
let login_snapshot = Snapshot {
status: login_res.status().as_u16(),
session_cookie: login_res
.response()
.cookies()
.find(|cookie| cookie.name() == "session")
.map(|cookie| cookie.into_owned()),
body: parse_body(actix_test::read_body(login_res).await.as_ref()),
};
let Some(cookie) = login_snapshot.session_cookie.clone() else {
return (login_snapshot, None, None);
};
let profile_req = actix_test::TestRequest::get()
.uri("/api/v1/users/me")
.cookie(cookie.clone())
.to_request();
let profile_res = actix_test::call_service(&app, profile_req).await;
let profile_snapshot = Snapshot {
status: profile_res.status().as_u16(),
session_cookie: None,
body: parse_body(actix_test::read_body(profile_res).await.as_ref()),
};
let interests_req = actix_test::TestRequest::put()
.uri("/api/v1/users/me/interests")
.cookie(cookie)
.set_json(interests_payload)
.to_request();
let interests_res = actix_test::call_service(&app, interests_req).await;
let interests_snapshot = Snapshot {
status: interests_res.status().as_u16(),
session_cookie: None,
body: parse_body(actix_test::read_body(interests_res).await.as_ref()),
};
(
login_snapshot,
Some(profile_snapshot),
Some(interests_snapshot),
)
}❌ New issue: Large Method |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Reduce string-heavy function argument usage in\nbackend/tests/diesel_profile_interests_adapters.rs by introducing\nstronger local types and shared credentials input.\n\n- Add Credentials struct and use it in run_flow\n- Type seed_user and db_contains_interest_id UUID parameters as Uuid\n- Update call sites without changing test assertions or constants
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extract session middleware, response snapshot capture, and profile/interests\nflow execution helpers in user_state_profile_interests_startup_modes_bdd\nto reduce run_profile_interests_flow complexity while preserving behaviour.\n\nKeep the cookie guard semantics and world assignment flow unchanged.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extract helper functions from run_flow to reduce method size and remove\nstring-heavy argument passing.\n\nChanges include:\n- add build_test_app, do_login, do_get_profile, and\n do_update_interests helpers\n- update run_flow to orchestrate those helpers with\n Credentials input\n- keep existing test assertions and behaviour unchanged\n\nThis keeps startup-mode test coverage intact while addressing\nCodeScene large-method and argument-smell findings.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: backend/tests/diesel_profile_interests_adapters.rs Comment on file async fn do_get_profile<S>(app: &S, cookie: Cookie<'static>) -> Snapshot
where
S: actix_web::dev::Service<
actix_http::Request,
Response = actix_web::dev::ServiceResponse<actix_web::body::BoxBody>,
Error = actix_web::Error,
>,
{
let profile_req = actix_test::TestRequest::get()
.uri("/api/v1/users/me")
.cookie(cookie)
.to_request();
let profile_res = actix_test::call_service(app, profile_req).await;
Snapshot {
status: profile_res.status().as_u16(),
session_cookie: None,
body: parse_body(actix_test::read_body(profile_res).await.as_ref()),
}
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Extract a shared call_and_capture helper for the profile and\ninterests request helpers in the Diesel adapter tests.\n\nThis removes the remaining duplicated Actix request execution and\nSnapshot construction while preserving the existing test flow and\nassertions.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Override the transitive DOMPurify dependency used by Mermaid tooling\nso workspace audits no longer fail on the current XSS advisories.\n\nThis updates the root pnpm override and refreshes the lockfile to\nresolve DOMPurify 3.3.2 across the affected dependency paths.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationbackend/tests/diesel_profile_interests_adapters.rs: What lead to degradation?The module contains 2 functions with similar structure: do_get_profile,do_update_interests Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationbackend/tests/diesel_profile_interests_adapters.rs: What lead to degradation?The module contains 2 functions with similar structure: do_get_profile,do_update_interests Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
Owner
Author
|
@CodeRabbit if you feel that no further action is needed with respect to this comment, please provide a suitable message to accompany the lint suppression to be applied to the codescene diagnostic. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/outbound/persistence/diesel_user_preferences_repository.rs (1)
167-173:⚠️ Potential issue | 🟡 MinorValidate revision bounds at the write boundary to match read-side behaviour.
cast_revision_for_dbcurrently uses an unchecked cast (revision as i32), whilst the symmetriccast_revision_from_dbfunction correctly validates withi32::try_from. This asymmetry allows oversized revisions to wrap silently instead of surfacing a repository-level error. Refactorcast_revision_for_dbto returnResult<i32, UserPreferencesRepositoryError>and propagate errors at lines 207 and 240 using the?operator (thesavefunction already returnsResult).Proposed fix
-#[expect( - clippy::cast_possible_wrap, - reason = "revision values are always small positive integers" -)] -fn cast_revision_for_db(revision: u32) -> i32 { - revision as i32 +fn cast_revision_for_db(revision: u32) -> Result<i32, UserPreferencesRepositoryError> { + i32::try_from(revision).map_err(|_| { + UserPreferencesRepositoryError::query(format!("revision out of range: {revision}")) + }) }- let revision_i32 = cast_revision_for_db(preferences.revision); + let revision_i32 = cast_revision_for_db(preferences.revision)?;- let expected_i32 = cast_revision_for_db(expected); + let expected_i32 = cast_revision_for_db(expected)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/outbound/persistence/diesel_user_preferences_repository.rs` around lines 167 - 173, Replace the unchecked cast in cast_revision_for_db with a fallible conversion: change cast_revision_for_db to return Result<i32, UserPreferencesRepositoryError> and use i32::try_from(revision).map_err(|_| UserPreferencesRepositoryError::query(format!("revision out of range: {revision}"))) so oversized u32 revisions produce an error instead of wrapping; then update callers in save (where revision_i32 and expected_i32 are computed) to propagate the error with the ? operator (i.e., let revision_i32 = cast_revision_for_db(preferences.revision)? and let expected_i32 = cast_revision_for_db(expected)?), keeping the save function's existing Result return type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/domain/preferences_service_tests.rs`:
- Around line 100-167: Two nearly identical tests
update_maps_missing_for_update_to_conflict and
update_maps_concurrent_write_conflict_to_conflict should be merged into a single
parameterized rstest: create an #[rstest] with parameters for the mocked
repository behavior (the error returned by
MockUserPreferencesRepository::expect_save) and the expected JSON detail check,
perform the shared setup once (create user_id, repo,
repo.expect_find_by_user_id, make_service, and build UpdatePreferencesRequest),
then in the body set repo.expect_save().return_once to return the injected error
and assert the single error path (error.code() == ErrorCode::Conflict and the
details match the provided expected value) instead of duplicating the full
service/test setup across two tests; reference test names
update_maps_missing_for_update_to_conflict,
update_maps_concurrent_write_conflict_to_conflict,
MockUserPreferencesRepository,
UserPreferencesRepositoryError::missing_for_update,
UserPreferencesRepositoryError::concurrent_write_conflict, make_service, and
UpdatePreferencesRequest to locate and refactor the code.
In `@backend/src/outbound/persistence/diesel_user_interests_command.rs`:
- Around line 77-83: Change the mapping for
UserPreferencesRepositoryError::RevisionMismatch to return an Error::conflict
rather than Error::internal and attach a details payload containing the expected
and actual revisions; specifically, in the match arm for
UserPreferencesRepositoryError::RevisionMismatch { expected, actual } replace
the internal error with Error::conflict("preferences changed
concurrently").with_details(json!({ "code": "revision_mismatch",
"expectedRevision": expected, "actualRevision": actual })) so callers receive a
409-style semantic error and can inspect the revision fields.
In
`@backend/src/outbound/persistence/diesel_user_interests_command/tests/retry.rs`:
- Around line 114-119: Add assertions that the persisted interest IDs match
expected values in the retry tests by checking
saved_preferences.interest_theme_ids from the repository.last_save_call()
result; after retrieving (saved_preferences, expected_revision) (the tuple
returned by repository.last_save_call() used in these tests), assert that
saved_preferences.interest_theme_ids equals the expected vector/collection for
the retry case, and add the same check to the other test that uses
repository.last_save_call() around the second retry (the one mirrored by the
block at the later test), ensuring both tests validate the persisted IDs as well
as the DTO and revision.
- Around line 8-145: Collapse the four separate tokio tests into a single
parameterised rstest that iterates over the four retry scenarios and uses a
small helper assertion to check the saved result; specifically, replace the
individual tests set_interests_retries_after_insert_race_revision_mismatch,
set_interests_retries_after_stale_update_revision_mismatch,
set_interests_retries_after_missing_preferences_for_update, and
set_interests_retries_after_concurrent_write_conflict with an #[rstest] that
supplies (repository, expected_revision, expected_saved_revision,
expected_interest_uuid) cases, reuse DieselUserInterestsCommand::new(...) and
its set_interests(&user_id, ...) call in a shared fixture, and assert via a
helper that calls repository.last_save_call() and compares expected_revision and
saved_preferences.revision; ensure you still configure repositories the same way
(InsertRaceRetryRepository, StaleUpdateRetryRepository,
StubUserPreferencesRepository + set_save_failure(StubFailure::...), and
interest_theme_id values) so the test logic and expectations remain identical.
In
`@backend/src/outbound/persistence/diesel_user_interests_command/tests/support.rs`:
- Around line 188-213: The stub save method in StubUserPreferencesRepository
currently overwrites stored_preferences unconditionally; change save to enforce
expected_revision semantics by first reading the current stored_preferences
(from self.stored_preferences) and rejecting the call when expected_revision is
None but no existing row (return a not-found/appropriate
UserPreferencesRepositoryError) or when expected_revision is Some(rev) that does
not match the current revision (return a revision-mismatch error) instead of
writing; preserve the existing save_failures handling and only update
self.stored_preferences and self.last_save after the revision checks pass.
---
Outside diff comments:
In `@backend/src/outbound/persistence/diesel_user_preferences_repository.rs`:
- Around line 167-173: Replace the unchecked cast in cast_revision_for_db with a
fallible conversion: change cast_revision_for_db to return Result<i32,
UserPreferencesRepositoryError> and use i32::try_from(revision).map_err(|_|
UserPreferencesRepositoryError::query(format!("revision out of range:
{revision}"))) so oversized u32 revisions produce an error instead of wrapping;
then update callers in save (where revision_i32 and expected_i32 are computed)
to propagate the error with the ? operator (i.e., let revision_i32 =
cast_revision_for_db(preferences.revision)? and let expected_i32 =
cast_revision_for_db(expected)?), keeping the save function's existing Result
return type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96d78c8b-f7e3-457b-8b4e-2884a0db5453
📒 Files selected for processing (11)
backend/migrations/2026-03-07-000000_add_non_negative_revision_checks/down.sqlbackend/migrations/2026-03-07-000000_add_non_negative_revision_checks/up.sqlbackend/src/domain/ports/user_preferences_repository.rsbackend/src/domain/preferences_service.rsbackend/src/domain/preferences_service_tests.rsbackend/src/outbound/persistence/diesel_user_interests_command.rsbackend/src/outbound/persistence/diesel_user_interests_command/tests/retry.rsbackend/src/outbound/persistence/diesel_user_interests_command/tests/support.rsbackend/src/outbound/persistence/diesel_user_preferences_repository.rsbackend/tests/diesel_route_annotation_repository.rsbackend/tests/diesel_user_preferences_repository.rs
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extract shared RetrySaveTracker helpers for the retry repositories\nso the duplicated find and save test paths collapse into one\nplace.\n\nKeep the scenario-specific mismatch tuples and messages intact,\nand use a tightly scoped clippy expectation because the\nCodeScene-directed helper shape exceeds the repo's argument\nthreshold.\n\nCo-authored-by: Codex <noreply@openai.com>
Verify the reported findings against the current branch and\napply only the live fixes.\n\n- map revision mismatches in the interests command to conflict\n errors with structured details\n- make the Diesel revision-to-DB conversion fallible and cover it\n with a regression test\n- enforce optimistic-concurrency semantics in the stub\n preferences repository used by retry coverage\n- collapse the duplicated preferences-service and retry tests into\n parameterised rstest cases while preserving their assertions\n\nCo-authored-by: Codex <noreply@openai.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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
`@backend/src/outbound/persistence/diesel_user_interests_command/tests/retry.rs`:
- Around line 50-130: The function build_retry_case is flagged by CodeScene for
exceeding the line threshold but is intentionally cohesive; add a short
justification comment directly above build_retry_case explaining that the
function intentionally keeps all RetryScenario arms together for readability and
test fixture integrity (mentioning RetryScenario, RetryCase, and the specific
repository builders InsertRaceRetryRepository, StaleUpdateRetryRepository, and
StubUserPreferencesRepository), so automated tooling reviewers know this is
deliberate and the function should not be split.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 617930f9-26a6-461a-aff4-22605b1ad368
📒 Files selected for processing (6)
backend/src/domain/preferences_service_tests.rsbackend/src/outbound/persistence/diesel_user_interests_command.rsbackend/src/outbound/persistence/diesel_user_interests_command/tests/mapping.rsbackend/src/outbound/persistence/diesel_user_interests_command/tests/retry.rsbackend/src/outbound/persistence/diesel_user_interests_command/tests/support.rsbackend/src/outbound/persistence/diesel_user_preferences_repository.rs
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: backend/src/domain/preferences_service_tests.rs Comment on file MutationType, UnitSystem, UserId, UserPreferences,
};
use chrono::Utc;
use rstest::rstest;❌ New issue: Code Duplication |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: backend/src/outbound/persistence/diesel_user_interests_command/tests/support.rs Comment on file async fn find_by_user_id(
&self,
user_id: &UserId,
) -> Result<Option<UserPreferences>, UserPreferencesRepositoryError> {
if *user_id != self.user_id {
return Ok(None);
}
Ok(self
.tracker
.select_preferences(None, &self.competing_preferences)
.cloned())
}❌ New issue: Code Duplication |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: backend/src/outbound/persistence/diesel_user_interests_command/tests/retry.rs Comment on file fn build_retry_case(scenario: RetryScenario) -> RetryCase {
let user_id = user_id();
match scenario {
RetryScenario::InsertRaceRevisionMismatch => RetryCase {
repository: RetryRepositoryHarness::InsertRace(Arc::new(
InsertRaceRetryRepository::new(
user_id.clone(),
UserPreferences::builder(user_id.clone())
.interest_theme_ids(vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afa9")])
.safety_toggle_ids(Vec::new())
.unit_system(UnitSystem::Metric)
.revision(1)
.build(),
),
)),
user_id,
interest_theme_ids: vec![interest_theme_id("3fa85f64-5717-4562-b3fc-2c963f66afa6")],
expected_revision: Some(1),
expected_saved_revision: 2,
expected_saved_interest_ids: vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afa6")],
},
RetryScenario::StaleUpdateRevisionMismatch => RetryCase {
repository: RetryRepositoryHarness::StaleUpdate(Arc::new(
StaleUpdateRetryRepository::new(
UserPreferences::builder(user_id.clone())
.interest_theme_ids(vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afa6")])
.safety_toggle_ids(vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afa8")])
.unit_system(UnitSystem::Metric)
.revision(2)
.build(),
UserPreferences::builder(user_id.clone())
.interest_theme_ids(vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afaa")])
.safety_toggle_ids(vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afa8")])
.unit_system(UnitSystem::Metric)
.revision(3)
.build(),
),
)),
user_id,
interest_theme_ids: vec![interest_theme_id("3fa85f64-5717-4562-b3fc-2c963f66afa7")],
expected_revision: Some(3),
expected_saved_revision: 4,
expected_saved_interest_ids: vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afa7")],
},
RetryScenario::MissingPreferencesForUpdate => {
let existing_preferences = UserPreferences::builder(user_id.clone())
.interest_theme_ids(vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afa6")])
.safety_toggle_ids(vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afa8")])
.unit_system(UnitSystem::Metric)
.revision(4)
.build();
let repository = Arc::new(StubUserPreferencesRepository::with_preferences(
existing_preferences,
));
repository.set_save_failure(StubFailure::MissingForUpdate { expected: 4 });
RetryCase {
repository: RetryRepositoryHarness::Stub(repository),
user_id,
interest_theme_ids: vec![interest_theme_id("3fa85f64-5717-4562-b3fc-2c963f66afa7")],
expected_revision: Some(4),
expected_saved_revision: 5,
expected_saved_interest_ids: vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afa7")],
}
}
RetryScenario::ConcurrentWriteConflict => {
let repository = Arc::new(StubUserPreferencesRepository::default());
repository.set_save_failure(StubFailure::ConcurrentWriteConflict);
RetryCase {
repository: RetryRepositoryHarness::Stub(repository),
user_id,
interest_theme_ids: vec![interest_theme_id("3fa85f64-5717-4562-b3fc-2c963f66afa6")],
expected_revision: None,
expected_saved_revision: 1,
expected_saved_interest_ids: vec![uuid_id("3fa85f64-5717-4562-b3fc-2c963f66afa6")],
}
}
}
}❌ New issue: Large Method |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. Please address the following concerns and ensure all commit gates succeed:
|
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 19791
fn cast_revision_for_db(revision: u32) -> Result<i32, UserPreferencesRepositoryError> {
i32::try_from(revision).map_err(|_| {
UserPreferencesRepositoryError::query(format!("revision out of range: {revision}"))
})
}Both call sites in |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extract the four retry-case constructors out of build_retry_case\nso the dispatcher stays below the method-size threshold without\nchanging any scenario data or test behaviour.\n\nCo-authored-by: Codex <noreply@openai.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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
`@backend/src/outbound/persistence/diesel_user_interests_command/tests/retry.rs`:
- Around line 146-149: Rename the confusing variable actual_expected_revision to
a clearer name (e.g., passed_expected_revision or recorded_expected_revision) in
the test where you destructure repository.last_save_call(); update the let
binding (let (saved_preferences, passed_expected_revision) =
repository.last_save_call().expect(...)) and any subsequent uses (the assert_eq!
comparing it to expected_revision) so the name reflects that this is the
expected-revision value passed into the save call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3775cdf-4c15-4ffb-a59e-43c90fd9fbb4
📒 Files selected for processing (1)
backend/src/outbound/persistence/diesel_user_interests_command/tests/retry.rs
Replace two duplicated preferences-service conflict tests with one\nparameterised rstest while preserving the original missing\nrevision and mismatched revision scenarios.\n\nThis keeps the conflict-path assertions identical and reduces\nduplication in the test module.\n\nCo-authored-by: Codex <noreply@openai.com>
Rename the saved-call expected revision binding in the retry\nassertion helper so it reflects the value recorded on the\nrepository save call.\n\nThis keeps the test logic unchanged while making the assertion\nintent easier to read.\n\nCo-authored-by: Codex <noreply@openai.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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
1 participant
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.
Summary
Changes
Documentation
Code
Tests
Rationale
Implementation plan (per ExecPlan)
Validation and acceptance
How to test locally
Artifacts
Notes
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/d72b1dd4-5d4d-42ba-8f79-68826a3d6e5c
Summary by Sourcery
Switch user profile and interests endpoints from fixture-only wiring to DB-backed adapters when a database pool is configured, while preserving fixture fallbacks and documenting the chosen adapter strategy.
New Features:
Enhancements:
Documentation:
Tests: