refactor: single source of truth for resource-doc registry (closes BG v1.3 alias gap) - #2902
Merged
simonredfern merged 12 commits intoSep 1, 2026
Conversation
…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.
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.
…ocs 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.
…ion 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.
hongwei1
changed the title
…int 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.
…evel 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.
…ift 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.
…efined 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.
"".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-<name>`, 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.
…passed
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.
…I 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.
…red 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.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A bug report proposed a
ResourceDocRegistryrefactor for a production bug: addingBGv2-getAccountDetailsto an API Collection failed with an invalid-operation-id error, because OBP-API kept two hand-maintained resource-doc registries (the per-version dispatcher used by/resource-docs/{VERSION}/...and API Explorer, and the union inAPIUtil.allStaticResourceDocs/getAllResourceDocsused wherever an operation id must be resolved) and they had drifted.Two corrections to that report, verified directly against
developat the time this branch was cut:OBP-30051) is wrong — the actual failure isOBP-40048: Invalid operation_id.a1231e3f5/60b9f75d1) before this branch was cut. That merged fix closed the BGv2 gap and the v6-vs-v7 aggregation gap, and already addedResourceDocRegistryParityTest.This PR verifies that merged fix end-to-end, closes the drift instance it left behind (the Berlin Group v1.3 alias), and — following up on the repo owner's answers to the three open questions the original report raised — implements the proposed refactor: a single
ResourceDocRegistryobject that both the dispatcher and the global union now derive from, so this class of drift becomes structurally impossible rather than something that has to be remembered by hand each time a standard is added.Changes
fix: addOBP_BERLIN_GROUP_1_3_Alias.allResourceDocstoAPIUtil.allStaticResourceDocs(superseded by commit 4, kept as its own step for bisectability). Reproduced the failure first (OBP-40048on a valid alias operation id against an instance withberlin_group_v1_3_alias_pathset), then confirmed the fix resolves it.test: extendResourceDocRegistryParityTestto cover the BG v1.3 alias surface.refactor: removeResourceDocsAPIMethods'sactiveResourceDocsmatch block — all ~19 arms were confirmed pure identity functions, a leftover from the pre-http4s Lift route-filter era.refactor: introducecode.api.util.ResourceDocRegistryas the single source of truth for "which resource docs does version X serve".Http4sBGv2becomes aScannedApisregistrant (fully convention-driven, like the other Berlin Group / UK Open Banking standards — no more hand-maintained entry or special case inApiVersionUtils.valueOf). The global union is now deduped byoperationId.ResourceDocRegistryParityTestis rewritten to iterate the registry itself rather than a hand-typed list of standards, so a newly added standard is covered by construction.test: add an HTTP-level regression test pinningBGv2-getAccountDetailsas a resolvableapi-collection-endpointoperation id (the exact request from the sandbox bug report) — previously only covered by a unit-level membership check.test: enable the Berlin Group v1.3 alias by default in the test environment (berlin_group_v1_3_alias_path=0.6/v1intest.default.propsand both CI workflows' generated copy of it), so its operation ids get the same HTTP-level regression coverage as commit 5, not just a unit-level check. This couldn't be done by toggling the prop per-test at runtime — the alias'sScannedApiVersionidentity is captured once by a process-wide classpath scan (ScannedApis.versionMapScannedApis, alazy valshared across the whole JVM/shard) that gets forced by the first unrelated request that falls through the route chain, almost always before any test-specific prop override could run.Test plan
POST /obp/v4.0.0/my/api-collections/{name}/api-collection-endpointswithoperation_id=BGv2-getAccountDetails→201(re-confirms the already-merged fix, both before and after the registry refactor)Content.vue→createMyAPICollectionEndpoint), not just curlOBP-40048on a BG v1.3 alias operation id against an instance withberlin_group_v1_3_alias_pathset, before the fix;201after201ApiCollectionEndpointTestnow exercises bothBGv2-getAccountDetailsand the alias'sBGv1-getPaymentInitiationStatusas real HTTP requests, alongside existing OBPv6.0.0/UK Open Banking/Berlin Group v1.3 canonical coverageResourceDocRegistryParityTestpins both operation ids and is registry-driven end to endResourceDocsTest/SwaggerDocsTest/V7ResourceDocsAggregationTest/Http4sBGv2ResourceDocTest/RetiredApiStandardsTest/GetScannedApiVersionsTestgreen (123 tests) after the registry refactor./run_tests_parallel.sh— 3583 tests, 0 failures (final run, after all 6 commits, alias enabled by default)