SimpleJsonDataOperator's read paths hand out the instances it keeps in its own cache, so a caller that mutates a returned entity has already changed the store — no update(T) call involved — and the change persists at the next flush.
Measured
getAll(WhereCondition...) returns cache.values() entries directly (new ArrayList<>(cache.values()) on the unfiltered path, collection.add(each) over cache.values() on the filtered one); getById likewise reads the cache. Nothing copies. So:
BanData ban = operator.query().where("player_uuid").eq(uuid).first();
ban.setActive(false); // the store now reports active = false
// no update(...) call at alland JsonStore.flushAllCaches() (scheduled) or destroyAllOperators() (shutdown) writes that mutation to disk. The relational operator behaves the opposite way: getRawListHandler materialises every row through GSON.fromJson, so a mutation there is local until update(T) is called.
The practical effects are:
- a module that mutates a loaded entity and deliberately does not save it still saves it, on JSON only;
- the same code has different persistence semantics on
datasource.type: json and on sqlite/mysql, which is the sort of divergence datasource.type is meant to hide;
- a test cannot distinguish "the service persisted this" from "the service touched the object it was handed" against the JSON backend. That last one was how this surfaced: making a JSON-backed store's
update(T) a no-op changed nothing observable, so a service with no verification at all looked correct.
Suggested fix
Return detached copies from the read paths, as the relational operator effectively does. That has a cost (a Gson round trip per row per read) and would change behaviour for any module currently relying on the aliasing, deliberately or not — so this is a decision, not an obvious fix. Documenting the current semantics on DataOperator's read methods would at least stop it being discovered by accident.
Scope
Framework read-only finding from Phase 17 wave 1 (UltiEssentials, while writing fixtures for UltiKits/UltiEssentials#35). No module in this monorepo is known to depend on either behaviour.
SimpleJsonDataOperator's read paths hand out the instances it keeps in its own cache, so a caller that mutates a returned entity has already changed the store — noupdate(T)call involved — and the change persists at the next flush.Measured
getAll(WhereCondition...)returnscache.values()entries directly (new ArrayList<>(cache.values())on the unfiltered path,collection.add(each)overcache.values()on the filtered one);getByIdlikewise reads the cache. Nothing copies. So:and
JsonStore.flushAllCaches()(scheduled) ordestroyAllOperators()(shutdown) writes that mutation to disk. The relational operator behaves the opposite way:getRawListHandlermaterialises every row throughGSON.fromJson, so a mutation there is local untilupdate(T)is called.The practical effects are:
datasource.type: jsonand onsqlite/mysql, which is the sort of divergencedatasource.typeis meant to hide;update(T)a no-op changed nothing observable, so a service with no verification at all looked correct.Suggested fix
Return detached copies from the read paths, as the relational operator effectively does. That has a cost (a Gson round trip per row per read) and would change behaviour for any module currently relying on the aliasing, deliberately or not — so this is a decision, not an obvious fix. Documenting the current semantics on
DataOperator's read methods would at least stop it being discovered by accident.Scope
Framework read-only finding from Phase 17 wave 1 (UltiEssentials, while writing fixtures for UltiKits/UltiEssentials#35). No module in this monorepo is known to depend on either behaviour.