Remediation: ESPI REST controller modernization (post-#116)

[dfcoffin]

Issue #98 Remediation Plan (originally tracked as PR #115)

Re-aimed 2026-05-20: This plan was originally written against PR #115's branch. PR #115 was closed as superseded by PR #116 (merged 2026-05-20), which landed a renamed and slimmer subset of #115's scope. The dangerous bits that were the original blockers (ManageController shell exec, web/api/RetailCustomerController write methods, web/api/ServiceStatusController) did not merge and remain as .disabled files on main.

The plan below has been re-scoped to target what actually landed in main via #116, plus pre-conditions for safely re-enabling the .disabled files later. File path references generally use the *RESTController.java naming that exists on main; per-phase paths should still be verified at execution time.

Source review: see /review PR #115 findings.

Organized into 8 phases (and one deferred bucket) — each phase is self-contained, can be a separate commit, and has its own acceptance criteria. Ordered by risk-first, then dependency (CI unblock → data correctness → structural cleanup → polish → tests).

---

Phase 1 — De-stream REST controllers (3–4 hr) ⚡ NEXT

Surfaced by PR #118 CI failure: MeterReadingControllerTest.shouldReturn200WhenExists throws java.util.ConcurrentModificationException because MeterReadingController returns a StreamingResponseBody, and the streaming worker thread races with the test thread on MockHttpServletResponse's LinkedCaseInsensitiveMap while Spring Security's OnCommittedResponseWrapper writes security headers.

StreamingResponseBody is used by every GET endpoint PR #116 added (9 controllers, ~30+ endpoints): ApplicationInformationRESTController, CustomerAccountRESTController, CustomerRESTController, IntervalBlockRESTController, ElectricPowerQualitySummaryRESTController, ReadingTypeRESTController, UsagePointController, UsageSummaryRESTController, MeterReadingController.

Why drop it (not just patch the test): What's on main today isn't real streaming — it's StreamingResponseBody wrapping JAXB-marshalled bytes. The service builds the full DTO graph, JAXB marshals it to a complete byte[], and only then does the streaming worker dribble that already-buffered payload out. The whole response is in heap before the first byte hits the wire, so removing StreamingResponseBody loses zero real streaming benefit — including on /Subscription/* endpoints. In exchange, the "fake streaming" costs: race conditions with Spring Security headers (the CME), per-request WebAsyncManager worker thread overhead, permanent asyncDispatch() tax on every MockMvc test, harder operational debugging, forced chunked transfer encoding. It's premature optimization that introduced concrete bugs. Prerequisite for Phases 2 and 3 — subscription scoping is much simpler against sync controllers, and Atom link/UUID work is much cleaner without async thread-context concerns. Also unblocks CI — until this lands, the CME re-surfaces randomly and fails arbitrary PRs.

• 1.1 ApplicationInformationRESTController — replace ResponseEntity<StreamingResponseBody> returns with ResponseEntity<String> (already-marshalled XML) or ResponseEntity<EntryDto> / ResponseEntity<FeedDto> + JAXB message converter
• 1.2 CustomerAccountRESTController
• 1.3 CustomerRESTController
• 1.4 IntervalBlockRESTController
• 1.5 ElectricPowerQualitySummaryRESTController
• 1.6 ReadingTypeRESTController
• 1.7 UsagePointController
• 1.8 UsageSummaryRESTController
• 1.9 MeterReadingController
• 1.10 Remove all import org.springframework.web.servlet.mvc.method.annotation.StreamingResponseBody; lines that are no longer referenced
• 1.11 Update corresponding MockMvc tests: drop any request().asyncStarted() / MvcResult / asyncDispatch() wrappers (added or about to be added to work around the CME)
• 1.12 Run mvn verify -pl openespi-datacustodian -am 3× to confirm no remaining flakes
• 1.13 Confirm MeterReadingControllerTest.shouldReturn200WhenExists passes deterministically

Acceptance: No StreamingResponseBody references remain in openespi-datacustodian controllers; full datacustodian test suite passes 3 consecutive clean runs; no MockMvc test uses asyncDispatch (unless a deliberately-async endpoint exists, which currently it does not).

Production utility implementations of /Subscription/* and /Batch/Bulk/* likely DO need real streaming — a year of 15-min interval data across many usage points for one customer can exceed 100 MB, and a bulk export across thousands of customers is unbounded. But "real streaming" is not StreamingResponseBody + JAXB. Real streaming means:

1. Open a streaming JPA cursor (@QueryHints(@QueryHint(name=HINT_FETCH_SIZE, value="100")) returning Stream<Entity> inside a @Transactional(readOnly=true)) or JdbcTemplate.queryForStream(...)
2. Use a StAX XMLStreamWriter pointed at response.getOutputStream()
3. Write the Atom feed envelope, then per-entry: marshal one DTO → flush() → advance the cursor
4. Write the feed footer and close the stream

That's the architecture that delivers actual memory bounding and progressive transmission. It belongs in production utility forks of this codebase; this Sandbox / reference implementation doesn't need it (response sizes are small by definition of "sandbox", and the test/CI cost of streaming far outweighs the benefit). When a fork does need it, implement it endpoint-by-endpoint with explicit StAX + cursor — do not reach for StreamingResponseBody as a shortcut, since it delivers the test-flake costs without the memory benefits.

---

Phase 2 — Subscription scoping (data-leakage fix) (3–5 hr)

Currently a third-party token gets unfiltered global lists. Model on UsagePointController which does this correctly.

For each controller below, the pattern is:

1. Inject SubscriptionService (or whatever UsagePointController uses)
2. Resolve subscription from access token via ApiAccessValidator.requireSubscriptionId(authHeader)
3. For non-admin scopes, filter the result set to entities belonging to that subscription's authorized usage points
4. Admin scope retains unfiltered access

• 2.1 IntervalBlockRESTController list endpoints
• 2.2 MeterReadingController list endpoints
• 2.3 ElectricPowerQualitySummaryRESTController list endpoints
• 2.4 ReadingTypeRESTController — note ReadingType is shared metadata; verify scoping semantics with the spec before filtering
• 2.5 UsageSummaryRESTController list endpoints
• 2.6 CustomerRESTController list endpoints
• 2.7 CustomerAccountRESTController list endpoints
• 2.8 Add MockMvc test per controller: third-party token sees only their subscription's data; admin token sees all

Acceptance: Each list endpoint has an integration test proving that two different subscription tokens get disjoint result sets, and admin sees the union.

---

Phase 3 — ESPI 4.0 Atom + JSON compliance (4–6 hr)

The most important compliance fix. Touches both code and conventions.

• 3.1 BaseExportService.createServiceStatusEntry (if present on main — verify) — replace selfLink.setHref("ServiceStatus") / upLink.setHref("") with the canonical pattern https://{host}/DataCustodian/espi/1_1/resource/{resource}/{id}. Inject host from config (espi.base-url or equivalent).
• 3.2 Audit every createAtomEntry(title, dto) callsite in DtoExportServiceImpl and ensure the entry has:
  • rel="self" href with full resource URI
  • rel="up" href with full parent collection URI
  • rel="related" hrefs where applicable
• 3.3 Replace all UUID.randomUUID() in Atom-id generation with UUID Version 5 (deterministic from resource URI). Use the project's existing espiIdGeneratorService if available; otherwise add a helper.
• 3.4 Verify entry IDs serialize as urn:uuid:xxxxxxxx-xxxx-5xxx-xxxx-xxxxxxxxxxxx
• 3.5 Remove produces = APPLICATION_JSON_VALUE from every ESPI resource endpoint on main: ApplicationInformationRESTController, AuthorizationController, CustomerAccountRESTController, CustomerRESTController, ElectricPowerQualitySummaryRESTController, IntervalBlockRESTController, MeterReadingController, ReadingTypeRESTController, UsagePointController, UsageSummaryRESTController. Keep XML only — APPLICATION_XML_VALUE or APPLICATION_ATOM_XML_VALUE as appropriate.
• 3.6 Add an integration test that marshals one feed + one entry per resource and validates against openespi-common/src/main/resources/schema/ESPI_4.0/espi.xsd using a Validator from SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI).newSchema(espiXsd)

Acceptance: All ESPI resource feeds/entries pass XSD validation; Atom IDs are UUID v5; no JSON content-type is offered for ESPI resources.

---

Phase 4 — ID type cleanup (1–2 hr)

Wider than originally scoped — Long retailCustomerId is spread across MVC controllers on main, not just one method.

• 4.1 DtoExportService.createRetailCustomerEntry(Long retailCustomerId) → UUID retailCustomerId (verify signature still exists on main)
• 4.2 openespi-datacustodian/.../web/custodian/AssociateUsagePointController lines 65 / 74 — Long retailCustomerId → UUID
• 4.3 openespi-datacustodian/.../web/custodian/RetailCustomerController:85 — Long retailCustomerId → UUID
• 4.4 openespi-datacustodian/.../web/customer/CustomerDownloadMyDataController lines 46 / 64 — Long retailCustomerId → UUID
• 4.5 openespi-datacustodian/.../web/customer/MeterReadingController:45 — Long retailCustomerId → UUID
• 4.6 openespi-datacustodian/.../web/customer/UsagePointController:88 — Long retailCustomerId → UUID
• 4.7 openespi-thirdparty/.../web/MeterReadingController:45 — Long retailCustomerId → UUID
• 4.8 openespi-thirdparty/.../web/UsagePointController:80 — Long retailCustomerId → UUID
• 4.9 openespi-thirdparty/.../web/custodian/RetailCustomerController:90 — Long retailCustomerId → UUID
• 4.10 Final sweep: git grep -nE "Long\\s+\\w*[Cc]ustomerId" openespi-datacustodian openespi-common openespi-thirdparty returns empty
• 4.11 Verify RetailCustomerEntity PK is UUID per CLAUDE.md — if it's still Long, that's a separate (larger) issue: file it as a new issue

Acceptance: No Long-keyed RetailCustomer references in datacustodian, common, or thirdparty modules. All controller path variables and service signatures use UUID.

---

Phase 5 — Constructor + facade structural cleanup (2–3 hr)

Verify scope on main first — the renamed/slimmed #116 may have already partially addressed this. If DtoExportServiceImpl constructor on main is no longer 11-arg, scale this phase down.

• 5.1 Verify on main: count DtoExportServiceImpl constructor parameters. If ≤4, mark this phase N/A. If still large:
  • 5.1a (preferred) Switch DTO tests that enumerate dependencies to Mockito @InjectMocks so they no longer pass null, null, null, …
  • 5.1b Extract a small MarshallerOnlyExporter from DtoExportServiceImpl that the DTO tests can construct cheaply
• 5.2 Verify on main: is DtoExportServiceFacade present? If yes, either delegate all feed/entry methods to DtoExportServiceImpl, or remove DtoExportServiceImpl from the facade's deps and centralize logic in the facade. Do not keep both code paths.
• 5.3 Verify: are there still two createServiceStatusEntry implementations? If yes, collapse into one.
• 5.4 Confirm @Qualifier("dtoExportServiceImpl") resolves correctly (default Spring bean name should match)

Acceptance: Adding a new dependency to DtoExportServiceImpl does not require touching DTO test files. At most one createServiceStatusEntry exists.

---

Phase 6 — Pagination & behavior polish (2 hr)

Verify each item against main first. Several originally-listed controllers (TimeConfigurationController, RetailCustomerController.index(), BatchController) are still .disabled on main — those sub-items should move into the Deferred bucket below.

Applicable on main:

• 6.1 ReadingTypeRESTController list endpoint — implement limit/offset (pass to a paginated repository call) or drop the parameters
• 6.2 OffsetPageable.getPageNumber() (if present on main) — replace offset / limit with Math.floorDiv(offset, limit); add a unit test for non-multiple offsets
• 6.3 ApiAccessValidator.requireSubscriptionId (if present on main) — return 401 UNAUTHORIZED for missing/empty token, 403 FORBIDDEN only for valid-but-insufficient scope

Acceptance: Pagination parameters either work or are removed; HTTP status codes follow spec.

---

Phase 7 — CustomerMapper correctness (1 hr)

Verify scope on main first — confirm CustomerMapper.toDto(RetailCustomerEntity) exists on main with the issue. If the method wasn't merged, defer to when it gets re-introduced.

• 7.1 CustomerMapper.toDto(RetailCustomerEntity) — null-safe name concatenation (skip nulls, trim, return null rather than "null null")
• 7.2 Stop populating OrganisationDto.organisationName with a person name. Either leave it null when the source is a RetailCustomer, or split the mapping into a person-customer branch and an organisation-customer branch
• 7.3 Add a unit test covering: all-null name, first-only, last-only, both, organisation case
• 7.4 Design question to raise: should RetailCustomerEntity even marshal as a Customer (different schema)? If not, deprecate this mapper method and route consumers to the proper resource

Acceptance: Tests cover null permutations; no "null null" artifact possible.

---

Phase 8 — Test fortification (3 hr)

• 8.1 XSD-validation integration test (Phase 3.6) confirmed running in mvn verify
• 8.2 Subscription-scoping tests from Phase 2.8 confirmed running
• 8.3 Run full mvn verify -pl openespi-common,openespi-datacustodian -am clean

Acceptance: All new tests green; no regressions in existing suite.

---

Deferred — Pre-conditions for re-enabling .disabled controllers

These items were originally Phase 2 blockers for PR #115 but did NOT land in main via #116. They live in .disabled files. Apply these fixes as part of the same PR that re-enables each file — never re-enable as-is.

• D.1 web/api/ManageRESTController.java.disabled — remove resetDatabase / initializeDatabase REST endpoints before re-enabling. Two options:
  • Option A (preferred): delete them entirely; move admin operations to a non-HTTP CLI
  • Option B: gate behind SCOPE_DataCustodian_Admin_Access and @Profile("!prod"), and never echo stderr into HTTP response
• D.2 web/api/RetailCustomerRESTController.java.disabled — add @PreAuthorize("hasAuthority('SCOPE_DataCustodian_Admin_Access')") to create(), update(), delete() before re-enabling (or delete those methods if they're still 501 stubs)
• D.3 web/api/ServiceStatusRESTController.java.disabled — replace authHeader.substring(7) with a guarded extraction (startsWith("Bearer ") AND length() > 7 AND non-empty token, else 401) before re-enabling
• D.4 web/api/ApplicationInformationController.createApplicationInformation (if/when added) — add @Valid and bean-validation constraints on clientSecret, redirectUri, scope, tosUri, clientName
• D.5 BatchRESTController.java.disabled — when re-enabling:
  • subscriptionUsagePoint(...) must filter by the subscription's specific usage points, not delegate to subscription(...)
  • upload() / .bulk() must log when called (log.warn("Unimplemented endpoint called: {}", …)) before returning 501
  • downloadCollection(@PathVariable Long retailCustomerId, …) → use UUID (also covered by Phase 4)
  • Streaming approach for /Batch/Bulk/* and /Batch/Subscription/* — these endpoints can legitimately return 100+ MB in production utility deployments. If/when this Sandbox repo needs real streaming for them, implement it as StAX XMLStreamWriter + a streaming JPA cursor (or JdbcTemplate.queryForStream) marshalling one entry at a time. Do not wrap JAXB-marshalled bytes in StreamingResponseBody — that's the fake streaming pattern Phase 1 just removed; it has no memory benefit and brings back the CME footgun. For Sandbox/CI purposes, a synchronous response of bounded test-data size is fine; the streaming path is only worth the engineering investment if a production fork pulls this code as-is.
• D.6 TimeConfigurationRESTController.java.disabled — when re-enabling, honor limit/offset or drop them
• D.7 ServiceStatusController — replace hardcoded applicationStatus = "0" with a DataCustodianApplicationStatus.OK constant/enum
• D.8 Audit token logging in AuthorizationService.findByAccessToken and any new auth-handling code — ensure raw tokens are never logged

Acceptance: Every .disabled file ships with the corresponding security/correctness fix in the same PR.

---

Cross-cutting reviewer checklist (apply per commit)

• Edit-tool diffs only (no sed/awk per CLAUDE.md)
• No new JAXB on entities, no new JPA on DTOs
• No new Long-keyed ESPI entities
• AssertJ assertions chained per CLAUDE.md
• mvn test -pl <touched-module> -am green before each commit
• PR title uses conventional-commit format (feat:, fix:, chore:, etc. with colon) to clear PR Validation

---

Suggested phase grouping into PRs (if breaking up)

• PR-A: Phase 1 (de-stream controllers) — mechanical refactor, unblocks CI, prerequisite for PR-B and PR-C
• PR-B: Phase 2 (data-leakage fix) — high-impact, focused review
• PR-C: Phase 3 (ESPI compliance) — touches many files, dedicated review
• PR-D: Phases 4 + 5 + 6 + 7 (cleanup & polish)
• PR-E: Phase 8 (test fortification) — could fold into A–D, or stand alone

Deferred items (D.1–D.8) ship one-by-one with whichever PR re-enables the corresponding .disabled file.

---

Total estimated effort: ~18–23 hours focused work on the 8 active phases, 5 PRs if split. Deferred items add ~3–5 hours when their respective .disabled files are re-enabled.

---

Change log

• 2026-05-20 Initial plan written against PR feat - Issue 98, implementing remaining controller functionality  #115 review findings.
• 2026-05-20 Phase 2.5 added (de-stream controllers) after PR chore: ignore npm tooling artifacts in .gitignore #118 CI failure exposed the StreamingResponseBody × Spring Security CME pattern.
• 2026-05-20 Plan re-aimed at main after PR feat - Issue 98, implementing remaining controller functionality  #115 closed (superseded by feat: Modernize ESPI REST controllers (Issue #98, no auth-server migration) #116). Dropped original Phases 0 and 1 (no longer applicable). Promoted Phase 2.5 → Phase 1. Renumbered subsequent phases. Moved original Phase 2 security items to Deferred bucket (the dangerous code lives in .disabled files, not on main yet). Updated file path references to *RESTController.java where applicable. Widened Phase 4 ID cleanup scope to include MVC controllers across all three modules.

[dfcoffin]

Phase 1 complete in #120 (merged 2026-05-21 as 797b4dd). CME flake resolved across all 9 controllers / 26 endpoints; CI now deterministic. Phase 2 (subscription scoping) is next.