# Code Improvement Suggestions

> Date: 2026-04-09  
> Based on current codebase state after all bug fixes.

---

## 1. Critical Bugs Still Present

### 1.1 `queue_name` filter added N times in `GetJobsInfo` (apiserver)

**File:** `controllers/jobs.go` ~line 226  
**Problem:** The `queue_name:in:(...)` RBAC filter is appended inside the per-filter loop — once for every non-queue filter in the request. With 2 filters (e.g. `job_status` + `submit_user`), it gets added twice. Seen in server logs: `queue_name:in:(normal,priority,short)` appears twice in every query.

**Fix:** Move the `accessQueues` append **outside** the loop, after it completes.

```go
// BEFORE (inside loop — adds queue_name once per non-queue filter)
for i, filter := range filters {
    ...
    } else {
        if len(accessQueues) > 0 {
            filters = append(filters, ...)  // BUG: runs N times
        }
    }
}

// AFTER (outside loop — adds once)
for i, filter := range filters { ... }
if len(accessQueues) > 0 && !queueFilterPresent {
    filters = append(filters, utils.FilterCondition{
        Field: "queue_name", Operator: "in",
        Value: "(" + strings.Join(accessQueues, ",") + ")",
    })
}
```

---

## 2. Design Issues

### 2.1 `controllers/jobs.go` is 3590 lines — needs splitting

The file handles job query, submit, control, restart, array jobs, and verbose output. Each is a distinct concern.

**Suggested split:**
```
controllers/
  jobs_query.go    — GetJobsInfo, GetJobByIDs (~600 lines)
  jobs_submit.go   — CreateJob, RestartJobByID (~500 lines)
  jobs_control.go  — ControlJob, ControlJobByID (~400 lines)
  jobs_array.go    — array job handling (~300 lines)
  jobs_helpers.go  — shared: parseArrayName, processJobData, etc.
```

### 2.2 RBAC permission check duplicated across controllers

`GetRequireResourcePermission` + `accessQueues` logic is copy-pasted in `GetJobsInfo`, `ControlJob`, `ControlJobByID`, `RestartJobByID`. Extract to a helper:

```go
// controllers/rbac_helpers.go
func getJobAccessQueues(username string) ([]string, error) { ... }
func checkJobViewPermission(username, queueName string) error { ... }
```

### 2.3 `xcecli/cmd/jobs/submit.go` is 734 lines

`JobSubmitter` mixes CLI flag parsing, file upload, interactive job handling, and HTTP submission. Split:

```
cmd/jobs/
  submit.go          — cobra command + flag binding only
  submitter.go       — JobSubmitter struct + BuildJobRequest
  submit_files.go    — file upload logic (already in files.go ✓)
  submit_interactive.go — interactive/TTY/xjob handling
```

---

## 3. Code Quality

### 3.1 `ioutil.ReadFile` deprecated (Go 1.16)

**File:** `controllers/rbac_check.go:54`  
**Fix:** `os.ReadFile(path)` — one-line change, tracked as Q3.

### 3.2 Inconsistent error return convention

Some functions return `(error, T)`, others return `(T, error)`. Go convention is always `(T, error)`.

```go
// Non-standard (apiserver/controllers/rbac_check.go)
func GetRequireResourcePermission(...) (error, models.RolePermission)

// Should be
func GetRequireResourcePermission(...) (models.RolePermission, error)
```

This affects ~8 call sites in `jobs.go` that do `err, perm := GetRequire...`.

### 3.3 Magic strings for domain types

`"all"`, `"self"`, `"self_ug"`, `"queue"` appear as raw strings in multiple files. They're defined as constants in `models/roles.go` but not always used.

**Fix:** Replace all raw string comparisons with the constants (`models.DomainTypeAll`, etc.).

### 3.4 `xcecli` output printers have no shared time formatting

`page_print_jobs.go` now has `parseJobTime`/`formatJobTime`. The same pattern will be needed for `start_time`, `end_time` in other printers. Move to `common/output/time.go`.

---

## 4. Architecture

### 4.1 In-memory store has no persistence

After restart, all submitted jobs (non-builtin) are lost. The `lib/pq` PostgreSQL driver is already in `go.mod`. When backend integration happens, the notification handlers (`OnJobCreated`, etc.) should write through to PostgreSQL, not just the in-memory store.

### 4.2 `UserRoleSeeder` only implemented for `config_file`

After the startup fix, PAM/LDAP/OAuth2 providers don't implement `AllUsers()` — so after restart with those providers, users still need to re-login to get their roles. Consider storing user-role assignments in a persistent store (DB or file) so they survive restarts regardless of provider.

### 4.3 Rate limiter `limiters` map grows unboundedly

**File:** `middleware/rate_limit.go`  
The per-endpoint `limiters` map is never evicted. Under normal operation with fixed routes this is fine, but worth noting for future dynamic routing.

---

## 5. Priority Order

```mermaid
quadrantChart
    title Impact vs Effort
    x-axis Low Effort --> High Effort
    y-axis Low Impact --> High Impact
    quadrant-1 Do First
    quadrant-2 Plan
    quadrant-3 Later
    quadrant-4 Quick Wins
    queue_name double-add: [0.10, 0.90]
    ioutil fix: [0.05, 0.20]
    error convention: [0.25, 0.60]
    split jobs.go: [0.60, 0.70]
    split submit.go: [0.50, 0.50]
    RBAC helper: [0.35, 0.65]
    time.go shared: [0.15, 0.35]
    persistence: [0.80, 0.85]
    UserRoleSeeder all providers: [0.30, 0.55]
```

**Do immediately:**
1. Fix `queue_name` double-add bug in `GetJobsInfo` (10 min, high impact)
2. `ioutil` → `os.ReadFile` (1 min)

**Next sprint:**
3. Fix `(error, T)` → `(T, error)` return convention
4. Extract RBAC helpers to reduce duplication in `jobs.go`
5. Split `jobs.go` into focused files

**Backend integration phase:**
6. PostgreSQL persistence via notification handlers
7. `UserRoleSeeder` for PAM/LDAP providers