Add Java language support to DynamicResourceDoc - #96
Open
hongwei1 wants to merge 65 commits into
Open
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.
…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.
…urce docs DynamicResourceDocJavaTest's role-gated scenario duplicated the same 17-line 401/403/200 assertion block already in DynamicResourceDocTest, which SonarCloud's new-code duplication gate flagged. Both now share V400ServerSetup.assertRoleGated401Then403Then200.
…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.
Kept in sync with frozen_type_meta_data (regenerated by the previous commit) via code.util.FrozenMetaDataText -- FrozenMetaDataTextTest fails when the two disagree.
…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.
Two bugs made createJavaHttp4sEndpoint's validation a no-op in practice, discovered by manually enabling dynamic_code_compile_validate_enable against a real Java method_body that calls a non-whitelisted OBP method: - Javassist's LoaderClassPath reads a class's bytecode via classLoader.getResourceAsStream(...), but the compiler's ch.obermuhlner.scriptengine.java.MemoryClassLoader only overrides loadClass() and never exposes compiled bytes as a classpath resource. getDynamicCodeDependentMethods silently found nothing to check no matter what the Java code called. Fix: read the bytes from the classloader's private byte map and register them with Javassist directly via ByteArrayClassPath. - Once dependency lookup worked, a rejection was still swallowed: JsonResponseException never sets a Throwable message, so wrapping the validation call in Box.tryo turned it into Failure(null, Full(theException), Empty), and DynamicEndpoints.scala's `case Failure(msg: String, ...)` pattern fails to match a null msg -- silently falling through to a generic "compiled code return nothing" error and discarding the real rejection reason. Fix: run validation outside any Box.tryo, mirroring how the Scala path's CompiledObjects.validateDependency() (also never tryo-wrapped) already lets the exception propagate uncaught. Manually verified end-to-end: a Java method_body calling code.api.util.APIUtil.getPropsValue (not on the dependency whitelist) is now rejected with 400 OBP-40046, naming the exact forbidden call.
…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.
DynamicUtil.Validation's dependency whitelist and Constant.SHOW_USED_CONNECTOR_METHODS were final vals, computed once when first touched (in practice during server boot, before any test scenario runs). setPropsValues -- the standard per-scenario props override used throughout this test suite -- mutates Props.lockedProviders, which only reaches code that re-reads Props on each call; it cannot un-freeze an already-computed field. That made this security path untestable without a real server restart, which is how the earlier manual verification (see prior commit) had to prove it. Changed the whole SHOW_USED_CONNECTOR_METHODS / Validation dependency chain from val to def so it re-evaluates from live props on each call. This costs nothing extra in production: the one expensive step, DynamicUtil.compileScalaCodeUnchecked, is already memoized by the exact source string, so re-evaluating is a cache hit unless the underlying props value actually changed. DynamicResourceDocJavaSecurityValidationTest exercises the resulting create-time HTTP path directly: a Java method_body calling code.api.util.APIUtil.getPropsValue (not on the dependency whitelist) is rejected with 400 OBP-40046, naming the exact forbidden call -- the same assertion the manual verification made by hand.
"".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.
any grant targeting a consent user redirects to its granting human with
a warn-log; sole exemption is createdByProcess == consent_user.
ConsentUtil.addEntitlements tags its writes accordingly.
- process column retired: parameter and accessor removed from the
Entilement trait and implementation
group queries now key on group_id; GroupEntitlementJsonV600 exposes
created_by_process instead of process.
- Explicit-target guards (400, "…names a consent user…"):
addEntitlement v2.0.0 + v7.0.0; addUserToGroup v6;
grantUserAccessToViewById v5.1; createAccountAccessRequest v6
(reject at creation) plus repeated check at its approval;
createAccount endpoints v2.0.0, v3.1.0, v4.0.0 (regular +
settlement), v5.0.0, v7.0.0.
- Implicit-target resolution to the accountable user (currently a
human): bank-creator grants (v2.2, v5, v6, v7 incl. the
generated-bank endpoint), v6
dynamic-entity creator roles, v3.0.0 entitlement-request requester,
createAccount owner fallbacks, and the connector-internal
HOLDING-account holder.
- Rename: effectiveHumanUserId → accountableUserId
createJavaHttp4sEndpoint memoized the whole Box[Http4sEndpointIO] -- including whatever Validation.validateDependency decided -- keyed only by the exact method_body string. Once dynamic_code_compile_validate_enable and the whitelist became live-reloadable (previous commit), this became a bypass: compile a Java source while validation is off, then turn validation on and tighten the whitelist, then resubmit the identical source in a new create/update call -- the cached Full(...) from the first, unvalidated compile is returned directly, and validateDependency never runs a second time. Split the memoization so only the actual javax.tools.JavaCompiler invocation is cached (deterministic given the source, and the one genuinely expensive step); dependency validation now runs on every call, unmemoized, exactly like the Scala path's CompiledObjects.validateDependency() already does. Also corrected an overclaiming comment: the val-to-def change makes Validation.allowedRuntimePermissions itself always current, but Sandbox.sandbox(bankId) separately caches the whole Sandbox it builds per bankId, so a sandbox built before a permissions-prop change keeps the old snapshot. Left that cache alone -- SecurityManager enforcement is already a no-op on this JDK (JEP 486), so neither the stale nor the fresh permission list is actually enforced either way. DynamicResourceDocJavaSecurityValidationTest gains a regression scenario reproducing the exact bypass sequence: compile the malicious source once with validation off, enable strict validation, resubmit the identical source under a new doc, and assert it is still rejected.
…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.
enableStrictValidation() wrapped setPropsValues with an expression body (def ... = setPropsValues(...)), which .github/scripts/ check_test_isolation.py's brace-based scanner does not recognize as a safe "helper" scope -- it only classifies a def as safe when it finds an opening brace immediately following the def name. The call was flagged as running at class-instantiation time and failed CI's lint step before compilation even started. Verified locally with python3 .github/scripts/check_test_isolation.py.
…r_methods getDynamicCodeDependentMethods and APIUtil.getDependentMethods were both gated by SHOW_USED_CONNECTOR_METHODS, an unrelated introspection/reporting toggle that defaults to false. This meant dynamic_code_compile_validate_enable alone did nothing: with the reporting prop left at its default, the bytecode scan always returned an empty dependency list, so every dynamic-code call passed the whitelist check regardless of what it actually invoked. Add a force parameter that Validation.validateDependency sets to true, making security validation controlled solely by dynamic_code_compile_validate_enable as documented. The existing caller in DynamicCompileEndpoint keeps its prior behaviour unchanged.
createJavaHttp4sEndpoint re-registered the compiled class's bytes into the shared Javassist ClassPool on every call, including compile-cache hits. ClassPool.appendClassPath has no dedup, so a long-running process serving the same dynamic endpoint repeatedly (e.g. on each resourceDocs-list rebuild) grew the ClassPool's classpath chain without bound. Move the registration inside the compile memoization block so it runs exactly once per distinct source string, tied to the same cache lifetime as the compiled class itself.
…lumn
getJsonDynamicResourceDoc passed Lang.get straight through as a
constructor argument. Rows created before the Lang column existed have a
genuine SQL NULL there, and an explicit null argument bypasses
JsonDynamicResourceDoc's own "Scala" default -- that default only applies
when the argument is omitted. Legacy rows therefore reported
programming_lang as null/empty instead of the documented default.
Fall back explicitly with Option(...).filter(isNotBlank).getOrElse("Scala"),
matching the pattern already used for the other nullable text fields in
the same method. Added a regression test that forces a genuine NULL via
raw SQL (bypassing the ORM, which always writes "Scala") and asserts GET
still reports "Scala".
…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.
… dynamic resource doc Two compounding bugs, both required to reproduce and both required to fix, found by running the feature against a real packaged jar and a real Postgres database instead of the in-process test harness: 1. Every Java method_body implements Supplier<Function<Object[], Object>> per convention. javac always erases that generic Supplier.get() to a synthetic bridge method (Object get()) whose body just invokevirtual-calls the real, properly-typed get() -- an ordinary same-class call. The dependency scanner's same-class exemption only recognised a Scala-specific mangled-name convention, so this call was treated as a dependency on a forbidden method: the dynamically-compiled class lives under the OBP-owned code.* package, but its randomly-generated name can never appear in a static whitelist. 2. CompiledObjects.validateDependency() re-validated this.partialFunction a second time after DynamicUtil.createJavaHttp4sEndpoint had already validated the real compiled Java class internally. For Java, this.partialFunction is OBP's own Http4sEndpointIO wrapper, not user code, and its bytecode legitimately calls internal OBP helpers (javaValueToJValue, logger, CustomJsonFormats.formats, compactRender) that were never meant to be whitelisted. Together these meant dynamic_code_compile_validate_enable=true rejected every Java doc unconditionally, benign or not -- invisible to the existing test suite because no scenario combined a genuinely benign Java body with strict validation turned on. Fix getDynamicCodeDependentMethods to recurse through any same-class self-call rather than treat it as a leaf dependency, and make CompiledObjects.validateDependency() a no-op for the Java language (the real validation already happened inside createJavaHttp4sEndpoint).
auth_type metrics column etc.
…alias-resource-doc-registry refactor: single source of truth for resource-doc registry (closes BG v1.3 alias gap)
…ourcedoc-java-support
…weep-and-cache-contracts test: endpoint sweeps and serialization contracts, and the defects they found
…ourcedoc-java-support # Conflicts: # obp-api/src/main/scala/code/api/util/DynamicUtil.scala
…scan The Supplier.get() bridge-method exemption added for Java dynamic resource docs only unrolled one level: it appended a same-class method's own callees as raw dependency tuples instead of recursing into them. A Java method_body that factors logic into its own private helper methods (fully ordinary Java) reintroduces the exact same false rejection one level deeper -- the un-recursed helper's callees get flagged as calls to the compiled class's own randomly-generated, unwhitelistable name. Replace the one-level unroll with a proper recursive expansion that follows same-class calls to arbitrary depth until a genuinely foreign dependency is reached, with a visited-set cycle guard: direct or mutual recursion between same-class private methods (e.g. a factorial/fibonacci helper) is ordinary Java and must not recurse forever, and a cycle correctly contributes nothing further rather than falling back to a leaf that gets rejected the same way. Applies uniformly to both the new Java same-class case and the pre-existing Scala nested-closure case, which had the identical one-level limitation.
POST /obp/v6.0.0/management/dynamic-resource-docs/validate re-implemented the request_verb and example_request_body preconditions the v4 create endpoint checks, but not the programming_lang check added alongside Java support. CompiledObjects falls through to the Scala compile path for any value it doesn't recognise as "java"/"Java", so validate would report valid=true for a body that happens to compile as Scala under a bogus or misspelled programming_lang -- a verdict that contradicts what the create endpoint would actually do (400 DynamicCodeLangNotSupport), defeating the point of a dry-run validation endpoint. Add the same language check, mirroring Http4s400's validateDynamicResourceDocBody. No prior test exercised this endpoint at all; added coverage for the rejection and for both supported languages.
resourceDocs' per-row catch swallows any exception from toResourceDoc and logs it as "likely stored under the deprecated Lift contract -- re-author the body against the new native contract", regardless of cause. That is accurate for a genuine compile failure, but CompiledObjects.validateDependency now runs fresh on every construction (not just at create/update time), so a previously-registered doc can start throwing JsonResponseException here too -- purely because dynamic_code_compile_validate_dependencies was tightened after it was registered. The endpoint silently drops from the listing either way, but the log sends whoever reads it toward rewriting a body that was never the problem. Catch JsonResponseException separately and report the actual whitelist rejection reason (via the same JsonResponseExtractor pattern Http4s600's validate endpoint already uses for this exception shape), falling back to the existing generic message for every other cause. No dedicated test: this only changes log wording for a rejection path already covered end-to-end by DynamicResourceDocJavaSecurityValidationTest; asserting on log content here would need capture infrastructure this codebase doesn't otherwise use for a diagnostics-only change.
allowedCompilationMethods was changed from val to def so it observes a live props override (needed for setPropsValues-driven tests), but validateDependency's collect guard reads it twice per dependency tuple -- Map.get and a separate .exists scan. Each read re-derives the whitelist from scratch: a prop read, a regex-based rewrite of the whitelist source into compilable Scala, and a ConcurrentHashMap lookup keyed on that ~1.5KB string. For N dependency tuples that is up to 2N re-derivations where one would do. Bind it to a val at the top of the method instead. Still observes props changes between calls (each validateDependency invocation re-binds), just not multiple times within the same call.
createJavaHttp4sEndpoint reflectively reads the compiled class's bytes out of MemoryClassLoader's private mapClassBytes map to hand them to Javassist for dependency validation. The lookup result was passed straight to ByteArrayClassPath unchecked: if a future java-scriptengine version keys that map differently (internal name vs binary name, or drops the field entirely), get() returns null, and the failure only surfaces later as an opaque NPE inside Javassist -- nothing at the actual point of breakage indicates the cause was this reflective coupling to an internal field. Check for null and throw immediately with a message that names the field and the class it was looked up for. The throw happens inside the same Box tryo the rest of the compile step already runs in, so it is reported the same way any other compile failure is -- no behavior change on the success path. Also documents (no functional change) the pre-existing, unbounded-by-design memoClassPool/dynamicCompileResult caching tradeoff that the Java compile path now shares: each distinct compiled method_body retains its ClassLoader and ClassPool for the life of the process. This is consistent with the existing Scala compile cache's design (dynamic resource doc creation is gated behind operator-only roles, not open to arbitrary callers) rather than a new gap introduced here.
|
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
method_bodyfor a DynamicResourceDoc endpoint can now be Scala (default, unchanged) or Java, mirroring the existing precedent used by ConnectorMethod/DynamicMessageDoc.method_bodyvia the JSR-223 java engine into a nativeHttp4sEndpointIO, reusing the samejavax.tools.JavaCompilerbackend ascreateJavaFunction.Extraction.decomposeonly reflects Scala types.programming_langvalues with a 400 before attempting compilation.DynamicResourceDocsEndpointGroup, the v6.0.0dynamic-resource-docs/validateendpoint) that were still constructingCompiledObjectswithout the new field.Test plan
DynamicUtilJavaHttp4sEndpointTest(3 scenarios): compiled-endpoint adapter unit tests, isolated from the HTTP round trip.DynamicResourceDocJavaTest(3 scenarios): role-gated Java doc 401/403/200, reject-unsupported-language, backward-compat withprogramming_langomitted.DynamicResourceDocTest(existing Scala-only suite): unchanged, all 8 scenarios still pass.FrozenClassTest: regeneratedfrozen_type_meta_datafor the newprogramming_langfield onJsonDynamicResourceDoc../run_all_tests.sh, 4 shards, 3522 tests): green.