build: pay the Scala 3 debt on 2.13 (flip sequenced after Doobie) - #91
Draft
hongwei1 wants to merge 257 commits into
Draft
build: pay the Scala 3 debt on 2.13 (flip sequenced after Doobie)#91hongwei1 wants to merge 257 commits into
hongwei1 wants to merge 257 commits into
Conversation
com.tesobe.CacheKeyFromArguments is a Scala 2 blackbox macro: it cannot expand at a
Scala 3 call site, and Scala 3's macro-annotation equivalent is still experimental,
so the flip needs these 24 keys written out rather than ported.
The macro read the enclosing class and method symbols plus the (non-@CacheKeyOmit)
argument names, and assigned (classFullName, methodName, args.mkString("_")) to the
placeholder var. Each site now states exactly that, and the placeholder var - three
random UUIDs that only existed so the macro had something to overwrite - is gone.
Getting an argument dimension wrong here is a cross-user data leak: a key that omits
a parameter serves the first caller's result to every later caller for the whole TTL,
and these sites cache metrics, FX rates, transactions, provider lookups and a user's
locale. Three things guard that:
- CacheKeyGoldenTest drives real cached methods and looks the produced key up in a
real Redis. It was written and run GREEN AGAINST THE MACRO BUILD FIRST, then kept
green after the rewrite - the keys are byte-identical, argument dimensions
included, not merely plausible. It deletes the exact key it expects before each
call, so a stale entry cannot satisfy the assertion.
- every rewritten key was machine-compared against its method's parameter list; all
24 cover every parameter (the macro had no @CacheKeyOmit sites in main sources -
the only occurrence was inside a generator string - so no parameter is excluded).
- the live cache/info namespace set is unchanged (15/15).
Also here: the dormant doCache branch of the connector generator emits an explicit key
instead of the macro (the enclosing class name is derived from the connector path it
already receives); the com.github.OpenBankProject.scala-macros dependency is dropped;
and the java.util.UUID.randomUUID imports left dead by removing the placeholder vars
are removed.
Verified: full suite 3484 tests / 0 failures; golden key test green on both sides of
the rewrite; contract surface diff exactly zero; cache/info namespaces unchanged;
single-suffix audit clean; dependency tree loses scala-macros and nothing else.
…eflection triage Three of the plan's dependency questions, answered by measurement. avro: deleted rather than migrated. The entire avro surface was one trait, AvroSerializer, with no implementor and no caller anywhere in main, test or config, and no source referenced org.apache.avro directly. avro4s 4.x has no Scala 3 build and 5.x is Scala 3-only with a different API, so this would have been a flip blocker; instead the trait, avro4s-core and avro go. snappy-java follows: it was pinned only to override what avro pulled, and with avro gone a dependency:tree with the pin removed shows no consumer left at all. scalameta 4.1.12 -> 4.13.6, the earliest line published for both _2.13 and _3, so the flip is a suffix swap. Used only by two in-repo lint helpers that parse this project's own sources; DuplicatedMessages asserts one of them. chill and elastic4s: no change, decision recorded. chill/chill-bijection/ bijection-core publish no _3 at all, and elastic4s's _3 line stops at 8.18.0 below our 8.19.1 - but all of them are macro-free (verified by scanning every class file in each jar for scala/reflect/macros references), so they are consumed as _2.13 through for3Use2_13 after the flip. That mattered most for chill: it is the cache value codec, and a macro there would have made the byte-compatible cache guarantee unkeepable. docs/scala3-reflection-triage.md classifies the runtime-reflection surface for the flip. The criterion is not a judgement call: ReflectUtils' OBP_TYPE_REGEX covers both com.openbankproject.commons.* and code.*, and only the former stays Scala 2.13, so 'does this reflection reach code.* types' decides A/B/C. Class C came out as five files, each with the detector that would catch it - the failure mode is silent (reflection on a TASTy-only class returns no members rather than throwing), which is why the flip's acceptance test is a zero-diff contract surface rather than a green compile. The doc also records why obp-commons keeps its unsuffixed coordinate: measured, no consumer outside this repository names it, so the rename would break the sibling worktrees to protect nobody. Verified: full suite 3484 tests / 0 failures; contract surface diff against a freshly rebuilt pre-change build exactly zero; cache/info namespaces unchanged (15/15); single-suffix audit clean; dependency tree loses avro, avro4s, snappy, magnolia, shapeless 2.3.9 and scalap, and moves scalameta to 4.13.6.
…spath maven-dependency-plugin's copy-dependencies only ever ADDS to target/lib, and the thin jar's manifest puts everything in that directory on the runtime classpath. So on an incremental build a dependency that has been removed from the pom goes on being loaded, and an upgraded one is loaded alongside its predecessor. This is not tidiness. Measured on this branch before the fix, target/lib held: - avro-1.11.4.jar, after avro was removed. Its removal was a CVE-2024-47561 remediation (CVSS 9.8, RCE via schema parsing), which therefore had no effect at run time for anyone who did not build clean. - json4s 3.6.12 next to 4.1.0-M8, i.e. both sides of a major upgrade at once, with load order deciding which one answered. - two scalameta versions and two fastparse versions. maven-clean-plugin now empties target/lib in prepare-package, declared ahead of copy-dependencies so it runs first; excludeDefaultDirectories keeps it from touching target/classes, whose contents this phase depends on. scripts/check_runtime_lib_pruned.sh compares lib/ against the resolved runtime dependency set and fails on anything that is not in it. Shown red before the fix (12+ stale jars, including the json4s pair) and green after; also shown red again with a stale jar planted deliberately, so the check is known to be able to fail. It handles classified artifacts (org.jline:jline:jdk8, com.github.jnr:jffi:native), which dependency:list prints with an extra field - parsing those as unclassified reports them as stale on every run. No separate duplicate-version check: Maven resolves one version per groupId:artifactId, so a pruned lib/ cannot hold two of the same artifact, and a filename-based heuristic cannot see groupIds - it misreads io.swagger:swagger-parser 1.x and the v3 parser as one artifact twice.
Props initializes lazily, so the private lockedProviders field this trait reads by reflection is null until something has used Props. A suite that mixes in PropsReset without otherwise touching Props - a pure unit suite - therefore aborted in beforeAll before running a single test, with a NullPointerException naming a Lift-internal field rather than anything about the suite. Every existing user happened to touch Props on the way in, so the trap only appears when a new suite does not. Reading Props.mode forces the initializer, and the read is null-safe on top of that.
…r seam
Scala 3 has no ToolBox - scala.quoted.staging compiles quotes, not strings - so the
flip has to replace the compiler that DynamicUtil uses. This puts that compiler behind
an interface now, on 2.13, with the existing ToolBox as the only implementation, so the
flip swaps one class instead of editing every caller.
This is a correction to the migration plan's S2, which called for moving the ToolBox
into a module pinned to Scala 2.13 forever, on the reasoning that dynamic code only
references OBP *model* types and those stay on 2.13. It references far more than that:
DynamicUtil.importStatements puts code.api.util.{APIUtil, CallContext},
code.bankconnectors._, code.api.cache.Caching and others in scope, and
InternalConnector.createScalaFunction builds the method signature from the Connector
trait and calls back into InternalConnector. All of those are obp-api's own classes,
which become TASTy-only at the flip, and a 2.13 compiler cannot read TASTy. So a 2.13
compiler island cannot compile obp-api's dynamic code at all; the compiler must track
obp-api's Scala version, and what this seam isolates is the compiler API.
Behaviour is deliberately unchanged, including the parts that look accidental:
- the ToolBox retry (it intermittently fails a first compile and succeeds on an
identical second call),
- compile-once-per-source caching, which moved into the implementation,
- and the split between a compile error and an error thrown while evaluating: the
first is a Failure with no cause, the second a Failure carrying the exception, as
Box.tryo produced. DynamicCompileFailure carries that distinction so the customer's
failing method_body keeps its stack trace.
DynamicCompilerFourChainPocTest is the plan's S2 acceptance test: legacy Scala-2-style
method_body snippets shaped like each of the four chains that compile at run time
(Dynamic Connector, Internal Connector, Dynamic Endpoints, ABAC rules), plus the error
semantics and the caching contract. At the flip it is what shows a dotc-based
implementation still accepts stored snippets (plan risk F-9).
DynamicCompilerKillSwitchTest covers the off state (plan risk S-4) in its own suite,
and needs EnvVarOverride: the runner and CI both export
OBP_ALLOW_USER_GENERATED_SCALA_CODE=true, which beats setPropsValues, and Lift's
provider precedence also keeps a later 'false' push from overriding the 'true' pushes
the POC suite makes. Both facts were found by the full-suite gate - each version passed
when its suite ran alone.
No claim is made about sandbox posture: the permission machinery is untouched, and
SecurityManager remains a no-op on JDK 24+ exactly as before.
Verified: full suite 3493 tests / 0 failures; the nine new tests green both alone and
in the full run; single-suffix audit clean.
scalatest 3.0.8 has no Scala 3 build - verified against Maven Central - so the flip cannot carry it, and 3.2.x is not a drop-in: it moved the style traits into per-style packages and removed the old names. Checked in the 3.2.20 jars rather than assumed: org.scalatest.flatspec.AnyFlatSpec is present, org.scalatest.FlatSpec, FeatureSpec and Matchers are gone. AnyFeatureSpec also capitalises its DSL, feature/scenario becoming Feature/Scenario. Done here on 2.13, as its own step, because that is the only way to verify it. On 2.13 the suite still runs, so this change is answerable by 3493 tests; folded into the flip it would have been unverifiable until every Scala 3 compile error was also fixed, and any failure afterwards would have had two candidate causes. Mechanical, and applied by script across 354 files: org.scalatest.FlatSpec -> org.scalatest.flatspec.AnyFlatSpec org.scalatest.FeatureSpec -> org.scalatest.featurespec.AnyFeatureSpec org.scalatest.Matchers -> org.scalatest.matchers.should.Matchers feature( / scenario( -> Feature( / Scenario( (1210 + 3131 call sites) Tag, GivenWhenThen, BeforeAndAfter*, Suite and Ignore have not moved and are untouched. Commented-out call sites were rewritten too, so they still match the live ones if anyone uncomments them. Two files needed more than the script: ServerSetup and OBPEnumerationTest import org.scalatest._, and that wildcard no longer supplies the moved names, so they get explicit imports. ServerSetup is the base class most suites extend, which is why only two files were affected rather than sixty. .github/scripts/check_test_isolation.py learned the capitalised spellings; its regex matched only lowercase scenario/feature, so after the rename every setPropsValues call would have read as being at class-body level. Verified: full suite 3493 tests / 0 failures - the same count as before the migration, which is the part that matters: a silently undiscovered suite would show up as a drop, not as a failure. Test-isolation lint clean.
…ch fix
Sub-second Redis TTLs were rounded up to one second. The in-house memoize layer wrote with
SETEX, whose unit is whole seconds, under a max(1, ttl.toSeconds) floor; scalacache stored
with millisecond precision. So a 300ms TTL became a 1s TTL.
Not a live bug - every current caller passes whole seconds
(connector.cache.ttl.seconds.* multiplied to millis) and a zero TTL never reaches Redis
because Caching forwards it uncached. It matters because the entire claim made for
replacing scalacache is that keys, values and TTLs are unchanged, and this quietly made
that claim false for one input class. PSETEX takes milliseconds and restores it.
RedisTtlPrecisionTest was written first and shown red against the SETEX version
(1 was not equal to 2: after 600ms a 300ms entry was still cached), green after.
Also examined in this round, no change needed, recorded so the next round need not redo it:
- the 25 explicit cache keys: no argument dimension lost (machine-checked against each
method's parameter list, plus the golden A/B against the macro)
- the scalatest DSL rename across 354 files: one Feature( inside a string, and it is a
comment correctly naming the renamed call, not corrupted content
- in-memory expiry: evicts on read and honours the TTL; the only signature change is
dropped @cacheKeyExclude annotations
- -Xsource:3 quickfix residue: no refinement types left behind, and the only
package-shadowing suspect is a local val plus a comment, not a real one
- the regenerated gRPC service: exposes getBanks alone, in both the descriptor and
bindService. The three RPCs the hand-written filter used to hide are absent - the
filter was unnecessary because api.proto declares only getBanks.
The protoc-gen-scala shim baked this machine's absolute path into a file cached under
target/grpc-codegen. It works where it is written, but the cache is a directory that gets
copied, and this repository is routinely checked out into several worktrees at once - a
copied cache would then quietly point at another checkout's jars. The shim now resolves its
own lib/ at run time. Verified by deleting the shim, re-running the generator, and confirming
the regenerated sources are byte-identical to what is checked in.
Also examined this round, no change needed:
- the LogLevel <-> Redis mapping moved out of the deleted hand-written generated file: all
six levels map identically in both directions, the None fallback is preserved, and the
proto's enum numbers match the old Int constants, so the wire form is unchanged
- parse failures are now cached where they previously were not: the original had a
inside computeIfAbsent's mapping function, so a NonLocalReturnControl unwound before
anything was stored. Callers see the identical Failure either way, the same source always
parses the same way, and compile failures were already cached - so this is an intentional
difference, not a regression
- DynamicUtil's public surface: only the ToolBox val was removed, and nothing referenced it
Reported separately rather than fixed here (task_48e89583): NewStyle.getEndpointMappings and
LocalMappedConnectorInternal.getCurrentFxRateCached both put the whole CallContext in their
cache key. CallContext carries startTime, correlationId, url, verb, ipAddress and user, so the
key is unique per request: those caches never hit, and every call writes a Redis key that
lives until its TTL. It is pre-existing - the macro included every parameter that was not
@CacheKeyOmit, and neither site annotated callContext - and this migration reproduced the
macro's keys deliberately, so fixing it here would have meant changing caching behaviour
inside a migration whose whole claim is that behaviour is unchanged.
Five minimal compiles against the real lift-persistence_2.13 jar narrow the failure from "Lift Mapper does not work on Scala 3" to the KeyedMapper / KeyedMetaMapper part of the hierarchy: plain Mapper[A] compiles, and neither IdPK nor the object-extends-class idiom is the trigger. The last point rules out an entity-side refactor, which is the expensive route somebody would otherwise try first. The same idiom written in dependency-free Scala 3 source compiles clean, which points at the 2.13 pickling rather than the shape, and therefore at cross-building the lift-persistence fork as the next experiment. Recorded as a lead with its limits stated, not as a conclusion.
…imating it Compiling the fork's own sources with Scala 3 does not crash - it produces ordinary migration errors, so there is no dotty bug to wait on. 162 errors plain, 95 under -source:3.0-migration, and the residue splits cleanly: 42 cyclic errors that the compiler itself says are missing explicit result types, and 18 TypeTag errors that cannot be annotated away because scala-reflect does not exist on Scala 3. Those 18 share a root cause with the plan's F-1 item, so the two should be taken on together rather than costed separately. Run in a scratch clone; no repository was modified.
…esis The TypeTag pile was mechanical after all - the tag is only stored, never introspected, and no consumer reads it, so ClassTag is a drop-in and the count went 95 to 79. The cyclic pile was not. -explain-cyclic suggests adding explicit result types; annotating all four object By overloads changed nothing, same 42 errors on the same lines. What they actually share is the F-bounded keyed hierarchy: 33 are primaryKeyField accesses through it and 9 are the trait declarations themselves. That is the same construct as the assertion failure, so the two symptoms are one problem and cross-building does not escape it - it only makes the failure legible. The previous commit's framing was too optimistic; superseded here rather than edited out.
…tream Only the assertion failure has a small repro. Four synthetic models of the cyclic form - up to a mutually recursive trait pair with a concrete entity and meta object - all compile clean, and two direct fixes on the fork's own sources moved nothing: simplifying KeyedMetaMapper's redundant self-type, and adding explicit result types to the object By overloads. So the trigger is still unidentified and an upstream report would have to point at the whole fork. Recording the four models and two failed fixes so the next person bisects the real sources instead of building up from synthetic ones.
hongwei1
changed the title
… commit 639133d added grpc patterns here and said it excluded them from duplication analysis. It did not: SonarCloud runs this project in Automatic Analysis mode, which does not read sonar.cpd.exclusions. The quality gate failed on that commit too, and still fails. The proof is a pattern nobody added recently - obp-api/src/test/**/*.scala has been listed here all along, and API1_2_1Test.scala still reports 13.3% duplication. So the file has never been configuration, and the new-code duplication in this PR comes from the scalatest rename touching 5041 lines across 358 already-duplicated suites. Fixing it needs SonarCloud project settings or a scanner step in CI, neither of which belongs in this PR. Documented in place so the list is not trusted again.
…o build/scala-3-migration
… 3.2.20 The base gained two test files after this branch left it, written against scalatest 3.0.8 and without the package-prefix implicit import that -Xsource:3 requires. Both of those are this branch's changes, so the breakage is this branch's to fix: FlatSpec becomes AnyFlatSpec, Matchers comes from org.scalatest.matchers.should, and org.json4s.jvalue2monadic is imported explicitly. This is why the pull_request workflow failed while the push workflow passed and the local suite was green - only the merge with the base contains these files. Worth remembering for the next long-lived branch: a green branch build says nothing about the merge.
The previous note said the whole file is inert. Only the test pattern is demonstrably not applied - API1_2_1Test.scala reports 13.3% duplication while being listed, and an excluded file would report 0. The grpc files report 0-3%, which is equally consistent with the exclusion working and with generated code that does not trip CPD. Stating that as proof of inertness was the same over-reading that produced two earlier corrections on this branch, so the note now marks the two halves separately.
3.8.4, the latest release and six versions past the first one tried, fails with the same assertion text as 3.3.8 and 3.7.2. Three compiler generations reject the construct identically, so upgrading is not a route out of the blocker and should not be offered as one when the remaining options are weighed.
… them fails Every crash recorded here came from declaring a class in the keyed hierarchy. Using an already-compiled one is a separate question and it works: a Scala 3 file compiled against obp-api's own 2.13 classes reads the meta object, calls a query method and touches a field, exit 0 with classfiles produced. That turns 'keep the entity layer on 2.13' from a hypothesis into a measured route. Recorded with the caveat that makes it non-trivial: MetaMapper's existential degrades to Any across the boundary, which the compiler warns can hide type errors, so where the boundary is drawn matters and one entity is not the whole surface.
The Any-degradation caveat on the 2.13-entity-layer route turns out to be one line. Of 163 MetaMapper mentions, 156 are getSingleton and nearly all the rest are meta-object declarations - both stay inside the entity module. Three sites actually cross, and only Boot's List[MetaMapper[_]] carries the existential; BaseMetaMapper is non-generic and has nothing to degrade. That site does not need the generic type either: models is consumed only by Schemifier.schemify, which javap shows takes Seq[BaseMetaMapper]. Not changing it here - it is preparation for a route nobody has chosen. Recorded so the route can be costed honestly.
The previous commit said Boot's List[MetaMapper[_]] could be narrowed to List[BaseMetaMapper] without changing behaviour. Checking every consumer before making the change showed otherwise: models has six, and four are test helpers calling bulkDelete_!!, which javap confirms is on MetaMapper and not among BaseMetaMapper's seven schema members. So the site needs its generic type, and under a 2.13 entity split those four helpers would sit on the Scala 3 side calling a method on a value degraded to Any. The boundary cost is a real problem affecting real code, not one annotation. Still not changing anything - the route is unchosen.
The header block described setex(max(1, ttl.toSeconds)) and claimed it matched scalacache's sub-second rounding. Both halves are wrong: the code has used psetex since the TTL fix, and scalacache stored with millisecond precision, so setex never matched it - that false claim is what made the rounding look intentional in the first place. The file therefore contradicted itself, with the accurate account sitting forty lines below in cachePut. Behaviour is unchanged and already pinned by RedisTtlPrecisionTest; cache suites 9/0.
…ed after it The blocker document listed routes without saying which was taken, which is the state that invites someone to re-run the same experiments. The route chosen is to remove Lift Mapper rather than work around it: no 2.13 entity module, no patch to the fork. That work is already underway in the OBP-API-I copy on lift-mapper-remove, with ATMs the first table fully off Lift. The flip becomes possible when entities no longer extend KeyedMapper; until then the four disproved routes below stand as disproved. What this branch delivered does not depend on that sequencing and ships on its own.
hongwei1
changed the title
CallContext carries per-request state (startTime, correlationId, url, verb, ipAddress, user), so a key rendering it is unique per request: the cache can never hit, and getCurrentFxRateCached wrote a fresh Redis entry per call that lived out its TTL. Measured with both TTLs forced on: two calls differing only in CallContext produced two keys there, and one key after this change. Both sites inherited this from the com.tesobe CacheKeyFromArguments macro, which rendered every parameter not annotated @CacheKeyOmit - neither annotated theirs. The explicitization reproduced the macro output verbatim, so these two keys now intentionally diverge from the macro-era format. Every other cache site already keys on business arguments only, and the connector generator stamps @CacheKeyOmit onto callContext for the methods it generates. getEndpointMappings also 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. Caching only the mappings fixes that and keeps a hit from handing the caller the originating request's CallContext. 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. CacheKeyCallContextTest guards the invariant across every cache site. CacheKeyGoldenTest covers neither of these two methods, so no golden string changes; its scaladoc records the intentional divergence.
build: migrate to Scala 2.13
First table of the Lift Mapper removal. Atms.buildOne now returns
DoobieAtmsProvider; the Lift AtmsProvider implementation is gone. The MappedAtm
entity itself stays for now - ToSchemify, the sandbox import and the MxOF JSON
factory still reference it, and those are the next steps for this table.
The provider is ported from the reference branch, not merged, and audited on the
way in. Two things that audit caught:
- the reference calls DoobieUtil.runQuery for its INSERT/UPDATE/DELETE. On this
line runQuery's out-of-request fallback is Strategy.void on an
autoCommit=false pool, so those writes would have been rolled back when the
connection returned. The four write calls now use runUpdate; the four reads
still use runQuery.
- java.sql.Timestamp has no Meta instance without doobie.implicits.javasql._,
which the two Doobie files already on this line import and the reference did
not.
The provider test asked MappedAtmsProvider directly, so it would have kept
testing the old implementation after the switch. It now goes through
Atms.atmsProvider.vend, and was proved load-bearing first: breaking the Lift
read path made it fail with '0 did not equal 3'.
Its last assertion compared whole objects, which cannot hold once the provider
answers with the commons Atm type instead of MappedAtm entities. It compares the
fields it actually depended on instead - id, bank, name, address, location and
licence - rather than dropping to a weaker check.
Suite 3507/0, unchanged from the pre-change baseline.
The previous commit moved every ATM read onto the Doobie provider but left the sandbox import writing the row with MappedAtm.create. That is the split-brain state the migration has to avoid: a table whose writes go through Mapper while its reads come back through Doobie. Both halves now go through Atms.atmsProvider.vend. createSaveableAtms builds the commons Atm and wraps it in a SaveableAtm that persists via createOrUpdateAtm, and AtmType becomes AtmT rather than the entity. SandboxDataLoadingTest already verified the imported rows by reading them back through the provider, so it covers this change as written. Note for anyone running that suite alone: two of its scenarios fail on a missing V_ACCOUNT_ACCESS_WITH_VIEWS, a SQL view another suite's setup creates. It is unrelated to ATMs and does not occur in the full run. Suite 3507/0, unchanged.
The next step of this table's migration takes MappedAtm out of ToSchemify.models. Every reset path clears tables by looping that list and calling bulkDelete_!!, so the moment the entity leaves it nothing clears atm rows unless an explicit Doobie DELETE is added to all four paths. A leak like that does not fail where it is caused. It fails later, as a count one too high in a suite that never mentions ATMs. This test makes it fail on the commit that causes it: it writes rows through the provider, runs the same resetDatabaseForTestClass the next class will run, and asserts the table is empty. Proved load-bearing before being kept: removing MappedAtm from ToSchemify made it fail, and restoring it made it pass again. A first attempt asserted that two scenarios in one class do not see each other's rows. That was wrong about the framework - the reset runs per test class, not per test - and it failed with '2 did not equal 0' for that reason rather than for a real defect. It now drives the reset directly instead of inferring it. Suite 3508/0: the baseline 3507 plus this test.
… method enumeration Connector.connectorMethods/implementedMethods enumerate Connector's own decls to decide which members are genuine dynamic-dispatch connector methods. Two kinds of Scala 3-compiled trait member leaked through and got miscounted: - synthetic cross-module setters for Connector's own protected vals (bankTTL, formats, ...), named "_setter_$xyz_=" or "_setter_@xyz_=" - isVal/isVar report false for them under Scala 2.13's scala.reflect.runtime.universe, the same gap fixed for InternalConnector.messageDocs, but connectorMethods needed the same exclusion independently since implementedMethods unions it in; - the eight protected implicit def adapter conversions (boxToTuple, tupleToBoxTuple, OBPReturnTypeToBox, ...) - both isPublic and isImplicit are equally unreadable cross-compiler for these, so both were tried and both failed; named explicitly instead. ConnectorTest's own WrongOutBoundType.unapply needed a third, narrower fix: Option[AnyVal] connector method parameters (dependents: Option[Int], isActive: Option[Boolean]) read back as Option[Object] - the JVM parameter signature boxes/erases the value type with no TASTy to recover it from. A wildcard-tolerant type comparison is now used only for the final by-name field check, not for the CallContext/query-params shape detection earlier in the same method - using it there deleted the erased-looking fields from the comparison entirely instead of tolerating their type.
Every diff FrozenClassTest reported was one of three benign, already- understood renderings from the migration, not a real API structure change: json4s's package rename (org.json4s.JsonAST.JValue -> org.json4s.JValue, already normalised in RestConnector_vMar2019_FrozenTest), an Option[AnyVal] connector/JSON field reading back as Option[Object] under Scala 2.13's scala.reflect.runtime.universe (a JVM erasure gap with no TASTy to recover the original argument from - same shape already fixed in Connector.scala and ConnectorTest.scala), and BigDecimal now rendering as its fully-qualified scala.math.BigDecimal. Regenerated via FrozenClassUtil.getFrozenApiInfo, per the test's own stated remediation.
…readField gap json4s's Scala 3 ScalaSigReader.readField throws NoSuchElementException extracting an Option[AnyVal] field (e.g. User.isDeleted: Option[Boolean]) declared on a Scala-2.13-compiled obp-commons type, because it can only recover the erased type argument by reading TASTy and those classes have none. This is the extract-direction mirror of the earlier ObpCommonsProductSerializer decompose fix. Add ObpCommonsProductDeserializer, which builds any concrete obp-commons case class directly from its constructor parameter names/types (read via scala.reflect.runtime.universe, which resolves the real type argument correctly) instead of letting json4s's default Reflector walk the class. Along the way, fix two pre-existing bugs this newly-exercised code path uncovered: ReflectUtils.invokeConstructor threw for any class with an auxiliary constructor (e.g. BankCommons), and a missing @optional field with no Option wrapper had no tolerated default.
…optional The deserializer's fallback for a missing (JNothing) constructor field defaulted every primitive to its zero value and every non-primitive to null, modeled on JNothingSerializer - a mechanism for a different problem, schema evolution in a stored/cached JSON blob predating a new column. Applied inside generic extraction, the same tolerance also covered genuinely untrusted input: a POST body missing a required field extracted successfully instead of failing, either producing a wrong result (CreateViewJson missing every boolean field returned 201 instead of 400) or deferring the failure downstream past where it could be caught as a proper 400 (a payment body missing its amount built a null-carrying instance that later NPE'd as an uncaught 500). Removed the primitive defaults entirely - a missing primitive now falls through to Extraction.extract, which throws MappingException for it, matching json4s's own behavior for a required field. Replaced the blanket non-primitive null-default with a check on the constructor parameter's own annotations: only default to null when the field is explicitly marked @optional (com.openbankproject.commons.util.optional, already used for exactly this on fields like BankCommons.swiftBic), checked before extraction in buildInstance rather than inferred from the missing field's shape alone.
frozen_type_meta_data.txt is the reviewable text sibling of the binary frozen_type_meta_data blob, and FrozenMetaDataTextTest fails when the two disagree. The blob was regenerated for the Scala 3 flip; its text rendering was not, until now. Rewritten via FrozenMetaDataText's own generator - the diff is the same three benign renderings already covered by the blob regeneration (json4s's package rename, Option[AnyVal] reading back as Option[Object], and one now-added type: JArrayBody removed, matching the blob).
getPrimaryConstructor took .alternatives.head - the first overloaded constructor scala-reflect happened to enumerate. For a class with only one constructor this is harmless; for one with an auxiliary constructor as well (code.methodrouting.MethodRoutingParam(key: String, value: String) also declares def this(jObject: JObject), a JSON convenience constructor), the enumeration order is not guaranteed and differed between environments - locally it consistently returned the real primary constructor, in CI it consistently returned the auxiliary one instead, corrupting every reflection built on top of it for that type (constructor param info, instance construction, JSON extraction). Select by isPrimaryConstructor, a real flag scala-reflect exposes for exactly this, instead of position.
…a symbol flag The previous fix (13418d5) replaced positional .alternatives.head with a filter on isPrimaryConstructor, a flag scala-reflect exposes specifically for this. It read correctly for every local check but did not change CI's answer at all - CI kept picking MethodRoutingParam's auxiliary `def this(jObject: JObject)` as if it were the primary constructor, identically to before that fix landed. Replaced the flag-based selection with a JVM-bytecode-only one: a case class's primary constructor parameters are exactly its declared instance fields. Pick the constructor alternative whose every parameter name appears in the class's own declared fields (ordinary classfile metadata, unaffected by TASTy or by whatever made isPrimaryConstructor disagree between environments), falling back to positional selection only if none match. Kept code.util.PrimaryConstructorDiag2Test in this commit rather than deleting it after local verification, since local verification of this exact bug has now been wrong twice - it prints MethodRoutingParam's declared fields, constructor alternatives, and isPrimaryConstructor flag directly, so CI's own log carries the ground truth this time if this fix also fails there.
… the fix Kept deliberately in the previous commit so CI's own log would carry ground truth about isPrimaryConstructor/declared fields for MethodRoutingParam if the field-name-based fix also failed there. It didn't - all 9 CI test shards and compile now pass. The diagnostic has served its purpose.
…ripts Both scripts took a source and destination path straight from sys.argv and opened them without resolving ".."/symlink segments first, flagged by SonarCloud as 5 high-severity path-validation findings on PR #91. Both scripts are meant to accept whatever directories the caller names, so the fix is not to constrain them to a fixed root - it's making sure what gets opened is the path actually meant, not one still carrying unresolved traversal segments.
… scripts The .resolve() fix (887ab16) collapses ".."/symlink segments but did not satisfy SonarCloud's rule, which stayed on the same two flagged lines in each file after that commit - its pattern wants src/dst constrained inside some fixed base directory, which doesn't fit either script: both take a source and destination the caller names on the command line, by design, and constraining them to a fixed root would break the tool's actual job. NOSONAR on the four flagged lines with the reasoning inline; .resolve() stays, since normalizing the path before use is still worth doing regardless of what satisfies the rule.
…cy script Postgres freezes a view's column types at creation time and refuses to change them on CREATE OR REPLACE VIEW. The legacy SQL script under src/main/scripts/sql/OIDC cast username to text; the changelog's version selected it unchanged, which is a no-op on a fresh install (authuser.username is varchar(100) either way) but fails on any database upgrading from that script, where the view already exists with username frozen as text: ERROR: cannot change data type of view column "username" from text to character varying(100) Cast it the same way the legacy script did, so the column type the view already carries is preserved across the CREATE OR REPLACE. PostgresMigrationTest gains a scenario that builds the legacy-shaped view by hand and asserts createOidcViews can replace it without throwing.
GET .../resource-docs/v7.0.0/swagger 400s with "OBP-50000: Unknown Error. Can not convert internal swagger file." The underlying cause is an AssertionError from scala.reflect.runtime: "no symbol could be loaded from class cats.effect.kernel.Par$ParallelF$". TestEmailResponseJsonV700 is a case class nested inside Http4s700.Implementations7_0_0. Reflecting its own type requires scala-reflect to resolve its owner chain, which walks into the enclosing object's signature - and that signature transitively touches IO's companion object, where cats-effect's Par trait declares ParallelF as an abstract type member with no runtime companion class. scala-reflect's classfile fallback tries to load one anyway and throws. SwaggerJSONFactory.getAllEntities/getNestedRefEntities called ReflectUtils.getType unguarded, so this single unreflectable entity took down Swagger generation for the whole requested API version. Route both call sites through a safeGetType helper that logs and excludes the entity from schema generation instead of raising - the entity still appears as a definition, just without its nested fields expanded. Verified by reverting the fix and re-running: it reproduces the exact peer-reported stack trace one-for-one.
computingSensitivePatterns is set only while sensitivePatterns computes itself, to route around a Scala 3 LazyVals non-reentrancy deadlock: that computation calls APIUtil.getPropsAsBoolValue, the first touch of APIUtil$ on the thread, which triggers the whole APIUtil$ class-init cascade - including eager vals that read db.password/db.url and can log them via getPropsValue's env-source debug line. Every log call routes through maskSensitive, which needs sensitivePatterns, so any log call landing on that thread during the window would recurse into the same lazy val and deadlock without the guard. The guard's fallback was to return the message completely unmasked - not limited to SecureLogging's own bootstrap traffic, since the window is the entire cascade on whatever thread happens to trigger it first, which can be a request thread as easily as a startup one. A credential logged during that one-time window went out in cleartext. Apply a small set of static patterns (password/secret/token/jdbc - plain vals, no props lookup, so they can't recurse) during the guard window instead of skipping masking outright.
…hape getPrimaryConstructor's field-name-subset heuristic can't disambiguate a class where one constructor's parameters are a strict subset of another's, and both are subsets of the declared fields - BankCommons is exactly this shape: a 9-field primary constructor and a 7-field auxiliary one sharing those first 7 names. Both pass the filter, so .find (first match) still depends on alternatives' order, the same nondeterminism this function was already fixed for once. The primary constructor's parameters are always exactly the class's fields, so it can only be the candidate with the most parameters; select by that instead of position. toOther, a separate function doing the same "pick a constructor" job, was never routed through the fix at all - it still picked alternatives(0) directly. Every Bank -> BankCommons conversion goes through it via Converter.toCommons, so this was live, not dormant. getFieldValues' Scala 3 val/lazy-val recovery clause (a zero-arg method declared directly on the class) can't tell a val-accessor from an ordinary def by shape alone - both compile identically. Require a matching backing field (accounting for Scala 3 LazyVals' "$lzy1"-suffixed field name) before accepting the method as a field, so a genuine helper method isn't reported as one. Guard the added runtimeClass lookup itself: it can throw on a type it resolves to a refinement rather than a nominal class (observed for a path-dependent inner class), which this function never touched before - fall back to the prior permissive behaviour there rather than newly crashing on inputs it used to handle.
sameTypeAllowingErasedGeneric accepted a type-arg pair as equivalent whenever either side stringified to "Object" - the erasure a Scala 3-compiled connector method's AnyVal generic argument reads back as - with no check on what the other side actually was. That is looser than the documented reason for it: only AnyVal generic arguments erase this way, so treating an arbitrary reference type (String, a case class) on the other side as compatible papers over a real mismatch this check exists to catch. Require the non-erased side to itself be a named AnyVal. Distinguishing which AnyVal - Int vs Boolean vs Long - is not recoverable without TASTy, so that part of the leniency stays; only the "matches anything" acceptance narrows.
Both disjuncts checked the same JsonSchemaGeneratorTypes constant against itself (tBigDecimal twice; tListWildcard twice in a three-way check) - JsonSchemaGeneratorTypes declares exactly one constant for each, so the second check in each pair could never differ from the first. Harmless, but misleading: it reads as if two distinct types were handled.
methodNameToSymbols excluded Connector's two public vals (formats, messageDocs) from the dynamic-method map by name only - isVal/isVar can't do it (Scala 3-compiled Connector has no ScalaSig for the reflect API to read), and neither can isPublic/isMethod, since a val getter and a genuine method compile to the identical JVM shape. A val/def added to Connector later without updating this name list would silently leak into the map as if it were a real dynamic-dispatch connector method. Connector.scala's own connectorMethods already has the general fix for this same problem: every dynamic-dispatch connector method operates on some entity plus CallContext, so it always takes at least one parameter. Apply the same filter here - it also turns out to exclude several zero-arg members (nameOfConnector, the *TTL config getters, the $default$N synthetic parameter defaults) that the name-only list never covered, which were leaking into the map before this fix regardless of formats/messageDocs.
ObpCommonsProductSerializer/Deserializer intercept every obp-commons domain object on the JSON (de)serialization hot path, and both re-derived a class's constructor info (Type lookup + getPrimaryConstructor's full alternatives scan) from scratch on every single call - the sibling MapperSerializer a few lines above already avoids exactly this cost via Memo, keyed by Type. A class's constructor parameter names/types don't change between calls; only the per-instance values do. Cache the constructor lookup by Class, same Memo already in use here, and keep re-reading values per call.
Converter and ConverterWithType declared five identical implicit vals verbatim; only toCommons itself differed, and only by which Type it passed to ReflectUtils.toOther (typeTag[D].tpe vs a constructor parameter). Converter now extends ConverterWithType with that Type fixed to typeTag[D].tpe and inherits everything, the same pattern OBPEnumeration already uses over OBPEnumerationBase for the identical shape.
implementedMethods re-resolved "code.bankconnectors.Connector" via mirror.staticClass inside its .collect predicate, once per candidate member of the concrete connector's full type - potentially hundreds of redundant symbol-table lookups per connector singleton at first access. connectorMethods, two lines above, already hoists the equivalent lookup into a val before its own filter runs; do the same here.
password/secret/token/jdbc left out anything shaped like an Authorization bearer token or an API/private key - neither matches any of the four substrings, so either would still pass through the bootstrap window in cleartext despite the window not being scoped to any particular message source. Add Authorization: Bearer and a generic "key" category. Regex find() matches anywhere in the string rather than only at the start, so "key" also catches api_key/private_key the same way "token" already covers access_token/refresh_token/id_token without a separate pattern per variant.
…' order Selecting the subset-matching candidate with the most parameters still fell back to alternatives' JVM-supplied order whenever two candidates tied on size - the exact nondeterminism this function exists to remove, just one level down from the case it was already fixed for. The primary constructor's parameters are exactly the declared fields, not merely the most of them among the candidates: selecting by exact set equality first (falling back to size only when no candidate matches exactly, e.g. a class with a body-declared val beyond its constructor's own fields) means a same-size tie can never satisfy it in the first place - only one constructor's parameters can equal the full field set.
isKnownAnyVal only recognized the 8 built-in primitive names (Int, Boolean, ...), so a custom AnyVal-derived value class used as a connector method's generic argument - which erases to Object the same way a primitive does - would fail this check and report a false type mismatch. Subtype-check against scala.AnyVal instead, covering both the built-ins and any value class the same way.
The generic "key" pattern added for the bootstrap window matched any "...key: value" shape, not just a credential - MappedMetrics logs "cache key: (...)" during a normal cached call, and this pattern would have silently mangled it during the bootstrap window with no way to turn that back on (bootstrapPatterns is not props-gated the way sensitivePatterns' own equally loose "key" pattern is). Require an api_/private_/secret_/access_/encryption_ prefix instead of a bare "key", closing the credential gap this pattern exists for without also catching cache/database/map keys that carry no secret.
The exact-match selection compares parameter names, not types or order, so two constructors whose parameter names both equal declaredFieldNames but differ in type/position (a legal overload) would still tie and fall back to alternatives' order for that specific shape. No class in this codebase does this - closing it would mean comparing parameter types too, itself a source of cross-compiler reflection gaps this migration keeps finding - so it's recorded as a known limitation rather than silently assumed away.
ConsentUtil logs consumer_key='...' on a consent/consumer mismatch - a credential-shaped value the prior password/secret/token/jdbc/api_key- style prefix list didn't cover, so it would still leak in cleartext if that log call landed inside the bootstrap window. Add consumer_key. This list stays a known-common-case enumeration, not a closed one - grepping the codebase also turned up public_key and session_key, but neither has a confirmed log call site the way consumer_key does, so they're left out rather than added speculatively; documented on the list itself as a maintenance note for the next one found. Also give each of the six prefixes (api/private/secret/access/ encryption/consumer) its own test scenario instead of one shared api_key case - a dropped or mistyped alternative would otherwise compile and pass silently, since none of the five other prefixes' coverage depended on it.
…mments
The block comment above bootstrapPatterns spelled out the prefix list
("api/private/secret/access key") separately from the key pattern's own
comment a few lines below - the two had already drifted once (the block
comment still said "api/private/secret/access" after encryption and
consumer were added to the pattern itself). Point at the single list
instead of keeping two.
No test exercised this endpoint under either auth style (OAuth1 or DirectLogin token) - added while investigating a peer-reported 401 from a real-process OIDC end-to-end script that a Maven test JVM cannot reproduce (tracked as a known issue, not fixed here: the process-specific ResourceDocMatcher index for v3.0.0 comes back empty, root cause not yet located). Both auth styles return 200 in this suite, so the coverage gap itself - not a bug in the matching logic reachable from here - was the actual finding.
createAccountsAndViews' duplicateIbans check compared raw IBAN strings across the whole import batch, unlike the neighboring duplicateNumbers/ duplicateIds checks which are scoped by (bank, value). An IBAN is only unique per bank - DoobieBankAccountRoutingQueries' own unique index is (bankId, scheme, address), and existingIbans already looks IBANs up per bank - so two different sandbox banks reusing the same IBAN in their own address space got the whole import rejected. The shipped example_import.json fixture hits this: bank Y's accounts mirror bank X's, IBANs included. Scope the check by (bank, iban) instead. Found by a fresh-boot real-jar test that exercises the full fixture against a genuinely empty database (.local-testing/obp-tests/run-suite6-fresh-boot-sandbox-import.sh) - suite2's cloned-database approach never hits this since the clone already carries months of prior data.
|
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.
Everything Scala 3 needs that can be done on 2.13, delivered and verified. The flip itself is
not here: it is blocked, and the blocker is documented rather than worked around. One commit per
verified step, same structure as #90.
Based on the head of #90 (
build/scala-2.13-migration); retarget todevelop-obponce #90 merges.What is in it
target/libpruned so a removed dependency actually leaves the runtime classpathCacheKeyFromArgumentsmacro replaced with explicit keys; the dead avro stack dropped-Xsource:3(307 files)DynamicScalaCompilerinterfaceThree plan premises that measurement overturned
for3Use2_13. Only_34.1.0-M8 with scala3-staging extracts Scala 3case classes.
DynamicUtil.importStatementsputsobp-api's own classes in scope, so the ToolBox cannot be isolated into a separate module. It
became a compiler seam instead.
_3, and 3.2.x removed the legacy styletraits — an unplanned prerequisite.
Verification
Every commit: full local suite, consumer-contract surface diff against a same-source baseline,
single-Scala-suffix audit. Milestone gates:
Three review rounds over the full diff, each fix reproduced by a failing test first: a sub-second
Redis TTL rounded up to 1 s by
SETEX(round 1), a protoc shim with an absolute path baked in(round 2), zero findings (round 3).
Not covered, deliberately:
run_probeswas withheld — itsreset_env()mutates the shareddatabase and another session's server holds 8080.
perm_matrixandtwo_tppproduce no verdicton a non-8080 port; running the identical script against an unmodified base build produced the
same failures, which is what shows they are not attributable to this branch.
Also fixed in passing:
target/libnever pruned removed dependencies, so avro(CVE-2024-47561, CVSS 9.8) stayed on the runtime classpath after being dropped. The detector was
shown failing before it passed.
Why the flip is not here
Scala 3 cannot compile against Lift's
KeyedMapper/KeyedMetaMapperhierarchy, which roughly140 entity classes extend. Full evidence in
docs/scala3-lift-mapper-blocker.md; the short version:Mapper[A]is fine — the failure is confined to the F-bounded keyed half.IdPK, and not by theobject X extends class Xidiom — so rewriting howthe entities are spelled cannot fix it. That is the expensive route somebody would try first.
_3does not escape it: compiling Lift's own sources turns theassertion failure into 42 cyclic errors in the same construct. Two symptoms, one problem.
TypeTaglooked like a blocking API change and is not — the tag is only stored, neverintrospected, and no consumer reads it, so
ClassTagis a drop-in (95 → 79 errors). This sharesa root cause with the plan's F-1 item.
The document also records what was tried and failed, so it is not retried: four synthetic
models that all compile clean, and two direct fixes on the fork that moved nothing.
Decided: Doobie first. Of the remaining routes — patching Lift's core type structure in our
fork, keeping the entity layer on 2.13, or migrating persistence off Lift — the one taken is to
remove Lift Mapper rather than work around it. The flip is not abandoned, it is sequenced after
the persistence migration, because that migration deletes the blocker instead of containing it.
That work is already underway on
lift-mapper-removein theOBP-API-Icopy, with ATMs thefirst table fully off Lift.
Nothing in this PR depends on that sequencing: it pays the 2.13-side debt the flip will need
whenever it happens, and each item stands on its own merits today.
Known CI state
SonarCloud's quality gate fails: new-code duplication 14.8% against a 3% threshold. It is not
a code defect and it is not pre-existing drift — it is the scalatest rename touching 5041 lines
across 358 test suites that were already heavily duplicated.
An earlier commit here (
639133d1c) added exclusions tosonar-project.propertiesand itsmessage says it addressed this. It did not, and the gate failed on that commit too. SonarCloud
runs this project in Automatic Analysis mode, which does not read
sonar.cpd.exclusions— provenby
obp-api/src/test/**/*.scala, listed there long before this branch, whileAPI1_2_1Test.scalastill reports 13.3% duplication. The file now carries a warning to that effect.
Making exclusions effective needs either SonarCloud project settings (Administration → Analysis
Scope) or a scanner step in CI. Both are outside this PR.