Skip to content

fix(data): truthful copies, counts and id-less rows in the data layer (#520, #522, #521, #546, #543, #515) - #559

Merged
wisdommen merged 35 commits into
alphafrom
fix/p17-fu-data
Oct 3, 2026
Merged

wisdommen merged 35 commits into
alphafrom
fix/p17-fu-data

Conversation

@wisdommen

@wisdommen wisdommen commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

This pull request targets alpha deliberately (integration branch for 6.3.0), not the default main. It fixes seven silent defects in the data layer: a boolean change lost, a JSON read that handed out the store's own objects, a delete count that counted rows it never removed, id-less rows no update could reach, a missing conditional write, a registry read that could return null, and an update of a deleted row that either crashed (JSON) or pretended to succeed (SQL).

本 PR 有意指向 alpha(6.3.0 集成分支)。修复数据层的六个静默缺陷:布尔字段更新丢失、JSON 读取返回缓存实例、删除计数不实、无 id 行无法更新、缺少条件写入、注册表并发读返回 null。

Issue closure

Closing keywords do not fire for pull requests into alpha; these issues are closed by hand after the merge.

Closes #520
Closes #522
Closes #521
Closes #546
Closes #543
Closes #515
Closes #558

What changes

Issue Change
#520 BeanCopyUtil decides assignability against the wrapper of a primitive target type, so boolean and char fields copy; the JSON update(T) from a detached entity no longer drops them.
#522 The JSON operator returns detached copies from getById, getAll, page, getLike (and every query() terminal) and caches a copy on insert; exist(entity) looks the entry up by id, as SQL does; update(T) fires onUpdate() on the incoming entity, as the relational operators do, then swaps a copy into the cache.
#521 Query#delete() returns the rows the backend removed (relational affected-row count, JSON entries removed) through the internal RowCountingDelete interface; delById keeps its signature. A matched null-id row throws DataAccessException before anything is deleted. A third-party operator (no RowCountingDelete) is counted only for rows present before and gone after its own delById.
#546 SQLite table initialisation gives every id IS NULL row an id (keyed by _rowid_, only id written, all rows in one transaction, one INFO line per table with the count, idempotent): the id the entity reports through getId(), or a new UUID when it reports none — written only if an entity read back with it reports it. A derived id reported by more than one row without an id, or already held by another row, is written to none of them; rows left are counted per table by reason in one WARNING. Every write path now stores getId() in the id column, so an entity that overrides getId() onto another column no longer inserts a NULL id. update(T), update(column, value, id), delById, updateAll by a null id throw DataAccessException on every backend.
#543 New default boolean updateIf(T entity, WhereCondition... expected) on DataOperator. Relational: one UPDATE … WHERE id = ? AND <expected> decided by the affected-row count; JSON: check and write under the operator lock. A null id, an unmapped condition column or a null expected value throws DataAccessException on every backend. The default throws UnsupportedOperationException naming the implementing class.
#558 An update by a non-null id that matches no row writes nothing and logs one WARNING (table, id) every time on JSON, SQLite and MySQL, and returns normally (JSON no longer throws a raw NullPointerException); new default int updateCounted(T entity) on DataOperator returns 1/0 so the caller can tell. A third-party operator without a count is counted by whether the row exists before its update.
#515 DataStoreManager's registry is a ConcurrentHashMap and getDatastore reads each key once.

Decisions applied

Checklist rows amended

FEATURES.md and UAT-CHECKLIST.md, section Data persistence, IDs in file order:

Swept for the eight checklist defect classes; one vacuous-pass risk (PVP already disabled before the test) was fixed in 2d2673bd.

Red-when-reverted evidence

Each issue has a test(<n>) commit that fails, then a fix commit that touches no test file. With only the fix reverted, each named test fails:

Issue Test RED with fix reverted
#520 BeanCopyUtilPrimitiveFieldsTest 3 of 3 fail
#522 DetachedReadParityTest 7 of 8 fail, all on the JSON backend
#521 QueryDeleteCountTest 5 of 7 fail
#546 NullIdRowsTest 7 of 8 fail
#543 ConditionalUpdateTest compilation error (method absent)
#546 (review) NullIdRowsTest (derived-id cases) 3 of 3 new cases fail
#543 (review) ConditionalUpdateTest (refusals) 2 of 2 new cases fail
#546 (Codex 1) NullIdRowsTest (rows left unaddressable) 2 of 2 new cases fail
#522 (Codex 2) DetachedReadParityTest#existMatchesByIdAfterALocalChange fails on JSON
#521 (Codex 3) QueryDeleteCountTest$UncountedOperator 2 of 2 fail
#546 (option A) NullIdRowsTest#backfillWritesTheReportedId fails
#558 (warn) MissingRowUpdateTest 3 of 3 fail
#558 (counted) MissingRowUpdateTest (updateCounted) compilation error (method absent)
#515 DataStoreManagerConcurrencyTest structural check, plus the race in 17 of 20 repetitions (12 of 20 in the first RED run)

Full mvn -B clean verify -DexcludedGroups=: 6094 tests, 0 failures; CJK scope gate 0 violations; japicmp reports only additions.

Review

Gate 1 (independent review, max effort) raised 10 findings; 9 are fixed on this branch (two blockers about entities whose getId() is derived from another column, an unmapped column and a null value in updateIf, one transaction for the backfill, three efficiency/duplication items, one inventory row), each behavioural fix test-first with a revert proof. The tenth — update() by a non-null id that matches no row is a raw NullPointerException on JSON and silent on SQL — depends on a trade-off the maintainer has not decided and is filed as #558.

  • Backfill, derived id shared by rows (maintainer 2026-09-29): 「一行都不动,只警告」 — a derived id reported by more than one NULL-id row, or already held by another row, is written to none of them; the warning counts them. This closes the two Codex P1 threads about that shape and ends the backfill's revisions by decision.
  • update() by a non-null id that matches no row: raw NullPointerException on JSON, silent no-op on SQLite/MySQL #558 (maintainer 2026-09-29): 「不写,并告诉调用方没写成」 — write nothing, warn every time, and let the caller learn it (updateCounted). This supersedes an earlier answer (「不写,但每次记一条警告」) given on a premise that turned out wrong: five modules catch exceptions from update (all for storage failures). Three of them (UltiEconomy#40, UltiTrade#52, UltiWorlds#49) are filed to adopt updateCounted after this merges.

Compatibility

COMPATIBILITY.md gains five entries under Behavioral changes that need no migration period: detached JSON reads (#522), Query#delete() counting rows removed (#521, corrects behaviour that contradicted its javadoc), the NULL-id backfill and null-id refusal (#546), the added updateIf default method (#543), and the missing-row update plus the added updateCounted default method (#558). No existing public signature changes; the added interface method is default, so modules compiled against 6.2.x still link.

Consumer impact

Measured with git grep on the fifteen module origin/master trees and UltiTools-External-Example, each negative with a control query returning hits:

  • Query#delete(): 5 call sites (UltiSocial FriendService ×4, UltiWorlds WorldService ×1); none reads the count. A matched null-id row now throws — reachable only through a hand-edited JSON entry without an id.
  • implements DataOperator: 0 (control DataOperator<: 48 hits).
  • Code that changes a loaded entity without update(...): 3 candidate files read, none relies on it (setters before insert, or reflection).
  • update/updateAll/delById: 44 sites read; ids come from stored entities or a constant, so none passes a null id once SQLite rows are backfilled.
  • DataStoreManager, BeanCopyUtil, updateIf: 0 module references.

Documentation

Companion pull request into UltiTools-Dev-Doc alpha: UltiKits/UltiTools-Dev-Doc#104 (detached reads, null-id refusal, SQLite backfill, a Conditional Update section, updateCounted for a missing row, delete() returns rows removed; prose and inline code only, marked as of v6.3.0).

🤖 Generated with Claude Code

wisdommen and others added 18 commits September 29, 2026 20:30
… JSON update(T)

- copyProperties over one field of each primitive and wrapper type
- detached-copy update(T) on a JSON store keeps a boolean and a char change

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…type

Field#get boxes every primitive, and boolean.class.isAssignableFrom(Boolean.class)
is false, so boolean and char values fell through to convertValue, which had no
branch for either and dropped the write. update(T) on the JSON store therefore
lost a boolean or char change made on a detached copy.

- compare against the wrapper of a primitive target type before converting
- no other conversion changes

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… alike

- getAll, getAll(conditions), getById, page, getLike, query().first(), and the
  inserted instance: a change without update() leaves the store unchanged
- update(T) on a returned copy persists on both backends

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ances

getById, getAll, page and getLike returned the instances the JSON operator
caches, and insert cached the caller's instance, so on datasource.type json a
change to a loaded entity without update() was written at the next flush. The
relational backends never did this.

- every JSON read returns a copy made through the same Gson form the store writes
- insert caches a copy; update(T) builds the new entry on a copy and swaps it in
- update(T) fires onUpdate() on the incoming entity before copying, as the
  relational backends do
- FEATURES/UAT rows ultitools.storage.detached-reads; COMPATIBILITY entry

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…-id match

- JSON and SQLite: the count equals rows removed; a row another writer removed
  first is not counted
- an operator whose delete removes nothing reports 0
- a matched null-id row throws DataAccessException before anything is deleted

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…atch

delete() returned the number of rows it matched and skipped a matched row with a
null id while still counting it, so its int could report a removal that never
happened.

- relational delById now carries the DELETE's affected-row count, the JSON
  operator the number of cache entries removed, through the internal
  RowCountingDelete interface; delById keeps its void signature
- a third-party operator is counted by checking the row is gone after delById
- a matched null-id row throws DataAccessException before anything is deleted
- FEATURES/UAT rows ultitools.storage.query-delete-count; COMPATIBILITY entry

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ate/delete

- legacy NULL-id rows get distinct UUIDs, other columns byte-identical, one log
  line per table, nothing on a second start; a backfilled row is updatable
- MySQL runs no backfill
- update(T), update(column, value, id), delById, updateAll with a null id throw
  DataAccessException on SQLite and JSON; a refused updateAll writes nothing

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…y a null id

UltiTools-API 6.2.0 inserted rows without an id and SQLite's DDL accepted the
NULL, so every update or delete of such a row matched nothing and returned
normally. Applies the maintainer decision of 2026-09-27.

- SQLite table init gives every id IS NULL row a UUID, keyed by _rowid_ and
  writing only id, and logs one line per table with the count; idempotent
- MySQL (cannot hold a NULL id) runs no backfill
- update(T), update(column, value, id), delById and updateAll with a null id
  throw DataAccessException on the relational and JSON backends; updateAll
  checks every entity before writing
- FEATURES/UAT rows ultitools.storage.null-id-backfill / null-id-refused;
  COMPATIBILITY entry

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…es hold

- JSON, SQLite and MySQL: applies and returns true when every expected
  condition holds; false and nothing written once the balance changed, for a
  missing row, or when one of two conditions fails; a quoted value matches
- null id throws DataAccessException
- two operators over one database: the stalled second writer does not apply
- the interface default throws UnsupportedOperationException naming the class

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ether it applied

DataOperator offered no conditional write, so a module could not fence a lease
lock. UltiEconomy's wallet merge will condition each account write on the
balance it read (maintainer decision 2026-09-29).

- added default method updateIf(entity, expected...); the default throws
  UnsupportedOperationException naming the implementing class
- relational: one UPDATE ... WHERE id = ? AND <expected>, decided by the
  affected-row count; SET clause shared with update(T); columns allow-listed,
  values bound
- JSON: check and write under the operator lock, matching as getAll does
- null id throws DataAccessException
- FEATURES/UAT rows ultitools.storage.conditional-update; COMPATIBILITY entry

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…er churn

- the registry is a ConcurrentMap (deterministic)
- a reader started after registration sees the store
- repeated stress: reads of a stable type during register/unregister churn,
  and reads of a type being unregistered, never return null

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ds a key once

getDatastore read a plain HashMap without the writers' lock, up to three times
per call, so a registered type could come back null while another thread
registered or unregistered a store.

- dataMap is a ConcurrentHashMap; register/unregister stay synchronized
- getDatastore reads each key once and returns the value it read
- FEATURES/UAT rows ultitools.storage.concurrent-lookup

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…hanged value

The row set PVP false and read it back after a restart; if the world's PVP was
already disabled, that passed without the update ever reaching the row. The
precondition now establishes PVP enabled, and step 1 reads it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…at backfill

Gate-1 review findings: an entity overriding getId() onto another column
(UltiEssentials UuidKeyedDataEntity, UltiKits KitClaimData) had its raw id field
written (NULL) while every WHERE binds getId(), and the backfill gave such rows a
random UUID no lookup would ever use.

- insert/update and insertAll/updateAll write getId() into the id column
- the backfill writes the reported id, and a new UUID only when the entity
  reports none or a duplicate

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…entity reports

Gate-1 review. Every WHERE binds getId(), but the write paths stored the
inherited id field, which an entity overriding getId() onto another column
(UltiEssentials UuidKeyedDataEntity, UltiKits KitClaimData) never sets: such
entities still inserted NULL ids on SQLite and failed to insert on MySQL. The
backfill then gave those rows a random UUID no lookup uses, and pre-empted
UltiEssentials' own deterministic repair.

- insert, insertAll, update(T)/updateIf (shared SET clause) and updateAll write
  getId() for the id column through one persistedValue helper
- the backfill materialises each NULL-id row through the entity type (shared
  row mapper) and writes its getId(), or a new UUID when it reports none or a
  duplicate; all rows in one transaction instead of one autocommit per row
- log line names how many rows took the entity's own id; rows/COMPATIBILITY
  updated

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ue everywhere

Gate-1 review: on JSON an unmapped column returned false, so a compare-and-set
loop would retry forever where SQLite/MySQL throw; a null expected value never
matched in SQL and threw a raw NullPointerException on JSON.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ue everywhere

Gate-1 review.
- JSON updateIf checks every condition first: a null condition, a column the
  entity does not map with @column, and a null or non-serialisable value throw
  DataAccessException, as the relational operators do
- relational updateIf refuses a null expected value (column = NULL never holds)
- JSON: update(T) and updateIf share findStored/replaceEntry; detach and the
  transaction snapshot copy through a JSON tree; a condition's value is
  serialised once per condition, not once per entry
- javadoc and FEATURES/UAT rows updated

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Gate-1 review: DataStoreManager#getDatastore runs only in UltiTools#initDataStore;
modules read the selected store through UltiToolsPlugin#getDataOperator. The row
claimed the lookup ran whenever a data operator was obtained.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T12:51:06.394240Z 7bc4ab6 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@codacy-production

codacy-production Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 238 complexity

Metric Results
Complexity 238

View in Codacy

🟢 Coverage 91.46% diff coverage · +0.11% coverage variation

Metric Results
Coverage variation ✅ +0.11% coverage variation (-1.00%)
Diff coverage ✅ 91.46% diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (dfe71e0) 15352 12560 81.81%
Head commit (7bc4ab6) 15570 (+218) 12756 (+196) 81.93% (+0.11%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#559) 316 289 91.46%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@wisdommen

Copy link
Copy Markdown
Member Author

@codex review

wisdommen and others added 2 commits September 29, 2026 21:40
…nection

Codacy (Opengrep SQL-injection patterns) flagged the backfill's raw
prepareStatement calls on concatenated SQL. The statements use only the
@table name and a constant row-identifier column, and every value is bound;
they now go through QueryRunner's connection-taking overloads like the rest of
the class, still inside the one transaction.

- drop fully qualified java.util.UUID / java.util.Collections names that the
  imports added on this branch made redundant (PMD)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ngrep

Every caller passes a string literal from the test class itself.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c7bdae364e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +257 to +260
if (id != null && assigned.add(id) && assignId(statement, id, rowIds.get(i))) {
reported++;
} else if (assignId(statement, UUID.randomUUID().toString(), rowIds.get(i))) {
generated++;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep generated backfill IDs aligned with getId

When a legacy entity overrides getId() to derive it from another column and that value is null or duplicates another row, this fallback writes a random UUID only to the physical id column. Reloading the row still returns the unchanged null/duplicate value from getId(); null-valued rows remain unmodifiable, while duplicate-valued rows can direct a later update(entity) or delById(entity.getId()) at the other row and corrupt or delete it. Either update the getter's backing value consistently or leave/refuse rows that cannot be made addressable without changing it.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3cc5f74a (test first in a50d17ed; red when the fix is reverted).

The backfill now writes a candidate id only if an entity read back with that id in the id column reports it from getId(): the id the entity already reports, otherwise a new UUID. A row that no id would make addressable is left as it is and counted in one WARNING per table: a derived id that is null, a derived id another row already holds, or a row that cannot be read as the entity. For the duplicate case this also means the row is not given an id that looks repaired while update(entity) / delById(entity.getId()) still address the other row. NullIdRowsTest$DerivedId pins all three cases (backfillWritesTheReportedId, unreadableRowIsLeft).

中文:已修复。只写入「实体读回后 getId() 确实给出」的 id;无法使其可定位的行(派生 id 为 null、与他行重复、无法读成实体)保持原样并计入一条警告。

wisdommen and others added 3 commits September 29, 2026 21:43
…warns

Codex review (P1): for an entity that derives getId() from another column, a
random id written to the id column changes nothing getId() reports, so a row
whose backing value is null stays unreachable, and a duplicate still addresses
the other row. Such rows, and rows that cannot be read as the entity, must be
left without an id and reported.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…t the rest

Codex review (P1) on the gate-1 round: the fallback wrote a random UUID to rows
whose entity derives getId() from another column, which changes nothing any
lookup uses; for a duplicate derived id, update/delete would still address the
other row.

- a candidate id (the entity's own, else a new UUID) is written only if an
  entity read back with it in the id column reports it
- a row no id would make addressable -- derived id null or already another
  row's, or unreadable as the entity -- is left and counted in one WARNING
- javadoc, FEATURES and COMPATIBILITY state the rule

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…hEquals)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3cc5f74a51

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

for (T entity : toDelete) {
if (entity.getId() != null) {
operator.delById(entity.getId());
if (entity.getId() == null) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject unaddressable derived-ID rows before deleting

When legacy SQLite data contains a repaired row with physical id = X and a leftover NULL-id row whose overridden getId() derives the same X, a query matching only the leftover row passes this null-only check and deleteOne(X) deletes the repaired row instead. For example, the duplicate shape preserved by NullIdRowsTest#backfillWritesTheReportedId makes query().where("name").eq("duplicate").delete() remove home while leaving duplicate. Fresh evidence after the prior backfill fix is that the final test explicitly retains this duplicate-derived row but only exercises mutation of the addressable row; the delete path still needs to refuse or quarantine rows whose physical id does not match the reported id.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, and held for a maintainer decision rather than patched in this round.

The row shape here — two rows reporting the same derived getId() — became addressable-for-one-of-them because of this pull request's own backfill change: given two NULL-id rows that both report U, the backfill writes U into the first, and every update(entity) / delById(entity.getId()) / query()…delete() made from the second then lands on the first. UltiEssentials' own repair deliberately refuses that shape (its "shared identity" branch leaves both rows alone). The write paths cannot tell the two entities apart, since both report U, so a check on update/delete alone cannot close it.

This is the third round in which the backfill's rule has had to change (random UUID → the reported id → leave rows that cannot be made addressable), which departs further each time from the maintainer's 2026-09-27 decision ("backfill every id IS NULL row with a UUID"). The options — including not assigning a derived id that more than one row reports, which mirrors UltiEssentials, and restricting the framework backfill to entities whose getId() reads the id column — have gone to the maintainer. Measured on the long-lived test server (read-only): 27 NULL-id rows across the five derived-id tables, 0 of them with a shared or null derived id or one that clashes with an existing id.

中文:确认。该形状(两行报告同一个派生 id)由本 PR 的回填改动造成(第一行被写入 U,第二行的更新/删除落到第一行)。回填规则已连续三轮调整,偏离维护者 2026-09-27 的决定,因此交维护者决定,不在本轮继续修补。

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved by maintainer decision (2026-09-29, option A, 「一行都不动,只警告」) in 07a7b4a1 (test first in 2a7f2c76; red when the fix is reverted): a derived id reported by more than one row without an id, or already held by another row, is now written to none of those rows, so no write through one of them can reach another row. The WARNING counts the rows left per table by reason, and they stay refused on update/delete by a null id. NullIdRowsTest$DerivedId#backfillWritesTheReportedId covers the shared id, the already-held id, a null id and a unique id. Codex's review of head 7bc4ab6 found nothing.

中文:按维护者决定(选项 A:「一行都不动,只警告」)修复:共享或已被占用的派生 id 一行都不写,只在警告中计数。

@wisdommen

Copy link
Copy Markdown
Member Author

@codex review

wisdommen and others added 8 commits September 29, 2026 22:12
…r rows it removed

Codex review (P2): for a DataOperator that does not implement RowCountingDelete,
delete() counted a row as removed whenever it was absent after delById, even if
another writer had removed it first.

- a row already gone before this delete is not counted; a row present before
  and gone after its delById is
- QueryImplTest#deleteTest stubs the before/after existence its mock now sees

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nd gone after

Codex review (P2). An operator without RowCountingDelete cannot report what its
delById removed; checking only afterwards counted rows another writer had
already removed. The row is now counted only if it existed immediately before
this operator's own delById and is gone after it. Javadoc and COMPATIBILITY
state the remaining case (a concurrent removal during that one call).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…none of them

Maintainer decision 2026-09-29 (option A, 「一行都不动,只警告」) on the Codex P1
threads: when a derived id is reported by more than one NULL-id row, or is
already held by another row, none of those rows is written; the warning counts
them per table. Same rule as UltiEssentials' own repair.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…one of them

Maintainer decision 2026-09-29 (option A, 「一行都不动,只警告」), closing the
Codex P1 threads on the NULL-id backfill after three revisions of its rule:

- a reported id that more than one NULL-id row shares is written to none of
  them (UltiEssentials' own repair applies the same rule)
- a reported id another row already holds is checked before writing
- the WARNING counts the rows left per table by reason; they stay refused on
  update/delete by a null id (#546 decision of 2026-09-27)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… every backend

Maintainer 2026-09-29 (「不写,并告诉调用方没写成」): update(T),
update(column, value, id) and updateAll addressed by a non-null id that matches
no row write nothing and log one WARNING naming the table and id per call, on
JSON, SQLite and MySQL. The JSON backend threw a raw NullPointerException.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…every backend

Maintainer 2026-09-29 (「不写,并告诉调用方没写成」), first half.

- relational update(T), update(column, value, id) and updateAll log one WARNING
  naming the table and id for each update that changed no row
- JSON update(T) and update(column, value, id) no longer throw a raw
  NullPointerException for an id no entry has: nothing is written, one WARNING
- the call still returns normally: module code catches exceptions from update
  as storage failures (measured: 5 modules)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…date wrote

Maintainer 2026-09-29 (「不写,并告诉调用方没写成」), second half: a counted
update returns 1 for a stored row and 0 (nothing written, one WARNING) for a
missing one on JSON, SQLite and MySQL; a null id throws; a third-party operator
without a count is counted by whether the row exists before its update.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…date wrote

Maintainer 2026-09-29 (「不写,并告诉调用方没写成」), second half.

- added default method int updateCounted(T entity); existing update(...)
  signatures unchanged
- relational: the UPDATE's affected-row count; JSON: entries replaced
- default for third-party operators: 0 without writing when no row has the id,
  otherwise update(T) and 1 (javadoc names the one remaining miscount)
- null id throws DataAccessException
- FEATURES/UAT rows ultitools.storage.missing-row-update; COMPATIBILITY entry

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
wisdommen added a commit that referenced this pull request Oct 1, 2026
Revert b7b46fe as instructed by the supervisor. PR #559 already owns issue #515; duplicate PR #579 is retired without deleting its branch. Preserve history before carrying the reviewed source from #559.

Co-Authored-By: Claude Code (gpt-6.1-sol via claude-ocx) <noreply@anthropic.com>
wisdommen added a commit that referenced this pull request Oct 1, 2026
Revert 74a316a after reverting its fix. Retain the duplicate branch and all prior test evidence; the configuration branch will carry the reviewed datastore source and regression test owned by PR #559.

Co-Authored-By: Claude Code (gpt-6.1-sol via claude-ocx) <noreply@anthropic.com>
wisdommen added a commit that referenced this pull request Oct 1, 2026
Copy only DataStoreManager.java and DataStoreManagerConcurrencyTest.java exactly from PR #559 source head 7bc4ab6. The source fix commit also changes inventory documents, so carry its final reviewed files without unrelated data-layer or documentation changes. Supervisor correction keeps #559 as owner of #515 and retires duplicate #579; no semantic adaptation or new Codex run.

Co-Authored-By: Claude Code (gpt-6.1-sol via claude-ocx) <noreply@anthropic.com>
@wisdommen
wisdommen merged commit 053f9d4 into alpha Oct 3, 2026
13 checks passed
wisdommen added a commit that referenced this pull request Oct 3, 2026
Brings #564 up to date with alpha after #559, #581 and #561 merged.
Conflicts in FEATURES.md, UltiToolsPlugin.java, PluginManager.java,
lang/en.json and lang/zh.json are resolved exactly as the gate-3
integration merge 64c0a107 resolved them (union rows/keys,
unregister(plugin, nameStillInUse, shutdown), the server-thread guard
in performReload()). The resulting tree equals
git merge-tree --merge-base=089c592c 64c0a107 origin/alpha (fab3723):
the integration resolution plus #581's later FEATURES row correction.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
wisdommen added a commit that referenced this pull request Oct 3, 2026
…pdate)

Brings #566 up to date with alpha after #559, #581, #561 and #564 merged.
Conflicts in COMPATIBILITY.md, FEATURES.md, UltiToolsPlugin.java,
PluginManager.java and lang/{en,zh}.json are resolved exactly as the
gate-3 integration resolved them (merges 0546fd95, 9eaba676, 8c0a3780):
union rows and keys in ID order, converter preparation before the
extract-then-resolve language order in both constructors, and #581's
releaseConfigEntities beside #566's class-aware failure log.

Carries the integration-only resolution where #566's pre-construction
depend: precheck meets #581 (Follow-up 27.9): registerInstance's
"uninstalled required plugin" refusal releases the instance's
configuration entities before returning false.

Each resolved file equals the gate-3 integration tree's copy (cd1abbbe)
with #570's and #572's own changes reverse-applied.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
wisdommen added a commit that referenced this pull request Oct 3, 2026
Brings #570 up to date with alpha after #559, #581, #561, #564 and #566
merged. Conflicts in FEATURES.md, UAT-CHECKLIST.md, ConfigManager.java
and lang/{en,zh}.json are resolved exactly as the gate-3 integration
merge d36c20bc resolved them (union rows and keys; #570's English
"Configuration save failed! File path: " text applied to the two catch
blocks #581 moved in saveRegisteredEntities). Each resolved file is
byte-equal to the gate-3 integration tree's copy (cd1abbbe); the whole
tree differs from cd1abbbe only by #572's own five files.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DataStoreManager: getDatastore reads dataMap without the lock register holds, so a concurrent read can return null

1 participant