626 lines
19 KiB
Markdown
626 lines
19 KiB
Markdown
# BFF Architecture Review - Logic Overlaps & Patterns
|
|
|
|
**Date**: October 8, 2025
|
|
**Scope**: Backend For Frontend (BFF) service architecture and validation patterns
|
|
**Status**: 🟡 Generally Good - Minor Improvements Recommended
|
|
|
|
---
|
|
|
|
## Executive Summary
|
|
|
|
The BFF layer demonstrates **good separation of concerns** with proper use of validation pipes and domain schemas. However, there are a few areas where business logic could be better organized and some potential duplication in validation concerns.
|
|
|
|
### Key Findings
|
|
|
|
✅ **Strengths**:
|
|
- Controllers use `ZodValidationPipe` consistently
|
|
- Domain schemas are imported and reused (no schema duplication in BFF)
|
|
- Clear service layer separation (orchestrator, validator, builder patterns)
|
|
- Infrastructure validation properly isolated in BFF services
|
|
|
|
⚠️ **Concerns**:
|
|
- Payment validation duplicated across services (2 locations)
|
|
- SIM business logic mixed with infrastructure concerns
|
|
- Some business rules could be moved to domain layer
|
|
|
|
---
|
|
|
|
## 🔍 Validation Pattern Analysis
|
|
|
|
### ✅ Correct Patterns Observed
|
|
|
|
#### 1. Controller Input Validation
|
|
All controllers properly use `ZodValidationPipe` with domain schemas:
|
|
|
|
```typescript:20:22:apps/bff/src/modules/orders/orders.controller.ts
|
|
@Post()
|
|
@UsePipes(new ZodValidationPipe(createOrderRequestSchema))
|
|
async create(@Request() req: RequestWithUser, @Body() body: CreateOrderRequest) {
|
|
```
|
|
|
|
```typescript:125:127:apps/bff/src/modules/auth/presentation/http/auth.controller.ts
|
|
@UsePipes(new ZodValidationPipe(validateSignupRequestSchema))
|
|
async validateSignup(@Body() validateData: ValidateSignupRequest, @Req() req: Request) {
|
|
```
|
|
|
|
```typescript:151:152:apps/bff/src/modules/subscriptions/subscriptions.controller.ts
|
|
@UsePipes(new ZodValidationPipe(simTopupRequestSchema))
|
|
async topUpSim(
|
|
```
|
|
|
|
**Assessment**: ✅ Excellent pattern, consistent across all controllers
|
|
|
|
---
|
|
|
|
#### 2. Domain Schema Imports
|
|
BFF properly imports schemas from domain package:
|
|
|
|
```typescript:6:11:apps/bff/src/modules/orders/orders.controller.ts
|
|
import {
|
|
createOrderRequestSchema,
|
|
sfOrderIdParamSchema,
|
|
type CreateOrderRequest,
|
|
type SfOrderIdParam,
|
|
} from "@customer-portal/domain/orders";
|
|
```
|
|
|
|
```typescript:14:16:apps/bff/src/modules/users/users.controller.ts
|
|
import {
|
|
updateCustomerProfileRequestSchema,
|
|
type UpdateCustomerProfileRequest,
|
|
} from "@customer-portal/domain/auth";
|
|
```
|
|
|
|
**Assessment**: ✅ No schema duplication in BFF layer
|
|
|
|
---
|
|
|
|
#### 3. Infrastructure Validation in Services
|
|
BFF services handle infrastructure-dependent validation (DB, API calls):
|
|
|
|
```typescript:84:104:apps/bff/src/modules/orders/services/order-validator.service.ts
|
|
async validateUserMapping(
|
|
userId: string
|
|
): Promise<{ userId: string; sfAccountId?: string; whmcsClientId: number }> {
|
|
const mapping = await this.mappings.findByUserId(userId);
|
|
|
|
if (!mapping) {
|
|
this.logger.warn({ userId }, "User mapping not found");
|
|
throw new BadRequestException("User account mapping is required before ordering");
|
|
}
|
|
|
|
if (!mapping.whmcsClientId) {
|
|
this.logger.warn({ userId, mapping }, "WHMCS client ID missing from mapping");
|
|
throw new BadRequestException("WHMCS integration is required before ordering");
|
|
}
|
|
|
|
return {
|
|
userId: mapping.userId,
|
|
sfAccountId: mapping.sfAccountId || undefined,
|
|
whmcsClientId: mapping.whmcsClientId,
|
|
};
|
|
}
|
|
```
|
|
|
|
**Assessment**: ✅ Correct - infrastructure checks belong in BFF
|
|
|
|
---
|
|
|
|
## 🟡 Areas for Improvement
|
|
|
|
### 1. **Payment Method Validation Duplication** (Priority: MEDIUM)
|
|
|
|
**Problem**: Payment validation logic exists in two places:
|
|
|
|
| Service | File | Lines |
|
|
|---------|------|-------|
|
|
| `OrderValidator` | `apps/bff/src/modules/orders/services/order-validator.service.ts` | 109-122 |
|
|
| `OrderFulfillmentValidator` | `apps/bff/src/modules/orders/services/order-fulfillment-validator.service.ts` | ~130-150 (private method) |
|
|
|
|
**Current Implementation (OrderValidator)**:
|
|
```typescript:109:122:apps/bff/src/modules/orders/services/order-validator.service.ts
|
|
async validatePaymentMethod(userId: string, whmcsClientId: number): Promise<void> {
|
|
try {
|
|
const pay = await this.whmcs.getPaymentMethods({ clientid: whmcsClientId });
|
|
const paymentMethods = Array.isArray(pay.paymethods) ? pay.paymethods : [];
|
|
if (paymentMethods.length === 0) {
|
|
this.logger.warn({ userId }, "No WHMCS payment method on file");
|
|
throw new BadRequestException("A payment method is required before ordering");
|
|
}
|
|
} catch (e: unknown) {
|
|
const err = getErrorMessage(e);
|
|
this.logger.error({ err }, "Payment method verification failed");
|
|
throw new BadRequestException("Unable to verify payment method. Please try again later.");
|
|
}
|
|
}
|
|
```
|
|
|
|
**Impact**:
|
|
- Logic duplicated in two validators
|
|
- Changes must be made in two places
|
|
- Potential for inconsistent error messages
|
|
|
|
**Recommendation**:
|
|
Create a shared payment validation service:
|
|
|
|
```typescript
|
|
// apps/bff/src/modules/orders/services/payment-validator.service.ts
|
|
@Injectable()
|
|
export class PaymentValidatorService {
|
|
constructor(
|
|
private readonly whmcs: WhmcsConnectionOrchestratorService,
|
|
@Inject(Logger) private readonly logger: Logger
|
|
) {}
|
|
|
|
async validatePaymentMethodExists(userId: string, whmcsClientId: number): Promise<void> {
|
|
try {
|
|
const pay = await this.whmcs.getPaymentMethods({ clientid: whmcsClientId });
|
|
const paymentMethods = Array.isArray(pay.paymethods) ? pay.paymethods : [];
|
|
|
|
if (paymentMethods.length === 0) {
|
|
this.logger.warn({ userId, whmcsClientId }, "No payment method on file");
|
|
throw new BadRequestException("A payment method is required before ordering");
|
|
}
|
|
} catch (e: unknown) {
|
|
if (e instanceof BadRequestException) throw e;
|
|
|
|
const err = getErrorMessage(e);
|
|
this.logger.error({ err, userId }, "Payment method verification failed");
|
|
throw new BadRequestException("Unable to verify payment method. Please try again later.");
|
|
}
|
|
}
|
|
}
|
|
|
|
// Then both validators inject and use it
|
|
constructor(private readonly paymentValidator: PaymentValidatorService) {}
|
|
|
|
async validatePaymentMethod(userId: string, whmcsClientId: number): Promise<void> {
|
|
return this.paymentValidator.validatePaymentMethodExists(userId, whmcsClientId);
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### 2. **SIM Business Logic in BFF Service** (Priority: MEDIUM)
|
|
|
|
**Problem**: `SimValidationService` contains business rules that should be in domain layer.
|
|
|
|
**File**: `apps/bff/src/modules/subscriptions/sim-management/services/sim-validation.service.ts`
|
|
|
|
**Current Issues**:
|
|
|
|
#### Issue 2a: Hardcoded Business Rules
|
|
```typescript:29:35:apps/bff/src/modules/subscriptions/sim-management/services/sim-validation.service.ts
|
|
// Check if this is a SIM service
|
|
const isSimService =
|
|
subscription.productName.toLowerCase().includes("sim") ||
|
|
subscription.groupName?.toLowerCase().includes("sim");
|
|
|
|
if (!isSimService) {
|
|
throw new BadRequestException("This subscription is not a SIM service");
|
|
}
|
|
```
|
|
|
|
**Issue**: Business rule (what makes a subscription a "SIM service") is in BFF layer, not domain.
|
|
|
|
**Should be**:
|
|
```typescript
|
|
// packages/domain/subscriptions/validation.ts
|
|
export function isSimSubscription(subscription: Subscription): boolean {
|
|
return (
|
|
subscription.productName.toLowerCase().includes("sim") ||
|
|
subscription.groupName?.toLowerCase().includes("sim")
|
|
);
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
#### Issue 2b: Magic Numbers/Test Data
|
|
```typescript:58:74:apps/bff/src/modules/subscriptions/sim-management/services/sim-validation.service.ts
|
|
// 4. Final fallback - for testing, use the known test SIM number
|
|
if (!account) {
|
|
// Use the specific test SIM number that should exist in the test environment
|
|
account = "02000331144508";
|
|
|
|
this.logger.warn(
|
|
`No SIM account identifier found for subscription ${subscriptionId}, using known test SIM number: ${account}`,
|
|
{
|
|
userId,
|
|
subscriptionId,
|
|
productName: subscription.productName,
|
|
domain: subscription.domain,
|
|
customFields: subscription.customFields ? Object.keys(subscription.customFields) : [],
|
|
note: "Using known test SIM number 02000331144508 - should exist in Freebit test environment",
|
|
}
|
|
);
|
|
}
|
|
```
|
|
|
|
**Issue**: Hardcoded test data in service layer, should be in config.
|
|
|
|
**Should be**:
|
|
```typescript
|
|
// config/env.validation.ts or constants file
|
|
export const TEST_SIM_ACCOUNT = process.env.TEST_SIM_ACCOUNT || "02000331144508";
|
|
|
|
// In service
|
|
if (!account && this.configService.get('NODE_ENV') === 'test') {
|
|
account = TEST_SIM_ACCOUNT;
|
|
this.logger.warn(`Using test SIM account: ${account}`);
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
#### Issue 2c: Custom Field Mapping Logic
|
|
```typescript:110:195:apps/bff/src/modules/subscriptions/sim-management/services/sim-validation.service.ts
|
|
private extractAccountFromCustomFields(
|
|
customFields: Record<string, unknown>,
|
|
subscriptionId: number
|
|
): string {
|
|
// Common field names for SIM phone numbers in WHMCS
|
|
const phoneFields = [
|
|
"phone",
|
|
"msisdn",
|
|
"phonenumber",
|
|
"phone_number",
|
|
"mobile",
|
|
"sim_phone",
|
|
"Phone Number",
|
|
"MSISDN",
|
|
// ... 30+ more variations
|
|
];
|
|
|
|
for (const fieldName of phoneFields) {
|
|
const rawValue = customFields[fieldName];
|
|
if (rawValue !== undefined && rawValue !== null && rawValue !== "") {
|
|
const accountValue = this.formatCustomFieldValue(rawValue);
|
|
this.logger.log(`Found SIM account in custom field '${fieldName}': ${accountValue}`);
|
|
return accountValue;
|
|
}
|
|
}
|
|
|
|
return "";
|
|
}
|
|
```
|
|
|
|
**Issue**: WHMCS field mapping is provider-specific knowledge, should be in provider layer.
|
|
|
|
**Should be**:
|
|
```typescript
|
|
// packages/domain/subscriptions/providers/whmcs/sim-field-mapper.ts
|
|
export const WHMCS_SIM_ACCOUNT_FIELDS = [
|
|
"phone",
|
|
"msisdn",
|
|
"phonenumber",
|
|
"phone_number",
|
|
"mobile",
|
|
"sim_phone",
|
|
// ... etc
|
|
] as const;
|
|
|
|
export function extractSimAccountFromWhmcsFields(
|
|
customFields: Record<string, unknown>
|
|
): string | null {
|
|
for (const fieldName of WHMCS_SIM_ACCOUNT_FIELDS) {
|
|
const value = customFields[fieldName];
|
|
if (value !== undefined && value !== null && value !== "") {
|
|
return String(value);
|
|
}
|
|
}
|
|
return null;
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### 3. **Order Business Rules in BFF Service** (Priority: LOW)
|
|
|
|
**Current Implementation**:
|
|
```typescript:180:201:apps/bff/src/modules/orders/services/order-validator.service.ts
|
|
validateBusinessRules(orderType: string, skus: string[]): void {
|
|
const error = getOrderTypeValidationError(orderType, skus);
|
|
if (error) {
|
|
this.logger.warn({ orderType, skuCount: skus.length, error }, "Order validation failed");
|
|
throw new BadRequestException(error);
|
|
}
|
|
|
|
// Log successful validation
|
|
this.logger.log(
|
|
{ orderType, skuCount: skus.length },
|
|
`Order business rules validated successfully for ${orderType} order`
|
|
);
|
|
}
|
|
```
|
|
|
|
**Assessment**: This is actually **OK** because it delegates to domain function `getOrderTypeValidationError()`.
|
|
|
|
The function correctly imports from domain:
|
|
```typescript
|
|
import { getOrderTypeValidationError } from "@customer-portal/domain/orders/validation";
|
|
```
|
|
|
|
**Recommendation**: ✅ Keep as-is, this is the correct pattern.
|
|
|
|
---
|
|
|
|
## 🟢 Good Architectural Patterns
|
|
|
|
### 1. **Service Layer Separation**
|
|
|
|
The orders module demonstrates excellent separation:
|
|
|
|
```
|
|
orders/
|
|
├── orders.controller.ts → HTTP layer (validation pipes only)
|
|
├── services/
|
|
│ ├── order-orchestrator.service.ts → Workflow coordination
|
|
│ ├── order-validator.service.ts → Validation aggregation
|
|
│ ├── order-builder.service.ts → Data transformation
|
|
│ ├── order-fulfillment-orchestrator.service.ts → Fulfillment workflow
|
|
│ └── order-fulfillment-validator.service.ts → Fulfillment validation
|
|
```
|
|
|
|
**Benefits**:
|
|
- Single Responsibility Principle
|
|
- Easy to test each layer independently
|
|
- Clear separation of concerns
|
|
|
|
---
|
|
|
|
### 2. **Orchestrator Pattern**
|
|
|
|
```typescript:33:80:apps/bff/src/modules/orders/services/order-orchestrator.service.ts
|
|
async createOrder(userId: string, rawBody: unknown) {
|
|
this.logger.log({ userId }, "Order creation workflow started");
|
|
|
|
// 1) Complete validation (format + business rules)
|
|
const { validatedBody, userMapping, pricebookId } =
|
|
await this.orderValidator.validateCompleteOrder(userId, rawBody);
|
|
|
|
this.logger.log(
|
|
{
|
|
userId,
|
|
orderType: validatedBody.orderType,
|
|
skuCount: validatedBody.skus.length,
|
|
},
|
|
"Order validation completed successfully"
|
|
);
|
|
|
|
// 2) Build order fields (includes address snapshot)
|
|
const orderFields = await this.orderBuilder.buildOrderFields(
|
|
validatedBody,
|
|
userMapping,
|
|
pricebookId,
|
|
validatedBody.userId
|
|
);
|
|
|
|
// 3) Create Order in Salesforce via integration service
|
|
const created = await this.salesforceOrderService.createOrder(orderFields);
|
|
|
|
// 4) Create OrderItems from SKUs
|
|
await this.orderItemBuilder.createOrderItemsFromSKUs(
|
|
created.id,
|
|
validatedBody.skus,
|
|
pricebookId
|
|
);
|
|
|
|
this.logger.log(
|
|
{
|
|
orderId: created.id,
|
|
skuCount: validatedBody.skus.length,
|
|
},
|
|
"Order creation workflow completed successfully"
|
|
);
|
|
|
|
return {
|
|
sfOrderId: created.id,
|
|
status: "Created",
|
|
message: "Order created successfully in Salesforce",
|
|
};
|
|
}
|
|
```
|
|
|
|
**Benefits**:
|
|
- Clear workflow steps
|
|
- Easy to understand business process
|
|
- Each step can be modified independently
|
|
- Excellent logging for debugging
|
|
|
|
---
|
|
|
|
### 3. **Distributed Transaction Pattern**
|
|
|
|
```typescript:83:142:apps/bff/src/modules/orders/services/order-fulfillment-orchestrator.service.ts
|
|
private async executeFulfillmentWithTransactions(
|
|
sfOrderId: string,
|
|
payload: Record<string, unknown>,
|
|
idempotencyKey: string
|
|
): Promise<OrderFulfillmentContext> {
|
|
const context: OrderFulfillmentContext = {
|
|
sfOrderId,
|
|
idempotencyKey,
|
|
validation: null,
|
|
steps: this.initializeSteps(
|
|
typeof payload.orderType === "string" ? payload.orderType : "Unknown"
|
|
),
|
|
};
|
|
|
|
this.logger.log("Starting transactional fulfillment orchestration", {
|
|
sfOrderId,
|
|
idempotencyKey,
|
|
});
|
|
|
|
// Step 1: Validation (no rollback needed)
|
|
try {
|
|
context.validation = await this.orderFulfillmentValidator.validateFulfillmentRequest(
|
|
sfOrderId,
|
|
idempotencyKey
|
|
);
|
|
|
|
if (context.validation.isAlreadyProvisioned) {
|
|
this.logger.log("Order already provisioned, skipping fulfillment", { sfOrderId });
|
|
return context;
|
|
}
|
|
} catch (error) {
|
|
this.logger.error("Fulfillment validation failed", {
|
|
sfOrderId,
|
|
error: getErrorMessage(error),
|
|
});
|
|
throw error;
|
|
}
|
|
|
|
// Step 2: Get order details (no rollback needed)
|
|
try {
|
|
const orderDetails = await this.orderOrchestrator.getOrder(sfOrderId);
|
|
if (!orderDetails) {
|
|
throw new Error("Order details could not be retrieved.");
|
|
}
|
|
context.orderDetails = orderDetails;
|
|
} catch (error) {
|
|
this.logger.error("Failed to get order details", {
|
|
sfOrderId,
|
|
error: getErrorMessage(error),
|
|
});
|
|
throw error;
|
|
}
|
|
|
|
// Step 3: Execute the main fulfillment workflow as a distributed transaction
|
|
let mappingResult: OrderItemMappingResult | undefined;
|
|
let whmcsCreateResult: { orderId: number } | undefined;
|
|
let whmcsAcceptResult: WhmcsOrderResult | undefined;
|
|
|
|
const fulfillmentResult =
|
|
await this.distributedTransactionService.executeDistributedTransaction(
|
|
// ... transaction steps
|
|
);
|
|
```
|
|
|
|
**Benefits**:
|
|
- Idempotency built-in
|
|
- Clear rollback strategy
|
|
- Transactional integrity across services
|
|
- Proper error handling and logging
|
|
|
|
---
|
|
|
|
## 📋 Summary of Recommendations
|
|
|
|
### Immediate Actions
|
|
|
|
1. **Extract Payment Validation to Shared Service**
|
|
- Create `PaymentValidatorService`
|
|
- Both `OrderValidator` and `OrderFulfillmentValidator` use it
|
|
- Eliminates duplication, ensures consistency
|
|
|
|
### Medium Priority
|
|
|
|
2. **Move SIM Business Logic to Domain**
|
|
- Extract `isSimSubscription()` to `packages/domain/subscriptions/validation.ts`
|
|
- Move WHMCS field mappings to `packages/domain/subscriptions/providers/whmcs/`
|
|
- Move test constants to config
|
|
|
|
3. **Extract SIM Account Extraction Logic**
|
|
- Create domain function for SIM account extraction
|
|
- Keep infrastructure concerns (DB, logging) in BFF
|
|
- Move business rules (what fields to check, validation logic) to domain
|
|
|
|
### Low Priority (Optional)
|
|
|
|
4. **Consider Creating Base Validator Service**
|
|
```typescript
|
|
// apps/bff/src/core/validation/base-validator.service.ts
|
|
export abstract class BaseValidatorService {
|
|
constructor(
|
|
protected readonly logger: Logger,
|
|
protected readonly mappingsService: MappingsService
|
|
) {}
|
|
|
|
protected async validateUserMapping(userId: string) {
|
|
// Common validation logic used by multiple validators
|
|
}
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
## 🎯 BFF Validation Checklist
|
|
|
|
Use this checklist when adding new validation:
|
|
|
|
- [ ] **Format validation**: Use `ZodValidationPipe` in controller
|
|
- [ ] **Schema source**: Import from `@customer-portal/domain`
|
|
- [ ] **Business rules**: Check if rule belongs in domain layer
|
|
- [ ] **Infrastructure checks**: Keep in BFF service (DB, API calls)
|
|
- [ ] **Shared logic**: Consider extracting to shared service if used > 1 place
|
|
- [ ] **Error messages**: Use consistent, user-friendly messages
|
|
- [ ] **Logging**: Log validation failures with context
|
|
- [ ] **Type safety**: Use inferred types from domain schemas
|
|
|
|
---
|
|
|
|
## 📊 Validation Logic Distribution
|
|
|
|
Current distribution of validation logic:
|
|
|
|
| Layer | Responsibility | Examples | Status |
|
|
|-------|---------------|----------|--------|
|
|
| **Controller** | Input format validation | `@UsePipes(ZodValidationPipe)` | ✅ Good |
|
|
| **Domain Schemas** | Data structure & format rules | `createOrderRequestSchema` | ✅ Good |
|
|
| **Domain Validation** | Business rules (pure logic) | `hasSimServicePlan()` | ✅ Good |
|
|
| **BFF Services** | Infrastructure validation | `validateUserMapping()`, `validatePaymentMethod()` | 🟡 Minor duplication |
|
|
| **BFF Services** | Business logic | `isSimService` check in `SimValidationService` | ⚠️ Should move to domain |
|
|
|
|
---
|
|
|
|
## 🔍 Validation Flow Example (Orders)
|
|
|
|
```
|
|
1. HTTP Request
|
|
↓
|
|
2. OrdersController
|
|
├─ @UsePipes(ZodValidationPipe(createOrderRequestSchema)) ← Format validation
|
|
└─ Passes validated body to OrderOrchestrator
|
|
↓
|
|
3. OrderOrchestrator.createOrder()
|
|
├─ Calls OrderValidator.validateCompleteOrder()
|
|
│ ├─ Format validation (schema) ← Zod schema
|
|
│ ├─ Business validation (schema) ← Domain validation
|
|
│ ├─ User mapping check (infrastructure) ← BFF validation
|
|
│ ├─ Payment method check (infrastructure) ← BFF validation
|
|
│ ├─ SKU validation (infrastructure) ← BFF validation
|
|
│ └─ Business rules (delegates to domain) ← Domain validation
|
|
└─ Continue with order creation
|
|
```
|
|
|
|
**Assessment**: ✅ Clean separation, proper delegation
|
|
|
|
---
|
|
|
|
## 📝 Notes
|
|
|
|
### Comparison with Domain Layer
|
|
|
|
- **Domain Layer**: Pure business logic, no dependencies on infrastructure
|
|
- **BFF Layer**: Infrastructure-dependent validation (DB, external APIs)
|
|
- **Clear Boundary**: BFF delegates pure business rules to domain functions
|
|
|
|
### Previous Validation Cleanup
|
|
|
|
The codebase has already:
|
|
- Eliminated password validation duplication
|
|
- Moved SKU validation to domain layer
|
|
- Consolidated order business rules
|
|
|
|
This review builds on that work to identify remaining improvements.
|
|
|
|
---
|
|
|
|
## 🔗 Related Documents
|
|
|
|
- [VALIDATION_AUDIT_REPORT.md](./VALIDATION_AUDIT_REPORT.md) - Domain validation overlaps
|
|
- [docs/validation/VALIDATION_PATTERNS.md](docs/validation/VALIDATION_PATTERNS.md) - Validation patterns guide
|
|
- [docs/validation/VALIDATION_CLEANUP_SUMMARY.md](docs/validation/VALIDATION_CLEANUP_SUMMARY.md) - Previous cleanup work
|
|
|