Aman-Mittal commented on PR #39:
URL:
https://github.com/apache/fineract-consumer-facing/pull/39#issuecomment-4978005248
# CQRS & Spring Modulith Analysis — PR #39
> Covers both the **existing codebase** (what's in `main`) and **what PR #39
adds** (beneficiaries + access control refactor).
---
## 1. Module Map (as-built)
```mermaid
graph TD
auth[authentication]
loans[loans]
savings[savings]
transfers[transfers]
registration[registration]
user[user]
otp[otp]
infra[infrastructure]
auth --> user
auth --> otp
loans --> user
loans --> otp
savings --> user
savings --> otp
transfers --> user
transfers --> otp
registration --> user
registration --> otp
infra --> user
auth --> infra
loans --> infra
savings --> infra
transfers --> infra
registration --> infra
user --> infra
otp --> infra
```
**Key facts from static analysis:**
- 7 business modules + 1 infrastructure module
- `user` and `otp` are **shared kernel** modules — every other module
depends on them
- No module depends directly on `savings`, `loans`, or `transfers` ✅ (no
cyclical business deps)
- `infrastructure` itself imports from `user` — a mild inversion
---
## 2. CQRS Assessment
### ✅ What the Project Does Well
#### 2.1 Package-level command/query split
Every module follows the convention:
```
{module}/
command/
api/ ← write-side controller
data/ ← command objects + response DTOs
domain/ ← aggregates (JPA entities)
exception/
repository/ ← JpaRepository (full write access)
service/ ← implements command use cases
query/
api/ ← read-side controller
data/ ← projection DTOs
exception/
repository/ ← read-only Repository (not JpaRepository)
service/ ← implements query use cases
```
This is the correct structural intent for CQRS in a monolith.
#### 2.2 Separate repository interfaces with different base types
`user` module (best example in the codebase):
| Side | Interface | Extends |
|---|---|---|
| Command | `UserCommandRepository` | `JpaRepository<User, Long>` |
| Query | `UserQueryRepository` | `Repository<User, Long>` (read-only) |
`UserQueryRepository` uses **JPQL constructor expressions** to project
directly into DTOs:
```java
@Query("SELECT new UserQueryData(u.id, u.publicId, ...) FROM User u WHERE
u.publicId = :publicId")
Optional<UserQueryData> findByPublicId(UUID publicId);
```
This is **textbook CQRS projection** — the query side never exposes the
domain entity.
#### 2.3 `@Command` and `@Query` marker annotations
Both are defined as custom method-level annotations. They serve as
documentation markers and potential AOP pointcut hooks (though no AOP is wired
currently — see finding 2.5).
#### 2.4 Separate data shapes — commands are not recycled as queries
Command DTOs (`InitiateAddBeneficiaryCommand`,
`ConfirmAddBeneficiaryCommand`) are distinct from query DTOs
(`BeneficiaryQueryData`). No shared mutable DTO is passed across the boundary. ✅
---
### ❌ CQRS Violations
#### 2.5 `@Command` / `@Query` annotations are unused markers — no
enforcement
```java
@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface Command {}
```
These annotations have **no AOP advice, no validator, and no framework
hook** attached. They are documentation-only. SonarQube rule `java:S1144` will
flag them as unused. More importantly:
- No enforcement that `@Command` methods are in a `command` package
- No enforcement that `@Query` methods don't mutate state
- No `@Transactional(readOnly = true)` auto-applied to `@Query` methods
**Recommendation:** Wire an AOP aspect or use Spring's `@Transactional`
composition:
```java
// Replace @Query annotation with a composed annotation
@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
@Transactional(readOnly = true)
public @interface Query {}
// Replace @Command with
@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
@Transactional
public @interface Command {}
```
This makes the annotation meaningful and removes the need to manually add
`@Transactional` everywhere.
---
#### 2.6 PR #39 — `BeneficiaryQueryRepository` extends `JpaRepository`
instead of `Repository`
```java
// BeneficiaryQueryRepository.java (added in PR #39)
public interface BeneficiaryQueryRepository extends
JpaRepository<Beneficiary, Long> { // ❌
List<Beneficiary> findAllByUserIdAndActiveTrueOrderByNameAsc(Long
userId);
Optional<Beneficiary>
findByUserIdAndFineractAccountIdAndAccountTypeAndActiveTrue(...);
}
```
Compare with the correct pattern from `user` module:
```java
// UserQueryRepository.java (correct)
public interface UserQueryRepository extends Repository<User, Long> { // ✅
```
By extending `JpaRepository`, `BeneficiaryQueryRepository` inherits
`save()`, `delete()`, `deleteAll()`, `flush()` — full write access — on the
**query side**. This breaks the CQRS read/write segregation. The read side
should **never** be able to mutate state.
**Fix:**
```java
public interface BeneficiaryQueryRepository extends Repository<Beneficiary,
Long> {
List<Beneficiary> findAllByUserIdAndActiveTrueOrderByNameAsc(Long
userId);
Optional<Beneficiary>
findByUserIdAndFineractAccountIdAndAccountTypeAndActiveTrue(...);
}
```
---
#### 2.7 PR #39 — `BeneficiaryQueryRepository` imports the command-side
domain entity
```java
// BeneficiaryQueryRepository.java
import
org.apache.fineract.consumer.beneficiaries.command.domain.Beneficiary; // ←
command package
import
org.apache.fineract.consumer.beneficiaries.command.domain.BeneficiaryAccountType;
// ← command package
public interface BeneficiaryQueryRepository extends
JpaRepository<Beneficiary, Long> { ... }
```
The query side is reading the **write-side aggregate**. In strict CQRS, the
query side should have its own read model or use projections into DTOs directly
(like `UserQueryRepository` does). The entity class belongs to the
command/write model.
**Recommendation:** Either:
1. Move `Beneficiary` entity to a shared `beneficiaries/domain/` package
(not under `command/`), or
2. Use JPQL constructor projections like `UserQueryRepository` does —
project directly into `BeneficiaryQueryData` without exposing the entity to the
query side at all.
---
#### 2.8 `UserQueryService` is called from command-side services of other
modules
```java
// AuthenticationCommandServiceImpl.java (command side)
import org.apache.fineract.consumer.user.query.service.UserQueryService; //
← query service
// BeneficiariesCommandServiceImpl.java (command side)
private final UserQueryService userQueryService; // ← query service in
command handler
UserQueryData user = userQueryService.findByPublicId(publicId); // ← read
in command
```
This is a practical and widely-accepted pattern in "pragmatic CQRS" — the
command handler reads state before mutating it. However, it means:
- Command handlers **bypass** their own read model and call into another
module's query service
- The `user` module's query service becomes a synchronous dependency of
every command handler in the system
**The concern is not correctness but coupling.** If `UserQueryService`
changes its interface or becomes async, every command handler breaks. This
would be better expressed via domain events or an anti-corruption layer.
---
#### 2.9 No read model / materialized view — queries hit the same write store
In all modules, both `CommandRepository` and `QueryRepository` target the
**same PostgreSQL tables**. This is fine for a monolith with moderate load, but
it means:
- Queries contend with writes on the same rows/indexes
- No optimised query-side schema (e.g., denormalized views for listing)
- The `@Transactional(readOnly = true)` on query methods (when present) is
the only isolation mechanism
This is a known trade-off for "simple CQRS" — not a bug, but worth
documenting as an architectural decision.
---
#### 2.10 `BeneficiariesCommandServiceImpl.initiateAdd` is `@Command` but
has no `@Transactional`
```java
@Override
@Command // ← implies a write operation
// ❌ missing @Transactional — but reads from repository
public BeneficiaryChallengeCommandData initiateAdd(Jwt jwt,
InitiateAddBeneficiaryCommand command) {
requireNameAvailable(user.getId(), command.getName()); // ← DB read
resolveAccount(...); // ← Feign calls
...
}
```
`confirmAdd` and `delete` have `@Transactional`, but `initiateAdd` and
`initiateUpdate` do not. Since `@Command` is an unenforced marker (finding
2.5), the inconsistency goes undetected.
---
## 3. Spring Modulith Assessment
### What is Spring Modulith?
Spring Modulith is the Spring-native framework for modular monoliths. It
provides:
- `@ApplicationModule` — module boundary declarations
- `ApplicationModuleTest` — per-module integration tests with mocked
neighbours
- Module dependency verification via
`ApplicationModules.of(App.class).verify()`
- `@NamedInterface` — explicit public API contracts
- Event-based inter-module communication via `@ApplicationModuleListener`
---
### ❌ Spring Modulith is Not Used
The project has `@SpringBootApplication` with a flat classpath scan:
```java
@SpringBootApplication
@EnableFeignClients(basePackages = "org.apache.fineract.consumer", ...)
public class ConsumerApplication { ... }
```
There is **no Spring Modulith dependency** in `build.gradle`, no
`ApplicationModules.verify()`, and no `@ApplicationModuleTest`. This means:
| Modulith Feature | Status |
|---|---|
| Module boundary enforcement | ❌ Not enforced |
| Circular dependency detection | ❌ Not detected |
| Per-module integration tests | ❌ Absent |
| Named public API | ❌ Everything is public |
| Event-based decoupling | ❌ Direct synchronous calls only |
| Module documentation | ❌ No generated diagrams |
---
### What the Project Has Instead (Manual Modulith)
The package structure is the *intent* of Modulith — each module is a
top-level package. But without the framework, enforcement is purely by
convention:
```
consumer/
authentication/ ← module (by convention)
loans/ ← module (by convention)
savings/ ← module (by convention)
transfers/ ← module (by convention)
user/ ← shared kernel (by convention)
otp/ ← shared kernel (by convention)
infrastructure/ ← technical cross-cutting (by convention)
```
Nothing stops a developer from importing
`authentication.command.service.AuthenticationCommandServiceImpl` directly from
a `savings` class — the Java compiler will not complain.
---
### Specific Modulith Problems in PR #39
#### 3.1 `ActionPolicies` references exception types from 4 different
business modules
```java
// ActionPolicies.java (infrastructure/access/data)
import
org.apache.fineract.consumer.loans.command.exception.LoanCommandAccessDeniedException;
import
org.apache.fineract.consumer.loans.query.exception.LoanQueryAccessDeniedException;
import
org.apache.fineract.consumer.savings.command.exception.SavingsCommandAccessDeniedException;
import
org.apache.fineract.consumer.savings.query.exception.SavingsQueryAccessDeniedException;
import
org.apache.fineract.consumer.transfers.command.exception.TransferAccessDeniedException;
```
`ActionPolicies` is in the `infrastructure` layer but hard-codes exception
types from `loans`, `savings`, and `transfers`. Adding a new module means
editing `ActionPolicies` — this is the **Open/Closed Principle violation** in
terms of Modulith: the central policy knows about all leaf modules.
**With Spring Modulith**, each module would register its own policies via an
`ApplicationEvent` or a `@Bean` contribution, and `ActionPolicies` would
discover them rather than hard-code them.
**Recommended pattern:**
```java
// Each module contributes its own policy beans
// loans module:
@Bean
ActionPolicy loanViewPolicy() {
return ActionPolicy.builder()
.action(ConsumerAction.LOANS_VIEW)
.requiredScope(SCOPE_CONSUMER_FULL)
.requiresKycVerified(true)
.ownership(ResourceType.LOANS)
.denialException(LoanQueryAccessDeniedException::new)
.build();
}
```
---
#### 3.2 No `module-info.java` or `@NamedInterface` — no public API contract
Any class in any package is visible to any other module. The
`command.service.*Impl` classes (which should be internal) are as accessible as
the `command.service.*Service` interfaces (which are the intended public API).
**With Spring Modulith**, you would annotate:
```java
// In beneficiaries module root package
@org.springframework.modulith.ApplicationModule(
allowedDependencies = {"user", "otp", "infrastructure"}
)
package org.apache.fineract.consumer.beneficiaries;
```
And mark public interfaces:
```java
@org.springframework.modulith.NamedInterface("beneficiaries-command")
package org.apache.fineract.consumer.beneficiaries.command.service;
```
---
#### 3.3 `infrastructure` module imports from `user` module — dependency
inversion missing
```java
// UserClientResolver.java (infrastructure/web)
import org.apache.fineract.consumer.user.query.service.UserQueryService;
```
`infrastructure` should be a leaf that nothing else depends on. It should
not depend on a business module (`user`). This inverts the intended dependency
direction.
**Fix:** `UserClientResolver` belongs in the `user` module's query layer,
not in `infrastructure.web`. It uses `UserQueryService` — it is a user-module
concern.
---
#### 3.4 No inter-module events — all communication is synchronous method
calls
Modules talk to each other only via direct Spring bean injection. For
example, when `authentication` verifies a login and wants to notify `otp` to
send a code — it calls `OtpCommandService.createOtp()` directly.
```java
// AuthenticationCommandServiceImpl
otpCommandService.createOtp(publicId, OtpDestination.builder()...build());
```
In Spring Modulith, cross-module side effects should be triggered via domain
events:
```java
// authentication module publishes an event
events.publishEvent(new LoginChallengeIssuedEvent(publicId, userEmail));
// otp module listens
@ApplicationModuleListener
void on(LoginChallengeIssuedEvent event) {
otpCommandService.createOtp(event.publicId(), ...);
}
```
This decouples the modules — `authentication` no longer needs to know about
`otp` at compile time.
---
#### 3.5 DB migrations are not module-scoped
All Liquibase changesets live in a single `db/changelog/` directory:
```
db/changelog/
changes/
001-create-users-table.yaml ← user module
002-create-refresh-tokens.yaml ← authentication module
```
PR #39 adds a `beneficiaries` table migration, but it goes into the same
flat directory. In Spring Modulith, each module owns its migrations:
```
authentication/src/main/resources/db/authentication/
user/src/main/resources/db/user/
beneficiaries/src/main/resources/db/beneficiaries/
```
---
## 4. Summary Table
| Dimension | Criterion | Status | Severity |
|---|---|---|---|
| **CQRS** | Command/query package split | ✅ Present | — |
| **CQRS** | Separate repo base types (command: JPA, query: read-only) | ⚠️
Violated in PR #39 | Major |
| **CQRS** | Query repos project to DTOs (no entity leak) | ⚠️ Violated in
PR #39 | Major |
| **CQRS** | `@Command`/`@Query` annotations enforced | ❌ Markers only |
Minor |
| **CQRS** | `@Transactional` on all command/query methods | ❌ Inconsistent
| Major |
| **CQRS** | No write operations on query side | ❌ JpaRepository on query
repo | Major |
| **Modulith** | Spring Modulith dependency used | ❌ Not present | — |
| **Modulith** | Module boundary verification | ❌ None | Major |
| **Modulith** | Named public interfaces | ❌ None | Minor |
| **Modulith** | Event-based cross-module communication | ❌ All synchronous
| Minor |
| **Modulith** | Per-module DB migrations | ❌ Flat directory | Minor |
| **Modulith** | `infrastructure` has no upward deps | ❌ Imports `user` |
Major |
| **Modulith** | `ActionPolicies` closed to module additions | ❌ Hard-codes
all modules | Major |
---
## 5. Recommended Next Steps
### Immediate (in this PR)
1. **Fix `BeneficiaryQueryRepository`** — extend `Repository<Beneficiary,
Long>` instead of `JpaRepository`
2. **Move `Beneficiary` entity** to `beneficiaries/domain/` (shared between
command and query packages within the module)
3. **Add `@Transactional(readOnly = true)`** to `initiateAdd` and
`initiateUpdate`
4. **Move `UserClientResolver`** from `infrastructure.web` into the `user`
module
### Near-term (separate PRs)
5. **Add Spring Modulith** (`spring-modulith-starter-core`) and run
`ApplicationModules.of(ConsumerApplication.class).verify()` in a test — it will
surface illegal dependencies automatically
6. **Convert `@Command`/`@Query`** to composed annotations that carry
`@Transactional` semantics
7. **Refactor `ActionPolicies`** to accept `ActionPolicy` beans from each
module rather than hard-coding them
### Long-term
8. **Introduce domain events** for cross-module side effects (OTP sending,
KYC checks)
9. **Scope DB migrations per module** using Liquibase's `include` directive
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
