test: endpoint sweeps and serialization contracts, and the defects they found - #2895
Open
hongwei1 wants to merge 27 commits into
Open
test: endpoint sweeps and serialization contracts, and the defects they found#2895hongwei1 wants to merge 27 commits into
hongwei1 wants to merge 27 commits into
Conversation
The shipped fixtures could not be imported into a fresh database at all:
POST /obp/v2.1.0/sandbox/data-import answered 400 "Cannot import the sandbox
data" on both example_import.json files. Seven IBANs in the first and one in
the second were each attached to two accounts at two different banks, and
createAccounts rejects a duplicate IBAN.
The rejection is right. An IBAN is globally unique by ISO 13616 -- the bank
identifier is encoded inside the string, so two banks cannot hold the same one
-- and OBP depends on that rather than merely assuming it. LocalMappedConnector's
getBankAccountByRoutingLegacy refuses outright when a routing address matches
more than one account ("Routing MUST be unique"), and that is the lookup a
payment destination resolves through: BulkPaymentHandler and three v7.0.0
transaction paths all call it with bankId = None. Admitting a duplicate would
not produce a working account, it would produce one that any global-routing
payment then fails on, far from the import that caused it.
So the data was wrong, not the check. The strings were not IBANs in the first
place: 27 characters where Bosnia's format is 20, and mod-97 values of 36, 52,
50, 65, 79, 57, 45 and 0 where a valid IBAN gives 1. All sixteen are regenerated
here as structurally valid, globally unique numbers, with a distinct bank code
per bank -- which is the actual mechanism behind that uniqueness rather than a
suffix bolted on to make the strings differ.
The suites that normally cover sandbox import run against a clone of an existing
database, where the accounts already exist and the duplicate branch is never the
one that fires; only a genuinely fresh database reaches it. Hence a new test for
the direction that had none: two accounts at different banks sharing an IBAN must
be rejected, and the same two import cleanly once their IBANs differ.
Of the 850 endpoints a caller can reach, 384 are referenced by no test at all,
and of those that are tested only about a third carry an anonymous-access
scenario. Writing the rest by hand is several hundred near-identical files that
then rot one endpoint at a time; driving them off the ResourceDoc registry means
an endpoint added tomorrow is swept the day it is registered.
Three sweeps, each asserting something the others cannot see:
AuthSweepTest anonymous call is refused where the doc says it must be,
and is NOT refused where the doc says it is public; a
role-gated endpoint refuses a user holding no entitlements
FailureSweepTest a fully-entitled caller asking for something that does not
exist gets 4xx, never 5xx
SuccessSweepTest the endpoints that need no setup at all actually answer
EndpointCatalog is the single place that answers "what exists and what does each
one claim", so that SweepCoverageTest can check an exact identity: swept plus
skipped equals the catalog, with every skip carrying one of three enumerated
reasons. There is no fourth bucket for an endpoint to fall into quietly.
Three things about the source data are easy to get wrong and all three are
load-bearing, so they are written down in EndpointCatalog rather than discovered
again later. The docs live on the Http4s objects; every APIMethods*.scala is now
a stub whose Lift registrations are commented out, so reading those files for a
catalog finds nothing. "Needs authentication" is derived, not declared, and the
predicate has to be evaluated on the constructed ResourceDoc because the
constructor rewrites errorResponseBodies and several docs compute theirs from a
prop. And not every ALL_CAPS URL segment is a placeholder -- SANDBOX_TAN and
EMAIL are real literals, so substitution is driven by what the name says it is
rather than by a copy of the framework's private literal list.
Requests run in-process against Http4sApp.httpApp: no TCP, no server startup,
single-digit milliseconds each. Scenarios are one per version rather than one per
endpoint because ServerSetupWithTestData rebuilds its fixtures for every scenario
and at ~1600 assertions that cost, not the assertions, would dominate. Each
scenario collects every mismatch and fails once with the whole list.
Neither the cache's wire format nor its key derivation appears in any document
the API publishes, so a clean contract diff says nothing about either. The unit
suite is no better placed: RedisDeserializeMissTest round-trips through encode
and decode, and both run on whichever chill is on the classpath, so a format
change is invisible to it by construction -- the new encoder and the new decoder
agree with each other no matter what they agree on.
KryoGoldenCompatTest therefore reads bytes produced OUTSIDE this build.
kryo_golden_chill_0_9_3.txt holds ten values encoded by chill 0.9.3, taken from
a pre-upgrade classpath; it cannot be regenerated once that version is gone from
every tree, and regenerating it with the new chill would turn the file into a
test that chill can read itself. It is force-added past
obp-api/src/test/resources/** for that reason: the rule exists to keep generated
artefacts out, and this one is the opposite -- an input that no longer has a
generator.
What it asserts is narrow on purpose. Not that every value still decodes -- the
upgrade note accepts that some will not, and the consequence is a recompute.
What must never happen is the third outcome: decoding "successfully" into
something different, which is not a cold cache but wrong data served for a full
TTL with nothing in any log to say so. Measured here: nine of ten still decode
correctly, one recomputes, none misreads.
CacheKeyFormatTest pins the whole derived key rather than a substring of it.
The existing assertion checks that the caller's string survives INTO the key,
which passes for any prefix, separator or argument rendering; but invalidation
is pattern matching over the entire key -- NewStyle's
deleteKeysByPattern("*getMethodRoutings*") and the rate-limit patterns in
Caching -- and deleteKeysByPattern returns 0 and swallows a miss, so a broken
pattern reports nothing at all while the cache keeps serving stale routings.
The expected value is written down rather than derived, because a check that
computes its expectation the same way the code does cannot fail. If it breaks
after a library change, the fix is to re-read every deleteKeysByPattern call
site against the new shape first -- the failure IS that review being demanded.
Three of the concurrency and cache-invalidation scenarios guard themselves with assume(Redis.isRedisReady) and cancel where no Redis answers. CI has declared a redis service since the job was written, but nothing ever failed when that service was absent: a cancelled test reports as a pass, so dropping the services block, or a container that never became healthy, would have taken the rate limiter's Redis fast path and MethodRouting's cache invalidation out of the run without changing a single line of any report. Both are shared-state races, which is the class of defect a green suite is least able to rule out. RedisTestTarget turns that cancellation into a failure wherever OBP_TEST_REDIS_REQUIRED is set, which the workflows now set alongside the service they already had. Developers leave it unset and keep the skip. `required` is a parameter rather than a direct environment read so that both branches are reachable from a test -- the environment cannot be changed from inside a running JVM, and an unreachable branch in a guard is exactly what this is here to stop. Two smaller holes in the same family: The zero-test floor in run_tests_parallel.sh was 2000 against a suite its own comment called "~2900", and the real figure is 3571. A run could have lost a fifth of its tests and still passed it. Raised to 3200, and both numbers now come from a measurement rather than from memory. compile and report had no timeout-minutes while test has had one since it was written, so a hung Maven resolve blocked the build until GitHub's own six-hour ceiling rather than the job's.
…dler Extract the delegate-to-stub, route-to-connector, and metric/trace recording logic in the StarConnector InvocationHandler into named local functions. Behaviour is unchanged; this addresses SonarCloud scala:S3776 (cognitive complexity 39, limit 15) flagged on the InvocationHandler introduced when replacing CGLib's MethodInterceptor.
…nnot read another's bytes An empty List, written to Redis by chill 0.9.3 on Scala 2.12, decodes under chill 0.9.5 on 2.13 into a scala.collection.immutable.Queue. The decode SUCCEEDS. It is the call site, whose signature says List, that dies: class scala.collection.immutable.Queue cannot be cast to class scala.collection.immutable.List Measured on GET /management/dynamic-message-docs and GET /management/connector-methods: 200 on 2.12, 500 on 2.13 reading the entry 2.12 wrote, and correct in either version running alone. The 500 lasts the whole TTL, because a read that throws does not evict the key. A rolling upgrade, or any upgrade against a warm Redis, produces exactly this. The migration note anticipated stale entries and described the consequence as a cold cache. For values that fail to decode that is right. This is the case that does not fail: it returns the wrong type, and no log line anywhere says so. Prefixing the key is the fix rather than casting at the call sites. There are eight List-returning memoized methods today and the same drift can hit any other type, but more to the point, no amount of care at a call site makes bytes already in Redis readable. With the prefix, another build's entries are not addressable at all and age out on their own TTL -- which is what "cold cache after rollout" was supposed to mean. The prefix carries the Scala binary version, the axis that moved here, plus obp.cache.serialization.version for the case it does not cover: a dependency upgrade that changes the encoding while the Scala version stays put, which is what chill 0.9.3 to 0.9.5 would have been on its own. CacheSerializationNamespaceTest pins the property, not the string. Asserting "obpser1-scala2.13" would say nothing about whether isolation holds and would turn every legitimate bump into a test edit. It also asserts the reverse control -- one namespace must still read its OWN entries -- because an isolation that isolated everything would pass while disabling the cache. Verified end to end against two instances sharing one Redis: the two reproductions now answer 200, the reverse direction still works, and a second call within one version reuses its key rather than writing a new one.
IdempotencyMiddleware was mounted on one of nineteen route trees. The other eighteen carry 73 mutating payment endpoints between them -- UK Open Banking v3.1 and v4.0.1, Berlin Group v1.3 and v2, and the v4/v5.1/v6 core versions -- so a client retrying a payment on any of them repeated the payment. Tests first, because the middleware had none. IdempotencyMiddlewareTest pins the four properties a caller relies on when retrying: a same-key retry does not execute twice, the same key with a different body is refused rather than served the first response, one consumer's key cannot reach another's, and a 5xx is not cached so a retry actually retries. Wiring it up then failed end to end, and the way it failed is the reason this commit also changes the middleware. Http4sApp composes the version trees with `.orElse`, where OptionT.none means "not mine, try the next one". runRoutes called getOrElseF(404), turning a miss into an answer and terminating the chain: measured on POST /obp/v3.1.0/management/method_routings, which returned 201 without an Idempotency-Key and 404 with one. That was invisible while the middleware lived only on v7, the last link. It now returns the miss unchanged, and gives back the lock it took before running the routes -- otherwise a path this tree does not serve would hold the key for the lock's full 60s and refuse the request entitled to use it. Both new properties are pinned. Neither was reachable from the original unit tests: their inner routes are hand-built and have no fallthrough chain, so only running a real instance behind the real router could show it. Installation now has two documented requirements, both tested: inside ResourceDocMiddleware, because the body hash comes from CallContext.httpBody and without it every payload hashes alike -- which would return the first caller's receipt for a second, different payment -- and on every tree, because of the above. Verified against a live instance on three non-v7 payment paths: v3.1.0 method_routings, UK OB v4.0.1 domestic-payments and domestic-payment-consents, and Berlin Group v2 sepa-credit-transfers. Each: first call executes, replay returns the cached response with Idempotency-Replay: true and no second write, a changed body under the same key gives 409, and a call without a key is unaffected.
The sweep found ten endpoints whose ResourceDoc disagreed with their behaviour. They are not one defect; they are four, and only six of the ten needed a product change. The other four were the sweep being wrong. Six docs corrected: getAllProductsV600, getAllApiProductsV600 -- the description still interpolated userAuthenticationMessage(!getProductsIsPublic), so with the prop true it published "authentication optional" while the route called withUser unconditionally. The route is deliberate: its own comment reads "(all banks; auth-required; cached)", and the api-products bucket records why -- "the v6 Lift conditional public-access path (getApiProductsIsPublic) is simplified -- public gating would be a Phase 3 follow-up if needed". So the doc is what was out of step. Restoring the conditional route would have been implementing that follow-up, which is a product decision and makes two endpoints public by default. getConfigProps, getConnectorTraces -- hand-written "Authentication is Required." The constructor matches userAuthenticationMessage(true) verbatim and this is not it, so nothing reached errorResponseBodies and both published as public. getMyApiCollectionEndpoint, getApiCollectionEndpoints -- these contradicted themselves: errorResponseBodies listed $AuthenticatedUserIsRequired while the description said userAuthenticationMessage(false). The description wins -- the constructor's second branch REMOVES the error body when the text says optional -- so an explicit declaration was being deleted by a text match. Four needed no product change, and AuthSweepTest is corrected instead: createConsentRequest, getConsentRequest and createVRPConsentRequest require an APPLICATION, not a user. All three answer OBP-20200 "The application cannot be identified", and createVRPConsentRequest says so in its own prose: "Client, Consumer or Application Authentication is mandatory for this endpoint". userAuthenticationMessage(false) is accurate. EndpointCatalog.needsAuthentication reproduces the middleware's predicate, which reads only errorResponseBodies and roles -- both about the user -- so it classified them public and then failed them for behaving as documented. A 401 now only fails the check when it is the user one; an application 401 is reported as an observation, named rather than swallowed, since resource-docs cannot express "needs an application" without authMode. verifyRequestSignResponse refuses with OBP-20311 "The Request is not signed" -- a third mechanism, which authMode cannot model either. Left failing on purpose. AuthSweepTest: 9 failures before, 2 after. The two that remain are verifyRequestSignResponse and createTransactionRequestFreeForm answering 500 where 403 was due, both already catalogued.
KryoGoldenCompatTest could not have caught the defect it was written to catch, for two independent reasons, and both were found the hard way -- by the defect reaching a running instance. Its ten golden values were Java collections. What OBP-API memoizes is Scala collections; java.util.ArrayList appears in none of the providers. And it compared with ==, under which an empty Scala List EQUALS an empty Queue -- both are Seq, and Seq equality is element-wise. So an empty List written by chill 0.9.3, decoding under 0.9.5 as a scala.collection.immutable.Queue, passed a test whose whole purpose was to notice exactly that, while every call site declaring List failed with ClassCastException. Adds kryo_scala_golden_chill_0_9_3.tsv: eleven Scala values encoded on the pre-migration 2.12 classpath, each recorded with the runtime class it was written as. That third column is the point -- the assertion is now on the class, because the class is what a call site depends on. Force-added past .gitignore for the same reason as the first fixture: it cannot be regenerated once every checkout carries 0.9.5. Subclassing is not drift. A Vector read back as Vector1 is assignable to every signature that named Vector and nothing can tell, so the check is isInstance rather than name equality. The Nil-to-Queue drift is recorded in a knownDrift baseline rather than left red. It is a property of two third-party libraries, not something this branch changes, and the mitigation is elsewhere and tested: Redis.serializationNamespace means a 2.13 instance cannot address a 2.12 entry at all. A permanently red suite is one people learn to ignore; a baseline entry costs a written reason. Anything not listed still fails, and a listed name that stops drifting also fails -- a baseline that outlives its hazard reads as still true to whoever comes next. Run: java 9/10 decode correctly, scala 9/11 decode into an assignable class, scala-list-empty and scala-option-none cold on rollout.
…se its skips Two ways this suite reported green over things it was not checking. It held a character-for-character copy of the expression that builds the dependency-whitelist source, rather than calling it. The two only stayed in step because whoever edited one happened to see the other -- and this is the one compile that happens reflectively at boot, so a divergence would not have failed at compile time either. The expression is now DynamicUtil.Validation.dependenciesScalaCode, named so both callers can reach it, and the test calls that. Three sandbox scenarios have been cancelling on every run, everywhere, since the build moved to a JDK past 17: SecurityManager enforcement was removed by JEP 411 and finished off by JEP 486, so DynamicUtil.Sandbox is a no-op and `assume` cancels. A cancelled check reads as a passing one in the summary line people actually look at, which is how "canceled 0" and three unrun scenarios coexisted. What goes unchecked is not incidental -- the sandbox is the only thing covering what runtime-compiled endpoint code may touch. OBP_TEST_SANDBOX_REQUIRED=true turns the cancellation into a failure, the same lever RedisTestTarget gives the Redis-dependent checks. Verified both ways: unset gives succeeded 6 / canceled 3, set gives succeeded 6 / failed 3 with a message naming what cannot run and why. The skip still stands by default, because no JDK on this build can enforce. The difference is that it is now somebody's decision rather than a silence.
A 5xx tells a caller the server broke and the request is worth retrying. Each of
these is a client-side condition, so the retry can never succeed -- and one of them
sits on a payment path, where that loop is the expensive kind.
getUserInvitation secretLink.toLong threw NumberFormatException on
any non-numeric segment -> 400 (InvalidNumber,
which already existed and says exactly this)
createConsumerDynamicRegistration verifyJwt does not return false for a missing or
unparseable PSD2-CERT, it THROWS ("No PEM-encoded
keys found"), and booleanToFuture only guards the
false case -> 400
getSignalChannelInfo a plain RuntimeException for "no such channel"
-> 404, with a new SignalChannelNotFound
(OBP-39021, continuing the Signal series)
createTransactionRequest four raw throws -- not authenticated, no such
bank, no such account, no such view -> 401/404
createTestEmail two "server is not configured" checks answered
500. The server is not broken, it is unconfigured,
and neither resolves without an operator editing
props -> 503
getConnectorMethodNames is different in kind: it is a regression this branch's own
base introduced, and it is fixed at the source rather than at the endpoint.
java.lang.reflect.Proxy passes null for a method declaring no parameters; cglib,
which the ByteBuddy swap replaced, passed a zero-length array. Everything downstream
treats args as a collection -- `.zip(args)`, `args.collectFirst`,
`extractKeyParams(args)`. isInheritedMember covers members Connector does not
declare, but a NO-ARGUMENT method that Connector DOES declare slips past it and
lands in routeToConnector. `callableMethods` is exactly that.
Confirmed by running both builds with the same grants against the same request:
GET /obp/v6.0.0/system/connector-method-names answers 200 on the 2.12/cglib build
and 500 on this one, with `Cannot invoke "scala.collection.IterableOnce.knownSize()"
because "that" is null` -- which is `zip` being handed the null. Normalising args at
the handler entry restores what cglib did, which is what a toolchain migration owes
its callers, and closes the whole family rather than this one endpoint.
Sweep after these: 57 tests, 0 failures, 0 suites aborted (was 47/9).
… deviations
Three of the sweep's findings turned out to be the sweep's own defects, and they are
the same defect twice over: a rule copied instead of called.
SuccessSweepTest kept its own placeholder rule, and it had drifted from
EndpointCatalog's -- the catalog substitutes any segment ending in ID, _CODE or
_NAME, this one knew _ID and _CODE and had never learnt _NAME. So
/signal/channels/CHANNEL_NAME/info was a placeholder to the catalog, which duly
replaced it with a channel that does not exist, and NOT a placeholder here, so this
suite selected it as an endpoint needing nothing created first and then failed it
for answering 404. The first half of its condition was vacuous as well:
`concretePath(doc) == concretePath(doc, Map.empty)` compares a default argument with
the same value passed explicitly. Now calls EndpointCatalog.hasPlaceholder.
EndpointCatalog resolved VIEW_ID but not GRANT_VIEW_ID, so an endpoint that looks up
the view before it checks roles answered on the view and the role gate never ran.
That is the same reason a real bank id is already passed for the role assertion.
createTransactionRequestFreeForm was reported as "expected 403, got 500" -- two
defects stacked, the endpoint's raw throw and this placeholder never resolving.
Two deviations remain and are recorded rather than left red, with the reason each is
not a defect:
verifyRequestSignResponse refuses with OBP-20311, JWS request signing --
a third mechanism that authMode cannot express,
so neither the doc nor this sweep can declare it
createTransactionRequestFreeForm answers 400 rather than 403; the endpoint
deliberately delegates the decision to the
connector and says so in its own comment, and an
existing test depends on it. Whether an
authorisation failure should be 400 at all is a
product question
The exemption list expires on its own: an entry whose endpoint stops deviating fails
the run, and so does one naming an operationId no longer in the catalog. A baseline
that outlives its hazard reads as still true to whoever comes next.
One process note, because it nearly went the other way. The stale-entry scenario was
first written inside the per-version loop, so it registered once per API version and
the duplicate name aborted the whole suite at construction. Maven reported
"Tests: succeeded 31, failed 0" and BUILD SUCCESS over a suite that ran nothing --
the same shape as the cancelled-check and the zero-test cases already fixed on this
branch. Reading `Suites: completed N, aborted 0` alongside the test counts is what
caught it.
…attribute The audit line read `skipped="N"` off <testsuite>. ScalaTest's JUnit reporter does not emit that attribute -- it puts a <skipped/> child inside each cancelled <testcase> -- so the counter was structurally always zero. A run in which an entire suite cancelled every one of its tests would still have printed "0 skipped/canceled", and that is the number somebody checks precisely when they suspect tests are not running. Measured on the run that prompted this: the audit said 0, the reports held 15 across eight suites. Twelve were news. NginxForwarderTest (4), Http4sServerIntegrationTest, EmbeddedRabbitMQ, two v6 integration suites, RootAndBanksTest (2), BankAccountCreationListenerTest (2) -- integration and infrastructure tests, the category where a silent skip costs the most. Only DynamicUtilTest's three were already known, and only because somebody had looked at them by hand. Counts the child elements, keeping the attribute path first for reporters that do emit it. Why each one skips, and whether it should, needs environment decisions and is not part of this change. Fourth instance of one family on this branch: a cancelled check reporting as a pass, a zero-test run reporting as a pass, an aborted suite reporting BUILD SUCCESS, and now a skip counter that could not count. Each sat in the layer whose job is to notice that tests did not run.
CacheKeyFromArguments renders every parameter that is not annotated
@CacheKeyOmit. CallContext carries per-request state (startTime,
correlationId, url, verb, ipAddress, user), so both keys were unique per
request: the cache could never hit, and getCurrentFxRateCached wrote a fresh
Redis entry per call that lived out its TTL.
getEndpointMappings additionally cached the (mappings, callContext) tuple.
chill/Kryo cannot encode the lambda reachable through
CallContext.resourceDocument, so every write failed and cachePut swallowed it
as "result served uncached" - endpointMapping.cache.ttl.seconds bought nothing
but a WARN per call. A hit would also have handed the caller the originating
request's CallContext.
Split the memoized half into getEndpointMappingsCached(bankId) rather than
annotating callContext on the caller: CacheKeyFromArguments reads the
parameters of the method whose body ends in buildCacheKey, so binding the
result to a val first leaves it with no parameters and it emits
Nil.mkString("_") - an empty argument segment, i.e. every bankId sharing one
entry. Verified with javap that the key now renders bankId :: Nil, and that
getCurrentFxRateCached renders bankId :: from :: to :: Nil.
Add invalidateEndpointMappingCache() on create/update/delete, mirroring
invalidateMethodRoutingCache: while callContext was in the key nothing could
hit, so a stale entry was unreachable by construction; now that the cache
works, writes have to publish themselves.
The scope key was derived from the consumer id alone, with no method, path, or resolved-operation component. Reusing one Idempotency-Key across two different endpoints (same or empty request body on both, e.g. two DELETEs) made the second call replay the first's cached response instead of executing: the caller was told an operation succeeded that never ran. This risk existed narrowly within v7.0.0 before the middleware was wired onto every version tree; wiring it onto all ~17 trees widened it to the entire API surface, and the middleware's own test only covered consumer-scoping, not endpoint-scoping. Fold the resolved operation id into the scope hash. operationId is set by ResourceDocMiddleware once it matches a ResourceDoc, and is stable across path-template placeholders and bridge-cascade path rewrites, unlike the raw request path. A tree that finds no ResourceDoc match falls back to method+path; that fallback only ever feeds a lock-then-release cycle a miss discards, so it does not need to be canonical, only present. Added a regression test reproducing the collision (two different endpoints, same consumer, same key, empty bodies on both sides) and updated the in-flight test's manually-planted lock key, whose comment already said "same derivation the middleware uses" -- it now is.
createTransactionRequest wrapped the whole view lookup (Views.views.vend .systemView(...).or(.customView(...))) inside tryons(..., 404, ...), which catches any Exception the block raises and reports it via the given failCode regardless of cause. A connection-pool exhaustion, a transient SQL error, or a Mapper bug during that lookup was therefore indistinguishable from a genuine "no such view" -- both produced the same 404, telling a client with retry logic to stop retrying a payment request that would in fact succeed once the backend recovered. Split the lookup out of the exception-swallowing block: only a lookup that completes successfully and returns an empty Box is a genuine client-side not-found and maps to 404; anything the lookup itself throws now propagates untouched, so it falls through to ErrorResponseConverter's catch-all (500) like any other unexpected server-side failure. Extracted into resolveCreateTransactionRequestView so the distinction is unit-testable without a live Mapper connection.
createConsumerDynamicRegistration wrapped the whole JwtUtil.verifyJwt call in tryons(PostJsonIsNotSigned, 400, ...), which catches any Exception the block raises regardless of cause. verifyJwt goes through Nimbus JOSE (JWK.parseFromPEMEncodedObjects, SignedJWT.parse, RSASSAVerifier), and a missing signature algorithm in the JVM's registered security providers (a hardened/FIPS JRE, a stripped provider list, a provider-registration bug) surfaces there as a JOSEException wrapping NoSuchAlgorithmException -- caught by the same blanket tryons and reported to the caller as "your JSON is not signed" (400), even though nothing about their certificate or JWT is wrong and every other caller would fail identically until an operator fixes the JVM. Walk the exception's cause chain for NoSuchAlgorithmException/ NoSuchProviderException before deciding the status code: that shape propagates untouched (500, via ErrorResponseConverter's catch-all), while everything else -- a malformed PEM, an unparseable JWT, an actual signature mismatch -- still maps to 400 as before. Extracted into resolveJwtSignatureValid so the distinction is unit-testable without live PEM/JWT material.
resetPasswordUrl reported a missing public_obp_portal_url/portal_external_url
as 400 -- a bare Future.failed(new Exception(s"$IncompleteServerConfiguration
...")) whose message starts with "OBP-10056: ", which
ErrorResponseConverter's OBP-prefix path promotes only to
{401,403,408,429} and defaults everything else to 400. An admin resetting a
user's password was told their request was bad when the actual problem is
an operator who hasn't configured the portal URL yet.
This is the identical condition Http4s700's createTestEmail already reports
as 503 in this same codebase, with the reasoning that a 500 (or, worse, a
400) tells a caller with retry logic the fault is transient or their own,
when neither resolves without an operator editing props. Route the same
message through tryons with an explicit 503 instead of a bare exception, so
it bypasses the 400 default entirely.
Extracted into resolveResetPasswordPortalUrl so the distinction is
unit-testable without touching Props.
SweepCoverageTest's "the failure sweep covers the same set the auth sweep does" scenario computed authScope and failureScope from the exact same literal expression, typed out twice (catalog.filter(EndpointCatalog.skipReason(_).isEmpty).map(_.operationId).toSet). Two independent copies of one expression are equal by construction: the scenario could never fail, even after a real future divergence where one sweep grows a filter of its own and the endpoints that fall between the two are covered by neither -- silently, forever, because the guard was comparing two copies of itself rather than each sweep's actual coverage. Expose each sweep's own definition of what it covers as AuthSweepTest.scope / FailureSweepTest.scope, and have both the sweep's own byVersion grouping and SweepCoverageTest's comparison read that single definition. A change to either sweep's filtering is now automatically reflected on both sides of the comparison instead of needing to be kept in sync by hand. SweepCoverageDriftCheckTest scans SweepCoverageTest.scala's source rather than asserting at runtime: a value-equality check on the current catalog cannot distinguish "read from the real source" from "coincidentally equal duplicate" -- both produce the identical Set today, and the difference only matters for whether a FUTURE divergence gets caught, which no runtime assertion against today's catalog can exercise.
operationId is set on the CallContext only when ResourceDocMiddleware's matcher finds a ResourceDoc for the current version tree; every other tree in the .orElse fallthrough chain sees it absent. Since this middleware is installed on every tree, a miss still acquired and released a lock keyed on a method+path fallback before falling through -- and two requests racing that same miss-tier lock (two genuinely concurrent copies of a request destined for a later tier, or two different endpoints whose fallback happened to collide) could have the loser answered a definite 409 by a tier that was never going to serve either of them, before the request ever reached its real destination. Gate the lock/response-key path on operationId being present. A miss-tier now passes straight through to the wrapped routes with no Redis round trip at all, removing both the false-conflict window and the redundant lookups on every tier a mutating request passes through before reaching the one that serves it. Key-format validation stays unconditional, since a malformed header is a client error regardless of which tier eventually resolves the path.
…ep placeholders
isPlaceholder's ends-with-ID/_CODE/_NAME heuristic left these three
ALL_CAPS segments untouched, treating them as literals. All three are
enumerated values a live endpoint validates inline, not literals:
Http4sBGv2PIS's payment-service branches guard on
Set("payments","bulk-payments","periodic-payments").contains(paymentService),
and the auth-context-updates/consent SCA branches guard on
List(SMS, EMAIL[, IMPLICIT]).contains(scaMethod). Sent verbatim as
"PAYMENT_SERVICE"/"SCA_METHOD", both guards fail and the sweep reports
the resulting 404/400 as the endpoint's own defect -- reproducing today
for SCA_METHOD via OBPv5.0.0-createUserAuthContextUpdateRequest.
Add all three to isPlaceholder and give each a real accepted value in
concretePath's defaultValue, the same way VIEW_ID resolves to "owner"
so an endpoint's real logic runs instead of failing at an entity check
before the assertion under test ever gets exercised.
Now that CallContext is out of getEndpointMappingsCached's key, the memoized entry can actually be hit -- which means the single delete in invalidateEndpointMappingCache leaves a real window: a reader that fetched the pre-write value from the provider a moment earlier can still finish its own cache write after the delete completes, silently reintroducing the stale entry for the rest of endpointMapping.cache.ttl.seconds with nothing left to clear it before the next write. Schedule a second delete shortly after the first, delay configurable via endpointMapping.cache.invalidation.delay.ms (default 500ms). Any straggler write that lands in the gap gets cleared moments later instead of surviving the full TTL.
… checks
The floor comparison reads "${SF_TOTAL:-0}" -lt 3200, but the message
printed on failure still said "(< 2000 floor)" -- left over from before
the threshold was raised. A run producing e.g. 2500 tests is correctly
failed by the check, but the printed diagnostic reads as
self-contradictory (2500 is not less than 2000) to whoever is reading
the CI log to find the cause.
Add a source-scan test pinning that the two numbers match, the same
drift-guard shape SweepCoverageDriftCheckTest already uses for a Scala
file, so a future threshold change can't silently leave the message
behind again.
FailureSweepTest.omniscientUser and SuccessSweepTest.entitledCaller were byte-for-byte identical bodies (grant every ApiRole to resourceUser1, build a DirectLogin header), and AuthSweepTest, FailureSweepTest and SuccessSweepTest each looked up LocalMappedConnector.getBanksLegacy(None) independently. None of it lived in EndpointCatalog, the module this package already treats as the one place shared sweep logic belongs. Add SweepFixtures with realBankId and omniscientCaller, mixed into all three test classes; AuthSweepTest's realEntities now calls the shared realBankId instead of repeating the lookup. A source-scan test pins each construction to exactly one occurrence across the package, the same drift-guard shape SweepCoverageDriftCheckTest already uses.
SonarCloud flagged "code.example.Provider.getAll(Some(bank))" (3x) and "obpser1-scala2.13" (4x) as duplicated string literals. Named as SampleCallerKey and CurrentNamespace; no behavior change.
SonarCloud flagged "Idempotency-Key" (4x), "Bearer c1" (15x) and "/not-served-by-this-tree" (3x) as duplicated string literals. Named as IdempotencyKeyHeaderName, Consumer1Auth and UnservedPath; the three uri"..." literal constructions became Uri.unsafeFromString(UnservedPath) so they could reference the constant. No behavior change.
SonarCloud flagged "a check" as duplicated 6 times. Named as CheckLabel; no behavior change.
|
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
1 participant
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.
Regression coverage the 2.13 migration needed, plus the defects that coverage found.
The branch started as four commits that had nowhere to live after #2890 merged. Running
them turned up real regressions, so it now carries both the tests and the fixes.
What this fixes
A rolling upgrade returns 500 for a full TTL. An empty
List, written to Redis bychill 0.9.3 on 2.12, decodes under 0.9.5 as a
scala.collection.immutable.Queue. Thedecode succeeds; the call site, whose signature says
List, throwsClassCastException. Measured on
GET /management/dynamic-message-docsandGET /management/connector-methods: 200 on 2.12, 500 on 2.13 reading 2.12's entry,correct in either version alone. A failed read does not evict the key, so it lasts the
whole TTL with nothing in any log. Fixed by namespacing the cache key with the Scala
binary version, so one build cannot address another's entries.
73 mutating payment endpoints had no idempotency.
IdempotencyMiddlewarewas mountedon one of nineteen route trees. The other eighteen carry UK Open Banking v3.1/v4.0.1,
Berlin Group v1.3/v2 and the v4/v5.1/v6 core versions, so a client retrying a payment
repeated it. Wiring it up also required fixing the middleware: it turned a route miss
into a 404, which terminated the
.orElsechain that composes the versions.GET /system/connector-method-namesregressed with the cglib→ByteBuddy swap.java.lang.reflect.Proxypassesnullfor a no-argument method where cglib passed anempty array, and a no-arg method that
Connectoritself declares slips pastisInheritedMember. Confirmed by running both builds against the same request: 200 on2.12, 500 here. Normalising at the handler entry restores cglib's contract and closes the
whole family, not just this endpoint.
Six client-side conditions reported as server faults, one of them on a payment path.
A 5xx tells a caller to retry a request that can never succeed.
Six endpoints published the wrong authentication requirement. Two still read a props
value the route had stopped honouring; two hand-wrote a sentence the ResourceDoc
constructor matches verbatim; two contradicted themselves, listing
$AuthenticatedUserIsRequiredinerrorResponseBodieswhile the description saidauthentication was optional — the description wins, and silently deleted the declaration.
Plus a sandbox fixture whose 16 IBANs were neither valid (27 characters, mod-97 never 1)
nor unique, which made the import fail on any fresh deployment.
What this adds
Sweeps driven off the ResourceDoc registry rather than hand-written per endpoint, so an
endpoint added tomorrow is covered the day it is registered: anonymous access, role
gating, crash-freedom, and a coverage identity that fails if anything leaves the swept
set without a stated reason.
Golden fixtures for the two axes a contract diff cannot see. Both were encoded on the
pre-migration classpath and cannot be regenerated once every checkout carries the new
chill. The Scala fixture records the runtime class each value was written as, because
comparing values is not enough —
List() == Queue()is true, which is exactly how thedefect above would have passed a test written to catch it.
Known deviations, and why they are not failures
Two endpoints deviate deliberately and are listed with reasons rather than left red:
verifyRequestSignResponserefuses withOBP-20311(JWS request signing, a thirdmechanism
authModecannot express), andcreateTransactionRequestFreeFormanswers 400rather than 403 because it delegates the decision to the connector, as its own comment
says. Both lists — this one and the Kryo drift baseline — fail if an entry stops applying
or names something no longer in the catalog. A baseline that outlives its hazard reads as
still true.
Verification
Full suite locally: 3604 tests, 0 failures, 0 errors, all shards passed.
CI on
dcd5b329a: all 9 shards, compile, report and docker green.Endpoint sweep: 57 passing, 0 failing, 0 suites aborted (47/9 when the fixes started).
Each fix was checked against a running instance rather than compilation alone. That
mattered: the idempotency wiring compiled and passed 11 unit tests while still being
broken, because the unit tests' inner routes have no fallthrough chain to break.
Four separate false greens were found and fixed along the way, all in the layer whose job
is to notice that tests did not run: a cancelled check reporting as a pass, a zero-test
run reporting as a pass, an aborted suite reporting BUILD SUCCESS, and a skip counter
reading an attribute ScalaTest does not emit. The last one revealed 15 tests skipping
silently on every run, twelve of which nobody knew about.