build: pay the Scala 3 debt on 2.13 (flip sequenced after Doobie) - #91
Draft
hongwei1 wants to merge 396 commits into
Draft
build: pay the Scala 3 debt on 2.13 (flip sequenced after Doobie)#91hongwei1 wants to merge 396 commits into
hongwei1 wants to merge 396 commits into
Conversation
hongwei1
changed the title
hongwei1
changed the title
|
One table replaced with a Doobie row case class and a V089 migration reproducing the probed DDL. mparentproductcode models the hierarchy by value, not by foreign key: getProductTree walks it by repeatedly looking up (bankId, parentProductCode) and an empty string terminates the walk. A product with no parent must therefore store "" and never NULL, so that column stays non-nullable while the free-text ones do not. createOrUpdate also reads the existing parent before writing, because the connector only supplies parentProductCode when the caller did — an update that omits it must not reset it. The first attempt failed 19 tests across three shards. Http4s310's createProduct passes termsAndConditionsUrl = null as a literal; Lift's MappedString stored that as SQL NULL, while a bare String binding throws at bind time. The throw was swallowed by the surrounding tryo and surfaced as 404 instead of 201, with nothing in the message pointing at a null. Every free-text column is now bound as Option and read back with orNull, reproducing Lift's round trip. CLAUDE.md's null-binding note gains the write-side case, which is easier to miss than the query-side one because the null is a literal in the caller rather than data. The sandbox importer gains a SaveableProduct that writes through the store, following the SaveableAtm precedent: the import must not write with Mapper when every read comes back through the store. Mapper's field validation there is dropped rather than reimplemented — no validator was ever declared on the product entity, so it always passed. MappedProductsProviderTest's fixtures move from MappedProduct.create to createOrUpdate; its assertions are unchanged.
|
One 53-column table replaced with a Doobie row case class and a V090 migration reproducing the probed DDL. The row is split across three tuples because Scala tuples stop at 22 elements. The connector writes a dozen columns through orNull — mcounty, both branch-routing columns, all fourteen drive-up times, mbranchtype, mmoreinfo and mphonenumber — so those are bound as Option and read back as null, reproducing Lift's round trip. The lobby times are the exception: the connector defaults them to "00:00" and they are never null. Two guards that have never fired are preserved rather than repaired. branchRouting's fallback to "BRANCH_ID" compares the FIELD OBJECT to null and to "" instead of its value, and a MappedString object is neither, so callers have always seen the stored value including null. getBranchLocal's defaulting to "OBP" compares an Option[String] to null, which is likewise always false. Correcting either would change what every caller of an unrouted branch receives. The first attempt failed CreateBranchTest: the generated UPDATE excluded the two key columns from its SET list by filtering chunks of four rather than individual columns, so mname and mline1 were never written and an update silently kept the old name. Only mname had a test watching it. The generator now asserts that every non-key column appears exactly once in the SET body. The sandbox importer gains a SaveableBranch writing through the store, following SaveableAtm and SaveableProduct. Mapper's field validation is dropped rather than reimplemented — no validator was ever declared on the branch entity. MappedBranchesProviderTest's fixtures set a handful of fields and relied on MappedString's "" default for the rest; a local helper now passes the unset columns explicitly.
One table replaced with a Doobie row case class and a V091 migration reproducing the probed DDL. The table keeps its MAPPER prefix, which is unlike every other table here and is now stated in the migration. The unique index on (user_c, accountbankpermalink, accountpermalink) is load-bearing: getOrCreateAccountHolder is a check-then-insert that relies on the database rejecting a concurrent duplicate so the loser can re-read the committed row. Without it a user could be recorded twice as holder of one account and one revoke would leave the other behind. source genuinely holds NULL and getAccountsHeldByUser branches three ways on it — no filter, IS NULL, or an exact match — so the column stays nullable and all three branches are preserved. The first attempt failed two API1_2_1Test revoke scenarios with a 500: canRevokeOwnerAccess looks holders up by a ViewDefinition's bankId and accountId, and a SYSTEM view has neither, so both arrive as null. Lift rendered that as `= NULL` and returned no rows; a bare String binding throws instead. Every string binding in the file is now Option, with the reasoning at find. CLAUDE.md's null-binding note gains the rule this keeps violating: audit the callers for literal nulls and for identifiers that are optional in the domain BEFORE writing the store. Three migrations in a row compiled, passed their targeted suites, and failed the full run on a null arriving from a call site that had not been read.
One table replaced with a Doobie row case class and a V092 migration reproducing the probed DDL. First table done under the caller-audit rule added to CLAUDE.md: grepping the callers before writing the store turned up .BankId(bankId.getOrElse(null)) immediately, so bankid was bound as Option from the start rather than after a failing full run. Green first time. process is unique globally rather than per bank, the same shape as endpointmapping.operationid: a bank-level and a system-level doc cannot share a process name, and the optional bank id narrows a read without being part of the key. The reads are inconsistent with the write and stay that way: a supplied bank id filters on bankid, while an absent one does not constrain it at all, so a system-level lookup also matches bank-level rows. That is expressed once in a bankFilter helper instead of being re-derived at each of the five call sites, and stated in the migration. bankId is not written on update — Mapper did not set it either, so a doc cannot move between system and bank scope after creation.
One table replaced with a Doobie row case class and a V093 migration reproducing the probed DDL. Green first time; the caller audit found all three nulls up front — bankid, examplerequestbody and successresponsebody — and they are bound as Option before anything was written. The two optional JSON bodies matter beyond not throwing: the reader filters blank before parsing, so an absent body has to stay NULL rather than become "", or the column's meaning would depend on the reader instead of the data. This provider differs from the near-identical dynamicmessagedoc one that landed just before it, and both are preserved as they were: this update DOES write bankId, and its lookup deliberately ignores bankId so an update addressed by id finds the doc whatever its scope and then rescopes it. Migrating the two back to back makes them easy to harmonise by accident. The unique index on (requesturl, requestverb) is global rather than per bank, so a bank-level and a system-level doc cannot claim the same route. roles is stored as roles_c because ROLES is a SQL reserved word.
One table replaced with a Doobie row case class and a V094 migration reproducing the probed DDL. Green first time. ProjectionStore was reading this table's name and three column names off Lift's metadata to build raw SQL for its user-scoped EXISTS joins — a dependency on the ORM rather than on the data. Those four names are now constants on the companion so the DDL and that hand-built SQL cannot drift apart. bankid is the NULL-vs-empty case again: both scoped queries use IS NULL when no bank is supplied, matching Lift's NullRef rather than "no filter", so a row storing "" would be invisible to them. Routed through one scopedBank helper. The revoke walk is preserved intact — it follows GrantedBy edges to remove the target user and everyone they granted downstream, with a visited set that terminates re-share cycles and absorbs the owner row's self-edge. The (dynamicdataid, grantedby) index exists to serve that walk, which the migration now says so it does not read as redundant beside the unique index. The unique index on (dynamicdataid, userid) is what makes grant an upsert rather than an append and lets allows answer with a single lookup.
Brings in the nine commits upstream added after this branch was cut - chat message constraints and email digest, the password-policy endpoint, signal channel sanitizing, and the Sonar annotations - so the branch is tested as it will merge rather than as it stands alone. They were written on 2.12. Compiling and testing them under 2.13 is the point of merging here rather than leaving it to the merge button: a long-lived branch being green on its own says nothing about the merge, which is what the pull_request build actually compiles.
One table replaced with a Doobie row case class and a V095 migration reproducing the probed DDL. Green first time. All three dynamic-* providers scope differently, and each keeps its own behaviour. dynamicmessagedoc and dynamicresourcedoc leave bankid unconstrained when no bank id is supplied, so a system-level lookup also sees bank-level rows; this one uses IS NULL, so it does not. Having migrated the other two immediately before, the difference is easy to harmonise by accident, so it is stated in the migration — it is only visible by reading all three. getDynamicEntities keeps its third mode: returnBothBankAndSystemLevel ignores scope entirely and returns every row. delete keeps its two branches. A row we loaded is deleted by its own id; anything that merely names an entity deletes every row with that name. Those are materially different blast radii, so they stay separate rather than being unified on the name. Only dynamicentityid is unique — nothing constrains (bankid, entityname) even though getByEntityName treats that pair as a key, so two entities in one scope may share a name. Recorded with id ASC pinning the lookup.
One table replaced with a Doobie row case class and a V096 migration reproducing the probed DDL. This clears the whole code/dynamicEntity package. bankid and userid both hold NULL but are read differently, and the difference is load-bearing. bankid uses IS NULL for the system-level case, so a system-level query excludes bank-level rows. userid is compared with `= ?` even when the caller passes None, because the provider wrote By(UserId, userId.getOrElse(null)) — Lift rendered that as `= NULL`, which matches nothing, so a personal-entity query with no user id has always returned zero rows rather than every row. That reads like a bug but callers depend on the empty result as an access check; writing it "correctly" as IS NULL would start returning every ownerless personal record. Both behaviours are preserved literally and spelled out at the two helpers and in the migration, since neither is visible without reading the other. The four get/getAll scopes collapse to a personal/impersonal choice over two scoping helpers rather than four hand-written branches, and the community reads keep their own helper — they deliberately ignore owner and personal flag. ProjectionStore was again reading this table's name and six column names off Lift metadata; those are now constants on the companion, as the ACL table's already are. Http4s600's orphaned-record cleanup and the useRowLevelAccess warning both counted rows with hand-built scope filters; both now go through findAllCommunity, which is the scoping they actually wanted.
One table replaced with a Doobie row case class and a V097 migration reproducing the probed DDL. The first attempt failed 12 tests on `oops, null` even though bank_id and account_id were already bound as Option. The failing value was Some(null), not None: a system view loaded from the database carries BankId(null), so Some(view.bankId.value) wraps a null, and Doobie unwraps the Some and hands the non-nullable Put that null. Lift's By(field, null) rendered `= NULL`. The scoping helper now collapses Some(null) to None with flatMap(Option(_)), and CLAUDE.md's null note gains this case — binding as Option is necessary but not sufficient when the Option itself can wrap a null. The unique index on (bank_id, account_id, view_id, permission) is what makes a permission single-valued per view, and resetViewPermissions depends on it: it deletes the view's rows then re-inserts each permission inside a Try so a concurrent reset is absorbed by the constraint. That holds for CUSTOM views only — H2 and Postgres treat NULLs in a unique index as distinct, so for SYSTEM views, where both id columns are NULL, the constraint never fires and two concurrent resets can both insert. Pre-existing; recorded in the migration. bulkDeleteAllAccountAccessAndViews scopes its view and access deletes to one account and then deletes EVERY view permission in the system. That over-reach is pre-existing and is marked at the call site rather than narrowed, since narrowing changes what a caller's cleanup destroys.
The authorisation link between a user and a view, replaced with a Doobie row case class and a V098 migration reproducing the probed DDL. 56 call sites across 17 main files and eight test files. All five columns of the unique index are load-bearing, and the migration says so: revokeAccess matches on (bank, account, view, user) and cannot tell two applications' grants apart, while the per-consumer revokes match on (bank, account, view, consumer) and cannot tell two users apart — on a joint account they would delete whichever row came back first. Undoing one consent's grant needs the whole tuple, which is why deleteRow addresses a row by all five. The table carries a SECOND, dead consumer_id column in deployed databases. Helper.addColumnIfNotExists emits ADD COLUMN IF NOT EXISTS "consumer_id" — quoted, so lowercase and distinct from the CONSUMER_ID Schemifier created. The existence check never matched and MigrationOfAccountAccessAddedConsumerId added a duplicate nothing reads. This migration builds only the live column and explains the twin. Also fixes a latent build breakage introduced at the eleventh table: DoobieTransactionTypeProvider.scala declared package code.transactiontypes beside a file declaring code.TransactionTypes. Those are distinct packages to scalac but the same directory on a case-insensitive filesystem, so the class files overwrite each other and any from-scratch compile fails with "location not matching its contents". Every build since survived only because Zinc's incremental analysis never rescanned that directory; clearing it exposed the collision. The new file now matches the package the rest of the directory uses. MigrationOfSystemViewsToCustomViews keyed off view_fk, the deprecated numeric link no row has carried since. It is left as the no-op it already was rather than rewritten against a column it was never about.
Mandate, MandateProvision and SignatoryPanel become plain row case classes with SQL stores, and their DDL moves from Schemifier to a Flyway script. The three tables had no direct coverage and neither do the v6.0.0 endpoints above them, so MandateProviderTest is added first and was confirmed green against the Mapper implementation before the rewrite. It pins what the API actually depends on: store-generated ids, the three listing orders (mandates newest-updated first, provisions by sortOrder, panels by name), that an update restamps the row and so reorders the listing, and that a miss is Empty rather than a failure. Free-text columns are bound as Option and read back with orNull so a null stays a SQL NULL instead of throwing at bind time, as MappedString and MappedText behaved. The endpoints fill every optional field with "" before calling, so a null is not expected here - but a store that throws on one turns a tolerated input into a 500. The update paths look the row up before writing so an unknown id stays Empty rather than becoming a no-op that reports success.
MappedSigningBasket and its two join tables become plain row case classes with SQL stores, and their DDL moves from Schemifier to a Flyway script. Covered by SigningBasketServiceSBSApiTest. Membership stays as unconstrained as it was: neither join table has a unique index, so the same payment can be listed in a basket twice, and BASKETID itself is only indexed rather than unique - reads take the first match by insertion order, which is what Mapper's find did. Cancelling a basket remains a status change rather than a delete, so an authorisation that referenced the basket can still be explained afterwards. Mapper ran entity.validate before saving a new basket and threw on a violation. The only validated field was Status against MappedString(50) and the only status written on create is the constant RCVD, so that branch could not fire; the column length still enforces it.
hongwei1
force-pushed
the
build/scala-3-migration
branch
from
August 17, 2026 17:07
074957c to
f4b79eb
Compare
|
ConsentRequest becomes a plain row case class with a SQL store, and its DDL moves from Schemifier to a Flyway script. Covered by ConsentRequestTest and VRPConsentRequestTest. consumerId is bound as Option: it is genuinely optional - the entity declared its default as null rather than "" and createConsentRequest passes null when the call has no consumer attached - so it has to reach the column as SQL NULL instead of throwing at bind time. MigrationOfConsentRequestConsumerIdFieldLength now names the table as a string rather than reaching through the Mapper singleton, so the historical script still runs against databases created before Flyway owned this table.
MappedCounterparty, MappedCounterpartyMetadata and MappedCounterpartyWhereTag
become plain row case classes with SQL stores, and their DDL moves from
Schemifier to a Flyway script.
The metadata row keeps its surrogate key because the mutators are keyed by it,
and the counterparty row keeps its own because the bespoke key/value rows are
keyed by the surrogate rather than by the counterparty id.
Three behaviours are preserved deliberately and marked as such at the call
site:
- deleteCorporateLocation and deletePhysicalLocation delete the where-tag row
and leave the pointer to it behind. A dangling pointer reads back as no
location, so the observable result is unchanged;
- newPublicAliasName's collision check maps to the addPublicAlias function
rather than the alias value, so it can never match and has never fired;
- currency reads back "" rather than null for a NULL column, because the
entity exposed it through the field's toString.
createBank-style callers of MappedCounterparty.mDescription.maxLen now read
MappedCounterparty.descriptionMaxLength instead, and
MigrationOfMappedCounterpartyDescriptionLength names the table as a string so
the historical script still runs against older databases.
MappedBank becomes a plain row case class with a SQL store, and its DDL moves from Schemifier to a Flyway script. The row exposes the field names the Bank trait declares rather than the column names, because the proxy connector serializes a connector result to JSON and re-extracts it as BankCommons: a bank id sitting under any other name comes back null, which ProxyConnectorTest catches. permalink keeps its plain, non-unique index. The entity carried a note that a unique one would be right, held back by tests that create the same bank twice, so reads take the first match rather than assuming there is only one. getBankLegacy and getBanksLegacy still default the routing scheme and address on the way out without storing them - copy on the row where Mapper set the fields on an unsaved entity. The sandbox importer writes banks through the store like branches, products and ATMs, and hands out the transient row before save() runs because createAccountsAndViews reads the bank ids while the rows are still unwritten - the same thing MappedSaveable did with an unsaved entity.
|
MappedTransaction becomes a plain row case class with a SQL store, and its DDL moves from Schemifier to a Flyway script. toTransaction and toTransactionCore stay on the row unchanged. The afterSave webhook fan-out moves into the store's insert, still wrapped in tryo, so every write still triggers it and a webhook subscriber still cannot fail the transaction that was just written. The read filters become a TransactionQuery value rather than a list of Mapper query params. It is also part of the cache key for a transaction read, so it has to be a value with a stable rendering: two requests asking for different pages, date ranges or directions must not share a cached answer. The translation is unchanged - both date filters and the ordering work on tFinishDate, the intended sort field of an OBPOrdering is ignored, and no ordering means no ORDER BY at all. The unique index spans (transactionId, bank, account) because a transfer is written once per side and both rows carry the same transaction id; lookups by id alone take the first match.
MappedTransactionRequest becomes a plain row case class with a SQL store, and its DDL moves from Schemifier to a Flyway script. toTransactionRequest stays on the row unchanged, including its reading of the type-specific half of the request back out of the stored JSON body. Dates are converted to java.util.Date on read, which is what MappedDate handed out. The java.sql.Date the driver returns is a subclass and type-checks either way, but it serializes to an empty JSON object rather than a date string, and start_date and end_date go straight into the transaction-request responses. updateAllPendingTransactionRequests stays a no-op: Mapper's updateStatus only set the field on the in-memory entity and never saved, so that loop has never written anything. It is marked as such rather than quietly turned into a path that writes. The remaining behaviour is unchanged, including createTransactionRequestImpl210 storing the routing SCHEME as the fallback for the counterparty's routing ADDRESS, and the counterparty read ordering by updatedAt while ignoring the intended sort field of an OBPOrdering.
MappedCustomer becomes a plain row case class with a SQL store, and its DDL moves from Schemifier to a Flyway script. A customer and an agent are the same row, so the agent provider reads and writes the same store. The row keeps its surrogate key: tax residences, addresses and dependants are keyed by it rather than by the customer id, so deleting a customer or reading its dependants has to resolve the surrogate first. The listing translation is preserved as it was, including that its date filters work on updatedAt while its ordering works on mLastOkDate - two different columns, not a typo - and that an empty customer-type list matches nothing rather than everything. populateMissingUUIDs names its backup table as a string rather than reaching through the Mapper singleton, and finds the rows to repair with an explicit "IS NULL OR = ''" instead of Mapper's NullRef.
MappedMetric and MetricArchive become plain row case classes over a shared
store, and their DDL moves from Schemifier to a Flyway script. The two tables
have the same shape bar the archive's metricId, and both are read by the same
filters, so the query building lives in one place rather than twice.
The read filters become a MetricQuery value. It is part of the cache key for a
metrics read, so it has to be a value with a stable rendering: two requests
asking for different pages, ranges or filters must not share a cached answer.
Behaviour preserved as it was:
- an unrecognised sort field falls back to date descending regardless of the
direction asked for;
- "anonymous" means the literal four-letter string "null" in the user id
column, not SQL NULL;
- a bank id is matched by the shape of the url, not by a column;
- ElasticsearchMetrics still reads the SQL table and still honours only
paging, the date range and the ordering;
- bulkDeleteConnectorMetrics still empties the API-metric table.
The archive write still reports a failure as false rather than throwing, which
is what the archiver uses to skip deleting the source row, and still
de-duplicates on the source row's primary key rather than the archive's own id.
The aggregate, top-apis and top-consumers reads were already raw SQL and are
untouched.
A date read back as java.sql.Date type-checks as a java.util.Date but serializes to an empty JSON object, and a connector-result row must expose the trait's field names because the proxy connector round-trips it through JSON. Both were found by full-suite failures whose message pointed away from the store that caused them.
MappedConsent becomes a plain row case class with a SQL store, and its DDL
moves from Schemifier to a Flyway script.
The row keeps its surrogate key because the atomic status transitions
(DoobieConsentStatusQueries, DoobieConsentSchedulerQueries) address a consent
by it, and those guarded updates are unchanged - the storage swap does not
touch how a concurrent revoke beats a stale scheduler write.
Behaviour preserved as it was:
- a status filter matches case-insensitively by listing both cases rather
than by lowering the column, and an empty status list matches nothing;
- an unknown sort field is not sorted on at all;
- a provider|providerId filter narrows only when it resolves to exactly one
user;
- expireAllPreviousValidBerlinGroupConsents writes the note of the consent
being made valid, not of the consent it terminates;
- a null expiry never matches the expiry sweep, so an open-ended UK consent
stays perpetual.
The guards that read `consent.mUserId == user.userId` now read
`consent.userId`: Lift's MappedField.equals compared against the underlying
value, so this was already a value comparison rather than a field identity one.
MappedBankAccount becomes a plain row case class with a SQL store, and its DDL moves from Schemifier to a Flyway script. Balance writes stay where they were: DoobieBankAccountQueries owns them because they lock the row and apply a delta. The store's setBalance is for fixtures that seed a starting balance, not for the payment path. The row keeps its surrogate key because the physical-card rows reference an account by it rather than by (bank, account id). The account rules stay a fixed pair of scheme/value columns read back through createAccountRule, which skips a rule with an empty scheme; account routings are still read from their own table. The sandbox importer writes accounts through the store and hands out the transient row before save() runs, because createTransactions reads the account ids while the rows are still unwritten - the same thing MappedSaveable did.
…efined order Two defects from the registry refactor, both in how allStaticResourceDocs was assembled. Folding every per-version aggregation into the union added 287 operation ids it never carried (the older aggregations are not subsets of the v7 one -- an endpoint dropped after v4 keeps its operation id there), and 234 of those collide on partialFunctionName with an entry already present. Http4s600's top-apis and popular-apis and JSONFactory6.0.0's metrics all build `partialFunctionName -> operationId` with `.toMap`, where the last entry wins, so with v1.2.1 sorting last the reported operation_id flipped to the oldest id: getBanks became OBPv1.2.1-getBanks, root became OBPv1.2.1-root. Restrict the union to obpUnionVersion (the current OBP aggregation) plus every non-OBP standard. Consequence, deliberate and documented at the constant: an operation id living only in a superseded aggregation stays unresolvable, exactly as before the refactor. The scanned half of the registry was a plain Map, so the same `.toMap` consumers resolved a partialFunctionName shared by two scanned standards according to hash iteration order -- undefined, and free to shift when a standard is added or removed. The Berlin Group v1.3 alias re-stamps the canonical BG v1.3 docs and so collides with BG v2 on getAccountDetails and four other names, and test.default.props now activates that alias for every test run. Sort it by fullyQualifiedVersion into a ListMap; BG v2 then wins those names, matching the behaviour before this branch. ResourceDocRegistryParityTest follows the narrowed union and regains the per-surface non-empty assertion, without which a standard whose docs stop being registered passes as a trivial subset. A new scenario pins obpUnionVersion as the newest OBP-standard version in the registry, so adding a v8 aggregation without moving it fails instead of silently dropping v8-only operation ids. Verified against a running instance: OBPv1.2.1-getBanks and OBPv3.0.0-getAggregateMetrics are rejected with OBP-40048 again, while BGv2-getAccountDetails, BGv1-getPaymentInitiationStatus and OBPv7.0.0-getMyMetrics still resolve. Full local suite 3573/0.
"".split("/") returns Array(""), not an empty array, so berlinGroupV13AliasPath
was List("") on a default instance -- nonEmpty. Every downstream
`if (berlinGroupV13AliasPath.nonEmpty)` guard therefore took its ACTIVE branch
with an empty prefix: Http4sBGv13Alias published 55 docs stamped with the
degenerate ScannedApiVersion("", "", ""), whose operation ids came out as
`BG-<name>`, and its route bridge matched the prefix "/" (every request) only
to fall through again.
That was invisible while the alias sat outside the global operation-id union.
Now that this branch folds it in, those 55 junk ids became resolvable: verified
against a running default instance that api-collection-endpoint creation
accepted BG-getAccountDetails and BG-getPaymentInitiationStatus with 201,
naming endpoints no route serves. Filtering empty segments makes "unset" mean
"inactive" again -- both now return 400, while BGv1.3, BGv2, UK and OBP ids are
unaffected and /resource-docs/BGv1.3/obp still serves its 55 docs.
OBP_BERLIN_GROUP_1_3_Alias.apiVersion has to guard .head/.last against the now
genuinely empty list: the ScannedApis classpath scan catches a throwing
companion and only logs a warning, so an unguarded NoSuchElementException would
drop the alias silently. Inactive registrations keep the empty-string version,
which deliberately does not equal ConstantsBG.berlinGroupVersion1 -- colliding
there would let this doc-less object win ScannedApis' .toMap and blank out the
canonical BG v1.3 resource docs.
The alias assertions in both tests no longer depend on a prop that only exists
in a gitignored file. test.default.props is excluded by .gitignore:21, so the
CI workflows carried berlin_group_v1_3_alias_path while a fresh clone or an IDE
runner did not: deleting the line locally reproduced two failures whose
messages gave no hint a prop was missing. They now cancel with an explanatory
message when the alias is inactive, and read the expected operation id back
from the alias's own docs instead of hard-coding the BGv1- prefix, which is
derived from the configured path. Verified both ways: with the prop set 13/13
pass, without it 11 pass and 2 cancel. Full local suite 3573/0.
…passed
Two follow-ups from reviewing the registry work itself.
The scanned half was sorted by fullyQualifiedVersion, which concatenates
apiStandard.toUpperCase and apiShortVersion and can therefore collide across
distinct keys -- ("BG", "v1.3") and ("BGV", "1.3") both render "BGV1.3", and
berlin_group_v1_3_alias_path lets a deployment choose the alias's half of such
a pair. sortBy is only stable with respect to its input, and the input is the
unordered ScannedApis.versionMapScannedApis, so a tie would hand the order back
to hash iteration and with it the `.toMap` winner for a shared
partialFunctionName. Sort by (apiStandard, apiShortVersion) instead: that pair
is exactly ScannedApiVersion's equals/hashCode key, so two distinct keys of
that Map always differ in it and the order is total. The resulting sequence is
unchanged -- alias, BG v1.3, BG v2, UK 2.0/3.1/4.0.1 -- so BG v2 keeps winning
the names it shares with the alias.
The obpUnionVersion guard ranked versions with ApiVersionUtils.versions.indexOf,
which returns -1 for anything absent from that equally hand-maintained list. A
-1 loses every maxBy comparison, so adding a v8.0.0 aggregation to the registry
while forgetting ApiVersionUtils.versions left v7 as the maximum and the
scenario green -- precisely the two-places-to-edit slip it was written to catch.
Assert first that every OBP version in the registry can be ranked at all.
Verified by injecting an unregistered OBPv8.0.0: the guard now fails with
"OBP versions in the registry but missing from ApiVersionUtils.versions:
OBPv8.0.0", where before it passed. Full local suite 3573/0.
any grant targeting a consent user redirects to its granting human with
a warn-log; sole exemption is createdByProcess == consent_user.
ConsentUtil.addEntitlements tags its writes accordingly.
- process column retired: parameter and accessor removed from the
Entilement trait and implementation
group queries now key on group_id; GroupEntitlementJsonV600 exposes
created_by_process instead of process.
- Explicit-target guards (400, "…names a consent user…"):
addEntitlement v2.0.0 + v7.0.0; addUserToGroup v6;
grantUserAccessToViewById v5.1; createAccountAccessRequest v6
(reject at creation) plus repeated check at its approval;
createAccount endpoints v2.0.0, v3.1.0, v4.0.0 (regular +
settlement), v5.0.0, v7.0.0.
- Implicit-target resolution to the accountable user (currently a
human): bank-creator grants (v2.2, v5, v6, v7 incl. the
generated-bank endpoint), v6
dynamic-entity creator roles, v3.0.0 entitlement-request requester,
createAccount owner fallbacks, and the connector-internal
HOLDING-account holder.
- Rename: effectiveHumanUserId → accountableUserId
…I version
Two defects found reviewing the registry against the union it replaced.
Berlin Group and UK Open Banking both publish getBalances, getAccountList and
getAccountBalances. Http4s600's top-apis/popular-apis and JSONFactory6.0.0's
metrics resolve a partialFunctionName with `.toMap`, which keeps the LAST
matching entry, so registry order decides the operation_id they report. The
hand-written union listed UK before BG, giving Berlin Group all three; sorting
the scanned standards alphabetically put UK last and silently flipped them to
UKv4.0.1-getBalances, UKv2.0-getAccountList and UKv2.0-getAccountBalances.
Replace the alphabetical sort with an explicit standardPrecedence (UK Open
Banking, then Berlin Group) and move Berlin Group v1.3 out of the explicit
block so it is ordered by that precedence rather than pinned ahead of it. A
standard absent from the list -- including the alias, whose apiStandard is
whatever berlin_group_v1_3_alias_path names -- ranks below all of them and can
never override a first-class standard. Verified against a running instance:
the three names resolve to BGv1.3-getBalances, BGv2-getAccountList and
BGv2-getAccountBalances again, matching the values measured before this branch.
A configuration-gated standard that is switched off reports
ScannedApiVersion("", "", ""), whose fullyQualifiedVersion is "" as well. While
ScannedApis kept that registration, ApiVersionUtils.valueOf("") resolved
successfully and, because the resource-docs route tolerates an empty path
segment, GET /obp/v7.0.0/resource-docs//obp answered 200 with an empty document
list where any other unknown version string gets 400 InvalidApiVersionString.
Drop unaddressable registrations in ScannedApis.versionMapScannedApis, which
fixes ApiVersionUtils, ResourceDocRegistry and Boot's version enablement in one
place. Verified: that request now returns 400, and BG v1.3, BG v2, UK 4.0.1 and
OBP v7.0.0 still serve 55, 22, 89 and 1031 docs. With the alias no longer
registered while inactive it is not a registry surface at all, so the parity
test's now-unreachable "cancel when unconfigured" branch is removed.
Both defects reached a green CI because nothing asserted either value; two
scenarios now pin them. Full local suite 3575/0.
…red name
standardPrecedence ranked a version by its apiStandard string, and the alias
takes that string from the first segment of berlin_group_v1_3_alias_path. A
deployment may point it at a name an existing standard already uses:
configured as "BG/v9" the alias reports ScannedApiVersion("BG", "BG", "v9"),
ranks alongside Berlin Group, and -- sorting after "v2" on the tie-breaker --
comes last, so its re-stamped copies won getBalances, getAccountList and
getAccountBalances away from the canonical docs it had copied. Metrics,
top-apis and popular-apis would then report BGv9-getBalances instead of
BGv1.3-getBalances. The comment on standardPrecedence claimed the opposite,
that the alias "can never override a first-class standard no matter how a
deployment configures it".
Match the alias by identity instead and rank it below every listed standard,
which makes that claim true for any configuration. sortKey takes the derived
alias version as a curried parameter and is package-private so the guarantee
can be tested against a synthetic alias, rather than only under whichever
berlin_group_v1_3_alias_path the JVM happens to have booted with.
Verified both directions with the new scenario: reverting to the string-based
rank fails it with "(1,BG,v9) was not less than (1,BG,v2)" -- the mechanism
itself -- and it passes with the fix. Full local suite 3576/0.
auth_type metrics column etc.
…alias-resource-doc-registry refactor: single source of truth for resource-doc registry (closes BG v1.3 alias gap)
…weep-and-cache-contracts test: endpoint sweeps and serialization contracts, and the defects they found
…ie stores develop's work here is mostly one theme: a consent user must not accumulate durable roles or own things in its own right. It landed as a guard inside Entitlement.addEntitlement plus explicit per-endpoint checks, and it retired the entitlement `process` column in favour of group_id and created_by_process. It also added mobile-phone fields to ResourceUser, an auth_type column and activity-dashboard indexes to the metrics tables, GET /my/metrics with Top Users and Top Consumers, a serialization namespace on every Redis memoize key, and a resource-doc registry that gives the Berlin Group v1.3 alias a tie-free order. All of that was written against Lift Mapper entities. This branch had already moved the entitlement, resource-user and metrics tables to Doobie, so the resolution carries the behaviour across rather than restoring the entities. The on-behalf-of guard reads createdByConsentId from the Doobie row (an Option, so the null/empty dance is gone) and is otherwise unchanged. The columns develop added by Schemifier come from db.changelog-develop-merge.yaml instead, since ToSchemify.models is empty on this branch and Schemifier creates nothing: a Mapper column here would compile and then not exist. Redis: this branch replaced scalacache with its own memoize layer, so develop's namespace is applied in redisMemoKey rather than through CacheConfig. The two key-format tests move from asserting the sampled envelope as the whole key to asserting it as the suffix - equality against the bare sample would now be asserting the absence of the namespace, which is the opposite of what develop added it for. Three defects were introduced while resolving conflicts and are fixed here rather than left for CI, all three invisible except through a symptom somewhere else: The master changelog gained two `- include:` entries folded into one YAML mapping. Duplicate keys are not an error - the last wins - so db.changelog-provenance.yaml was silently dropped and the tables it creates were never made. It surfaced as `Table "CHAT_EMAIL_DIGEST_STATE" not found` from a DELETE in the per-class test reset, with every shard aborting before it ran a test. No existing check could see it: they each read a changelog on its own and none asked whether master still referenced it. check_changelog_preconditions.py now verifies that every schema changelog is included exactly once, and that the `- include:` and `file:` counts agree. The entitlement INSERT kept `process` in its column list with a value of `""`. In SQL that is a quoted identifier, not an empty string, so the statement never parsed - and addEntitlement wraps the write in tryo, so the grant silently did not happen. 642 scenarios failed with 403 across suites that never mention entitlements. The column is nullable and the field is retired, so it is simply absent from the statement now. MetricQuery collected OBPUserId but not OBPUserIds, so the server-locked user set behind GET /my/metrics was dropped and the endpoint returned every user's rows - a data leak, not just a failing test. It is now rendered as `userid IN (...)`, with an empty set matching nothing rather than removing the clause: no visible users is not the same as no restriction. Each of the three has a test that fails on the defect and names it, rather than leaving the next person to work back from a 403 or a missing table. 4073 scenarios pass on H2 and on Postgres.
develop widens dynamicresourcedoc.examplerequestbody, .successresponsebody and
.errorresponsebodies with MigrationOfDynamicResourceDocBodyFieldsLength, whose own comment says a
response example "routinely exceeds varchar(255)". That migration reads Mapper metadata which does
not exist on this branch and was deleted in the merge along with two others; the other two got
changesets, this one did not, so the three columns stayed at the baseline's VARCHAR(255) and any
body over 255 characters failed the INSERT outright. The endpoints wrap the write, so the caller
saw a generic error rather than a length complaint.
${text.type} is the per-vendor spelling the baseline already uses for methodbody, so the columns
end up where the migration intended: text on Postgres, wide enough not to be a limit on H2.
The precondition is a sqlCheck rather than the columnExists the other changesets use, because the
columns are present either way and the question is their width. It reads
character_maximum_length, which is NULL once the type is unbounded, so a database already widened
by the upstream migration counts zero and marks this run instead of re-applying it.
check_changelog_preconditions.py rejected the changeset until it was taught modifyDataType - by
design, since it refuses to pass a change type it does not know the right precondition for. It now
requires a sqlCheck reading information_schema for these, and says why tableExists/columnExists
cannot serve.
serializationNamespace exists so two builds whose Kryo encodings differ cannot address each
other's entries. It derived its discriminator from scala.util.Properties.versionNumberString,
which reads the STANDARD LIBRARY - and Scala 3 compiles against the 2.13 one, so it answered
"2.13" here too. This branch therefore produced byte-identical keys to develop, on exactly the
upgrade the namespace was added to protect: measured as the prefix "obpser1-scala2.13" in this
branch's own golden-key test output, on a build whose scala.compiler is 3.3.8.
The failure that follows is the one already documented above the value: an entry written by one
chill/Scala combination decodes under the other into a different collection type, the decode
succeeds, and the call site whose signature says List gets a ClassCastException - a 500 for the
whole TTL, since a read that throws does not evict.
The compiler generation is not in any version string the runtime exposes, so it is a class probe:
scala.runtime.Scala3RunTime ships in scala3-library and does not exist in scala-library 2.13. Both
halves are kept ("3-lib2.13"), because the encoding depends on the compiler that produced the
classes and on the library they were compiled against. A 2.13 build keeps develop's spelling
exactly, so only this side moves and no one else cold-starts a cache.
The test probes with a different Scala-3-only class (scala.runtime.LazyVals$) than the
implementation uses: repeating the production probe would make the test agree with it however
wrong both were.
`CurrentNamespace should include("3")` was meant to say the namespace names the compiler
generation. It says nothing: "obpser1-scala2.13" contains a '3' as well, so the assertion held in
precisely the state the test exists to reject. The real work was being done by the line above it,
and this one only added false confidence. It now looks for "scala3".
Two comments corrected alongside it, both wrong in ways a reader would act on:
Redis.scala - the block explaining what the namespace is for had a second doc comment placed
between it and `serializationNamespace`, so it documented nothing and the value it explains was
left bare. The probe moves above it.
db.changelog-develop-merge.yaml - the precondition's comment said character_maximum_length is NULL
for an unbounded type. That is Postgres and MySQL; H2 reports 1000000000 and SQL Server -1. The
changeset is correct either way because it counts columns still at exactly 255, which is what the
comment now says.
getDistinctParentIds and getParentIdWithAttributes were written for AttributeQueryTrait.getParentIdByParams and NewAttributeQueryTrait.getParentIdByParams. Both traits are dead code with zero mixers anywhere in the tree, removed in the next commit as part of the net.liftweb.mapper cleanup - and neither ever called into these two methods either, so their removal leaves this file's only remaining function, getDistinctProviders, unaffected. Found while auditing the mapper cleanup's blast radius: the doc comments on both methods asserted a caller that had not existed since the traits were deleted, which is worse than no comment at all.
First step of unbundling lift-persistence: the fork's mapper package is 46.5% of its
lines, has no upstream Scala 3 port (Lift itself deleted persistence rather than
porting it), and OBP-API has had zero live Mapper entities since the Doobie migration
completed (ToSchemify.models = Nil). This removes the last obp-api references to it,
without touching the dependency itself - obp-api still pulls in lift-persistence for
common/util/db, same as before.
Deleted outright (all confirmed zero external references, not just zero imports):
AttributeQueryTrait/NewAttributeQueryTrait (self: BaseMetaMapper, no mixers anywhere),
CommonFunctions (validUri/validUrl, zero call sites), MappedAccountNumber/
DefaultStringField/MappedUUID/UUIDString (MappedString subclasses with no entity left
to use them), and MappedClassNameTest - which asserted over classOf[Mapper[_]]
subtypes, a set that has been permanently empty since the last entity was moved to
Doobie. It is the same "assertion that could not fail" shape as the CacheKeyFormatTest
fix earlier on this branch.
Two deletions needed care because nothing importing net.liftweb.mapper pointed at
them - they are reachable only through a class-name string, so removing the jar
without removing these would compile clean and then fail at runtime:
- JsonSerializers.MapperSerializer: ReflectUtils.forType("net.liftweb.mapper.Mapper")
inside an eager val, wired into the json4s Formats chain. Deleting the object
without also dropping it from the `serializers ::` list would leave a reference
to a name that no longer exists.
- ClassScanUtils.getMappers: Class.forName("net.liftweb.mapper.LongKeyedMapper")
inside a try/catch that logs and returns Nil on any Exception - the failure mode
a `net.liftweb.mapper`-string grep cannot see and a deleted jar would hit silently.
Zero callers, confirmed before deletion.
LocalMappedConnectorDataImport.MappedSaveable (zero instantiations) is deleted the
same way, with the historical comments at its three call-alike sites updated to say
"the now-removed MappedSaveable" rather than describing a type that no longer exists.
Same treatment for two comments in DoobieQueries.scala that credited
AttributeQueryTrait/NewAttributeQueryTrait as callers of getDistinctParentIds/
getParentIdWithAttributes - untrue even before this commit, since those two methods
already had zero callers (deleted separately, previous commit).
Remaining touches are narrowing, not removal: 18 migration scripts had a dead `DB`
import alongside the `Schemifier` one they actually use (`Schemifier.infoF` as a
logging callback - handled in the next commit), and three files had a dead
`import code.util.{MappedUUID, UUIDString}` left over from before those types moved to
Doobie-native construction.
Verification: mvn -Pprod -DskipTests clean install clean on first pass (deletions are
self-checking - the compiler is the reachability proof). H2 Surefire audit: 4073/0/0
(4075 - the 2 MappedClassNameTest scenarios, the only test-file change here).
Postgres was flakier to pin down and worth recording. Three concurrent 4-shard runs
and one 6-shard run all failed, but never on a real assertion:
- Shard 2 hit `run_tests_parallel.sh`'s 1200s per-shard timeout in every attempt,
once at 19m51s - a hair under the cap. The JVM's own shutdown hooks fired cleanly
mid-scenario each time, with zero exceptions, zero OOM/jetsam events, zero
Postgres connection errors in any log. This machine had a second worktree's
orphaned scalatest fork alive for >40h during every attempt (a stray
forkMode=once JVM this repo's own comments already document as a known
reparenting hazard) plus this session's own earlier background work, pushing
load average past 7 - not something to kill blindly (not owned by this session),
so shard 2's package set was instead run standalone (own Postgres database, own
ports, 1800s budget, no sibling shards competing for CPU): 1197 succeeded, 0
failed, 0 exceptions.
- Shard 3 failed once, on ResourceDocsTest's v4.0.0 scenarios, with a
scala.xml.XML.loadString error on a literal "<random-string>" placeholder inside
an existing (untouched by this commit) v4.0.0 endpoint description -
`resource_docs.take(3).foreach(doc => stringToNodeSeq(doc.description))` only
XML-validates the first three docs returned, so whether this fires depends on
resource-doc ordering, not on anything this commit changed. The suite passed
standalone (63/63) and again as part of shard 3's full package set run the same
isolated way as shard 2: 1008 succeeded, 0 failed.
Both isolated runs together cover every package the 4-shard split runs; shards 1 and 4
were clean across all three concurrent attempts. That is full Postgres coverage, green,
just not all four shards inside one concurrent invocation this particular machine could
sustain today.
…local ones
Second step of unbundling lift-persistence, following the mapper-surface deletions in
the previous commit. Four symbols were still genuinely called (not just imported) from
obp-api, none of them mapper-specific in behaviour - Schemifier's logging callback and
schema-name lookup both operate purely on net.liftweb.db types, and DB/
DefaultConnectionIdentifier under the mapper package are forwarders to the db/util
originals, not distinct implementations. Decompiled the shipped jar (javap) to copy
each one exactly rather than guess:
Schemifier.infoF(msg: => AnyRef): Unit = logger.info(msg) - unwrapped, verbatim
Schemifier.getDefaultSchemaName(conn: SuperConnection): String =
conn.schemaName.or(conn.driverType.defaultSchemaName).or(DB.globalDefaultSchemaName)
.openOr(conn.getMetaData.getUserName) - unwrapped, verbatim
Both now live on Migration.DbFunction, next to the tableExistsByName/
makeBackUpOfTableByName helpers that already carried the "copied from
net.liftweb.mapper.Schemifier" comment for the same reason. 62 call sites across 41
migration scripts and StoredProcedureUtils.scala move from `Schemifier.infoF _` to
`DbFunction.infoF _` - a mechanical substitution, verified uniform first: every one of
those 41 files used Schemifier for infoF and nothing else, and every one already
imported DbFunction unqualified for other Migration helpers, so the now-dead
`import net.liftweb.mapper.Schemifier` line comes out alongside each substitution.
`net.liftweb.mapper.DB` becomes `net.liftweb.db.DB` in Migration.scala (11 call sites)
and `net.liftweb.mapper.DefaultConnectionIdentifier` becomes
`net.liftweb.util.DefaultConnectionIdentifier` in DBUtil.scala - both confirmed
identical singletons by decompiling: `mapper.DB` is `object DB extends db.DB1`, and
`mapper.DefaultConnectionIdentifier` is a one-line forwarder to `util.DefaultConnectionIdentifier`.
Migration.DbFunction.tableExists(BaseMetaMapper, ...) and makeBackUpOfTable(BaseMetaMapper)
are deleted outright: both were the last two consumers of BaseMetaMapper, both had zero
callers (confirmed by grep before deletion - the only remaining hits are doc comments in
other migration scripts that already say the entity behind them is gone), and both have
had *ByName successors in active use for a while.
Two call sites intentionally untouched: Boot.scala:540 and
MockedRabbitMqAdapter.scala:3322 still call Schemifier.schemify(true, Schemifier.infoF _,
ToSchemify.models: _*) on an empty list - a no-op, but the whole call and its
ToSchemify.models plumbing come out in the next commit along with Boot's remaining
Schemifier-adjacent setup, rather than half-migrating a call this commit does not also
delete.
net.liftweb.mapper now has zero live references from obp-api (grep -rn
"net\.liftweb\.mapper" obp-api/src/main | grep -v '^\s*//' turns up only the two
Boot.scala/MockedRabbitMqAdapter.scala schemify calls and pre-existing commented-out
Lift-era files this refactor does not touch).
Verification: clean compile in one pass. H2 Surefire audit: 4073/0/0, unchanged from
the previous commit (no test files touched here).
Postgres: given the previous commit's documented machine-load flakiness on concurrent
shards, went straight to isolating each of the 4-shard split's package sets against its
own database and ports rather than re-running the concurrent layout first - two pairs
run concurrently (shard 1 with shard 4, then shard 2 with shard 3) for a bounded total
runtime without reintroducing the contention that caused the earlier timeouts. All four
green: shard 1 535/0, shard 2 1197/0, shard 3 1008/0, shard 4 1285/0 - full coverage of
the 4-shard layout, all BUILD SUCCESS, zero FAILED markers anywhere.
Third and final step of removing net.liftweb.mapper from obp-api. The previous two
commits took every reference down to two schemify calls, both already no-ops
(Schemifier.schemify(true, Schemifier.infoF _, ToSchemify.models: _*) on an empty
list), plus a MapperRules setting and a MetaMapper-typed field that fed them. All
four come out here, along with the last wildcard mapper import.
- Boot.scala:173's MapperRules.createForeignKeys_? assignment: the only reader was
Schemifier, and Schemifier's argument was always Nil, so this configured a
foreign-key policy for a schema-creation pass that never created anything. The
mapper_rules.create_foreign_keys prop it read is retired (release_notes.md, both
props templates).
- Boot.scala:539's schemifyAll(), renamed createDefaultChatRoom() with the
Schemifier.schemify line removed - it kept exactly one live side effect
(getOrCreateDefaultRoom()) and the name should say so, not describe schema work
that stopped happening once ToSchemify.models went to Nil.
- MockedRabbitMqAdapter.scala:3322's identical schemify call, and its now-dead
net.liftweb.mapper.Schemifier / bootstrap.liftweb.ToSchemify imports.
- ToSchemify.models itself: not just emptied, deleted. The object stays (it also
starts the optional gRPC server and registers a JVM shutdown hook, unrelated to
schema). Its four remaining "importers" - ServerSetup, LocalMappedConnectorTestSetup,
TestConnectorSetupWithStandardPermissions, SandboxDataLoadingTest - never actually
read the field; each import was dead weight left over from when their reset loops
iterated it. Removing them is confirmed safe by the same evidence that made the
field safe to delete: obp-api has had zero live Mapper entities since the Doobie
migration finished.
- Boot.scala:64's `import net.liftweb.mapper.{DefaultConnectionIdentifier => _, _}` -
the wildcard that supplied MapperRules, Schemifier and MetaMapper to this file.
Nothing else in it needed anything from that package.
LiquibaseSchemaSetupTest asserted `ToSchemify.models shouldBe empty` as half of pinning
"liquibase.enabled defaults to true because nothing else creates a table." That
assertion doesn't compile once the field is gone, and doesn't need to: the invariant it
protected is now enforced by the compiler rather than by a runtime check, since there
is no Schemifier.schemify call left anywhere in obp-api to accidentally un-empty a list
that no longer exists. Rewrote the test and the doc comments in LiquibaseSchemaSetup.scala
and LiquibaseOnExistingSchemaTest.scala that described the old mechanism, so none of them
keep pointing at a symbol that isn't there.
One more comment turned out to be stale independently of this refactor, caught only
because it was about to become more obviously wrong: AtmTableResetIsolationTest.scala's
doc comment said MappedAtm was "still in Boot.ToSchemify.models" and reset "happens for
free" via that list's bulkDelete_!! loop - checked, and all four reset paths it lists
already carry an explicit `DELETE FROM mappedatm` (ServerSetup:150 and the same line
number pattern in the other three). MappedAtm moved to Doobie a while ago; the comment
was never updated to say so. Corrected to describe the current mechanism instead of a
superseded one.
obp-api/pom.xml's comment on the lift-persistence dependency said Scala 3 doesn't exist
"see docs/scala3-lift-mapper-blocker.md" as if obp-api's own code were still blocked by
it. It isn't, any more - grep -rn "net\.liftweb\.mapper" across obp-api and obp-commons
main sources now turns up only comments and the pre-existing entirely-commented-out
Lift-era files this refactor doesn't touch. What is still pinned to _2.13 is the
ARTIFACT: lift-persistence bundles common+db+mapper+proto+util as one jar, and no
Scala 3 build of the bundle exists because mapper can't compile under Scala 3. Reworded
to say that rather than implying obp-api's own mapper usage is the blocker.
Verification: clean compile in one pass. H2 Surefire audit: 4073/0/0, unchanged (the
4 dead-import deletions and the LiquibaseSchemaSetupTest rewrite add or remove no
scenarios). Postgres: same isolated-per-shard-pair strategy as the previous commit,
same numbers - shard 1 535/0, shard 2 1197/0, shard 3 1008/0, shard 4 1285/0, all
BUILD SUCCESS, zero FAILED anywhere.
Also did the one check the test suite cannot: a real production-mode boot
(flushall_build_and_run.sh, backed by an isolated in-memory H2 rather than any
suite's shared setup) reached `Ember-Server service bound to address: 127.0.0.1:8080`
with no ExceptionInInitializerError and no Schemifier line anywhere in the log, then
served two live requests against it - GET /obp/v5.1.0/root (200) and
GET /obp/v5.1.0/resource-docs/v5.1.0/obp (200, 3.5MB, 599 resource_docs entries) - the
second one specifically to drive the json4s Formats chain end to end now that
MapperSerializer is gone from it (removed two commits ago), on a real multi-megabyte
payload rather than a test fixture.
End state: grep -rn "net\.liftweb\.mapper" obp-api/src obp-commons/src, filtered to
non-comment lines, returns nothing. obp-api's dependency on net.liftweb.mapper is zero.
…ook never ran
Found by code review of the previous three commits (the net.liftweb.mapper removal):
deleting ToSchemify.models and the Schemifier.schemify(...) call that read it removed the
only thing in the whole codebase that ever touched the ToSchemify object. Scala objects
initialize their entire body - every val and top-level statement - on first access to any
member, not at class-load time. Before this session's earlier commits, Boot.scala's
schemifyAll() reading ToSchemify.models was that first access; once schemifyAll() was
renamed to createDefaultChatRoom() and stopped touching ToSchemify, and models itself was
deleted, nothing else in the tree ever referenced it again (confirmed by grep - zero live
code hits, only comments).
The object's body is not just schema-adjacent bookkeeping: it starts the optional gRPC
server (grpc.server.enabled) and registers the JVM's one ORDERED shutdown hook - added,
per its own comment, specifically to fix a race between two previously-concurrent hooks
(gRPC could still be serving a request while the DB pool closed underneath it). With the
object never initializing, both silently stop happening: grpc.server.enabled=true starts
no server and logs no error, and - regardless of that flag - the app stops gracefully
closing the Hikari pool and Redis on shutdown at all.
Verified live, not just by reading the bytecode-initialization rule: booted the packaged
jar and sent it SIGTERM. Before this fix, no HikariPool shutdown log line appeared at all.
After renaming the object to ProcessLifecycle and adding an explicit
ProcessLifecycle.start() call in Boot.boot() (with a comment explaining why an explicit
call is required rather than relying on incidental access), the same test produces
"HikariPool-1 - Shutdown initiated..." / "Shutdown completed." from the shutdown-hook
thread.
Also renamed for the same reason the earlier commits already applied to schemifyAll(): the
object's name described work it no longer does (nothing about it is "to schemify" any
more - the schema half left when models did), and that mismatch is very likely part of why
nothing noticed it had gone silently unreachable.
Swept the doc comments the same review flagged as referencing renamed/deleted symbols by
name, in the files most likely to be read while debugging boot order or writing a new
migration:
- Boot.scala: the comment above the executeScripts calls still said "AFTER schemifyAll()
above", read right next to the createDefaultChatRoom() call it was talking about.
- Migration.scala's `database` object doc comment named `schemifyAll()` and
`tableExists(ResourceUser)` - the latter is the exact Mapper-typed overload the
previous commit deleted; a reader copying that comment's example would write code that
no longer compiles.
- Two migration scripts' historical comments (MigrationOfConsentAuthContextDropIndex,
MigrationOfMappedUserAuthContext) named the same deleted overload as "what this used to
call" without saying the overload itself is gone, not just unused.
Verification: clean compile. H2 Surefire audit: 4073/0/0, unchanged (no test files
touched). Postgres: same isolated-per-shard-pair strategy as the prior three commits -
shard 1 535/0, shard 2 1197/0, shard 3 1008/0, shard 4 1285/0, all BUILD SUCCESS, zero
FAILED anywhere.
Second of two cleanups from the code review of the net.liftweb.mapper removal.
DbFunction.maybeWrite took a `logFunc: (=> AnyRef) => Unit` because Schemifier did - it
was a general-purpose library where a caller might reasonably want its own logger. Here it
never was: all 64 call sites across the 41 migration scripts and StoredProcedureUtils
passed the identical `DbFunction.infoF _`, and infoF was never anything but
`logger.info(msg)`. Checked before removing: 64 occurrences of the call, 64 of them that
exact shape, and infoF had no other reader.
A parameter with zero call-site variance is not an abstraction, so `logger.info(ct)` moves
inline and infoF goes. This also finishes what the previous commits started - they already
deleted the Mapper-typed overloads (tableExists, makeBackUpOfTable) once each had a single
call shape, and leaving this one pluggable was inconsistent with that.
Also corrects an overstatement the same review flagged in LiquibaseSchemaSetupTest: the
comment replacing the retired `ToSchemify.models shouldBe empty` assertion claimed the
invariant is now "enforced by the compiler". That holds only for the exact regression it
replaced - repopulating a field that no longer exists. It does not hold for the wider claim
the surrounding doc makes ("nothing else creates a table"): lift-persistence still ships
net.liftweb.mapper, so a new Mapper entity plus a fresh Schemifier.schemify call would
compile and run, and no test in the suite boots Boot.scala to notice one running beside
Liquibase. The comment now says what is actually guaranteed and what is not.
Verification: clean compile. H2 Surefire audit 4073/0/0, unchanged. Postgres, isolated per
shard pair as before: shard 1 535/0, shard 2 1197/0, shard 3 1008/0, shard 4 1285/0, all
BUILD SUCCESS.
recordConnectorTrace called APIUtil.getCorrelationId() to fill the correlationId column, but that function is a stub returning "" since the Lift teardown (it used to read Lift's container session). The matching connectormetric row is written with the correlation id routeToConnector already extracted from the CallContext for exactly this purpose - trace rows just never received it, so with write_connector_trace enabled every row was written with correlationid = '' and could not be looked up by OBPCorrelationId or joined to its metric row. Pass the already-extracted correlationId into recordConnectorTrace instead of re-deriving it.
scopeFor keyed the dedup lock/response cache on (consumer or Authorization
header, operation id) alone. Two gaps:
- No user in the scope. One Consumer (API Explorer, the Portal, a bank's
mobile app) serves many users, so a second user reusing a key the first
user had already used against the same operation was served the first
user's cached response - their own request never ran.
- No concrete path in the scope. The operation id is the ResourceDoc
template ("OBPv5.1.0-deleteAtm"), never substituted with the real
BANK_ID/ATM_ID. Without the path, one key covered every resource under
an operation - deleting atm-1 then atm-2 under the same key deleted only
the first and replayed its 204 for the second, since a DELETE's body
hash is sha256("") for both requests and gave no other discriminator.
Add both to the scope key. Also add guaranteeCase to the lock acquired in
runAndCache: it was released only on the two normal completion paths, but
ResourceDocMiddleware wraps every endpoint in a timeout that CANCELS the
fiber holding the lock, so a slow POST answered 504 left the lock held for
its full 60s TTL and told the client's well-behaved retry "operation
already in flight" when nothing was.
IdempotencyMiddlewareTest's "in flight" scenario hard-codes the scope hash
the middleware computes; updated it to the new four-part formula.
TokenBinding.verifyTokenBinding compared a bound access token's cnf.x5t#S256 claim against whatever PeerTrust resolved as the caller's certificate, without checking how that certificate was resolved. On the out-of-the-box configuration (mtls.enabled unset, no trusted proxies), PeerTrust.trustForwardedHeaderWithoutTls defaults to true, so an unauthenticated PSD2-CERT header is enough to name "the caller" - by design, for endpoints that only need some certificate to attribute a request to. RFC 8705 sender-constraining needs more than that: a certificate is public information (a QWAC is not a secret), so an attacker who replays a stolen bound access token alongside the victim's own public certificate in that header would pass ENFORCE/REQUIRED verification even though nothing proved they hold the matching private key. Add PeerTrust.UnauthenticatedHopDetail as the named marker for that one resolution (already used internally, just not exposed for a caller to check), and have TokenBinding treat it as equivalent to no certificate at all via a new callerCertificateForBinding - reading cc.certificateTrust / certificateTrustDetail rather than re-deriving anything from the raw header, so it can never disagree with what PeerTrust actually decided.
allStaticResourceDocs deduplicated the union with distinctBy(_.operationId), which keeps the FIRST occurrence in iteration order. Every other consumer of this ordering (Http4s600's top-apis/popular-apis, JSONFactory6.0.0's metrics, and this registry's own sortKey docstring) is built on the opposite convention: the standard sorted LATER wins a name it shares with one sorted earlier, via a `.toMap` where the last entry wins. This silently broke the Berlin Group v1.3 alias safeguard sortKey already implements. The alias re-stamps the canonical BG v1.3 docs with implementedInApiVersion.copy(apiStandard = doc.implementedInApiVersion.apiStandard), so with the natural configuration (berlin_group_v1_3_alias_path ending in "v1.3") its operation ids are byte-identical to the canonical ones, and sortKey ranks the alias first specifically so the union's last-wins dedup keeps the canonical entry. distinctBy's first-wins direction handed the win to the alias instead, replacing all 55 canonical BG v1.3 docs with copies whose URL prefix is the alias path. Switch to reverse/distinctBy/reverse: same last-wins direction as every other consumer, while preserving the relative order of what survives.
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.
|
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.