From bd9f8ca8462b8e3b05794f3dc348eab9c2ca6b27 Mon Sep 17 00:00:00 2001 From: simonredfern Date: Tue, 2 Jun 2026 07:13:35 +0200 Subject: [PATCH 1/3] Removing get banks from v7.0.0 (it waas a regression used for testing http4s) - it will cascade to v6.0.0 --- .../scala/code/api/v6_0_0/Http4s600.scala | 29 ++++++++++++-- .../scala/code/api/v7_0_0/Http4s700.scala | 38 +++---------------- .../code/api/v7_0_0/Http4s700RoutesTest.scala | 8 ++-- 3 files changed, 36 insertions(+), 39 deletions(-) diff --git a/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala b/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala index a4d95448c9..3c20bf7b76 100644 --- a/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala +++ b/obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala @@ -14120,6 +14120,11 @@ object Http4s600 { | |The type field must be one of "STRING", "INTEGER", "DOUBLE" or "DATE_WITH_DAY" | + |Each Personal Data Field is identified by its own USER_ATTRIBUTE_ID. The "name" is not a unique key: + |this endpoint always creates a new field, so the same "name" can occur on multiple fields for the same user + |(e.g. two "phone_number" fields). To change the value of an existing field, use PUT /my/personal-data-fields/USER_ATTRIBUTE_ID + |rather than POSTing again. To list a user's fields and their USER_ATTRIBUTE_IDs, use GET /my/personal-data-fields. + | |${userAuthenticationMessage(true)} |""".stripMargin, code.api.v5_1_0.UserAttributeJsonV510( @@ -14142,7 +14147,11 @@ object Http4s600 { "Get Personal Data Fields", s"""Get Personal Data Fields for the currently authenticated user. | - |Returns Personal Data Fields (IsPersonal=true) that are managed by the user. + |Returns all Personal Data Fields (IsPersonal=true) that are managed by the user, as a list. + | + |Each field has its own USER_ATTRIBUTE_ID. The "name" is not a unique key, so the list may contain + |multiple fields with the same "name" (each a distinct USER_ATTRIBUTE_ID). Use the USER_ATTRIBUTE_ID + |to fetch, update or delete a specific field. | |${userAuthenticationMessage(true)} |""".stripMargin, @@ -14162,7 +14171,10 @@ object Http4s600 { "GET", "/my/personal-data-fields/USER_ATTRIBUTE_ID", "Get Personal Data Field By Id", - s"""Get a Personal Data Field by USER_ATTRIBUTE_ID for the currently authenticated user. + s"""Get a single Personal Data Field by USER_ATTRIBUTE_ID for the currently authenticated user. + | + |USER_ATTRIBUTE_ID is the unique identifier of the field (not its "name"). Obtain it from + |GET /my/personal-data-fields. Returns 404 if no field with that USER_ATTRIBUTE_ID belongs to the user. | |${userAuthenticationMessage(true)} |""".stripMargin, @@ -14180,7 +14192,12 @@ object Http4s600 { "PUT", "/my/personal-data-fields/USER_ATTRIBUTE_ID", "Update Personal Data Field", - s"""Update a Personal Data Field by USER_ATTRIBUTE_ID for the currently authenticated user. + s"""Update a single Personal Data Field by USER_ATTRIBUTE_ID for the currently authenticated user. + | + |USER_ATTRIBUTE_ID identifies the exact field to update; this updates that one field in place and never + |creates a new one. The body's "name", "type" and "value" all replace the existing field's values, so a + |field can be renamed by changing "name". Returns 404 if no field with that USER_ATTRIBUTE_ID belongs to the user. + |The type field must be one of "STRING", "INTEGER", "DOUBLE" or "DATE_WITH_DAY". | |${userAuthenticationMessage(true)} |""".stripMargin, @@ -14207,7 +14224,11 @@ object Http4s600 { "DELETE", "/my/personal-data-fields/USER_ATTRIBUTE_ID", "Delete Personal Data Field", - s"""Delete a Personal Data Field by USER_ATTRIBUTE_ID for the currently authenticated user. + s"""Delete a single Personal Data Field by USER_ATTRIBUTE_ID for the currently authenticated user. + | + |USER_ATTRIBUTE_ID identifies the exact field to delete; only that one field is removed. If several fields + |share the same "name", deleting one leaves the others intact. Returns 404 if no field with that + |USER_ATTRIBUTE_ID belongs to the user. | |${userAuthenticationMessage(true)} |""".stripMargin, diff --git a/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala b/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala index dfc9528fce..e8e942a9a8 100644 --- a/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala +++ b/obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala @@ -202,38 +202,12 @@ object Http4s700 { http4sPartialFunction = Some(root) ) - // Route: GET /obp/v7.0.0/banks - val getBanks: HttpRoutes[IO] = HttpRoutes.of[IO] { - case req @ GET -> `prefixPath` / "banks" => - EndpointHelpers.executeAndRespond(req) { implicit cc => - for { - (banks, callContext) <- NewStyle.function.getBanks(Some(cc)) - } yield JSONFactory400.createBanksJson(banks) - } - } - - resourceDocs += ResourceDoc( - null, - implementedInApiVersion, - nameOf(getBanks), - "GET", - "/banks", - "Get Banks", - s"""Get banks on this API instance - |Returns a list of banks supported on this server: - | - |* ID used as parameter in URLs - |* Short and full name of bank - |* Logo URL - |* Website""", - EmptyBody, - banksJSON, - List( - UnknownError - ), - apiTagBank :: Nil, - http4sPartialFunction = Some(getBanks) - ) + // Note: GET /obp/v7.0.0/banks is intentionally NOT defined here. + // The v7 implementation used the older v4.0.0 shape (BanksJson400: `id`, `short_name`), + // which is behind v6.0.0's BanksJsonV600 (`bank_id`, `bank_code`). Rather than duplicate + // (and drift from) the v6 shape, we let the request fall through to `v700ToV600Bridge`, + // which rewrites /obp/v7.0.0/banks → /obp/v6.0.0/banks and serves Http4s600.getBanks, + // tagging the response `X-OBP-Version-Served: v6.0.0`. v7 thus inherits the latest shape. // Note: resource-docs requests (`GET /obp/v7.0.0/resource-docs/...`) are intercepted by // `Http4sResourceDocs.routes`, which is registered earlier in `Http4sApp.baseServices` diff --git a/obp-api/src/test/scala/code/api/v7_0_0/Http4s700RoutesTest.scala b/obp-api/src/test/scala/code/api/v7_0_0/Http4s700RoutesTest.scala index 77678eb7a2..476de674b6 100644 --- a/obp-api/src/test/scala/code/api/v7_0_0/Http4s700RoutesTest.scala +++ b/obp-api/src/test/scala/code/api/v7_0_0/Http4s700RoutesTest.scala @@ -211,7 +211,9 @@ class Http4s700RoutesTest extends ServerSetupWithTestData { Given("GET /obp/v7.0.0/banks request") val (statusCode, json, _) = makeHttpRequest("/obp/v7.0.0/banks") - Then("Each bank has id, short_name, full_name, logo, website") + // /obp/v7.0.0/banks cascades to v6.0.0 via v700ToV600Bridge, so the response + // uses the v6 BanksJsonV600 shape: bank_id / bank_code (not v4's id / short_name). + Then("Each bank has bank_id, bank_code, full_name, logo, website") statusCode shouldBe 200 json match { case JObject(fields) => @@ -220,8 +222,8 @@ class Http4s700RoutesTest extends ServerSetupWithTestData { banks.headOption match { case Some(JObject(bankFields)) => val keys = bankFields.map(_.name) - keys should contain("id") - keys should contain("short_name") + keys should contain("bank_id") + keys should contain("bank_code") keys should contain("full_name") case _ => fail("Expected bank to be a JSON object") From 31b1fc434979f67591484b6cf1be455982a07999 Mon Sep 17 00:00:00 2001 From: simonredfern Date: Tue, 2 Jun 2026 07:43:33 +0200 Subject: [PATCH 2/3] Create todo_remove_duplicate_v7.0.0_endpoints.md --- todo_remove_duplicate_v7.0.0_endpoints.md | 207 ++++++++++++++++++++++ 1 file changed, 207 insertions(+) create mode 100644 todo_remove_duplicate_v7.0.0_endpoints.md diff --git a/todo_remove_duplicate_v7.0.0_endpoints.md b/todo_remove_duplicate_v7.0.0_endpoints.md new file mode 100644 index 0000000000..6b139b7d2f --- /dev/null +++ b/todo_remove_duplicate_v7.0.0_endpoints.md @@ -0,0 +1,207 @@ +# TODO: Remove duplicate v7.0.0 endpoints (handoff) + +**Status:** In progress — committed `getBanks` fix + uncommitted removal of 19 identical endpoints. +**Date:** 2026-06-02 +**Context:** Several endpoints were added to `Http4s700.scala` purely as http4s migration scaffolding +(the file literally labels them `// ── POC endpoints — one per EndpointHelper category ──`). +Where a v7 endpoint is *behaviourally identical* to an earlier version, it should not live in v7 — +it should cascade. Where v7 *intentionally changed/improved* behaviour, it must stay. + +--- + +## The rule + +> Remove a v7.0.0 endpoint **only if it is behaviourally identical** to an earlier version. +> If v7 changed/improved anything (response codes, shape, behaviour), **keep it in v7**. + +## How the cascade works (why deletion is safe for identical endpoints) + +`Http4s700.scala` defines `v700ToV600Bridge` (search the file for it). Any unmatched +`/obp/v7.0.0/*` request is rewritten to `/obp/v6.0.0/*` and served by `Http4s600`, tagged +with response header `X-OBP-Version-Served: v6.0.0`. The bridge chain is fully continuous: + +``` +v700 → v600 → v510 → v500 → v400 → v310 → v300 → v220 → v210 → v200 → v140 → v130 +``` + +So deleting a v7 endpoint cascades the request down to wherever that endpoint is actually +defined (e.g. `getExplicitCounterpartyById` has no v5/v6 successor and cascades to its v4 home, +which is the newest shape that exists). Routing is by `resourceDocs` (URL+verb) — removing the +`val` + its `resourceDocs += ResourceDoc(...)` block removes it from both routing and the +`v7ResourceDocIndex` the bridge consults, so the cascade engages automatically. + +--- + +## Work done so far + +### 1. `getBanks` — committed (`bd9f8ca84`) +This one was a **regression**, not an improvement: v7 served the older v4 shape +(`BanksJson400`: `id`, `short_name`) while v6 has the newer `BanksJsonV600` (`bank_id`, +`bank_code`). Deleted from v7 so it cascades to v6's correct shape. Test +`Http4s700RoutesTest` updated to assert `bank_id`/`bank_code`. **Already committed.** + +### 2. 19 identical endpoints — removed (UNCOMMITTED working-tree change to `Http4s700.scala`) +Each uses the same JSON factory as its lower-version twin and all their v7 test scenarios +still pass when served via the cascade: + +``` +getBank, getCurrentUser, getCoreAccountById, getPrivateAccountByIdFull, +getExplicitCounterpartyById, getFeatures, getConnectors, getProviders, getUsers, +getCustomersAtOneBank, getCustomerByCustomerId, getAccountsAtBank, getCacheConfig, +getCacheInfo, getDatabasePoolInfo, getStoredProcedureConnectorHealth, getMigrations, +getCacheNamespaces, getScannedApiVersions +``` + +`Http4s700.scala`: 4061 → 3370 lines; resourceDoc count 64 → 45. +Main + test compile clean; `Http4s700RoutesTest` → **141/141 pass**. + +### 3. 3 endpoints KEPT in v7 (NOT identical — improved response codes) +These were caught because deleting them turned their test scenarios red: + +| Endpoint | v7 behaviour (kept) | cascaded v6 would give | +|---|---|---| +| `deleteEntitlement` | **204** No Content | 200 (yields `""`) | +| `addEntitlement` (duplicate role) | **409** Conflict | 400 | +| `getUserByUserId` (missing user) | **404** Not Found | 400 (`unboxFullOrFail` default code) | + +v6 codes follow the older OBP convention (200-for-DELETE, 400-for-missing). v7's 204/409/404 +are the more RESTful, intentional choices — so they stay. + +--- + +## How "identical" was checked (and the GAP — please read) + +Two layers were used: + +1. **Static shape audit** — compared each v7 handler's `yield` factory/case-class against the + lower-version handler's. This catches *shape* regressions (it's how `getBanks` was found) but + only compares the **output type**, not the full handler. It did **not** diff auth helpers, + declared roles, query-param parsing, error lists, or *which* data is passed into the factory. + +2. **Bridge-routed test run (the decisive check)** — `Http4s700RoutesTest` drives + `Http4s700.wrappedRoutesV700Services`, which *includes* `v700ToV600Bridge`. So deleted + endpoints' test requests are actually served by the lower version. Passing = the cascaded + response satisfied that test's assertions. This is exactly how the 3 non-identical endpoints + were found. + +**⚠️ The gap:** "identical" here means *equivalent up to what the v7 tests assert*. Per-endpoint +test depth was **not** audited. If one of the 19 has an untested query-param filter, error path, +or auth edge case that the lower version handles differently, the cascade would diverge silently +and neither check would have caught it. + +### Recommended next step (to make it rigorous) +Before/instead of trusting the test coverage, do a runtime equivalence diff per endpoint: + +1. On the pre-deletion build, capture each of the 19 endpoints' responses across an input matrix: + valid request, missing resource, unauthenticated, missing-role, bad query params, malformed body. +2. On the post-deletion (cascade) build, capture the same matrix. +3. Diff status code + body + relevant headers (ignore `X-OBP-Version-Served`). Any non-empty diff = + not identical → that endpoint should be restored to v7 (like the 3 above). + +The git diff `bd9f8ca84^..bd9f8ca84` (getBanks) and the current uncommitted change show the exact +block-removal pattern: delete the `val NAME: HttpRoutes[IO] = ...` plus its +`resourceDocs += ResourceDoc(... http4sPartialFunction = Some(NAME))` block. + +--- + +## Files +- `obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala` — endpoint defs + `v700ToV600Bridge` +- `obp-api/src/main/scala/code/api/v6_0_0/Http4s600.scala` — cascade target for most +- `obp-api/src/test/scala/code/api/v7_0_0/Http4s700RoutesTest.scala` — the suite (in-process, hits the bridge) +- Lower-version files (`Http4s500/400/...`) — cascade targets for endpoints with no v6 twin + +## Decisions for the next developer +- Confirm the 19 with the runtime-diff matrix above (or accept test-coverage-bounded equivalence). +- Decide whether `deleteEntitlement`/`addEntitlement`/`getUserByUserId` should instead have their + improved codes **ported down into v6** (v6 is not yet STABLE) and then be removed from v7 too — + that would make v6 canonical with the better semantics. Current choice: keep them in v7. + +> **NOTE:** The working-tree removal of the 19 endpoints was **reverted** — `Http4s700.scala` is +> back at HEAD (`getBanks` cascade still committed in `bd9f8ca84`; the other 19 are present again). +> Use the evidence table + script below to redo the removal from scratch. + +--- + +## Appendix A — Per-endpoint evidence (the 19 identical removals) + +All 19 build the **same JSON factory** as their lower-version twin (so same response shape). +Line numbers are in `Http4s600.scala` unless noted. `getExplicitCounterpartyById` has no +v5/v6 successor — v4 is the newest shape — so it cascades two extra hops to its v4 home. + +| v7 endpoint | verb + URL | factory (v7 == twin) | cascades to | +|---|---|---|---| +| getBank | GET /banks/BANK_ID | createBankJsonV600 | v6 `getBank` | +| getCurrentUser | GET /users/current | createUserInfoJSON | v6 `getCurrentUser` | +| getCoreAccountById | GET /my/banks/BANK_ID/accounts/ACCOUNT_ID/account | createModeratedCoreAccountJsonV600 | v6 `getCoreAccountByIdV600` | +| getPrivateAccountByIdFull | GET /banks/BANK_ID/accounts/ACCOUNT_ID/VIEW_ID/account | createBankAccountJSON600 | v6 `getPrivateAccountByIdFull` | +| getExplicitCounterpartyById | GET .../counterparties/COUNTERPARTY_ID | createCounterpartyWithMetadataJson400 | **v4** `Http4s400` (newest shape) | +| getFeatures | GET /features | FeaturesJsonV600 | v6 `getFeatures` | +| getConnectors | GET /system/connectors | createConnectorsJson | v6 `getConnectors` | +| getProviders | GET /providers | createProvidersJson | v6 `getProviders` | +| getUsers | GET /users | createUsersInfoJsonV600 | v6 `getUsers` | +| getCustomersAtOneBank | GET /banks/BANK_ID/customers | createCustomersJson | v6 `getCustomersAtOneBank` | +| getCustomerByCustomerId | GET /banks/BANK_ID/customers/CUSTOMER_ID | createCustomerWithAttributesJson | v6 `getCustomerByCustomerId` | +| getAccountsAtBank | GET /banks/BANK_ID/accounts | BasicAccountsJsonV600 | v6 `getAccountsAtBank` | +| getCacheConfig | GET /system/cache/config | createCacheConfigJsonV600 | v6 `getCacheConfig` | +| getCacheInfo | GET /system/cache/info | createCacheInfoJsonV600 | v6 `getCacheInfo` | +| getDatabasePoolInfo | GET /system/database/pool | createDatabasePoolInfoJsonV600 | v6 `getDatabasePoolInfo` | +| getStoredProcedureConnectorHealth | GET /system/connectors/stored_procedure_vDec2019/health | StoredProcedureConnectorHealthJsonV600 | v6 `getStoredProcedureConnectorHealth` | +| getMigrations | GET /system/migrations | createMigrationScriptLogsJsonV600 | v6 `getMigrations` | +| getCacheNamespaces | GET /system/cache/namespaces | createCacheNamespacesJsonV600 | v6 `getCacheNamespaces` | +| getScannedApiVersions | GET /api/versions | ScannedApiVersionJsonV600 | v6 `getScannedApiVersions` | + +**Do NOT remove these 3** (improved codes — see table in the main body): +`deleteEntitlement` (204), `addEntitlement` (409), `getUserByUserId` (404). + +## Appendix B — Reproducible removal script + +Each endpoint is a `val NAME: HttpRoutes[IO] = ...` immediately followed by its +`resourceDocs += ResourceDoc(... http4sPartialFunction = Some(NAME))` block. The script below +deletes both (plus any immediately-preceding `//` comment lines and one trailing blank line), +asserts no block overlaps, and prints what it removed. It is idempotent on the names listed. + +```python +import re +f = "obp-api/src/main/scala/code/api/v7_0_0/Http4s700.scala" +lines = open(f).read().split("\n") + +# 19 IDENTICAL endpoints only — the 3 behaviourally-improved ones are deliberately NOT here +targets = ["getBank","getCurrentUser","getCoreAccountById","getPrivateAccountByIdFull", +"getExplicitCounterpartyById","getFeatures","getConnectors","getProviders","getUsers", +"getCustomersAtOneBank","getCustomerByCustomerId","getAccountsAtBank","getCacheConfig", +"getCacheInfo","getDatabasePoolInfo","getStoredProcedureConnectorHealth","getMigrations", +"getCacheNamespaces","getScannedApiVersions"] + +val_re = {t: re.compile(rf"^\s*(lazy )?val {t}: HttpRoutes\[IO\]") for t in targets} +some_re = {t: re.compile(rf"http4sPartialFunction = Some\({t}\)\s*$") for t in targets} + +blocks = [] +for t in targets: + vi = next(i for i,l in enumerate(lines) if val_re[t].search(l)) + si = next(i for i,l in enumerate(lines) if some_re[t].search(l)) + assert si > vi, t + ci = next(i for i in range(si+1, len(lines)) if re.match(r"^\s*\)\s*$", lines[i])) + start = vi + while start-1 >= 0 and re.match(r"^\s*//", lines[start-1]): + start -= 1 + blocks.append((start, ci, t)) + +blocks.sort() +for (a,b,t),(c,d,t2) in zip(blocks, blocks[1:]): + assert b < c, f"OVERLAP {t} vs {t2}" + +to_del = set() +for a,b,t in blocks: + end = b + if end+1 < len(lines) and lines[end+1].strip()=="": + end += 1 + to_del.update(range(a, end+1)) + +open(f,"w").write("\n".join(l for i,l in enumerate(lines) if i not in to_del)) +print(f"removed {len(to_del)} lines; resourceDoc count should drop 64 -> 45") +``` + +After running: `mvn test-compile -pl obp-api -am -q` then +`mvn test -pl obp-api -q -DwildcardSuites="code.api.v7_0_0.Http4s700RoutesTest" -DfailIfNoTests=false` +— expect **141/141 pass**. If any scenario goes red, that endpoint is NOT identical → restore it +to v7 (that is precisely how the 3 kept endpoints were identified). From 74ec0040014da3f225e8e3620d455b95fb01e9d5 Mon Sep 17 00:00:00 2001 From: simonredfern Date: Tue, 2 Jun 2026 07:54:49 +0200 Subject: [PATCH 3/3] fixtest after removing getBanks in v7.0.0 And notes in todo about removing duplicate endpoints --- .../Http4sServerIntegrationTest.scala | 4 +- ...eDocMiddlewareEnableDisablePropsTest.scala | 19 ++++++---- todo_remove_duplicate_v7.0.0_endpoints.md | 38 ++++++++++++++++++- 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/obp-api/src/test/scala/code/api/http4sbridge/Http4sServerIntegrationTest.scala b/obp-api/src/test/scala/code/api/http4sbridge/Http4sServerIntegrationTest.scala index b7b171bfbc..2916691d36 100644 --- a/obp-api/src/test/scala/code/api/http4sbridge/Http4sServerIntegrationTest.scala +++ b/obp-api/src/test/scala/code/api/http4sbridge/Http4sServerIntegrationTest.scala @@ -257,8 +257,8 @@ class Http4sServerIntegrationTest extends ServerSetup with DefaultUsers with Ser And("X-OBP-Version-Served header indicates the fallback version") versionServed should equal(Some("v6.0.0")) - When("We request a native v7.0.0 endpoint (/banks is migrated)") - val (_, _, nativeVersionServed) = makeHttp4sGetRequestFull("/obp/v7.0.0/banks") + When("We request a native v7.0.0 endpoint (/root is native to v7; /banks was removed and now cascades to v6)") + val (_, _, nativeVersionServed) = makeHttp4sGetRequestFull("/obp/v7.0.0/root") Then("Native v7 endpoints do not set X-OBP-Version-Served") nativeVersionServed should equal(None) diff --git a/obp-api/src/test/scala/code/api/util/http4s/ResourceDocMiddlewareEnableDisablePropsTest.scala b/obp-api/src/test/scala/code/api/util/http4s/ResourceDocMiddlewareEnableDisablePropsTest.scala index c7fc9b3d7e..1c7b4f8058 100644 --- a/obp-api/src/test/scala/code/api/util/http4s/ResourceDocMiddlewareEnableDisablePropsTest.scala +++ b/obp-api/src/test/scala/code/api/util/http4s/ResourceDocMiddlewareEnableDisablePropsTest.scala @@ -60,10 +60,13 @@ class ResourceDocMiddlewareEnableDisablePropsTest extends ServerSetup with Given // OperationIds match `APIUtil.buildOperationId(v, partialFunctionName)` → // s"$fullyQualifiedVersion-$name". v7.0.0's fully qualified form is "OBPv7.0.0". private val rootOpId = "OBPv7.0.0-root" - private val getBanksOpId = "OBPv7.0.0-getBanks" + // A second NATIVE v7 endpoint. getBanks was removed from v7 (it now cascades to v6), + // so it can no longer serve as a "native v7" allowlist target. /api/versions is public, + // native to v7, and needs no DB — the same properties that make /root suitable here. + private val versionsOpId = "OBPv7.0.0-getScannedApiVersions" - private val rootPath = "/obp/v7.0.0/root" - private val banksPath = "/obp/v7.0.0/banks" + private val rootPath = "/obp/v7.0.0/root" + private val versionsPath = "/obp/v7.0.0/api/versions" private def get(path: String): Int = { val req = Request[IO](Method.GET, Uri.unsafeFromString(path), headers = Headers.empty, @@ -98,12 +101,12 @@ class ResourceDocMiddlewareEnableDisablePropsTest extends ServerSetup with Given status shouldBe 404 And("other endpoints in the same version are unaffected") - get(banksPath) shouldBe 200 + get(versionsPath) shouldBe 200 } scenario("api_enabled_endpoints contains a different operationId → 404 for non-listed", EnableDisablePropsTag) { - Given(s"api_enabled_endpoints=[$getBanksOpId] (root is NOT listed)") - setPropsValues("api_enabled_endpoints" -> s"[$getBanksOpId]") + Given(s"api_enabled_endpoints=[$versionsOpId] (root is NOT listed)") + setPropsValues("api_enabled_endpoints" -> s"[$versionsOpId]") When("requesting GET /obp/v7.0.0/root") val rootStatus = get(rootPath) @@ -112,7 +115,7 @@ class ResourceDocMiddlewareEnableDisablePropsTest extends ServerSetup with Given rootStatus shouldBe 404 And("the explicitly enabled endpoint still serves") - get(banksPath) shouldBe 200 + get(versionsPath) shouldBe 200 } scenario("api_enabled_endpoints contains the operationId → endpoint serves", EnableDisablePropsTag) { @@ -132,7 +135,7 @@ class ResourceDocMiddlewareEnableDisablePropsTest extends ServerSetup with Given When("requesting two unrelated v7 endpoints directly against Http4s700's wrapped routes") val rootStatus = get(rootPath) - val banksStatus = get(banksPath) + val banksStatus = get(versionsPath) Then("both still serve — the middleware deliberately ignores version-level Props") And("(the prefix-level gate lives in Http4sApp.gate, which this test bypasses)") diff --git a/todo_remove_duplicate_v7.0.0_endpoints.md b/todo_remove_duplicate_v7.0.0_endpoints.md index 6b139b7d2f..9ac5187030 100644 --- a/todo_remove_duplicate_v7.0.0_endpoints.md +++ b/todo_remove_duplicate_v7.0.0_endpoints.md @@ -1,6 +1,7 @@ # TODO: Remove duplicate v7.0.0 endpoints (handoff) -**Status:** In progress — committed `getBanks` fix + uncommitted removal of 19 identical endpoints. +**Status:** `getBanks` removal committed (`bd9f8ca84`) + CI follow-up fixes (see Appendix C). +The 19-endpoint removal was prototyped then **reverted** — not yet done. Redo it via Appendix A/B. **Date:** 2026-06-02 **Context:** Several endpoints were added to `Http4s700.scala` purely as http4s migration scaffolding (the file literally labels them `// ── POC endpoints — one per EndpointHelper category ──`). @@ -205,3 +206,38 @@ After running: `mvn test-compile -pl obp-api -am -q` then `mvn test -pl obp-api -q -DwildcardSuites="code.api.v7_0_0.Http4s700RoutesTest" -DfailIfNoTests=false` — expect **141/141 pass**. If any scenario goes red, that endpoint is NOT identical → restore it to v7 (that is precisely how the 3 kept endpoints were identified). + +**⚠️ `Http4s700RoutesTest` alone is NOT enough** — see Appendix C. Other suites reference v7 URLs as +fixtures and will break when you remove an endpoint they assumed was native. Run, at minimum: +`code.api.v7_0_0.Http4s700RoutesTest`, `code.api.v7_0_0.V7ResourceDocsAggregationTest`, +`code.api.v7_0_0.Http4s700TransactionTest`, `code.api.http4sbridge.Http4sServerIntegrationTest`, +`code.api.http4sbridge.Http4sLiftBridgePropertyTest`, +`code.api.util.http4s.ResourceDocMiddlewareEnableDisablePropsTest`. + +--- + +## Appendix C — Testing anti-pattern this exposed (READ BEFORE removing more) + +It is unusual for OBP to test an endpoint at a version that does **not** define it. The v7 suite does, +because the duplicated "POC" endpoints were tested at their **v7 URLs** to exercise the http4s +middleware — so tests assert `/obp/v7.0.0/banks` *behaviour* even though banks' canonical home is v6. +The instant an endpoint cascades, those tests are exercising the wrong layer. That is exactly what +broke CI after the `getBanks` removal (`bd9f8ca84`): the commit fixed `Http4s700RoutesTest` but missed +two other suites that hard-coded `/obp/v7.0.0/banks` as a *native* v7 endpoint. + +**The two CI follow-up fixes (already done, in the working tree):** + +| Suite (shard) | What it wrongly assumed | Fix | +|---|---|---| +| `Http4sServerIntegrationTest` (4) | a native v7 endpoint sets **no** `X-OBP-Version-Served`; used `/v7.0.0/banks` — which now cascades and sets `v6.0.0` | repointed the native-endpoint probe to `/obp/v7.0.0/root` | +| `ResourceDocMiddlewareEnableDisablePropsTest` (3) | allowlist target `OBPv7.0.0-getBanks` exists | repointed to `getScannedApiVersions` / `/obp/v7.0.0/api/versions` | + +**Rule of thumb when removing each of the 19 — classify every failing/affected v7 test:** + +| Test kind | Example | Action | +|---|---|---| +| Asserts an **endpoint's behaviour** at a non-defining version | `Http4s700RoutesTest` "banks list"; `V7ResourceDocsAggregationTest` banks scenario | **Delete** the v7 scenario — coverage belongs at the endpoint's home-version suite (e.g. v6's `getBanks` test). Do **not** repoint it to assert v6 shape via the cascade (that just re-creates the anti-pattern). | +| Asserts **infrastructure** (routing, cascade header, enable/disable middleware, CORS) and merely needs *a* native endpoint as a fixture | the two fixed above; `Http4sServerIntegrationTest` CORS/native probes | **Keep**, but pin the fixture to an endpoint genuinely native to v7 (`/root`, `/api/versions`). Never use a cascaded URL as the "native" fixture. | + +Net: removing the 19 should generally **shrink** the v7 test surface (delete behavioural scenarios), +not migrate it. Only the handful of genuine infra tests stay, repointed to native v7 fixtures.