Skip to content
Merged
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ Note: `executeFutureCreated` returns 201; pair it with `cc.user.openOrThrowExcep

**Use `NEW_ACCOUNT_ID` for PUT-creates-account URLs**: When a `PUT /banks/BANK_ID/accounts/ACCOUNT_ID` *creates* the account (it doesn't exist yet), the middleware's `validateAccount` keys off the literal `ACCOUNT_ID` template var and tries to look it up → 404 before the handler runs. Change the ResourceDoc URL template to `/banks/BANK_ID/accounts/NEW_ACCOUNT_ID` (or any non-standard ALL_CAPS variant) — middleware treats it as a wildcard and skips the lookup, but the path still matches the route pattern. The handler can check "already exists" inline with `Connector.connector.vend.checkBankAccountExists(...)` and return 409/400 as needed.

**Reserved ALL_CAPS literals — don't use them as placeholders**: `ResourceDocMatcher` in `Http4sSupport.scala` keeps an explicit `literalAllCapsSegments` set: `SANDBOX_TAN`, `COUNTERPARTY`, `SEPA`, `FREE_FORM`, `ACCOUNT`, `ACCOUNT_OTP`, `REFUND`, `SIMPLE`, `AGENT_CASH_WITHDRAWAL`, `CARD`, `OPEN_CORRIDOR_PROMISE`, `OPEN_CORRIDOR_SETTLEMENT`, `EMAIL`, `SMS`, `IMPLICIT`, `NOT_EMAIL_NEITHER_SMS`. These are matched as **literals** (real Lift endpoints register them as concrete SCA-method / transaction-request-type segments — e.g. `/banks/BANK_ID/my/consents/EMAIL`). Any other ALL_CAPS segment is a wildcard. If you migrate an endpoint whose URL template uses one of these names as a *placeholder variable* (e.g. v3.0/v4.0 `getUsersByEmail` had `/users/email/EMAIL/terminator` with EMAIL meaning "any email value"), the matcher will only fire when the URL segment is literally `EMAIL` — real callers pass actual addresses and miss the doc entirely → middleware skips auth/role validation → handler 500s on the empty CallContext. Rename the placeholder to something outside the literal set (e.g. `EMAIL` → `USER_EMAIL`), and apply the rename in **both** the http4s `ResourceDoc` and the original Lift `ResourceDoc` (resource-docs aggregation reads both, and `collectResourceDocs` dedup keys off URL + verb).
**Reserved ALL_CAPS literals — don't use them as placeholders**: `ResourceDocMatcher` in `Http4sSupport.scala` treats every value of the `TransactionRequestTypes` and `StrongCustomerAuthentication` enums (plus `NOT_EMAIL_NEITHER_SMS`) as a **literal** URL segment: `literalAllCapsSegments` is built from those enums, so adding a transaction-request type to the enum is all it takes. Any other ALL_CAPS segment is a wildcard. Until 2026-10 the set was kept by hand, and `MOBILE_WALLET`, `UTILITY`, `BULK`, `HOLD`, `CARDANO` and the `ETH_*` types were never added: the v7 MOBILE_WALLET template then claimed every transaction-request type (UTILITY and BULK were validated under the MOBILE_WALLET doc; HOLD/CARDANO/ETH on v7 got an empty 404 instead of falling through to v6). When several templates match, the one with the most literal segments wins, whatever the registration order. `ResourceDocSelfResolveTest` checks that every registered doc's own URL resolves to that doc. If you migrate an endpoint whose URL template uses an enum value as a *placeholder variable* (e.g. v3.0/v4.0 `getUsersByEmail` had `/users/email/EMAIL/terminator` with EMAIL meaning "any email value"), the matcher will only fire when the URL segment is literally `EMAIL` — real callers pass actual addresses and miss the doc entirely → middleware skips auth/role validation → handler 500s on the empty CallContext. Rename the placeholder to something outside the literal set (e.g. `EMAIL` → `USER_EMAIL`), and apply the rename in **both** the http4s `ResourceDoc` and the original Lift `ResourceDoc` (resource-docs aggregation reads both, and `collectResourceDocs` dedup keys off URL + verb).

**Bypass roles vs required roles**: Some Lift handlers check entitlements inline as **bypass** conditions inside authorisation helpers — e.g. `checkAuthorisationToCreateTransactionRequest` honours `canCreateAnyTransactionRequest` to let the caller skip the view-permission check, but the role is never a hard requirement. These roles are correctly absent from the Lift ResourceDoc role list — putting them in the doc would make Lift enforce them as required (since Lift DOES enforce doc roles by default), breaking the "view permission OR role" intent. The same holds for http4s middleware. So the trap on migration is the reflex copy: don't move a bypass role from inline-only into `Some(List(...))` just because it appears in the handler. Audit before copying: if the role appears in the Lift handler only inside an authorisation OR-chain ("has view permission OR has role X"), it belongs as `None` in the doc with the inline view/role logic preserved. Bypass roles must stay out of the doc.

Expand Down
2 changes: 2 additions & 0 deletions docs/telemetry_conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,8 @@ thread and class figures on its own port. Remove it once the Micrometer JVM bind
| `obp.api.redis.commands` | timer | `command`, `result` | `Redis.use` |
| `cache.gets`, `cache.puts`, `cache.evictions`, `cache.size` | standard | `cache` = `in_memory`, `json_schema`, `message_docs`, `on_behalf_of` | every Guava cache, through `Telemetry.monitorCache` |
| `hikaricp.*` | standard | `pool` | the database pool (`CustomDBVendor`) |
| `obp.api.database.connection.held_too_long` | gauge | | connections out of the pool right now for longer than `database_connection_hold_warning_seconds` (`DatabaseConnectionHoldWatch`) |
| `obp.api.database.connection.hold_warnings` | counter | | warnings written by `DatabaseConnectionHoldWatch`, one per connection held too long |
| `jvm.*`, `process.*`, `system.*` | standard | | JVM memory, heap after garbage collection (`jvm.memory.usage.after.gc`), garbage collection, threads, classes, CPU, uptime, open files |
| `obp.api.instance.info` | gauge, always 1 | `api_instance_id`, `git_commit` | start-up |
| the counters in section 11 | | | `TelemetryBindings` |
Expand Down
22 changes: 21 additions & 1 deletion ideas/ASSET_REGISTRY.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ The work is ordered so that each step changes nothing a caller can see until the
| 4 | `Asset` and `AssetStatusHistory` tables, provider and boot seed (§2, §6, §7) | Done, not committed |
| 5 | Read-only endpoints `GET /assets`, `GET /assets/ASSET_CODE`, reverse chain lookup (§8), and the Glossary entry (§11) | Done, not committed |
| 6 | `isValidCurrencyISOCode` and `currencyDecimalPlaces` read from the registry (§4) | Done, not committed |
| Later | Case-insensitive codes (§4), rejecting excess decimals (§4), write endpoints (§8), amount storage (§5, §7 Part A), precision corrections (§7 Part B), folding `lovelace` and `wei` (§7 Part C, §10) | Each changes behaviour or waits on an open question |
| 7 | Case-insensitive codes (§4): codes upper-cased in requests by `ResourceDocMiddleware`, compared ignoring case | Done, not committed |
| 8 | Rejecting excess decimals (§4): 400 (OBP-10068) from `ResourceDocMiddleware`, crypto assets exempt until §7 | Done, not committed |
| Later | write endpoints (§8), amount storage (§5, §7 Part A), precision corrections (§7 Part B), folding `lovelace` and `wei` (§7 Part C, §10) | Each changes behaviour or waits on an open question |

**Step 1.** `CurrencyHandlingTest` is a pure unit test (no server, no database). It asserts correct behaviour only. The three known defects are written as the correct expectation inside `pendingUntilFixed`, so they report as pending now and fail, demanding the wrapper's removal, once fixed:
- codes are not accepted in every letter case;
Expand Down Expand Up @@ -106,6 +108,24 @@ The registry is read once into memory and read again after any write through `As

`AssetLookupTest` (`code.asset`, CI shard 8) checks that the seeded registry gives the built-in answer for every code, that a newly registered asset is accepted at once with its own decimal places, and that an empty registry falls back. Suites that rewrite the registry restore the seeded one when they end (`RestoresSeededAssetRegistry`), and the test database resets also clear the in-memory copy; otherwise a later suite in the same JVM, such as `CurrencyHandlingTest`, would read a registry holding only test assets. Run with `FundsAvailableTest`, both `TransactionRequestsTest` suites, the v4.0.0 `AccountTest` and `CardanoTransactionRequestTest`: 104 passed, 3 pending (the known defects), none failed.


**Step 7.** Currency codes are case-insensitive. `code.asset.CurrencyCodes` holds the rule:
- **Requests.** Once a static ResourceDoc has matched, `ResourceDocMiddleware.withCurrencyCodesUpperCased` upper-cases the currency codes in the request before the endpoint runs. In the JSON body it changes the string fields that the endpoint's example body uses for a currency code (a name ending in `currency` or `currency_code`, ignoring case and underscores); it edits the text, so amounts keep their exact form. In the query string it changes parameters named the same way. Only values that look like a code (2 to 12 letters or digits) are touched. Dynamic Entity and Dynamic Endpoint requests are left as sent.
- **Path segments.** The only currency codes in a URL path are those of `GET /banks/BANK_ID/fx/FROM_CURRENCY_CODE/TO_CURRENCY_CODE`, which already upper-cases them, and `GET /assets/ASSET_CODE`, which already ignores case.
- **Stored codes.** About ten comparisons against a stored currency (transaction request against account currency, funds available, settlement accounts, bulk payments, FX) use `CurrencyCodes.same`, so rows written before this rule in another case still match. `AssetLookup` ignores case, so `jpy` gets 0 decimal places. `ada` is no longer a legacy spelling, because it matches `ADA`.
- **Stored rows.** The runOnce migration `upperCaseStoredCurrencyCodes` (`MigrationOfCurrencyCodesUpperCase`) rewrites the codes in every currency column in upper case, `ada` to `ADA` among them, so database queries that select by currency (for example the FX rate lookup) find old rows too. It leaves codes inside stored JSON alone, and a `mappedcurrency` row whose upper-case twin exists. `lovelace` and `wei` become `LOVELACE` and `WEI`; their amounts are converted later (§7 Part C). `CurrencyCodesUpperCaseMigrationTest` covers it.

`CurrencyCodesTest` (unit) covers the rule. The two letter-case defects in `CurrencyHandlingTest` are no longer pending. `AssetLookupTest`, `FundsAvailableTest` (lower-case `eur` now gives 200) and `ExchangeRateTest` (an FX rate created with `eur`/`usd` is stored as `EUR`/`USD` and read back with lower-case path segments) were updated.


**Step 8.** An amount with more decimal places than its currency allows is refused with 400 (OBP-10068, `InvalidAmountPrecision`) instead of being cut off when stored. `code.asset.AmountPrecision` finds the amounts; `ResourceDocMiddleware.validateAmountPrecision` runs it as the last validation step, after authentication and Roles, so it applies to every static endpoint in every version:
- **Where an amount is found.** In a JSON body, an object with an `amount` field (any letter case) next to its currency: the field named `currency`, or else the object's only currency-named field. In a query string, an `amount` parameter next to a currency parameter. Numbers are parsed exactly (`useBigDecimalForDouble`). Trailing zeros do not count.
- **Not checked.** Unknown codes and non-numbers (left to the endpoint), and crypto assets: `AssetLookup.enforcedDecimalPlaces` is None for a registry `CRYPTO` asset and for `lovelace` and `wei`. Their registered decimal places are still the built-in 2, so enforcing them would refuse 0.001 ETH. They are checked once §7 records their real precision.
- **Not changed.** Amounts OBP computes itself (FX conversion, for example) are still cut off by `convertToSmallestCurrencyUnits`; only amounts a caller sends are refused.
- **Precision used.** The registry's decimal places, which still equal the built-in table (§7 Part B corrects them). So CZK is held at 0 decimal places and 10.50 CZK is refused, as it would be stored as 10 today.

`code.asset.AmountPrecisionTest` (unit) covers the rule; `code.api.v4_0_0.AmountPrecisionTest` covers account creation and a SANDBOX_TAN transaction request; `FundsAvailableTest` covers a query amount.

---

## 1. Model
Expand Down
6 changes: 6 additions & 0 deletions obp-api/src/main/resources/props/sample.props.template
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ connector=star
#hikari.keepaliveTime=30000
#hikari.maxLifetime=1800000

# Write a warning to the log (and so to the log cache) when a database connection has been out of
# the pool for longer than this many seconds. The warning names the thread that took it and the
# OBP-API code that thread is running, and a second warning gives the total time once the
# connection goes back. 0 turns this off.
#database_connection_hold_warning_seconds=60

## if connector = star, then need to set which connectors will be used. For now, obp support rest, akka.
starConnector_supported_types=mapped,internal

Expand Down
13 changes: 10 additions & 3 deletions obp-api/src/main/scala/bootstrap/liftweb/CustomDBVendor.scala
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ TESOBE (http://www.tesobe.com/)
package bootstrap.liftweb

import code.api.util.APIUtil
import code.util.DatabaseConnectionHoldWatch
import code.util.Helper.MdcLoggable
import com.zaxxer.hikari.pool.ProxyConnection
import com.zaxxer.hikari.{HikariConfig, HikariDataSource}

import java.sql.Connection
Expand Down Expand Up @@ -90,7 +90,12 @@ class CustomDBVendor(driverName: String,
// Telemetry: the pool's connections, waits and timeouts, as the standard hikaricp_* series.
config.setMetricRegistry(code.telemetry.Telemetry.registry)

val ds: HikariDataSource = new HikariDataSource(config)
// Every connection taken from the pool, whichever way it is taken (Lift Mapper, Doobie, the
// request transaction), passes through here, so DatabaseConnectionHoldWatch sees them all.
val ds: HikariDataSource = new HikariDataSource(config) {
override def getConnection(): Connection = DatabaseConnectionHoldWatch.watch(super.getConnection())
}
DatabaseConnectionHoldWatch.pool = Some(ds)
}

def createOne: Box[Connection] = {
Expand All @@ -109,6 +114,8 @@ trait CustomProtoDBVendor extends ConnectionManager with MdcLoggable {
createOne
}

def releaseConnection(conn: Connection): Unit = {conn.asInstanceOf[ProxyConnection].close()}
// A plain close(): the connection may be DatabaseConnectionHoldWatch's wrapper rather than
// HikariCP's own ProxyConnection, and either way close() returns it to the pool.
def releaseConnection(conn: Connection): Unit = conn.close()

}
13 changes: 13 additions & 0 deletions obp-api/src/main/scala/code/api/util/DBUtil.scala
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,19 @@ object DBUtil {

def isSqlServer: Boolean = dbUrl.contains("sqlserver")

/** The endpoint timeout in whole seconds, rounded up, which is the unit JDBC takes. */
private[util] def queryTimeoutSeconds: Int =
math.max(1L, (Constant.longEndpointTimeoutInMillis + 999) / 1000).toInt

/**
* SQL Server-safe alternative to Lift's DB.runQuery.
*
* Lift's DB.runQuery uses DB.asString which doesn't handle SQL Server's NVARCHAR type
* (JDBC type -9), causing MatchError. This function handles all JDBC types properly.
*
* The query is cancelled by the database after `long_endpoint_timeout`, so use this for
* read-only queries only.
*
* @param query SQL query string
* @param params Query parameters (for prepared statement)
* @return Tuple of (column names, rows as List[List[String]])
Expand All @@ -53,6 +60,12 @@ object DBUtil {
DB.use(DefaultConnectionIdentifier) { conn =>
val stmt = conn.prepareStatement(query)
try {
// The database cancels the query once the caller can no longer receive its answer. The
// endpoint timeout (long_endpoint_timeout) answers the caller with a 504, but without this
// the query carried on, holding its pool connection, for as long as it took: one
// aggregate-metrics query held its connection for about ten minutes. Only read-only
// SELECTs come through here, so cancelling never interrupts a write.
stmt.setQueryTimeout(queryTimeoutSeconds)
// Set parameters
params.zipWithIndex.foreach { case (param, idx) =>
stmt.setString(idx + 1, param)
Expand Down
1 change: 1 addition & 0 deletions obp-api/src/main/scala/code/api/util/ErrorMessages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ object ErrorMessages {
val IpPenaltyAlreadyExists = "OBP-10064: This IP address already has a penalty. Remove it first to change it."
val IpPenaltyNotFound = "OBP-10065: This IP address has no penalty."
val InvalidTrafficWindow = "OBP-10067: Invalid window. Use window=1, 5 or 15 (minutes)."
val InvalidAmountPrecision = "OBP-10068: Invalid amount precision. The amount has more decimal places than its currency allows, and OBP does not round or cut off amounts."
val InvalidIpPenalty = "OBP-10066: Invalid IP penalty. per_minute_limit must be 0 or more, duration_minutes between 1 and 10080 (one week), and reason between 1 and 255 characters."
// Not an error: the text of the X-Rate-Limit-Warning header a self-service endpoint returns in
// shadow mode. SCOPE and LIMIT are replaced at runtime, e.g. "signup" and "5 per hour".
Expand Down
Loading
Loading