refactor: fix HIGH+MEDIUM architectural violations from code review
HIGH: fix watch_medium data-loss bug, standardize error handling on ApiError, fix dep direction (rss/template-askama no longer dep on application), extract ImageFetcher port (remove reqwest from app layer), move event construction from save_review to use case, extract infra-wiring crate (DbPool/EventBusBackend dedup), deduplicate presentation helpers (encode_error, export streaming, multipart parsing) MEDIUM: split LocalApContentQuery god-trait 10→3 methods, dedup movie resolution orchestration, add RemoteActorDto/PersonDto mappers, move AppConfig to infra-wiring, fix SocialQueryPort Uuid→UserId, replace stringly-typed api-types with domain enums, move count_reviews_in_year to StatsRepository, dedup event publisher cfg blocks, extract should_enrich, move group_by_month to application, dedup count_local_posts, add FederationFlags Default, TUI input helper + ShowError rename + typed auth errors, api-types cleanup (UserSettingsDto/UserProfileBase/PreviewRowData) 102 files changed, -681 lines net
This commit is contained in:
@@ -4,7 +4,10 @@ use domain::ports::EventHandler;
|
||||
use domain::{
|
||||
errors::DomainError,
|
||||
events::DomainEvent,
|
||||
ports::{LocalApContentQuery, UserFederationSettingsQuery},
|
||||
ports::{
|
||||
GoalRepository, LocalApContentQuery, MovieRepository, ReviewRepository, StatsRepository,
|
||||
UserFederationSettingsQuery,
|
||||
},
|
||||
value_objects::{MovieId, ReviewId, UserId},
|
||||
};
|
||||
use std::sync::Arc;
|
||||
@@ -17,20 +20,33 @@ use crate::urls::{actor_url, goal_url, review_url};
|
||||
pub struct ActivityPubEventHandler {
|
||||
ap_service: Arc<ActivityPubService>,
|
||||
content_query: Arc<dyn LocalApContentQuery>,
|
||||
review_repo: Arc<dyn ReviewRepository>,
|
||||
movie_repo: Arc<dyn MovieRepository>,
|
||||
goal_repo: Arc<dyn GoalRepository>,
|
||||
stats_repo: Arc<dyn StatsRepository>,
|
||||
federation_settings: Arc<dyn UserFederationSettingsQuery>,
|
||||
base_url: String,
|
||||
}
|
||||
|
||||
impl ActivityPubEventHandler {
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub fn new(
|
||||
ap_service: Arc<ActivityPubService>,
|
||||
content_query: Arc<dyn LocalApContentQuery>,
|
||||
review_repo: Arc<dyn ReviewRepository>,
|
||||
movie_repo: Arc<dyn MovieRepository>,
|
||||
goal_repo: Arc<dyn GoalRepository>,
|
||||
stats_repo: Arc<dyn StatsRepository>,
|
||||
federation_settings: Arc<dyn UserFederationSettingsQuery>,
|
||||
base_url: String,
|
||||
) -> Self {
|
||||
Self {
|
||||
ap_service,
|
||||
content_query,
|
||||
review_repo,
|
||||
movie_repo,
|
||||
goal_repo,
|
||||
stats_repo,
|
||||
federation_settings,
|
||||
base_url,
|
||||
}
|
||||
@@ -157,16 +173,12 @@ impl ActivityPubEventHandler {
|
||||
.federation_settings
|
||||
.get_federation_flags(user_id)
|
||||
.await
|
||||
.unwrap_or(domain::models::FederationFlags {
|
||||
goals: true,
|
||||
reviews: true,
|
||||
watchlist: true,
|
||||
});
|
||||
.unwrap_or_default();
|
||||
if !flags.reviews {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let review = match self.content_query.get_review_by_id(review_id).await? {
|
||||
let review = match self.review_repo.get_review_by_id(review_id).await? {
|
||||
Some(r) => r,
|
||||
None => return Ok(()),
|
||||
};
|
||||
@@ -175,7 +187,7 @@ impl ActivityPubEventHandler {
|
||||
let actor = actor_url(&self.base_url, user_id.value());
|
||||
|
||||
let movie = self
|
||||
.content_query
|
||||
.movie_repo
|
||||
.get_movie_by_id(review.movie_id())
|
||||
.await
|
||||
.ok()
|
||||
@@ -227,16 +239,12 @@ impl ActivityPubEventHandler {
|
||||
.federation_settings
|
||||
.get_federation_flags(user_id)
|
||||
.await
|
||||
.unwrap_or(domain::models::FederationFlags {
|
||||
goals: true,
|
||||
reviews: true,
|
||||
watchlist: true,
|
||||
});
|
||||
.unwrap_or_default();
|
||||
if !flags.reviews {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let review = match self.content_query.get_review_by_id(review_id).await? {
|
||||
let review = match self.review_repo.get_review_by_id(review_id).await? {
|
||||
Some(r) => r,
|
||||
None => return Ok(()),
|
||||
};
|
||||
@@ -245,7 +253,7 @@ impl ActivityPubEventHandler {
|
||||
let actor = actor_url(&self.base_url, user_id.value());
|
||||
|
||||
let movie = self
|
||||
.content_query
|
||||
.movie_repo
|
||||
.get_movie_by_id(review.movie_id())
|
||||
.await
|
||||
.ok()
|
||||
@@ -310,11 +318,7 @@ impl ActivityPubEventHandler {
|
||||
.federation_settings
|
||||
.get_federation_flags(user_id)
|
||||
.await
|
||||
.unwrap_or(domain::models::FederationFlags {
|
||||
goals: true,
|
||||
reviews: true,
|
||||
watchlist: true,
|
||||
});
|
||||
.unwrap_or_default();
|
||||
if !flags.watchlist {
|
||||
return Ok(());
|
||||
}
|
||||
@@ -324,7 +328,7 @@ impl ActivityPubEventHandler {
|
||||
let actor = actor_url(&self.base_url, user_id.value());
|
||||
|
||||
let poster_url = self
|
||||
.content_query
|
||||
.movie_repo
|
||||
.get_movie_by_id(movie_id)
|
||||
.await
|
||||
.ok()
|
||||
@@ -373,7 +377,7 @@ impl ActivityPubEventHandler {
|
||||
.get_local_reviews_for_movie(movie_id)
|
||||
.await?;
|
||||
|
||||
let movie = self.content_query.get_movie_by_id(movie_id).await?;
|
||||
let movie = self.movie_repo.get_movie_by_id(movie_id).await?;
|
||||
let movie = match movie {
|
||||
Some(m) => m,
|
||||
None => return Ok(()),
|
||||
@@ -393,11 +397,7 @@ impl ActivityPubEventHandler {
|
||||
.federation_settings
|
||||
.get_federation_flags(user_id)
|
||||
.await
|
||||
.unwrap_or(domain::models::FederationFlags {
|
||||
goals: true,
|
||||
reviews: true,
|
||||
watchlist: true,
|
||||
});
|
||||
.unwrap_or_default();
|
||||
if !flags.reviews {
|
||||
continue;
|
||||
}
|
||||
@@ -436,23 +436,24 @@ impl ActivityPubEventHandler {
|
||||
.federation_settings
|
||||
.get_federation_flags(user_id)
|
||||
.await
|
||||
.unwrap_or(domain::models::FederationFlags {
|
||||
goals: true,
|
||||
reviews: true,
|
||||
watchlist: true,
|
||||
});
|
||||
.unwrap_or_default();
|
||||
if !flags.goals {
|
||||
return Ok(());
|
||||
}
|
||||
let Some((goal, current)) = self
|
||||
.content_query
|
||||
.get_goal_with_progress(user_id, year)
|
||||
let Some(goal) = self
|
||||
.goal_repo
|
||||
.find_by_user_and_year(user_id, year)
|
||||
.await
|
||||
.ok()
|
||||
.flatten()
|
||||
else {
|
||||
return Ok(());
|
||||
};
|
||||
let current = self
|
||||
.stats_repo
|
||||
.count_reviews_in_year(user_id, year)
|
||||
.await
|
||||
.unwrap_or(0);
|
||||
let ap_id = goal_url(&self.base_url, user_id.value(), year);
|
||||
let actor = actor_url(&self.base_url, user_id.value());
|
||||
let obj = goal_to_ap_object(
|
||||
@@ -481,21 +482,14 @@ impl ActivityPubEventHandler {
|
||||
.federation_settings
|
||||
.get_federation_flags(user_id)
|
||||
.await
|
||||
.unwrap_or(domain::models::FederationFlags {
|
||||
goals: true,
|
||||
reviews: true,
|
||||
watchlist: true,
|
||||
});
|
||||
.unwrap_or_default();
|
||||
if !flags.goals {
|
||||
return Ok(());
|
||||
}
|
||||
let current = self
|
||||
.content_query
|
||||
.get_goal_with_progress(user_id, year)
|
||||
.stats_repo
|
||||
.count_reviews_in_year(user_id, year)
|
||||
.await
|
||||
.ok()
|
||||
.flatten()
|
||||
.map(|(_, c)| c)
|
||||
.unwrap_or(0);
|
||||
|
||||
let ap_id = goal_url(&self.base_url, user_id.value(), year);
|
||||
@@ -519,11 +513,7 @@ impl ActivityPubEventHandler {
|
||||
.federation_settings
|
||||
.get_federation_flags(user_id)
|
||||
.await
|
||||
.unwrap_or(domain::models::FederationFlags {
|
||||
goals: true,
|
||||
reviews: true,
|
||||
watchlist: true,
|
||||
});
|
||||
.unwrap_or_default();
|
||||
if !flags.goals {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
@@ -4,7 +4,7 @@ use async_trait::async_trait;
|
||||
use chrono::DateTime;
|
||||
use domain::{
|
||||
models::RemoteGoalEntry,
|
||||
ports::{LocalApContentQuery, RemoteGoalRepository},
|
||||
ports::{GoalRepository, RemoteGoalRepository},
|
||||
value_objects::UserId,
|
||||
};
|
||||
use k_ap::{ApContentReader, ApObjectHandler};
|
||||
@@ -15,7 +15,7 @@ use crate::urls::{actor_url, goal_url};
|
||||
|
||||
pub struct GoalObjectHandler {
|
||||
pub remote_goal_repo: Arc<dyn RemoteGoalRepository>,
|
||||
pub content_query: Arc<dyn LocalApContentQuery>,
|
||||
pub goal_repo: Arc<dyn GoalRepository>,
|
||||
pub base_url: String,
|
||||
}
|
||||
|
||||
@@ -29,8 +29,8 @@ impl ApContentReader for GoalObjectHandler {
|
||||
) -> anyhow::Result<Vec<(Url, serde_json::Value, DateTime<chrono::Utc>)>> {
|
||||
let uid = UserId::from_uuid(user_id);
|
||||
let goals = self
|
||||
.content_query
|
||||
.list_goals_for_user(&uid)
|
||||
.goal_repo
|
||||
.list_for_user(&uid)
|
||||
.await
|
||||
.map_err(|e| anyhow::anyhow!(e.to_string()))?;
|
||||
|
||||
|
||||
@@ -51,6 +51,11 @@ pub struct ActivityPubDeps {
|
||||
pub remote_watchlist_repo: std::sync::Arc<dyn domain::ports::RemoteWatchlistRepository>,
|
||||
pub remote_goal_repo: std::sync::Arc<dyn domain::ports::RemoteGoalRepository>,
|
||||
pub local_ap_content: std::sync::Arc<dyn domain::ports::LocalApContentQuery>,
|
||||
pub movie_repo: std::sync::Arc<dyn domain::ports::MovieRepository>,
|
||||
pub review_repo: std::sync::Arc<dyn domain::ports::ReviewRepository>,
|
||||
pub diary_repo: std::sync::Arc<dyn domain::ports::DiaryRepository>,
|
||||
pub goal_repo: std::sync::Arc<dyn domain::ports::GoalRepository>,
|
||||
pub stats_repo: std::sync::Arc<dyn domain::ports::StatsRepository>,
|
||||
pub user_repo: std::sync::Arc<dyn domain::ports::UserRepository>,
|
||||
pub federation_settings: std::sync::Arc<dyn domain::ports::UserFederationSettingsQuery>,
|
||||
pub base_url: String,
|
||||
@@ -68,6 +73,11 @@ pub async fn wire(deps: ActivityPubDeps) -> anyhow::Result<ActivityPubWire> {
|
||||
remote_watchlist_repo,
|
||||
remote_goal_repo,
|
||||
local_ap_content,
|
||||
movie_repo,
|
||||
review_repo,
|
||||
diary_repo,
|
||||
goal_repo,
|
||||
stats_repo,
|
||||
user_repo,
|
||||
federation_settings,
|
||||
base_url,
|
||||
@@ -76,6 +86,8 @@ pub async fn wire(deps: ActivityPubDeps) -> anyhow::Result<ActivityPubWire> {
|
||||
} = deps;
|
||||
let review_handler = std::sync::Arc::new(ReviewObjectHandler {
|
||||
content_query: std::sync::Arc::clone(&local_ap_content),
|
||||
movie_repo: std::sync::Arc::clone(&movie_repo),
|
||||
diary_repo,
|
||||
review_store,
|
||||
event_publisher: std::sync::Arc::clone(&event_publisher),
|
||||
base_url: base_url.clone(),
|
||||
@@ -87,7 +99,7 @@ pub async fn wire(deps: ActivityPubDeps) -> anyhow::Result<ActivityPubWire> {
|
||||
});
|
||||
let goal_handler = std::sync::Arc::new(goal_handler::GoalObjectHandler {
|
||||
remote_goal_repo,
|
||||
content_query: std::sync::Arc::clone(&local_ap_content),
|
||||
goal_repo: std::sync::Arc::clone(&goal_repo),
|
||||
base_url: base_url.clone(),
|
||||
});
|
||||
let composite = std::sync::Arc::new(composite_handler::CompositeObjectHandler {
|
||||
@@ -136,6 +148,10 @@ pub async fn wire(deps: ActivityPubDeps) -> anyhow::Result<ActivityPubWire> {
|
||||
let event_handler = std::sync::Arc::new(ActivityPubEventHandler::new(
|
||||
std::sync::Arc::clone(&concrete),
|
||||
local_ap_content,
|
||||
review_repo,
|
||||
movie_repo,
|
||||
goal_repo,
|
||||
stats_repo,
|
||||
federation_settings,
|
||||
base_url,
|
||||
)) as std::sync::Arc<dyn domain::ports::EventHandler>;
|
||||
|
||||
@@ -4,7 +4,7 @@ use async_trait::async_trait;
|
||||
use domain::{
|
||||
events::DomainEvent,
|
||||
models::ReviewSource,
|
||||
ports::{EventPublisher, LocalApContentQuery},
|
||||
ports::{DiaryRepository, EventPublisher, LocalApContentQuery, MovieRepository},
|
||||
value_objects::{Comment, ExternalMetadataId, MovieId, Rating, ReviewId, UserId},
|
||||
};
|
||||
use k_ap::{ApContentReader, ApObjectHandler};
|
||||
@@ -16,6 +16,8 @@ use crate::urls::{actor_url, review_url};
|
||||
|
||||
pub struct ReviewObjectHandler {
|
||||
pub content_query: Arc<dyn LocalApContentQuery>,
|
||||
pub movie_repo: Arc<dyn MovieRepository>,
|
||||
pub diary_repo: Arc<dyn DiaryRepository>,
|
||||
pub review_store: Arc<dyn RemoteReviewRepository>,
|
||||
pub event_publisher: Arc<dyn EventPublisher>,
|
||||
pub base_url: String,
|
||||
@@ -69,7 +71,7 @@ impl ApContentReader for ReviewObjectHandler {
|
||||
}
|
||||
|
||||
async fn count_local_posts(&self) -> anyhow::Result<u64> {
|
||||
self.content_query
|
||||
self.diary_repo
|
||||
.count_local_posts()
|
||||
.await
|
||||
.map_err(|e| anyhow::anyhow!(e.to_string()))
|
||||
@@ -97,13 +99,18 @@ impl ApObjectHandler for ReviewObjectHandler {
|
||||
let actor_url_str = obj.attributed_to.to_string();
|
||||
let review_id = ReviewId::generate();
|
||||
let movie_id = if let Some(ref ext_id) = obj.external_metadata_id {
|
||||
match self
|
||||
.content_query
|
||||
.get_movie_by_external_metadata_id(ext_id)
|
||||
.await
|
||||
{
|
||||
Ok(Some(movie)) => movie.id().clone(),
|
||||
_ => MovieId::from_uuid(uuid::Uuid::new_v5(
|
||||
let found = if let Ok(ext_meta_id) = ExternalMetadataId::new(ext_id.clone()) {
|
||||
self.movie_repo
|
||||
.get_movie_by_external_id(&ext_meta_id)
|
||||
.await
|
||||
.ok()
|
||||
.flatten()
|
||||
} else {
|
||||
None
|
||||
};
|
||||
match found {
|
||||
Some(movie) => movie.id().clone(),
|
||||
None => MovieId::from_uuid(uuid::Uuid::new_v5(
|
||||
&uuid::Uuid::NAMESPACE_URL,
|
||||
ext_id.as_bytes(),
|
||||
)),
|
||||
|
||||
Reference in New Issue
Block a user