build: pay the Scala 3 debt on 2.13 (flip sequenced after Doobie) - #91
Draft
hongwei1 wants to merge 385 commits into
Draft
build: pay the Scala 3 debt on 2.13 (flip sequenced after Doobie)#91hongwei1 wants to merge 385 commits into
hongwei1 wants to merge 385 commits into
Conversation
hongwei1
changed the title
hongwei1
changed the title
|
Three tables replaced with Doobie row case classes and a V079 migration reproducing the probed DDL. RoutingSchemeValidation shares the file but is pure logic — scheme-name regexes, the global allow-list, and the country-prefix rule for the CARDANO/ETHEREUM settlement rails. Only the entity and provider above it are rewritten; the validation object is spliced back byte for byte so a storage swap cannot perturb payment-address validation. Two constraints carry behaviour and the migration says so at each: ROUTINGSCHEME(scheme) unique is what makes scheme usable as the handle for every read, the update and the soft delete, and BANKSUPPORTEDROUTINGSCHEME(bankid, scheme) unique is what makes putBankSupportedRoutingScheme an upsert rather than an append. deleteRoutingScheme stays a soft delete: status goes to RETIRED and the row remains so historical addresses still resolve. Nothing removes these rows, which is easy to mistake for an oversight, so it is stated. getAbacRulesByPolicy keeps filtering in memory. policy is a comma-joined tag list in one column, so a SQL LIKE would also match a policy name that is a substring of another; pushing the filter into SQL would read as an optimisation and be a correctness regression. ABACRULE has three plain indexes and no unique one, though abacRuleId is the handle the update and delete key off and getAbacRuleByName reads by rulename, so two rules may share a name. Pre-existing; reproduced with id ASC pinning the lookup.
… Mapper Two tables replaced with Doobie row case classes and a V080 migration reproducing the probed DDL. endpointmapping.operationid is unique globally, not per bank. The provider looks rows up by (operationId, bankId), which reads as though the pair is the key, so the constraint is stated in the migration: a bank-level and a system-level mapping cannot share an operation id, and bankId only narrows a read. bankid itself genuinely holds NULL for system-level rows and is bound as an Option so SQL NULL survives. DYNAMICENTITYINDEX keeps its five backfill-bookkeeping columns in the DDL, since deployed databases have them, but leaves them out of the row model: no code path reads or writes backfillcheckpoint, rowcountexpected, coercionerrors, lasterror or provisionerversion, and carrying them as always-default fields would suggest the provisioner maintains state it does not. Its index on (entityname, bankid, fieldname) is plain rather than unique though markReady does a find-then-insert on that triple, so a concurrent double-provision would leave two rows. Pre-existing; reproduced with id ASC pinning the lookup.
Two tables replaced with Doobie row case classes and a V081 migration reproducing the probed DDL. This OneToMany was live, unlike the counterparty-bespoke one: the invitees accessor read mInvitees. It is replaced with an explicit query preserving id ASC, which is the order the invitees were supplied in. createMeeting still accepts staffUser and still does not store it — Mapper's .mStaffUserId line is commented out, so the column has always been NULL and present.staffUserId has always been "". Preserved with a note; writing it would change what every existing meeting reports. mcustomeruserid and mstaffuserid hold RESOURCEUSER's numeric primary key rather than the public user_id, so present resolves them through joins and an unresolved key yields "" as before.
… off Lift Mapper Two tables replaced with Doobie row case classes and a V082 migration reproducing the probed DDL. MAPPEDCUSTOMERMESSAGE has two owner columns and they are not interchangeable: user_c holds RESOURCEUSER's numeric key and is written only by the deprecated addMessage path, while customer holds MAPPEDCUSTOMER's numeric key and is written only by createCustomerMessage. Each read filters on exactly one of them, so a message created one way is invisible to the other reader. That is what the deprecation note on the user field is about; the split is reproduced and documented rather than unified, since merging them is a data-model change and not a storage swap. MAPPEDTRANSACTIONREQUESTTYPECHARGE has no index beyond its primary key though its only read filters on (mbankid, mtransactionrequesttypeid) and expects at most one row. Pre-existing; reproduced with id ASC pinning which row a lookup sees. MappedCustomerMessagesTest's bulkDelete_!! calls become deleteAll on the new store.
Two tables replaced with Doobie row case classes and a V083 migration
reproducing the probed DDL.
The Lift entity CardAction is deleted rather than migrated: it was never
registered for schema creation, so its table has never existed in any
database and no code path read or wrote it. The probe returned zero
columns for it.
networks and allows are both comma-joined lists in one column but are
parsed differently — allows filters empties, networks does not, so an
empty networks column yields List("") rather than Nil. Preserved; the
difference is visible to every caller.
The two write paths represent an absent replacement differently and both
are reproduced: update writes the literal string "null" for the reason,
because Mapper called toString on a null reasonRequested, while create
leaves the columns genuinely NULL.
cvv and brand stay create-only. Mapper's update never set them, which
matters because the CVV column holds a SHA-256 hash and
updatePhysicalCard is never handed a plaintext to re-hash.
PinReset's upsert still looks an existing row up by replacement date
alone, ignoring which card it belongs to, so a reset requested on the
same instant for another card is updated instead of a row being
inserted. Preserved verbatim — narrowing the lookup would change which
rows exist.
The only constraint is (mbankid, mbankcardnumber, missuenumber). It does
not cover mcardid, which three provider methods key off, nor the
(bank, serial, cardNumber) triple createPhysicalCard checks before
inserting. Both are assumed unique without the database enforcing it;
recorded in the migration.
One table replaced with a Doobie row case class and a V084 migration reproducing the probed DDL. Both unique indexes are load-bearing rather than protective, and the migration says so: each leg — (bank, account, transaction) on the debit side and on the credit side — may appear in the book at most once, which is what stops a transaction being booked into two different double-entry pairs. The connector wraps the insert in tryo, so a duplicate booking surfaces as a Failure. The two find(...).or(find(...)) call sites in LocalMappedConnector become findByLeg and findByTransactionId on the store, keeping the debit-then-credit precedence in one place instead of spread across the connector. The transactionrequest* columns hold "" rather than NULL for a movement that did not originate from a transaction request, so the accessors map empty to None as before.
One table replaced with a Doobie row case class and a V085 migration reproducing the probed DDL. The optional bank id every read and the delete accept only narrows the match — it is not part of the key, and only dynamicendpointid is unique. A system-level lookup will therefore find a bank-level row with the same id, and nothing but the id being generated prevents that. The semantics are pulled into a single idCondition helper instead of being re-spelled at each of the five call sites, and recorded in the migration. bankid genuinely holds NULL for system-level endpoints, so it is bound as an Option and the reader turns null-or-empty back into None as before.
One table replaced with a Doobie row case class and a V086 migration reproducing the probed DDL. Writes already went through Doobie — ConnectorMetricBatchWriter batches the inserts — so only the reads changed. bulkDeleteConnectorMetrics deletes MappedMetric, not MappedConnectorMetric: it empties the API-metric table and leaves connector metrics untouched, so the method does the opposite of its name. That is a pre-existing defect, left verbatim with the reasoning at the call site. Any caller invoking it today relies on the API metrics being cleared, so silently redirecting it under a storage swap would change what the call destroys. Worth its own change with a caller audit. date is stored as date_c because DATE collides with a SQL reserved word. Five plain indexes and no unique one is correct here: a connector may call the same function under the same correlation id more than once and each call is its own row.
One table replaced with a Doobie row case class and a V087 migration reproducing the probed DDL. The unique index on (mbankid, muserid, mrolename) is load-bearing for authorisation, not an optimisation, and the migration says so: addEntitlement deliberately lets a concurrent duplicate grant hit the constraint, then re-reads and returns the committed row rather than failing or creating a second grant. Without it a role could be held twice and one revoke would leave the other behind. Four columns break the m-prefix convention because the entity overrode dbColumnName — group_id, process, granted_by_user_id and entitlement_request_id. The migration pins the exact names. entitlement_request_id is the one optional column defaulting to NULL rather than "", and its reader also rejects the all-zero UUID since only request-born grants set it. The other three use the empty-string convention and are read through an empty check. Empty ByList kept its "no rows" meaning on both the read and the two delete paths: an empty user-id or role-name list matches nothing rather than everything, which on the delete side is the difference between a no-op and emptying the table.
One table replaced with a Doobie row case class and a V088 migration reproducing the probed DDL. The three optional scope columns hold SQL NULL, not "", when the scope is broader, and that is load-bearing: getByConsumerId resolves a limit by trying four increasingly general scopes and each tier matches the columns it is not scoping on with IS NULL. A row storing "" would be invisible to every tier. The four-tier fallback is kept intact and the IS NULL semantics routed through one `scoped` helper, so "None means the column must be NULL" cannot drift into "None means do not filter". The readers stay laxer than the queries — apiName, apiVersion and bankId map both NULL and "" to None while the lookups accept only NULL. That asymmetry is Lift's and is preserved. The six call-limit columns carry no database default. Their defaults come from props (rate_limiting_per_second and friends, -1 when unset) and are resolved in application code at insert time, which is where Lift's field defaults came from. On update an omitted limit keeps its stored value, as Mapper's per-field foreach did. createOrUpdateConsumerCallLimits still does not invalidate the rate-limit cache while createConsumerCallLimits and updateConsumerCallLimits both do. Preserved with a note rather than corrected. createdAt and updatedAt are exposed on the row because the v5.1.0 and v6.0.0 JSON factories report them on the rate-limit resource.
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
|
Top Users and Top Consumers in v7.0.0
fix: widen DynamicResourceDoc example/response body columns to text- #94
…registry The resource-docs dispatcher serves the BG v1.3 alias (active only when berlin_group_v1_3_alias_path is set) through its ScannedApis registration, but APIUtil.allStaticResourceDocs never included it. Its docs carry their own operation ids, re-derived from the alias version string, so alias operation ids failed the getAllResourceDocs membership check used by api-collection-endpoint creation and other operation-id lookups -- the same gap BGv2 had before it was added to this union. Reproduced against a running instance with the alias prop set (OBP-40048 on a valid alias operation id) and confirmed the fix resolves it.
The alias surface is gated by berlin_group_v1_3_alias_path, which is unset in the default test environment, so its operation-id list is legitimately empty there -- skip the non-empty assertion for it while still running the membership check against getAllResourceDocs.
…ocs dispatcher Every one of its ~19 arms was `case X => resourceDocs`, unchanged -- a leftover from the pre-http4s Lift route-filter era that stopped doing any filtering once the corresponding version moved fully onto http4s. getResourceDocsList now feeds resourceDocs directly into activePlusLocalResourceDocs, with identical output.
…ion from one registry
Introduce ResourceDocRegistry as the single source of truth for "which
resource docs does version X serve", replacing two independently
hand-maintained registries: ResourceDocsAPIMethods.getResourceDocsList
(the per-version dispatcher used by /resource-docs/{VERSION}/... and API
Explorer) and APIUtil.allStaticResourceDocs (the union used wherever an
operation id must be resolved). These drifted three times by hand --
Berlin Group v2, v7-only operation ids, and the Berlin Group v1.3 alias
all had to be independently added to both places, and were each missed
at least once. Deriving both from one registry map makes that class of
drift structurally impossible going forward.
Http4sBGv2 becomes a ScannedApis registrant (its apiVersion is
ConstantsBG.berlinGroupVersion2), so it is now fully convention-driven
like the other Berlin Group / UK Open Banking standards and needs no
hand-maintained entry in the registry or a special case in
ApiVersionUtils.valueOf. The global union is now deduped by operationId
-- the underlying per-version buffers legitimately overlap (each
OBP-standard aggregation repeats every older version's docs), and
consumers only ever .find or build a lookup map from the result.
ResourceDocRegistryParityTest is rewritten to iterate the registry
itself rather than a hand-typed list of standards, so a future standard
reachable by the dispatcher is covered by construction and the test's
job narrows to catching an accidental regression back to two
independently maintained registries.
Verified live against a running instance, before and after: BGv2 and
Berlin Group v1.3 alias operation ids both still resolve through
POST /my/api-collections/{name}/api-collection-endpoints. Full local
suite: 3582 tests, 0 failures.
…int operation id Adds an HTTP-level regression test for the sandbox bug report this branch started from: creating an API collection endpoint with operation_id=BGv2-getAccountDetails now returns 201, alongside the existing coverage for OBPv6.0.0, UK Open Banking, and Berlin Group v1.3 operation ids in the same scenario. Previously the only regression guard for this exact operation id was the unit-level membership check in ResourceDocRegistryParityTest; this exercises the actual endpoint.
…evel coverage berlin_group_v1_3_alias_path could not be toggled per-test at runtime: its ScannedApiVersion identity is captured once by ScannedApis. versionMapScannedApis' process-wide classpath scan (a lazy val, shared across the whole JVM/shard), which gets forced by the first unrelated request that falls through Http4sApp's route chain -- almost always long before any test-specific setPropsValues call. The only way to exercise a real alias operation id end to end is to have the prop already set before the JVM boots. Set berlin_group_v1_3_alias_path=0.6/v1 in test.default.props (local) and both CI workflows' generated test.default.props (build_pull_request. yml, build_container.yml). Add a regression test in ApiCollectionEndpointTest mirroring the existing per-standard coverage (OBPv6.0.0/UK Open Banking/Berlin Group v1.3 canonical) for the alias's BGv1-getPaymentInitiationStatus operation id, and pin the same operation id in ResourceDocRegistryParityTest alongside the existing BGv2-getAccountDetails pin.
…ift instance The global operation-id union used to be built from the v6.0.0 aggregation, so operation ids belonging to endpoints that exist only in v7.0.0 were absent from it and could not be added to an API collection. That drift instance had no regression test: the OBPv6.0.0-* cases in ApiCollectionEndpointTest pass under both the old v6-based union and the current v7-based one, so they cannot detect it. Pin OBPv7.0.0-getMyMetrics (v7-only -- not part of Http4sResourceDocAggregation.v600) as a real api-collection-endpoint request, and add the matching named pin in ResourceDocRegistryParityTest alongside the BGv2 and Berlin Group v1.3 alias ones, so all three historical drift instances now have both HTTP-level and registry-level coverage.
…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.
|
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.