Refactor Expense towards Jerre's 'pragmatic' model

[ZzAve]

---

Description of Changes

This pull request includes a comprehensive refactor across multiple modules:

• Migrated ExpenseService, CostExpenseService, and TravelExpenseService to proper domain modules, ensuring a more logical separation of concerns.

• Introduced domain models for User, Person, and Expense to centralize business logic within the domain layer.

• Implemented a temporary WirespecResponseHeaderAdvice to facilitate response header mapping.

• Updated the handling of Wirespec Pageable for improved integration.

• Reorganized files within the application module, consolidating expense-related logic under a dedicated package.

[ZzAve]

Review from Claude:

This is a substantial architectural refactoring that introduces Hexagonal Architecture (Ports & Adapters pattern) to the expense management domain. Overall, this is high-quality work that significantly improves code organization and testability.

📋 Summary of Changes

New domain module created with:

• Pure domain models (CostExpense, TravelExpense, Person, User)
• Port interfaces defining boundaries (CostExpensePersistencePort, ExpensePersistencePort, etc.)
• Domain services with business logic (CostExpenseService, TravelExpenseService, ExpenseService)
• Custom event system moved to domain layer

Application layer updated with:

• Adapter implementations (CostExpensePersistenceAdapter, ExpensePersistenceAdapter)
• Mappers between domain models and JPA entities
• Controllers refactored to use domain services

---

✅ Strengths

1. Excellent separation of concerns: Domain module has ZERO Spring/Jakarta dependencies ✓
2. Clear architectural boundaries: Port interfaces clearly define contracts
3. Consistent naming conventions: *Port for interfaces, *Adapter for implementations
4. Immutable domain models: Using Kotlin data classes appropriately
5. Good performance: Using EntityManager.getReference() avoids unnecessary queries in CostExpensePersistenceAdapter.kt:17
6. Tests properly updated: See CostExpenseMailServiceTest.kt:25-33 using domain models

---

⚠️ Issues & Recommendations

🔴 Critical: Domain Purity Violation

Location: domain/src/main/kotlin/community/flock/eco/workday/domain/common/ApplicationEvent.kt:5

  abstract class ApplicationEvent : EventObject {

Issue: Domain layer extends java.util.EventObject, coupling it to Java's event system.

Fix: Use your own Event interface or create a pure domain base class:
abstract class ApplicationEvent {
abstract val source: Any
abstract val timestamp: Instant
}

---

🟡 Medium: Mappers Have Service Dependencies

Location: workday-application/src/main/kotlin/community/flock/eco/workday/application/expense/ExpenseMapper.kt:22,43

  @Component
  class CostExpenseMapper(
      private val personService: PersonService,
  ) {
      fun consume(input: CostExpenseInput, id: UUID? = null) = CostExpense(
          person = personService.findByUuid(...)?.toDomain()
              ?: error("Cannot find person")
      )
  }

Issues:

1. Mappers should be pure functions, not have service dependencies
2. Error handling uses generic error() instead of domain exceptions
3. Person lookup happens in mapper instead of controller/service

Recommendation:

  // Make mapper pure
  class CostExpenseMapper {
      fun consume(input: CostExpenseInput, person: Person, id: UUID? = null) =
          CostExpense(person = person, ...)
  }

  // Move person lookup to controller or create a dedicated service

---

🟡 Medium: Incomplete Error Handling

Location: ExpenseMapper.kt:38,67

  ?: error("Cannot find person")

Issue: Throws generic IllegalStateException. API consumers get unhelpful errors.

Fix: Create domain exceptions:

  sealed class DomainException(message: String) : Exception(message)
  class PersonNotFoundException(personId: UUID) : DomainException("Person $personId not found")

  // Then:
  ?: throw PersonNotFoundException(UUID.fromString(input.personId.value))

---

🟡 Medium: UUID Parsing Safety

Location: ExpenseController.kt:73,97,112 (multiple places)

  val id = request.path.id.let(UUID::fromString)

Issue: Can throw IllegalArgumentException if UUID is malformed.

Recommendation: Validate at API boundary or add error handling:

  val id = try {
      UUID.fromString(request.path.id)
  } catch (e: IllegalArgumentException) {
      return ExpenseById.Response400(Error("Invalid UUID format"))
  }

---

🟢 Low: ApplicationEvent Timestamp Should Be Non-Nullable

Location: domain/src/main/kotlin/community/flock/eco/workday/domain/common/ApplicationEvent.kt:8

  val timestamp: Instant?

Issue: Events should always have timestamps.

Fix: val timestamp: Instant (remove nullable)

---

🟢 Low: Resolve Delete Event TODO

Location: domain/src/main/kotlin/community/flock/eco/workday/domain/expense/ExpenseService.kt:42-44

  // TODO: nobody listens to this event, can we remove?
  //  especially since 'this' is removed from the hibernate cache
  applicationEventPublisher.publishEvent(DeleteExpenseEvent(it))

Recommendation: If nobody listens, remove it. Publishing events for deleted entities is semantically odd (they're already gone). If needed for auditing, publish BEFORE deletion.

---

🟢 Low: Missing Documentation

Port interfaces and domain services lack KDoc comments. Add documentation for:

• Port contracts
• Business rules in services
• Complex mapping logic

Example:

  /**
   * Port for persisting cost expenses.
   * Implementations should handle entity-domain conversion.
   */
  interface CostExpensePersistencePort {
      /**
       * Creates a new cost expense.
       * @throws PersistenceException if creation fails
       */
      fun create(costExpense: CostExpense): CostExpense
  }

---

🔒 Security

No security vulnerabilities introduced. Authorization checks remain properly in controller layer:

• ExpenseController.kt:71,95,110 - @PreAuthorize annotations present ✓
• ExpenseController.kt:229-237 - Authentication checks for ownership ✓

---

⚡ Performance

• Good: Using EntityManager.getReference() in adapters avoids extra queries ✓
• Note: Consider if Person entity needs full loading in mappers (see mapper issue above)

---

📐 Project Conventions

✅ Follows documented conventions from CLAUDE.md:

• Business logic correctly placed in workday-application (not in vendored modules)
• Package structure follows community.flock.eco.workday.* pattern
• Proper module dependency: workday-application → domain

---

🎯 Overall Assessment

Rating: ⭐⭐⭐⭐ (4/5)

This is a well-executed architectural improvement that successfully implements Hexagonal Architecture with clear separation between domain and infrastructure. The refactoring significantly improves maintainability and testability.

To make it excellent, address:

1. Remove EventObject dependency from domain (breaks domain purity)
2. Refactor mappers to pure functions
3. Improve error handling with domain exceptions
4. Add documentation to ports and services

Bottom line: Merge-worthy with the issues noted above tracked for follow-up work. The architectural foundation is solid. 🚀