# MenuService — Deep Code Analysis Report

**Date:** 2026-03-02
**Scope:** GB5Framework MenuService — all layers (SL, BLL, DAL, QB, DTO)
**Files Reviewed:**
- `FrameworkSL/Controllers/Menu/MenuService.cs`
- `FrameworkSL/Endpoints/Menu/GetMenuForUserModule.cs`
- `FrameworkSL/Endpoints/Menu/GetMenu.cs`
- `FrameworkSL/Endpoints/Menu/GetSelectListMenu.cs`
- `FrameworkSL/Endpoints/Menu/GetReportMenuDetailsForMenu.cs`
- `FrameworkBLL/Menu/MenuBLL.cs` / `IMenuBLL.cs`
- `FrameworkBLL/RoleVsMenu/RoleVsMenuBLL.cs`
- `FrameworkDAL/CustomCode/Menu/MenuDAL.cs` / `IMenuDAL.cs`
- `FrameworkDAL/Query/Menu/MenuQB.cs`
- `FrameworkDAL/DTO/Menu/MenuTreeDTO.cs`

---

## Executive Summary

The MenuService is a critical path component — it loads the navigation tree, report criteria, web-service settings, and role access rights for every user session. Several issues were found across **Security/VAPT**, **Performance**, **Memory**, **Best Practices**, and **Architectural compliance**. Three findings are **Critical** severity with direct security impact.

---

## 1. VAPT / Security Issues

### CRIT-01 — AllowAnonymous on All Menu Endpoints
**Severity:** Critical
**Files:** `GetMenuForUserModule.cs:38`, `GetMenu.cs:24`, `GetSelectListMenu.cs:24`, `GetReportMenuDetailsForMenu.cs:21`

All four SL endpoints are decorated with `AllowAnonymous()`. The menu tree exposes the entire navigation structure of the application, including report criteria, web-service URLs, BizTransactionClass IDs, role rights, and entity metadata. An unauthenticated caller can retrieve:
- Full menu tree for any `UserId`/`ModuleId` combination
- Report criteria and web-service URL templates (with `BaseUri` embedded from `LoginDTO`)
- Drill-down configuration including `PartyBranchId`

**Impact:** OWASP A01 (Broken Access Control). An attacker can enumerate the full feature set of the application, discover internal API endpoints from `WebFormURL`/`WebFormSecondURL`, and map business logic for targeted attacks.

**Recommendation:** Replace `AllowAnonymous()` with appropriate role/policy enforcement, e.g.:
```csharp
// Roles("Authenticated") or the framework's equivalent policy
Roles("User");
```
Authentication should validate the `Login` header token server-side before executing.

---

### CRIT-02 — GET_SELECTLIST_MENU Has No Tenant Filter
**Severity:** Critical
**Files:** `MenuQB.cs:1283-1293`, `MenuDAL.cs:1040-1048`

```sql
SELECT M.MENUID, M.MENUCODE, M.MENUNAME, M.PARENTIDS, M.SOURCETYPE
FROM MMENU M
WHERE M.STATUS = 1;
```

The `GET_SELECTLIST_MENU` query has **no ClientId / DatabaseName filter** and the `GetSelectListMenu` DAL method passes `null` as parameters:
```csharp
var result = await _queryExecutor.QueryAsync<MenuPicklistDTO>(LoginDTO, sql, null!);
```

This returns **all menu records from all tenants** to anyone calling this endpoint. With `AllowAnonymous` also active (CRIT-01), this is a direct cross-tenant data leak.

**Impact:** OWASP A01 + A04. Full menu catalogue of all tenants is publicly accessible.

**Recommendation:** Add mandatory tenant filter and pass `LoginDTO.ClientId` as parameter:
```sql
WHERE M.STATUS = 1 AND M.ClientId = @clientid
```

---

### CRIT-03 — SQL Replaced With String Injection Pattern in GetReportCriteriaForMenu
**Severity:** Critical
**Files:** `MenuDAL.cs:158-171`

```csharp
Sql = Sql.Replace(":baseuri", LoginDTO.BaseUri);
Sql = Sql.Replace(":partybranchid", LoginDTO.WorkPartyBranchId.ToString());
```

`LoginDTO.BaseUri` is embedded directly into the SQL string using string replacement — not as a parameterized value. If `BaseUri` contains SQL metacharacters (e.g. `'; DROP TABLE--`), this opens a SQL injection path. Even if `BaseUri` is server-set, this pattern is architecturally unsafe and violates the project's own non-negotiable rule against SQL string concatenation.

Similarly, the `IsFromScreen` flag is used to conditionally strip a SQL `WHERE` clause:
```csharp
if (IsFromScreen == 1)
    Sql = Sql.Replace("and rolevsmenu.Allow like '%0%'", "");
```
This string-manipulation approach to dynamically alter access control SQL is fragile and bypasses role enforcement. A client-controlled `IsFromScreen=1` removes the access check entirely.

**Recommendation:**
- Move `BaseUri`/`PartyBranchId` substitutions to SQL parameters: `@baseuri`, `@partybranchid`.
- Apply the access-control condition server-side based on a validated server context, not a raw client-supplied integer.

---

### HIGH-01 — LoginDTO Accepted via HTTP Header Without Validation
**Severity:** High
**Files:** All SL endpoints — e.g. `GetMenuForUserModule.cs:44`

```csharp
public record GetMenuForUserModuleParameters(
    [property: QueryParam] int UserId,
    [property: QueryParam] int ModuleId,
    [property: FromHeader] string Login   // ← raw JSON from client
);
```

The `Login` header is deserialized from client-supplied JSON into a `LoginDTO`. The `UserId` in the query parameter and the `UserId` inside the `Login` header JSON are separate values that are never cross-validated. A client can:
- Pass `UserId=1` (admin) in the query string
- Pass a different (their own) user in the `Login` header
- Receive the menu tree for the admin user while their `LoginDTO` scoped to another tenant

**Recommendation:** The `UserId` should be derived exclusively from the authenticated session/token, not accepted as a client-supplied query parameter.

---

### HIGH-02 — Exception Message Leaked to Client
**Severity:** High
**Files:** `GetMenuForUserModule.cs:68-69`, `GetMenu.cs:51`, `GetReportMenuDetailsForMenu.cs:51`

```csharp
return await GB5Shared.ResponseStandard.Response.CreateExceptionError<string>(
    ex, CacheKeyLevel.NOT_REQUIRED, LoginDTO, ex.Message, 500);
```

`ex.Message` is passed directly as a response parameter. This can expose internal details such as table names, column names, connection strings fragments, or stack information to the caller.

**Recommendation:** Log the full exception server-side; return only a generic error message to the client:
```csharp
_logger.LogError(ex, "GetMenuForUserModule failed for User {UserId}", req.UserId);
return await Response.CreateExceptionError<string>(ex, ..., "An error occurred.", 500);
```

---

### MED-01 — CancellationToken Not Propagated
**Severity:** Medium
**Files:** `MenuBLL.cs:39,167,179,197`, `MenuDAL.cs:29,131,146,1036`

All `IMenuBLL` and `IMenuDAL` interface methods lack `CancellationToken` parameters entirely. The SL endpoints receive a `ct` parameter but it is never forwarded to BLL or DAL. Database queries cannot be cancelled when the client disconnects.

**Recommendation:** Add `CancellationToken ct` to all interface and implementation method signatures and pass `ct` through to every `_queryExecutor` call.

---

### MED-02 — Wrong Cache Key EntityConstant in GetMenuForUserModule
**Severity:** Medium
**Files:** `GetMenuForUserModule.cs:52`

```csharp
return KeyGenerator.KeyGeneration(
    new { req.UserId, req.ModuleId },
    EntityConstant.OBJECTDOCUMENTSET,  // ← Wrong constant
    CacheKeyLevel.USER_LEVEL, LoginDTO);
```

The cache key uses `EntityConstant.OBJECTDOCUMENTSET` instead of `EntityConstant.MENU`. This means:
- Menu cache entries can collide with document-set cache entries if they share the same composite key.
- A cache invalidation on document-set will incorrectly evict menu data, and vice versa.

---

## 2. Performance Issues

### PERF-01 — 60-Field Manual Mapping Loop in BLL (GetMenuForUserModule)
**Severity:** High
**Files:** `MenuBLL.cs:51-152`

```csharp
for (int i = 0; i < menuList.Count; i++)
{
    MenuTreeDTO menuTreeDTO = new MenuTreeDTO();
    menuTreeDTO.Id = menu.MenuId;
    menuTreeDTO.Code = menu.MenuCode;
    // ... 58 more manual assignments ...
    MenuTreeDTOs.Add(menuTreeDTO);
}
```

A 60+ field manual property-copy loop runs in the BLL for every menu item. This same data is then:
1. Serialized to `Json1` (unused intermediate variable)
2. Passed to `TreeHelper.ConvertToTree()`
3. Serialized again to `Json`

The intermediate serialization to `Json1` at line 155 is completely unused and wastes CPU + memory allocation.

**Recommendation:** Use Dapper's direct mapping to `MenuTreeDTO`, or `ValueInjecter` (already imported) to eliminate the manual loop. Remove the unused `Json1` intermediate.

---

### PERF-02 — Date Calculation Code Duplicated Twice
**Severity:** Medium
**Files:** `MenuDAL.cs:430-535` (GetCriteriaConfigForMenu) and `MenuDAL.cs:790-891` (GetWebServiceSettingForMenu)

Identical 100+ line blocks computing `TodayFrom`, `TodayTo`, `YesterdayFrom`/`To`, `WeekFrom`, `MonthFrom`, `YearFrom`, `CurrentMonthFrom`, `CurrentYearFrom`, `CurrentYearTo` — with `await ToEpoch()` calls — appear **twice** in the same class.

Each `ToEpoch` call is a `Task.FromResult` wrapper that allocates a `Task<long>` object for a synchronous value computation. There are ~20 such `await Task.FromResult` calls per invocation path.

**Recommendation:**
- Extract a `ComputeDateRangeBoundaries()` private method returning a struct/record — called once, shared between both methods.
- Change `ToEpoch` to a synchronous method (it does no I/O):
```csharp
private static long ToEpoch(DateTime date)
    => new DateTimeOffset(date.ToUniversalTime()).ToUnixTimeSeconds();
```

---

### PERF-03 — StreamAsync Used But Immediately Materialized to List
**Severity:** Medium
**Files:** `MenuDAL.cs:174-177`, `MenuDAL.cs:301`, `MenuDAL.cs:415-418`, `MenuDAL.cs:564`

```csharp
await foreach (var item in _queryExecutor.StreamAsync<...>(LoginDTO, Sql, Parameters))
{
    ReportMenuLoadingFlatQueryNewDTOs.Add(item);  // defeats streaming entirely
}
// or:
.ToListAsync();
```

`StreamAsync` is used to fetch data but is always immediately materialized into a `List<T>`. This provides no memory benefit over `QueryAsync` — worse, it adds the overhead of the streaming channel. Use `QueryAsync` for data that is always fully consumed.

The only valid use of `StreamAsync` would be if the result set could be large enough to process row-by-row without full materialization.

---

### PERF-04 — Multiple GroupBy+Select.First() Scans on Same List
**Severity:** Medium
**Files:** `MenuDAL.cs:204-245`

```csharp
// Four separate LINQ passes over ReportMenuLoadingFlatQueryNewDTOs
TempReportMenuLoadingFlatQueryNewDTOs = ReportMenuLoadingFlatQueryNewDTOs
    .GroupBy(x => x.MenuId).Select(g => g.First()).ToList();

TempReportMenuLoadingFlatQueryNewDTOs = ReportMenuLoadingFlatQueryNewDTOs
    .GroupBy(x => new { x.ReportCriteriaReportId, x.WebServiceId, x.WebServiceCriteriaSlNo })
    .Select(g => g.First()).ToList();
// ... and two more similar passes
```

The same source list `ReportMenuLoadingFlatQueryNewDTOs` is scanned four times with different `GroupBy` keys. For large menus this is O(4N) work. A single pass using `Dictionary` or `ToLookup()` would be more efficient.

---

### PERF-05 — Reflection-Heavy MultiLevelDTO Helper
**Severity:** Medium
**Files:** `MenuDAL.cs:594-745`

`MultiLevelDTO<T>` uses `Activator.CreateInstance`, `GetType().GetProperty(FilterField)`, `.GetValue()`, and `InjectFrom` (ValueInjecter) in hot loops. This involves:
- Per-item reflection on every property access
- Boxing/unboxing via `object` parameters
- Repeated `Type.GetType(TargetType)` calls (string-based type resolution)
- Round-trip `JsonConvert.SerializeObject → DeserializeObject` to convert types

This is called multiple times per `GetReportCriteriaForMenu` request. For menus with many criteria, this becomes a significant allocation and CPU hotspot.

**Recommendation:** Replace with strongly-typed LINQ projection. The generality of `MultiLevelDTO` is used for a fixed, known set of DTO shapes — generic reflection is not needed here.

---

### PERF-06 — ChangeObjectToExpectedObject Wraps a Synchronous Op in a Task
**Severity:** Low
**Files:** `MenuDAL.cs:747-759`

```csharp
public async Task<object> ChangeObjectToExpectedObject(object Obj, string TargetType)
{
    var json = JsonConvert.SerializeObject(Obj);
    var result = JsonConvert.DeserializeObject(json, target!)!;
    return await Task.FromResult(result);  // synchronous disguised as async
}
```

This method does no I/O — wrapping it in `async Task` causes a state machine allocation and `Task.FromResult` allocation on every call. It should be synchronous or use `ValueTask`.

---

### PERF-07 — Double JSON Serialization in GetMenuForUserModule
**Severity:** Low
**Files:** `MenuBLL.cs:155-158`

```csharp
string Json1 = JsonConvert.SerializeObject(MenuTreeDTOs);    // allocated but never used
IList<MenuTreeDTO> TreeTopLevelCategory = TreeHelper.ConvertToTree(MenuTreeDTOs);
string Json = JsonConvert.SerializeObject(TreeTopLevelCategory);
return Json;
```

`Json1` is computed and discarded. This allocates a large string for a complex DTO list with no effect.

---

## 3. Memory Issues

### MEM-01 — Large Object Graph Held in Memory Across Multiple In-Memory Collections
**Severity:** High
**Files:** `MenuDAL.cs:164-390`

`GetReportCriteriaForMenu` creates and holds in memory simultaneously:
- `ReportMenuLoadingFlatQueryNewDTOs` (full flat result)
- `TempReportMenuLoadingFlatQueryNewDTOs` (multiple reshuffled copies)
- `MenuDetailDTOs`, `MenuCriteriaDTOs`, `MenuReportVsFormatsDTOs`, `MenuReportVsViewsDTOs`
- `ReportViewFieldsLoadingDTOs`, `WebServiceSettingQueryDTOs`, `DrillDownMenuDTOs`
- Multiple serialized JSON strings

All of these co-exist in memory before any single collection is eligible for GC. For complex menus this can balloon to several MB per concurrent request.

**Recommendation:** Process in pipelines — release intermediate collections as soon as they are no longer needed (`= null` or use a narrower scope).

---

### MEM-02 — MenuTreeDTO Has No Auto-Properties (Old-Style Backing Fields)
**Severity:** Low
**Files:** `MenuTreeDTO.cs` (entire file)

`MenuTreeDTO` declares ~50 private backing fields and corresponding explicit property get/set bodies. This is Java-era style that adds ~50 extra field slots per object. Modern C# auto-properties or `record` types reduce memory footprint and compile to identical IL.

---

### MEM-03 — ToEpoch Allocates 20 Task Objects Per Request
**Severity:** Low
**Files:** `MenuDAL.cs:929-933`

```csharp
public async Task<long> ToEpoch(DateTime date)
{
    DateTimeOffset dto = new DateTimeOffset(date.ToUniversalTime());
    return await Task.FromResult(dto.ToUnixTimeSeconds());
}
```

Called ~20 times in `GetCriteriaConfigForMenu` and ~20 times in `GetWebServiceSettingForMenu` per request. Each `await Task.FromResult` allocates a heap `Task<long>`. Synchronous conversion eliminates 40 allocations per request.

---

## 4. Best Practice Issues

### BP-01 — Bare `catch (Exception) { throw; }` Throughout
**Severity:** Medium
**Files:** `MenuBLL.cs:160-163`, `MenuBLL.cs:168-176`, and nearly every method

```csharp
catch (Exception)
{
    throw;
}
```

Every method in `MenuBLL` and many in `MenuDAL` wraps the entire method body in a try/catch that does nothing except re-throw. This adds stack-unwinding overhead with zero value — the exception already propagates without the catch. These should be removed entirely unless logging or recovery is added.

---

### BP-02 — Unused Legacy Controller Still in Codebase
**Severity:** Low
**Files:** `FrameworkSL/Controllers/Menu/MenuService.cs`

The entire file is commented out (1,300+ bytes of dead code). It references old MVC controller patterns, `DaprClient` state-store calls, and hardcoded `ttlInSeconds=60` cache TTL. This should be deleted, not left commented.

---

### BP-03 — RoleVsMenuBLL Injects IQueryExecutor Directly
**Severity:** Medium
**Files:** `RoleVsMenuBLL.cs:24,31`

```csharp
private readonly IQueryExecutor _queryExecutor;
public RoleVsMenuBLL(IRoleVsMenuDAL RoleVsMenuDAL, AutoNumber AutoNumber, IQueryExecutor QueryExecutor)
```

`RoleVsMenuBLL` holds `IQueryExecutor` and calls `_queryExecutor.ExecuteAsync` directly (lines 67, 75) — bypassing the DAL. This violates the strict SL→BLL→DAL layering rule from the architecture spec.

---

### BP-04 — RoleVsMenuBLL: Delete-then-Re-Insert Without Transaction
**Severity:** High
**Files:** `RoleVsMenuBLL.cs:62-77`

```csharp
shql = await _RoleVsMenuDAL.DeleteRoleVsMenu(RoleId, ModuleId, LoginDTO);
await _queryExecutor.ExecuteAsync(LoginDTO, shql);
// ... no transaction ...
await _RoleVsMenuDAL.SaveRoleVsMenu(RoleVsMenuDTOs, LoginDTO);
```

The delete and subsequent insert of role-vs-menu permissions run as independent, non-atomic operations. If the save fails after the delete, the user's role permissions are wiped with nothing written back. This leaves the system in a corrupted access-control state.

**Recommendation:** Wrap in a `BeginTransactionAsync` / `CommitAsync` / `RollbackAsync` block as required by project standards.

---

### BP-05 — RoleVsMenuBLL: SaveRoleVsMenu Called Inside a Loop Scope Mismatch
**Severity:** Medium
**Files:** `RoleVsMenuBLL.cs:84-110`

```csharp
for (int i = 0; i < RoleVsMenuDTOs.Count; i++)
{
    RoleVsMenuDTO roleVsMenu = RoleVsMenuDTOs[i];
    roleVsMenu.RoleVsMenuId = AutoNumberDTO.StartNumber + i;
    RoleVsMenus.Add(roleVsMenu);
}
// Commented-out per-item saves + the actual save below the loop
await _RoleVsMenuDAL.SaveRoleVsMenu(RoleVsMenuDTOs, LoginDTO);
```

The loop builds `RoleVsMenus` (a second list) but `SaveRoleVsMenu` is called with the original `RoleVsMenuDTOs` — not with `RoleVsMenus` that was actually populated with generated IDs. The `RoleVsMenuId` values assigned in the loop are never persisted.

---

### BP-06 — `throw Error` Instead of `throw` Loses Stack Trace
**Severity:** Low
**Files:** `RoleVsMenuBLL.cs:147`

```csharp
catch (Exception Error)
{
    throw Error;   // resets stack trace origin
}
```

`throw Error` resets the exception's stack trace to the current catch block. Use bare `throw` to preserve the original origin.

---

### BP-07 — Naming Convention Inconsistencies
**Severity:** Low
**Files:** Multiple

- Parameters named with PascalCase in DAL methods (`UserId`, `ModuleId`, `LoginDTO`) instead of camelCase.
- `IMenuBLL` constructor parameter named `IMenuBLL` (matches interface name) — misleading naming.
- `MenuQB` is declared as a non-static class but has only `const` fields — should be `static class MenuQB`.

---

### BP-08 — GetSelectListMenu Ignores CriteriaDTO Parameter
**Severity:** Medium
**Files:** `MenuDAL.cs:1036-1048`

```csharp
public async Task<string> GetSelectListMenu(CriteriaDTO CriteriaDTO, LoginDTO LoginDTO)
{
    string sql = MenuQB.GET_SELECTLIST_MENU;
    var result = await _queryExecutor.QueryAsync<MenuPicklistDTO>(LoginDTO, sql, null!);
    // CriteriaDTO is accepted but completely ignored
}
```

The `CriteriaDTO` parameter (and its filters — search text, paging, active status) is accepted by the interface and BLL but silently discarded in the DAL. The endpoint accepts user filtering intent that has zero effect on the query.

---

### BP-09 — GetCacheKey Throws Silently Swallowed by Empty Catch
**Severity:** Medium
**Files:** `GetMenuForUserModule.cs:48-58`, `GetMenu.cs:31-41`, `GetReportMenuDetailsForMenu.cs:31-40`

```csharp
protected override string? GetCacheKey(..., LoginDTO LoginDTO)
{
    try
    {
        return KeyGenerator.KeyGeneration(...);
    }
    catch (Exception)
    {
        throw;   // bare rethrow — no log
    }
}
```

Cache key generation failures are rethrown with no logging. If `KeyGenerator` throws (e.g., due to a null field in `LoginDTO`), the endpoint fails entirely with no diagnostic information.

---

## 5. Architecture Compliance Issues

### ARCH-01 — BLL References `Newtonsoft.Json` for Tree Building
**Severity:** Low
**Files:** `MenuBLL.cs:9`

The BLL performs JSON serialization (`JsonConvert.SerializeObject`) and builds the tree structure. Per the architecture spec, BLL should orchestrate business logic — serialization is an SL concern. The BLL should return `IList<MenuTreeDTO>` and let the SL/framework serialize.

---

### ARCH-02 — DAL Contains Business Logic (Date Calculations, URI Manipulation)
**Severity:** Medium
**Files:** `MenuDAL.cs:421-591`, `MenuDAL.cs:935-1028`

`GetCriteriaConfigForMenu` calculates "today", "yesterday", "last week", "last month", "current year" boundaries and constructs epoch timestamps — this is business/domain logic that belongs in BLL.

`ChangedUri` performs string manipulation of web-service URI templates — also business logic, not data access.

---

### ARCH-03 — DAL Directly Calls Other DAL Methods Internally
**Severity:** Low
**Files:** `MenuDAL.cs:224`, `MenuDAL.cs:285`, `MenuDAL.cs:301`

`GetReportCriteriaForMenu` directly calls `GetReportViewFieldsForMenu`, `GetCriteriaConfigForMenu`, and `GetWebServiceSettingForMenu` — all DAL methods calling each other. Orchestration of multiple data-fetch operations belongs in BLL.

---

## 6. Summary Table

| ID | Category | Severity | Issue |
|----|----------|----------|-------|
| CRIT-01 | Security/VAPT | **Critical** | AllowAnonymous on all menu endpoints |
| CRIT-02 | Security/VAPT | **Critical** | GET_SELECTLIST_MENU has no tenant filter + null params |
| CRIT-03 | Security/VAPT | **Critical** | BaseUri injected via string replacement into SQL |
| HIGH-01 | Security/VAPT | High | UserId accepted as client query param, not from auth token |
| HIGH-02 | Security/VAPT | High | Raw exception message leaked to HTTP response |
| MED-01 | Security | Medium | CancellationToken not propagated through BLL/DAL |
| MED-02 | Security | Medium | Wrong EntityConstant in menu cache key |
| PERF-01 | Performance | High | 60-field manual mapping loop in BLL |
| PERF-02 | Performance | Medium | Date calculation code duplicated twice (200+ lines) |
| PERF-03 | Performance | Medium | StreamAsync immediately materialised — no streaming benefit |
| PERF-04 | Performance | Medium | Four sequential GroupBy scans on same list |
| PERF-05 | Performance | Medium | Reflection-heavy MultiLevelDTO in hot path |
| PERF-06 | Performance | Low | Synchronous logic wrapped in async Task (ToEpoch) |
| PERF-07 | Performance | Low | Unused intermediate JSON serialization (Json1) |
| MEM-01 | Memory | High | Multiple large in-memory collections co-existing per request |
| MEM-02 | Memory | Low | MenuTreeDTO uses old-style backing fields (50+ private fields) |
| MEM-03 | Memory | Low | 40 Task allocations per request from ToEpoch |
| BP-01 | Best Practice | Medium | Empty catch-rethrow blocks with no logging |
| BP-02 | Best Practice | Low | Dead commented-out controller file |
| BP-03 | Best Practice | Medium | BLL directly holds IQueryExecutor (bypasses DAL) |
| BP-04 | Best Practice | High | Delete + Insert without transaction — data corruption risk |
| BP-05 | Best Practice | Medium | Built list with IDs never passed to Save — IDs lost |
| BP-06 | Best Practice | Low | `throw Error` loses stack trace |
| BP-07 | Best Practice | Low | Naming inconsistencies (PascalCase params, non-static QB) |
| BP-08 | Best Practice | Medium | CriteriaDTO parameter silently ignored in GetSelectListMenu |
| BP-09 | Best Practice | Medium | Cache key exceptions rethrown with no logging |
| ARCH-01 | Architecture | Low | BLL performs JSON serialization |
| ARCH-02 | Architecture | Medium | DAL contains business logic (date calcs, URI manipulation) |
| ARCH-03 | Architecture | Low | DAL methods orchestrate other DAL methods |

---

## 7. Recommended Fix Priority

### Immediate (this sprint)
1. **CRIT-01** — Add authentication to all menu endpoints.
2. **CRIT-02** — Add tenant filter to `GET_SELECTLIST_MENU` + pass `clientId` param.
3. **CRIT-03** — Convert `BaseUri`/`PartyBranchId` SQL replacements to `@` parameters.
4. **BP-04** — Wrap RoleVsMenu delete+insert in a transaction.

### Short-term (next sprint)
5. **HIGH-01** — Derive `UserId` from authenticated token, not query string.
6. **HIGH-02** — Stop passing `ex.Message` to client; log server-side only.
7. **MED-01** — Add `CancellationToken` to all BLL and DAL interfaces.
8. **BP-05** — Fix `SaveRoleVsMenu` to use the ID-assigned `RoleVsMenus` list.
9. **BP-08** — Apply `CriteriaDTO` filters in `GetSelectListMenu`.

### Medium-term (tech debt)
10. **PERF-01** — Replace manual 60-field mapping with auto-mapper or direct Dapper mapping.
11. **PERF-02** — Extract shared `ComputeDateRangeBoundaries()` method; make `ToEpoch` synchronous.
12. **ARCH-02** — Move date calculations and URI manipulation from DAL to BLL.
13. **BP-03** — Remove `IQueryExecutor` from `RoleVsMenuBLL`; use DAL exclusively.
14. **MEM-01** — Reduce in-memory object lifetime in `GetReportCriteriaForMenu`.
15. **BP-02** — Delete the commented-out `MenuService.cs` controller.