|
| 1 | +--- |
| 2 | +"@objectstack/plugin-audit": minor |
| 3 | +--- |
| 4 | + |
| 5 | +feat(plugin-audit)!: retire `sys_comment.visibility` and `sys_comment.reply_count` (#4756, ADR-0049) |
| 6 | + |
| 7 | +Both fields were modelled with **zero** runtime consumers — nothing in this repo, |
| 8 | +in `objectui`, or in `cloud` ever read or maintained either one. ADR-0049 |
| 9 | +enforce-or-remove; maintainer decision: remove both. Same disposition, and for |
| 10 | +the same stated reason, as `sys_attachment.share_type` / `sys_attachment.visibility` |
| 11 | +in #2755 ("attachment access is derived from the parent record"). |
| 12 | + |
| 13 | +**REMOVED — `sys_comment.visibility`** (`'public' | 'internal' | 'private'`, |
| 14 | +defaulted `'public'`). |
| 15 | + |
| 16 | +This one is a **security-looking key with no gate behind it**, which is the |
| 17 | +primary reason it goes rather than stays. No code path consulted it: not |
| 18 | +`enforceFeedsCapability`, not the record-level gates added in #4630, not the |
| 19 | +REST layer, not objectui's discussion panel. A comment an author marked |
| 20 | +`private` was visible to exactly the same people as a `public` one — an app |
| 21 | +author (or an AI authoring metadata) reading the field list would reasonably |
| 22 | +believe otherwise, and get a silent security failure instead of an error. That |
| 23 | +is the Prime Directive #10 trap in its textbook shape. |
| 24 | + |
| 25 | +There is **no replacement key**: after #4630, who can see a comment is decided |
| 26 | +by the record-level permissions of the record its `thread_id` names — one |
| 27 | +coherent rule. A per-row enum layered on top would be a second source of truth |
| 28 | +for the same question. The enum's only genuinely missing meaning ("hidden from |
| 29 | +external/portal principals") depends on external principals existing at all, |
| 30 | +which waits on ADR-0090 D11's `externalSharingModel`; today there is nobody to |
| 31 | +hide a comment from. This does not foreclose that design — when portals land, |
| 32 | +a visibility key can return **enforce-first**, with a real gate and tests. |
| 33 | + |
| 34 | +**FROM → TO:** stop sending `visibility` on `sys_comment` writes; to restrict |
| 35 | +who sees a discussion, restrict who can read the record `thread_id` points at. |
| 36 | + |
| 37 | +**REMOVED — `sys_comment.reply_count`** (`number`, `defaultValue: 0`, |
| 38 | +`readonly: true`). |
| 39 | + |
| 40 | +Never incremented anywhere, and `readonly` meant an author could not set it by |
| 41 | +hand either, so every row read `0` forever — a UI binding an "N replies" badge |
| 42 | +to it rendered `0` for every thread. Deliberately **not** replaced by an |
| 43 | +`afterInsert`/`afterDelete` roll-up: the predicate/bulk write-hook gaps tracked |
| 44 | +by #4770 / #4778 / #4779 (a hook that returns early without a single-record id |
| 45 | +lets the whole bulk operation through) are exactly where a hook-maintained |
| 46 | +counter drifts — a bulk delete of replies would never decrement it. A counter |
| 47 | +that drifts is worse than no counter, because both the UI and an AI reading the |
| 48 | +record trust it. If a badge needs the number, aggregate `parent_id` children at |
| 49 | +read time; a designed roll-up can be revisited once #4775's family has settled |
| 50 | +bulk-hook semantics. |
| 51 | + |
| 52 | +**FROM → TO:** replace reads of `reply_count` with a count of `sys_comment` rows |
| 53 | +whose `parent_id` is the comment's id. |
| 54 | + |
| 55 | +**Stored data.** Existing databases keep both columns as **unmanaged leftovers** |
| 56 | +— no migration, matching #2755. What changes where: |
| 57 | + |
| 58 | +- **Reads are loud everywhere.** The read-axis gates (#4134 / #4226) resolve |
| 59 | + field names from the object schema, not from the table, so a filter, sort, |
| 60 | + `select` or `expand` naming `visibility` / `reply_count` now answers |
| 61 | + `400 INVALID_FIELD` on every deployment, leftover column or not. A "0 replies" |
| 62 | + badge that silently lied becomes an error that names itself. |
| 63 | +- **Writes are loud on new databases only.** A database provisioned after this |
| 64 | + change has no such column, so the write fails at the driver and is mapped to |
| 65 | + the same `400 INVALID_FIELD` envelope. On a pre-existing database the leftover |
| 66 | + column still accepts a value nothing will ever read — record validation does |
| 67 | + not reject undeclared keys. Dropping the two columns is an optional manual |
| 68 | + cleanup, not a requirement. |
0 commit comments