From 6ad14de880bf10490989b5cf77450b3c931ea245 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 31 Aug 2026 15:46:05 +0200 Subject: [PATCH 01/12] fix: include Berlin Group v1.3 alias docs in the global resource-doc registry The resource-docs dispatcher serves the BG v1.3 alias (active only when berlin_group_v1_3_alias_path is set) through its ScannedApis registration, but APIUtil.allStaticResourceDocs never included it. Its docs carry their own operation ids, re-derived from the alias version string, so alias operation ids failed the getAllResourceDocs membership check used by api-collection-endpoint creation and other operation-id lookups -- the same gap BGv2 had before it was added to this union. Reproduced against a running instance with the alias prop set (OBP-40048 on a valid alias operation id) and confirmed the fix resolves it. --- obp-api/src/main/scala/code/api/util/APIUtil.scala | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/obp-api/src/main/scala/code/api/util/APIUtil.scala b/obp-api/src/main/scala/code/api/util/APIUtil.scala index 9e45f49432..b3d685f30b 100644 --- a/obp-api/src/main/scala/code/api/util/APIUtil.scala +++ b/obp-api/src/main/scala/code/api/util/APIUtil.scala @@ -4892,6 +4892,12 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{ // ++ code.api.MxOF.OBP_MXOF_1_0_0.allResourceDocs // ++ code.api.BahrainOBF.v1_0_0.ApiCollector.allResourceDocs ++ code.api.berlin.group.v1_3.OBP_BERLIN_GROUP_1_3.allResourceDocs + // The BG v1.3 alias (active only when berlin_group_v1_3_alias_path is set) is served by + // the resource-docs dispatcher via its ScannedApis registration, but its docs carry their + // own operation ids (re-derived from the alias version), so it hit the same + // getAllResourceDocs membership gap as BGv2 below. Empty when the prop is unset, so this + // is a no-op on instances without the alias configured. + ++ code.api.berlin.group.v1_3.OBP_BERLIN_GROUP_1_3_Alias.allResourceDocs // BGv2 was missing here even though /resource-docs/BGv2 serves it, so a BGv2 operation id // (e.g. BGv2-getAccountDetails) failed the getAllResourceDocs membership check that // api-collection-endpoints (and anything else resolving operation ids) relies on. From f1e9f4ea66f562bef5d518cbe247d9e8d889b469 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 31 Aug 2026 15:48:54 +0200 Subject: [PATCH 02/12] test: cover Berlin Group v1.3 alias in resource-doc registry parity test The alias surface is gated by berlin_group_v1_3_alias_path, which is unset in the default test environment, so its operation-id list is legitimately empty there -- skip the non-empty assertion for it while still running the membership check against getAllResourceDocs. --- .../util/ResourceDocRegistryParityTest.scala | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala index 520d04a223..fa0e25247a 100644 --- a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala +++ b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala @@ -11,11 +11,12 @@ import org.scalatest.Tag * * These are two parallel registries (ResourceDocsAPIMethods dispatches per * standard/version; getAllResourceDocs aggregates them all), and they have drifted - * twice: Berlin Group v2 was served by the dispatcher but missing from the global - * registry (so BGv2-getAccountDetails could not be added to an API collection), - * and the global registry was based on the v6 aggregation, excluding v7-only - * operation ids. When you add a NEW API standard, register its docs in BOTH - * places — and add its surface to this list. + * three times: Berlin Group v2 was served by the dispatcher but missing from the + * global registry (so BGv2-getAccountDetails could not be added to an API + * collection), the global registry was based on the v6 aggregation, excluding + * v7-only operation ids, and the Berlin Group v1.3 alias (active only when + * berlin_group_v1_3_alias_path is set) was missing too. When you add a NEW API + * standard, register its docs in BOTH places — and add its surface to this list. */ class ResourceDocRegistryParityTest extends ServerSetup { @@ -24,19 +25,24 @@ class ResourceDocRegistryParityTest extends ServerSetup { private lazy val allOperationIds: Set[String] = APIUtil.getAllResourceDocs.map(_.operationId).toSet - private lazy val surfaces: List[(String, Seq[String])] = List( - ("OBP standard (v7 aggregation)", code.api.v7_0_0.Http4s700.allResourceDocs.map(_.operationId).toSeq), - ("Berlin Group v1.3", code.api.berlin.group.v1_3.Http4sBGv13.resourceDocs.map(_.operationId).toSeq), - ("Berlin Group v2", code.api.berlin.group.v2.Http4sBGv2.resourceDocs.map(_.operationId).toSeq), - ("UK Open Banking 2.0.0", code.api.UKOpenBanking.v2_0_0.OBP_UKOpenBanking_200.allResourceDocs.map(_.operationId).toSeq), - ("UK Open Banking 3.1.0", code.api.UKOpenBanking.v3_1_0.OBP_UKOpenBanking_310.allResourceDocs.map(_.operationId).toSeq), - ("UK Open Banking 4.0.1", code.api.UKOpenBanking.v4_0_1.OBP_UKOpenBanking_401.allResourceDocs.map(_.operationId).toSeq) + // optional=true surfaces are gated by a prop that is unset by default (test props + // included), so their operation-id list is legitimately empty in most environments -- + // the membership check still runs (trivially true when empty) but the non-empty + // assertion is skipped for them. + private lazy val surfaces: List[(String, Seq[String], Boolean)] = List( + ("OBP standard (v7 aggregation)", code.api.v7_0_0.Http4s700.allResourceDocs.map(_.operationId).toSeq, false), + ("Berlin Group v1.3", code.api.berlin.group.v1_3.Http4sBGv13.resourceDocs.map(_.operationId).toSeq, false), + ("Berlin Group v1.3 alias", code.api.berlin.group.v1_3.Http4sBGv13Alias.resourceDocs.map(_.operationId).toSeq, true), + ("Berlin Group v2", code.api.berlin.group.v2.Http4sBGv2.resourceDocs.map(_.operationId).toSeq, false), + ("UK Open Banking 2.0.0", code.api.UKOpenBanking.v2_0_0.OBP_UKOpenBanking_200.allResourceDocs.map(_.operationId).toSeq, false), + ("UK Open Banking 3.1.0", code.api.UKOpenBanking.v3_1_0.OBP_UKOpenBanking_310.allResourceDocs.map(_.operationId).toSeq, false), + ("UK Open Banking 4.0.1", code.api.UKOpenBanking.v4_0_1.OBP_UKOpenBanking_401.allResourceDocs.map(_.operationId).toSeq, false) ) feature("getAllResourceDocs contains every per-standard resource-doc surface") { - surfaces.foreach { case (label, operationIds) => + surfaces.foreach { case (label, operationIds, optional) => scenario(s"$label operation ids are all resolvable globally", RegistryParityTag) { - operationIds should not be empty + if (!optional) operationIds should not be empty val missing = operationIds.filterNot(allOperationIds.contains) withClue(s"$label operation ids missing from getAllResourceDocs: ${missing.take(10).mkString(", ")} ") { missing shouldBe empty From f1344c1c232e489d6d734c1e27da486ef2eadce2 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 31 Aug 2026 15:50:36 +0200 Subject: [PATCH 03/12] refactor: remove dead activeResourceDocs identity match in resource-docs dispatcher Every one of its ~19 arms was `case X => resourceDocs`, unchanged -- a leftover from the pre-http4s Lift route-filter era that stopped doing any filtering once the corresponding version moved fully onto http4s. getResourceDocsList now feeds resourceDocs directly into activePlusLocalResourceDocs, with identical output. --- .../ResourceDocsAPIMethods.scala | 28 +------------------ 1 file changed, 1 insertion(+), 27 deletions(-) diff --git a/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/ResourceDocsAPIMethods.scala b/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/ResourceDocsAPIMethods.scala index b14f064629..8263d92d79 100644 --- a/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/ResourceDocsAPIMethods.scala +++ b/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/ResourceDocsAPIMethods.scala @@ -355,35 +355,9 @@ trait ResourceDocsAPIMethods extends MdcLoggable with APIMethods220 with APIMeth logger.debug(s"There are ${resourceDocs.length} resource docs available to $requestedApiVersion") - val activeResourceDocs = requestedApiVersion match { - case ApiVersion.v7_0_0 => resourceDocs - case ConstantsBG.`berlinGroupVersion1` => resourceDocs // fully on http4s — no Lift route filter - case ConstantsBG.`berlinGroupVersion2` => resourceDocs - case ApiVersion.v1_2_1 => resourceDocs - case ApiVersion.v6_0_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.v5_1_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.v5_0_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.v4_0_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.v3_1_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.v3_0_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.v2_2_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.v2_1_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.v2_0_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.v1_4_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.v1_3_0 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.`dynamic-entity` => resourceDocs // runtime CRUD now on Http4sDynamicEntity; routes are Nil, skip Lift-route filter - case ApiVersion.`dynamic-endpoint` => resourceDocs // dispatch now on Http4sDynamicEndpoint (proxy + native Piece C); routes carry only the stub, skip Lift-route filter - case ApiVersion.ukOpenBankingV20 => resourceDocs // fully on http4s — no Lift route filter - case ApiVersion.ukOpenBankingV31 => resourceDocs // fully on http4s — no Lift route filter - case _ => resourceDocs - } - - logger.debug(s"There are ${activeResourceDocs.length} resource docs available to $requestedApiVersion") - - val activePlusLocalResourceDocs = ArrayBuffer[ResourceDoc]() - activePlusLocalResourceDocs ++= activeResourceDocs + activePlusLocalResourceDocs ++= resourceDocs requestedApiVersion match { // only `obp` standard show the `localResourceDocs` From 4b469ef5053098c6b01812e4d812840c72c6450a Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 31 Aug 2026 16:51:32 +0200 Subject: [PATCH 04/12] refactor: derive resource-doc dispatch and the global operation-id union from one registry Introduce ResourceDocRegistry as the single source of truth for "which resource docs does version X serve", replacing two independently hand-maintained registries: ResourceDocsAPIMethods.getResourceDocsList (the per-version dispatcher used by /resource-docs/{VERSION}/... and API Explorer) and APIUtil.allStaticResourceDocs (the union used wherever an operation id must be resolved). These drifted three times by hand -- Berlin Group v2, v7-only operation ids, and the Berlin Group v1.3 alias all had to be independently added to both places, and were each missed at least once. Deriving both from one registry map makes that class of drift structurally impossible going forward. Http4sBGv2 becomes a ScannedApis registrant (its apiVersion is ConstantsBG.berlinGroupVersion2), so it is now fully convention-driven like the other Berlin Group / UK Open Banking standards and needs no hand-maintained entry in the registry or a special case in ApiVersionUtils.valueOf. The global union is now deduped by operationId -- the underlying per-version buffers legitimately overlap (each OBP-standard aggregation repeats every older version's docs), and consumers only ever .find or build a lookup map from the result. ResourceDocRegistryParityTest is rewritten to iterate the registry itself rather than a hand-typed list of standards, so a future standard reachable by the dispatcher is covered by construction and the test's job narrows to catching an accidental regression back to two independently maintained registries. Verified live against a running instance, before and after: BGv2 and Berlin Group v1.3 alias operation ids both still resolve through POST /my/api-collections/{name}/api-collection-endpoints. Full local suite: 3582 tests, 0 failures. --- .../ResourceDocsAPIMethods.scala | 27 +----- .../code/api/berlin/group/v2/Http4sBGv2.scala | 10 ++- .../main/scala/code/api/util/APIUtil.scala | 39 ++------- .../scala/code/api/util/ApiVersionUtils.scala | 3 - .../code/api/util/ResourceDocRegistry.scala | 82 +++++++++++++++++++ .../util/ResourceDocRegistryParityTest.scala | 62 +++++++------- 6 files changed, 134 insertions(+), 89 deletions(-) create mode 100644 obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala diff --git a/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/ResourceDocsAPIMethods.scala b/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/ResourceDocsAPIMethods.scala index 8263d92d79..a35a0483a4 100644 --- a/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/ResourceDocsAPIMethods.scala +++ b/obp-api/src/main/scala/code/api/ResourceDocs1_4_0/ResourceDocsAPIMethods.scala @@ -3,8 +3,6 @@ package code.api.ResourceDocs1_4_0 import code.api.Constant.{GET_DYNAMIC_RESOURCE_DOCS_TTL, GET_STATIC_RESOURCE_DOCS_TTL, HostName, PARAM_LOCALE} import code.api.OBPRestHelper import code.api.cache.Caching -import code.api.dynamic.endpoint.OBPAPIDynamicEndpoint -import code.api.dynamic.entity.OBPAPIDynamicEntity import code.api.util.APIUtil._ import code.api.util.ApiRole.{canReadDynamicResourceDocsAtOneBank, canReadResourceDoc} import code.api.util.ApiTag._ @@ -22,7 +20,6 @@ import code.api.v4_0_0.{APIMethods400, OBPAPI4_0_0} import code.api.v5_0_0.OBPAPI5_0_0 import code.api.v5_1_0.OBPAPI5_1_0 import code.api.v6_0_0.OBPAPI6_0_0 -import code.api.berlin.group.ConstantsBG import code.apicollectionendpoint.MappedApiCollectionEndpointsProvider import code.util.Helper import code.util.Helper.{MdcLoggable, ObpS, SILENCE_IS_GOLDEN} @@ -331,27 +328,9 @@ trait ResourceDocsAPIMethods extends MdcLoggable with APIMethods220 with APIMeth logger.debug(s"getResourceDocsList says requestedApiVersion is $requestedApiVersion") - val resourceDocs = requestedApiVersion match { - case ApiVersion.v7_0_0 => code.api.v7_0_0.Http4s700.allResourceDocs // Use aggregated docs for v7.0.0 - case ConstantsBG.`berlinGroupVersion1` => code.api.berlin.group.v1_3.Http4sBGv13.resourceDocs - case ConstantsBG.`berlinGroupVersion2` => code.api.berlin.group.v2.Http4sBGv2.resourceDocs - case ApiVersion.v6_0_0 => code.api.util.http4s.Http4sResourceDocAggregation.v600 - case ApiVersion.v5_1_0 => code.api.util.http4s.Http4sResourceDocAggregation.v510 - case ApiVersion.v5_0_0 => code.api.util.http4s.Http4sResourceDocAggregation.v500 - case ApiVersion.v4_0_0 => code.api.util.http4s.Http4sResourceDocAggregation.v400 - case ApiVersion.v3_1_0 => code.api.util.http4s.Http4sResourceDocAggregation.v310 - case ApiVersion.v3_0_0 => code.api.util.http4s.Http4sResourceDocAggregation.v300 - case ApiVersion.v2_2_0 => code.api.util.http4s.Http4sResourceDocAggregation.v220 - case ApiVersion.v2_1_0 => code.api.util.http4s.Http4sResourceDocAggregation.v210 - case ApiVersion.v2_0_0 => code.api.util.http4s.Http4sResourceDocAggregation.v200 - case ApiVersion.v1_4_0 => code.api.util.http4s.Http4sResourceDocAggregation.v140 - case ApiVersion.v1_3_0 => code.api.util.http4s.Http4sResourceDocAggregation.v130 - case ApiVersion.v1_2_1 => code.api.util.http4s.Http4sResourceDocAggregation.v121 - case ApiVersion.`dynamic-endpoint` => OBPAPIDynamicEndpoint.allResourceDocs - case ApiVersion.`dynamic-entity` => OBPAPIDynamicEntity.allResourceDocs - case version: ScannedApiVersion => ScannedApis.versionMapScannedApis.get(version).map(_.allResourceDocs).getOrElse(ArrayBuffer.empty[ResourceDoc]) - case _ => ArrayBuffer.empty[ResourceDoc] - } + // ResourceDocRegistry is the single source of truth for both this per-version dispatch and + // APIUtil.allStaticResourceDocs' global operation-id union -- see that object's doc comment. + val resourceDocs = ResourceDocRegistry.docsFor(requestedApiVersion) logger.debug(s"There are ${resourceDocs.length} resource docs available to $requestedApiVersion") diff --git a/obp-api/src/main/scala/code/api/berlin/group/v2/Http4sBGv2.scala b/obp-api/src/main/scala/code/api/berlin/group/v2/Http4sBGv2.scala index 9e2d1ce640..b698cb0dc4 100644 --- a/obp-api/src/main/scala/code/api/berlin/group/v2/Http4sBGv2.scala +++ b/obp-api/src/main/scala/code/api/berlin/group/v2/Http4sBGv2.scala @@ -4,23 +4,31 @@ import cats.data.{Kleisli, OptionT} import cats.effect._ import code.api.berlin.group.ConstantsBG import code.api.util.APIUtil.ResourceDoc +import code.api.util.ScannedApis import code.api.util.http4s.ResourceDocMiddleware import code.util.Helper.MdcLoggable +import com.openbankproject.commons.util.ScannedApiVersion import org.http4s._ import scala.collection.mutable.ArrayBuffer -object Http4sBGv2 extends MdcLoggable { +object Http4sBGv2 extends MdcLoggable with ScannedApis { type HttpF[A] = OptionT[IO, A] val implementedInApiVersion = ConstantsBG.berlinGroupVersion2 + // ScannedApis discovery marker: makes BGv2 convention-driven like the other Berlin Group / + // UK Open Banking standards, so ResourceDocRegistry picks it up without a hand-maintained entry. + override val apiVersion: ScannedApiVersion = implementedInApiVersion + val resourceDocs: ArrayBuffer[ResourceDoc] = Http4sBGv2AIS.resourceDocs ++ Http4sBGv2PIS.resourceDocs ++ Http4sBGv2PIIS.resourceDocs + override val allResourceDocs: ArrayBuffer[ResourceDoc] = resourceDocs + val allRoutes: HttpRoutes[IO] = Kleisli[HttpF, Request[IO], Response[IO]] { req => Http4sBGv2AIS.routes(req) .orElse(Http4sBGv2PIS.routes(req)) diff --git a/obp-api/src/main/scala/code/api/util/APIUtil.scala b/obp-api/src/main/scala/code/api/util/APIUtil.scala index b3d685f30b..4d9f61bcff 100644 --- a/obp-api/src/main/scala/code/api/util/APIUtil.scala +++ b/obp-api/src/main/scala/code/api/util/APIUtil.scala @@ -32,9 +32,6 @@ import cats.effect.IO import code.abacrule.AbacRuleEngine import code.accountholders.AccountHolders import code.api.Constant._ -import code.api.UKOpenBanking.v2_0_0.OBP_UKOpenBanking_200 -import code.api.UKOpenBanking.v3_1_0.OBP_UKOpenBanking_310 -import code.api.UKOpenBanking.v4_0_1.OBP_UKOpenBanking_401 import code.api._ import code.api.berlin.group.ConstantsBG import code.api.berlin.group.v1_3.JSONFactory_BERLIN_GROUP_1_3.{ErrorMessageBG, ErrorMessagesBG} @@ -4874,35 +4871,13 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{ val allowedAnswerTransactionRequestChallengeAttempts = APIUtil.getPropsAsIntValue("answer_transactionRequest_challenge_allowed_attempts").openOr(3) - // Base is the v7 aggregation — the newest OBP-standard surface, which already contains the - // v6.0.0-and-older aggregation plus the v7-only endpoints (deduped by URL/method). Basing on - // the v6 aggregation silently excluded v7-only operation ids from everything that resolves - // operation ids through this list (api-collection endpoint validation, top-apis lookups, ...). - // ResourceDocRegistryParityTest pins the invariant that every per-standard surface the - // resource-docs dispatcher can serve is contained here. - lazy val allStaticResourceDocs = (code.api.v7_0_0.Http4s700.allResourceDocs - ++ OBP_UKOpenBanking_200.allResourceDocs - ++ OBP_UKOpenBanking_310.allResourceDocs - ++ OBP_UKOpenBanking_401.allResourceDocs - // Commented out: Lift endpoints migrated off / removed (Polish, STET, AUOpenBanking, MxOF/CNBV9, BahrainOBF) - // ++ code.api.Polish.v2_1_1_1.OBP_PAPI_2_1_1_1.allResourceDocs - // ++ code.api.STET.v1_4.OBP_STET_1_4.allResourceDocs - // ++ code.api.AUOpenBanking.v1_0_0.ApiCollector.allResourceDocs - // ++ code.api.MxOF.CNBV9_1_0_0.allResourceDocs - // ++ code.api.MxOF.OBP_MXOF_1_0_0.allResourceDocs - // ++ code.api.BahrainOBF.v1_0_0.ApiCollector.allResourceDocs - ++ code.api.berlin.group.v1_3.OBP_BERLIN_GROUP_1_3.allResourceDocs - // The BG v1.3 alias (active only when berlin_group_v1_3_alias_path is set) is served by - // the resource-docs dispatcher via its ScannedApis registration, but its docs carry their - // own operation ids (re-derived from the alias version), so it hit the same - // getAllResourceDocs membership gap as BGv2 below. Empty when the prop is unset, so this - // is a no-op on instances without the alias configured. - ++ code.api.berlin.group.v1_3.OBP_BERLIN_GROUP_1_3_Alias.allResourceDocs - // BGv2 was missing here even though /resource-docs/BGv2 serves it, so a BGv2 operation id - // (e.g. BGv2-getAccountDetails) failed the getAllResourceDocs membership check that - // api-collection-endpoints (and anything else resolving operation ids) relies on. - ++ code.api.berlin.group.v2.Http4sBGv2.resourceDocs).toList - + // Delegates to ResourceDocRegistry, the single source of truth shared with the per-version + // resource-docs dispatcher (ResourceDocsAPIMethods.getResourceDocsList) -- see that object's + // doc comment for why the two used to drift and how deriving both from one registry fixes it. + // Kept under this name so existing call sites (Http4s400, Http4s600, JSONFactory6.0.0, ...) + // don't need to move. + lazy val allStaticResourceDocs: List[ResourceDoc] = ResourceDocRegistry.allStaticResourceDocs + def allDynamicResourceDocs= (DynamicEntityHelper.doc ++ DynamicEndpointHelper.doc ++ DynamicEndpoints.dynamicResourceDocs).toList def getAllResourceDocs = allStaticResourceDocs ++ allDynamicResourceDocs diff --git a/obp-api/src/main/scala/code/api/util/ApiVersionUtils.scala b/obp-api/src/main/scala/code/api/util/ApiVersionUtils.scala index cc2641c67f..d291130498 100644 --- a/obp-api/src/main/scala/code/api/util/ApiVersionUtils.scala +++ b/obp-api/src/main/scala/code/api/util/ApiVersionUtils.scala @@ -2,7 +2,6 @@ package code.api.util import com.openbankproject.commons.util.ApiVersion._ import com.openbankproject.commons.util.ScannedApiVersion -import code.api.berlin.group.ConstantsBG object ApiVersionUtils { @@ -23,7 +22,6 @@ object ApiVersionUtils { v7_0_0 :: `dynamic-endpoint` :: `dynamic-entity` :: - ConstantsBG.berlinGroupVersion2 :: scannedApis ).distinct @@ -48,7 +46,6 @@ object ApiVersionUtils { case v7_0_0.fullyQualifiedVersion | v7_0_0.apiShortVersion => v7_0_0 case `dynamic-endpoint`.fullyQualifiedVersion | `dynamic-endpoint`.apiShortVersion => `dynamic-endpoint` case `dynamic-entity`.fullyQualifiedVersion | `dynamic-entity`.apiShortVersion => `dynamic-entity` - case version if version == ConstantsBG.berlinGroupVersion2.fullyQualifiedVersion || version == ConstantsBG.berlinGroupVersion2.apiShortVersion => ConstantsBG.berlinGroupVersion2 case version if(scannedApis.map(_.fullyQualifiedVersion).contains(version)) =>scannedApis.filter(_.fullyQualifiedVersion==version).head case version if(scannedApis.map(_.apiShortVersion).contains(version)) diff --git a/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala b/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala new file mode 100644 index 0000000000..f0194d0aca --- /dev/null +++ b/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala @@ -0,0 +1,82 @@ +package code.api.util + +import code.api.berlin.group.ConstantsBG +import code.api.util.APIUtil.ResourceDoc +import com.openbankproject.commons.util.ApiVersion._ +import com.openbankproject.commons.util.ApiVersion + +import scala.collection.immutable.ListMap + +/** + * Single source of truth for "which resource docs does version X serve" — used both by the + * per-version resource-docs dispatcher (ResourceDocsAPIMethods.getResourceDocsList, i.e. + * /resource-docs/{VERSION}/... and API Explorer) and by the global operation-id union + * (allStaticResourceDocs / getAllResourceDocs, used wherever an operation id must be resolved: + * api-collection-endpoint creation, top-apis/popular-apis lookups, metrics, ...). + * + * These used to be two independently hand-maintained registries and drifted three times: Berlin + * Group v2 was served by the dispatcher but missing from the union (BGv2-getAccountDetails could + * not be added to an API collection), the union was based on the v6 aggregation excluding v7-only + * operation ids, and the Berlin Group v1.3 alias was missing from the union too. Deriving both + * from one `registry` map makes that class of drift structurally impossible: add a version once, + * both call sites see it. + * + * Rule for adding a new API standard: implement `with ScannedApis` (see that trait) and it is + * picked up automatically via the `scanned` half of `registry` below — no edit needed here. Only + * standards that cannot be discovered that way (or that need to override the source composing + * function, e.g. the cumulative per-version OBP-standard aggregations) need an `explicit` entry. + * + * Deliberately its own file/object, NOT a member of `APIUtil`: the Implementations* objects for + * each version re-enter `APIUtil` during their own initialization (prop lookups, etc.), so a + * strict `val` living inside `APIUtil` risks a class-init deadlock. Everything here stays `lazy` + * and is first touched at request/test time, well after Props and `ApiVersion.setUrlPrefix` have + * run in Boot. + */ +object ResourceDocRegistry { + + /** version -> that surface's docs. Thunks, not values: the ScannedApis-discovered arms are + * lazy vals themselves and the OBP-standard aggregations are cumulative lazy vals too — wrapping + * in a function defers evaluation to first use of THIS registry, not construction of the map. */ + lazy val registry: ListMap[ApiVersion, () => Seq[ResourceDoc]] = { + val explicit: ListMap[ApiVersion, () => Seq[ResourceDoc]] = ListMap( + v7_0_0 -> (() => code.api.v7_0_0.Http4s700.allResourceDocs.toSeq), + ConstantsBG.berlinGroupVersion1 -> (() => code.api.berlin.group.v1_3.Http4sBGv13.resourceDocs.toSeq), + v6_0_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v600.toSeq), + v5_1_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v510.toSeq), + v5_0_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v500.toSeq), + v4_0_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v400.toSeq), + v3_1_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v310.toSeq), + v3_0_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v300.toSeq), + v2_2_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v220.toSeq), + v2_1_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v210.toSeq), + v2_0_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v200.toSeq), + v1_4_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v140.toSeq), + v1_3_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v130.toSeq), + v1_2_1 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v121.toSeq), + `dynamic-endpoint` -> (() => code.api.dynamic.endpoint.OBPAPIDynamicEndpoint.allResourceDocs.toSeq), + `dynamic-entity` -> (() => code.api.dynamic.entity.OBPAPIDynamicEntity.allResourceDocs.toSeq) + // Berlin Group v2 is NOT listed here -- Http4sBGv2 is a ScannedApis registrant (its + // apiVersion is ConstantsBG.berlinGroupVersion2), so it is picked up by `scanned` below. + ) + // Every standard discovered via ScannedApis (UK OB 200/310/401, BG v1.3 canonical + alias, + // BG v2, and any future `with ScannedApis` standard). Explicit entries win on key collision -- + // BG v1.3 canonical is both explicit above and a registrant, same underlying buffer either way. + val scanned: Map[ApiVersion, () => Seq[ResourceDoc]] = ScannedApis.versionMapScannedApis.collect { + case (version, apis) if !explicit.contains(version) => version -> (() => apis.allResourceDocs.toSeq) + } + explicit ++ scanned + } + + /** What the per-version resource-docs dispatcher serves for this version (empty if unknown). */ + def docsFor(version: ApiVersion): Seq[ResourceDoc] = registry.get(version).map(_ ()).getOrElse(Nil) + + /** The global operation-id union. Excludes the dynamic arms: those are runtime-mutable (created/ + * deleted dynamic entities and endpoints) and are appended FRESH by APIUtil.getAllResourceDocs on + * every call -- caching them in this lazy union would serve stale dynamic docs. Deduped by + * operationId: the underlying per-version buffers legitimately overlap (each OBP-standard + * aggregation repeats every older version's docs), and consumers only ever `.find`/build a + * lookup map from this list, never rely on it containing duplicates. */ + lazy val allStaticResourceDocs: List[ResourceDoc] = + (registry - `dynamic-endpoint` - `dynamic-entity`) + .values.flatMap(_ ()).toList.distinctBy(_.operationId) +} diff --git a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala index fa0e25247a..341c65e122 100644 --- a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala +++ b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala @@ -1,22 +1,22 @@ package code.api.util import code.setup.ServerSetup +import com.openbankproject.commons.util.{ApiVersion, ScannedApiVersion} import org.scalatest.Tag /** - * Guards the invariant that APIUtil.getAllResourceDocs — the global operation-id - * registry used wherever an operation id must be resolved (api-collection endpoint - * validation, top-apis operation-id lookups, ...) — contains every per-standard - * resource-doc surface the resource-docs dispatcher can serve to API Explorer. + * Guards the invariant that APIUtil.getAllResourceDocs — the global operation-id registry used + * wherever an operation id must be resolved (api-collection endpoint validation, top-apis + * operation-id lookups, ...) — contains every per-standard resource-doc surface the resource-docs + * dispatcher can serve to API Explorer. * - * These are two parallel registries (ResourceDocsAPIMethods dispatches per - * standard/version; getAllResourceDocs aggregates them all), and they have drifted - * three times: Berlin Group v2 was served by the dispatcher but missing from the - * global registry (so BGv2-getAccountDetails could not be added to an API - * collection), the global registry was based on the v6 aggregation, excluding - * v7-only operation ids, and the Berlin Group v1.3 alias (active only when - * berlin_group_v1_3_alias_path is set) was missing too. When you add a NEW API - * standard, register its docs in BOTH places — and add its surface to this list. + * Both sides are now derived from the single ResourceDocRegistry.registry map, so this class of + * drift (which happened three times by hand: Berlin Group v2, v7-only operation ids, and the + * Berlin Group v1.3 alias) is structurally impossible going forward — see ResourceDocRegistry's + * doc comment. This test's job is narrower than it used to be: it iterates the registry itself + * (rather than a hand-typed list of standards) so it stays correct as standards are added or + * removed without needing an edit here, and it catches an accidental regression back to two + * independently hand-maintained registries. */ class ResourceDocRegistryParityTest extends ServerSetup { @@ -25,24 +25,28 @@ class ResourceDocRegistryParityTest extends ServerSetup { private lazy val allOperationIds: Set[String] = APIUtil.getAllResourceDocs.map(_.operationId).toSet - // optional=true surfaces are gated by a prop that is unset by default (test props - // included), so their operation-id list is legitimately empty in most environments -- - // the membership check still runs (trivially true when empty) but the non-empty - // assertion is skipped for them. - private lazy val surfaces: List[(String, Seq[String], Boolean)] = List( - ("OBP standard (v7 aggregation)", code.api.v7_0_0.Http4s700.allResourceDocs.map(_.operationId).toSeq, false), - ("Berlin Group v1.3", code.api.berlin.group.v1_3.Http4sBGv13.resourceDocs.map(_.operationId).toSeq, false), - ("Berlin Group v1.3 alias", code.api.berlin.group.v1_3.Http4sBGv13Alias.resourceDocs.map(_.operationId).toSeq, true), - ("Berlin Group v2", code.api.berlin.group.v2.Http4sBGv2.resourceDocs.map(_.operationId).toSeq, false), - ("UK Open Banking 2.0.0", code.api.UKOpenBanking.v2_0_0.OBP_UKOpenBanking_200.allResourceDocs.map(_.operationId).toSeq, false), - ("UK Open Banking 3.1.0", code.api.UKOpenBanking.v3_1_0.OBP_UKOpenBanking_310.allResourceDocs.map(_.operationId).toSeq, false), - ("UK Open Banking 4.0.1", code.api.UKOpenBanking.v4_0_1.OBP_UKOpenBanking_401.allResourceDocs.map(_.operationId).toSeq, false) - ) - - feature("getAllResourceDocs contains every per-standard resource-doc surface") { - surfaces.foreach { case (label, operationIds, optional) => + private def label(version: ApiVersion): String = version match { + case sv: ScannedApiVersion => sv.fullyQualifiedVersion + case other => other.toString + } + + // Dynamic arms (dynamic-endpoint / dynamic-entity) are excluded: they are runtime-mutable and + // ResourceDocRegistry.allStaticResourceDocs itself excludes them for the same reason (see its + // doc comment) -- APIUtil.getAllResourceDocs appends them fresh via allDynamicResourceDocs + // instead, so comparing them against this registry-derived snapshot would be meaningless. + private lazy val surfaces: List[(String, Seq[String])] = + (ResourceDocRegistry.registry - ApiVersion.`dynamic-endpoint` - ApiVersion.`dynamic-entity`) + .toList + .map { case (version, docsThunk) => (label(version), docsThunk().map(_.operationId)) } + + feature("getAllResourceDocs contains every per-standard resource-doc surface the dispatcher can serve") { + scenario("the registry itself is non-empty", RegistryParityTag) { + surfaces should not be empty + surfaces.exists(_._2.nonEmpty) shouldBe true + } + + surfaces.foreach { case (label, operationIds) => scenario(s"$label operation ids are all resolvable globally", RegistryParityTag) { - if (!optional) operationIds should not be empty val missing = operationIds.filterNot(allOperationIds.contains) withClue(s"$label operation ids missing from getAllResourceDocs: ${missing.take(10).mkString(", ")} ") { missing shouldBe empty From 5af4cb67ea7605439ecad87d6833f9c022574c54 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 31 Aug 2026 17:07:09 +0200 Subject: [PATCH 05/12] test: pin BGv2-getAccountDetails as a resolvable api-collection-endpoint operation id Adds an HTTP-level regression test for the sandbox bug report this branch started from: creating an API collection endpoint with operation_id=BGv2-getAccountDetails now returns 201, alongside the existing coverage for OBPv6.0.0, UK Open Banking, and Berlin Group v1.3 operation ids in the same scenario. Previously the only regression guard for this exact operation id was the unit-level membership check in ResourceDocRegistryParityTest; this exercises the actual endpoint. --- .../v4_0_0/ApiCollectionEndpointTest.scala | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala b/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala index 5e57ea2a7e..755f1f1869 100644 --- a/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala +++ b/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala @@ -202,7 +202,28 @@ class ApiCollectionEndpointTest extends V400ServerSetup { val operationId= apiCollectionEndpoint.operation_id } - + + { + // Regression pin for the sandbox bug report: BGv2-getAccountDetails was served by the + // resource-docs dispatcher (/resource-docs/BGv2/obp) but missing from the global + // operation-id union getAllResourceDocs relies on, so this exact request used to fail + // with OBP-40048 Invalid operation_id. + Then(s"we test the $ApiEndpoint6- BGv2-getAccountDetails") + val requestApiCollectionEndpoint = (v4_0_0_Request / "my" / "api-collection-ids" / apiCollectionId / "api-collection-endpoints").POST <@ (user1) + + lazy val postApiCollectionEndpointJson = SwaggerDefinitionsJSON.postApiCollectionEndpointJson400.copy(operation_id="BGv2-getAccountDetails") + + val responseApiCollectionEndpointJson = makePostRequest(requestApiCollectionEndpoint, write(postApiCollectionEndpointJson)) + Then("We should get a 201") + responseApiCollectionEndpointJson.code should equal(201) + val apiCollectionEndpoint = responseApiCollectionEndpointJson.body.extract[ApiCollectionEndpointJson400] + + apiCollectionEndpoint.operation_id should be (postApiCollectionEndpointJson.operation_id) + apiCollectionEndpoint.api_collection_endpoint_id shouldNot be (null) + + val operationId= apiCollectionEndpoint.operation_id + } + { Then(s"we test the $ApiEndpoint7") val requestGet = (v4_0_0_Request / "my" / "api-collection-ids" / apiCollectionId / "api-collection-endpoints").GET <@ (user1) @@ -213,7 +234,7 @@ class ApiCollectionEndpointTest extends V400ServerSetup { val apiCollectionsJsonGet400 = responseGet.body.extract[ApiCollectionEndpointsJson400] - apiCollectionsJsonGet400.api_collection_endpoints.length should be (4) + apiCollectionsJsonGet400.api_collection_endpoints.length should be (5) } } } From 3ec0b993279248b3c6780a4063b6ae65697c8b60 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 31 Aug 2026 17:26:48 +0200 Subject: [PATCH 06/12] test: enable the Berlin Group v1.3 alias by default so it gets HTTP-level coverage berlin_group_v1_3_alias_path could not be toggled per-test at runtime: its ScannedApiVersion identity is captured once by ScannedApis. versionMapScannedApis' process-wide classpath scan (a lazy val, shared across the whole JVM/shard), which gets forced by the first unrelated request that falls through Http4sApp's route chain -- almost always long before any test-specific setPropsValues call. The only way to exercise a real alias operation id end to end is to have the prop already set before the JVM boots. Set berlin_group_v1_3_alias_path=0.6/v1 in test.default.props (local) and both CI workflows' generated test.default.props (build_pull_request. yml, build_container.yml). Add a regression test in ApiCollectionEndpointTest mirroring the existing per-standard coverage (OBPv6.0.0/UK Open Banking/Berlin Group v1.3 canonical) for the alias's BGv1-getPaymentInitiationStatus operation id, and pin the same operation id in ResourceDocRegistryParityTest alongside the existing BGv2-getAccountDetails pin. --- .github/workflows/build_container.yml | 3 +++ .github/workflows/build_pull_request.yml | 3 +++ .../util/ResourceDocRegistryParityTest.scala | 7 ++++++ .../v4_0_0/ApiCollectionEndpointTest.scala | 25 ++++++++++++++++++- 4 files changed, 37 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build_container.yml b/.github/workflows/build_container.yml index f9f87b6762..9893d4d4e6 100644 --- a/.github/workflows/build_container.yml +++ b/.github/workflows/build_container.yml @@ -289,6 +289,9 @@ jobs: echo ResetPasswordUrlEnabled=true >> obp-api/src/main/resources/props/test.default.props echo consents.allowed=true >> obp-api/src/main/resources/props/test.default.props echo hikari.maximumPoolSize=20 >> obp-api/src/main/resources/props/test.default.props + # Enables the Berlin Group v1.3 alias so ResourceDocRegistryParityTest and + # ApiCollectionEndpointTest can exercise a real alias operation id end to end. + echo berlin_group_v1_3_alias_path=0.6/v1 >> obp-api/src/main/resources/props/test.default.props echo write_metrics=false >> obp-api/src/main/resources/props/test.default.props # Log emails instead of opening a real SMTP socket: without this, # LocalMappedConnector.sendCustomerNotification's EMAIL branch calls diff --git a/.github/workflows/build_pull_request.yml b/.github/workflows/build_pull_request.yml index 39716fe115..c18b9ae0d8 100644 --- a/.github/workflows/build_pull_request.yml +++ b/.github/workflows/build_pull_request.yml @@ -283,6 +283,9 @@ jobs: echo ResetPasswordUrlEnabled=true >> obp-api/src/main/resources/props/test.default.props echo consents.allowed=true >> obp-api/src/main/resources/props/test.default.props echo hikari.maximumPoolSize=20 >> obp-api/src/main/resources/props/test.default.props + # Enables the Berlin Group v1.3 alias so ResourceDocRegistryParityTest and + # ApiCollectionEndpointTest can exercise a real alias operation id end to end. + echo berlin_group_v1_3_alias_path=0.6/v1 >> obp-api/src/main/resources/props/test.default.props echo write_metrics=false >> obp-api/src/main/resources/props/test.default.props # Log emails instead of opening a real SMTP socket: without this, # LocalMappedConnector.sendCustomerNotification's EMAIL branch calls diff --git a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala index 341c65e122..9daddf9850 100644 --- a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala +++ b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala @@ -57,5 +57,12 @@ class ResourceDocRegistryParityTest extends ServerSetup { scenario("the operation id from the sandbox bug report resolves", RegistryParityTag) { allOperationIds should contain("BGv2-getAccountDetails") } + + // berlin_group_v1_3_alias_path is set in test.default.props precisely so this surface is + // reliably non-empty here (see that file's comment on why it must be set before boot, not + // toggled per-test) -- pin a concrete alias operation id, not just the generic loop above. + scenario("the operation id from the Berlin Group v1.3 alias resolves", RegistryParityTag) { + allOperationIds should contain("BGv1-getPaymentInitiationStatus") + } } } diff --git a/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala b/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala index 755f1f1869..492d6524ff 100644 --- a/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala +++ b/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala @@ -224,6 +224,29 @@ class ApiCollectionEndpointTest extends V400ServerSetup { val operationId= apiCollectionEndpoint.operation_id } + { + // Regression pin for the Berlin Group v1.3 alias gap: berlin_group_v1_3_alias_path + // (set to 0.6/v1 in test.default.props) activates Http4sBGv13Alias, whose docs are + // re-stamped copies of the canonical BG v1.3 docs carrying their own operation ids + // (BGv1-... here, not BGv1.3-...). These were served by the resource-docs dispatcher + // via ScannedApis discovery but missing from the global operation-id union, the same + // class of gap as BGv2 above. + Then(s"we test the $ApiEndpoint6- BGv1-getPaymentInitiationStatus (Berlin Group v1.3 alias)") + val requestApiCollectionEndpoint = (v4_0_0_Request / "my" / "api-collection-ids" / apiCollectionId / "api-collection-endpoints").POST <@ (user1) + + lazy val postApiCollectionEndpointJson = SwaggerDefinitionsJSON.postApiCollectionEndpointJson400.copy(operation_id="BGv1-getPaymentInitiationStatus") + + val responseApiCollectionEndpointJson = makePostRequest(requestApiCollectionEndpoint, write(postApiCollectionEndpointJson)) + Then("We should get a 201") + responseApiCollectionEndpointJson.code should equal(201) + val apiCollectionEndpoint = responseApiCollectionEndpointJson.body.extract[ApiCollectionEndpointJson400] + + apiCollectionEndpoint.operation_id should be (postApiCollectionEndpointJson.operation_id) + apiCollectionEndpoint.api_collection_endpoint_id shouldNot be (null) + + val operationId= apiCollectionEndpoint.operation_id + } + { Then(s"we test the $ApiEndpoint7") val requestGet = (v4_0_0_Request / "my" / "api-collection-ids" / apiCollectionId / "api-collection-endpoints").GET <@ (user1) @@ -234,7 +257,7 @@ class ApiCollectionEndpointTest extends V400ServerSetup { val apiCollectionsJsonGet400 = responseGet.body.extract[ApiCollectionEndpointsJson400] - apiCollectionsJsonGet400.api_collection_endpoints.length should be (5) + apiCollectionsJsonGet400.api_collection_endpoints.length should be (6) } } } From ff17b5e038001307bd0f77471ef3520a27c6e5b2 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 31 Aug 2026 17:58:25 +0200 Subject: [PATCH 07/12] test: pin a v7-only operation id as resolvable, covering the third drift instance The global operation-id union used to be built from the v6.0.0 aggregation, so operation ids belonging to endpoints that exist only in v7.0.0 were absent from it and could not be added to an API collection. That drift instance had no regression test: the OBPv6.0.0-* cases in ApiCollectionEndpointTest pass under both the old v6-based union and the current v7-based one, so they cannot detect it. Pin OBPv7.0.0-getMyMetrics (v7-only -- not part of Http4sResourceDocAggregation.v600) as a real api-collection-endpoint request, and add the matching named pin in ResourceDocRegistryParityTest alongside the BGv2 and Berlin Group v1.3 alias ones, so all three historical drift instances now have both HTTP-level and registry-level coverage. --- .../util/ResourceDocRegistryParityTest.scala | 9 +++++++ .../v4_0_0/ApiCollectionEndpointTest.scala | 25 ++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala index 9daddf9850..401ab2b241 100644 --- a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala +++ b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala @@ -54,6 +54,9 @@ class ResourceDocRegistryParityTest extends ServerSetup { } } + // The three named pins below are the three historical drift instances. The generic loop + // above would also catch them, but naming them keeps the specific regressions legible. + scenario("the operation id from the sandbox bug report resolves", RegistryParityTag) { allOperationIds should contain("BGv2-getAccountDetails") } @@ -64,5 +67,11 @@ class ResourceDocRegistryParityTest extends ServerSetup { scenario("the operation id from the Berlin Group v1.3 alias resolves", RegistryParityTag) { allOperationIds should contain("BGv1-getPaymentInitiationStatus") } + + // The union used to be built from the v6.0.0 aggregation, so v7-only operation ids were + // absent from it. getMyMetrics exists only in v7.0.0, so it pins the v7 base specifically. + scenario("a v7-only operation id resolves", RegistryParityTag) { + allOperationIds should contain("OBPv7.0.0-getMyMetrics") + } } } diff --git a/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala b/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala index 492d6524ff..9f1ce7e465 100644 --- a/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala +++ b/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala @@ -247,6 +247,29 @@ class ApiCollectionEndpointTest extends V400ServerSetup { val operationId= apiCollectionEndpoint.operation_id } + { + // Regression pin for the third drift instance: the global operation-id union used to be + // built from the v6.0.0 aggregation, so operation ids belonging to endpoints that exist + // ONLY in v7.0.0 (getMyMetrics, getTopUsers, getTopConsumers) were absent from it and + // could not be added to an API collection either. getMyMetrics is v7-only -- it is not + // part of Http4sResourceDocAggregation.v600 -- so this pins the v7 base specifically, + // unlike the OBPv6.0.0-* cases above which passed even under the old v6-based union. + Then(s"we test the $ApiEndpoint6- OBPv7.0.0-getMyMetrics (v7-only endpoint)") + val requestApiCollectionEndpoint = (v4_0_0_Request / "my" / "api-collection-ids" / apiCollectionId / "api-collection-endpoints").POST <@ (user1) + + lazy val postApiCollectionEndpointJson = SwaggerDefinitionsJSON.postApiCollectionEndpointJson400.copy(operation_id="OBPv7.0.0-getMyMetrics") + + val responseApiCollectionEndpointJson = makePostRequest(requestApiCollectionEndpoint, write(postApiCollectionEndpointJson)) + Then("We should get a 201") + responseApiCollectionEndpointJson.code should equal(201) + val apiCollectionEndpoint = responseApiCollectionEndpointJson.body.extract[ApiCollectionEndpointJson400] + + apiCollectionEndpoint.operation_id should be (postApiCollectionEndpointJson.operation_id) + apiCollectionEndpoint.api_collection_endpoint_id shouldNot be (null) + + val operationId= apiCollectionEndpoint.operation_id + } + { Then(s"we test the $ApiEndpoint7") val requestGet = (v4_0_0_Request / "my" / "api-collection-ids" / apiCollectionId / "api-collection-endpoints").GET <@ (user1) @@ -257,7 +280,7 @@ class ApiCollectionEndpointTest extends V400ServerSetup { val apiCollectionsJsonGet400 = responseGet.body.extract[ApiCollectionEndpointsJson400] - apiCollectionsJsonGet400.api_collection_endpoints.length should be (6) + apiCollectionsJsonGet400.api_collection_endpoints.length should be (7) } } } From d252dc5f4b1189f780d724af95a752a1760c6ac0 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 31 Aug 2026 22:13:19 +0200 Subject: [PATCH 08/12] fix: build the global union from the current OBP surface only, in a defined order Two defects from the registry refactor, both in how allStaticResourceDocs was assembled. Folding every per-version aggregation into the union added 287 operation ids it never carried (the older aggregations are not subsets of the v7 one -- an endpoint dropped after v4 keeps its operation id there), and 234 of those collide on partialFunctionName with an entry already present. Http4s600's top-apis and popular-apis and JSONFactory6.0.0's metrics all build `partialFunctionName -> operationId` with `.toMap`, where the last entry wins, so with v1.2.1 sorting last the reported operation_id flipped to the oldest id: getBanks became OBPv1.2.1-getBanks, root became OBPv1.2.1-root. Restrict the union to obpUnionVersion (the current OBP aggregation) plus every non-OBP standard. Consequence, deliberate and documented at the constant: an operation id living only in a superseded aggregation stays unresolvable, exactly as before the refactor. The scanned half of the registry was a plain Map, so the same `.toMap` consumers resolved a partialFunctionName shared by two scanned standards according to hash iteration order -- undefined, and free to shift when a standard is added or removed. The Berlin Group v1.3 alias re-stamps the canonical BG v1.3 docs and so collides with BG v2 on getAccountDetails and four other names, and test.default.props now activates that alias for every test run. Sort it by fullyQualifiedVersion into a ListMap; BG v2 then wins those names, matching the behaviour before this branch. ResourceDocRegistryParityTest follows the narrowed union and regains the per-surface non-empty assertion, without which a standard whose docs stop being registered passes as a trivial subset. A new scenario pins obpUnionVersion as the newest OBP-standard version in the registry, so adding a v8 aggregation without moving it fails instead of silently dropping v8-only operation ids. Verified against a running instance: OBPv1.2.1-getBanks and OBPv3.0.0-getAggregateMetrics are rejected with OBP-40048 again, while BGv2-getAccountDetails, BGv1-getPaymentInitiationStatus and OBPv7.0.0-getMyMetrics still resolve. Full local suite 3573/0. --- .../code/api/util/ResourceDocRegistry.scala | 64 +++++++++++++++---- .../util/ResourceDocRegistryParityTest.scala | 49 ++++++++++---- 2 files changed, 89 insertions(+), 24 deletions(-) diff --git a/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala b/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala index f0194d0aca..4b308f3e15 100644 --- a/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala +++ b/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala @@ -3,7 +3,7 @@ package code.api.util import code.api.berlin.group.ConstantsBG import code.api.util.APIUtil.ResourceDoc import com.openbankproject.commons.util.ApiVersion._ -import com.openbankproject.commons.util.ApiVersion +import com.openbankproject.commons.util.{ApiStandards, ApiVersion, ScannedApiVersion} import scala.collection.immutable.ListMap @@ -61,22 +61,62 @@ object ResourceDocRegistry { // Every standard discovered via ScannedApis (UK OB 200/310/401, BG v1.3 canonical + alias, // BG v2, and any future `with ScannedApis` standard). Explicit entries win on key collision -- // BG v1.3 canonical is both explicit above and a registrant, same underlying buffer either way. - val scanned: Map[ApiVersion, () => Seq[ResourceDoc]] = ScannedApis.versionMapScannedApis.collect { - case (version, apis) if !explicit.contains(version) => version -> (() => apis.allResourceDocs.toSeq) - } + // + // Sorted, and folded into a ListMap, so the registry has ONE defined iteration order. + // ScannedApis.versionMapScannedApis is an unordered Map, and several standards share + // partialFunctionNames (the BG v1.3 alias re-stamps the canonical BG v1.3 docs, so it collides + // with both BG v1.3 and -- on getAccountDetails, getAccountList, getCardAccountBalances, + // getCardAccountTransactionList, getTransactionDetails -- with BG v2). Consumers such as + // Http4s600's top-apis/popular-apis and JSONFactory6.0.0's metrics build + // `partialFunctionName -> operationId` with `.toMap`, where the LAST entry wins, so leaving the + // order to Map's hash iteration would leave the reported operation_id undefined and let it + // shift silently whenever a standard is added or removed. + val scanned: ListMap[ApiVersion, () => Seq[ResourceDoc]] = + ScannedApis.versionMapScannedApis.toSeq + .collect { case (version: ScannedApiVersion, apis) if !explicit.contains(version) => + version -> (() => apis.allResourceDocs.toSeq) } + .sortBy(_._1.fullyQualifiedVersion) + .foldLeft(ListMap.empty[ApiVersion, () => Seq[ResourceDoc]])(_ + _) explicit ++ scanned } /** What the per-version resource-docs dispatcher serves for this version (empty if unknown). */ def docsFor(version: ApiVersion): Seq[ResourceDoc] = registry.get(version).map(_ ()).getOrElse(Nil) - /** The global operation-id union. Excludes the dynamic arms: those are runtime-mutable (created/ - * deleted dynamic entities and endpoints) and are appended FRESH by APIUtil.getAllResourceDocs on - * every call -- caching them in this lazy union would serve stale dynamic docs. Deduped by - * operationId: the underlying per-version buffers legitimately overlap (each OBP-standard - * aggregation repeats every older version's docs), and consumers only ever `.find`/build a - * lookup map from this list, never rely on it containing duplicates. */ + /** + * The OBP-standard surface the global union is built from. + * + * The registry also holds the cumulative aggregations for every older OBP version, because the + * dispatcher must still serve /resource-docs/OBPv4.0.0/obp and friends. Those are NOT folded into + * the union: they are not subsets of the v7 aggregation (an endpoint dropped after v4 keeps its + * operation id there), so including them would add ~287 operation ids that the union never + * carried, 234 of which collide on partialFunctionName with an entry already present -- and the + * `.toMap` consumers above would then report the OLDEST id (getBanks -> OBPv1.2.1-getBanks) + * instead of the current one in metrics, top-apis and popular-apis output. + * + * Consequence, deliberately accepted: an operation id that exists ONLY in a superseded + * aggregation stays unresolvable by api-collection-endpoint creation, exactly as before this + * refactor. ResourceDocRegistryParityTest pins that this constant is the newest OBP-standard + * version in the registry, so adding v8 without moving it fails the build rather than silently + * dropping v8-only operation ids from the union. + */ + val obpUnionVersion: ApiVersion = v7_0_0 + + private def isObpStandard(version: ApiVersion): Boolean = version match { + case sv: ScannedApiVersion => sv.apiStandard == ApiStandards.obp.toString + case _ => false + } + + /** Versions whose docs make up the global union: the current OBP surface plus every non-OBP + * standard. Excluding the other OBP-standard keys also excludes `dynamic-endpoint` / + * `dynamic-entity` (both carry apiStandard "obp"), which must stay out for a second reason: + * they are runtime-mutable and APIUtil.getAllResourceDocs appends them FRESH on every call, so + * caching them in this lazy union would serve stale dynamic docs. */ + lazy val unionVersions: Seq[ApiVersion] = + registry.keys.filter(v => v == obpUnionVersion || !isObpStandard(v)).toSeq + + /** The global operation-id union. Deduped by operationId: the surfaces legitimately overlap, and + * consumers only ever `.find`/build a lookup map from this list, never rely on duplicates. */ lazy val allStaticResourceDocs: List[ResourceDoc] = - (registry - `dynamic-endpoint` - `dynamic-entity`) - .values.flatMap(_ ()).toList.distinctBy(_.operationId) + unionVersions.flatMap(docsFor).toList.distinctBy(_.operationId) } diff --git a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala index 401ab2b241..3e3167c83a 100644 --- a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala +++ b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala @@ -1,7 +1,7 @@ package code.api.util import code.setup.ServerSetup -import com.openbankproject.commons.util.{ApiVersion, ScannedApiVersion} +import com.openbankproject.commons.util.{ApiStandards, ApiVersion, ScannedApiVersion} import org.scalatest.Tag /** @@ -30,23 +30,28 @@ class ResourceDocRegistryParityTest extends ServerSetup { case other => other.toString } - // Dynamic arms (dynamic-endpoint / dynamic-entity) are excluded: they are runtime-mutable and - // ResourceDocRegistry.allStaticResourceDocs itself excludes them for the same reason (see its - // doc comment) -- APIUtil.getAllResourceDocs appends them fresh via allDynamicResourceDocs - // instead, so comparing them against this registry-derived snapshot would be meaningless. + // Scoped to ResourceDocRegistry.unionVersions -- the current OBP surface plus every non-OBP + // standard. The superseded OBP aggregations (v6.0.0 and older) and the two dynamic arms are + // deliberately out of the union; see ResourceDocRegistry.obpUnionVersion for why, and for the + // accepted consequence that an operation id living only in a superseded aggregation stays + // unresolvable. private lazy val surfaces: List[(String, Seq[String])] = - (ResourceDocRegistry.registry - ApiVersion.`dynamic-endpoint` - ApiVersion.`dynamic-entity`) - .toList - .map { case (version, docsThunk) => (label(version), docsThunk().map(_.operationId)) } + ResourceDocRegistry.unionVersions.toList + .map(version => (label(version), ResourceDocRegistry.docsFor(version).map(_.operationId))) - feature("getAllResourceDocs contains every per-standard resource-doc surface the dispatcher can serve") { + feature("getAllResourceDocs contains every per-standard resource-doc surface the union covers") { scenario("the registry itself is non-empty", RegistryParityTag) { surfaces should not be empty - surfaces.exists(_._2.nonEmpty) shouldBe true } surfaces.foreach { case (label, operationIds) => scenario(s"$label operation ids are all resolvable globally", RegistryParityTag) { + // Non-empty matters as much as membership: an empty surface is trivially a subset of the + // union, so without this a standard whose docs silently stop being registered (the very + // failure mode this test exists for) would pass unnoticed. + withClue(s"$label contributed no operation ids at all -- did its docs stop being registered? ") { + operationIds should not be empty + } val missing = operationIds.filterNot(allOperationIds.contains) withClue(s"$label operation ids missing from getAllResourceDocs: ${missing.take(10).mkString(", ")} ") { missing shouldBe empty @@ -54,8 +59,28 @@ class ResourceDocRegistryParityTest extends ServerSetup { } } - // The three named pins below are the three historical drift instances. The generic loop - // above would also catch them, but naming them keeps the specific regressions legible. + // Guards the one hand-maintained knob left in the registry: if a v8.0.0 aggregation is added + // without moving obpUnionVersion, the union would keep serving the v7 surface and every + // v8-only operation id would silently be unresolvable -- the exact bug this PR started from. + scenario("obpUnionVersion is the newest OBP-standard version in the registry", RegistryParityTag) { + val obpVersions = ResourceDocRegistry.registry.keys.toList.collect { + case sv: ScannedApiVersion + if sv.apiStandard == ApiStandards.obp.toString && + sv != ApiVersion.`dynamic-endpoint` && sv != ApiVersion.`dynamic-entity` => sv + } + obpVersions should not be empty + // ApiVersionUtils.versions lists the OBP versions oldest-first, so the highest index wins. + val newest = obpVersions.maxBy(ApiVersionUtils.versions.indexOf(_)) + withClue(s"registry holds OBP versions ${obpVersions.map(_.fullyQualifiedVersion).mkString(", ")} " + + s"but obpUnionVersion is ${ResourceDocRegistry.obpUnionVersion} ") { + newest shouldBe ResourceDocRegistry.obpUnionVersion + } + } + + // The three named pins below are the three historical drift instances. They are NOT redundant + // with the loop above: both sides of that loop are now derived from ResourceDocRegistry, so its + // membership half holds by construction and cannot fail. What the loop still catches is a + // surface going empty; what these pins still catch is a specific operation id disappearing. scenario("the operation id from the sandbox bug report resolves", RegistryParityTag) { allOperationIds should contain("BGv2-getAccountDetails") From 39c131ab468fabeebc56e65bd8be305ad770698a Mon Sep 17 00:00:00 2001 From: Hongwei Date: Mon, 31 Aug 2026 23:52:23 +0200 Subject: [PATCH 09/12] fix: treat an unset berlin_group_v1_3_alias_path as no alias at all "".split("/") returns Array(""), not an empty array, so berlinGroupV13AliasPath was List("") on a default instance -- nonEmpty. Every downstream `if (berlinGroupV13AliasPath.nonEmpty)` guard therefore took its ACTIVE branch with an empty prefix: Http4sBGv13Alias published 55 docs stamped with the degenerate ScannedApiVersion("", "", ""), whose operation ids came out as `BG-`, and its route bridge matched the prefix "/" (every request) only to fall through again. That was invisible while the alias sat outside the global operation-id union. Now that this branch folds it in, those 55 junk ids became resolvable: verified against a running default instance that api-collection-endpoint creation accepted BG-getAccountDetails and BG-getPaymentInitiationStatus with 201, naming endpoints no route serves. Filtering empty segments makes "unset" mean "inactive" again -- both now return 400, while BGv1.3, BGv2, UK and OBP ids are unaffected and /resource-docs/BGv1.3/obp still serves its 55 docs. OBP_BERLIN_GROUP_1_3_Alias.apiVersion has to guard .head/.last against the now genuinely empty list: the ScannedApis classpath scan catches a throwing companion and only logs a warning, so an unguarded NoSuchElementException would drop the alias silently. Inactive registrations keep the empty-string version, which deliberately does not equal ConstantsBG.berlinGroupVersion1 -- colliding there would let this doc-less object win ScannedApis' .toMap and blank out the canonical BG v1.3 resource docs. The alias assertions in both tests no longer depend on a prop that only exists in a gitignored file. test.default.props is excluded by .gitignore:21, so the CI workflows carried berlin_group_v1_3_alias_path while a fresh clone or an IDE runner did not: deleting the line locally reproduced two failures whose messages gave no hint a prop was missing. They now cancel with an explanatory message when the alias is inactive, and read the expected operation id back from the alias's own docs instead of hard-coding the BGv1- prefix, which is derived from the configured path. Verified both ways: with the prop set 13/13 pass, without it 11 pass and 2 cancel. Full local suite 3573/0. --- .../v1_3/OBP_BERLIN_GROUP_1_3_Alias.scala | 16 ++++++++- .../main/scala/code/api/util/APIUtil.scala | 9 ++++- .../util/ResourceDocRegistryParityTest.scala | 35 +++++++++++++++---- .../v4_0_0/ApiCollectionEndpointTest.scala | 33 ++++++++++------- 4 files changed, 72 insertions(+), 21 deletions(-) diff --git a/obp-api/src/main/scala/code/api/berlin/group/v1_3/OBP_BERLIN_GROUP_1_3_Alias.scala b/obp-api/src/main/scala/code/api/berlin/group/v1_3/OBP_BERLIN_GROUP_1_3_Alias.scala index 2beba04807..8fc872c200 100644 --- a/obp-api/src/main/scala/code/api/berlin/group/v1_3/OBP_BERLIN_GROUP_1_3_Alias.scala +++ b/obp-api/src/main/scala/code/api/berlin/group/v1_3/OBP_BERLIN_GROUP_1_3_Alias.scala @@ -47,8 +47,22 @@ import scala.collection.mutable.ArrayBuffer */ object OBP_BERLIN_GROUP_1_3_Alias extends OBPRestHelper with MdcLoggable with ScannedApis { + /** + * The version this aggregator registers under. + * + * `berlinGroupV13AliasPath` is empty when `berlin_group_v1_3_alias_path` is unset, so `.head` / + * `.last` must be guarded: this object is instantiated by the ScannedApis classpath scan, which + * catches a throwing companion and merely logs a warning, so an unguarded NoSuchElementException + * would drop the alias silently. Inactive registrations keep the empty-string version they have + * always had, which no request can address and which deliberately does NOT equal + * ConstantsBG.berlinGroupVersion1 -- colliding with the canonical BG v1.3 key would let this + * (doc-less) object win ScannedApis' `.toMap` and blank out /resource-docs/BGv1.3/obp. + */ override val apiVersion: ScannedApiVersion = - ScannedApiVersion(berlinGroupV13AliasPath.head, berlinGroupV13AliasPath.head, berlinGroupV13AliasPath.last) + if (berlinGroupV13AliasPath.nonEmpty) + ScannedApiVersion(berlinGroupV13AliasPath.head, berlinGroupV13AliasPath.head, berlinGroupV13AliasPath.last) + else + ScannedApiVersion("", "", "") val versionStatus: String = ApiVersionStatus.DRAFT.toString diff --git a/obp-api/src/main/scala/code/api/util/APIUtil.scala b/obp-api/src/main/scala/code/api/util/APIUtil.scala index 4d9f61bcff..b9b110654c 100644 --- a/obp-api/src/main/scala/code/api/util/APIUtil.scala +++ b/obp-api/src/main/scala/code/api/util/APIUtil.scala @@ -4756,7 +4756,14 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{ } ) - val berlinGroupV13AliasPath = APIUtil.getPropsValue("berlin_group_v1_3_alias_path","").split("/").toList.map(_.trim) + // Empty segments are dropped so that "unset" really means "no alias". Without the filter an + // absent prop yields List("") -- "".split("/") returns Array(""), not an empty array -- which is + // nonEmpty, so every `if (berlinGroupV13AliasPath.nonEmpty)` guard downstream took its ACTIVE + // branch on a default instance: Http4sBGv13Alias published 55 docs stamped with the degenerate + // version ScannedApiVersion("", "", ""), whose operation ids came out as `BG-`, and its + // route bridge matched on the prefix "/" (every path) only to fall through again. + val berlinGroupV13AliasPath = + APIUtil.getPropsValue("berlin_group_v1_3_alias_path","").split("/").toList.map(_.trim).filter(_.nonEmpty) val getAtmsIsPublic = APIUtil.getPropsAsBoolValue("apiOptions.getAtmsIsPublic", true) diff --git a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala index 3e3167c83a..f574d1a42b 100644 --- a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala +++ b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala @@ -1,5 +1,6 @@ package code.api.util +import code.api.berlin.group.v1_3.{Http4sBGv13Alias, OBP_BERLIN_GROUP_1_3_Alias} import code.setup.ServerSetup import com.openbankproject.commons.util.{ApiStandards, ApiVersion, ScannedApiVersion} import org.scalatest.Tag @@ -25,7 +26,19 @@ class ResourceDocRegistryParityTest extends ServerSetup { private lazy val allOperationIds: Set[String] = APIUtil.getAllResourceDocs.map(_.operationId).toSet + // The Berlin Group v1.3 alias is the one surface in the union a deployment can switch off: + // berlin_group_v1_3_alias_path is unset by default and is supplied for test runs by + // test.default.props and by the two CI workflows. test.default.props is gitignored + // (.gitignore:21), so a fresh clone, a colleague's checkout or an IDE ScalaTest run may not have + // it -- assertions that genuinely need the alias cancel there instead of failing with a message + // that gives no hint a prop is missing. + private lazy val aliasVersion: ApiVersion = OBP_BERLIN_GROUP_1_3_Alias.apiVersion + private lazy val aliasIsConfigured: Boolean = Http4sBGv13Alias.resourceDocs.nonEmpty + private val aliasNotConfigured = + "berlin_group_v1_3_alias_path is not set, so the Berlin Group v1.3 alias contributes no docs" + private def label(version: ApiVersion): String = version match { + case v if v == aliasVersion && !aliasIsConfigured => "Berlin Group v1.3 alias (not configured)" case sv: ScannedApiVersion => sv.fullyQualifiedVersion case other => other.toString } @@ -35,17 +48,18 @@ class ResourceDocRegistryParityTest extends ServerSetup { // deliberately out of the union; see ResourceDocRegistry.obpUnionVersion for why, and for the // accepted consequence that an operation id living only in a superseded aggregation stays // unresolvable. - private lazy val surfaces: List[(String, Seq[String])] = + private lazy val surfaces: List[(ApiVersion, String, Seq[String])] = ResourceDocRegistry.unionVersions.toList - .map(version => (label(version), ResourceDocRegistry.docsFor(version).map(_.operationId))) + .map(version => (version, label(version), ResourceDocRegistry.docsFor(version).map(_.operationId))) feature("getAllResourceDocs contains every per-standard resource-doc surface the union covers") { scenario("the registry itself is non-empty", RegistryParityTag) { surfaces should not be empty } - surfaces.foreach { case (label, operationIds) => + surfaces.foreach { case (version, label, operationIds) => scenario(s"$label operation ids are all resolvable globally", RegistryParityTag) { + if (version == aliasVersion && !aliasIsConfigured) cancel(aliasNotConfigured) // Non-empty matters as much as membership: an empty surface is trivially a subset of the // union, so without this a standard whose docs silently stop being registered (the very // failure mode this test exists for) would pass unnoticed. @@ -86,11 +100,18 @@ class ResourceDocRegistryParityTest extends ServerSetup { allOperationIds should contain("BGv2-getAccountDetails") } - // berlin_group_v1_3_alias_path is set in test.default.props precisely so this surface is - // reliably non-empty here (see that file's comment on why it must be set before boot, not - // toggled per-test) -- pin a concrete alias operation id, not just the generic loop above. + // The alias's operation-id prefix is derived from the configured path (0.6/v1 in the test props + // yields BGv1-...), so the expected id is read back from the alias's own docs rather than + // hard-coded -- a deployment that configures a different path would otherwise fail here for no + // real reason. scenario("the operation id from the Berlin Group v1.3 alias resolves", RegistryParityTag) { - allOperationIds should contain("BGv1-getPaymentInitiationStatus") + if (!aliasIsConfigured) cancel(aliasNotConfigured) + val aliasOperationId = Http4sBGv13Alias.resourceDocs + .find(_.partialFunctionName == "getPaymentInitiationStatus").map(_.operationId) + withClue("the alias is configured but publishes no getPaymentInitiationStatus doc ") { + aliasOperationId shouldBe defined + } + allOperationIds should contain(aliasOperationId.get) } // The union used to be built from the v6.0.0 aggregation, so v7-only operation ids were diff --git a/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala b/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala index 9f1ce7e465..26d40aa469 100644 --- a/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala +++ b/obp-api/src/test/scala/code/api/v4_0_0/ApiCollectionEndpointTest.scala @@ -27,6 +27,7 @@ package code.api.v4_0_0 import org.json4s._ import code.api.ResourceDocs1_4_0.SwaggerDefinitionsJSON +import code.api.berlin.group.v1_3.Http4sBGv13Alias import code.api.util.APIUtil.OAuth._ import code.api.v4_0_0.APIMethods400.Implementations4_0_0 import com.github.dwickern.macros.NameOf.nameOf @@ -224,17 +225,24 @@ class ApiCollectionEndpointTest extends V400ServerSetup { val operationId= apiCollectionEndpoint.operation_id } - { - // Regression pin for the Berlin Group v1.3 alias gap: berlin_group_v1_3_alias_path - // (set to 0.6/v1 in test.default.props) activates Http4sBGv13Alias, whose docs are - // re-stamped copies of the canonical BG v1.3 docs carrying their own operation ids - // (BGv1-... here, not BGv1.3-...). These were served by the resource-docs dispatcher - // via ScannedApis discovery but missing from the global operation-id union, the same - // class of gap as BGv2 above. - Then(s"we test the $ApiEndpoint6- BGv1-getPaymentInitiationStatus (Berlin Group v1.3 alias)") + // Regression pin for the Berlin Group v1.3 alias gap: when berlin_group_v1_3_alias_path is + // set (0.6/v1 in test.default.props and in both CI workflows) Http4sBGv13Alias publishes + // re-stamped copies of the canonical BG v1.3 docs under their own operation ids -- served by + // the resource-docs dispatcher via ScannedApis discovery, but formerly missing from the + // global operation-id union, the same class of gap as BGv2 above. + // + // Guarded on the alias actually being configured, and the expected id is read back from its + // own docs rather than hard-coded: test.default.props is gitignored (.gitignore:21), so a + // fresh clone or an IDE runner may not carry that prop, and a deployment may configure a + // different path (which changes the id's prefix). + val aliasOperationId: Option[String] = Http4sBGv13Alias.resourceDocs + .find(_.partialFunctionName == "getPaymentInitiationStatus").map(_.operationId) + + aliasOperationId.foreach { opId => + Then(s"we test the $ApiEndpoint6- $opId (Berlin Group v1.3 alias)") val requestApiCollectionEndpoint = (v4_0_0_Request / "my" / "api-collection-ids" / apiCollectionId / "api-collection-endpoints").POST <@ (user1) - lazy val postApiCollectionEndpointJson = SwaggerDefinitionsJSON.postApiCollectionEndpointJson400.copy(operation_id="BGv1-getPaymentInitiationStatus") + lazy val postApiCollectionEndpointJson = SwaggerDefinitionsJSON.postApiCollectionEndpointJson400.copy(operation_id = opId) val responseApiCollectionEndpointJson = makePostRequest(requestApiCollectionEndpoint, write(postApiCollectionEndpointJson)) Then("We should get a 201") @@ -243,8 +251,6 @@ class ApiCollectionEndpointTest extends V400ServerSetup { apiCollectionEndpoint.operation_id should be (postApiCollectionEndpointJson.operation_id) apiCollectionEndpoint.api_collection_endpoint_id shouldNot be (null) - - val operationId= apiCollectionEndpoint.operation_id } { @@ -280,7 +286,10 @@ class ApiCollectionEndpointTest extends V400ServerSetup { val apiCollectionsJsonGet400 = responseGet.body.extract[ApiCollectionEndpointsJson400] - apiCollectionsJsonGet400.api_collection_endpoints.length should be (7) + // Six unconditional cases above, plus the Berlin Group v1.3 alias one when that alias is + // configured for this run. + val expected = if (aliasOperationId.isDefined) 7 else 6 + apiCollectionsJsonGet400.api_collection_endpoints.length should be (expected) } } } From 3125596ae2846b18d298f03fc47c9c541b0b877c Mon Sep 17 00:00:00 2001 From: Hongwei Date: Tue, 1 Sep 2026 00:08:26 +0200 Subject: [PATCH 10/12] fix: give the registry a tie-free order and a guard that cannot be bypassed Two follow-ups from reviewing the registry work itself. The scanned half was sorted by fullyQualifiedVersion, which concatenates apiStandard.toUpperCase and apiShortVersion and can therefore collide across distinct keys -- ("BG", "v1.3") and ("BGV", "1.3") both render "BGV1.3", and berlin_group_v1_3_alias_path lets a deployment choose the alias's half of such a pair. sortBy is only stable with respect to its input, and the input is the unordered ScannedApis.versionMapScannedApis, so a tie would hand the order back to hash iteration and with it the `.toMap` winner for a shared partialFunctionName. Sort by (apiStandard, apiShortVersion) instead: that pair is exactly ScannedApiVersion's equals/hashCode key, so two distinct keys of that Map always differ in it and the order is total. The resulting sequence is unchanged -- alias, BG v1.3, BG v2, UK 2.0/3.1/4.0.1 -- so BG v2 keeps winning the names it shares with the alias. The obpUnionVersion guard ranked versions with ApiVersionUtils.versions.indexOf, which returns -1 for anything absent from that equally hand-maintained list. A -1 loses every maxBy comparison, so adding a v8.0.0 aggregation to the registry while forgetting ApiVersionUtils.versions left v7 as the maximum and the scenario green -- precisely the two-places-to-edit slip it was written to catch. Assert first that every OBP version in the registry can be ranked at all. Verified by injecting an unregistered OBPv8.0.0: the guard now fails with "OBP versions in the registry but missing from ApiVersionUtils.versions: OBPv8.0.0", where before it passed. Full local suite 3573/0. --- .../scala/code/api/util/ResourceDocRegistry.scala | 12 +++++++++++- .../api/util/ResourceDocRegistryParityTest.scala | 14 +++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala b/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala index 4b308f3e15..882c4683e3 100644 --- a/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala +++ b/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala @@ -71,11 +71,21 @@ object ResourceDocRegistry { // `partialFunctionName -> operationId` with `.toMap`, where the LAST entry wins, so leaving the // order to Map's hash iteration would leave the reported operation_id undefined and let it // shift silently whenever a standard is added or removed. + // + // The sort key is (apiStandard, apiShortVersion) rather than fullyQualifiedVersion because it + // cannot tie: that pair is exactly ScannedApiVersion's equals/hashCode key, so two distinct + // keys of this Map always differ in it, and sortBy therefore yields a total order rather than + // falling back to the unordered input for ties. fullyQualifiedVersion concatenates the two + // (apiStandard.toUpperCase + apiShortVersion) and so can collide across distinct keys -- + // ("BG", "v1.3") and ("BGV", "1.3") both render "BGV1.3" -- which a deployment could reach by + // configuring berlin_group_v1_3_alias_path. The resulting order is unchanged in practice: + // alias ("0.6"/"") first, then BG v1.3, BG v2, then UK 2.0/3.1/4.0.1, so BG v2 keeps winning + // the names it shares with the alias, matching the behaviour before this branch. val scanned: ListMap[ApiVersion, () => Seq[ResourceDoc]] = ScannedApis.versionMapScannedApis.toSeq .collect { case (version: ScannedApiVersion, apis) if !explicit.contains(version) => version -> (() => apis.allResourceDocs.toSeq) } - .sortBy(_._1.fullyQualifiedVersion) + .sortBy(entry => (entry._1.apiStandard, entry._1.apiShortVersion)) .foldLeft(ListMap.empty[ApiVersion, () => Seq[ResourceDoc]])(_ + _) explicit ++ scanned } diff --git a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala index f574d1a42b..25b065255b 100644 --- a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala +++ b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala @@ -83,7 +83,19 @@ class ResourceDocRegistryParityTest extends ServerSetup { sv != ApiVersion.`dynamic-endpoint` && sv != ApiVersion.`dynamic-entity` => sv } obpVersions should not be empty - // ApiVersionUtils.versions lists the OBP versions oldest-first, so the highest index wins. + + // Rank by position in ApiVersionUtils.versions, which lists the OBP versions oldest-first. + // indexOf returns -1 for anything absent from that (also hand-maintained) list, and a -1 + // would lose every maxBy comparison -- so a v8.0.0 added to the registry but not to + // ApiVersionUtils.versions would leave v7 as the maximum and let this scenario pass, in + // exactly the two-places-to-edit case it exists to catch. Establish coverage first. + val unranked = obpVersions.filter(ApiVersionUtils.versions.indexOf(_) < 0) + withClue(s"OBP versions in the registry but missing from ApiVersionUtils.versions: " + + s"${unranked.map(_.fullyQualifiedVersion).mkString(", ")} -- add them there so they can be " + + s"ranked, otherwise this guard cannot see them ") { + unranked shouldBe empty + } + val newest = obpVersions.maxBy(ApiVersionUtils.versions.indexOf(_)) withClue(s"registry holds OBP versions ${obpVersions.map(_.fullyQualifiedVersion).mkString(", ")} " + s"but obpUnionVersion is ${ResourceDocRegistry.obpUnionVersion} ") { From 45a965a8f6c93c3fcce0800b8eeceb39fd714cb5 Mon Sep 17 00:00:00 2001 From: Hongwei Date: Tue, 1 Sep 2026 08:38:50 +0200 Subject: [PATCH 11/12] fix: restore Berlin Group precedence and stop registering an empty API version Two defects found reviewing the registry against the union it replaced. Berlin Group and UK Open Banking both publish getBalances, getAccountList and getAccountBalances. Http4s600's top-apis/popular-apis and JSONFactory6.0.0's metrics resolve a partialFunctionName with `.toMap`, which keeps the LAST matching entry, so registry order decides the operation_id they report. The hand-written union listed UK before BG, giving Berlin Group all three; sorting the scanned standards alphabetically put UK last and silently flipped them to UKv4.0.1-getBalances, UKv2.0-getAccountList and UKv2.0-getAccountBalances. Replace the alphabetical sort with an explicit standardPrecedence (UK Open Banking, then Berlin Group) and move Berlin Group v1.3 out of the explicit block so it is ordered by that precedence rather than pinned ahead of it. A standard absent from the list -- including the alias, whose apiStandard is whatever berlin_group_v1_3_alias_path names -- ranks below all of them and can never override a first-class standard. Verified against a running instance: the three names resolve to BGv1.3-getBalances, BGv2-getAccountList and BGv2-getAccountBalances again, matching the values measured before this branch. A configuration-gated standard that is switched off reports ScannedApiVersion("", "", ""), whose fullyQualifiedVersion is "" as well. While ScannedApis kept that registration, ApiVersionUtils.valueOf("") resolved successfully and, because the resource-docs route tolerates an empty path segment, GET /obp/v7.0.0/resource-docs//obp answered 200 with an empty document list where any other unknown version string gets 400 InvalidApiVersionString. Drop unaddressable registrations in ScannedApis.versionMapScannedApis, which fixes ApiVersionUtils, ResourceDocRegistry and Boot's version enablement in one place. Verified: that request now returns 400, and BG v1.3, BG v2, UK 4.0.1 and OBP v7.0.0 still serve 55, 22, 89 and 1031 docs. With the alias no longer registered while inactive it is not a registry surface at all, so the parity test's now-unreachable "cancel when unconfigured" branch is removed. Both defects reached a green CI because nothing asserted either value; two scenarios now pin them. Full local suite 3575/0. --- .../code/api/util/ResourceDocRegistry.scala | 64 +++++++++++-------- .../scala/code/api/util/ScannedApis.scala | 12 ++++ .../util/ResourceDocRegistryParityTest.scala | 48 +++++++++++--- 3 files changed, 89 insertions(+), 35 deletions(-) diff --git a/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala b/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala index 882c4683e3..88104642fb 100644 --- a/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala +++ b/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala @@ -40,7 +40,6 @@ object ResourceDocRegistry { lazy val registry: ListMap[ApiVersion, () => Seq[ResourceDoc]] = { val explicit: ListMap[ApiVersion, () => Seq[ResourceDoc]] = ListMap( v7_0_0 -> (() => code.api.v7_0_0.Http4s700.allResourceDocs.toSeq), - ConstantsBG.berlinGroupVersion1 -> (() => code.api.berlin.group.v1_3.Http4sBGv13.resourceDocs.toSeq), v6_0_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v600.toSeq), v5_1_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v510.toSeq), v5_0_0 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v500.toSeq), @@ -55,41 +54,56 @@ object ResourceDocRegistry { v1_2_1 -> (() => code.api.util.http4s.Http4sResourceDocAggregation.v121.toSeq), `dynamic-endpoint` -> (() => code.api.dynamic.endpoint.OBPAPIDynamicEndpoint.allResourceDocs.toSeq), `dynamic-entity` -> (() => code.api.dynamic.entity.OBPAPIDynamicEntity.allResourceDocs.toSeq) - // Berlin Group v2 is NOT listed here -- Http4sBGv2 is a ScannedApis registrant (its - // apiVersion is ConstantsBG.berlinGroupVersion2), so it is picked up by `scanned` below. + // Neither Berlin Group nor UK Open Banking is listed here: they are all ScannedApis + // registrants, so `scanned` picks them up and -- crucially -- orders them against each other + // by standardPrecedence below. Naming one of them here would pin it ahead of that ordering. ) // Every standard discovered via ScannedApis (UK OB 200/310/401, BG v1.3 canonical + alias, - // BG v2, and any future `with ScannedApis` standard). Explicit entries win on key collision -- - // BG v1.3 canonical is both explicit above and a registrant, same underlying buffer either way. + // BG v2, and any future `with ScannedApis` standard), folded into a ListMap so the registry has + // ONE defined iteration order. // - // Sorted, and folded into a ListMap, so the registry has ONE defined iteration order. - // ScannedApis.versionMapScannedApis is an unordered Map, and several standards share - // partialFunctionNames (the BG v1.3 alias re-stamps the canonical BG v1.3 docs, so it collides - // with both BG v1.3 and -- on getAccountDetails, getAccountList, getCardAccountBalances, - // getCardAccountTransactionList, getTransactionDetails -- with BG v2). Consumers such as - // Http4s600's top-apis/popular-apis and JSONFactory6.0.0's metrics build - // `partialFunctionName -> operationId` with `.toMap`, where the LAST entry wins, so leaving the - // order to Map's hash iteration would leave the reported operation_id undefined and let it - // shift silently whenever a standard is added or removed. - // - // The sort key is (apiStandard, apiShortVersion) rather than fullyQualifiedVersion because it - // cannot tie: that pair is exactly ScannedApiVersion's equals/hashCode key, so two distinct - // keys of this Map always differ in it, and sortBy therefore yields a total order rather than - // falling back to the unordered input for ties. fullyQualifiedVersion concatenates the two - // (apiStandard.toUpperCase + apiShortVersion) and so can collide across distinct keys -- - // ("BG", "v1.3") and ("BGV", "1.3") both render "BGV1.3" -- which a deployment could reach by - // configuring berlin_group_v1_3_alias_path. The resulting order is unchanged in practice: - // alias ("0.6"/"") first, then BG v1.3, BG v2, then UK 2.0/3.1/4.0.1, so BG v2 keeps winning - // the names it shares with the alias, matching the behaviour before this branch. + // Order matters beyond determinism: Http4s600's top-apis/popular-apis and JSONFactory6.0.0's + // metrics build `partialFunctionName -> operationId` with `.toMap`, where the LAST entry wins. + // Berlin Group and UK Open Banking share three partialFunctionNames -- getBalances, + // getAccountList, getAccountBalances -- and the hand-written union this registry replaced + // listed UK before BG, so Berlin Group won all three. Sorting alphabetically put UK last and + // silently flipped them to UKv4.0.1-getBalances / UKv2.0-getAccountList / + // UKv2.0-getAccountBalances in metrics output, so the precedence is now explicit. val scanned: ListMap[ApiVersion, () => Seq[ResourceDoc]] = ScannedApis.versionMapScannedApis.toSeq .collect { case (version: ScannedApiVersion, apis) if !explicit.contains(version) => version -> (() => apis.allResourceDocs.toSeq) } - .sortBy(entry => (entry._1.apiStandard, entry._1.apiShortVersion)) + .sortBy(entry => sortKey(entry._1)) .foldLeft(ListMap.empty[ApiVersion, () => Seq[ResourceDoc]])(_ + _) explicit ++ scanned } + /** + * Standards in ASCENDING precedence: a standard later in this list wins a partialFunctionName it + * shares with an earlier one, because the `.toMap` consumers keep the last entry. This + * reproduces the order of the hand-written union that preceded this registry (UK Open Banking, + * then Berlin Group). + * + * A standard that is not listed ranks below all of them -- including the Berlin Group v1.3 alias, + * whose apiStandard is whatever `berlin_group_v1_3_alias_path` names, so it can never override a + * first-class standard no matter how a deployment configures it. + */ + private val standardPrecedence: List[String] = + List(ApiVersion.ukOpenBankingV20.apiStandard, ConstantsBG.berlinGroupVersion1.apiStandard) + + /** + * Total order over registry keys: precedence first, then the version's own identity. + * + * The tie-breaker is (apiStandard, apiShortVersion) rather than fullyQualifiedVersion because + * that pair is exactly ScannedApiVersion's equals/hashCode key, so two distinct keys of a Map + * keyed by version always differ in it and sortBy never has to fall back to the unordered input. + * fullyQualifiedVersion concatenates the two (apiStandard.toUpperCase + apiShortVersion) and can + * therefore collide across distinct keys -- ("BG", "v1.3") and ("BGV", "1.3") both render + * "BGV1.3" -- which a deployment could reach through berlin_group_v1_3_alias_path. + */ + private def sortKey(version: ScannedApiVersion): (Int, String, String) = + (standardPrecedence.indexOf(version.apiStandard), version.apiStandard, version.apiShortVersion) + /** What the per-version resource-docs dispatcher serves for this version (empty if unknown). */ def docsFor(version: ApiVersion): Seq[ResourceDoc] = registry.get(version).map(_ ()).getOrElse(Nil) diff --git a/obp-api/src/main/scala/code/api/util/ScannedApis.scala b/obp-api/src/main/scala/code/api/util/ScannedApis.scala index bfbc688985..238de80b0e 100644 --- a/obp-api/src/main/scala/code/api/util/ScannedApis.scala +++ b/obp-api/src/main/scala/code/api/util/ScannedApis.scala @@ -21,9 +21,21 @@ trait ScannedApis { object ScannedApis { /** * this map value are all scanned objects those extends ScannedApiVersion, the key is it apiVersion field + * + * Registrants whose version carries no urlPrefix, apiStandard and apiShortVersion are dropped: + * such a version addresses nothing, and it is how a configuration-gated standard reports itself + * as switched off (OBP_BERLIN_GROUP_1_3_Alias falls back to ScannedApiVersion("", "", "") when + * berlin_group_v1_3_alias_path is unset). Keeping it here leaked into everything built from this + * map: its fullyQualifiedVersion is "" too, so ApiVersionUtils.valueOf("") resolved successfully + * and GET /obp/v7.0.0/resource-docs//obp answered 200 with an empty document list instead of the + * 400 InvalidApiVersionString any other unknown version string gets. */ lazy val versionMapScannedApis: Map[ScannedApiVersion, ScannedApis] = ClassScanUtils.getSubTypeObjects[ScannedApis] + .filter(it => isAddressable(it.apiVersion)) .map(it=> (it.apiVersion, it)) .toMap + + private def isAddressable(version: ScannedApiVersion): Boolean = + version.urlPrefix.trim.nonEmpty || version.apiStandard.trim.nonEmpty || version.apiShortVersion.trim.nonEmpty } diff --git a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala index 25b065255b..cbb5f9c624 100644 --- a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala +++ b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala @@ -1,6 +1,6 @@ package code.api.util -import code.api.berlin.group.v1_3.{Http4sBGv13Alias, OBP_BERLIN_GROUP_1_3_Alias} +import code.api.berlin.group.v1_3.Http4sBGv13Alias import code.setup.ServerSetup import com.openbankproject.commons.util.{ApiStandards, ApiVersion, ScannedApiVersion} import org.scalatest.Tag @@ -26,19 +26,19 @@ class ResourceDocRegistryParityTest extends ServerSetup { private lazy val allOperationIds: Set[String] = APIUtil.getAllResourceDocs.map(_.operationId).toSet - // The Berlin Group v1.3 alias is the one surface in the union a deployment can switch off: + // The Berlin Group v1.3 alias is the one surface a deployment can switch off: // berlin_group_v1_3_alias_path is unset by default and is supplied for test runs by // test.default.props and by the two CI workflows. test.default.props is gitignored // (.gitignore:21), so a fresh clone, a colleague's checkout or an IDE ScalaTest run may not have - // it -- assertions that genuinely need the alias cancel there instead of failing with a message - // that gives no hint a prop is missing. - private lazy val aliasVersion: ApiVersion = OBP_BERLIN_GROUP_1_3_Alias.apiVersion + // it -- the pin below cancels there instead of failing with a message that gives no hint a prop + // is missing. The per-surface loop needs no such guard: an unconfigured alias reports the + // unaddressable ScannedApiVersion("", "", ""), which ScannedApis now drops, so it is not a + // surface at all rather than an empty one. private lazy val aliasIsConfigured: Boolean = Http4sBGv13Alias.resourceDocs.nonEmpty private val aliasNotConfigured = "berlin_group_v1_3_alias_path is not set, so the Berlin Group v1.3 alias contributes no docs" private def label(version: ApiVersion): String = version match { - case v if v == aliasVersion && !aliasIsConfigured => "Berlin Group v1.3 alias (not configured)" case sv: ScannedApiVersion => sv.fullyQualifiedVersion case other => other.toString } @@ -48,18 +48,17 @@ class ResourceDocRegistryParityTest extends ServerSetup { // deliberately out of the union; see ResourceDocRegistry.obpUnionVersion for why, and for the // accepted consequence that an operation id living only in a superseded aggregation stays // unresolvable. - private lazy val surfaces: List[(ApiVersion, String, Seq[String])] = + private lazy val surfaces: List[(String, Seq[String])] = ResourceDocRegistry.unionVersions.toList - .map(version => (version, label(version), ResourceDocRegistry.docsFor(version).map(_.operationId))) + .map(version => (label(version), ResourceDocRegistry.docsFor(version).map(_.operationId))) feature("getAllResourceDocs contains every per-standard resource-doc surface the union covers") { scenario("the registry itself is non-empty", RegistryParityTag) { surfaces should not be empty } - surfaces.foreach { case (version, label, operationIds) => + surfaces.foreach { case (label, operationIds) => scenario(s"$label operation ids are all resolvable globally", RegistryParityTag) { - if (version == aliasVersion && !aliasIsConfigured) cancel(aliasNotConfigured) // Non-empty matters as much as membership: an empty surface is trivially a subset of the // union, so without this a standard whose docs silently stop being registered (the very // failure mode this test exists for) would pass unnoticed. @@ -103,6 +102,35 @@ class ResourceDocRegistryParityTest extends ServerSetup { } } + // Berlin Group and UK Open Banking both publish getBalances, getAccountList and + // getAccountBalances. Http4s600's top-apis/popular-apis and JSONFactory6.0.0's metrics resolve + // a partialFunctionName with `.toMap`, which keeps the LAST matching entry, so the registry's + // iteration order decides the operation_id those endpoints report. The hand-written union that + // preceded this registry listed UK before BG, giving Berlin Group the three names; sorting the + // scanned standards alphabetically silently handed them to UK Open Banking instead. This pins + // the resolved values so the precedence cannot drift again unnoticed. + scenario("Berlin Group keeps the partialFunctionNames it shares with UK Open Banking", RegistryParityTag) { + val resolved = APIUtil.getAllResourceDocs + .map(doc => doc.partialFunctionName -> doc.operationId).toMap + resolved.get("getBalances") shouldBe Some("BGv1.3-getBalances") + resolved.get("getAccountList") shouldBe Some("BGv2-getAccountList") + resolved.get("getAccountBalances") shouldBe Some("BGv2-getAccountBalances") + } + + // An unconfigured configuration-gated standard reports ScannedApiVersion("", "", ""), whose + // fullyQualifiedVersion is "" as well. While ScannedApis kept it, ApiVersionUtils.valueOf("") + // resolved successfully and GET /obp/v7.0.0/resource-docs//obp answered 200 with an empty + // document list instead of the 400 every other unknown version string gets. + scenario("an unaddressable empty version is not a registered API version", RegistryParityTag) { + ScannedApis.versionMapScannedApis.keys.foreach { version => + withClue(s"$version was registered despite addressing nothing ") { + (version.urlPrefix.trim + version.apiStandard.trim + version.apiShortVersion.trim) should not be empty + } + } + ApiVersionUtils.versions.map(_.fullyQualifiedVersion) should not contain "" + an[IllegalArgumentException] should be thrownBy ApiVersionUtils.valueOf("") + } + // The three named pins below are the three historical drift instances. They are NOT redundant // with the loop above: both sides of that loop are now derived from ResourceDocRegistry, so its // membership half holds by construction and cannot fail. What the loop still catches is a From 9b238b0d2e636b5e4d9d2fc9a172df2299d709db Mon Sep 17 00:00:00 2001 From: Hongwei Date: Tue, 1 Sep 2026 10:14:36 +0200 Subject: [PATCH 12/12] fix: rank the Berlin Group v1.3 alias by identity, not by its configured name standardPrecedence ranked a version by its apiStandard string, and the alias takes that string from the first segment of berlin_group_v1_3_alias_path. A deployment may point it at a name an existing standard already uses: configured as "BG/v9" the alias reports ScannedApiVersion("BG", "BG", "v9"), ranks alongside Berlin Group, and -- sorting after "v2" on the tie-breaker -- comes last, so its re-stamped copies won getBalances, getAccountList and getAccountBalances away from the canonical docs it had copied. Metrics, top-apis and popular-apis would then report BGv9-getBalances instead of BGv1.3-getBalances. The comment on standardPrecedence claimed the opposite, that the alias "can never override a first-class standard no matter how a deployment configures it". Match the alias by identity instead and rank it below every listed standard, which makes that claim true for any configuration. sortKey takes the derived alias version as a curried parameter and is package-private so the guarantee can be tested against a synthetic alias, rather than only under whichever berlin_group_v1_3_alias_path the JVM happens to have booted with. Verified both directions with the new scenario: reverting to the string-based rank fails it with "(1,BG,v9) was not less than (1,BG,v2)" -- the mechanism itself -- and it passes with the fix. Full local suite 3576/0. --- .../code/api/util/ResourceDocRegistry.scala | 32 ++++++++++++++----- .../util/ResourceDocRegistryParityTest.scala | 22 +++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala b/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala index 88104642fb..641c02ee48 100644 --- a/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala +++ b/obp-api/src/main/scala/code/api/util/ResourceDocRegistry.scala @@ -73,7 +73,7 @@ object ResourceDocRegistry { ScannedApis.versionMapScannedApis.toSeq .collect { case (version: ScannedApiVersion, apis) if !explicit.contains(version) => version -> (() => apis.allResourceDocs.toSeq) } - .sortBy(entry => sortKey(entry._1)) + .sortBy(entry => sortKey(code.api.berlin.group.v1_3.OBP_BERLIN_GROUP_1_3_Alias.apiVersion)(entry._1)) .foldLeft(ListMap.empty[ApiVersion, () => Seq[ResourceDoc]])(_ + _) explicit ++ scanned } @@ -82,27 +82,43 @@ object ResourceDocRegistry { * Standards in ASCENDING precedence: a standard later in this list wins a partialFunctionName it * shares with an earlier one, because the `.toMap` consumers keep the last entry. This * reproduces the order of the hand-written union that preceded this registry (UK Open Banking, - * then Berlin Group). - * - * A standard that is not listed ranks below all of them -- including the Berlin Group v1.3 alias, - * whose apiStandard is whatever `berlin_group_v1_3_alias_path` names, so it can never override a - * first-class standard no matter how a deployment configures it. + * then Berlin Group). A standard that is not listed ranks below all of them. */ private val standardPrecedence: List[String] = List(ApiVersion.ukOpenBankingV20.apiStandard, ConstantsBG.berlinGroupVersion1.apiStandard) + /** Below every entry of standardPrecedence, whose lowest index is -1 for an unlisted standard. */ + private val derivedStandardRank: Int = -2 + /** * Total order over registry keys: precedence first, then the version's own identity. * + * `derivedAliasVersion` is the version of a standard that merely re-publishes another standard's + * docs -- today only the Berlin Group v1.3 alias. It is matched by identity, NOT by its + * apiStandard, because that string is the first segment of `berlin_group_v1_3_alias_path` and a + * deployment may legitimately choose one that an existing standard already uses: configured as + * "BG/v9" the alias would otherwise rank alongside Berlin Group and, sorting after "v2", let its + * re-stamped copies win getBalances, getAccountList and getAccountBalances away from the + * canonical docs it copied. Ranking it derivedStandardRank keeps that impossible for any + * configuration. + * * The tie-breaker is (apiStandard, apiShortVersion) rather than fullyQualifiedVersion because * that pair is exactly ScannedApiVersion's equals/hashCode key, so two distinct keys of a Map * keyed by version always differ in it and sortBy never has to fall back to the unordered input. * fullyQualifiedVersion concatenates the two (apiStandard.toUpperCase + apiShortVersion) and can * therefore collide across distinct keys -- ("BG", "v1.3") and ("BGV", "1.3") both render * "BGV1.3" -- which a deployment could reach through berlin_group_v1_3_alias_path. + * + * Curried and package-private so a test can rank against a synthetic alias without having to + * restart the JVM under a different berlin_group_v1_3_alias_path. */ - private def sortKey(version: ScannedApiVersion): (Int, String, String) = - (standardPrecedence.indexOf(version.apiStandard), version.apiStandard, version.apiShortVersion) + private[util] def sortKey(derivedAliasVersion: ScannedApiVersion) + (version: ScannedApiVersion): (Int, String, String) = { + val rank = + if (version == derivedAliasVersion) derivedStandardRank + else standardPrecedence.indexOf(version.apiStandard) + (rank, version.apiStandard, version.apiShortVersion) + } /** What the per-version resource-docs dispatcher serves for this version (empty if unknown). */ def docsFor(version: ApiVersion): Seq[ResourceDoc] = registry.get(version).map(_ ()).getOrElse(Nil) diff --git a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala index cbb5f9c624..3816545da8 100644 --- a/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala +++ b/obp-api/src/test/scala/code/api/util/ResourceDocRegistryParityTest.scala @@ -1,5 +1,6 @@ package code.api.util +import code.api.berlin.group.ConstantsBG import code.api.berlin.group.v1_3.Http4sBGv13Alias import code.setup.ServerSetup import com.openbankproject.commons.util.{ApiStandards, ApiVersion, ScannedApiVersion} @@ -117,6 +118,27 @@ class ResourceDocRegistryParityTest extends ServerSetup { resolved.get("getAccountBalances") shouldBe Some("BGv2-getAccountBalances") } + // The Berlin Group v1.3 alias only re-publishes the canonical BG v1.3 docs, so it must never + // win a partialFunctionName away from the standard it copied. Its apiStandard is the first + // segment of berlin_group_v1_3_alias_path, so a deployment can point it at a name an existing + // standard already uses ("BG/v9"); ranking by that string alone put the alias alongside Berlin + // Group and, sorting after "v2", ahead of it. Ranking is by identity instead, and the synthetic + // alias below exercises the colliding configuration without needing a JVM under that prop. + scenario("a derived alias never outranks the standard it re-publishes", RegistryParityTag) { + val syntheticAlias = ScannedApiVersion("BG", "BG", "v9") + val rankOf = ResourceDocRegistry.sortKey(syntheticAlias) _ + withClue("the alias must sort before Berlin Group, i.e. lose the `.toMap` last-wins race ") { + rankOf(syntheticAlias) should be < rankOf(ConstantsBG.berlinGroupVersion2) + rankOf(syntheticAlias) should be < rankOf(ConstantsBG.berlinGroupVersion1) + } + withClue("the alias must also sort before UK Open Banking ") { + rankOf(syntheticAlias) should be < rankOf(ApiVersion.ukOpenBankingV401) + } + withClue("UK must still sort before Berlin Group, so BG keeps the names they share ") { + rankOf(ApiVersion.ukOpenBankingV401) should be < rankOf(ConstantsBG.berlinGroupVersion2) + } + } + // An unconfigured configuration-gated standard reports ScannedApiVersion("", "", ""), whose // fullyQualifiedVersion is "" as well. While ScannedApis kept it, ApiVersionUtils.valueOf("") // resolved successfully and GET /obp/v7.0.0/resource-docs//obp answered 200 with an empty