diff --git a/COMPATIBILITY.md b/COMPATIBILITY.md index 766d62568..4fa19f623 100644 --- a/COMPATIBILITY.md +++ b/COMPATIBILITY.md @@ -459,6 +459,84 @@ This section governs the third kind. `PluginInstallUtils.UndeterminedEntriesException` (`@ApiStatus.Internal`), so no outcome discards what another established. +- The JSON storage backend no longer hands out the entities it caches (#522). Before 6.3.0, + `SimpleJsonDataOperator`'s read paths (`getById`, `getAll`, `page`, `getLike`, and every + `query()` terminal built on them) returned the very instances it kept in memory, and `insert` + cached the instance it was given, so on `datasource.type: json` changing a loaded (or just + inserted) entity **without** calling `update(...)` changed the store and was written to disk at + the next flush. On SQLite and MySQL the same code never persisted anything, because every read + materialises the row afresh. As of 6.3.0 every JSON read returns a detached copy produced by the + same Gson form the store writes to disk, and `insert` caches a copy: a change reaches the store + only through `update(...)` (or `update(column, value, id)`), on every backend alike. `update(T)` + also fires `onUpdate()` on the entity passed in, before its fields are copied into the store, + exactly as the relational backends do — so an `AuditableDataEntity`'s `updatedAt`/`updatedBy` + now show on the caller's instance on the JSON backend too, and `exist(entity)` looks the entry + up by the entity's id, as the relational backends do, instead of comparing it with the cached + copy through `equals()`. A module that relied on the old + aliasing — changing a loaded entity and counting on the next flush to save it — must now call + `update(...)`; no module in this monorepo was found doing so (see the pull request's consumer + impact list). The cost is one Gson round trip per entity returned, the same materialisation the + relational backends already pay (see `ultitools.storage.detached-reads` in `FEATURES.md`). +- `Query#delete()` returns the number of rows actually removed, as its javadoc always said (#521). + It used to return the number of rows the query *matched*, and it skipped a matched row whose id + was `null` while still counting it, so a caller reading the `int` as "rows removed" could be told + a delete succeeded when it removed nothing. As of 6.3.0 the count comes from the backend's own + affected-row count (a row another writer removed between the read and the delete is not + counted), and a matched row with a `null` id is refused with a `DataAccessException` naming the + entity type **before** any row is deleted, since no delete can address it. This corrects + behaviour that contradicted the documentation, so it takes no migration period. A third-party + `DataOperator` implementation, which cannot report what its `delById` removed, is counted by + checking that the row existed immediately before that call and is gone after it (see `ultitools.storage.query-delete-count` in `FEATURES.md`). +- Rows left without an id by UltiTools-API 6.2.0 are repaired, and addressing a row by a null id + is refused (#546, maintainer decision of 2026-09-27). 6.2.0 did not assign an id in `insert`, and + SQLite's generated DDL accepted a `NULL` primary key, so every row a module inserted without an + id on that release was stored with none; such a row could be read, but every `update`/`delete` of + it bound `WHERE id = NULL`, matched nothing and returned normally, so a change the module + reported as saved was lost at the next restart. As of 6.3.0, when a SQLite-backed table is + initialised every row whose `id` is `NULL` is given the id its entity reports through `getId()`, + or a new UUID when the entity reports none, in either case only if the entity read back with that + id reports it; a row that no written id would make addressable is left as it is and counted, by + reason, in one WARNING line per table: a derived id that more than one row without an id reports + (none of those rows is written — maintainer decision of 2026-09-29, the rule UltiEssentials' own + repair applies), a derived id another row already holds, or a derived id that is `null` or a row + that cannot be read as the entity — only the `id` + column is written, all rows in one transaction, so the repair writes user data at startup, which + is what the maintainer decided — and one INFO line names the table, the count and how many rows + took the entity's own id; a second start finds nothing and logs nothing. The reported id comes + first because an entity may derive `getId()` from another column (UltiEssentials' + `UuidKeyedDataEntity` and UltiKits' `KitClaimData` derive it from a `uuid` column) and every + lookup binds that value, so a random id would leave such a row exactly as unreachable as `NULL` + did. For the same reason every write path (`insert`, `insertAll`, `update(T)`, `updateAll`, + `updateIf`) now stores `getId()` in the `id` column rather than the inherited field: an entity + that overrides `getId()` never sets that field, so on 6.3.0 before this change it still inserted + a `NULL` id on SQLite, and on MySQL its insert failed outright. MySQL never + accepted a `NULL` id and runs no backfill. Independently, `update(T)`, `update(column, value, id)`, + `delById` and `updateAll` addressed by a `null` id now throw `DataAccessException` on every + backend instead of silently matching nothing (the JSON backend used to throw a raw + `NullPointerException`); `updateAll` checks every entity before it writes any. A call with a + non-null id that matches no row is unchanged. See `ultitools.storage.null-id-backfill` and + `ultitools.storage.null-id-refused` in `FEATURES.md`. +- `DataOperator` gains one method, `boolean updateIf(T entity, WhereCondition... expected)` (#543): + a conditional write that applies only while the stored row still matches every expected + condition, and reports whether it applied, on the JSON, SQLite and MySQL backends. No existing + method's signature changes, and it is a `default` method, so a module compiled against 6.2.x + still links. Its default body throws `UnsupportedOperationException` naming the implementing + class rather than quietly performing an unconditional write — a third-party `DataOperator` + implementation keeps working for every other method and must implement `updateIf` before a caller + can rely on it. The framework's own operators implement it (see + `ultitools.storage.conditional-update` in `FEATURES.md`). +- An update by a non-null id that matches no row writes nothing and says so (#558, maintainer + decision of 2026-09-29). `update(T)`, `update(column, value, id)` and `updateAll` now log one + WARNING naming the table and the id each time, on JSON, SQLite and MySQL, and return normally — + as SQLite and MySQL already did, silently; the JSON backend used to throw a raw + `NullPointerException`, which a module catching `RuntimeException` or `Exception` around the call + saw as a failed write. The caller learns the outcome through a new method, + `int updateCounted(T entity)` on `DataOperator`: `1` for a written row, `0` when no row has the id. + It is a `default` method, so no existing signature changes and a module compiled against 6.2.x + still links; a third-party implementation that does not override it is counted by whether the row + exists before its `update` (the one remaining miscount: a delete by another writer during that + call). See `ultitools.storage.missing-row-update` in `FEATURES.md`. + ### Behavioral changes that do need one - A documented default value flipping. diff --git a/FEATURES.md b/FEATURES.md index 022bba21d..901751fbb 100644 --- a/FEATURES.md +++ b/FEATURES.md @@ -222,6 +222,13 @@ way every row in that section's own reconciliation note accounts for — both ar | ID | Feature | Kind | How to reach | Permission | Target | Tier | Manual | Source | |---|---|---|---|---|---|---|---|---| | ultitools.storage.backend-select | Choose the ORM storage backend (`json`, `sqlite`, or `mysql`) via `config.yml`; falls back to `json` if the configured backend is unavailable | persistence | `datasource.type` in `plugins/UltiTools/config.yml`, applied only on a full server restart — `/ul reload` (`UltiTools#reloadPlugins`) reloads config, language, and modules but never re-runs `initDataStore`, so the active data store is unchanged until restart | n/a | n/a | admin | brief | UltiTools#initDataStore | +| ultitools.storage.concurrent-lookup | Looking up the registered data store (`DataStoreManager#getDatastore`) from any thread returns the registered store, or the `json` fallback when the requested type is not registered, and never `null` for a type that is registered — the registry is a concurrent map and each lookup reads each key once (before 6.3.0 a lookup racing a registration or unregistration could return `null`, #515) | persistence | automatic, when the framework selects its data store at start-up (`UltiTools#initDataStore`, the only caller); modules read the store selected there through `UltiToolsPlugin#getDataOperator`, not through this lookup, while a `JsonStore` registers itself whenever one is constructed | n/a | n/a | internal | none | DataStoreManager#getDatastore | +| ultitools.storage.conditional-update | `DataOperator#updateIf(entity, expected...)` writes the entity over the stored row only while that row still matches every expected condition, and returns whether it wrote: on SQLite and MySQL one `UPDATE … WHERE id = ? AND ` whose affected-row count decides, so it holds across servers sharing a database; on JSON the check and the write run under the operator's lock. A null id, a condition column the entity does not map with `@Column`, or a null expected value throws `DataAccessException` on every backend, so a compare-and-set loop cannot retry forever; an implementation that does not provide it throws `UnsupportedOperationException` naming its class (added in 6.3.0, #543; UltiEconomy's wallet merge conditions each account write on the balance it read) | persistence | module code calls `updateIf(...)` and re-reads when it returns false | n/a | n/a | internal | none | AbstractRelationalDataOperator#updateIf, SimpleJsonDataOperator#updateIf | +| ultitools.storage.detached-reads | Every entity a `DataOperator` read returns (`getById`, `getAll`, `page`, `getLike`, `query()`) is a detached copy, and `insert` stores a copy of the entity passed in: changing an entity without calling `update(...)` changes nothing stored, and `exist(entity)` finds the stored row by its id, on the JSON backend exactly as on SQLite and MySQL (before 6.3.0 the JSON backend handed out its cached instances, so such a change was written at the next flush, #522) | persistence | module code reads an entity, changes it, and does or does not call `update(...)` | n/a | n/a | internal | none | SimpleJsonDataOperator#detach | +| ultitools.storage.missing-row-update | An update by a non-null id that matches no row (`update(T)`, `update(column, value, id)`, `updateAll`) writes nothing and logs one WARNING naming the table and the id, every time, on JSON, SQLite and MySQL, and returns normally; `DataOperator#updateCounted(entity)` (added in 6.3.0) returns `1` or `0` so the caller can tell nothing was written — a third-party implementation without a count is counted by whether the row exists before its update. Before 6.3.0 SQLite/MySQL returned silently and JSON threw a raw `NullPointerException` (#558, maintainer 2026-09-29) | persistence | module code updates an entity another writer has deleted | n/a | n/a | internal | none | AbstractRelationalDataOperator#updateCounted, SimpleJsonDataOperator#updateCounted | +| ultitools.storage.null-id-backfill | When a SQLite-backed module table is initialised, every row whose `id` is NULL (rows UltiTools-API 6.2.0 inserted without an id) is given the id its entity reports through `getId()` — which an entity may derive from another column, as UltiEssentials' and UltiKits' do — or a new UUID when it reports none — in either case only if the entity then reports that id, because every lookup binds `getId()`; a row that no written id would make addressable is left as it is and counted, by reason, in one WARNING line per table — a derived id that more than one row without an id reports (none of those rows is written, maintainer decision 2026-09-29, the rule UltiEssentials' own repair applies), a derived id another row already holds, or a derived id that is null or a row that cannot be read as the entity; only the `id` column is written, all rows in one transaction, and one INFO line names the table, the count and how many took the entity's own id; a table with no such rows logs nothing, so a second start is silent. Before 6.3.0 such a row could be read but every update or delete of it matched nothing, so a change such as `/world set` reported success and was lost at the next restart (#546, maintainer decision 2026-09-27). MySQL cannot hold a NULL id and runs no backfill. Every write path also stores `getId()` in the `id` column, so an entity whose `getId()` is derived from another column no longer inserts a NULL id on 6.3.0 | persistence | automatic, when a module first obtains its `DataOperator` for the table after a start | n/a | n/a | admin | none | AbstractRelationalDataOperator#backfillNullIds | +| ultitools.storage.null-id-refused | `update(T)`, `update(column, value, id)`, `delById` and `updateAll` addressed by a null id throw `DataAccessException` naming the table or store and the entity type, on the SQLite, MySQL and JSON backends; `updateAll` checks every entity before writing any. Before 6.3.0 the relational backends matched no row and returned normally, and the JSON backend threw a raw `NullPointerException` (#546) | persistence | module code calls one of those methods with an entity or id that is null | n/a | n/a | internal | none | AbstractRelationalDataOperator#requireId, SimpleJsonDataOperator#requireId | +| ultitools.storage.query-delete-count | `query()…delete()` returns the number of rows the backend actually removed — a matched row another writer removed first is not counted — and refuses, with a `DataAccessException` naming the entity type and before deleting anything, a matched row whose id is null (before 6.3.0 it returned the match count and skipped a null-id row while counting it, #521) | persistence | module code calls `query().where(...)…delete()` and reads its return value | n/a | n/a | internal | none | QueryImpl#delete | | ultitools.storage.restart-survival | Data written through a `DataOperator` survives a full server restart, in whichever backend is active | persistence | write data via any module command backed by `@Table`, then restart the server | n/a | n/a | admin | none | DataStoreManager#getDatastore | ## Module configuration persistence diff --git a/UAT-CHECKLIST.md b/UAT-CHECKLIST.md index bbfc3bfca..c2985e7b2 100644 --- a/UAT-CHECKLIST.md +++ b/UAT-CHECKLIST.md @@ -149,6 +149,13 @@ a real module's real call pattern, not a synthetic one. |---|---|---|---|---|---| | ultitools.storage.backend-select | `language: en` in config.yml; `datasource.type: sqlite` in config.yml (shipped default); at least one loaded module has performed a `@Table`-backed data operation since the last restart — `SQLiteDataStore` creates its `.db` file lazily from `getOperator`, never at startup, so a server with zero data operations has no file to find | Restart the server, trigger one module data operation, and read the startup console log | Console shows `Data Storage Method: sqlite`. The `.db` file's location depends on which `getOperator` overload the module used: an internal `UltiToolsPlugin` module (`getOperator(UltiToolsPlugin, Class)`, the common case) creates `plugins/UltiTools/sqliteDB/.db` — NOT a file under that module's own `plugins//` data folder; only a plain Bukkit plugin using the External Plugin API (`getOperator(File, Class)`) creates `data.db` directly under its own data folder | server | | | ultitools.storage.backend-select.neg-fallback | `datasource.type: mysql` AND `mysql.enable: true` in config.yml, but `mysql.host` points at an unreachable server — this combination selects `DataStoreManager#reportBackendSelection`'s connection-failed branch specifically; `mysql.enable: false` (the shipped default) with `datasource.type: mysql` reaches the same fallback through a different, `mysql.enable`-is-false reason and is not what this row exercises | Restart the server and read the startup console log | Two SEVERE lines: `Data store downgraded: datasource.type in config.yml is 'mysql', but 'json' is actually in use. Reason: MySQL connection failed, the data source is unavailable (see the connection error above)`, then `Player data will be read from and written to the 'json' store. Data is not shared between backends - if this is not what you want, stop the server, fix config.yml and restart.` — the INFO-level `Data Storage Method: json` startup line alone would NOT satisfy this row, since it never mentions that config asked for `mysql` (issue #183) | server | | +| ultitools.storage.concurrent-lookup | not covered — unit-pinned. The lookup runs only at start-up (`UltiTools#initDataStore`), and the race needs it to coincide with a store registration or unregistration on another thread, which no command or server action arranges on demand | — | Held by `DataStoreManagerConcurrencyTest`: the registry is a `ConcurrentMap` (`registryIsAConcurrentMap`); a reader started after registration sees the store (`readerStartedAfterRegistrationSeesTheStore`); across 20 repetitions each, reads of a registered type during register/unregister churn of other types never return `null` (`readsNeverReturnNullDuringChurn`), and a type being registered and unregistered reads as itself or as the json fallback, never `null` (`typeBeingUnregisteredReadsAsItselfOrJson`); plus the pre-existing `DataStoreManagerTest$ConcurrentReadWriteTests#concurrentReadWriteShouldBeSafe` | server | | +| ultitools.storage.conditional-update | not covered — unit-pinned here. No shipped module calls `updateIf` until UltiEconomy's wallet merge adopts it (UltiKits/UltiEconomy#39), whose own checklist carries the real-server row for a single-server merge; the concurrent case — two servers sharing a database, one stalled between its read and its write — cannot be arranged on demand on a real server | — | Held by `ConditionalUpdateTest`: on the JSON, SQLite and MySQL operators, `updateIf` returns true and writes while the stored balance equals the one read (`appliesWhenExpectedHolds`), returns false and writes nothing once it changed (`refusesWhenExpectedNoLongerHolds`), for a missing row (`falseForMissingRow`) or when one of two conditions fails, and matches a value containing a quote (`everyConditionMustHold`); a null id throws `DataAccessException` (`throwsForNullId`), and so do an unmapped condition column (`unknownColumnThrows`) and a null expected value (`nullExpectedValueThrows`) on all three; of two operators over one database, the writer conditioned on a balance the other already changed does not apply (`secondWriterLoses`); an implementation without it throws `UnsupportedOperationException` naming its class (`defaultThrowsNamingTheClass`) | server | | +| ultitools.storage.detached-reads | not covered — unit-pinned. No shipped module changes a loaded entity without calling `update(...)` (the only code path that would show the difference), so the row cannot be exercised through any command this repository or the fifteen modules ship | — | Held by `DetachedReadParityTest` (`getAllReturnsDetachedCopies`, `getAllWithConditionsReturnsDetachedCopies`, `getByIdReturnsDetachedCopy`, `pageReturnsDetachedCopies`, `getLikeReturnsDetachedCopies`, `queryFirstReturnsDetachedCopy`, `insertedInstanceIsDetached`, `existMatchesByIdAfterALocalChange`, `updatePersistsTheChange`): each runs the same scenario on the JSON backend and on the relational backend, and a change made to a returned entity without `update(...)` leaves a fresh `getById` unchanged on both, `exist(entity)` still finds the row by its id after such a change on both, and `update(...)` persists it on both | server | | +| ultitools.storage.missing-row-update | not covered — unit-pinned. No shipped module calls `updateCounted` until the three module adoptions (UltiEconomy, UltiTrade, UltiWorlds) land, and an update racing another writer's delete cannot be arranged on demand on a real server | — | Held by `MissingRowUpdateTest`: on JSON, SQLite and MySQL, `update(T)` (`updateEntity`), `update(column, value, id)` (`updateColumn`) and `updateAll` (`updateAll`) by an id no row has throw nothing, write nothing and log one WARNING naming the table and id per call; `updateCounted` returns 1 and writes for a stored row, 0 with one WARNING for a missing one (`updateCountedReportsTheWrite`), throws `DataAccessException` for a null id (`updateCountedRefusesNullId`); a third-party operator's default counts by existence before its update (`defaultCountsByExistence`) | server | | +| ultitools.storage.null-id-backfill | `language: en` in config.yml; `datasource.type: sqlite` (shipped default); UltiWorlds loaded; while the server is still running, `/world info world` shows `PVP: Enabled` (run `/world set world pvp true` first if it does not — otherwise step 2 below cannot tell a saved change from an unchanged value); then, with the server stopped, `plugins/UltiTools/sqliteDB/UltiWorlds.db` table `world_settings` holds a row with `world_name = 'world'` whose `id` IS NULL — a server that ran UltiTools-API 6.2.0 has such rows already; otherwise make one with `sqlite3 plugins/UltiTools/sqliteDB/UltiWorlds.db "UPDATE world_settings SET id = NULL WHERE world_name = 'world'"`, and count the NULL-id rows first with `SELECT COUNT(*) FROM world_settings WHERE id IS NULL` (record that number as N) | 1. Start the server, read the console, and run `/world info world`. 2. Run `/world set world pvp false`, stop the server, start it again, run `/world info world`, and read the console of this second start. 3. With the server stopped, run `SELECT COUNT(*) FROM world_settings WHERE id IS NULL`. 4. Cleanup: `/world set world pvp true` | 1: exactly one console line containing `Assigned an id to N row(s) of table 'world_settings' that had none` (N as counted in the Preconditions), and `/world info world` shows `PVP: Enabled`. 2: `world.set.success` for the set; after the restart `/world info world` shows `PVP: Disabled` — before 6.3.0 it showed `Enabled` again, because the update matched no row — and the second start's console has no `Assigned an id` line for `world_settings`. 3: `0` | server | | +| ultitools.storage.null-id-refused | not covered — unit-pinned. No shipped command passes a null id to a `DataOperator` once the backfill above has run, so the refusal cannot be triggered through any command this repository or the fifteen modules ship | — | Held by `NullIdRowsTest$RefuseNullId` (`updateEntity`, `updateColumn`, `deleteById`, `updateAllWritesNothing`): on the SQLite and JSON backends each of `update(T)`, `update(column, value, id)`, `delById` and `updateAll` with a null id throws `DataAccessException`, and a refused `updateAll` leaves the stored row unchanged | server | | +| ultitools.storage.query-delete-count | not covered — unit-pinned. No shipped command reports `Query#delete()`'s return value to an operator, and the two cases that distinguish a removed-row count from a match count (a row removed by another writer between the read and the delete, a matched row with a null id) cannot be arranged on demand on a real server | — | Held by `QueryDeleteCountTest`: on the JSON backend and on the relational backend, `delete()` returns 2 for two removed rows and a re-query finds neither (`jsonReturnsRemovedCount`, `sqliteReturnsRemovedCount`); a matched row another writer removed first is not counted (`jsonDoesNotCountARowItDidNotRemove`, `sqliteDoesNotCountARowItDidNotRemove`); an operator whose delete removes nothing reports 0 (`foreignOperatorThatRemovesNothingCountsZero`); a matched null-id row throws `DataAccessException` naming the entity type and nothing is deleted (`jsonRefusesNullIdRow`, `refusesBeforeDeletingAnything`) | server | | | ultitools.storage.restart-survival | A row written through a `DataOperator` while the server is up (any module command backed by `@Table`) | Stop the server completely, then start it again, then read the same row back through the same module command | The same value is returned after restart, in whichever backend `ultitools.storage.backend-select` is currently active | server | | ## Module configuration persistence diff --git a/src/main/java/com/ultikits/ultitools/interfaces/DataOperator.java b/src/main/java/com/ultikits/ultitools/interfaces/DataOperator.java index 02322fe86..9970d66d9 100644 --- a/src/main/java/com/ultikits/ultitools/interfaces/DataOperator.java +++ b/src/main/java/com/ultikits/ultitools/interfaces/DataOperator.java @@ -5,6 +5,8 @@ import com.ultikits.ultitools.abstracts.data.BaseDataEntity; import com.ultikits.ultitools.entities.WhereCondition; +import com.ultikits.ultitools.exceptions.DataAccessException; +import com.ultikits.ultitools.exceptions.ErrorCode; import com.ultikits.ultitools.interfaces.impl.data.QueryImpl; /** @@ -120,6 +122,95 @@ enum LikeType { */ void update(T obj) throws IllegalAccessException; + /** + * Counted update: {@link #update(BaseDataEntity)}, returning how many rows it wrote, so the + * caller can tell that nothing was written (#558). + *

+ * An update by a non-null id that matches no row writes nothing on every backend: SQLite, + * MySQL and JSON each log one WARNING naming the table and the id, and return normally. + * {@code update(T)} gives the caller no way to see that; this method returns {@code 0} + * instead of {@code 1}. Use it wherever "the row was gone" must be treated as a failure -- + * for example a write that credits an account that another server has just removed. For a + * write that should apply only while the stored row still has the values you read, use + * {@link #updateIf} instead. + *

+ * The framework's operators return the backend's own affected-row count. This default, which + * a third-party implementation inherits unless it overrides it, checks with + * {@link #exist(WhereCondition...)} whether a row with the entity's id exists, returns + * {@code 0} without writing if it does not, and otherwise calls {@code update(T)} and returns + * {@code 1}. It cannot see what that {@code update} wrote, so one case remains in which it + * reports {@code 1} although nothing was written: another writer deleting the row during that + * one call. + * + * @param entity the new state of the row, carrying the id of the row to write + * @return the number of rows written: {@code 1}, or {@code 0} when no row has that id + * @throws DataAccessException if {@code entity}'s id is {@code null}, or the write fails + * @since 6.3.0 + */ + default int updateCounted(T entity) { + Object id = entity.getId(); + if (id == null) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, + "Refusing to update a " + entity.getClass().getName() + " by a null id: no row can be addressed by it."); + } + if (!exist(WhereCondition.builder().column("id").value(id).build())) { + return 0; + } + try { + update(entity); + } catch (IllegalAccessException e) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, "Failed to access entity fields", e); + } + return 1; + } + + /** + * Conditional update: writes {@code entity} over the stored row with the same id only if + * that row still matches every one of {@code expected}, and reports whether it did (#543). + *

+ * The check and the write are one step: on SQLite and MySQL a single + * {@code UPDATE ... WHERE id = ? AND } whose affected-row count decides the result, + * so it holds across servers sharing one database; on the JSON backend the check and the write + * run under the operator's own lock (a JSON store is local to one server). The conditions mean + * exactly what they mean in {@link #getAll(WhereCondition...)} on the same backend, values are + * bound as parameters, and a condition whose {@code isEmpty()} is true is ignored. + *

+ * Typical use is compare-and-set: read a row, compute the new state, and write it conditioned + * on the value that was read; if another writer changed the row in between, nothing is + * written and {@code false} comes back, so the caller re-reads and decides again rather than + * writing on top. UltiEconomy's one-time wallet merge uses it this way, conditioned on the + * balance it read: + * 

{@code
+     * Account read = accounts.getById(id);
+     * double seen = read.getBalance();
+     * read.setBalance(seen + amount);
+     * if (!accounts.updateIf(read, WhereCondition.builder().column("balance").value(seen).build())) {
+     *     // someone else wrote first: re-read and decide again
+     * }
+     * }
+ * Like {@link #update(BaseDataEntity)}, every mapped field of {@code entity} is written, and + * {@code onUpdate()} fires on {@code entity} before the fields are read, whether or not the + * write then applies. + * + * @param entity the new state of the row, carrying the id of the row to write + * @param expected the conditions the stored row must still meet + * @return {@code true} if the row matched and was written; {@code false} if no row with that + * id matched every condition, in which case nothing was written + * @throws com.ultikits.ultitools.exceptions.DataAccessException if {@code entity}'s id is + * {@code null}, a condition names a column the entity does not map with + * {@code @Column}, or a condition's value is {@code null} (no backend can compare with + * it; the write could never apply) -- on every backend, so a misspelt column cannot turn + * a retry loop into an endless one + * @throws UnsupportedOperationException if this implementation does not provide conditional + * writes -- the default, so a third-party implementation is never silently + * unconditional + * @since 6.3.0 + */ + default boolean updateIf(T entity, WhereCondition... expected) { + throw new UnsupportedOperationException(getClass().getName() + + " does not implement DataOperator#updateIf, so it cannot perform a conditional write."); + } + /** * Returns a new fluent query builder for this data operator. * diff --git a/src/main/java/com/ultikits/ultitools/interfaces/impl/data/AbstractRelationalDataOperator.java b/src/main/java/com/ultikits/ultitools/interfaces/impl/data/AbstractRelationalDataOperator.java index 56361715b..ca0d01e6e 100644 --- a/src/main/java/com/ultikits/ultitools/interfaces/impl/data/AbstractRelationalDataOperator.java +++ b/src/main/java/com/ultikits/ultitools/interfaces/impl/data/AbstractRelationalDataOperator.java @@ -3,18 +3,22 @@ import java.lang.reflect.Field; import java.sql.Connection; import java.sql.PreparedStatement; +import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; import java.time.LocalDateTime; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Set; +import java.util.UUID; import java.util.concurrent.Callable; +import java.util.logging.Level; import java.util.logging.Logger; import javax.sql.DataSource; @@ -27,6 +31,8 @@ import com.google.gson.GsonBuilder; import com.google.gson.JsonDeserializer; import com.google.gson.JsonNull; +import com.google.gson.JsonObject; +import com.google.gson.JsonParseException; import com.google.gson.JsonPrimitive; import com.google.gson.JsonSerializer; import com.ultikits.ultitools.abstracts.data.BaseDataEntity; @@ -53,9 +59,14 @@ * @since 6.2.0 */ @SuppressWarnings("PMD.AvoidAccessibilityAlteration") // ORM maps private @Column fields to SQL -- see 08-GATE05-TRIAGE.md -public abstract class AbstractRelationalDataOperator> implements DataOperator { +public abstract class AbstractRelationalDataOperator> + implements DataOperator, RowCountingDelete { private static final Logger LOGGER = Logger.getLogger(AbstractRelationalDataOperator.class.getName()); + /** The primary-key column every generated table declares; its value is always {@code getId()}. */ + private static final String ID_COLUMN = "id"; + /** Alias of the row-identifier column {@link #backfillNullIds} selects next to the entity's own. */ + private static final String BACKFILL_ROWID = "ultitools_backfill_rowid"; /** * Default Gson has no bundled adapter for {@code java.time.LocalDateTime}: its reflective * fallback tries to reach {@code LocalDateTime}'s private fields, which JDK 9+'s module @@ -203,6 +214,210 @@ private void initializeTable() { } } + /** + * Gives every row of this operator's table whose {@code id} is {@code NULL} an id that makes it + * addressable, and logs one line naming the table and the count when there were any (#546, + * maintainer decision of 2026-09-27). + *

+ * UltiTools-API 6.2.0 did not assign an id in {@link #insert}, and SQLite's generated DDL + * ({@code PRIMARY KEY (`id`)} with no {@code NOT NULL}) accepted the {@code NULL}, so every row + * a module inserted without an id on that release was stored with none. Such a row can be read + * by any other column, but {@code WHERE id = ?} bound to {@code null} matches nothing, so every + * update or delete of it silently did nothing. + *

+ * Each row gets the id its entity reports through {@code getId()} (an entity may derive it from + * another column), or a new UUID when it reports none -- but only if an entity read back with + * that id in the {@code id} column reports it, since every lookup binds {@code getId()}. A row + * no written id would make addressable is left as it is, and one WARNING per table counts them + * by reason: a derived id that more than one row without an id reports (none of them is + * written, maintainer decision of 2026-09-29, the rule UltiEssentials' own repair applies), a + * derived id another row already holds, and a derived id that is {@code null} or a row that + * cannot be read as the entity. Only the {@code id} column is written, guarded by {@code id IS NULL} and + * keyed by the engine's row identifier, all rows in one transaction. A second start writes + * nothing and logs no INFO line; rows that were left are counted again. A failure rolls the + * whole repair back and is logged at {@code SEVERE}; the table is still usable, and an update + * or delete through a row without an id is refused rather than silently matching nothing. + *

+ * Called by the subclass whose engine can hold a {@code NULL} id ({@code SQLiteDataOperator}); + * MySQL rejects a {@code NULL} primary key, so its operator never calls it. + * + * @param rowIdColumn the engine's row-identifier pseudo-column (SQLite's {@code _rowid_}); a + * constant supplied by the subclass, never caller input + * @return the number of rows given an id + * @since 6.3.0 + */ + protected final int backfillNullIds(String rowIdColumn) { + String select = "SELECT " + rowIdColumn + " AS " + BACKFILL_ROWID + ", `" + tableName + "`.* FROM `" + + tableName + "` WHERE `id` IS NULL"; + String update = "UPDATE `" + tableName + "` SET `id` = ? WHERE " + rowIdColumn + " = ? AND `id` IS NULL"; + String held = "SELECT COUNT(*) FROM `" + tableName + "` WHERE `id` = ?"; + int reported = 0; + int generated = 0; + int shared = 0; + int alreadyHeld = 0; + int unusable = 0; + try (Connection conn = dataSource.getConnection()) { + boolean autoCommit = conn.getAutoCommit(); + conn.setAutoCommit(false); + try { + List rowIds = new ArrayList<>(); + List candidates = new ArrayList<>(); + List fromEntity = new ArrayList<>(); + readRowsWithoutId(conn, select, rowIds, candidates, fromEntity); + Map occurrences = new HashMap<>(); + for (String id : candidates) { + if (id != null) { + occurrences.merge(id, 1, Integer::sum); + } + } + for (int i = 0; i < rowIds.size(); i++) { + String id = candidates.get(i); + if (id == null) { + unusable++; + } else if (occurrences.get(id) > 1) { + // Maintainer decision 2026-09-29 ("touch none of those rows, only warn"): a reported id more + // than one row shares is written to none of them, or a write made through + // one row would reach the other -- UltiEssentials' own repair does the same. + shared++; + } else if (isHeld(conn, held, id) || !assignId(conn, update, id, rowIds.get(i))) { + alreadyHeld++; + } else if (fromEntity.get(i)) { + reported++; + } else { + generated++; + } + } + conn.commit(); + } catch (SQLException e) { + conn.rollback(); + throw e; + } finally { + conn.setAutoCommit(autoCommit); + } + } catch (SQLException e) { + LOGGER.log(Level.SEVERE, "Could not assign ids to the rows of table '" + tableName + + "' that have none; nothing was changed. Rows without an id cannot be updated or deleted " + + "until this succeeds on a later start.", e); + return 0; + } + int repaired = reported + generated; + if (repaired > 0) { + LOGGER.info("Assigned an id to " + repaired + " row(s) of table '" + tableName + + "' that had none (rows written without an id by UltiTools-API 6.2.0): " + reported + + " took the id the entity itself reports, " + generated + " were given a new UUID; " + + "only the id column was written."); + } + int left = shared + alreadyHeld + unusable; + if (left > 0) { + LOGGER.warning(left + " row(s) of table '" + tableName + "' still have no id and were left as " + + "they are: " + shared + " share a reported id with another row without an id, " + + alreadyHeld + " report an id another row already holds, " + unusable + + " report no id or could not be read as " + type.getName() + ". No value written to " + + "the id column would make them addressable by getId() alone, so they cannot be " + + "updated or deleted by id."); + } + return repaired; + } + + /** + * Reads every row of {@code select} (row identifier first, then the table's columns) and + * decides which id, if any, would make it addressable. + *

+ * Every {@code WHERE id = ?} binds {@code getId()}, and an entity may derive {@code getId()} + * from another column -- UltiEssentials' and UltiKits' do -- so an id is useful only if an + * entity read back with it in the {@code id} column reports it. The id the entity already + * reports is tried first; otherwise a new UUID. A candidate that would not be reported back + * (the entity derives a {@code null} id), and a row that cannot be read as the entity at all, + * get {@code null}: writing anything to such a row would change nothing any lookup uses. + */ + private void readRowsWithoutId(Connection conn, String select, List rowIds, List candidates, + List fromEntity) throws SQLException { + RowMapper mapper = getRowMapper(); + queryRunner.query(conn, select, rs -> { + while (rs.next()) { + rowIds.add(rs.getObject(1)); + T entity = readEntity(mapper, rs); + Object reported = entity == null ? null : entity.getId(); + boolean hasReported = reported != null && !reported.toString().isEmpty(); + String candidate = hasReported ? reported.toString() : UUID.randomUUID().toString(); + candidates.add(entity != null && reportsAs(entity, candidate) ? candidate : null); + fromEntity.add(hasReported); + } + return null; + }); + } + + private T readEntity(RowMapper mapper, ResultSet rs) throws SQLException { + try { + return mapper.map(rs); + } catch (JsonParseException e) { + LOGGER.warning("A row of table '" + tableName + "' without an id could not be read as " + + type.getName() + " (" + e.getMessage() + "); it is left without an id."); + return null; + } + } + + /** + * Whether {@code entity}, read back with {@code candidate} in its {@code id} column, reports + * {@code candidate} from {@code getId()} -- that is, whether writing it makes the row + * addressable. + */ + private boolean reportsAs(T entity, String candidate) { + JsonObject tree = GSON.toJsonTree(entity).getAsJsonObject(); + // BaseDataEntity's id field is named like its column. + tree.addProperty(ID_COLUMN, candidate); + Object reported = GSON.fromJson(tree, type).getId(); + return reported != null && candidate.equals(reported.toString()); + } + + /** Whether a row of this table already holds {@code id} in its id column. */ + private boolean isHeld(Connection conn, String held, String id) throws SQLException { + Number count = queryRunner.query(conn, held, new ScalarHandler(), id); + return count != null && count.longValue() > 0; + } + + /** + * Writes {@code id} into the row with this row identifier, on the backfill's own connection + * (so inside its transaction), if the row still has none. A value another row already holds + * violates the primary key; that is reported as {@code false} and the row is left without an + * id, since any other value would not be what its entity reports. The statement fails alone: + * SQLite rolls back only the failing statement, and the transaction continues. + */ + private boolean assignId(Connection conn, String update, String id, Object rowId) throws SQLException { + try { + return queryRunner.update(conn, update, id, rowId) > 0; + } catch (SQLException e) { + if (isConstraintViolation(e)) { + return false; + } + throw e; + } + } + + private static boolean isConstraintViolation(SQLException e) { + String state = e.getSQLState(); + // SQLState class 23 is "integrity constraint violation"; the SQLite driver reports a + // PRIMARY KEY/UNIQUE failure with no SQLState, but always names the constraint. + return (state != null && state.startsWith("23")) + || (e.getMessage() != null && e.getMessage().contains("constraint")); + } + + /** + * Refuses an update or delete addressed by a {@code null} id (#546): {@code WHERE id = ?} bound + * to {@code null} matches no row, so the call used to return normally having changed nothing. + * + * @param id the id the caller addressed the row by + * @param operation what was attempted, for the message + * @throws DataAccessException if {@code id} is {@code null} + */ + private void requireId(Object id, String operation) { + if (id == null) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, + "Refusing to " + operation + " a row of table '" + tableName + "' (" + type.getName() + + ") by a null id: no row can be addressed by it."); + } + } + /** * The read handler used by {@link #getAll(WhereCondition...)}, {@link #getLike}, and * {@link #page}: fires {@code onLoad()} once per row it materializes. {@link #getById} and @@ -228,6 +443,27 @@ protected ResultSetHandler> getListHandler() { * delete-hook-path split). */ private ResultSetHandler> getRawListHandler() { + RowMapper mapper = getRowMapper(); + return rs -> { + List list = new ArrayList<>(); + while (rs.next()) { + list.add(mapper.map(rs)); + } + return list; + }; + } + + /** Materialises the current row of a result set as an entity; see {@link #getRowMapper()}. */ + private interface RowMapper { + E map(ResultSet rs) throws SQLException; + } + + /** + * Materialises one row as an entity without firing {@code onLoad()}: shared by + * {@link #getRawListHandler()} and {@link #backfillNullIds}, so a row is read the same way + * whichever needs it. A column labelled {@link #BACKFILL_ROWID} is skipped. + */ + private RowMapper getRowMapper() { // Build mappings from SQL column names to Java field names and boolean detection. // Gson matches JSON keys to Java field names, so we must use field names (camelCase) // as map keys, not SQL column names (snake_case). @@ -248,25 +484,23 @@ private ResultSetHandler> getRawListHandler() { } return rs -> { - List list = new ArrayList<>(); ResultSetMetaData meta = rs.getMetaData(); int cols = meta.getColumnCount(); - while (rs.next()) { - Map map = new LinkedHashMap<>(); - for (int i = 1; i <= cols; i++) { - String colName = meta.getColumnLabel(i).toLowerCase(Locale.ROOT); - Object value = rs.getObject(i); - if (booleanColumns.containsKey(colName)) { - value = normaliseBoolean(value, colName); - } - // Use Java field name as key so Gson can match it during deserialization - String fieldName = columnToFieldName.getOrDefault(colName, colName); - map.put(fieldName, value); + Map map = new LinkedHashMap<>(); + for (int i = 1; i <= cols; i++) { + String colName = meta.getColumnLabel(i).toLowerCase(Locale.ROOT); + if (BACKFILL_ROWID.equals(colName)) { + continue; + } + Object value = rs.getObject(i); + if (booleanColumns.containsKey(colName)) { + value = normaliseBoolean(value, colName); } - String json = GSON.toJson(map); - list.add(GSON.fromJson(json, type)); + // Use Java field name as key so Gson can match it during deserialization + String fieldName = columnToFieldName.getOrDefault(colName, colName); + map.put(fieldName, value); } - return list; + return GSON.fromJson(GSON.toJson(map), type); }; } @@ -472,7 +706,7 @@ public List page(int page, int size, WhereCondition... whereConditions) { public void insert(T obj) { // Auto-generate UUID for id if not set if (obj.getId() == null) { - obj.setId(java.util.UUID.randomUUID().toString()); + obj.setId(UUID.randomUUID().toString()); } // Fires before the fields below are read for the SQL parameters, so whatever onCreate() // writes (e.g. AuditableDataEntity's createdAt/createdBy) is what actually gets persisted. @@ -493,15 +727,7 @@ public void insert(T obj) { sql.append("`").append(column.value()).append("`"); values.append("?"); try { - Object value = field.get(obj); - if (value != null && !BasicTypeUtil.isBasicType(field.getType())) { - String jsonString = GSON.toJson(value); - if (jsonString.startsWith("\"") && jsonString.endsWith("\"")) { - jsonString = jsonString.substring(1, jsonString.length() - 1); - } - value = jsonString; - } - params.add(value); + params.add(persistedValue(field, column.value(), obj)); } catch (IllegalAccessException e) { throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, "Failed to access entity fields", e); @@ -556,13 +782,27 @@ public void del(WhereCondition... whereConditions) { */ @Override public void delById(Object id) { + deleteByIdCounted(id); + } + + /** + * {@link #delById(Object)}, returning the affected-row count the database reported for the + * {@code DELETE} (#521), so {@code Query#delete()} can return rows removed instead of rows + * matched. + * + * @param id the row id + * @return the number of rows the {@code DELETE} removed + */ + @Override + public int deleteByIdCounted(Object id) { + requireId(id, "delete"); T entity = fetchRawById(id); if (entity != null) { entity.onDelete(); } String sql = "DELETE FROM " + tableName + " WHERE id = ?"; try { - queryRunner.update(sql, id); + return queryRunner.update(sql, id); } catch (SQLException e) { throw new DataAccessException(ErrorCode.DATA_OPERATION_FAILED, "Failed to delete entity by id: " + id, e); @@ -571,12 +811,13 @@ public void delById(Object id) { @Override public void update(String column, Object value, Object id) { + requireId(id, "update"); if (value != null && !BasicTypeUtil.isBasicType(value.getClass())) { value = GSON.toJson(value); } String sql = "UPDATE " + tableName + " SET " + column + " = ? WHERE id = ?"; try { - queryRunner.update(sql, value, id); + warnIfNoRow(queryRunner.update(sql, value, id), id); } catch (SQLException e) { throw new DataAccessException(ErrorCode.DATA_OPERATION_FAILED, "Failed to update column: " + column, e); @@ -585,13 +826,114 @@ public void update(String column, Object value, Object id) { @Override public void update(T obj) throws IllegalAccessException { + updateRow(obj); + } + + /** + * {@inheritDoc} + *

+ * Returns the database's affected-row count for the {@code UPDATE}. + */ + @Override + public int updateCounted(T entity) { + try { + return updateRow(entity); + } catch (IllegalAccessException e) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, "Failed to access entity fields", e); + } + } + + /** + * {@link #update(BaseDataEntity)}, returning the number of rows the {@code UPDATE} changed. + * A non-null id that matches no row writes nothing and logs one WARNING (#558). + */ + private int updateRow(T obj) throws IllegalAccessException { + requireId(obj.getId(), "update"); // Fires before the fields below are read for the SQL parameters, so whatever onUpdate() // writes (e.g. AuditableDataEntity's updatedAt/updatedBy) is what actually gets // persisted. onUpdate() does not touch createdAt/createdBy, so an entity carrying its // original creation values in memory persists them unchanged here. obj.onUpdate(); - StringBuilder sql = new StringBuilder("UPDATE ").append(tableName).append(" SET "); + StringBuilder sql = new StringBuilder(); + List params = new ArrayList<>(); + appendUpdateSet(sql, params, obj); + sql.append(" WHERE id = ?"); + params.add(obj.getId()); + try { + return warnIfNoRow(queryRunner.update(sql.toString(), params.toArray()), obj.getId()); + } catch (SQLException e) { + throw new DataAccessException(ErrorCode.DATA_OPERATION_FAILED, + "Failed to update entity", e); + } + } + + /** + * Logs one WARNING when an update by a non-null id changed no row, and returns the count + * unchanged (#558, maintainer decision 2026-09-29). The call still returns normally: a row + * another writer deleted is not a storage failure, and module code catches exceptions from + * {@code update} as storage failures. + */ + private int warnIfNoRow(int changed, Object id) { + if (changed == 0) { + LOGGER.warning("Update of table '" + tableName + "' matched no row with id '" + id + + "'; nothing was written."); + } + return changed; + } + + /** + * {@inheritDoc} + *

+ * One {@code UPDATE ... SET WHERE id = ? AND } statement; the + * database's affected-row count decides the result, so the check and the write cannot be + * separated by another writer, on the same server or another one sharing the database. + * Condition columns pass the same allow-list as every other WHERE clause here, and values are + * bound as parameters. + */ + @Override + public boolean updateIf(T entity, WhereCondition... expected) { + requireId(entity.getId(), "update"); + entity.onUpdate(); + StringBuilder sql = new StringBuilder(); List params = new ArrayList<>(); + try { + appendUpdateSet(sql, params, entity); + } catch (IllegalAccessException e) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, "Failed to access entity fields", e); + } + List conditions = new ArrayList<>(); + conditions.add(WhereCondition.builder().column("id").value(entity.getId()).build()); + if (expected != null) { + for (WhereCondition condition : expected) { + if (condition == null) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, + "updateIf was given a null condition for table '" + tableName + "'."); + } + if (!condition.isEmpty() && condition.getValue() == null) { + // `column = NULL` is never true in SQL, so the write could never apply and a + // caller's re-read-and-retry loop would spin forever. + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, + "updateIf cannot compare column '" + condition.getColumn() + "' with a null value."); + } + conditions.add(condition); + } + } + appendConditions(sql, params, conditions.toArray(new WhereCondition[0]), true); + try { + return queryRunner.update(sql.toString(), params.toArray()) > 0; + } catch (SQLException e) { + throw new DataAccessException(ErrorCode.DATA_OPERATION_FAILED, + "Failed to conditionally update entity", e); + } + } + + /** + * Appends {@code UPDATE SET `col` = ?, ...} for every {@code @Column} field of + * {@code obj}, and the values in the same order, serialised exactly as {@link #insert} does. + * Shared by {@link #update(BaseDataEntity)} and {@link #updateIf}. + */ + private void appendUpdateSet(StringBuilder sql, List params, T obj) throws IllegalAccessException { + sql.append("UPDATE ").append(tableName).append(" SET "); Field[] fields = ReflectionUtil.getFields(obj.getClass()); boolean first = true; for (Field field : fields) { @@ -602,26 +944,10 @@ public void update(T obj) throws IllegalAccessException { sql.append(", "); } sql.append("`").append(column.value()).append("` = ?"); - Object value = field.get(obj); - if (value != null && !BasicTypeUtil.isBasicType(field.getType())) { - String jsonString = GSON.toJson(value); - if (jsonString.startsWith("\"") && jsonString.endsWith("\"")) { - jsonString = jsonString.substring(1, jsonString.length() - 1); - } - value = jsonString; - } - params.add(value); + params.add(persistedValue(field, column.value(), obj)); first = false; } } - sql.append(" WHERE id = ?"); - params.add(obj.getId()); - try { - queryRunner.update(sql.toString(), params.toArray()); - } catch (SQLException e) { - throw new DataAccessException(ErrorCode.DATA_OPERATION_FAILED, - "Failed to update entity", e); - } } // ===== Transaction support ===== @@ -672,7 +998,7 @@ public void insertAll(List entities) { // into insert() would silently skip this path. for (T entity : entities) { if (entity.getId() == null) { - entity.setId(java.util.UUID.randomUUID().toString()); + entity.setId(UUID.randomUUID().toString()); } entity.onCreate(); } @@ -699,15 +1025,7 @@ public void insertAll(List entities) { for (T entity : entities) { int idx = 1; for (ColumnMapping col : columns) { - Object value = col.field.get(entity); - if (value != null && !BasicTypeUtil.isBasicType(col.field.getType())) { - String json = GSON.toJson(value); - if (json.startsWith("\"") && json.endsWith("\"")) { - json = json.substring(1, json.length() - 1); - } - value = json; - } - pstmt.setObject(idx++, value); + pstmt.setObject(idx++, persistedValue(col.field, col.columnName, entity)); } pstmt.addBatch(); } @@ -724,6 +1042,10 @@ public void updateAll(List entities) throws IllegalAccessException { if (entities == null || entities.isEmpty()) { return; } + // Checked before the batch starts, so a refused batch writes nothing. + for (T entity : entities) { + requireId(entity.getId(), "update"); + } try { transaction((Callable) () -> { List columns = getColumnMappings(); @@ -751,20 +1073,16 @@ public void updateAll(List entities) throws IllegalAccessException { entity.onUpdate(); int idx = 1; for (ColumnMapping col : columns) { - Object value = col.field.get(entity); - if (value != null && !BasicTypeUtil.isBasicType(col.field.getType())) { - String json = GSON.toJson(value); - if (json.startsWith("\"") && json.endsWith("\"")) { - json = json.substring(1, json.length() - 1); - } - value = json; - } - pstmt.setObject(idx++, value); + pstmt.setObject(idx++, persistedValue(col.field, col.columnName, entity)); } pstmt.setObject(idx, entity.getId()); pstmt.addBatch(); } - pstmt.executeBatch(); + int[] changed = pstmt.executeBatch(); + for (int row = 0; row < changed.length; row++) { + // Statement.SUCCESS_NO_INFO (-2) says nothing about the row; only 0 means no match. + warnIfNoRow(changed[row], entities.get(row).getId()); + } } catch (SQLException | IllegalAccessException e) { throw new DataAccessException(ErrorCode.DATA_OPERATION_FAILED, "Batch update failed", e); @@ -781,6 +1099,29 @@ public void updateAll(List entities) throws IllegalAccessException { } } + /** + * The value written for one {@code @Column} field of {@code obj}, serialised as every write + * path here always has: a non-basic value goes out as JSON, with a bare string's quotes + * removed. + *

+ * The {@code id} column is written from {@code getId()}, not from the inherited field, because + * {@code getId()} is what every {@code WHERE id = ?} binds (#546). An entity that overrides + * {@code getId()} onto another column -- UltiEssentials' and UltiKits' do -- never sets the + * inherited field, so writing the field stored a {@code NULL} id that no update or delete could + * reach (and that MySQL refused outright). + */ + private Object persistedValue(Field field, String columnName, T obj) throws IllegalAccessException { + Object value = ID_COLUMN.equals(columnName) ? obj.getId() : field.get(obj); + if (value != null && !BasicTypeUtil.isBasicType(field.getType())) { + String json = GSON.toJson(value); + if (json.startsWith("\"") && json.endsWith("\"")) { + json = json.substring(1, json.length() - 1); + } + value = json; + } + return value; + } + // ===== Column mapping helper ===== private static class ColumnMapping { diff --git a/src/main/java/com/ultikits/ultitools/interfaces/impl/data/QueryImpl.java b/src/main/java/com/ultikits/ultitools/interfaces/impl/data/QueryImpl.java index 8c012580c..3c76cd1ab 100644 --- a/src/main/java/com/ultikits/ultitools/interfaces/impl/data/QueryImpl.java +++ b/src/main/java/com/ultikits/ultitools/interfaces/impl/data/QueryImpl.java @@ -10,6 +10,8 @@ import com.ultikits.ultitools.abstracts.data.BaseDataEntity; import com.ultikits.ultitools.annotations.Column; import com.ultikits.ultitools.entities.WhereCondition; +import com.ultikits.ultitools.exceptions.DataAccessException; +import com.ultikits.ultitools.exceptions.ErrorCode; import com.ultikits.ultitools.interfaces.DataOperator; import com.ultikits.ultitools.interfaces.Query; import com.ultikits.ultitools.utils.ReflectionUtil; @@ -255,15 +257,54 @@ public long count() { return list().size(); } + /** + * Deletes every row this query matches and returns the number of rows the backend actually + * removed (#521). + *

+ * Before 6.3.0 this returned the number of rows the query matched, and a matched row + * with a {@code null} id was skipped yet still counted, so the value could report rows removed + * that were not. Now: + *

    + *
  • the count comes from the backend: the framework's operators report each delete's + * affected rows through {@link RowCountingDelete}, so a matched row that another writer + * removed first is not counted. A third-party operator that does not implement it cannot + * report what its {@code delById} removed, so a row is counted only if it existed + * immediately before that call and is gone after it; a row another writer removes during + * that one call is the only case this can still count;
    • + *
  • a matched row with a {@code null} id cannot be addressed by any delete, so it is + * refused with a {@link DataAccessException} naming the entity type before + * anything is deleted, instead of being skipped silently.
    • + *
+ * + * @return the number of rows removed + * @throws DataAccessException if a matched row has a {@code null} id; nothing is deleted then + */ @Override public int delete() { List toDelete = list(); for (T entity : toDelete) { - if (entity.getId() != null) { - operator.delById(entity.getId()); + if (entity.getId() == null) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, + "Query#delete() matched a " + entity.getClass().getName() + " row with a null id, " + + "which no delete can address; nothing was deleted."); } } - return toDelete.size(); + int removed = 0; + for (T entity : toDelete) { + removed += deleteOne(entity.getId()); + } + return removed; + } + + private int deleteOne(Object id) { + if (operator instanceof RowCountingDelete) { + return ((RowCountingDelete) operator).deleteByIdCounted(id); + } + WhereCondition byId = WhereCondition.builder().column("id").value(id).build(); + boolean presentBefore = operator.exist(byId); + operator.delById(id); + boolean goneAfter = !operator.exist(byId); + return presentBefore && goneAfter ? 1 : 0; } // === Internal Helpers === diff --git a/src/main/java/com/ultikits/ultitools/interfaces/impl/data/RowCountingDelete.java b/src/main/java/com/ultikits/ultitools/interfaces/impl/data/RowCountingDelete.java new file mode 100644 index 000000000..12a1d3e96 --- /dev/null +++ b/src/main/java/com/ultikits/ultitools/interfaces/impl/data/RowCountingDelete.java @@ -0,0 +1,29 @@ +package com.ultikits.ultitools.interfaces.impl.data; + +import org.jetbrains.annotations.ApiStatus; + +/** + * A data operator that can report how many rows a delete by id actually removed (#521). + *

+ * {@code DataOperator#delById} returns {@code void}, and its signature is part of the published + * API, so the affected-row count travels through this internal side interface instead. + * {@link QueryImpl#delete()} uses it to return the number of rows the backend removed rather than + * the number the query matched. The framework's own operators implement it; a third-party + * {@code DataOperator} that does not is counted by checking that the row existed immediately before + * its {@code delById} and is gone after it. + * + * @since 6.3.0 + */ +@ApiStatus.Internal +public interface RowCountingDelete { + + /** + * Deletes the row with this id, exactly as {@code DataOperator#delById} does, and returns the + * number of rows the backend removed. + * + * @param id the row id, not {@code null} + * @return the number of rows removed: {@code 0} when no row had this id (for example because + * another writer removed it first), otherwise {@code 1} + */ + int deleteByIdCounted(Object id); +} diff --git a/src/main/java/com/ultikits/ultitools/interfaces/impl/data/json/SimpleJsonDataOperator.java b/src/main/java/com/ultikits/ultitools/interfaces/impl/data/json/SimpleJsonDataOperator.java index 743cf9fb5..cd361c6ea 100644 --- a/src/main/java/com/ultikits/ultitools/interfaces/impl/data/json/SimpleJsonDataOperator.java +++ b/src/main/java/com/ultikits/ultitools/interfaces/impl/data/json/SimpleJsonDataOperator.java @@ -24,6 +24,7 @@ import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; +import java.util.logging.Logger; import org.bukkit.Bukkit; import org.bukkit.ChatColor; @@ -37,11 +38,13 @@ import com.google.gson.reflect.TypeToken; import com.ultikits.ultitools.abstracts.data.BaseDataEntity; import com.ultikits.ultitools.annotations.Column; +import com.ultikits.ultitools.annotations.Table; import com.ultikits.ultitools.entities.WhereCondition; import com.ultikits.ultitools.exceptions.DataAccessException; import com.ultikits.ultitools.exceptions.ErrorCode; import com.ultikits.ultitools.interfaces.Cached; import com.ultikits.ultitools.interfaces.DataOperator; +import com.ultikits.ultitools.interfaces.impl.data.RowCountingDelete; import com.ultikits.ultitools.manager.JsonTransactionManager; import com.ultikits.ultitools.utils.BeanCopyUtil; import com.ultikits.ultitools.utils.FileUtils; @@ -55,7 +58,9 @@ * @author wisdomme * @version 1.0.0 */ -public class SimpleJsonDataOperator> implements DataOperator, Cached { +public class SimpleJsonDataOperator> + implements DataOperator, Cached, RowCountingDelete { + private static final Logger LOGGER = Logger.getLogger(SimpleJsonDataOperator.class.getName()); /** * Default Gson has no bundled adapter for {@code java.time.LocalDateTime}: its reflective * fallback tries to reach {@code LocalDateTime}'s private fields, which JDK 9+'s module @@ -161,9 +166,9 @@ private Map uncheckedCast(Object snapshot) { } /** - * Deep-copies this operator's current cache: serialize/deserialize to break references, since - * {@code update(T)} mutates cached entities in-place via {@link - * BeanCopyUtil#copyProperties}. Package-private hook shared by {@link #transaction(Callable)} + * Deep-copies this operator's current cache: serialize/deserialize to break references, so a + * rollback restores the values as they were even if a cached instance is replaced or changed + * later. Package-private hook shared by {@link #transaction(Callable)} * (the pre-existing per-operator mechanism, unchanged) and {@link #beforeMutate()} (the new * per-plugin {@link JsonTransactionManager} hook, 02-05) -- one copy of the deep-copy logic, * two callers. @@ -171,7 +176,7 @@ private Map uncheckedCast(Object snapshot) { synchronized Map snapshotCache() { Map snapshot = new HashMap<>(); for (Map.Entry entry : cache.entrySet()) { - snapshot.put(entry.getKey(), GSON.fromJson(GSON.toJson(entry.getValue()), type)); + snapshot.put(entry.getKey(), detach(entry.getValue())); } return snapshot; } @@ -221,9 +226,58 @@ private String resolveColumn(String column) { return fieldName != null ? fieldName : column; } + /** + * Returns a detached copy of {@code entity}, produced by the same Gson form this operator + * persists to disk (#522). + *

+ * Every read path hands out one of these rather than the instance held in {@link #cache}, + * and {@link #insert} stores one rather than the caller's instance. Before 6.3.0 the cached + * instances themselves were returned, so changing a loaded entity without calling + * {@code update(...)} changed the store and was written at the next flush -- on + * {@code datasource.type: json} only; the relational backends materialise every row afresh, + * so there a change without {@code update(...)} never persisted. The copy is what makes the + * same module code behave the same on every backend. + * + * @param entity the cached (or caller-supplied) instance + * @return a new instance equal in every persisted field + */ + private T detach(T entity) { + // Through a JSON tree rather than text: the same Gson form, without pretty-printing a + // string only to parse it again. + return GSON.fromJson(GSON.toJsonTree(entity), type); + } + + /** + * Refuses an update or delete addressed by a {@code null} id (#546), which no entry can have + * as its key; the cache is a {@code ConcurrentHashMap}, so it used to surface as a raw + * {@code NullPointerException}. Mirrors {@code AbstractRelationalDataOperator}'s refusal. + */ + private void requireId(Object id, String operation) { + if (id == null) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, + "Refusing to " + operation + " an entry of JSON store '" + type.getName() + + "' by a null id: no entry can be addressed by it."); + } + } + + private List detachAll(List entities) { + List copies = new ArrayList<>(entities.size()); + for (T entity : entities) { + copies.add(detach(entity)); + } + return copies; + } + + /** + * Looks the entry up by the entity's id, as the relational operators do (#522). Comparing the + * caller's instance with the cached one through {@code equals()} stopped matching once reads + * and inserts were detached: an entity whose {@code equals()} covers a field it changed without + * calling {@code update(...)} no longer equals the stored copy. + */ @Override public boolean exist(T object) { - return cache.containsValue(object); + Object id = object == null ? null : object.getId(); + return id != null && findStored(id) != null; } @Override @@ -235,10 +289,12 @@ public boolean exist(WhereCondition... whereConditions) { @Override public T getById(Object id) { - T entity = cache.get(id); - if (entity != null) { - entity.onLoad(); + T cached = cache.get(id); + if (cached == null) { + return null; } + T entity = detach(cached); + entity.onLoad(); return entity; } @@ -256,7 +312,7 @@ public List getAll() { */ @Override public List getAll(WhereCondition... whereConditions) { - List results = getAllRaw(whereConditions); + List results = detachAll(getAllRaw(whereConditions)); for (T entity : results) { entity.onLoad(); } @@ -288,15 +344,10 @@ private List getAllRaw(WhereCondition... whereConditions) { throw new DataAccessException(ErrorCode.DATA_QUERY_FAILED, "Query value is not serializable"); } List collection = new ArrayList<>(); + String valueJson = GSON.toJson(condition.getValue()); for (T each : cache.values()) { Map map = GSON.fromJson(GSON.toJson(each), mapType); - Object byPath = JsonPathUtil.getByPath(map, resolveColumn(condition.getColumn())); - if (byPath == null) { - continue; - } - String data = GSON.toJson(byPath); - String value = GSON.toJson(condition.getValue()); - if (conditionCal(data, value, condition)) collection.add(each); + if (matches(map, condition, valueJson)) collection.add(each); } // Multiple conditions are ANDed: the first condition establishes the initial set, // every condition after it intersects, and an empty set stays empty. The original @@ -343,10 +394,11 @@ public List getLike(String column, String value, LikeType likeType) { break; } } - for (T entity : res) { + List copies = detachAll(res); + for (T entity : copies) { entity.onLoad(); } - return res; + return copies; } @Override @@ -364,7 +416,7 @@ public List page(int page, int size, WhereCondition... whereConditions) { if (end > all.size()) { end = all.size(); } - List slice = all.subList(start, end); + List slice = detachAll(all.subList(start, end)); for (T entity : slice) { entity.onLoad(); } @@ -386,10 +438,11 @@ public synchronized void insert(T obj) { } // Fires before the cache is touched, mirroring AbstractRelationalDataOperator.insert: // whatever onCreate() writes (e.g. AuditableDataEntity's createdAt/createdBy) is exactly - // what ends up cached (and, on flush(), persisted) -- obj is the same instance stored - // below, not a copy. + // what ends up cached (and, on flush(), persisted). A detached copy is cached, not obj + // itself, so a later change to obj does not reach the store without update() -- the + // same as on the relational backends (#522). obj.onCreate(); - cache.putIfAbsent(obj.getId(), obj); + cache.putIfAbsent(obj.getId(), detach(obj)); } /** @@ -434,15 +487,10 @@ public synchronized void del(WhereCondition... whereConditions) { } Collection> collection = new ArrayList<>(); Set> values = cache.entrySet(); + String valueJson = GSON.toJson(condition.getValue()); for (Map.Entry next : values) { Map map = GSON.fromJson(GSON.toJson(next.getValue()), mapType); - Object byPath = JsonPathUtil.getByPath(map, resolveColumn(condition.getColumn())); - if (byPath == null) { - continue; - } - String data = GSON.toJson(byPath); - String value = GSON.toJson(condition.getValue()); - if (conditionCal(data, value, condition)) collection.add(next); + if (matches(map, condition, valueJson)) collection.add(next); } // A copy of the same defect getAll had; likewise changed to a genuine intersection. // On the delete path, the original code would delete every row the second condition @@ -468,16 +516,30 @@ public synchronized void del(WhereCondition... whereConditions) { */ @Override public synchronized void delById(Object id) { + deleteByIdCounted(id); + } + + /** + * {@link #delById(Object)}, returning how many cache entries it removed (#521), so + * {@code Query#delete()} can return rows removed instead of rows matched. + * + * @param id the entry id + * @return {@code 1} if an entry with this id was removed, {@code 0} if there was none + */ + @Override + public synchronized int deleteByIdCounted(Object id) { + requireId(id, "delete"); beforeMutate(); T entity = cache.get(id); if (entity != null) { entity.onDelete(); } - cache.remove(id); + return cache.remove(id) != null ? 1 : 0; } @Override public synchronized void update(String column, Object value, Object id) { + requireId(id, "update"); beforeMutate(); if (!Serializable.class.isAssignableFrom(value.getClass())) { // GATE-05 group two (08-21): routed to the typed data-access hierarchy. Unlike the @@ -486,6 +548,10 @@ public synchronized void update(String column, Object value, Object id) { throw new DataAccessException(ErrorCode.DATA_PERSISTENCE_FAILED, "Query value is not serializable"); } T obj = cache.get(id); + if (obj == null) { + warnNoEntry(id); + return; + } Type mapType = new TypeToken>(){}.getType(); Map map = GSON.fromJson(GSON.toJson(obj), mapType); // Without resolving the column name first, putByPath would write into a brand-new key @@ -497,20 +563,159 @@ public synchronized void update(String column, Object value, Object id) { @Override public synchronized void update(T obj) { - beforeMutate(); + updateEntry(obj); + } + + /** + * {@inheritDoc} + *

+ * Returns the number of entries replaced, decided under this operator's lock. + */ + @Override + public synchronized int updateCounted(T entity) { + return updateEntry(entity); + } + + /** + * {@link #update(BaseDataEntity)}, returning how many entries it changed: an id no entry has + * writes nothing and logs one WARNING, where it used to throw a raw + * {@code NullPointerException} (#558). + */ + private int updateEntry(T obj) { Object id = obj.getId(); - T old = cache.get(id); + requireId(id, "update"); + T old = findStored(id); + // Copied onto a detached copy of the cached entry, which then replaces it in one put: + // reads run without this lock, so mutating the cached instance field by field could let + // a concurrent read serialise a half-updated entity. Callers always pass a detached + // instance since #522 (no read hands out the cached one), and BeanCopyUtil copies every + // primitive field since #520. + // + // onUpdate() fires on the caller's obj before its fields are copied, exactly as + // AbstractRelationalDataOperator.update(T) fires it before reading the fields it writes: + // whatever the hook writes (e.g. AuditableDataEntity's updatedAt/updatedBy) is both what + // persists and what the caller's instance shows afterwards, on every backend. + obj.onUpdate(); if (old == null) { - old = cache.get(id.toString()); + warnNoEntry(id); + return 0; } - BeanCopyUtil.copyProperties(obj, old, "id"); - // Fires on old (the instance actually cached below), after the incoming obj's fields have - // been copied onto it -- callers may pass either the same cached instance (the common - // get-mutate-update pattern) or a fresh detached instance with the same id; either way, - // whatever onUpdate() writes on the entity that ends up cached is what persists. Mirrors - // AbstractRelationalDataOperator.update(T)'s "fires before the row is written" ordering. - old.onUpdate(); - cache.put(old.getId(), old); + beforeMutate(); + replaceEntry(old, obj); + return 1; + } + + /** + * Logs one WARNING for an update by a non-null id that matches no entry (#558, maintainer + * decision 2026-09-29), naming the store by its {@code @Table} value -- the directory it is + * kept in -- as the relational backends name the table. + */ + private void warnNoEntry(Object id) { + Table table = type.getAnnotation(Table.class); + String name = table != null ? table.value() : type.getSimpleName(); + LOGGER.warning("Update of table '" + name + "' matched no entry with id '" + id + + "'; nothing was written."); + } + + /** The cached entry for {@code id}, trying its string form when the key type differs. */ + private T findStored(Object id) { + T stored = cache.get(id); + return stored != null ? stored : cache.get(id.toString()); + } + + /** + * Replaces {@code stored} in the cache with a copy carrying every field of {@code source} + * except {@code id}, in one {@code put}, so a concurrent read never sees a half-copied entry. + * Shared by {@link #update(BaseDataEntity)} and {@link #updateIf}. + */ + private void replaceEntry(T stored, T source) { + T updated = detach(stored); + BeanCopyUtil.copyProperties(source, updated, "id"); + cache.put(updated.getId(), updated); + } + + /** + * {@inheritDoc} + *

+ * The stored entry is checked against {@code expected} with the same matching + * {@link #getAll(WhereCondition...)} uses, and replaced, while this operator's lock is held, + * so no other write through this operator can come between the check and the write. A JSON + * store belongs to one server; nothing here coordinates two servers. + */ + @Override + public synchronized boolean updateIf(T entity, WhereCondition... expected) { + Object id = entity.getId(); + requireId(id, "update"); + List conditions = evaluableConditions(expected); + T stored = findStored(id); + entity.onUpdate(); + if (stored == null) { + return false; + } + Type mapType = new TypeToken>(){}.getType(); + Map map = GSON.fromJson(GSON.toJson(stored), mapType); + for (WhereCondition condition : conditions) { + if (!matches(map, condition, GSON.toJson(condition.getValue()))) { + return false; + } + } + beforeMutate(); + replaceEntry(stored, entity); + return true; + } + + /** + * The non-empty conditions of an {@link #updateIf} call, each checked before anything is + * read: a {@code null} condition, a column the entity does not map with {@code @Column}, and a + * {@code null} or non-serialisable value are refused with a {@link DataAccessException}, as + * the relational operators refuse them. {@link #getAll(WhereCondition...)} would simply match + * nothing for an unknown column, but a conditional write that can never apply would make the + * caller's re-read-and-retry loop spin forever. + */ + private List evaluableConditions(WhereCondition... expected) { + List conditions = new ArrayList<>(); + if (expected == null) { + return conditions; + } + for (WhereCondition condition : expected) { + if (condition == null) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, + "updateIf was given a null condition for JSON store '" + type.getName() + "'."); + } + if (condition.isEmpty()) { + continue; + } + String column = condition.getColumn(); + if (column == null || !columnToFieldName.containsKey(column.toLowerCase(Locale.ROOT))) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, + "Unknown column '" + column + "' for entity " + type.getName() + + " -- it is not among the entity's @Column mappings."); + } + if (condition.getValue() == null) { + throw new DataAccessException(ErrorCode.DATA_ENTITY_INVALID, + "updateIf cannot compare column '" + column + "' with a null value."); + } + if (!Serializable.class.isAssignableFrom(condition.getValue().getClass())) { + throw new DataAccessException(ErrorCode.DATA_QUERY_FAILED, "Query value is not serializable"); + } + conditions.add(condition); + } + return conditions; + } + + /** + * Whether one serialised entry satisfies one condition -- the single matching rule shared by + * {@link #getAll(WhereCondition...)}, {@link #del(WhereCondition...)} and {@link #updateIf}. + * An entry that lacks the condition's column never matches. {@code valueJson} is the + * condition's value already serialised, once per condition rather than once per entry; each + * caller has already checked that the value is serialisable. + */ + private boolean matches(Map map, WhereCondition condition, String valueJson) { + Object byPath = JsonPathUtil.getByPath(map, resolveColumn(condition.getColumn())); + if (byPath == null) { + return false; + } + return conditionCal(GSON.toJson(byPath), valueJson, condition); } @Override @@ -561,7 +766,6 @@ public void gc() { @Override public synchronized R transaction(Callable action) throws Exception { // Deep copy: serialize/deserialize to break references - // (update(T) mutates entities in-place via BeanCopyUtil.copyProperties) Map snapshot = snapshotCache(); try { return action.call(); @@ -604,6 +808,10 @@ public synchronized void insertAll(List entities) { @Override public synchronized void updateAll(List entities) throws IllegalAccessException { + // Checked before the batch starts, so a refused batch changes nothing. + for (T entity : entities) { + requireId(entity.getId(), "update"); + } try { transaction((Callable) () -> { for (T entity : entities) { diff --git a/src/main/java/com/ultikits/ultitools/interfaces/impl/data/sqlite/SQLiteDataOperator.java b/src/main/java/com/ultikits/ultitools/interfaces/impl/data/sqlite/SQLiteDataOperator.java index 4e9d72414..0891a488a 100644 --- a/src/main/java/com/ultikits/ultitools/interfaces/impl/data/sqlite/SQLiteDataOperator.java +++ b/src/main/java/com/ultikits/ultitools/interfaces/impl/data/sqlite/SQLiteDataOperator.java @@ -27,6 +27,9 @@ public class SQLiteDataOperator> extends Abstra */ public SQLiteDataOperator(DataSource dataSource, Class type) { super(dataSource, type); + // SQLite's DDL accepted a NULL id, and UltiTools-API 6.2.0 inserted rows without one + // (#546): give them ids now. _rowid_ is SQLite's built-in row identifier. + backfillNullIds("_rowid_"); } @Override diff --git a/src/main/java/com/ultikits/ultitools/manager/DataStoreManager.java b/src/main/java/com/ultikits/ultitools/manager/DataStoreManager.java index f424879e3..2ac001288 100644 --- a/src/main/java/com/ultikits/ultitools/manager/DataStoreManager.java +++ b/src/main/java/com/ultikits/ultitools/manager/DataStoreManager.java @@ -6,8 +6,8 @@ import org.bukkit.Bukkit; import java.io.File; -import java.util.HashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; import org.jetbrains.annotations.ApiStatus; @@ -17,7 +17,12 @@ */ @ApiStatus.Internal public class DataStoreManager { - private static final Map dataMap = new HashMap<>(); + /** + * Registered stores by type. A {@link ConcurrentHashMap} because {@link #getDatastore(String)} + * reads it without the writers' lock (#515): a plain {@code HashMap} read during a writer's + * resize could miss a present key, and nothing ordered the read after the write. + */ + private static final Map dataMap = new ConcurrentHashMap<>(); /** * Register data store. @@ -56,13 +61,19 @@ public static void close() { * @return Data store */ public static DataStore getDatastore(String type) { - if (type == null) { - type = "json"; + String key = type == null ? "json" : type; + // Each key is read once and the value read is the value returned (#515). The previous + // version read the map up to three times per call, so a type registered when the call + // started could be unregistered between the null check and the return, and the call + // returned null instead of either that store or the json fallback. + DataStore store = dataMap.get(key); + if (store != null) { + return store; } - if ("json".equals(type) && dataMap.get(type) == null) { + if ("json".equals(key)) { return new JsonStore(UltiTools.getInstance().getDataFolder().getAbsolutePath() + File.separator + "data"); } - return dataMap.get(type) == null ? dataMap.get("json") : dataMap.get(type); + return dataMap.get("json"); } /** diff --git a/src/main/java/com/ultikits/ultitools/utils/BeanCopyUtil.java b/src/main/java/com/ultikits/ultitools/utils/BeanCopyUtil.java index a2fffedb7..ddc7c5583 100644 --- a/src/main/java/com/ultikits/ultitools/utils/BeanCopyUtil.java +++ b/src/main/java/com/ultikits/ultitools/utils/BeanCopyUtil.java @@ -2,7 +2,10 @@ import java.lang.reflect.Field; import java.lang.reflect.Modifier; +import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; /** * Bean copy utility class. @@ -14,6 +17,22 @@ */ @SuppressWarnings("PMD.AvoidAccessibilityAlteration") // Copies private fields between bean instances -- see 08-GATE05-TRIAGE.md public final class BeanCopyUtil { + + /** Primitive type to its wrapper class; see {@link #boxed(Class)}. */ + private static final Map, Class> PRIMITIVE_TO_WRAPPER; + + static { + Map, Class> map = new HashMap<>(); + map.put(boolean.class, Boolean.class); + map.put(char.class, Character.class); + map.put(byte.class, Byte.class); + map.put(short.class, Short.class); + map.put(int.class, Integer.class); + map.put(long.class, Long.class); + map.put(float.class, Float.class); + map.put(double.class, Double.class); + PRIMITIVE_TO_WRAPPER = Collections.unmodifiableMap(map); + } private BeanCopyUtil() { throw new UnsupportedOperationException("Utility class"); @@ -65,7 +84,7 @@ public static void copyProperties(Object source, Object target, boolean ignoreNu } java.util.Set ignoreSet = ignoreFields == null ? - java.util.Collections.emptySet() : + Collections.emptySet() : new java.util.HashSet<>(java.util.Arrays.asList(ignoreFields)); List sourceFields = ReflectionUtil.getAllFields(source.getClass()); @@ -108,7 +127,7 @@ private static void copyFieldValue(Object source, Object target, Field sourceFie return; } - if (value != null && !targetField.getType().isAssignableFrom(value.getClass())) { + if (value != null && !boxed(targetField.getType()).isAssignableFrom(value.getClass())) { value = convertValue(value, targetField.getType()); if (value == null) { return; @@ -147,7 +166,7 @@ private static Object convertValue(Object value, Class targetType) { return null; } - if (targetType.isAssignableFrom(value.getClass())) { + if (boxed(targetType).isAssignableFrom(value.getClass())) { return value; } @@ -162,6 +181,20 @@ private static Object convertValue(Object value, Class targetType) { return null; } + /** + * Returns the wrapper class of a primitive type, or the type itself otherwise. + *

+ * {@link Field#get} always boxes, so a {@code boolean} field reads as a {@code Boolean} and + * {@code boolean.class.isAssignableFrom(Boolean.class)} is {@code false}. Deciding + * assignability against the primitive type sent every {@code boolean} and {@code char} value + * to {@link #convertValue}, which had no branch for either and dropped the write (#520). The + * numeric primitives only survived because {@link #convertNumber} happens to cover them. + */ + private static Class boxed(Class type) { + Class wrapper = PRIMITIVE_TO_WRAPPER.get(type); + return wrapper != null ? wrapper : type; + } + private static Object convertNumber(Number num, Class targetType) { if (targetType == Integer.class || targetType == int.class) { return num.intValue(); diff --git a/src/test/java/com/ultikits/ultitools/interfaces/impl/data/ConditionalUpdateTest.java b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/ConditionalUpdateTest.java new file mode 100644 index 000000000..3e019e216 --- /dev/null +++ b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/ConditionalUpdateTest.java @@ -0,0 +1,321 @@ +package com.ultikits.ultitools.interfaces.impl.data; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.nio.file.Path; +import java.sql.Connection; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Supplier; +import java.util.logging.Logger; + +import javax.sql.DataSource; + +import org.bukkit.Bukkit; +import org.bukkit.Server; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import com.ultikits.ultitools.abstracts.data.BaseDataEntity; +import com.ultikits.ultitools.annotations.Column; +import com.ultikits.ultitools.annotations.Table; +import com.ultikits.ultitools.entities.Comparison; +import com.ultikits.ultitools.entities.WhereCondition; +import com.ultikits.ultitools.exceptions.DataAccessException; +import com.ultikits.ultitools.interfaces.DataOperator; +import com.ultikits.ultitools.interfaces.impl.data.json.SimpleJsonDataOperator; +import com.ultikits.ultitools.interfaces.impl.data.mysql.MysqlDataOperator; +import com.ultikits.ultitools.interfaces.impl.data.sqlite.SQLiteDataOperator; +import com.zaxxer.hikari.HikariConfig; +import com.zaxxer.hikari.HikariDataSource; + +/** + * {@code DataOperator#updateIf}: a write that applies only while the stored row still matches the + * caller's expected conditions, and reports whether it applied (#543). UltiEconomy's wallet merge + * conditions each account write on the balance it read and re-reads when the write does not apply + * (maintainer decision 2026-09-29), so the scenario below is that one, on the JSON, SQLite and MySQL + * operators (H2 in MySQL mode standing in for both relational engines). + */ +@DisplayName("DataOperator#updateIf applies only while the expected values still hold (#543)") +class ConditionalUpdateTest { + + private static DataSource dataSource; + + @TempDir + Path tempDir; + + @Table("conditional_account") + public static class Account extends BaseDataEntity { + private static final long serialVersionUID = 1L; + + @Column("owner") + private String owner; + + @Column(value = "balance", type = "DOUBLE") + private double balance; + + public Account() { + } + + public Account(String owner, double balance) { + this.owner = owner; + this.balance = balance; + } + + public String getOwner() { + return owner; + } + + public void setOwner(String owner) { + this.owner = owner; + } + + public double getBalance() { + return balance; + } + + public void setBalance(double balance) { + this.balance = balance; + } + } + + private static final class Backend { + private final String label; + private final Supplier> factory; + + private Backend(String label, Supplier> factory) { + this.label = label; + this.factory = factory; + } + } + + @BeforeAll + static void initDataSource() { + if (Bukkit.getServer() == null) { + Server mockServer = mock(Server.class); + Logger mockLogger = mock(Logger.class); + when(mockServer.getLogger()).thenReturn(mockLogger); + Bukkit.setServer(mockServer); + } + HikariConfig config = new HikariConfig(); + config.setJdbcUrl("jdbc:h2:mem:conditionalupdate;DB_CLOSE_DELAY=-1;MODE=MySQL"); + config.setUsername("sa"); + // No setPassword: the in-memory database is created without one on first connect. + dataSource = new HikariDataSource(config); + } + + @BeforeEach + void dropTable() throws Exception { + try (Connection conn = dataSource.getConnection(); + Statement stmt = conn.createStatement()) { + stmt.execute("DROP TABLE IF EXISTS conditional_account"); + } + } + + private List backends() { + List backends = new ArrayList<>(); + String jsonDir = tempDir.toFile().getAbsolutePath(); + backends.add(new Backend("json", () -> new SimpleJsonDataOperator<>(jsonDir, Account.class))); + backends.add(new Backend("sqlite", () -> new SQLiteDataOperator<>(dataSource, Account.class))); + backends.add(new Backend("mysql", () -> new MysqlDataOperator<>(dataSource, Account.class))); + return backends; + } + + private static WhereCondition balanceIs(double balance) { + return WhereCondition.builder().column("balance").value(balance).build(); + } + + private void resetTable() throws Exception { + dropTable(); + } + + @Nested + @DisplayName("on every backend") + class OnEveryBackend { + + @Test + @DisplayName("applies and returns true while the stored balance still equals the one read") + void appliesWhenExpectedHolds() throws Exception { + for (Backend backend : backends()) { + resetTable(); + DataOperator operator = backend.factory.get(); + Account account = new Account("alice", 100.0); + operator.insert(account); + + Account read = operator.getById(account.getId()); + read.setBalance(read.getBalance() + 50.0); + + assertThat(operator.updateIf(read, balanceIs(100.0))).as(backend.label).isTrue(); + assertThat(operator.getById(account.getId()).getBalance()).as(backend.label).isEqualTo(150.0); + } + } + + @Test + @DisplayName("returns false and writes nothing once the stored balance has changed") + void refusesWhenExpectedNoLongerHolds() throws Exception { + for (Backend backend : backends()) { + resetTable(); + DataOperator operator = backend.factory.get(); + Account account = new Account("alice", 100.0); + operator.insert(account); + + Account stale = operator.getById(account.getId()); + Account current = operator.getById(account.getId()); + current.setBalance(120.0); + operator.update(current); + + stale.setBalance(stale.getBalance() + 50.0); + stale.setOwner("overwritten"); + assertThat(operator.updateIf(stale, balanceIs(100.0))).as(backend.label).isFalse(); + + Account stored = operator.getById(account.getId()); + assertThat(stored.getBalance()).as(backend.label).isEqualTo(120.0); + assertThat(stored.getOwner()).as(backend.label).isEqualTo("alice"); + } + } + + @Test + @DisplayName("returns false for an id no row has") + void falseForMissingRow() throws Exception { + for (Backend backend : backends()) { + resetTable(); + DataOperator operator = backend.factory.get(); + Account ghost = new Account("ghost", 1.0); + ghost.setId("no-such-id"); + + assertThat(operator.updateIf(ghost, balanceIs(1.0))).as(backend.label).isFalse(); + assertThat(operator.getById("no-such-id")).as(backend.label).isNull(); + } + } + + @Test + @DisplayName("throws DataAccessException for a null id") + void throwsForNullId() throws Exception { + for (Backend backend : backends()) { + resetTable(); + DataOperator operator = backend.factory.get(); + + assertThatThrownBy(() -> operator.updateIf(new Account("no-id", 1.0), balanceIs(1.0))) + .as(backend.label) + .isInstanceOf(DataAccessException.class); + } + } + + @Test + @DisplayName("every expected condition must hold, compared with the same semantics as getAll") + void everyConditionMustHold() throws Exception { + for (Backend backend : backends()) { + resetTable(); + DataOperator operator = backend.factory.get(); + Account account = new Account("O'Brien", 100.0); + operator.insert(account); + Account read = operator.getById(account.getId()); + read.setBalance(90.0); + + WhereCondition ownerMatches = WhereCondition.builder().column("owner").value("O'Brien").build(); + WhereCondition balanceAbove = WhereCondition.builder().column("balance").value(200.0) + .comparison(Comparison.GREATER).build(); + assertThat(operator.updateIf(read, ownerMatches, balanceAbove)) + .as("%s: applied although one of two conditions failed", backend.label).isFalse(); + assertThat(operator.updateIf(read, ownerMatches, balanceIs(100.0))) + .as("%s: a value containing a quote did not match", backend.label).isTrue(); + assertThat(operator.getById(account.getId()).getBalance()).as(backend.label).isEqualTo(90.0); + } + } + } + + @Nested + @DisplayName("conditions it cannot evaluate are refused on every backend") + class RefusedConditions { + + @Test + @DisplayName("a column the entity does not map throws DataAccessException instead of returning false") + void unknownColumnThrows() throws Exception { + for (Backend backend : backends()) { + resetTable(); + DataOperator operator = backend.factory.get(); + Account account = new Account("alice", 100.0); + operator.insert(account); + Account read = operator.getById(account.getId()); + + assertThatThrownBy(() -> operator.updateIf(read, + WhereCondition.builder().column("balanc").value(100.0).build())) + .as("%s: a misspelt column would make a compare-and-set loop retry forever", backend.label) + .isInstanceOf(DataAccessException.class); + } + } + + @Test + @DisplayName("a null expected value throws DataAccessException, since no backend can compare it") + void nullExpectedValueThrows() throws Exception { + for (Backend backend : backends()) { + resetTable(); + DataOperator operator = backend.factory.get(); + Account account = new Account(null, 100.0); + operator.insert(account); + Account read = operator.getById(account.getId()); + read.setOwner("claimed"); + + assertThatThrownBy(() -> operator.updateIf(read, + WhereCondition.builder().column("owner").value(null).build())) + .as(backend.label) + .isInstanceOf(DataAccessException.class); + assertThat(operator.getById(account.getId()).getOwner()).as(backend.label).isNull(); + } + } + } + + @Nested + @DisplayName("two operators over one database") + class TwoWriters { + + @Test + @DisplayName("the second writer's update, conditioned on the balance it read before the first wrote, does not apply") + void secondWriterLoses() { + SQLiteDataOperator serverA = new SQLiteDataOperator<>(dataSource, Account.class); + SQLiteDataOperator serverB = new SQLiteDataOperator<>(dataSource, Account.class); + Account account = new Account("alice", 100.0); + serverA.insert(account); + + Account readByA = serverA.getById(account.getId()); + Account readByB = serverB.getById(account.getId()); + + readByA.setBalance(readByA.getBalance() + 50.0); + assertThat(serverA.updateIf(readByA, balanceIs(100.0))).isTrue(); + + readByB.setBalance(readByB.getBalance() + 30.0); + assertThat(serverB.updateIf(readByB, balanceIs(100.0))) + .as("the stalled writer applied on top of the other server's write") + .isFalse(); + + assertThat(serverA.getById(account.getId()).getBalance()).isEqualTo(150.0); + } + } + + @Nested + @DisplayName("an implementation that does not provide it") + class NotImplemented { + + @Test + @DisplayName("the default throws UnsupportedOperationException naming the implementing class") + void defaultThrowsNamingTheClass() { + @SuppressWarnings("unchecked") + DataOperator foreign = mock(DataOperator.class, CALLS_REAL_METHODS); + Account account = new Account("alice", 1.0); + account.setId("id-1"); + + assertThatThrownBy(() -> foreign.updateIf(account, balanceIs(1.0))) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessageContaining(foreign.getClass().getName()); + } + } +} diff --git a/src/test/java/com/ultikits/ultitools/interfaces/impl/data/DetachedReadParityTest.java b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/DetachedReadParityTest.java new file mode 100644 index 000000000..74f61c513 --- /dev/null +++ b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/DetachedReadParityTest.java @@ -0,0 +1,234 @@ +package com.ultikits.ultitools.interfaces.impl.data; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.nio.file.Path; +import java.sql.Connection; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; +import java.util.function.Function; +import java.util.logging.Logger; + +import javax.sql.DataSource; + +import org.bukkit.Bukkit; +import org.bukkit.Server; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import com.ultikits.ultitools.abstracts.data.BaseDataEntity; +import com.ultikits.ultitools.annotations.Column; +import com.ultikits.ultitools.annotations.Table; +import com.ultikits.ultitools.entities.WhereCondition; +import com.ultikits.ultitools.interfaces.DataOperator; +import com.ultikits.ultitools.interfaces.DataOperator.LikeType; +import com.ultikits.ultitools.interfaces.impl.data.json.SimpleJsonDataOperator; +import com.ultikits.ultitools.interfaces.impl.data.sqlite.SQLiteDataOperator; +import com.zaxxer.hikari.HikariConfig; +import com.zaxxer.hikari.HikariDataSource; + +/** + * Every read path hands out a detached copy on every backend (#522): changing a returned entity + * without calling {@code update(...)} changes nothing stored, and calling {@code update(...)} + * does. The JSON store used to hand out the instances it caches, so the same module code had + * different persistence semantics on {@code datasource.type: json} and on SQLite/MySQL. + *

+ * Each test runs the same scenario on the JSON backend and on the relational backend (H2 in + * MySQL mode standing in for SQLite, as in {@code BackendIdContractTest}), so a divergence + * between the two is what fails. + */ +@DisplayName("Read paths return detached copies on every backend (#522)") +class DetachedReadParityTest { + + private static DataSource dataSource; + + @TempDir + Path tempDir; + + private final List backends = new ArrayList<>(); + + @Table("detached_read_entity") + public static class ReadEntity extends BaseDataEntity { + private static final long serialVersionUID = 1L; + + @Column("name") + private String name; + + public ReadEntity() { + } + + public ReadEntity(String name) { + this.name = name; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + // Equality over a mutable field as well as the id, the shape Lombok's + // @EqualsAndHashCode(callSuper = true) gives most module entities. + @Override + public boolean equals(Object other) { + if (!(other instanceof ReadEntity)) { + return false; + } + ReadEntity that = (ReadEntity) other; + return Objects.equals(getId(), that.getId()) && Objects.equals(name, that.name); + } + + @Override + public int hashCode() { + return Objects.hash(getId(), name); + } + } + + private static final class Backend { + private final String label; + private final DataOperator operator; + + private Backend(String label, DataOperator operator) { + this.label = label; + this.operator = operator; + } + } + + @BeforeAll + static void initDataSource() { + if (Bukkit.getServer() == null) { + Server mockServer = mock(Server.class); + Logger mockLogger = mock(Logger.class); + when(mockServer.getLogger()).thenReturn(mockLogger); + Bukkit.setServer(mockServer); + } + HikariConfig config = new HikariConfig(); + config.setJdbcUrl("jdbc:h2:mem:detachedread;DB_CLOSE_DELAY=-1;MODE=MySQL"); + config.setUsername("sa"); + // No setPassword: the in-memory database is created without one on first connect. + dataSource = new HikariDataSource(config); + } + + @BeforeEach + void setUp() throws Exception { + try (Connection conn = dataSource.getConnection(); + Statement stmt = conn.createStatement()) { + stmt.execute("DROP TABLE IF EXISTS detached_read_entity"); + } + backends.clear(); + backends.add(new Backend("json", + new SimpleJsonDataOperator<>(tempDir.toFile().getAbsolutePath(), ReadEntity.class))); + backends.add(new Backend("sqlite", new SQLiteDataOperator<>(dataSource, ReadEntity.class))); + } + + /** + * Stores one entity named "stored" on the backend, reads it back through {@code read}, + * renames the returned instance without calling update, and returns what a fresh + * {@code getById} then reports. + */ + private String nameAfterMutatingWithoutUpdate(Backend backend, Function, ReadEntity> read) { + ReadEntity entity = new ReadEntity("stored"); + backend.operator.insert(entity); + String id = entity.getId(); + ReadEntity returned = read.apply(backend.operator); + assertThat(returned).as("%s: the read path returned nothing", backend.label).isNotNull(); + returned.setName("mutated-without-update"); + return backend.operator.getById(id).getName(); + } + + private void assertEveryBackendKeepsTheStoredValue(String path, Function, ReadEntity> read) { + for (Backend backend : backends) { + assertThat(nameAfterMutatingWithoutUpdate(backend, read)) + .as("%s: mutating an entity returned by %s changed the store without update()", backend.label, path) + .isEqualTo("stored"); + } + } + + @Test + @DisplayName("getAll() hands out a detached copy") + void getAllReturnsDetachedCopies() { + assertEveryBackendKeepsTheStoredValue("getAll()", op -> op.getAll().get(0)); + } + + @Test + @DisplayName("getAll(conditions) hands out a detached copy") + void getAllWithConditionsReturnsDetachedCopies() { + assertEveryBackendKeepsTheStoredValue("getAll(conditions)", + op -> op.getAll(WhereCondition.builder().column("name").value("stored").build()).get(0)); + } + + @Test + @DisplayName("getById hands out a detached copy") + void getByIdReturnsDetachedCopy() { + assertEveryBackendKeepsTheStoredValue("getById", op -> op.getById(op.getAll().get(0).getId())); + } + + @Test + @DisplayName("page hands out a detached copy") + void pageReturnsDetachedCopies() { + assertEveryBackendKeepsTheStoredValue("page", + op -> op.page(1, 10, WhereCondition.builder().column("name").value("stored").build()).get(0)); + } + + @Test + @DisplayName("getLike hands out a detached copy") + void getLikeReturnsDetachedCopies() { + assertEveryBackendKeepsTheStoredValue("getLike", op -> op.getLike("name", "sto", LikeType.START).get(0)); + } + + @Test + @DisplayName("query().first() hands out a detached copy") + void queryFirstReturnsDetachedCopy() { + assertEveryBackendKeepsTheStoredValue("query().first()", op -> op.query().where("name").eq("stored").first()); + } + + @Test + @DisplayName("the entity passed to insert is not the stored instance either") + void insertedInstanceIsDetached() { + for (Backend backend : backends) { + ReadEntity entity = new ReadEntity("stored"); + backend.operator.insert(entity); + entity.setName("mutated-after-insert"); + assertThat(backend.operator.getById(entity.getId()).getName()) + .as("%s: mutating the inserted instance changed the store without update()", backend.label) + .isEqualTo("stored"); + } + } + + @Test + @DisplayName("exist(entity) finds the stored row by its id after the entity changed without update()") + void existMatchesByIdAfterALocalChange() { + for (Backend backend : backends) { + ReadEntity entity = new ReadEntity("stored"); + backend.operator.insert(entity); + entity.setName("changed-locally"); + assertThat(backend.operator.exist(entity)) + .as("%s: exist(entity) missed a stored row because a field changed locally", backend.label) + .isTrue(); + } + } + + @Test + @DisplayName("update(T) on a returned copy persists the change") + void updatePersistsTheChange() throws Exception { + for (Backend backend : backends) { + ReadEntity entity = new ReadEntity("stored"); + backend.operator.insert(entity); + ReadEntity copy = backend.operator.getById(entity.getId()); + copy.setName("updated"); + backend.operator.update(copy); + assertThat(backend.operator.getById(entity.getId()).getName()) + .as("%s: update(T) did not persist the change", backend.label) + .isEqualTo("updated"); + } + } +} diff --git a/src/test/java/com/ultikits/ultitools/interfaces/impl/data/MissingRowUpdateTest.java b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/MissingRowUpdateTest.java new file mode 100644 index 000000000..e6b3da999 --- /dev/null +++ b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/MissingRowUpdateTest.java @@ -0,0 +1,291 @@ +package com.ultikits.ultitools.interfaces.impl.data; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.nio.file.Path; +import java.sql.Connection; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.function.Supplier; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + +import javax.sql.DataSource; + +import org.bukkit.Bukkit; +import org.bukkit.Server; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import com.ultikits.ultitools.abstracts.data.BaseDataEntity; +import com.ultikits.ultitools.annotations.Column; +import com.ultikits.ultitools.annotations.Table; +import com.ultikits.ultitools.entities.WhereCondition; +import com.ultikits.ultitools.exceptions.DataAccessException; +import com.ultikits.ultitools.interfaces.DataOperator; +import com.ultikits.ultitools.interfaces.impl.data.json.SimpleJsonDataOperator; +import com.ultikits.ultitools.interfaces.impl.data.mysql.MysqlDataOperator; +import com.ultikits.ultitools.interfaces.impl.data.sqlite.SQLiteDataOperator; +import com.zaxxer.hikari.HikariConfig; +import com.zaxxer.hikari.HikariDataSource; + +/** + * An update by a non-null id that matches no row writes nothing and logs one WARNING naming the + * table and the id, every time, on JSON, SQLite and MySQL (#558, maintainer 2026-09-29: + * 「不写，并告诉调用方没写成」). Before, the relational backends returned silently and the JSON + * backend threw a raw {@code NullPointerException}. + */ +@DisplayName("An update by an id no row has writes nothing and warns, on every backend (#558)") +class MissingRowUpdateTest { + + private static final String TABLE = "missing_row_entity"; + private static final String MISSING_ID = "no-such-id"; + + private static DataSource dataSource; + + @TempDir + Path tempDir; + + private final List warnings = new ArrayList<>(); + private final Handler capture = new Handler() { + @Override + public void publish(LogRecord logRecord) { + if (Level.WARNING.equals(logRecord.getLevel())) { + warnings.add(logRecord); + } + } + + @Override + public void flush() { + // nothing buffered + } + + @Override + public void close() { + // nothing to release + } + }; + private final List loggers = Arrays.asList( + Logger.getLogger(AbstractRelationalDataOperator.class.getName()), + Logger.getLogger(SimpleJsonDataOperator.class.getName())); + + @Table(TABLE) + public static class Row extends BaseDataEntity { + private static final long serialVersionUID = 1L; + + @Column("name") + private String name; + + public Row() { + } + + public Row(String id, String name) { + setId(id); + this.name = name; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + } + + static final class Backend { + final String label; + final Supplier> factory; + + Backend(String label, Supplier> factory) { + this.label = label; + this.factory = factory; + } + } + + @BeforeAll + static void initDataSource() { + if (Bukkit.getServer() == null) { + Server mockServer = mock(Server.class); + Logger mockLogger = mock(Logger.class); + when(mockServer.getLogger()).thenReturn(mockLogger); + Bukkit.setServer(mockServer); + } + HikariConfig config = new HikariConfig(); + config.setJdbcUrl("jdbc:h2:mem:missingrowupdate;DB_CLOSE_DELAY=-1;MODE=MySQL"); + config.setUsername("sa"); + // No setPassword: the in-memory database is created without one on first connect. + dataSource = new HikariDataSource(config); + } + + @BeforeEach + void setUp() { + for (Logger logger : loggers) { + logger.addHandler(capture); + } + } + + @AfterEach + void tearDown() { + for (Logger logger : loggers) { + logger.removeHandler(capture); + } + } + + private static void dropTable() throws Exception { + try (Connection conn = dataSource.getConnection(); + Statement stmt = conn.createStatement()) { + stmt.execute("DROP TABLE IF EXISTS " + TABLE); + } + } + + List backends() { + String jsonDir = tempDir.resolve(TABLE).toFile().getAbsolutePath(); + return Arrays.asList( + new Backend("json", () -> new SimpleJsonDataOperator<>(jsonDir, Row.class)), + new Backend("sqlite", () -> new SQLiteDataOperator<>(dataSource, Row.class)), + new Backend("mysql", () -> new MysqlDataOperator<>(dataSource, Row.class))); + } + + DataOperator fresh(Backend backend) throws Exception { + dropTable(); + warnings.clear(); + DataOperator operator = backend.factory.get(); + for (Row row : operator.getAll()) { + operator.delById(row.getId()); + } + operator.insert(new Row("present", "stored")); + warnings.clear(); + return operator; + } + + private List warningsNaming(String id) { + List lines = new ArrayList<>(); + for (LogRecord logRecord : warnings) { + String message = logRecord.getMessage(); + if (message != null && message.contains(TABLE) && message.contains("'" + id + "'")) { + lines.add(message); + } + } + return lines; + } + + @Test + @DisplayName("update(T): no exception, nothing written, one WARNING per call") + void updateEntity() throws Exception { + for (Backend backend : backends()) { + DataOperator operator = fresh(backend); + Row missing = new Row(MISSING_ID, "ghost"); + + assertThatCode(() -> operator.update(missing)).as(backend.label).doesNotThrowAnyException(); + assertThatCode(() -> operator.update(missing)).as(backend.label).doesNotThrowAnyException(); + + assertThat(operator.getById(MISSING_ID)).as("%s: the update created a row", backend.label).isNull(); + assertThat(operator.getAll()).as(backend.label).hasSize(1); + assertThat(warningsNaming(MISSING_ID)).as("%s: one WARNING per call", backend.label).hasSize(2); + } + } + + @Test + @DisplayName("update(column, value, id): no exception, nothing written, one WARNING per call") + void updateColumn() throws Exception { + for (Backend backend : backends()) { + DataOperator operator = fresh(backend); + + assertThatCode(() -> operator.update("name", "ghost", MISSING_ID)) + .as(backend.label).doesNotThrowAnyException(); + + assertThat(operator.getById(MISSING_ID)).as(backend.label).isNull(); + assertThat(operator.getAll()).as(backend.label).hasSize(1); + assertThat(warningsNaming(MISSING_ID)).as(backend.label).hasSize(1); + } + } + + @Test + @DisplayName("updateAll: the present row is written, the missing one warns once") + void updateAll() throws Exception { + for (Backend backend : backends()) { + DataOperator operator = fresh(backend); + Row present = operator.getById("present"); + present.setName("renamed"); + + assertThatCode(() -> operator.updateAll(Arrays.asList(present, new Row(MISSING_ID, "ghost")))) + .as(backend.label).doesNotThrowAnyException(); + + assertThat(operator.getById("present").getName()).as(backend.label).isEqualTo("renamed"); + assertThat(operator.getById(MISSING_ID)).as(backend.label).isNull(); + assertThat(warningsNaming(MISSING_ID)).as(backend.label).hasSize(1); + assertThat(warningsNaming("present")).as("%s: a matched row warned", backend.label).isEmpty(); + } + } + + @Test + @DisplayName("updateCounted tells the caller: 1 and written for a stored row, 0 and nothing written (one WARNING) for a missing one") + void updateCountedReportsTheWrite() throws Exception { + for (Backend backend : backends()) { + DataOperator operator = fresh(backend); + Row present = operator.getById("present"); + present.setName("renamed"); + + assertThat(operator.updateCounted(present)).as(backend.label).isEqualTo(1); + assertThat(operator.getById("present").getName()).as(backend.label).isEqualTo("renamed"); + + assertThat(operator.updateCounted(new Row(MISSING_ID, "ghost"))).as(backend.label).isZero(); + assertThat(operator.getById(MISSING_ID)).as(backend.label).isNull(); + assertThat(warningsNaming(MISSING_ID)).as(backend.label).hasSize(1); + } + } + + @Test + @DisplayName("updateCounted with a null id throws DataAccessException, as update does") + void updateCountedRefusesNullId() throws Exception { + for (Backend backend : backends()) { + DataOperator operator = fresh(backend); + assertThatThrownBy(() -> operator.updateCounted(new Row(null, "no-id"))) + .as(backend.label).isInstanceOf(DataAccessException.class); + } + } + + @Test + @DisplayName("a third-party operator: counted by whether the row exists before its update") + void defaultCountsByExistence() throws Exception { + @SuppressWarnings("unchecked") + DataOperator foreign = mock(DataOperator.class, CALLS_REAL_METHODS); + doNothing().when(foreign).update(any(Row.class)); + Row row = new Row("row-1", "x"); + + doReturn(false).when(foreign).exist(any(WhereCondition[].class)); + assertThat(foreign.updateCounted(row)).isZero(); + verify(foreign, never()).update(any(Row.class)); + + doReturn(true).when(foreign).exist(any(WhereCondition[].class)); + assertThat(foreign.updateCounted(row)).isEqualTo(1); + verify(foreign, times(1)).update(row); + + assertThatThrownBy(() -> foreign.updateCounted(new Row(null, "no-id"))) + .isInstanceOf(DataAccessException.class); + + doThrow(new IllegalAccessException("field")).when(foreign).update(any(Row.class)); + assertThatThrownBy(() -> foreign.updateCounted(row)).isInstanceOf(DataAccessException.class); + } +} diff --git a/src/test/java/com/ultikits/ultitools/interfaces/impl/data/NullIdRowsTest.java b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/NullIdRowsTest.java new file mode 100644 index 000000000..da9f8320c --- /dev/null +++ b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/NullIdRowsTest.java @@ -0,0 +1,493 @@ +package com.ultikits.ultitools.interfaces.impl.data; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.nio.file.Path; +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + +import javax.sql.DataSource; + +import org.bukkit.Bukkit; +import org.bukkit.Server; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import com.ultikits.ultitools.abstracts.data.BaseDataEntity; +import com.ultikits.ultitools.annotations.Column; +import com.ultikits.ultitools.annotations.Table; +import com.ultikits.ultitools.exceptions.DataAccessException; +import com.ultikits.ultitools.interfaces.DataOperator; +import com.ultikits.ultitools.interfaces.impl.data.json.SimpleJsonDataOperator; +import com.ultikits.ultitools.interfaces.impl.data.mysql.MysqlDataOperator; +import com.ultikits.ultitools.interfaces.impl.data.sqlite.SQLiteDataOperator; +import com.zaxxer.hikari.HikariConfig; +import com.zaxxer.hikari.HikariDataSource; + +/** + * Rows left with a NULL id by UltiTools-API 6.2.0 on SQLite (#546; maintainer decision + * 2026-09-27): table initialisation backfills them with a UUID, writing only the id column and + * logging one line per table, and update/delete by a null id throw instead of matching nothing. + *

+ * H2 stands in for SQLite as elsewhere in this package. H2 refuses a NULL in a PRIMARY KEY column, + * so the legacy table is created here without one; SQLite's generated DDL + * ({@code PRIMARY KEY (`id`)} with no {@code NOT NULL}) accepted NULL, which is how the rows + * exist. The backfill keys rows by {@code _rowid_}, which both engines provide. + */ +@DisplayName("NULL-id rows: backfill at table init, refuse null-id update/delete (#546)") +class NullIdRowsTest { + + private static DataSource dataSource; + private static final Logger OPERATOR_LOGGER = Logger.getLogger(AbstractRelationalDataOperator.class.getName()); + + @TempDir + Path tempDir; + + private final List logged = new ArrayList<>(); + private final Handler capture = new Handler() { + @Override + public void publish(LogRecord logRecord) { + logged.add(logRecord); + } + + @Override + public void flush() { + // nothing buffered + } + + @Override + public void close() { + // nothing to release + } + }; + + @Table("null_id_entity") + public static class LegacyEntity extends BaseDataEntity { + private static final long serialVersionUID = 1L; + + @Column("name") + private String name; + + @Column(value = "score", type = "INT") + private int score; + + public LegacyEntity() { + } + + public LegacyEntity(String name, int score) { + this.name = name; + this.score = score; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + } + + @BeforeAll + static void initDataSource() { + if (Bukkit.getServer() == null) { + Server mockServer = mock(Server.class); + Logger mockLogger = mock(Logger.class); + when(mockServer.getLogger()).thenReturn(mockLogger); + Bukkit.setServer(mockServer); + } + HikariConfig config = new HikariConfig(); + config.setJdbcUrl("jdbc:h2:mem:nullidrows;DB_CLOSE_DELAY=-1;MODE=MySQL"); + config.setUsername("sa"); + // No setPassword: the in-memory database is created without one on first connect. + dataSource = new HikariDataSource(config); + } + + @BeforeEach + void setUp() throws Exception { + execute("DROP TABLE IF EXISTS null_id_entity"); + OPERATOR_LOGGER.addHandler(capture); + } + + @AfterEach + void tearDown() { + OPERATOR_LOGGER.removeHandler(capture); + } + + private static void execute(String sql) throws Exception { + try (Connection conn = dataSource.getConnection(); + Statement stmt = conn.createStatement()) { + // Every caller passes a string literal from this class; nothing reaches it from input. + // nosemgrep: java_inject_rule-SqlInjection + stmt.execute(sql); + } + } + + /** The table as 6.2.0 left it: three NULL-id rows and one row that has an id. */ + private static void createLegacyTable() throws Exception { + execute("CREATE TABLE null_id_entity (`id` VARCHAR(255), `name` VARCHAR(255), `score` INT)"); + execute("INSERT INTO null_id_entity (`id`, `name`, `score`) VALUES " + + "(NULL, 'world', 1), (NULL, 'world_nether', 2), (NULL, 'world_the_end', 3), ('kept-id', 'kept', 4)"); + } + + /** Every row as "rowid|id|name|score", ordered by rowid. */ + private static List rows() throws Exception { + List rows = new ArrayList<>(); + try (Connection conn = dataSource.getConnection(); + Statement stmt = conn.createStatement(); + ResultSet rs = stmt.executeQuery( + "SELECT _rowid_, `id`, `name`, `score` FROM null_id_entity ORDER BY _rowid_")) { + while (rs.next()) { + rows.add(rs.getLong(1) + "|" + rs.getString(2) + "|" + rs.getString(3) + "|" + rs.getInt(4)); + } + } + return rows; + } + + private static List withoutIds(List rows) { + List stripped = new ArrayList<>(); + for (String row : rows) { + String[] parts = row.split("\\|", -1); + stripped.add(parts[0] + "|" + parts[2] + "|" + parts[3]); + } + return stripped; + } + + private List backfillLinesFor(String table) { + List lines = new ArrayList<>(); + for (LogRecord logRecord : logged) { + if (logRecord.getMessage() != null && logRecord.getMessage().contains("'" + table + "'")) { + lines.add(logRecord.getMessage()); + } + } + return lines; + } + + private List warningsFor(String table) { + List lines = new ArrayList<>(); + for (LogRecord logRecord : logged) { + if (Level.WARNING.equals(logRecord.getLevel()) && logRecord.getMessage() != null + && logRecord.getMessage().contains("'" + table + "'")) { + lines.add(logRecord.getMessage()); + } + } + return lines; + } + + private List backfillLines() { + List lines = new ArrayList<>(); + for (LogRecord logRecord : logged) { + if (logRecord.getMessage() != null && logRecord.getMessage().contains("null_id_entity")) { + lines.add(logRecord.getMessage()); + } + } + return lines; + } + + @Nested + @DisplayName("backfill at table initialisation") + class Backfill { + + @Test + @DisplayName("every NULL-id row gets a distinct UUID and nothing else in the row changes") + void nullIdRowsGetDistinctIdsAndOtherColumnsStayIdentical() throws Exception { + createLegacyTable(); + List before = rows(); + + new SQLiteDataOperator<>(dataSource, LegacyEntity.class); + + List after = rows(); + assertThat(withoutIds(after)).as("a column other than id changed").isEqualTo(withoutIds(before)); + List ids = new ArrayList<>(); + for (String row : after) { + ids.add(row.split("\\|", -1)[1]); + } + assertThat(ids).doesNotContain("null").doesNotHaveDuplicates().contains("kept-id"); + for (String id : ids) { + if (!"kept-id".equals(id)) { + assertThat(id).matches("[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}"); + } + } + } + + @Test + @DisplayName("one log line names the table and the count; a second start writes and logs nothing") + void logsOncePerTableAndIsIdempotent() throws Exception { + createLegacyTable(); + + new SQLiteDataOperator<>(dataSource, LegacyEntity.class); + List firstLines = backfillLines(); + List afterFirst = rows(); + + logged.clear(); + new SQLiteDataOperator<>(dataSource, LegacyEntity.class); + + assertThat(firstLines).hasSize(1); + assertThat(firstLines.get(0)).contains("null_id_entity").contains("3"); + assertThat(backfillLines()).as("a second start with no NULL ids logged a line").isEmpty(); + assertThat(rows()).as("a second start rewrote ids").isEqualTo(afterFirst); + } + + @Test + @DisplayName("a backfilled row can be updated by its new id, and the change survives a new operator") + void backfilledRowIsUpdatable() throws Exception { + createLegacyTable(); + SQLiteDataOperator operator = new SQLiteDataOperator<>(dataSource, LegacyEntity.class); + + LegacyEntity world = operator.query().where("name").eq("world").first(); + world.setName("world-renamed"); + operator.update(world); + + SQLiteDataOperator restarted = new SQLiteDataOperator<>(dataSource, LegacyEntity.class); + assertThat(restarted.getById(world.getId()).getName()).isEqualTo("world-renamed"); + } + + @Test + @DisplayName("MySQL cannot hold a NULL id, so its operator runs no backfill") + void mysqlOperatorRunsNoBackfill() throws Exception { + createLegacyTable(); + + new MysqlDataOperator<>(dataSource, LegacyEntity.class); + + assertThat(rows()).filteredOn(row -> row.contains("|null|")).hasSize(3); + assertThat(backfillLines()).isEmpty(); + } + } + + /** + * An entity whose {@code getId()} is derived from another column, the shape UltiEssentials' + * {@code UuidKeyedDataEntity} and UltiKits' {@code KitClaimData} use: the inherited + * {@code id} field is never set, and every {@code WHERE id = ?} binds {@code getId()}. + */ + @Table("derived_id_entity") + public static class DerivedIdEntity extends BaseDataEntity { + private static final long serialVersionUID = 1L; + + @Column("uuid") + private String uuid; + + @Column("name") + private String name; + + public DerivedIdEntity() { + } + + public DerivedIdEntity(String uuid, String name) { + this.uuid = uuid; + this.name = name; + } + + @Override + public String getId() { + return uuid; + } + + @Override + public void setId(String id) { + this.uuid = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + } + + private static List derivedRows() throws Exception { + List rows = new ArrayList<>(); + try (Connection conn = dataSource.getConnection(); + Statement stmt = conn.createStatement(); + ResultSet rs = stmt.executeQuery( + "SELECT `id`, `uuid`, `name` FROM derived_id_entity ORDER BY _rowid_")) { + while (rs.next()) { + rows.add(rs.getString(1) + "|" + rs.getString(2) + "|" + rs.getString(3)); + } + } + return rows; + } + + @Nested + @DisplayName("entities whose getId() is derived from another column") + class DerivedId { + + @BeforeEach + void dropDerivedTable() throws Exception { + execute("DROP TABLE IF EXISTS derived_id_entity"); + } + + @Test + @DisplayName("insert writes getId() into the id column, so update and delete by it reach the row") + void insertWritesTheReportedId() throws Exception { + SQLiteDataOperator operator = new SQLiteDataOperator<>(dataSource, DerivedIdEntity.class); + DerivedIdEntity entity = new DerivedIdEntity("11111111-1111-1111-1111-111111111111", "first"); + operator.insert(entity); + + assertThat(derivedRows()).containsExactly("11111111-1111-1111-1111-111111111111|11111111-1111-1111-1111-111111111111|first"); + + entity.setName("renamed"); + operator.update(entity); + assertThat(operator.getById(entity.getId()).getName()).isEqualTo("renamed"); + + logged.clear(); + new SQLiteDataOperator<>(dataSource, DerivedIdEntity.class); + assertThat(backfillLinesFor("derived_id_entity")).as("a row inserted on 6.3.0 had no id").isEmpty(); + + operator.delById(entity.getId()); + assertThat(derivedRows()).isEmpty(); + } + + @Test + @DisplayName("insertAll and updateAll write getId() into the id column too") + void batchPathsWriteTheReportedId() throws Exception { + SQLiteDataOperator operator = new SQLiteDataOperator<>(dataSource, DerivedIdEntity.class); + DerivedIdEntity a = new DerivedIdEntity("22222222-2222-2222-2222-222222222222", "a"); + DerivedIdEntity b = new DerivedIdEntity("33333333-3333-3333-3333-333333333333", "b"); + operator.insertAll(Arrays.asList(a, b)); + a.setName("a2"); + b.setName("b2"); + operator.updateAll(Arrays.asList(a, b)); + + assertThat(derivedRows()).containsExactly( + "22222222-2222-2222-2222-222222222222|22222222-2222-2222-2222-222222222222|a2", + "33333333-3333-3333-3333-333333333333|33333333-3333-3333-3333-333333333333|b2"); + } + + @Test + @DisplayName("a derived id reported by more than one row, or already held by another row, is written to none of them") + void backfillWritesTheReportedId() throws Exception { + // Maintainer decision 2026-09-29 (option A, 「一行都不动，只警告」): the same rule as + // UltiEssentials' own repair. Writing a shared id into one of the rows would let a write + // made through the other row land on it. + execute("CREATE TABLE derived_id_entity (`id` VARCHAR(255), `uuid` VARCHAR(255), `name` VARCHAR(255))"); + execute("INSERT INTO derived_id_entity (`id`, `uuid`, `name`) VALUES " + + "('55555555-5555-5555-5555-555555555555', '55555555-5555-5555-5555-555555555555', 'existing'), " + + "(NULL, '44444444-4444-4444-4444-444444444444', 'home'), " + + "(NULL, '44444444-4444-4444-4444-444444444444', 'duplicate'), " + + "(NULL, NULL, 'no-uuid'), " + + "(NULL, '55555555-5555-5555-5555-555555555555', 'clash'), " + + "(NULL, '66666666-6666-6666-6666-666666666666', 'unique')"); + + SQLiteDataOperator operator = new SQLiteDataOperator<>(dataSource, DerivedIdEntity.class); + + assertThat(derivedRows()).containsExactly( + "55555555-5555-5555-5555-555555555555|55555555-5555-5555-5555-555555555555|existing", + "null|44444444-4444-4444-4444-444444444444|home", + "null|44444444-4444-4444-4444-444444444444|duplicate", + "null|null|no-uuid", + "null|55555555-5555-5555-5555-555555555555|clash", + "66666666-6666-6666-6666-666666666666|66666666-6666-6666-6666-666666666666|unique"); + assertThat(warningsFor("derived_id_entity")).hasSize(1); + assertThat(warningsFor("derived_id_entity").get(0)) + .contains("4 row(s)") + .contains("2 share a reported id") + .contains("1 report an id another row already holds"); + + assertThat(operator.getById("55555555-5555-5555-5555-555555555555").getName()) + .as("a write through the clashing row could reach the existing row") + .isEqualTo("existing"); + DerivedIdEntity unique = operator.getById("66666666-6666-6666-6666-666666666666"); + unique.setName("unique-renamed"); + operator.update(unique); + assertThat(operator.getById("66666666-6666-6666-6666-666666666666").getName()).isEqualTo("unique-renamed"); + } + + @Test + @DisplayName("a row that cannot be read as the entity is left without an id, and the others are repaired") + void unreadableRowIsLeft() throws Exception { + execute("DROP TABLE IF EXISTS null_id_entity"); + execute("CREATE TABLE null_id_entity (`id` VARCHAR(255), `name` VARCHAR(255), `score` VARCHAR(255))"); + execute("INSERT INTO null_id_entity (`id`, `name`, `score`) VALUES (NULL, 'readable', '1'), (NULL, 'unreadable', 'not-a-number')"); + + new SQLiteDataOperator<>(dataSource, LegacyEntity.class); + + List ids = new ArrayList<>(); + try (Connection conn = dataSource.getConnection(); + Statement stmt = conn.createStatement(); + ResultSet rs = stmt.executeQuery("SELECT `name`, `id` FROM null_id_entity ORDER BY _rowid_")) { + while (rs.next()) { + ids.add(rs.getString(1) + "|" + (rs.getString(2) == null ? "null" : "set")); + } + } + assertThat(ids).containsExactly("readable|set", "unreadable|null"); + assertThat(warningsFor("null_id_entity")).isNotEmpty(); + } + } + + @Nested + @DisplayName("update and delete by a null id throw") + class RefuseNullId { + + private List> operators() { + return Arrays.asList( + new SQLiteDataOperator<>(dataSource, LegacyEntity.class), + new SimpleJsonDataOperator<>(tempDir.toFile().getAbsolutePath(), LegacyEntity.class)); + } + + @Test + @DisplayName("update(T) with a null id") + void updateEntity() { + for (DataOperator operator : operators()) { + assertThatThrownBy(() -> operator.update(new LegacyEntity("no-id", 1))) + .as(operator.getClass().getSimpleName()) + .isInstanceOf(DataAccessException.class); + } + } + + @Test + @DisplayName("update(column, value, id) with a null id") + void updateColumn() { + for (DataOperator operator : operators()) { + assertThatThrownBy(() -> operator.update("name", "x", null)) + .as(operator.getClass().getSimpleName()) + .isInstanceOf(DataAccessException.class); + } + } + + @Test + @DisplayName("delById with a null id") + void deleteById() { + for (DataOperator operator : operators()) { + assertThatThrownBy(() -> operator.delById(null)) + .as(operator.getClass().getSimpleName()) + .isInstanceOf(DataAccessException.class); + } + } + + @Test + @DisplayName("updateAll with one null-id entity writes nothing") + void updateAllWritesNothing() throws Exception { + for (DataOperator operator : operators()) { + LegacyEntity stored = new LegacyEntity("stored", 1); + operator.insert(stored); + LegacyEntity changed = operator.getById(stored.getId()); + changed.setName("changed"); + + assertThatThrownBy(() -> operator.updateAll(Arrays.asList(changed, new LegacyEntity("no-id", 2)))) + .as(operator.getClass().getSimpleName()) + .isInstanceOf(DataAccessException.class); + assertThat(operator.getById(stored.getId()).getName()) + .as("%s: updateAll wrote part of a batch it refused", operator.getClass().getSimpleName()) + .isEqualTo("stored"); + } + } + } +} diff --git a/src/test/java/com/ultikits/ultitools/interfaces/impl/data/QueryDeleteCountTest.java b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/QueryDeleteCountTest.java new file mode 100644 index 000000000..d2dfbc431 --- /dev/null +++ b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/QueryDeleteCountTest.java @@ -0,0 +1,280 @@ +package com.ultikits.ultitools.interfaces.impl.data; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.sql.Connection; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.logging.Logger; + +import javax.sql.DataSource; + +import org.bukkit.Bukkit; +import org.bukkit.Server; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import com.ultikits.ultitools.abstracts.data.BaseDataEntity; +import com.ultikits.ultitools.annotations.Column; +import com.ultikits.ultitools.annotations.Table; +import com.ultikits.ultitools.entities.WhereCondition; +import com.ultikits.ultitools.exceptions.DataAccessException; +import com.ultikits.ultitools.interfaces.DataOperator; +import com.ultikits.ultitools.interfaces.impl.data.json.SimpleJsonDataOperator; +import com.ultikits.ultitools.interfaces.impl.data.sqlite.SQLiteDataOperator; +import com.zaxxer.hikari.HikariConfig; +import com.zaxxer.hikari.HikariDataSource; + +/** + * {@code Query#delete()} returns the number of rows the backend actually removed (#521). It used + * to return how many rows the query matched, and to skip a matched row with a null id while still + * counting it. + */ +@DisplayName("Query#delete() returns the rows actually removed (#521)") +class QueryDeleteCountTest { + + private static DataSource dataSource; + + @TempDir + Path tempDir; + + @Table("query_delete_entity") + public static class DeleteEntity extends BaseDataEntity { + private static final long serialVersionUID = 1L; + + @Column("name") + private String name; + + @Column("score") + private int score; + + public DeleteEntity() { + } + + public DeleteEntity(String name, int score) { + this.name = name; + this.score = score; + } + + public String getName() { + return name; + } + + public int getScore() { + return score; + } + } + + @BeforeAll + static void initDataSource() { + if (Bukkit.getServer() == null) { + Server mockServer = mock(Server.class); + Logger mockLogger = mock(Logger.class); + when(mockServer.getLogger()).thenReturn(mockLogger); + Bukkit.setServer(mockServer); + } + HikariConfig config = new HikariConfig(); + config.setJdbcUrl("jdbc:h2:mem:querydelete;DB_CLOSE_DELAY=-1;MODE=MySQL"); + config.setUsername("sa"); + // No setPassword: the in-memory database is created without one on first connect. + dataSource = new HikariDataSource(config); + } + + @BeforeEach + void dropTable() throws Exception { + try (Connection conn = dataSource.getConnection(); + Statement stmt = conn.createStatement()) { + stmt.execute("DROP TABLE IF EXISTS query_delete_entity"); + } + } + + private SimpleJsonDataOperator json() { + return new SimpleJsonDataOperator<>(tempDir.toFile().getAbsolutePath(), DeleteEntity.class); + } + + private static void seed(DataOperator operator) { + operator.insert(new DeleteEntity("low-a", 1)); + operator.insert(new DeleteEntity("low-b", 2)); + operator.insert(new DeleteEntity("high", 9)); + } + + @Nested + @DisplayName("the count equals the rows removed") + class CountEqualsRowsRemoved { + + @Test + @DisplayName("JSON: delete() returns 2 for two removed rows and a re-query finds neither") + void jsonReturnsRemovedCount() { + SimpleJsonDataOperator operator = json(); + seed(operator); + + int removed = operator.query().where("score").lt(5).delete(); + + assertThat(removed).isEqualTo(2); + assertThat(operator.query().where("score").lt(5).list()).isEmpty(); + assertThat(operator.getAll()).extracting(DeleteEntity::getName).containsExactly("high"); + } + + @Test + @DisplayName("SQLite: delete() returns 2 for two removed rows and a re-query finds neither") + void sqliteReturnsRemovedCount() { + SQLiteDataOperator operator = new SQLiteDataOperator<>(dataSource, DeleteEntity.class); + seed(operator); + + int removed = operator.query().where("score").lt(5).delete(); + + assertThat(removed).isEqualTo(2); + assertThat(operator.query().where("score").lt(5).list()).isEmpty(); + assertThat(operator.getAll()).extracting(DeleteEntity::getName).containsExactly("high"); + } + + @Test + @DisplayName("JSON: a matched row removed by someone else before delete() runs is not counted") + void jsonDoesNotCountARowItDidNotRemove() { + SimpleJsonDataOperator operator = + new SimpleJsonDataOperator(tempDir.toFile().getAbsolutePath(), DeleteEntity.class) { + @Override + public List getAll() { + List all = super.getAll(); + // A concurrent writer removes one matched row between the read and the delete. + super.delById(all.stream().filter(e -> "low-a".equals(e.getName())).findFirst() + .orElseThrow(IllegalStateException::new).getId()); + return all; + } + }; + seed(operator); + + assertThat(operator.query().where("score").lt(5).delete()) + .as("delete() counted a row another writer had already removed") + .isEqualTo(1); + } + + @Test + @DisplayName("SQLite: a matched row removed by someone else before delete() runs is not counted") + void sqliteDoesNotCountARowItDidNotRemove() { + SQLiteDataOperator operator = + new SQLiteDataOperator(dataSource, DeleteEntity.class) { + @Override + public List getAll() { + List all = super.getAll(); + super.delById(all.stream().filter(e -> "low-a".equals(e.getName())).findFirst() + .orElseThrow(IllegalStateException::new).getId()); + return all; + } + }; + seed(operator); + + assertThat(operator.query().where("score").lt(5).delete()) + .as("delete() counted a row another writer had already removed") + .isEqualTo(1); + } + + @Test + @DisplayName("an operator whose delete removes nothing is reported as 0, not as the match count") + void foreignOperatorThatRemovesNothingCountsZero() { + @SuppressWarnings("unchecked") + DataOperator operator = mock(DataOperator.class); + DeleteEntity row = new DeleteEntity("low-a", 1); + row.setId("row-1"); + List rows = new ArrayList<>(Collections.singletonList(row)); + when(operator.getAll()).thenReturn(rows); + // delById is a no-op on this mock; the row is still there afterwards. + when(operator.exist(any(WhereCondition[].class))).thenReturn(true); + + assertThat(new QueryImpl<>(operator).delete()).isZero(); + } + } + + @Nested + @DisplayName("an operator that does not report affected rows") + class UncountedOperator { + + @Test + @DisplayName("a matched row another writer removed before this delete is not counted") + void rowGoneBeforeTheDeleteIsNotCounted() { + @SuppressWarnings("unchecked") + DataOperator operator = mock(DataOperator.class); + DeleteEntity row = new DeleteEntity("low-a", 1); + row.setId("row-1"); + when(operator.getAll()).thenReturn(new ArrayList<>(Collections.singletonList(row))); + // Gone already when delete() looks, and still gone afterwards: this delete removed nothing. + when(operator.exist(any(WhereCondition[].class))).thenReturn(false); + + assertThat(new QueryImpl<>(operator).delete()).isZero(); + } + + @Test + @DisplayName("a row present before and gone after its delById is counted") + void rowRemovedByThisDeleteIsCounted() { + @SuppressWarnings("unchecked") + DataOperator operator = mock(DataOperator.class); + DeleteEntity row = new DeleteEntity("low-a", 1); + row.setId("row-1"); + when(operator.getAll()).thenReturn(new ArrayList<>(Collections.singletonList(row))); + when(operator.exist(any(WhereCondition[].class))).thenReturn(true, false); + + assertThat(new QueryImpl<>(operator).delete()).isEqualTo(1); + } + } + + @Nested + @DisplayName("a matched row with a null id") + class NullIdRow { + + private void writeRowWithoutId(String fileName) throws IOException { + File file = new File(tempDir.toFile(), fileName + ".json"); + Files.write(file.toPath(), "{\"name\":\"no-id\",\"score\":1}".getBytes(StandardCharsets.UTF_8)); + } + + @Test + @DisplayName("JSON: delete() refuses it with DataAccessException naming the type, and deletes nothing") + void jsonRefusesNullIdRow() throws IOException { + writeRowWithoutId("legacy"); + SimpleJsonDataOperator operator = json(); + operator.insert(new DeleteEntity("low-b", 2)); + + assertThatThrownBy(() -> operator.query().where("score").lt(5).delete()) + .isInstanceOf(DataAccessException.class) + .hasMessageContaining(DeleteEntity.class.getName()); + assertThat(operator.query().where("score").lt(5).list()) + .as("a refused delete() must not have removed the other matched row") + .hasSize(2); + } + + @Test + @DisplayName("any operator: delete() refuses a matched null-id row before calling delById") + void refusesBeforeDeletingAnything() { + @SuppressWarnings("unchecked") + DataOperator operator = mock(DataOperator.class); + DeleteEntity withId = new DeleteEntity("low-a", 1); + withId.setId("row-1"); + DeleteEntity withoutId = new DeleteEntity("low-b", 2); + List rows = new ArrayList<>(); + rows.add(withId); + rows.add(withoutId); + when(operator.getAll()).thenReturn(rows); + + assertThatThrownBy(() -> new QueryImpl<>(operator).delete()) + .isInstanceOf(DataAccessException.class) + .hasMessageContaining(DeleteEntity.class.getName()); + verify(operator, never()).delById(any()); + } + } +} diff --git a/src/test/java/com/ultikits/ultitools/interfaces/impl/data/QueryImplTest.java b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/QueryImplTest.java index 8a7ceea94..0e2c3d1fd 100644 --- a/src/test/java/com/ultikits/ultitools/interfaces/impl/data/QueryImplTest.java +++ b/src/test/java/com/ultikits/ultitools/interfaces/impl/data/QueryImplTest.java @@ -462,6 +462,9 @@ void countTest() { @DisplayName("delete() should delete matching entities and return count") void deleteTest() { when(operator.getAll()).thenReturn(sampleData()); + // This mock does not report affected rows, so delete() checks each row before and + // after its delById: present, then gone, for both matched rows (#521). + when(operator.exist(any(WhereCondition[].class))).thenReturn(true, false, true, false); int deleted = query.where("score").lt(150).delete(); diff --git a/src/test/java/com/ultikits/ultitools/manager/DataStoreManagerConcurrencyTest.java b/src/test/java/com/ultikits/ultitools/manager/DataStoreManagerConcurrencyTest.java new file mode 100644 index 000000000..9588084f3 --- /dev/null +++ b/src/test/java/com/ultikits/ultitools/manager/DataStoreManagerConcurrencyTest.java @@ -0,0 +1,186 @@ +package com.ultikits.ultitools.manager; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.RepeatedTest; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +import com.ultikits.ultitools.interfaces.DataStore; + +/** + * {@link DataStoreManager#getDatastore(String)} never returns {@code null} for a registered store, + * however it races the writers (#515). + *

+ * The map used to be a plain {@code HashMap} that {@code register}/{@code unregister} mutated under + * a lock while {@code getDatastore} read it without one, three times per call. A read during a + * resize could miss a present key, and the second and third reads could see a different map than + * the first, so a type that was registered when the call started could come back as {@code null}. + * The first test pins the structural fix deterministically; the repeated stress tests exercise the + * behaviour, which a race can only show intermittently. + */ +@DisplayName("DataStoreManager#getDatastore never returns null for a registered store (#515)") +@Timeout(value = 60, unit = TimeUnit.SECONDS) +class DataStoreManagerConcurrencyTest { + + private DataStore json; + + private static DataStore store(String type) { + DataStore store = mock(DataStore.class); + when(store.getStoreType()).thenReturn(type); + return store; + } + + @SuppressWarnings({"unchecked", "PMD.AvoidAccessibilityAlteration"}) + private static Map dataMap() throws Exception { + Field field = DataStoreManager.class.getDeclaredField("dataMap"); + field.setAccessible(true); + return (Map) field.get(null); + } + + @BeforeEach + void registerJsonFallback() throws Exception { + dataMap().clear(); + // The json fallback is always registered here, so getDatastore never reaches the branch + // that builds a JsonStore from UltiTools.getInstance(). + json = store("json"); + DataStoreManager.register(json); + } + + @AfterEach + void clear() throws Exception { + dataMap().clear(); + } + + @Test + @DisplayName("the registry is a concurrent map, so a lock-free read is safe against a writer") + void registryIsAConcurrentMap() throws Exception { + assertThat(dataMap()) + .as("getDatastore reads without the writers' lock, so the map itself must be safe for that") + .isInstanceOf(ConcurrentMap.class); + } + + @Test + @DisplayName("a reader thread started after registration sees the registered store") + void readerStartedAfterRegistrationSeesTheStore() throws Exception { + DataStore sqlite = store("sqlite"); + DataStoreManager.register(sqlite); + AtomicReference seen = new AtomicReference<>(); + + Thread reader = new Thread(() -> seen.set(DataStoreManager.getDatastore("sqlite"))); + reader.start(); + reader.join(TimeUnit.SECONDS.toMillis(10)); + + assertThat(reader.isAlive()).isFalse(); + assertThat(seen.get()).isSameAs(sqlite); + } + + @RepeatedTest(20) + @DisplayName("reads of a registered type never return null while other types are registered and unregistered") + void readsNeverReturnNullDuringChurn() throws Exception { + DataStore stable = store("stable"); + DataStoreManager.register(stable); + int pairs = 16; + CyclicBarrier barrier = new CyclicBarrier(pairs * 2); + List failures = new CopyOnWriteArrayList<>(); + List reads = new CopyOnWriteArrayList<>(); + List threads = new ArrayList<>(); + for (int i = 0; i < pairs; i++) { + String type = "churn-" + i; + threads.add(new Thread(() -> { + try { + barrier.await(10, TimeUnit.SECONDS); + for (int round = 0; round < 50; round++) { + DataStore churn = store(type); + DataStoreManager.register(churn); + DataStoreManager.unregister(churn); + } + } catch (Throwable t) { + failures.add(t); + } + })); + threads.add(new Thread(() -> { + try { + barrier.await(10, TimeUnit.SECONDS); + for (int round = 0; round < 200; round++) { + reads.add(DataStoreManager.getDatastore("stable")); + } + } catch (Throwable t) { + failures.add(t); + } + })); + } + for (Thread thread : threads) { + thread.start(); + } + for (Thread thread : threads) { + thread.join(TimeUnit.SECONDS.toMillis(20)); + assertThat(thread.isAlive()).as("a thread did not finish").isFalse(); + } + + assertThat(failures).isEmpty(); + assertThat(reads).hasSize(pairs * 200).allMatch(read -> read == stable); + } + + @RepeatedTest(20) + @DisplayName("a type being unregistered reads as itself or as the json fallback, never null") + void typeBeingUnregisteredReadsAsItselfOrJson() throws Exception { + int readers = 8; + CyclicBarrier barrier = new CyclicBarrier(readers + 1); + List failures = new CopyOnWriteArrayList<>(); + List reads = new CopyOnWriteArrayList<>(); + List flapping = new CopyOnWriteArrayList<>(); + List threads = new ArrayList<>(); + threads.add(new Thread(() -> { + try { + barrier.await(10, TimeUnit.SECONDS); + for (int round = 0; round < 500; round++) { + DataStore mysql = store("mysql"); + flapping.add(mysql); + DataStoreManager.register(mysql); + DataStoreManager.unregister(mysql); + } + } catch (Throwable t) { + failures.add(t); + } + })); + for (int i = 0; i < readers; i++) { + threads.add(new Thread(() -> { + try { + barrier.await(10, TimeUnit.SECONDS); + for (int round = 0; round < 500; round++) { + reads.add(DataStoreManager.getDatastore("mysql")); + } + } catch (Throwable t) { + failures.add(t); + } + })); + } + for (Thread thread : threads) { + thread.start(); + } + for (Thread thread : threads) { + thread.join(TimeUnit.SECONDS.toMillis(20)); + assertThat(thread.isAlive()).as("a thread did not finish").isFalse(); + } + + assertThat(failures).isEmpty(); + assertThat(reads).hasSize(readers * 500) + .allMatch(read -> read == json || flapping.contains(read)); + } +} diff --git a/src/test/java/com/ultikits/ultitools/utils/BeanCopyUtilPrimitiveFieldsTest.java b/src/test/java/com/ultikits/ultitools/utils/BeanCopyUtilPrimitiveFieldsTest.java new file mode 100644 index 000000000..5b43b6ab6 --- /dev/null +++ b/src/test/java/com/ultikits/ultitools/utils/BeanCopyUtilPrimitiveFieldsTest.java @@ -0,0 +1,183 @@ +package com.ultikits.ultitools.utils; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.nio.file.Path; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import com.ultikits.ultitools.abstracts.data.BaseDataEntity; +import com.ultikits.ultitools.annotations.Column; +import com.ultikits.ultitools.annotations.Table; +import com.ultikits.ultitools.interfaces.impl.data.json.SimpleJsonDataOperator; + +/** + * Every primitive and wrapper field survives {@link BeanCopyUtil#copyProperties}, and therefore + * survives {@code SimpleJsonDataOperator#update(T)} from a detached copy (#520). + *

+ * A new class rather than an extension of {@code BeanCopyUtilTest}: that class pins the + * conversion rules for mismatched types, this one pins the same-type round trip for the whole + * primitive set at once, plus the JSON-store consumer the issue measured. + */ +@DisplayName("BeanCopyUtil copies every primitive and wrapper field (#520)") +class BeanCopyUtilPrimitiveFieldsTest { + + /** One field of each primitive type and each wrapper type. */ + public static class AllPrimitives { + private boolean aBoolean; + private char aChar; + private byte aByte; + private short aShort; + private int anInt; + private long aLong; + private float aFloat; + private double aDouble; + private Boolean boxedBoolean; + private Character boxedChar; + private Byte boxedByte; + private Short boxedShort; + private Integer boxedInt; + private Long boxedLong; + private Float boxedFloat; + private Double boxedDouble; + } + + @Table("primitive_flag_entity") + public static class FlagEntity extends BaseDataEntity { + private static final long serialVersionUID = 1L; + + @Column("active") + private boolean active; + @Column("grade") + private char grade; + @Column("boxed_grade") + private Character boxedGrade; + + public boolean isActive() { + return active; + } + + public void setActive(boolean active) { + this.active = active; + } + + public char getGrade() { + return grade; + } + + public void setGrade(char grade) { + this.grade = grade; + } + + public Character getBoxedGrade() { + return boxedGrade; + } + + public void setBoxedGrade(Character boxedGrade) { + this.boxedGrade = boxedGrade; + } + } + + @Nested + @DisplayName("copyProperties") + class CopyProperties { + + @Test + @DisplayName("copies a field of every primitive and wrapper type with its value intact") + void copiesEveryPrimitiveAndWrapperField() { + AllPrimitives source = new AllPrimitives(); + source.aBoolean = true; + source.aChar = 'x'; + source.aByte = 7; + source.aShort = 300; + source.anInt = 70_000; + source.aLong = 5_000_000_000L; + source.aFloat = 1.5f; + source.aDouble = 2.25d; + source.boxedBoolean = Boolean.TRUE; + source.boxedChar = 'y'; + source.boxedByte = 8; + source.boxedShort = 301; + source.boxedInt = 70_001; + source.boxedLong = 5_000_000_001L; + source.boxedFloat = 2.5f; + source.boxedDouble = 3.25d; + + AllPrimitives target = new AllPrimitives(); + BeanCopyUtil.copyProperties(source, target); + + assertThat(target).usingRecursiveComparison().isEqualTo(source); + } + } + + @Nested + @DisplayName("JSON store update(T) from a detached copy") + class JsonStoreDetachedUpdate { + + @TempDir + Path storeDir; + + private FlagEntity storedAndReloaded() { + SimpleJsonDataOperator store = + new SimpleJsonDataOperator<>(storeDir.toFile().getAbsolutePath(), FlagEntity.class); + FlagEntity original = new FlagEntity(); + original.setActive(true); + original.setGrade('A'); + original.setBoxedGrade('B'); + store.insert(original); + store.flush(); + return original; + } + + private FlagEntity detachedCopyOf(FlagEntity entity) { + FlagEntity copy = new FlagEntity(); + copy.setId(entity.getId()); + copy.setActive(entity.isActive()); + copy.setGrade(entity.getGrade()); + copy.setBoxedGrade(entity.getBoxedGrade()); + return copy; + } + + private FlagEntity freshLoad(String id) { + return new SimpleJsonDataOperator<>(storeDir.toFile().getAbsolutePath(), FlagEntity.class).getById(id); + } + + @Test + @DisplayName("a boolean changed from true to false on a detached copy reads back false after a fresh load") + void booleanChangeOnDetachedCopyPersists() { + FlagEntity original = storedAndReloaded(); + SimpleJsonDataOperator store = + new SimpleJsonDataOperator<>(storeDir.toFile().getAbsolutePath(), FlagEntity.class); + + FlagEntity copy = detachedCopyOf(original); + copy.setActive(false); + store.update(copy); + store.flush(); + + assertThat(freshLoad(original.getId()).isActive()) + .as("the boolean change on the detached copy was dropped by update(T)") + .isFalse(); + } + + @Test + @DisplayName("char and Character changes on a detached copy read back after a fresh load") + void charChangesOnDetachedCopyPersist() { + FlagEntity original = storedAndReloaded(); + SimpleJsonDataOperator store = + new SimpleJsonDataOperator<>(storeDir.toFile().getAbsolutePath(), FlagEntity.class); + + FlagEntity copy = detachedCopyOf(original); + copy.setGrade('C'); + copy.setBoxedGrade('D'); + store.update(copy); + store.flush(); + + FlagEntity reloaded = freshLoad(original.getId()); + assertThat(reloaded.getGrade()).as("the char change was dropped").isEqualTo('C'); + assertThat(reloaded.getBoxedGrade()).as("the Character change was dropped").isEqualTo('D'); + } + } +}