ciimsproxy/plans/2026-07-08-ciimsproxy-refactor-plan-v2.md

# CIIMS Proxy — Refactor & Refine Plan

> Based on [2026-07-08-ciimsproxy-codebase-analysis.md](./2026-07-08-ciimsproxy-codebase-analysis.md)
> Go 1.22 · 4 source files · 16 tests

---

## Objective

Address the remaining correctness gaps, improve abstraction quality, and optimize performance identified in the codebase analysis. The proxy is already functional and stable; this plan targets the next tier of quality improvements without breaking the existing HTTP API contract. SOAP template cleanup must preserve SOAP semantics; any byte-for-byte wire-format change should be explicit and isolated.

Expected outcomes:

- `receiveMessage` detects and reports CIIMS SOAP faults instead of silently returning empty results.
- HTTP non-2xx responses from CIIMS are treated as errors while still allowing SOAP fault bodies to be parsed and returned.
- SOAP response parsing is hardened for expected XML formatting variations; any retained regex patterns are compiled once at startup, not per request.
- A configured HTTP transport client is injected into handlers/tests instead of relying on package-level mutable client state.
- Deprecated `ioutil.ReadAll` is replaced with `io.ReadAll`.
- Handler-level (Gin endpoint) tests are added.
- Domain types and a `Config` struct improve type safety and testability.
- Template indentation is normalized for readability only if the team accepts the resulting byte-for-byte SOAP request change.

---

## Implementation Plan

### Phase 1 — Correctness (High Priority)

#### 1. Add SOAP Fault Detection to `receiveMessage`

- [ ] In `cmd/main/main.go`, after the outbound CIIMS call in `receiveMessage`, call `internal.GetErrMsg(resp)` and return `500` with the fault text if non-empty.
- [ ] If the outbound call returns a typed HTTP status error with a response body, inspect that body with `internal.GetErrMsg` before falling back to the transport/status error message.
- [ ] Log the SOAP fault at error level, consistent with `sendMessage`.
- [ ] Add a handler-level test in `cmd/main/main_test.go`: mock CIIMS returns `ErrMsg` for a receive call, verify `/receive` returns `500 {"error":"<fault text>"}` instead of `200 {"msgs":[]}`.

#### 2. Check HTTP Status Codes from CIIMS

- [ ] In `internal/http.go`, after `client.Do(req)`, check `resp.StatusCode`.
- [ ] Always read and close the response body once. If status is not 2xx, return the body string alongside a typed error such as `HTTPStatusError{StatusCode, Status, Body}`.
- [ ] Make `HTTPStatusError.Error()` include the status code and a truncated/sanitized body snippet, while still exposing the full body field for SOAP fault parsing by the handler.
- [ ] Update `sendMessage` and `receiveMessage` error handling: when `err != nil`, first parse `resp` or the typed error body for a SOAP fault; if found, return that fault, otherwise return the transport/status error.
- [ ] Add a test: mock CIIMS returns `500 Internal Server Error`, verify `Send()` returns an error.
- [ ] Add a test: mock CIIMS returns HTTP `500` with SOAP fault XML, verify the handler returns the SOAP fault text rather than a generic HTTP status error.
- [ ] Add a test: mock CIIMS returns `200 OK`, verify `Send()` succeeds normally.

### Phase 2 — Performance (High Priority)

#### 3. Harden Response Parsing and Pre-compile Patterns

- [ ] Prefer replacing regex-based parsing in `GetMsgs` and `GetErrMsg` with `encoding/xml.Decoder` token parsing that matches elements by local name (`string`, `errorMessage`) regardless of namespace prefix.
- [ ] If regex is retained as a short-term step, update patterns to handle expected formatting variations such as attributes on `<prefix:string>` and multiline content.
- [ ] If regex is retained, move `regexp.MustCompile(msgExp)` and `regexp.MustCompile(errExp)` from function bodies to package-level `var` declarations.
- [ ] Name them `msgRegex` and `errRegex`.
- [ ] Update `GetMsgs` and `GetErrMsg` to use the pre-compiled variables.
- [ ] Add tests for multiline string payloads, attributes on string elements, whitespace variations, and different namespace prefixes.
- [ ] Verify all existing tests pass without changes unless parser behavior is intentionally broadened.

#### 4. Inject a Configured HTTP Client

- [ ] Do **not** introduce package-level mutable client state such as `var httpClient *http.Client` plus `InitClient`; this risks nil-client bugs and test pollution.
- [ ] In `internal/http.go`, introduce a transport type such as `type Client struct { httpClient *http.Client }`.
- [ ] Add `NewClient(timeoutSec int) (*Client, error)` to validate/default the timeout and construct the underlying `http.Client`.
- [ ] Move outbound send behavior to a method such as `func (c *Client) Send(url, msg string) (string, error)`.
- [ ] Construct the client in `main()` after resolving configuration and pass it to handlers through a `Config`/handler struct or closure.
- [ ] Update tests to create their own `internal.Client` instances so timeout settings do not leak between tests.
- [ ] Add a test that verifies the injected client timeout is honored.
- [ ] Ensure the existing `TestSend_NetworkTimeout` test still passes.

### Phase 3 — Modernization (Medium Priority)

#### 5. Replace Deprecated `ioutil.ReadAll`

- [ ] In `internal/http.go`, replace `ioutil.ReadAll(resp.Body)` with `io.ReadAll(resp.Body)`.
- [ ] Remove the `"io/ioutil"` import and add `"io"`.
- [ ] Verify build and tests pass.

#### 6. Add Handler-Level Tests

- [ ] Create `cmd/main/main_test.go` with Gin's `httptest` setup.
- [ ] Test `POST /send` with valid JSON → mock CIIMS returns success → assert `200 {"error":""}`.
- [ ] Test `POST /send` with valid JSON → mock CIIMS returns SOAP fault → assert `500` with fault text.
- [ ] Test `POST /send` with missing required field → assert `400`.
- [ ] Update handlers to detect `*http.MaxBytesError` from binding and return `http.StatusRequestEntityTooLarge`.
- [ ] Test `POST /send` with body exceeding 1 MB → assert `413`.
- [ ] Test `POST /receive` with valid JSON → mock CIIMS returns messages → assert `200 {"msgs":[...]}`.
- [ ] Test `POST /receive` with `count` = 0 → assert `400`.
- [ ] Test `POST /receive` with `count` = 1001 → assert `400`.
- [ ] Test `POST /receive` with valid JSON → mock CIIMS returns SOAP fault → assert `500`.
- [ ] Test `GET /ping` → assert `200 {"message":"pong"}`.

### Phase 4 — Abstraction (Low Priority)

#### 7. Introduce Domain Types

- [ ] In `internal/codec.go`, define a `CIIMSResponse` struct with a `RawXML string` field.
- [ ] Add methods: `IsFault() bool`, `ErrorMessage() string`, `Messages() []string`.
- [ ] Update the injected client's `Send()` method to return `(*CIIMSResponse, error)` instead of `(string, error)`.
- [ ] Preserve access to any non-2xx response body through `CIIMSResponse.RawXML` and/or the typed status error so SOAP faults in HTTP 500 responses remain parseable.
- [ ] Update `sendMessage` and `receiveMessage` to use the new methods.
- [ ] Update all tests to use the new return type.
- [ ] Ensure no change in external behavior.

#### 8. Introduce a `Config` Struct

- [ ] In `cmd/main/main.go`, define a `Config` struct with fields `ServerURL`, `Timeout`, `Listen`.
- [ ] Include the injected CIIMS client/transport in `Config` or in a handler struct.
- [ ] Parse environment variables into a `Config` value in `main()`.
- [ ] Pass `Config` to handler functions via closure (or a handler struct).
- [ ] Remove package-level `defaultURL` and `timeout` variables.
- [ ] Update `getURL` to accept the server URL as a parameter.
- [ ] Verify all handler tests pass with the new structure.

#### 9. Remove Redundant `Send` Wrapper

- [ ] In `internal/http.go`, delete `postSim` after moving its logic into the injected client method.
- [ ] Decide whether to keep a package-level `Send(url, message, timeout)` compatibility wrapper for legacy internal tests; if kept temporarily, mark it as a thin compatibility helper and keep production handlers on the injected client.
- [ ] Otherwise, update all callers (in `main.go` and tests) to use `client.Send(...)`.
- [ ] Verify build and all tests pass.

### Phase 5 — Cleanup (Low Priority)

#### 10. Normalize Template Indentation

- [ ] Confirm this is acceptable as a byte-for-byte SOAP request change before implementing.
- [ ] In `internal/codec.go`, make `sendtpl` and `receivetpl` use consistent indentation (all spaces or all tabs).
- [ ] Update the golden-file test constants in `internal/codec_test.go` (`SendEsp`, `ReceiveEsp`) to match.
- [ ] Verify `TestSend` and `TestReceive` pass with the updated expected strings.

#### 11. Remove Unused `gommon/log` Dependency

- [ ] In `cmd/main/main.go`, replace `github.com/labstack/gommon/log` with Go's standard `log` package.
- [ ] Replace `log.Infof` → `log.Printf`, `log.Errorf` → `log.Printf("[ERROR] ...")` or use `log` with prefixes.
- [ ] Remove `github.com/labstack/gommon` from `go.mod` and run `go mod tidy`.
- [ ] Verify build and all tests pass.

---

## Verification Criteria

- [ ] `POST /receive` with a CIIMS SOAP fault returns `500 {"error":"<fault text>"}`, not `200 {"msgs":[]}`.
- [ ] CIIMS returning HTTP 500 produces an error from `Send()`, not a successful response.
- [ ] CIIMS returning HTTP 500 with a SOAP fault body returns the SOAP fault text to the caller.
- [ ] Oversized request bodies return `413`, not a generic `400`.
- [ ] `go test ./...` shows all tests passing; response parsing handles multiline/attribute/namespace-prefix variations.
- [ ] HTTP client timeout is configured through an injected client/transport and does not rely on package-level mutable initialization.
- [ ] `ioutil.ReadAll` no longer appears in the codebase.
- [ ] `cmd/main/main_test.go` exists with at least 9 handler-level tests covering all error paths.
- [ ] `internal.CIIMSResponse` type exists with `IsFault()`, `ErrorMessage()`, `Messages()` methods.
- [ ] `cmd/main` has no package-level `defaultURL` or `timeout` variables.
- [ ] `internal/http.go` has no `postSim`; production code uses the injected client `Send` method.
- [ ] `sendtpl` and `receivetpl` use consistent indentation only if the wire-format change was explicitly accepted.
- [ ] `github.com/labstack/gommon` is absent from `go.mod`.
- [ ] `go vet ./...` and `go build ./...` produce no errors.

---

## Execution Order

```
Phase 1 (Correctness) ──► Phase 2 (Performance) ──► Phase 3 (Modernization)
                                                            │
Phase 5 (Cleanup) ◄──────── Phase 4 (Abstraction) ◄────────┘
```

Phases 1 and 2 should be completed first as they fix remaining bugs and transport/testability issues. Phase 4's `Config`/injected-client work may be pulled earlier if it simplifies Phase 2. Phases 3–5 are quality-of-life improvements that can be done in any order, but Phase 4 should precede Phase 5 since removing `gommon/log` may be affected by the `Config` struct refactor.

---

## Potential Risks and Mitigations

1. **HTTP status code checking breaks CIIMS compatibility**
   - Some SOAP services return `500` with a SOAP fault in the body (which is valid SOAP). The current code already handles this via `GetErrMsg`. The fix should check for 2xx and treat everything else as a transport error, but still allow the SOAP fault body to be extracted if present.
   - Mitigation: On non-2xx, return a typed `HTTPStatusError` that exposes the response body and also return/preserve that body in the response value. The handler must parse SOAP faults first, then fall back to the status error.

2. **Injected `http.Client` changes timeout behavior**
   - The current code creates a new client per request with the configured timeout. An injected client has a fixed timeout set at construction.
   - Mitigation: This is the desired production behavior, and tests should construct isolated clients. If per-request timeout overrides are needed later, use `context.WithTimeout` on the request context.

3. **`CIIMSResponse` abstraction changes the return type**
   - All callers of `Send()` must be updated.
   - Mitigation: This is a mechanical refactor. The compiler will catch all call sites. Update them in one pass.

4. **Removing `gommon/log` changes log output format**
   - Standard `log` package has a different default format (timestamps with date/time).
   - Mitigation: This is acceptable—Gin already uses its own logger. Standardizing on `log` reduces dependencies and is more idiomatic.

5. **Template indentation change breaks golden-file tests**
   - `TestSend` and `TestReceive` do exact string comparison.
   - Mitigation: Update the expected constants (`SendEsp`, `ReceiveEsp`) in the same commit. The tests themselves verify correctness.