Scala 3 migration: Lift Web to http4s, Lift Mapper to Doobie, Flyway to Liquibase - #2899
Open
hongwei1 wants to merge 290 commits into
Open
Scala 3 migration: Lift Web to http4s, Lift Mapper to Doobie, Flyway to Liquibase#2899hongwei1 wants to merge 290 commits into
hongwei1 wants to merge 290 commits into
Conversation
Fourth table off Mapper. Gate 137 -> 136. Tags carry two parallel sets of methods - on a transaction and on an account - stored in one table and told apart by whether the transaction column is set. That distinction is the part a rewrite loses quietly, so most of the nine characterization scenarios written first check exactly it: an account tag must not appear among a transaction's tags, a transaction tag must not appear among the account's, and the two bulk deletes differ in scope accordingly. Audit of the ported provider: six writes were on runQuery and are now runUpdate, and the java.sql.Timestamp Meta import was missing again. Each of the six was checked individually against its statement rather than trusted to a pattern - an earlier table had a read misclassified as a write by a naive window match. DDL exported from Schemifier rather than written by hand, as for the previous tables. Suite 3533/0: 3524 plus these nine.
Fifth table off Mapper. Gate 136 -> 135. A where tag is one value per (transaction, view), not a list: adding a second for the same view replaces the first. Every other provider in this package appends, so a rewrite that copies the neighbours' shape would quietly start accumulating rows. That is the first thing the seven characterization scenarios pin, and it is why the ported provider's UPDATE path matters. Audit: five writes moved from runQuery to runUpdate, each checked against its own statement, and the java.sql.Timestamp Meta import was missing again. The cascade helper in V400ServerSetup asked the entity; it asks the provider per view now, as comments already do. Suite 3540/0: 3533 plus these seven.
Sixth table off Mapper, and the last of the transaction-metadata group. Gate 135 -> 134. Seven characterization scenarios were written against the Lift implementation first, since the provider had none. The one worth calling out asserts the imageUrl round trip: it is stored as text and handed back as a java.net.URL, and that conversion is the kind of thing a rewrite drops silently. Audit of the ported provider: four writes moved from runQuery to runUpdate, each checked against its own statement, and the java.sql.Timestamp Meta import was missing again - the same two findings as every other file taken from that branch. The cascade helper in V400ServerSetup now asks the provider per view, matching what comments and where tags already do. With this table the helper no longer touches any Lift entity for metadata. Suite 3547/0: 3540 plus these seven.
Seventh table off Mapper. Gate 134 -> 133. Unlike the previous six this one is written rather than ported - the reference branch never migrated it - so there was no prior implementation to audit, and no runQuery-for-writes to fix. The writes use runUpdate from the start. Eight characterization scenarios were written against the Lift version first. They pin normalisation (trim, lower-case, de-duplicate, drop blanks, on the way in and on the way into a query), the AND semantics of getProductCodesWithAllTags including that an empty request matches nothing rather than everything, and that setTags replaces by diffing rather than truncating. The DDL export lost something and it matters: the entity declared UniqueIndex(BankId, ProductCode, Tag) through dbIndexes, but only the plain Index(BankId, ProductCode) came out of the dump. The unique index is added by hand in V007. It is load-bearing rather than decorative - setTags only stays race-free at row level because the database refuses a duplicate triple, which is exactly why the original avoided truncate-and-reinsert. ProductTagsProvider keeps its name and delegates, so the two call sites in LocalMappedConnector did not have to change. Suite 3555/0: 3547 plus these eight.
Eighth table off Mapper. Gate 133 -> 132.
Written rather than ported; the reference branch never migrated this table.
getAllConnectorTraces builds a query from nine independent filters plus ordering
and paging. A rewrite that drops one fails silently - the endpoint just returns
rows it should have excluded - so the six characterization scenarios assert each
filter by showing a non-matching row is left out, not merely that a matching row
comes back.
The entity leaked into the API layer: getAllConnectorTraces returned
List[ConnectorTrace] and JSONFactory600 read Lift fields off it. It now answers
with a ConnectorTraceRow and the factory reads that, which is what let the entity
go. The nullable date column becomes the epoch when absent, matching what
MappedDateTime returned for an unset value.
Two things this table taught, both now written down:
- the table is connector_trace, not connectortrace - the entity overrode
dbTableName. My first test cleared the wrong table and aborted the run.
- FlywayBaselineExport writes a V001 covering every table, which collides with
the real V001 ('Found more than one migration with version 001'). It has to
be deleted from src/main/resources AND target/classes afterwards; a stale
copy on the runtime classpath fails identically. The tool now says so at the
top.
Also fixed in the test: the framework reset runs per test class, not per
scenario, so rows accumulated between scenarios until it cleared the table itself.
Suite 3561/0: 3555 plus these six.
Ninth table off Mapper. Gate 132 -> 131. This one needed no Doobie provider written at all. The ConsentItem entity had no provider and no call sites: grep finds no create/find/findAll anywhere. Every real access to the table is already raw SQL - DoobieConsentQueries, MappedConsent, the v5.1.0 endpoints, and the reference-id migration. The entity existed purely so Schemifier would create the table, so the migration is a change of who creates it and nothing else. The compile passed first try, which is the evidence that reading was right. The test therefore asserts what the entity was actually for: the table exists and carries the columns the surrounding SQL names. Those queries spell columns out, so a missing one is a runtime failure in the consent endpoints, not a compile error. It caught a wrong assumption of mine on the first run - I listed the columns from the field names and got 'column consentitemid missing'. Every field overrode dbColumnName to snake_case. Same lesson as connector_trace's dbTableName: names have to be read off the entity or the export, never inferred. Suite 3563/0: 3561 plus these two.
Tenth table off Mapper. Gate 131 -> 130.
Written rather than ported; the reference branch never migrated this table.
Two behaviours carried over deliberately, both easy to lose:
- getByOperationId stays cached with the same TTL prop and the same cache-key
shape. That string is the Redis key, so only the provider class name inside
it changes, exactly as the class did.
- update returns Empty for an unknown operation id rather than inserting. The
Mapper version did find-then-save and fell through to Empty, and the endpoint
tells update and create apart by that.
buildOne had to be widened as well: its return type was
MappedJsonSchemaValidationProvider.type, naming the concrete object, so the
provider could not be swapped without editing that line. It returns the trait now.
The UNIQUE index on operationid is added by hand in V010 - the entity declared it
through dbIndexes and the export drops those, the same gap producttag hit. It is
load-bearing here: getByOperationId and update both assume one row per operation
id, and update's find-then-write would otherwise race into duplicates.
Six provider scenarios were written first, including one that stores a 200-field
schema: JsonSchema is a MappedText, and a rewrite that gave it a bounded VARCHAR
would pass every other assertion and truncate real schemas. Endpoint tests pass
unchanged (33 with the provider tests).
Suite 3569/0: 3563 plus these six.
…ities The ten tables migrated off Lift Mapper so far each have their DDL in a Flyway script, because Schemifier no longer creates them. None of those scripts were in the repository: .gitignore excludes all of obp-api/src/main/resources, so the files existed only on the machine that wrote them. This is invisible locally and fatal on a clean checkout. Flyway finds nothing to apply, the tables are never created, and the first query against one fails deep inside an endpoint with a SQL error that points nowhere near the cause. Whitelist the migration directory, the same way docs/ and media/ are already whitelisted, and add a test that queries every migrated table. Removing a script makes that test abort with the missing table named, which is the earliest and most direct signal available.
…chema Eleventh table off Lift Mapper. The provider is written rather than ported - the Doobie branch never covered this table. createOrUpdate stays an upsert keyed on the transaction type id. That is load bearing rather than cosmetic: the table carries UniqueIndex(mTransactionTypeId) and UniqueIndex(mBankId, mShortCode), so an unconditional insert collides on the second call for the same type. Both unique indexes are declared in the migration by hand, because FlywayBaselineExport drops indexes declared through dbIndexes and exports only the columns. Writes go through runUpdate. Outside a request scope runQuery falls back to a transactor with Strategy.void over a pool that has autoCommit off, so an insert issued there is rolled back on return and the row silently never appears. The endpoint tests cover this table's behaviour end to end and stay untouched; only their cleanup moves to SQL, since the entity they used for it is gone.
PemUsage arrived in a commit titled "PEM Usage - WIP" and was never finished. The provider trait declares no methods, the implementation object is empty, the injector's vend is called from nowhere, and the entity was never added to Boot.ToSchemify - so the table it describes has never existed in any environment. Nothing to migrate to Doobie here: there is no behaviour to preserve and no table to hand over to Flyway. Deleting the package is the whole change, and it takes one more Lift entity out of the way of the Scala 3 flip. Its entry in MappedClassNameTest's exemption list goes with it, so that list stays a description of entities that exist.
…losed RequestScopeConnection publishes the current request's connection through a thread-local so work submitted to a Future keeps using the request's transaction. The proxy outlives the request whenever a task submitted late in request A runs after A's withBusinessDBTransaction has committed and closed the real connection - the thread it lands on still carries A's proxy. RequestAwareConnectionManager already guards the Lift side: it asks the proxy whether it is closed and falls back to a fresh vendor connection. DoobieUtil did not. Its secondary path even checked isClosed on DB.currentConnection, so the guard was missing only from the primary one, which is the path that actually carries request scope. The result was a query issued through HikariCP's closed-connection stub, failing with "Connection is closed" and surfacing as a 500 on a request with nothing wrong with it. It needs one request's async tail to overlap the next, so it did not reproduce in isolation and appeared as a few failures in a different suite on every parallel run - flakiness in shape, one missing check in substance. A full run had 45 of these in a single shard and 15 consequent failures; with the guard in place that shard has none and the suite is green. This is not test-only. The same overlap happens under any concurrent load, and grows with every table moved onto Doobie. The test builds the state directly rather than racing for it, and pins both halves: a dead proxy must fall back to the pool, and a live one must still be used, since a guard that always chose the pool would silently stop queries from seeing their own request's uncommitted writes.
Thirteenth table off Lift Mapper. Two call sites, not one: the Lift-era APIUtil.checkIfModifiedSinceHeader and its http4s counterpart in Http4s510 carry the same look-up-then-write logic, and both move to the new store. The table keeps its unique index on the cache key. FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them - confirmed by reading information_schema.indexes on a booted instance, which shows ETAG_ETAGRESOURCE as a UNIQUE INDEX. The exporter's header now says so, because reading its output as authoritative leads to quietly dropping constraints. The store writes through runUpdate: both writes happen inside a Future the request does not wait for, and runQuery's out-of-request fallback transactor is Strategy.void over a pool with autoCommit off, so those writes would be rolled back on return. Table identifiers are unquoted here. Quoted identifiers are case-sensitive and the table is created as ETAG, so a quoted "ETag" finds nothing - and the resulting failure is silent rather than red: the reset in ServerSetup runs while ScalaTest is still discovering suites, so a throwing statement there makes every suite fail to instantiate and the run reports zero tests rather than a failure. Covered end to end by ResponseHeadersTest, which drives create, update and the 304 hit, and depends on the async writes actually landing.
…s the schema Fourteenth table off Lift Mapper. The provider keeps the two behaviours the endpoints depend on: getByOperationId stays cached under the same TTL rule including the zero TTL in test mode, and update returns Empty for an unknown operation id rather than inserting, which is how the endpoint tells update apart from create. Allowed types stay one comma-separated string, so rows written before this change still read back. The unique index on the operation id is carried over explicitly, and the guard test now asserts unique indexes rather than only table existence. That assertion exists because FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them - a table copied out of that export looks complete and silently loses its constraint, with the only symptom being inserts that should have been rejected starting to succeed. The expected set was read from information_schema.indexes on a booted instance and confirms every unique index carried so far is correct. Covered by AuthenticationTypeValidationTest's 27 scenarios, which exercise create, update, delete and the cached lookup through the endpoints.
Fifteenth table off Lift Mapper. The entity leaked into signatures - lockUser returned Box[UserLocks] and JSONFactory400.createUserLockStatusJson took one - so both now use UserLocksTrait, which is all either side ever read from it. lockUser keeps its upsert shape: refresh the timestamp on an existing lock, otherwise insert with typeOfLock "lock_via_api". Re-locking must not add a second row, and USERLOCKS_USERID backs that up; the index is carried over explicitly and listed in the guard test, since FlywayBaselineExport does not emit it. unlockUser still answers Full(true) when there was no lock to remove. Callers read that as "not locked now" rather than "a row was deleted", and the endpoints would turn a stricter answer into a spurious failure. Covered by both LockUserTest suites and by DirectLoginTest, which locks a user and then asserts login is refused - so the write has to be visible to a later request.
Sixteenth table off Lift Mapper. MethodBody keeps its unbounded column - it is a MappedText holding whole method sources, and connector method bodies can run to several kilobytes. Lang stays nullable and defaults to "Scala" on read, matching the Mapper version. That default is not cosmetic: DynamicScalaCompiler picks its compiler from this value, and rows written before the column existed have it null - dropping the default would send an existing connector method through the wrong compiler. Both unique indexes are carried over explicitly and added to the guard test. FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them; both are confirmed present on a booted instance. The one on the method name is what makes getByMethodName a single-row lookup, which the connector dispatch path assumes. Covered by ConnectorMethodTest, DynamicCodeKillSwitchTest and ConnectorTest, which create, look up by name and by id, and exercise the compiled method through the dynamic connector.
…chema Seventeenth table off Lift Mapper. No injector sits in front of the provider - callers referenced MappedApiCollectionEndpointsProvider directly - so this moves the object itself to DoobieApiCollectionEndpointsProvider and updates every call site (NewStyle, Http4sResourceDocs, Http4s400, and one now-unused import in ResourceDocsAPIMethods). ApiCollectionEndpointTrait moves from the entity file into the provider file, since nothing else declared it. There is no update path - the Mapper version had none either, only create/get/ delete - so createdAt and updatedAt are both stamped once at insert. deleteApiCollectionEndpointById keeps its find-then-delete shape and stays Empty for a missing id rather than Full(false). NewStyle.deleteApiCollectionEndpointById unboxes the result with unboxFullOrFail, which only turns a missing row into an error on Empty - Full(false) would have been read as a successful no-op delete of an id that was never there. Both unique indexes are carried over explicitly and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them. The one on (apiCollectionId, operationId) is what stops the same endpoint being added twice to the same collection - createApiCollectionEndpoint does not check first, it relies on the database rejecting the duplicate. Covered end to end by ApiCollectionEndpointTest, which creates, lists, fetches a single endpoint, deletes it, and lists again to see it gone.
…chema Eighteenth table off Lift Mapper. Neither this table nor the v6.0.0 endpoints that use it (createFeaturedApiCollection, getFeaturedApiCollectionsAdmin, updateFeaturedApiCollection, deleteFeaturedApiCollection) had any test coverage - grep turns up only commented-out ResourceDoc registrations and one endpoint-name mention in frozen_type_meta_data. FeaturedApiCollectionsProviderTest is written first against the Mapper version to pin the contract this replaces: create then read back by either key, sort order on getAllFeaturedApiCollections, update in place, and delete by either key. As with ApiCollectionEndpoint, there is no injector in front of the provider - NewStyle called MappedFeaturedApiCollectionsProvider directly - so the object itself becomes DoobieFeaturedApiCollectionsProvider and the one call site moves with it. FeaturedApiCollectionTrait moves into the provider file; every other file already used the trait rather than the concrete type. Both unique indexes are carried over explicitly and added to the guard test. The one on apiCollectionId is what backs NewStyle.checkFeaturedApiCollectionDoesNotExist: that function reads the row back rather than trusting the insert to fail, so the database still has to reject a duplicate that slips past a race between the check and the insert. Both delete methods keep their find-then-delete shape and stay Empty for a missing row rather than Full(false), matching NewStyle.deleteFeaturedApiCollectionByApiCollectionId's use of unboxFullOrFail, which only turns a missing row into an error on Empty.
… Lift Mapper Nineteenth table off Lift Mapper. Neither the table nor the v1.3 Berlin Group AIS flows that use it had any test coverage, so ConsentAuthContextProviderTest is written first, driven through ConsentAuthContextProvider.vend like the real callers, to pin the contract before changing the implementation. That test surfaced a genuine bug in the Mapper provider's createOrUpdateConsentAuthContexts: the update branch's inner lambda parameter shadows the outer one - `.map(authContext => authContext.Key(authContext.key).Value(authContext.value).saveMe())` - so `authContext.key`/`.value` read the found row's own existing fields instead of the incoming BasicUserAuthContext's, and the "update" silently rewrites a row with its own current values. Every update through this path has been a no-op since it was written; only create-then-immediately-read masked it, since a fresh row already holds the value being "updated" to. The characterization test fails against the Mapper version for exactly this reason and passes against the new Doobie provider, which writes the incoming key/value. createConsentAuthContext keeps its always-insert, no-existence-check behaviour - duplicate (consentId, key) pairs are intentional, callers are expected to namespace their keys. The unique index backing this is (consentId, key, createdAt), which two writes for the same key inside the same millisecond can collide against; that is a real, narrow race in the existing design and this migration does not change it. The characterization test spaces its own two back-to-back writes out to avoid asserting on that race by accident. MigrationOfConsentAuthContextDropIndex - a historical one-time migration that already ran in every existing environment - no longer references the deleted Mapper entity; it checks for the table by name instead of via DbFunction.tableExists(MetaMapper), which needed the entity only for its _dbTableNameLC. Left in place so migration_script_log stays a complete history; a fresh environment's Flyway-created table never had the legacy index it drops, so the drop is a no-op there. The unique index is carried over and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them.
…ft Mapper Twentieth table off Lift Mapper. Sibling of the ConsentAuthContext migration - same table shape, same provider shape, and the same copy-pasted bug. UserAuthContextTest already covers createUserAuthContext (the always-insert path) end to end; nothing covered createOrUpdateUserAuthContexts, which is what AuthUser's login flow (external/SSO auth contexts) and ConsentUtil actually call. UserAuthContextProviderTest is written first, driven through UserAuthContextProvider.vend like the real callers, and it finds the same variable-shadowing bug as the consent-auth-context provider had: the update branch's inner lambda parameter shadows the outer one, so it saves the found row's own existing key/value back onto itself. Every update through this path has been a no-op since it was written - a user's auth context value set once never refreshed on a later login or consent flow. The characterization test fails against the Mapper version for exactly that reason and passes against the new Doobie provider, which writes the incoming key/value. createOrUpdateUserAuthContexts's create branch keeps its exact original shape too: it inserts without a consumerId, bypassing the check createUserAuthContext enforces, because the Mapper version's create branch called MappedUserAuthContext.create directly rather than going through the checked path. Three historical runtime migrations referenced the entity only to locate the table (DbFunction.tableExists(MetaMapper), which needs a MetaMapper only for its _dbTableNameLC) or to walk/delete rows with Mapper's typed API. All three now reference the table by name: MigrationOfMappedUserAuthContext and MigrationOfUserAuthContextFieldLength swap DbFunction.tableExists for tableExistsByName, and MigrationOfUserAuthContext's duplicate-row cleanup moves from findAll/delete_! to DoobieUtil with a proper IN-list. A new DbFunction.makeBackUpOfTableByName(tableName) generalises the existing MetaMapper-based backup helper, which now delegates to it. All three migrations are historical - every environment that had already run them has that recorded in migration_script_log, and each is a no-op against a fresh Flyway-created table (never had the legacy index, already created at the widened column length, or has no duplicate rows to remove). Kept only so migration_script_log stays a complete history. The unique index is carried over and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them.
Twenty-first table off Lift Mapper. Fired from AfterApiAuth on every login to record one-off "has this user done X yet" flags (create-or-update-bank, add-entitlement, add-bank-account); nothing in the suite exercised it, so UserInitActionProviderTest is written first against the Mapper version to pin create-then-update-in-place, the (userId, actionName, actionValue) triple acting as the full key, and that different users do not collide. Every caller discards the return value of createOrUpdateInitAction - only the write matters - so the entity's replacement is a plain case class (UserInitActionRow) rather than anything wired through a provider trait; there was no trait here to begin with; UserInitActionProvider stays a plain object with no injector. The unique index on the full (userId, actionName, actionValue) triple is carried over explicitly and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them. It is what makes "find then update in place" correct - the whole point of this table is one row per triple.
Twenty-second table off Lift Mapper. Nothing exercised it before this change; AccountIdMappingProviderTest is written first, confirmed against the Mapper version before the entity was touched, then confirmed again against the Doobie provider: get-or-create keyed on accountPlainTextReference, the reverse lookup by accountId, and that different references get different ids. The provider stays named MappedAccountIdMappingProvider rather than a Doobie* one. DynamicUtil's compiled-code template hands this exact import to every dynamic connector method, and connector method bodies are stored as raw Scala source in the connectormethod table and compiled at request time - a bank's already-deployed dynamic connector code can reference this name by hand. Renaming the object would break that code on its next compile for no benefit; Helper.convertToId/convertToReference call it directly too. Both unique indexes are carried over and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them. Neither index actually constrains accountPlainTextReference on its own, though - only mAccountId is unique, and every insert gets a fresh random UUID for it - so two concurrent creates for the same accountPlainTextReference do not collide and can both succeed despite getOrCreateAccountId's comment describing a retry for exactly that collision. That gap is reproduced as-is rather than tightened here: it is a schema/business-rule question this table's existing rows already live under, not something to decide inside a migration whose job is preserving behaviour.
…hema Twenty-third table off Lift Mapper. Sibling of AccountIdMapping - same table shape, same provider shape, same schema gap. TransactionIdMappingProviderTest is written first and confirmed against the Mapper version: get-or-create keyed on transactionPlainTextReference, the reverse lookup by transactionId, and that different references get different ids. Unlike AccountIdMapping's provider, this one is not referenced by name from DynamicUtil's compiled-code template, so it is free to rename; DoobieTransactionIdMappingProvider replaces MappedTransactionIdMappingProvider, including at its one direct call site in Helper.convertToId/convertToReference. Both unique indexes are carried over and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them. As with the sibling table, neither index actually constrains transactionPlainTextReference on its own - only TransactionId is unique, and every insert gets a fresh random UUID for it - so the same concurrent-duplicate gap exists here and is reproduced rather than tightened, for the same reason: it is a schema question this table's existing rows already live under, not something to decide inside a migration whose job is preserving behaviour.
… schema Twenty-fourth table off Lift Mapper. Third of the id-mapping triplet (AccountIdMapping, TransactionIdMapping, this one) - same table shape, same provider shape, same schema gap already documented on the first two. CustomerIdMappingProviderTest is written first and confirmed against the Mapper version. MappedCustomerIdMapping had a second, non-provider caller: DeleteCustomerCascade.deleteCustomerIdMapping called MappedCustomerIdMapping.bulkDelete_!! directly. That moves to a plain DELETE through DoobieUtil - DeletionUtil.databaseAtomicTask wraps callers in DB.use(DefaultConnectionIdentifier), which is exactly the fallback DoobieUtil.currentRequestConnection already reads Lift's DB.currentConnection for, so the delete participates in the same Mapper transaction as the rest of the cascade. Covered by DeleteCustomerCascadeTest, unchanged. The provider stays named MappedCustomerIdMappingProvider rather than a Doobie* one, for the same reason as MappedAccountIdMappingProvider: DynamicUtil's compiled-code template hands this exact import to every dynamic connector method, and a bank's already-deployed dynamic connector code can reference it by hand. mBankId/mCustomerNumber are deprecated columns (since 2019-08-23, "We used customerPlainTextReference instead") that neither provider method returns anything carrying, so the migration keeps the columns without threading them through the new provider. Both unique indexes are carried over and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them. As with the other two id-mapping tables, neither index actually constrains mCustomerPlainTextReference on its own; reproduced rather than tightened here.
…chema Twenty-fifth table off Lift Mapper. Nothing in the current codebase creates or reads this table - the only reference anywhere was DeleteAccountCascade.deleteBankAccountData bulk-deleting from it, presumably a leftover of a feature that used to write here. That delete moves to a plain SQL DELETE through DoobieUtil; the table itself is kept rather than dropped, since a production instance may still hold rows from whenever this was in active use, and cascade delete needs a real table to clear them from. The unique index on (bankId, accountId) is carried over and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them. Covered by DeleteAccountCascadeTest, unchanged.
Twenty-sixth table off Lift Mapper. No injector sits in front of the provider - callers referenced MappedApiCollectionsProvider directly, same as ApiCollectionEndpoint and FeaturedApiCollection before it - so this moves the object itself to DoobieApiCollectionsProvider and updates every call site (NewStyle, Http4s400, and ExampleValue's glossary text, which read ApiCollection.Description.maxLen off the Mapper field metadata and now states the same 2000-character limit as a literal). ApiCollectionTrait moves from the entity file into the provider file, since nothing else declared it. Both unique indexes are carried over explicitly and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them. The one on (userId, apiCollectionName) is what stops one user creating two collections with the same name - createApiCollection does not check first, it relies on the database rejecting the duplicate. updateApiCollectionById and deleteApiCollectionById keep their find-then-write/ find-then-delete shape and stay Empty for a missing id rather than Full(false): both of NewStyle's callers unbox the result with unboxFullOrFail, which only turns a missing row into an error on Empty. Covered end to end by both v4.0.0 and v5.1.0 ApiCollectionTest, and indirectly by FeaturedApiCollectionsProviderTest, which creates api collections as fixtures.
…chema
Twenty-seventh table off Lift Mapper. Security-critical - it backs account
lockout - and already partially prepared for this: DoobieBadLoginAttemptQueries
existed with an atomic UPDATE ... SET counter = counter + 1 for the concurrent
lost-update fix documented in CONCURRENCY_HAZARDS.md (hazard H), used only for
the increment path while every other operation still went through the Mapper
entity directly. This finishes the table: find, create, and resetBadLoginAttempts
move into the same object, and LoginAttempt (code.loginattempts.LoginAttempts.scala)
now goes through it end to end rather than mixing Doobie and Mapper calls.
Two other direct callers of the entity, outside the provider:
- LiftUsers.getUsers (locked/active user filtering) called
MappedBadLoginAttempt.findAll(By_>(...)) directly to find usernames over the
attempt threshold; that becomes
DoobieBadLoginAttemptQueries.usernamesOverThreshold.
- ConcurrentSecurityRaceTest's own fixture setup and assertion (scenario H)
used the Mapper API directly to seed and read the counter; both move to the
same Doobie queries the production code now uses. The scenario still passes
with all 8 concurrent increments landing, which is the actual regression
test for the atomic-update fix - if migrating this table had reintroduced a
read-modify-write race, this would be the test to catch it.
MigrationOfMappedBadLoginAttemptDropIndex - a historical migration that already
ran everywhere - no longer references the deleted entity; it checks for the
table by name instead of via DbFunction.tableExists(MetaMapper).
The unique index is carried over and added to the guard test, for the same
reason as every table so far: FlywayBaselineExport does not emit
dbIndexes-declared unique indexes even though Schemifier creates them. It is not
the index the historical migration drops - that one constrained mUsername alone
and would have rejected the same username under two different providers; this
one is (provider, mUsername).
Twenty-eighth table off Lift Mapper - the widest blast radius so far. The entity was not behind a single provider: it was reached from seven files directly (ConsentUtil, MigrationOfAccountRoutings, LocalMappedConnector, LocalMappedConnectorInternal, MappedBankAccount, LocalMappedConnectorDataImport, DeleteAccountCascade), and its two read methods (getAccountRouting/getAccountRoutingsByScheme) are part of the public Connector trait interface, returning the concrete Mapper type. DoobieBankAccountRoutingQueries now holds every query these call sites need; Connector.scala and NewStyle.scala's two signatures move to BankAccountRoutingTrait (obp-commons), the same trait the entity already implemented, so nothing downstream that only reads .bankId/.accountId/.accountRouting off the result needed to change. getBankAccountByRoutingLegacy's OBP-family fallback logic (try the implicit account-id reading first, fall back to a registered routing) and updateBankAccount's diff-based add/update/delete of routing schemes are ported statement-for-statement rather than restructured - both encode non-obvious behaviour with their own regression coverage (ObpAccountRoutingResolutionTest for the former). MigrationOfAccountRoutings - a historical migration - no longer references the deleted entity: its tableExists check moves to tableExistsByName, and its private, unreferenced createBankAccountRouting helper (not called by populate() or anything else, kept rather than deleted) is rewritten against DoobieBankAccountRoutingQueries instead of quietly dropped. Eight test files reached the entity directly as fixture setup rather than through any provider: five Berlin Group suites (AIS/PIIS/PIS/SBS + BerlinGroupConsentFixtures), SandboxDataLoadingTest's six unconditional bulkDelete_!! resets, ObpAccountRoutingResolutionTest (the OBP-scheme regression test), and LocalMappedConnectorTestSetup. All move to the same Doobie queries the production code now uses. Both unique indexes are carried over and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them. Both are read directly by application code (getBankAccountByRoutingLegacy, getAccountRouting) rather than only relied on implicitly. Covered end to end by the five Berlin Group suites, v3.1.0 AccountTest, v7 Http4s700RoutesTest (153 scenarios), and ObpAccountRoutingResolutionTest - 253 scenarios total, all green including the OBP-scheme fallback regression test.
Twenty-ninth and thirtieth tables off Lift Mapper, done together: MappedFXRate declares a Lift foreign key on MappedCurrency for both its currency-code columns, so neither could move independently. MappedCurrency is deleted outright rather than migrated. It has zero rows, zero application-level reads or writes anywhere in the codebase, and the FK it exists to be the target of was never actually enforced - confirmed by inserting an FX rate for a currency pair absent from MappedCurrency, which succeeded. It is dead code in the same sense PemUsage was: present in the schema, referenced by nothing at runtime. FXRateProviderTest is written first and confirmed against the Mapper version. ExchangeRateTest only covers NewStyle.getExchangeRate's fallback path, which builds an FXRate value without ever calling .saveMe() on it - a real gap, since nothing exercised createOrUpdateFXRate (the actual write path) or getCurrentFxRate's reverse-order lookup. getCurrentFxRate's reverse-order fallback and createOrUpdateFXRate's find-then-write are preserved exactly, including the gap that comes with them: the table has no unique index (only plain indexes on the two currency-code columns, matching Schemifier's real output), so two genuinely concurrent calls for the same (bankId, from, to) can both miss the find and both insert - the same shape as the id-mapping tables' documented gap, not something this migration changes. NewStyle.getExchangeRate's fallback branch keeps its "build without persisting" behaviour, now constructing FXRateRow (a plain case class) instead of an unsaved Mapper instance - there was never a database write on this path to begin with.
Thirty-first table off Lift Mapper - migration bookkeeping itself, the table every historical migration script (including several already ported in this series) reads and writes through Migration.saveLog/isExecuted via MigrationScriptLogProvider.vend. Nothing about that seam changes; only the implementation behind it does. ServerSetup.resetDatabaseForTestClass deliberately excludes this table from its per-test-class wipe: clearing it makes isExecuted always false, so a fresh test JVM would re-run every historical migration against a database that already has their effects, and a migration that retypes a view-projected column then fails outright. That exclusion was an identity check against the Mapper object (`m == MigrationScriptLog`) in a filter over ToSchemify.models; with the entity gone, the table is simply never in that list to begin with, so the check is removed rather than replaced. Every other migrated table gets an explicit DoobieUtil DELETE line in the same function - this is the one deliberate exception, called out in a comment where that DELETE list lives so it isn't added by reflex on the next table. The unique index on (name, isSuccessful) is carried over and added to the guard test, for the same reason as every table so far: FlywayBaselineExport does not emit dbIndexes-declared unique indexes even though Schemifier creates them. saveLog's find-then-write keys on exactly that pair. Covered by MigrationsTest end to end, and by the full suite staying green across every shard's many test classes in one run - the actual regression test for the exclusion, since a reintroduced wipe would only surface as a boot failure partway through a shard's test classes, not in any single test.
…he schema Thirty-second table off Lift Mapper. A write-only audit record: nothing in the codebase reads it back. LocalMappedConnector.saveTransactionRequestReasons writes rows alongside a transaction request's creation and never queries this table again. TransactionRequestReasonsProviderTest reads rows back directly to confirm the write itself is correct, since there is no production read path whose test would otherwise catch a column-mapping mistake. No unique index - only the primary key, matching Schemifier's real output. That is expected here, not a gap: multiple reasons naturally attach to one transactionRequestId, and nothing about the table was ever meant to enforce one-per-anything.
The dangling-$ref check walked the example bodies and translated each one, so it saw only the definitions a resource doc names directly. Most are reached as the target of a $ref from another: AllConsentJsonV510 is published because ConsentsJsonV510 holds a list of it, never as a body in its own right - and its frequency_per_day and remaining_requests were still `$ref:Object`, invisible to the per-body walk. Build the definitions the way the server builds them instead - createResourceDocsJson then loadDefinitions, as Http4sResourceDocs' swagger branch does - and walk that. loadDefinitions does the nested entity walk, so the assertion now covers the whole published surface. allConsentJsonV510 gains the two example values that walk then found.
41 sites did `list.asInstanceOf[List[XCommons]]`. The cast is erased, so it checks nothing where it is written; what it does is licence a checkcast at the first element access, and let whatever the elements actually are be serialized. The premise behind it - "the provider only ever constructs XCommons" - stopped holding when the stores moved to Doobie and started returning their own row types implementing the same trait. Not hypothetical: four sites already failed this way and were fixed in `fix: convert Commons list responses instead of casting them` - method_routings, endpoint-mappings, cards/CARD_ID and webui_props all threw ClassCastException instead of serving a response. 40 of the 41 are in MockedRabbitMqAdapter. The other is on a live request path: LocalMappedConnector.getProductCollectionItemsTree casts the attributes it gets from DoobieProductAttributeProvider, whose rows are ProductAttributeRow - not ProductAttributeCommons. It has not thrown yet only because createProductCollectionsTreeJson happens to take the trait; any consumer reading the tree's attributes at the Commons type would. Every XCommons companion extends Converter/ConverterWithType, so toCommonsList - which the four earlier fixes already used - was available at each of them. CommonsListConversionTest pins the mechanism on the real row type: the cast survives being written and throws on first use, toCommonsList gives back a list that does not. check_no_blind_commons_casts.py keeps the pattern from returning, and runs in both workflows and in run_tests_parallel.sh alongside the other three lints. Its scope is `List[...Commons]` deliberately: a cast to a list of something with no Converter behind it is a different question, left alone rather than swept in.
PostgresMigrationTest is the only thing that exercises the Postgres DDL - the rest of the suite runs on H2 - and it opened with `assume(postgresReachable)`. CI had no Postgres, so it cancelled itself on every run. A cancelled test reports as a pass, so nothing distinguished "CI verified the Postgres DDL" from "CI never ran the check" except reading the log for a cancellation nobody was looking for. That matters more since the changeover: the Postgres DDL is generated from the changelog at boot, so no one reads it before it runs. Both workflows now run a postgres:16-alpine service alongside the redis one, and set OBP_TEST_POSTGRES_URL/_USER/_PASSWORD for the test step. Only the shard that owns code.api.util runs the test; the variables are inert on the rest. OBP_TEST_POSTGRES_REQUIRED=true turns the cancellation into a failure, so a broken service, a wrong URL or a dropped `services:` block fails the build rather than quietly restoring the old behaviour. Developers leave it unset and keep the skip. The decision lives in PostgresTestTarget, which takes `required` as a parameter defaulted from the environment - the environment cannot be changed from inside the JVM, and a branch no test can enter is the kind of thing this whole change is about. PostgresTestTargetTest covers both branches and the parsing rule.
bringUpToDate chose between `update` and `changeLogSync` by looking at whether DATABASECHANGELOG existed. That was wrong in both directions, and both failures land on the deployments the adoption path exists to serve. A blanket changeLogSync marks the whole changelog applied on the strength of the tables being there - including the de-duplications and the unique indexes they clear the way for. Schemifier never created those indexes; that is why V057 and V116 existed. So a database reaching this build from Schemifier, holding duplicate rows and no constraint, had both recorded as done without either happening: the databases that needed the de-duplication were exactly the ones that skipped it. The same blindness loses a whole table - a legacy schema missing one had it marked created and was left without it. And a sync writes DATABASECHANGELOG row by row, committing as it goes, so a start killed during one leaves the table present and short of its rows. The next start saw a DATABASECHANGELOG, concluded the database was adopted and ran a plain `update` over objects that already existed - `MigrationFailedException ... Index "METRIC_CONSUMERID" already exists` - on that start and on every one after it. Verified against a real restart, not inferred. Every baseline changeset now carries `not tableExists` / `not indexExists` with `onFail: MARK_RAN`, so it decides for itself whether its object is there, and bringUpToDate is a plain `update` for all three states: empty, tables-without-a-record, and interrupted at any point in either. Preconditions are not part of a changeset's checksum - ChangeSet.generateCheckSum reads only the changes and the sql visitors - so nothing is invalidated on a database that has already run them. The index names in the changelog are already the truncated forms Postgres stores, so indexExists matches on both vendors. What is still not covered is a table that exists with the wrong columns; that was equally true of changeLogSync, the difference being that the mismatch is now per-object rather than whole-changelog. The cost is real and worth stating: an empty-database build goes from 1.8s to 13.8s on H2, because each precondition makes Liquibase snapshot the object (~35ms x 410). That is a one-off at boot and once per test JVM that builds a schema. The tests clone the schema into a database Liquibase has not touched in this JVM, via H2 SCRIPT/RUNSCRIPT. That is not ceremony: Liquibase keeps the applied-changeset list per database inside the process, so a fixture that edits DATABASECHANGELOG behind its back is invisible - `update` reports "Database is up to date" and does nothing, in a JVM where the same state aborts the boot of a fresh one. Confirmed by running the two phases as separate JVMs against an H2 file database before writing the in-suite version. scripts/normalise_generated_changelog.py inserts the preconditions, so a regeneration keeps them; its output was checked byte-for-byte against the committed file. check_changelog_preconditions.py fails if a changeset loses one - a changelog without them looks right and passes every fresh-database test in the suite, breaking only on the upgrades this exists to serve.
…re left Seventeen fields were frozen as `Option[Object]`. FrozenClassTest exists to fail when a STABLE API's shape changes, and a contract that says `Option[Object]` cannot fail when `Option[Long]` becomes `Option[Int]` - which is the change it is there to catch. They went in with the regeneration the Scala 3 flip needed, so the loss of precision was committed rather than noticed. The cause is the same one that broke the published swagger: scala-reflect reads ScalaSig, an attribute only Scala 2 classes carry, and on a Scala 3 class it falls back to the class file's Java generic signature, where a value type cannot be a type argument - `Option[Long]` is emitted as `scala.Option<java.lang.Object>`. Reference types keep their argument, which is why the damage is exactly Option of a value type. The example value is the only runtime source of the erased type, and each of these has one - the swagger fix put them there, and SwaggerFactoryUnitTest keeps them there, because the published schema derives the same types from the same place. FrozenClassUtil walks the example bodies once per class and refines from them; all seventeen come back as what they are declared to be, and the regenerated fixture differs in exactly those seventeen lines and nothing else. FrozenTypePrecisionTest fails if anything reaches the fixture still erased - whether because a new field arrives without an example or because the refinement is removed - so this cannot quietly degrade again.
Boot de-duplicated mappedentitlement and mapperaccountholders itself, through deduplicateBeforeUniqueIndexSchemify(), so their unique indexes could be built on a database that still held duplicate rows. Two things made that both dead and dangerous. Dead: it named a table that does not exist - `mapperaccountholder` for `mapperaccountholders`, and `user_` for `user_c` - and its first act is a table-existence probe, so that half returned silently and had never run. And it was placed to run before schemifyAll() issued the CREATE UNIQUE INDEX; schemifyAll() issues nothing now (ToSchemify.models is Nil) and the index comes from the Liquibase call fourteen lines earlier, so it already ran after the thing it existed to precede. Dangerous: once each baseline changeset carries `not indexExists` (the preceding boot-path fix), the CREATE UNIQUE INDEX actually runs on a legacy database - and with the de-duplication a no-op, it fails on the duplicate rows. That turned a working boot into a failing one on exactly the databases this code existed to serve. So the de-duplication moves into db.changelog-dedup.yaml as two changesets, guarded by tableExists / MARK_RAN like the eight already there, and the Boot call and method are removed. They carry no `dbms` restriction: the eight use `NOT IN (SELECT MIN ...)`, which names the table inside its own subquery and MySQL/MariaDB reject with ERROR 1093, so those are h2/postgresql only - but the Scala these replace ran on every vendor OBP ships a driver for, and taking that away would fail a MySQL boot rather than leave it as-is. They use the ROW_NUMBER() derived-table form the Scala used, which every target's window functions support and whose materialised derived table sidesteps 1093. They do NOT copy the Scala's NULL handling: it partitioned on the raw columns, grouping NULLs where a unique index keeps them apart, so it deleted rows that could never violate the index; these leave NULL keys alone like the other eight. check_changelog_data_migrations.py freezes both new statements (8 -> 10). sample.props.template notes that liquibase.enabled=false now opts out of these data repairs too, matching that switch's existing "you manage the schema yourself" meaning. LiquibaseOnExistingSchemaTest gains a scenario that clones a schema, drops the two indexes, inserts duplicates, and asserts the next boot collapses them and builds the indexes.
…mpty
Three accessors on MappedPhysicalCard turned a column that was never written into a value that says
it was written empty.
`networks` split the raw string with no guard, and `"".split(",")` is `Array("")`, so an empty
column became `["" ]` - the one-element list the sibling `allows` already guards against with
`Option(allowsRaw) match { case Some(x) if !x.isEmpty => ...; case _ => Nil }`. `cvv` and `brand`
were `Some(rawString)` unconditionally, so a column holding nothing became `Some("")`.
mcvv and mbrand were added to the model years after the table existed and Schemifier added them with
no backfill, so every card written before that release holds SQL NULL there; mnetworks the same. The
listing reads through Option to avoid doobie's NonNullableColumnRead, and the first fix collapsed
those Options to "" so the accessors would not dereference a null - which is what produced the empty
values above.
The right split is to keep the raw strings null and make the accessors null-safe, because NULL and
"" mean different things here: NULL is a column that was never written and reads as Nil / None; "" is
a client that actually sent an empty value and must get it back unchanged. Collapsing NULL to ""
merges the two - the fromRow now passes the raw strings through as-is (`.orNull`), `networks` maps
over the Option, and `cvv`/`brand` are `Option(raw)`. An actually-empty string still round-trips as
`[""]` / `Some("")`; only the absent case changes.
NullableColumnReadTest's physical-card scenario asserts Nil/None for the NULL columns; CardTest and
CardAttributeTest, which exercise the `[""]` round trip through the real endpoint, stay green.
Three defects in the erased-type recovery that FrozenClassUtil does when regenerating the frozen contract, all found reviewing the refiner added in the preceding fix. The seen-set gated the WALK, not just the recording: `if (seen.add(obj.getClass))` meant the first instance of a class decided whether anything below it was ever visited. An example body that holds `None` where a later one of the same class holds `Some(nested)` left the nested type unreached, so its erased Option-of-a-value-type fields stayed frozen as Object. Worse, gating the walk on class was the only thing bounding recursion, and these example bodies are lazy vals - a mutual reference is constructible and would recurse until the stack overflowed. Gate on object identity instead (an IdentityHashMap visited-set): that terminates on cycles and on repeats, and visits each object once, while recording stays once-per-class. Map values were never walked: a Map iterates as `(k, v)` pairs and a pair matched no case, so nothing inside a Map-typed field was visited. Added `case (_, v) => walk(v)`. Erasure was detected by `declared.toString != "Option[Object]"`, an exact match on one unqualified rendering - scala-reflect prints the same type as `Option[java.lang.Object]` when the symbol resolves the other way, and the string match would then silently refine nothing. Compare structurally instead: Option whose first type arg's symbol is `java.lang.Object`. FrozenTypePrecisionTest's guard is widened the same way, from `Option[Object]`/`Object` to any `[Object]` shape, so an erased `List[Long]` read as `List[Object]` is caught too.
`code_of` stripped a line at the first `//`, so a `//` inside a string literal - `"http://host/x"` - truncated the line and hid a cast written after it, letting a real offender pass as zero. Replaced the naive split with a string-aware scan: a `//` inside a `"..."` literal no longer ends the line, block- comment interiors (` * ...`) are still dropped whole, and a `/*` opener keeps what precedes it. Five in-file red/green cases pin it, including the string-literal `//` and a cast mentioned in a comment.
The container entrypoints were switched to `-cp "obp-api.jar:lib/*" bootstrap.http4s.Http4sServer` because a jar manifest's Class-Path never reaches the `java.class.path` system property, and both DotcScalaCompiler and json4s's ScalaSigReader build a runtime compiler classpath out of it - under `-jar` the server boots and looks healthy, then 500s on sandbox data import and every dynamic-code and Scala-3 field-type path. Thirteen remaining `java -jar` recommendations across scripts and docs would send anyone reproducing a start by hand straight into that failure. Converted all of them - README, the two flushall run scripts, scripts/mtls_env.sh, the docker README, and the system documentation (including its systemd unit and its scp deploy loop, which now copies the lib directory alongside the jar) - each with the one-line reason.
The block explaining why `update` alone is now correct for every boot state sat two scaladoc comments above the method - the one directly above `bringUpToDate` documented `causedByLockException`, so scaladoc and IDE hover attached the explanation to the wrong member and showed nothing on the method it describes. Moved it down to sit directly above `def bringUpToDate`.
getAllAggregateMetricsBox and getTopConsumersFuture built their WHERE clause by interpolating the caller's filter values through `sqlFriendly`, which is `s"'$value'"` and escapes nothing, then handed the finished string to DBUtil.runQuery - which prepareStatement's it with no bound parameters. Every character of the value is therefore parsed as SQL. The values are request parameters. Both are reachable from a read-only role: GET /obp/v5.1.0/management/aggregate-metrics (canReadAggregateMetrics) and GET /obp/v3.1.0/management/metrics/top-consumers (canReadMetrics), through APIUtil.getHttpRequestUrlParam, which URL-decodes and applies no character filter. `app_name`, `user_id`, `consumer_id`, `url`, `verb`, `correlation_id`, `implemented_by_partial_function` and `implemented_in_version` all reach sqlFriendly; the exclude/include list parameters reach the same sink through `s"'$i'"` and extendLikeQuery. Demonstrated, not inferred. With three rows seeded for one app, `app_name=no-such-app' OR '1'='1` made the aggregate count 3 instead of 0, and made top-consumers return the consumer row it should have filtered out - developer email included. Since the aggregate returns a count, that is a boolean read oracle over the whole database: authuser password hashes, consumer secrets and mappedconsent JWTs are all reachable one character at a time from a role that grants read access to usage counters. Both methods now build MetricsQueryFilters and call DoobieMetricsQueries.getAggregateMetrics / getTopConsumers, whose buildFilterConditions binds every operand. That is the same routing getTopApisFuture in this object already used - the metric-table migration converted it and left these two behind - so the safe path was sitting in the same file. The cache wrapper and cache key are unchanged (CacheKeyGoldenTest pins the key). sqlFriendly, sqlFriendlyInt, trueOrFalse, falseOrTrue, sqlTimestamp and extendLikeQuery have no callers left and are deleted, so the sink cannot be reached again. One behaviour difference worth naming: the old top-consumers branch wrote `userid = null` for the anon filter - bare SQL NULL, never true - where the aggregate branch wrote the string `'null'`. The Doobie path uses `'null'` for both, which is what the anon filter was meant to do. MetricsSqlInjectionTest is the regression, with a positive control on each query so a broken fixture cannot make it pass vacuously. It clears its own cache entries by exact key first: these reads memoize on the query-parameter list alone with a 24-hour TTL for an old fromDate, so a vulnerable build's answer is otherwise served to a fixed one and the fix looks inert. Exact key rather than a wildcard because the local runner shares one Redis across four shards.
getAllAggregateMetricsBox, getTopApisFuture and getTopConsumersFuture each carried their own copy of the same twenty `queryParams.collect` lines and their own MetricsQueryFilters construction, differing only in which fields they bothered to fill. Three near-identical blocks that had to be kept in step by hand: a filter added to one would silently be missing from the others, which is how the two SQL injection sinks came to differ from the sibling that had already been migrated. filtersFrom does it once. The include* fields are filled unconditionally because buildFilterConditions only reads them when isNewVersion is true, so they are inert for the two callers that run the exclude* branch. correlationId stays a parameter rather than becoming always-on, because the difference is real and not an oversight: the aggregate query has always filtered on it, and top-consumers has not - the SQL this replaced extracted a correlation id into a local and then never referenced it. Defaulting it on would have quietly added a filter to top-consumers. Behaviour is otherwise unchanged, including each caller's limit default (10 for top apis, 500 for top consumers). 54 lines in, 104 out.
OBP-OIDC and the Keycloak user-storage provider read v_oidc_users directly over JDBC; neither calls verify-credentials. So the lock check that lives in the HTTP login path is not on their route at all - an operator who locked an account watched the HTTP login refuse it while the same credentials kept working through either IdP. The script this view was lifted from carries a TODO saying exactly that; the view now carries the check instead. Only the explicit lock is expressible here: a userlocks row, which is what UserLocksProvider.lockUser writes and what both admin lock endpoints call. The other half of LoginAttempts.userIsLocked - badloginattempt.mbadattemptssincelastsuccessorreset over the max.bad.login.attempts prop - cannot be, because a view cannot read a prop and hardcoding the default 5 would silently disagree with any deployment that configured another value. Nothing writes userlocks when that counter overflows, so the two halves are independent and this one stands on its own. The limitation is written into the view. Fixing the WHERE clause exposed a second defect in the same select: au.username::text was projected with no alias, so on Postgres the column came back named "text" rather than "username" while H2 - which does not take ::text - named it after the whole cast expression. Both consumers read that column by name. Aliased explicitly. OidcViewLockedUserTest builds the view, locks a user and asserts the row disappears; it fails on the old view for the right reason, not on the column name.
…nothing allow_user_generated_scala_code was set on deployments where Sandbox.runInSandbox still restricted file, network and reflection access. JEP 486 removed SecurityManager in JDK 24, so System.setSecurityManager throws and AccessController.doPrivileged is a pass-through: the same switch now means "run arbitrary user-supplied Scala with the full rights of the JVM", and nothing says so at the point it matters. The only signal was a boot-time warning. compileScalaCode now refuses on a JVM where no SecurityManager is installed unless the operator has said so a second time with allow_user_generated_scala_code_without_sandbox, and returns OBP-50021 naming both the cause and the switch. Refusing to compile rather than refusing to boot keeps the failure scoped to the feature that lost its isolation and leaves a deployment that means it one deliberate edit away from working. Behaviour change, deliberate: a deployment on JDK 24+ that already has allow_user_generated_scala_code=true starts failing dynamic-code compilation until it adds the second switch. Default deployments are unaffected - the feature is off by default. The suite and CI are exactly the case the second switch is written for: knowingly unsandboxed, on throwaway data. Both now declare it - the props template, both workflows' Setup-props step, and run_tests_parallel.sh's env block - and sample.props.template documents it with the default false. Without that declaration the gate blocks the whole dynamic-code tier: 34 scenarios across eight suites. The kill switch still wins on its own; DynamicCodeSandboxGateTest asserts that, and has to force OBP_ALLOW_USER_GENERATED_SCALA_CODE out of the way with withEnvOverride to do it, because an env var always beats setPropsValues.
… so it validated nothing
Validation.validateDependency is the gate that refuses user-supplied Scala calling a restricted
type. It gets its call list from DynamicUtil.getDynamicCodeDependentMethods, which opened with
`if (SHOW_USED_CONNECTOR_METHODS) ... else Nil` - and so did APIUtil.getDependentMethods
underneath it. show_used_connector_methods is a diagnostic: it controls whether a response tells
the caller which connector methods an endpoint used, and it defaults to false. An operator who
switched the security validation on therefore got a validation that inspected an empty list and
passed every restricted call, and could not have fixed it by setting the diagnostic prop either -
SHOW_USED_CONNECTOR_METHODS is a final val on Constant, read once at class initialisation and
frozen for the life of the JVM. Both gates removed; getDependentConnectorMethods, the one caller
the flag actually exists for, still carries it itself.
Removing them exposed why nobody had noticed the scan never ran: it cannot run against a shared
javassist pool. getClassPool handed back ClassPool.getDefault - a process-wide singleton - after
appending a LoaderClassPath for the caller's loader, and APIUtil did the same in two more places.
One path per classloader, none ever removed. That was harmless while the scan was effectively
dead code; it is not once DynamicCompileEndpoint.validateDependencies runs it on every dynamic
endpoint request, because dynamic compilation mints a fresh classloader per snippet. The pool then
grows a search path per snippet, each pinning a classloader whose temp output directory has been
deleted, and every later lookup walks all of them - which is what the MEMORY_USER notes on these
callers were warning about.
The damage does not surface anywhere near javassist. Two suites failed with
"missing reference, looking for JValue/T in package object json4s" out of
dotty.tools.dotc.core.unpickleScala2.Scala2Unpickler - the Scala 3 compiler reading json4s's
Scala 2 pickles while compiling an unrelated later snippet. The dynamic compiler's classpath is
System.getProperty("java.class.path") read once into a val, so it is constant for the JVM and was
never the variable; the shared pool was. Each pool is now scoped to its classloader with
`new ClassPool(true)`, which starts from the system path exactly as getDefault does - lookups
resolve identically, the pool is just no longer shared - and is still memoized per loader.
Verified by isolation: reverting only the un-gating turned those two suites green and left only
DynamicCodeDependencyScanTest red. With the pool scoped, the full suite is 3868/0 on both H2 and
Postgres.
DynamicCodeDependencyScanTest compiles a class that calls a restricted type and fails if the scan
returns an empty list again.
… too checkAnswer short-circuits to true when consents.sca.enabled is false. That is the point of the switch - local development, where nobody can receive an OTP - but it applied wherever it was set. In production it means anyone holding a consent id in INITIATED state can move it to ACCEPTED with an arbitrary string, and the only thing that ever said so was a boot-time warning nobody has to read. Production now ignores the switch and verifies the answer; every other run mode behaves exactly as before. The decision is split out as scaVerificationRequired(scaEnabledProp, isProduction) because run mode cannot be changed from a test - Props.mode is read once at class initialisation - so a pure function is the only seam the behaviour can be asserted through. ConsentScaEnforcementTest pins all four combinations, and fails on the old `if (scaEnabled)` for the production-with-SCA-off one.
… row would not insert
The Lift entity declared email as MappedEmail, whose setFilter is notNull :: toLower :: trim, so
the normalisation lived in the field type and the entity never mentioned it. Carrying the column
across as a plain String dropped it silently and " Bob@Example.COM " began persisting verbatim.
ResourceUser's half of the same migration kept it, so the two stored copies of one user's address
had been disagreeing about case and whitespace - which matters because lookups by email compare
strings. Reuses ResourceUser.normalizeEmail rather than re-implementing it so they cannot drift
apart again, and the returned row carries the normalised value too: handing back the caller's raw
string would return an object that disagrees with what was just written.
Writing the test for that turned up a second defect in the same statements. user_c is a nullable
BIGINT and an AuthUser not yet linked to a resourceuser is a legitimate row, so the unlinked case
has to bind SQL NULL - but written inline in the interpolator as
`${if (row.user > 0L) Some(row.user) else None}` it was not bound as a parameter at all: the
database rejected the statement with a syntax error at that position, and because it is one
statement nothing inserted, not just the FK. Naming the value in a method with a declared
Option[Long] result is what makes it bind. Three sites, insert and update.
AuthUserEmailNormalisationTest covers insert and update; AuthUserUnboundInsertTest inserts an
unlinked row and fails with the syntax error on the inline form.
userIsLocked is the OR of two independent conditions: a row in userlocks, which an operator's lock writes, and mappedbadloginattempt.mbadattemptssincelastsuccessorreset exceeding max.bad.login.attempts. e4ea86b put the first into v_oidc_users and left the second, so an account locked out by failed attempts on the HTTP login path still authenticated through OBP-OIDC and the Keycloak provider - both read this view over JDBC and never call verify-credentials. Nothing writes userlocks when that counter overflows, so the two really are separate and the view has to carry both. The second looked unexpressible: a view cannot read a prop, and a hardcoded 5 would silently disagree with any deployment that configured another value. That reasoning assumed the view is static. It is not - createOidcViews runs on every boot and its changeset is already runOnChange - so the threshold goes in as a Liquibase changelog parameter, substituted before the checksum is computed. Change the prop, restart, and the view is rewritten. Props stay the single source of truth and no second place stores the value. The parameter is set in configure() rather than in createOidcViews because substitution happens when the changelog is parsed, and every path parses the whole master changelog - bringUpToDate only filters the oidc-views context out at execution time. Bound as an Int, not passed through as the raw prop string: it is substituted into DDL, so a string would let a malformed prop become SQL. A value that is not a number falls back to the prop's own default and logs an error rather than failing the boot - there is no configured value to honour in that case, and refusing to start would take down a deployment that today only breaks when somebody logs in. Strictly greater, not >=, mirroring userIsLocked: the view must not be stricter than the HTTP path. NOT EXISTS rather than a join, because (provider, musername) carries no uniqueness constraint and a join would duplicate user rows. Keyed on resourceuser's provider_/name_ - the pair every userIsLocked(user.provider, user.name) call site passes, and the pair DoobieUserQueries already joins this table on. Three scenarios: over the limit is hidden, exactly at the limit is not (the off-by-one guard), and moving max.bad.login.attempts moves the boundary with it - that last one is what fails if a hardcoded default is ever reintroduced.
…self stands 125950a scoped the javassist pool per classloader and its message presented that as the proven cause of two failures seen at the time - DynamicUtilTest and InternalConnectorTest reporting "missing reference, looking for JValue/T in package object json4s" - "verified by isolation". That attribution was wrong. The cause was a cross-checkout ~/.m2 overwrite: another checkout's mvn install replacing com.tesobe:obp-commons, which carries no Scala suffix, so nothing detects the mismatch. The error named it four lines below the one that gets read - "A signature in ~/.m2/.../obp-commons-1.10.1.jar refers to JValue/T in package object org.json4s.package which is not available ... the version on the classpath might be incompatible with the version used when compiling" it. Reading only the first line sent the investigation to dotty and javassist instead. Established by measurement, not inference. Fingerprinting the jar during a run caught the swap live, with the offending maven process's working directory recorded alongside it, and the other checkout confirmed both installs. Running the suite against an isolated repository (-Dmaven.repo.local, seeded with hard links so no dependency is re-downloaded) gives 3870/0 on H2 and Postgres with the scoping in place and nothing else changed. The earlier isolation experiment was confounded: a green run only meant ~/.m2 happened to be correct that time. Why it took so long is worth recording, because it is not about care. The same overwrite fails in opposite directions on the two lines: on Scala 2.13 it breaks compilation immediately and loudly (not found: value JsonSerializers), while here it compiles and waits to fail at runtime as a bad symbolic reference. The direction of the failure mode sets the cost of diagnosis. The scoping itself is unaffected and stays: a process-wide singleton that grows a search path per classloader and never releases one is a hazard under forkMode=once, where one JVM runs a whole shard. Fixing the right thing and explaining it wrongly are different mistakes; only the explanation is retracted. The note now sits at getClassPool, where the next reader of that code will find it.
…h relies on it cfb55aa scoped the sandbox import's duplicate-IBAN check by bank, on the premise that two banks may legitimately hold one IBAN. That premise is wrong. ISO 13616 encodes the institution in the string, so a shared IBAN is not a per-bank address space, it is bad data. More concretely, this instance depends on global uniqueness. Payment target accounts are resolved by routing with no bank context - BulkPaymentHandler:135, three Http4s700 transaction-request endpoints, getBankAccountByIban, and the to-account resolution inside the connector all pass bankId = None - and LocalMappedConnector.getBankAccountByRouting fails any lookup matching more than one row ("Routing MUST be unique"). Admitting a duplicate at import therefore does not produce a usable account; it produces one that fails every global-routing payment, reporting AccountRoutingNotUnique far from the cause. Trading a clear, actionable rejection at import for an obscure failure at payment time is the wrong trade. The unique index on (bankId, scheme, address), which the reverted commit cited, does not license the opposite reading: it is a storage constraint, and a per-bank index cannot authorise duplicates while a bank-less lookup exists. Storage constraint is not domain rule - that is the mistake worth naming, because the index really does say what it says. What actually broke was the fixtures. The 14 accounts in example_import.json carried seven strings shared across obp-bank-x-gh and obp-bank-y-gh; none was a valid IBAN (27 characters where Bosnia's is 20, and mod-97 of 36/52/50/65/79/57/45 where a valid IBAN gives 1), and all seven encoded the same institution while being attached to two banks. 2016-04-28/example_import.json had the same defect, one string shared across psd201-bank-x--uk and psd201-bank-y--uk. All 16 are regenerated as 20 characters, mod-97 = 1, globally unique, with a distinct bank code per bank (199/299/399/499 allocated across both files, so the two fixtures cannot collide when imported into one database). Values are replaced in place rather than by re-serialising the JSON, keeping the diff to one line per IBAN. The test that cfb55aa added is inverted accordingly: the same IBAN at a different bank must be rejected, and neither account may be created. Proven by negative control - restoring the per-bank scoping fails it at exactly the assertion that matters (201 did not equal 400) and nothing else. Not widened, deliberately: existingIbans still looks up per bank, so an IBAN already held at one bank is not detected when importing at another. That gap predates cfb55aa and closing it reaches into paths this change does not cover. Reported by a session validating the same import on develop-obp; verified independently here before acting.
107919d made compileScalaCode refuse on a JVM where no SecurityManager can be installed unless allow_user_generated_scala_code_without_sandbox is set. That is a behaviour change for anyone already running with allow_user_generated_scala_code=true on JDK 24+: dynamic code compilation starts failing with OBP-50021 after the upgrade. Until now it was stated only in that commit's message and in a comment in sample.props.template, neither of which an operator reads before upgrading. Written in the section format the server_mode removal already uses - what changed, then a Migration block with before/after props - so it sits where someone looking for breaking changes will find it. Includes the warning that belongs next to the switch rather than only in the props file: the feature compiles and runs Scala supplied over the API, so on a JVM with no enforceable sandbox, enabling it grants callers the privileges of the OBP-API process. Default deployments are unaffected; the feature is off unless explicitly turned on. Documentation only - no test or CI check reads README.md, verified before skipping the suite.
The fixture a new deployment imports had no test at all: example_import.json appears in the codebase only as a documentation link inside a ResourceDoc description. That is how seven 27-character strings with a failing mod-97 - each shared by two banks, each encoding a third - shipped as "IBAN"s and survived until a fresh-database run tripped over them, and it is why 0e0fcbb's regeneration of those values was not covered by any suite that ran green over it. The test posts the file through the same v2.1.0 data-import endpoint it is shipped for and requires 201. Proven to bite: restoring the pre-0e0fcbbf6 file turns it red, so it guards the fixture rather than merely reading it. 2016-04-28/example_import.json is deliberately not covered - it is rejected today, and would make this permanently red. Pre-existing, not caused by the IBAN work: the pre-change file fails the same way, and so does that fixture when imported alone, which rules out interference from the other import. Cause established since, and it is data rather than code - its accounts name owners by email (robert.xuk.x@example.com) while validateAccount matches user_name, and the file's own users section contains a different generation entirely (Robert.X.0.GH); separately, both its accounts carry an identical account id. Left alone because which way to reconcile that is a product decision, not a mechanical fix. Worth recording alongside: the import reports these as OBP-50005 "unspecified or internal error", discarding the per-check messages validateAccount builds ("Accounts must have owner(s) defined in data import. Violation: ..."). The diagnosis above came from reading the fixture, not from the response.
…s empty ReflectUtils.getNameToValues selects members with symbol.isVal || symbol.isVar. Both answer from Scala's own declaration metadata - ScalaSig on Scala 2, TASTy on Scala 3 - and scala.reflect.runtime.universe, the Scala 2.13 reflection library obp-commons is pinned to, has no TASTy reader: for a Scala-3-compiled class both come back false for every member. The function returned an empty map, and the allFields collectors built on it returned empty lists. SwaggerDefinitionsJSON declares 777 lazy vals and produced 0, measured directly. Nothing failed, which is the point: an empty list is a legal result, and every scenario that maps over allFields passed by doing nothing. SwaggerFactoryUnitTest had three such scenarios. getFieldValues in the same file already recovers from this - what survives into bytecode is the shape, a zero-arg method declared on the class itself with a backing field of the same name, or name$lzy… as Scala 3 spells a lazy val's field. That predicate is now shared rather than copied, and getNameToValues applies it as an additional branch, so Scala 2's isVal/isVar path is untouched (obp-commons' own ReflectUtilsTest, which runs on 2.13 where the bug cannot occur, still passes). includeVar = false still cannot exclude a Scala 3 var, for exactly the reason isVar fails there; documented at the function rather than silently approximated. Making the collector work exposed a mismatch it had been hiding. allFields fed everything non-null to SwaggerJSONFactory.translateEntity, which reads an entity's constructor arguments and therefore only means anything for a case class; several members here are plain values, and a PEM certificate string among them threw. Restricted to ReflectUtils.isObpObject. The regression test lives in obp-api, not beside ReflectUtils, because obp-commons compiles on 2.13 where the bug cannot be reproduced - and its sample object is top-level, because scala-reflect cannot load the symbol of an object nested in a class and the test would fail on its own fixture instead. SwaggerFactoryUnitTest now asserts a floor on allFields.size before using it: a floor well under the declared count, so it fails when the collector breaks rather than when someone adds a field.
ResourceDocMiddleware builds its lookup index from a version's own resourceDocs. A version with no entries gets no doc match, so authentication, role checks and entity resolution never run and the caller sees a bare 401. That was reported from a real-jar process for v3.0.0, five times, with the matcher's own debug line as the evidence - "Index keys for apiVersion=v3.0.0:" followed by nothing. It has never reproduced in a Maven test JVM, and the cause is still open. This does not reproduce it. It turns the one directly observed condition into an assertion, so an empty registration fails here naming the version instead of surfacing as an unexplained 401 wherever it happens to land. Writing it corrected a claim in that investigation. Its ruled-out list dismissed an initialisation cause because "gate takes routes by-value, so wrappedRoutesV300Services is always evaluated, touching the object". Evaluating it does happen and does not touch the object: wrappedRoutesVxxxServices is a Kleisli whose reference to Implementations… sits inside the lambda, so evaluating it builds a function and never runs the nested object's initialiser - which is where every resourceDocs += lives and where the index is built. Forcing all thirteen routes values leaves resourceDocs at 0 for twelve of them. That is a lead, not a cause: the first real request does initialise the object, and the ordering inside it is sound (Http4s300's last registration is at line 2219, the index at 2306), so the normal path self-heals. What it does show is that an initialiser failing once would leave the index permanently empty - the shape of an ExceptionInInitializerError cascade rather than of a routing bug, which is where the next attempt at this should look. The test touches the nested Implementations object for that reason. Touching the routes value instead measures nothing, as two earlier drafts of this test demonstrated by "finding" twelve empty versions that were merely uninitialised.
…ie stores develop added provenance to the three runtime-compiled-code entities - createdByUserId, updatedByUserId and a SHA-256 of the decoded method body, plus CreatedUpdated's timestamps - and exposed them on new read-only v7.0.0 endpoints while deliberately leaving the v4 responses frozen. It did that on the Lift Mapper entities. This branch had already moved DynamicResourceDoc, DynamicMessageDoc and ConnectorMethod to Doobie, so the resolution carries the fields across rather than restoring the entities: same columns, same server-side origin (the CallContext user and a hash computed in the provider, never the request body), same frozen v4 contract. ConnectorMethod had no entity left to hang the extra columns on, so the provenance read is a separate ConnectorMethodWithProvenance rather than more fields on JsonConnectorMethod - that one is the create/update request contract, and widening it would let a caller submit the values the server is supposed to set. ChatEmailDigestState arrived as a new Mapper entity, the first since this branch emptied ToSchemify.models. Carried across to Doobie for a reason that is not stylistic: with models empty Schemifier creates nothing, so a Mapper entity here compiles and then fails at runtime against a table nothing ever made. Its table comes from the changelog now, with the unique index the entity declared. Schema in db.changelog-provenance.yaml, a new file rather than an addition to the baseline, which is generated and would lose hand-written changesets on the next regeneration. Every changeset carries a MARK_RAN precondition so a database that already has the column records it as run. Two conflicts were not mechanical. OpenCorridorSettlement: develop changed settlement advices from one per beneficiary to one per party bank carrying the full covered list, and this branch had only renamed the accessors for Doobie - upstream's semantics kept, this branch's accessors applied. Glossary: the block boundaries made it look as though both sides had added items, and resolving it as "keep both" duplicated three entries; every conflicting block turned out to be upstream adding items next to shared ones, with this branch contributing nothing, so all five take upstream. 3905 scenarios pass on H2 and on Postgres.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for (_ <- 1 to 3) { | ||
| metrics.saveMetric("uid", "http://example.com/x", day, 5L, "uname", realApp, | ||
| "dev@example.com", "cid", "getBanks", "1.0", "GET", None, getCorrelationId(), | ||
| "body", "1.2.3.4", "1.2.3.4", "inst", null, null, null) |
| for (_ <- 1 to 3) { | ||
| metrics.saveMetric("uid", "http://example.com/x", day, 5L, "uname", realApp, | ||
| "dev@example.com", "cid", "getBanks", "1.0", "GET", None, getCorrelationId(), | ||
| "body", "1.2.3.4", "1.2.3.4", "inst", null, null, null) |
…iling it Both defects are in the provenance code this branch wrote while merging develop, not in what develop shipped. DoobieConnectorMethodProvider.create computed the body hash before entering its tryo block. The hash is over decodedMethodBody, which is URLDecoder.decode of a caller-supplied string, and that throws IllegalArgumentException on a malformed escape - '%' is an ordinary character in Scala source, so a connector method containing `100 % 7` is enough. Outside the tryo the exception leaves create uncaught and the request ends as an unhandled 500; the Mapper implementation computed the same hash inside its tryo and returned a Failure the endpoint could report. The two document providers already did it the right way round, so this was the odd one out. ConnectorMethodProvenanceEdgeTest reproduces it: before the fix it fails with "Illegal hex characters in escape (%) pattern". DynamicResourceDoc.update and DynamicMessageDoc.update took updatedByUserId and methodBodyHash with default None while the SET clause assigns them unconditionally, so omitting the arguments did not leave the stored provenance alone - it nulled it. The hash exists to make tampering with a runtime-compiled endpoint detectable, so clearing it silently defeats the feature. The defaults are gone; omitting them is now a compile error, which immediately surfaced three call sites that had been relying on them. 3907 scenarios pass on H2 and on Postgres.
chat_email_digest_state arrived with the develop merge and was not added to resetDatabaseForTestClass, which clears 140 tables including its two neighbours in the same feature, participant and chatroom. The row it holds is "when this user was last emailed a digest", and the scheduler reads it back to decide whether to skip a user - so a row surviving into the next class suppresses a digest that class expects, failing as a function of which suites share the shard's JVM rather than of either suite. No test writes that table yet, so the gap was silent; ChatEmailDigestStateResetTest closes it and fails without the reset. Extending check_changelog_preconditions.py to the new schema changelog turned up something worse than the gap it was meant to close. The guard hard-coded the baseline, so db.changelog-provenance.yaml was outside its scope; adding the file to the list left the count unchanged at 410, because changesets() anchors its split on `- changeSet:` at column 0 and the hand-written changelog nests it two spaces in under databaseChangeLog. The guard reported success over a file it had not read - the failure mode it exists to prevent, in the guard itself. The splitter now accepts either indentation and the count is 415, and addColumn changesets are checked for a columnExists precondition, since tableExists cannot express "this column is already here". Both proven by negative control: the reset test fails on the unmodified ServerSetup, and removing one precondition from the provenance changelog makes the guard name that changeset and exit 1. 3908 scenarios pass on H2 and on Postgres.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrates OBP-API to Scala 3, replacing the two frameworks that blocked it. 287 commits on top of
3df73fe11; 1027 files, +66009/-33154.Opened for review of the whole line. It does not merge cleanly onto
developyet — the branchis 26 commits behind and conflicts in 12 files, all of them where
developextended a Lift Mapperentity this branch had already moved to Doobie. That merge is in progress separately and will be
pushed here; the conflicts are listed at the end so reviewers know what is coming rather than
discovering it from a red merge box.
What changed
Scala 3. Lift Mapper cannot compile under Scala 3 — the compiler crashes on the
object X extends class Xshape every Mapper entity uses (reduced to a 5-line case). Both of itsconsumers had to go first.
Lift Web → http4s, complete.
net.liftweb.httpno longer appears in any.scalasource. Thereis no Lift fallback in the request chain: an unmatched
/obp/*path returns a JSON 404 fromnotFoundCatchAll. API versions are unchanged by this — a framework migration happens in placeinside the existing version file, and a version bump still means a changed API signature.
Lift Mapper → Doobie, complete.
ToSchemify.modelsis nowNil; Schemifier creates nothing.Flyway → Liquibase. Flyway needed one hand-written script set per vendor: 118 for H2, 118 for
Postgres, and nothing for the three other drivers its
vendorFolderwould have booted againstsilently, with no tables. One changelog now describes each change once and Liquibase emits the
dialect. The baseline is generated from a Postgres database the Flyway scripts built, not
hand-written, and is regenerated with
scripts/GenerateChangelog.javaplus a normaliser ratherthan edited.
Defects found and fixed along the way
Migrating the data layer surfaced behaviour that lived in Mapper's field types rather than in the
entities, and which a column-by-column port drops silently:
MappedBooleanreads back asfalsewhateverdefaultValuedeclares, a NULLMappedLongas the declared default — read as a hardcoded-1, six call-limit columns turned"the configured limit" into "no limit", and that value is what the rate limiter enforces from
MappedEmaillowercases and trims on every set and validates on save;authuserlost all of itMappedMetrics, reachable from read-only roles, giving a boolean-blindoracle over the whole database
Security fixes on top: a locked account could still authenticate through OIDC and Keycloak (both
read
v_oidc_usersover JDBC and never callverify-credentials);consents.sca.enabled=falseaccepted any SCA answer in production; the dynamic-code dependency validation inspected an empty
list because its scan was gated on an unrelated diagnostic prop; and
compileScalaCodenow refusesto run when no SecurityManager can be installed (JEP 486) unless the operator says so explicitly.
Testing
3876 scenarios, 0 failures, on both H2 and Postgres. Postgres is not optional here: H2 tolerates
things Postgres does not, and the schema is generated from the changelog at boot on both.
Conflicts with current develop
developaddedcreatedByUserId/updatedByUserId/ a method-body hash to DynamicResourceDoc,DynamicMessageDoc and ConnectorMethod, and one new Mapper entity (
ChatEmailDigestState). Thisbranch had already moved the first three to Doobie, so the resolution ports the new fields into the
Doobie stores and the changelog rather than restoring the Mapper entities;
ChatEmailDigestStateneeds the same treatment, since with
models = Nilits table would otherwise never be created.