build: pay the Scala 3 debt on 2.13 (flip sequenced after Doobie) - #91
Closed
hongwei1 wants to merge 476 commits into
Closed
build: pay the Scala 3 debt on 2.13 (flip sequenced after Doobie)#91hongwei1 wants to merge 476 commits into
hongwei1 wants to merge 476 commits into
Conversation
hongwei1
changed the title
hongwei1
changed the title
|
|
hongwei1
force-pushed
the
build/scala-3-migration
branch
from
August 17, 2026 17:07
074957c to
f4b79eb
Compare
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Every _3 dependency pulls in its own scala3-library_3 transitively, each at
whatever version it happened to build against (observed range 3.0.0-3.3.6
across the dependency tree), and Maven's mediation was resolving that down
to 3.3.4 - older than the 3.3.8 compiler actually compiling this module's
sources.
The mismatch is not cosmetic: the first full-suite run under the flipped
compiler hung every shard indefinitely at startup with no output past
classloading, silently, well before any test could log anything. The only
clue was a JVM warning naming scala.runtime.LazyVals - the class Scala 3
uses to implement `lazy val` initialization - loaded from
scala3-library_3-3.3.4.jar. That machinery changed its locking
implementation more than once across the 3.3.x line; running bytecode
compiled for 3.3.8 against 3.3.4's runtime support class is exactly the
kind of mismatch that would deadlock lazy val initialization rather than
fail cleanly. This migration's own changes made that failure mode likely:
OBPRestHelper.version/versionStatus and Saveable.value both went from
strict to lazy val in the previous commit, and both sit on the CallContext/
Connector initialization path every request and every test touches.
A direct dependency in obp-api's own pom overrides the transitive version
regardless of tree depth, pinned to ${scala.compiler} so the two can never
drift apart again. Verified: `mvn -pl obp-api dependency:tree` now resolves
a single scala3-library_3:3.3.8 with no other version anywhere in the tree,
and `mvn -pl obp-api -am test-compile` stays clean.
sensitivePatterns' initializer calls APIUtil.getPropsAsBoolValue, the first touch of APIUtil$ on that thread, which triggers APIUtil$'s own class init - eagerly evaluating publicAppUrlDefaults, which logs a debug message, which (every log call in MdcLoggable routes through maskSensitive) calls back into maskSensitive -> sensitivePatterns, on the same thread, before the first call returns. Scala 2's lazy val used a reentrant `synchronized` block, so this recursive call silently passed through; Scala 3's LazyVals uses a CountDownLatch, which is not reentrant, so the thread deadlocks waiting on a latch only it could count down - every test shard hung identically at startup. A ThreadLocal flag detects that one-time computation window and returns the message unmasked rather than recursing; everything logged in that window is APIUtil/SecureLogging's own prop-driven startup, not request data.
…sLazy miss getFieldValues filters candidate members by isLazy/isVal/isVar, which read from Scala's own declaration metadata (ScalaSig for Scala 2, TASTy for Scala 3). scala.reflect.runtime.universe - the Scala 2.13 reflection library obp-commons is pinned to - has no TASTy reader, so all three come back false for every member of a Scala-3-compiled class; only the bytecode-level shape (a zero-arg method with a return type) survives. ExampleValue.exampleNameToValue (a Scala-3-compiled obp-api object, scanned via this function) resolved to an empty map as a result, and any downstream lookup against it threw at initialization time. The added clause recovers that shape, but it also matches a case class's own synthetic zero-arg methods (toString, hashCode, productArity, ...), so it is only safe on a target whose zero-arg methods are all real fields - true for the plain objects getFieldsNameToValue scans, not for an arbitrary case-class instance. JSONFactory1_4_0's generic field-extraction branch reads a case class instance's fields for JSON schema output, so it is switched to scala.Product's productElementNames/productIterator instead, which needs no reflection metadata at all and can't pick up synthetic methods by construction.
…companion values ran eagerly, in OBPEnumerationBase's own constructor - while the concrete companion object (e.g. AuthenticationType$) is still mid-<clinit>, before the JVM marks the class initialized. Scala's runtime reflection, asked to inspect that same not-yet-initialized class from inside its own construction, silently returned every member symbol with isModule false, so decls.filter(_.isModule) found nothing and the assertion threw at <clinit> time - reproduced identically for a Scala-2-compiled companion once isolated, so this is a self-reflection-during-own-<clinit> problem, not a Scala-3/TASTy one. Deferring to first external access (lazy val), after the class is fully initialized, gets decls.filter(_.isModule) back to finding the right symbols. Symbol -> runtime instance still can't go through mirror.reflectModule(sym).instance though: for a Scala-3-compiled nested module, ModuleMirror's own name resolution reconstructs the wrong binary name and reflectModule throws ClassNotFoundException. That step is done by hand instead - a nested object's binary name is always "<outer's binary name><simple name>$" regardless of which Scala version compiled it, and loading it plus reading its MODULE$ field is reliable either way. Order is a narrower, separate caveat: decls preserves source declaration order for a Scala-2-compiled companion (asserted on by OBPEnumerationTest), but not for a Scala-3-compiled one, where it comes back in some other deterministic order instead. AuthenticationType, the only Scala-3-compiled subclass today, uses values only as an unordered set, so this doesn't bite in practice; documented inline for whoever adds the next one.
…Sig poisoning Four layered problems, found and fixed in sequence, all on the path to letting json4s actually extract/decompose a Scala-3-compiled case class: 1. org.json4s:json4s-native_3:4.1.0-M8 (Maven Central's last publish there, 2024-11-07) still throws "Can't find ScalaSig for class ..." extracting a Scala-3-compiled case class. The project moved to io.github.json4s after M8; 4.1.1 there is the first stable release with a working Scala-3 quotes-based reflector (core/src/main/scala-3/.../ScalaSigReader.scala upstream). Root pom, obp-commons and obp-api all repoint to the new groupId. 2. obp-commons (permanently Scala 2.13) and obp-api (now Scala 3) end up needing both json4s-native_2.13 and json4s-native_3 transitively, and both publish org.json4s.reflect.ScalaSigReader under the identical class name with a different implementation - the _2.13 one has no notion of the Scala-3 quotes-based reader. Whichever jar the classloader resolves first wins for every caller, and it consistently picked _2.13, silently reintroducing the ScalaSig error for Scala-3-compiled classes even with 4.1.1 on the classpath. obp-api excludes json4s-native_2.13 transitively from obp-commons, forcing the whole reactor onto _3 - the "verify binary compatibility" step the json4s-scala3-matrix memory flagged as the fallback if json4s's own upgrade wasn't enough on its own (it wasn't). 3. That exclusion breaks obp-commons's own json4s code in turn: JsonSerializers.scala's CustomFormats (extends DefaultFormats) and its Serializer[T] implementations are Scala-2.13-compiled trait mixins: Scala 2 compiles a trait's field initialization through a $init$ forwarder method that Scala 3 does not generate, so a $init$ call against the _3 jar's Formats/Serializer throws NoSuchMethodError at runtime. JsonSerializers.scala and JsonBoxSerializer.scala move into obp-api (code.api.util) so they get compiled by Scala 3 against the Scala-3 json4s jar they now depend on; JsonAble - a plain interface with no trait-field-init concerns, mixed into several obp-commons case classes that must stay in obp-commons - splits out to its own file and stays behind, and the two Scala-3-only TypeTag call sites the move exposed (ru.typeOf[optional], ru.typeOf[Mapper[_]], plus a handful of ru.typeOf[Long/Double/...]) go through ReflectUtils.forType instead. 4. Even with the right reflector selected, a ScalaSig-pickled override whose signature names org.json4s.JsonAST.JValue directly is unreadable from a Scala-2.13-compiled class once json4s-native_2.13 is off the classpath - reflecting anywhere near it throws "unsafe symbol JValue (child of class JsonAST) in runtime reflection universe". Every obp-commons toJValue override (ApiVersion, CommonModel's CounterpartyLimit, Enumerations' SimpleEnum, RequiredFieldValidation's three implementors) is repointed at the json.Formats/json.JValue aliases in JsonAliases.scala instead of importing org.json4s directly - those aliases are themselves Scala-2.13-compiled symbols in the same package, so they stay resolvable, and the method bodies are unaffected since json.JValue =:= org.json4s.JValue. Left over after all four: json4s's Scala-3 ScalaSigReader.readField (used to recover a generic field's erased type argument, e.g. an Option[String]) throws NoSuchElementException: None.get introspecting a Scala-2.13-compiled DTO - a genuine upstream limitation, unchanged between json4s 4.1.1 and 4.2.0-M4. RabbitMQConnector_vOct2024's two ResourceDoc examples that hit it (Extraction.decompose on InBoundOpenCorridorCreditNotificationData / InBoundOpenCorridorSettlementData, both obp-commons DTOs with Option fields) are hand-built with the JsonDSL ~ operator instead, since they are fixed example literals, not runtime data. ConnectorUtils.scala and MSsqlStoredProcedureBuilder.scala have generic Extraction.decompose(Any) call sites that can hit the same bug on arbitrary connector messages and still need a real fix, not a one-off literal rewrite.
|
Its own docstring promised "call by name methods OR val values", but the code only ever handled the method branch: it asserted methodSymbol.isMethod unconditionally, which was true for every Lift Mapper entity this was written against (Mapper exposes every column as a call-by-name accessor def). Post-Mapper-to-Doobie migration, an entity like MappedBankAccount is a plain case class, so its fields (e.g. accountPrimaryKey) are ordinary constructor vals - isMethod is correctly false for them, and the assertion threw "accountPrimaryKey is not method in Object MappedBankAccount(...)" for every one, breaking findBankIdIn and cascading into a 500 on every connector call that routes through it (surfaced here via saveHistoricalTransactionViaEndpoint, but not specific to it). Not a Scala-3-reflection gap like the isVal/isVar/isLazy ones elsewhere in this file: confirmed via an isolated diagnostic that isVal/isMethod are both read correctly here, they just don't match what this function assumed. Added the missing val/var branch, reading the field directly instead of searching for a zero-arg method alternative.
…" field Copying a same-named parameter from a source object to a target constructor worked by reflecting it as a method; when the source symbol wasn't a method it fell back to reflecting a completely unrelated hardcoded field named "attributes" instead of the actual parameter. That fallback firing at all depended on the source field genuinely not being a call-by-name method - true for whatever original case needed it, but also true for any ordinary case class constructor val, which is what most source objects going through ConverterWithType.toCommons actually are. Every plain val parameter hit the "attributes" fallback, and once no source object actually had an "attributes" member either, that threw ScalaReflectionException: <none> is not a method - surfaced here via createDynamicEntityImpl's use of ConverterWithType, 500ing customer/bank dynamic entity creation across almost every DynamicEntityTest scenario. Same shape as the getCallByNameValues fix: added the missing val/var branch, reading the field directly instead of assuming every source member is a method. The "attributes" fallback is kept as the last resort, unchanged, for whatever case actually relies on it.
…f converting it NewStyle.function.getDynamicEntities returns List[DynamicEntityT] - the provider trait - not List[DynamicEntityCommons]. Seven call sites across Http4s400/Http4s600 (system/bank/my dynamic entity listing endpoints) took that as List[DynamicEntityCommons] via a blind asInstanceOf, relying on the runtime instances happening to already be DynamicEntityCommons. They threw ClassCastException as soon as that stopped being true, 500ing every affected DynamicEntityTest scenario. DynamicEntityCommons's own companion is a ConverterWithType[DynamicEntityT, DynamicEntityCommons] - it exists specifically to do this conversion via reflection (the same ReflectUtils.toOther machinery fixed for Scala 3 case-class-val sources earlier this session), so the cast is replaced with DynamicEntityCommons.toCommonsList(...) instead of asInstanceOf.
…tching, getEnumContainer Three more reflection gaps the full suite surfaced beyond the 5 originally-crashing classes (all previously masked by the boot crash never letting these code paths run): 1. SwaggerTypes.scala used typeOf[JObject] et al. directly - needs TypeTag synthesis for org.json4s's own type symbol, which scala.reflect.runtime.universe can't resolve once json4s-native_2.13 is off obp-api's classpath (same ScalaSig-poisoning shape as the earlier json.* alias fixes, but via typeOf instead of a plain import). Rewritten to build every json4s-involving Type from ReflectUtils.forType + ru.appliedType instead of typeOf, which needs no TypeTag synthesis. Also fixed the class names themselves: org.json4s.JsonAST.JObject doesn't exist as a loadable class in json4s 4.x - JsonAST is a legacy compatibility object, the classes live directly under org.json4s. 2. getFieldValues' widened zero-arg-method filter (from the isVal/isVar/isLazy fix earlier this session) matched too much: java.lang.Object's notify()/wait() (reflectMethod on those outside a synchronized block throws IllegalMonitorStateException), scala.Any's asInstanceOf/ isInstanceOf (reflectMethod refuses to invoke a generic method at all), and even an unrelated JDK-internal interface method on whatever object JSONFactory1_4_0's unfiltered fallback branch happened to be looking at. Restricted the clause to it.owner == tp.typeSymbol - a member actually declared on the target's own class, never one it merely inherits - which excludes all of the above at once instead of naming bad owners one at a time. 3. OBPEnumeration.getEnumContainer (both overloads) used knownDirectSubclasses.head to find any one enum value and walk back up to its companion. knownDirectSubclasses reads Scala's own declaration metadata the same way isVal/isVar/isLazy and OBPEnumerationBase.modules do, and a Scala-3-compiled sealed trait reports zero known subclasses to it - .head then threw NoSuchElementException. The function never needed a subclass in the first place, only the companion - and a companion's binary name is always "<trait's name>$" regardless of compiler, the same technique OBPEnumerationBase.modules already uses. getValuesByInstance's own getInterfaces().headOption lookup (used to find which trait an enum value implements before calling getEnumContainer) got a matching fix: Scala 3 doesn't always list an EnumValue-derived trait before EnumValue itself, so headOption sometimes returned EnumValue - now finds the interface that extends EnumValue without being it. Combined verification (the 5 originally-crashing classes + SwaggerFactoryUnitTest + ResourceDocsTechnologyTest): 375/377, no regressions from any of these three changes. ResourceDocsTechnologyTest's remaining 2 failures are a separate, still-open issue - see project_scala3_json4s_extraction_saga memory.
…tEnumContainer cast bug Two more root causes on the same "resource docs generation" path, found chasing the same 2 scenarios through successive fixes: 1. JSONFactory1_4_0's unfiltered getFieldValues fallback branch reflected over whatever non-Product AnyRef it was given, with no check that the value was ever meant to be schema-relevant OBP data. Somewhere in the v5.0.0/v6.0.0 resource doc tree this reached a genuine java.util.stream.ReferencePipeline object, and getFieldValues' isMethod-shaped filter (declared directly on the target's own class, so not excluded by the owner check from the previous commit) picked up its internal opIsStateful() method, which reflectMethod can't invoke without an InaccessibleObjectException. Restricted the branch to ReflectUtils.isObpObject(entity) - a JDK type was never meant to reach this code path at all. 2. Underneath that: OBPEnumeration.getEnumContainer's return type was hardcoded to OBPEnumeration[T]. AuthenticationType (obp-api, Scala 3) extends OBPEnumerationWithType[T], a sibling of OBPEnumeration[T] under the shared OBPEnumerationBase[T] - not a subtype of it - so the function's own .asInstanceOf[OBPEnumeration[T]] threw ClassCastException the moment it was ever asked to resolve AuthenticationType specifically (a live bug independent of this session's knownDirectSubclasses fix, exposed by it rather than caused by it, since OBPEnumerationWithType didn't exist until AuthenticationType was introduced). Every caller only uses values/withNameOption/withIndexOption/example, all declared on the shared OBPEnumerationBase, so widening the return type to it is a pure fix with no narrowing loss. SwaggerFactoryUnitTest + ResourceDocsTechnologyTest: 13/13, both fully green.
… frozen test RestConnector_vMar2019_FrozenTest aborted its whole shard with Symbols$CyclicReference: illegal cyclic reference involving class BodyPartParser. Bisection showed this is not one bad member among many: any attempt to resolve RestConnector_vMar2019's own scala-reflect Type - decls, baseClasses, or asking any of its members whether they override something - forces scala.reflect.runtime to complete an unrelated symbol reachable from its inheritance chain, and that completion fails inside the JVM-wide shared symbol table, not inside the specific symbol being asked about. Per-symbol try/catch around the caller's own loop cannot catch it, no matter how granular. Route around it instead of trying to catch it: get the override-eligible method names from Connector's type, which is a plain OBP domain trait and resolves cleanly, then cross-reference against RestConnector_vMar2019's own declared methods via java.lang.Class reflection, which never touches scala.reflect.runtime.universe and so cannot hit this. Verified byte-for-byte equivalent to the old list against the persisted frozen metadata file.
SwaggerScalarFieldTypeTest and SwaggerOptionFieldTypeTest declared their fixture case classes nested inside the AnyFlatSpec test class. SwaggerJSONFactory. translateEntity reflects on a fixture value's runtime type via scala.reflect.runtime.universe, and for a nested case class that also means resolving the enclosing class - here a ScalaTest suite mixing in Assertions. Walking that unrelated third-party hierarchy throws (AssertionError: no symbol could be loaded from class org.scalatest.Assertions$UseDefaultAssertions$, and separately illegal cyclic inheritance once the enclosing class's own type becomes unresolvable). Moving the fixtures to file scope removes the enclosing class scala-reflect would otherwise have to walk. No production code changed.
…la 3 ToolBox is Scala 2.13-only and cannot read TASTy, so it fails on any dynamically-compiled snippet that references obp-api's own Scala-3-compiled classes (OBPReturnType, HttpCode, and friends injected via DynamicUtil.importStatements). This was the largest remaining source of test failures after the Scala 3 flip. DotcScalaCompiler drives dotty.tools.dotc.Driver directly: each distinct source is wrapped as a synthetic top-level object compiled to a temp directory against the running JVM's own classpath, then loaded with a URLClassLoader parented on the current thread's context classloader. Two things the wrapping has to get right, both found by running the actual test suite rather than assumed up front: - top-level type definitions (case class/class/object/trait/enum) are hoisted out as direct siblings of the result method rather than nested inside its body, because json4s's Reflector refuses to extract into a case class "defined in function bodies" - exactly what a naive whole-snippet block wrap would produce for DynamicUtil.toCaseObject's generated case classes. - the result accessor is a def, not a val: a runtime-compiled dynamic endpoint body can contain a `return` inside a nested lambda (recovered later as a NonLocalReturnControl by Sandbox.runInSandboxIO), and that only compiles when there is a real enclosing method to target. A val compiles to a bare field initializer with no such method. Compiled classes are kept on disk for the JVM's lifetime (registered for delete-on-exit rather than deleted immediately after evaluation): a class referenced only inside a returned closure's body is linked lazily, on first invocation of that closure, which can happen long after compilation returns - deleting the output directory right after evaluate() intermittently broke exactly that case. Verified: DynamicUtilTest, DynamicCompilerKillSwitchTest, DynamicCompilerFourChainPocTest, ConnectorMethodTest, EndpointMappingTest, EndpointMappingBankLevelTest, DynamicMessageDocTest, DynamicResourceDocTest, DynamicCodeKillSwitchTest, MethodRoutingTest, ConnectorProxyObjectMethodsTest. Four remaining failures (EndpointMappingTest, EndpointMappingBankLevelTest, MethodRoutingTest, ConnectorProxyObjectMethodsTest) are unrelated pre-existing issues - confirmed by ClassCastExceptions and reflection mismatches that occur entirely within the same classloader, with no DynamicScalaCompiler involvement in their stack traces.
…able by name InternalConnector.methodNameToSymbols filters Connector's decls with !t.isVal && !t.isVar to decide which members dynamic code may implement. Connector is now Scala 3-compiled and read via Scala 2.13's scala.reflect.runtime.universe, under which isVal/isVar report false for it - a val getter and a zero-arg def compile to the identical JVM shape, so the distinction is source-level information a non-TASTy reader cannot recover from any flag. messageDocs leaked through as if it were a genuine connector method, throwing IllegalStateException instead of falling through to the stub path. Connector's trait body declares exactly two public vals directly (formats, messageDocs; everything else at that scope is protected and already excluded by the isPublic check, which reflects a JVM access modifier and reads correctly across the compiler boundary). Excluded both by name.
Four response-serialization sites in Http4s310/Http4s400 blindly cast a provider-returned List[XT] (the trait) to List[XCommons], on the assumption that the provider only ever constructs XCommons. That assumption no longer holds where a Doobie-migrated provider (MappedMethodRoutingProvider, DoobieCardAttributeProvider) returns its own row type implementing the same trait: management/method_routings, management/endpoint-mappings, management/banks/BANK_ID/cards/CARD_ID, and management/webui_props all threw ClassCastException instead of serving a 200/201. Each affected XCommons companion already extends Converter/ConverterWithType for exactly this purpose - the singular per-item getters in the same files already used its implicit toCommons conversion correctly. Switched the four list sites to the same companion's toCommonsList instead of casting.
… gap json4s's default Reflector-based decompose calls ScalaSigReader.readField (a Scala 3 compiler staging run) whenever a field's generic type argument is erased to java.lang.Object on the classfile - the case for an Option[T] field where T is a primitive value type. readField only knows how to recover that argument from TASTy, so it throws NoSuchElementException: None.get for any Scala-2.13-compiled class, since none of them carry TASTy. This hits any obp-commons DTO with such a field, directly or nested arbitrarily deep inside another obp-commons value being decomposed (e.g. every OutBound message embeds OutboundAdapterCallContext -> User, and User.isDeleted: Option[Boolean] is exactly this shape), and was crashing code.api.v3_0_0.ViewsTests (via the UpdateViewJSON body built on the test side) and code.api.v2_2_0.API2_2_0Test's Get Message Docs scenarios (via connector example messages) with a server-side 500. Add ObpCommonsProductSerializer, registered last in JsonSerializers.serializers so every more specific existing serializer still gets first refusal. It builds the JSON for any com.openbankproject.commons case class from ReflectUtils.getConstructorArgs instead of the default Reflector - ReflectUtils reads Scala-2.13-compiled classes through their own compiler's runtime reflection, which has no TASTy dependency. Because it is registered in the shared Formats chain rather than invoked at one call site, nested obp-commons values get the same treatment automatically at any depth. Deliberately scoped to the com.openbankproject.commons package rather than also matching obp-api's own classes, which are Scala 3-compiled and reflecting those via scala.reflect.runtime.universe has its own, unrelated set of gaps.
… 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.
1. The mappedconsent join had no guard against the empty-string sentinel. opt() in MappedConsent.scala stores an empty consent_reference_id as '' rather than NULL, and every non-consent metric row also defaults to consent_reference_id = ''. An unguarded `ON m.consent_reference_id = c.consent_reference_id` therefore joined ALL non-consent rows to a single legacy/blanked consent row whenever one existed, and COALESCE(c.muserid, ...) attributed the estate's entire non-consent traffic to that one unrelated user. Add `AND m.consent_reference_id <> ''` to the join condition in buildAggregateMetricsQuery and buildTopUsersQuery - the same fix the NULLIF(..., '') calls already apply on the read side. 2. buildFilterConditions' user_id filter always bound to the raw metric.userid column, even in the two queries whose SELECT/GROUP BY attributes a consent-borne call to the granting human via COALESCE(c.muserid, m.userid). Filtering by a human's user_id excluded exactly the consent-borne calls the endpoint claims to attribute to them (their metric.userid is the consent's own shadow user), while filtering by the shadow user's id returned rows displayed under a different (the human's) identity. Add a resolvedUserIdExpr parameter, defaulting to the previous behaviour for callers with no consent resolution in play, and pass the COALESCE expression from buildAggregateMetricsQuery and buildTopUsersQuery.
Two independent gaps in updateMyMobilePhoneNumber and the mobile number on POST /users: - The regex character class is a union, not a required sequence, so " " (five spaces), "((.))" and "-.-.-" all matched despite the ResourceDoc promising "5 to 50 digits, spaces, dashes, dots or parentheses". A digit-free string would be stored as the user's mobile number with nothing for the later validation/SMS flow to send to. Require at least five actual digits alongside the shape check. - updateMyMobilePhoneNumber wrote straight to the authenticated principal with no check for a consent user. Under a Consent, cc.user is the consent's own shadow ResourceUser by default; letting that identity overwrite the mobile number - an authentication channel used for validation codes and SMS OTP - would let an agent repoint the granting human's second factor. Refuse it outright rather than silently redirecting to the resolved human, since a silent redirect here would let the agent change a security-relevant field the caller has no reason to believe they don't have permission to change.
getTopUsers and getTopConsumers called createQueriesByHttpParamsFuture directly on the raw request params instead of going through APIMetrics.applyMetricsFromDateDefault the way every other metrics-reading endpoint does. With no from_date, APIUtil.getFromDate substitutes the epoch, which makes MappedMetrics.determineMetricsCacheTTL classify the query as "only stable data" and pick the 24-hour TTL - so the default, no-parameter call an operator dashboard would make froze for a day while traffic kept arriving, and the first miss of that day scanned the whole metric table since 1970. Corrected the two ResourceDoc descriptions to match (they claimed "defaults to one year ago" / "the current date", which was never the actual range).
createAccountJSON's Links.Self does list.head.AccountId unconditionally. getAccount builds that list by filtering the caller's own private accounts down to the requested accountId, which is legitimately empty for an id that does not exist (or belongs to someone else) - the same shape a real TPP integration hits on a typo or a stale id. That empty list reached list.head and threw NoSuchElementException, answering 500 instead of the 404 UK Open Banking's spec calls for. Found by extending the endpoint auth/crash sweep to cover Berlin Group and UK Open Banking (previously OBP-standard only) - FailureSweepTest calls every endpoint with a nonexistent id and asserts none of them 5xx.
…n Banking
EndpointCatalog.all was Http4s700.allResourceDocs - the OBP-standard
aggregation only. AuthSweepTest, SuccessSweepTest and FailureSweepTest all
read their coverage from it, so every Berlin Group and UK Open Banking
endpoint was silently outside the anonymous-401/crash sweep: a doc in
those standards missing AuthenticatedUserIsRequired with empty roles would
let anonymous callers reach account data and nothing would catch it.
Switching to ResourceDocRegistry.allStaticResourceDocs (the same
cross-standard union APIUtil.getAllResourceDocs already exposes) needed
three follow-on fixes, all specific to a catalog that now spans multiple
independent route trees rather than one:
- EndpointCatalog.concretePath hard-coded "/obp/" + apiShortVersion.
Berlin Group and UK Open Banking routes match on Root / urlPrefix /
apiShortVersion with no "/obp" segment at all (see e.g.
Http4sBGv13AIS.bgV13Prefix) - urlPrefix is "obp" for the OBP standard by
construction (ApiVersion.setUrlPrefix patches it to the configured
apiPathZero at boot), so using implementedInApiVersion.urlPrefix
uniformly reproduces the old OBP behaviour while giving BG/UK their own
real prefix instead of a path that 404s before reaching any route.
- AuthSweepTest.messageOf only read the top-level "message" field. Berlin
Group requests get a PSD2-mandated {"tppMessages": [{"text": ...}]}
envelope instead (ErrorResponseConverter.toBgErrorBody) - the endpoint
was correctly answering 401, the sweep just could not see the message
text to compare it against. Fall back to tppMessages[0].text, which
carries the identical string the OBP envelope would have.
- SweepCoverageTest's "deduplicated by (url, verb)" check assumed one
route shape maps to one operation, true for OBP but not for Berlin
Group: several SCA sub-steps (e.g. updatePsuAuthentication /
selectPsuAuthenticationMethod / transactionAuthorisation) legitimately
share one URL and verb, disambiguated by request body rather than path.
Replaced with a check on operationId uniqueness, which is the union's
actual by-construction guarantee and still catches a genuine duplicate
(e.g. two ResourceDoc objects registered under the same operation id).
Two categories of endpoint answer non-2xx to SuccessSweepTest's
fully-entitled-but-consentless caller and are documented in
expectedNon2xx rather than treated as failures: Berlin Group AIS and UK
Open Banking account-read endpoints both require an established,
standard-tagged consent regardless of role, which the sweep's generic
fixture (grants every role, creates no consent) does not provide. The
403 in both cases is the endpoint correctly refusing, not a defect - the
anonymous case is what AuthSweepTest already covers independently.
- Updates to signal channels with added gRPC service.
…tired-lift-standards refactor: remove retired API standards and superseded Lift stubs
# Conflicts: # obp-api/src/main/scala/code/api/v6_0_0/APIMethods600.scala
Also glossary item to explain the three rate limiters.
v6.0.0 -> v5.1.0 etc.
activity. Ability to change dynamic entity e.g. indexing if change doesn't change structure / field names.
apiTagSignalChannel + dyanmic code Props
Glossary item related
…ie stores
develop's 73 commits here are four features and one large teardown.
The features: maker/checker for runtime-supplied code (a Dynamic Change Request holds a
proposed create/update/delete of a dynamic resource doc, connector method, message doc or ABAC
rule until a second person approves its payload hash, and the runtime executes only rows whose
body hash equals the approved one); API Product Subscriptions (one Consumer holds one product
for a period, with a status machine and the Scope and RateLimiting rows it created); Dynamic
Glossary Items, which an operator adds at runtime and which may shadow a static item; and the
on-behalf-of attribution framework, which renames accountableUserId to onBehalfOfUserId, pulls
the resolver into Users.onBehalfOfUserIdOf, and states in UserReference.scala, per column,
whether a write by a consent user is recorded against the consent user or the human it acts for.
The teardown: the 12 OBPAPIx_y_z aggregator objects and the VersionedOBPApis trait are gone,
along with the APIMethodsXYZ.scala shims. Version enumeration is now explicit in
Http4sResourceDocAggregation.allVersions, and the Lift ResourceDoc text those files carried as
comments was exported first to scripts/resource_doc_baseline/*.json, which is what the parity
audit now reads.
All four features were written against Lift Mapper entities and created their tables through
Schemifier. This branch has neither: ToSchemify.models is empty, so a Mapper column here would
compile and then not exist. The resolution carries the behaviour across instead of restoring the
entities - DynamicChangeRequest, DynamicGlossaryItem, ApiProductSubscription, its Scope join
table and its Attribute table are case classes over Doobie stores, MakerChecker's 33 Mapper call
sites read through the Doobie providers, and the five tables plus the approvedhash/isactive
columns on the four target types and dynamicentity.authmode come from a new
db.changelog-develop-merge-2.yaml.
Three of develop's entries in the new attribution policy name entities this branch does not
have, and one names a column it does not have:
- 16 of the 65 classes UserReference.scala names are Lift Mapper entities this branch already
moved to Doobie. They are only ever read to build the log line in Users.attributionOf, but
AgentDelegationTest resolves each one, so they are repointed at the class that actually owns
the write here, with the real column names beside them.
- PemUsageLastUser and UserScopeUser are dropped: 6bfcb4b deleted the PemUsage scaffold (its
table was never created in any environment) and ac72b13 deleted MappedUserScope (no
callers). A policy entry for a table that does not exist is not a policy.
- abacrule already carries isactive from the baseline, unlike the three dynamic-code tables, so
its maker/checker changeset adds only approvedhash. Liquibase aborted the boot on the
duplicate column, which reads as a test-infrastructure failure rather than a schema one.
Two defects were introduced while resolving conflicts and are fixed here rather than left for CI:
The attribution policy was not actually applied to dynamic data. MapppedDynamicDataProvider
defines ownerOf and never calls it - save, update, get, getAll and existsData all passed the raw
caller id - and MapppedDynamicEntityProvider.createOrUpdate stored dynamicEntity.userId
unresolved. Taking our side of those conflicts kept the Doobie method bodies and dropped the
wrapping develop had put around them, so a consent user's rows and definitions were owned by the
agent rather than by the human it acts for. That is the whole point of the feature, and it is
silent: nothing fails, the row is simply attributed to the wrong user.
Four resource-doc examples published a dangling $ref to Object. A field of an erased type takes
its published type from its example value, so ProvenanceJsonV700.is_active and three of
RateLimiterLimitJsonV700's windows, left as None by develop's examples, had no type to recover.
DynamicChangeRequestJsonV700.current_payload is a separate case: it is a bare JValue and is
genuinely JNothing for a CREATE, which json4s omits from the body entirely - that now publishes
as {"type":"object"}, which is what the null branch already answered for the same field, while
every other declared type with an empty example still fails. buildSwaggerSchema's own failures
named neither the class nor the field, so translateEntity now adds that; it is how the offending
field was found at all.
Alongside: the four table-count assertions move from 147 to 152 for the five new tables, and
run_tests_parallel.sh's per-shard hard kill becomes SHARD_TIMEOUT_SECONDS, defaulting to 2400s
on Postgres. That kill exists to stop a JVM whose Pekko threads hang after the tests finish, not
to bound how long tests may take, and at 1200s three of the four Postgres shards were killed
mid-run - each still emitting test output at the moment it died. A killed shard reports zero
failures while dropping ~800 tests from the audit line, so the run reads as a pass unless the
per-shard durations are checked.
Suite: 4233 tests, 0 failures on H2 and the same 4233 on Postgres, where the three shards now
finish in 20:43, 21:58 and 22:39 - which is what the old 20-minute kill was cutting off.
|
Owner
Author
|
Superseded by OpenBankProject#2899, which reviews the same branch at the same commit (build/scala-3-migration @ ba37e67) against upstream develop. This PR was opened on 2026-08-15, when the branch only carried the 2.13-side preparation; the title and description still describe that scope. The branch has since done the Scala 3 flip, the Doobie persistence migration and the Liquibase changeover, and OpenBankProject#2899 describes the result. Its base, build/scala-2.13-migration, merged via #90 on 2026-08-16, so the retarget this description asked for is moot. Closing to keep one review surface. Nothing is lost: the branch and every commit stay, and OpenBankProject#2899 is open and green. |
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
3 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.