zhiqiang feng
ce4c2b088f
Bug fixes: - Fix nil-pointer panic in sendMessage SOAP fault handler (used err.Error() on nil) - Fix missing return after SOAP fault response (caused fall-through to 200 OK) - Fix timeout=0 shadowing: removed package-level constant, use configured value - Fix receiveMessage not checking SOAP faults from CIIMS Codec hardening: - Replace fragile regex parsing with encoding/xml.Decoder for namespace-agnostic XML - XML-escape all user inputs (user, pass, event) — not just message body - Remove magic-number-based string slicing (split, GetErrMsg) Transport improvements: - Extract Client type with connection pooling (reuse http.Client across requests) - Add CIIMSResponse domain type with IsFault(), ErrorMessage(), Messages() - Add HTTPStatusError and ResponseTooLargeError typed errors - Replace deprecated ioutil.ReadAll with io.ReadAll - Add response body size limit (64 MiB) Security hardening: - Add MaxBytesReader (1MB request body limit) on both endpoints - Add URL allowlist via CIIMS_ALLOWED_SERVERS env var - Add normalizeBaseURL with scheme/host/query validation - Add count bounds validation (1-1000) on /receive - Add server-level timeouts (ReadHeaderTimeout, ReadTimeout, IdleTimeout) Configuration: - Introduce Config struct to replace package-level globals - Add loadConfig() with full validation and error propagation - Add getIntEnv() with positive-value enforcement Test coverage (75 tests, 35 new): - Phase 1: 8 internal tests (nil receiver, error types, SOAP+HTTP500, malformed XML) - Phase 2: 19 pure function tests (normalizeBaseURL, parseAllowedServers, getIntEnv) - Phase 3: 16 handler tests (send/receive success, SOAP fault, network error, HTTP 500, body too large, invalid JSON, missing fields, URL allowlist) - Phase 4: 8 config/router tests (loadConfig, newServer, newRouter) Toolchain: - Upgrade Go 1.13 → 1.22, gin 1.6.3 → 1.10.0, testify 1.5.1 → 1.10.0 Cleanup: - Remove dead code (unused post() function, commented-out defaults) - Replace println with structured logging - Add .gitignore - Rewrite README with API docs, env vars, security considerations
190 lines
12 KiB
Markdown
190 lines
12 KiB
Markdown
# 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.
|