diff --git a/.changeset/platform-iana-timezone-columns.md b/.changeset/platform-iana-timezone-columns.md new file mode 100644 index 0000000000..3459267763 --- /dev/null +++ b/.changeset/platform-iana-timezone-columns.md @@ -0,0 +1,19 @@ +--- +'@objectstack/platform-objects': minor +--- + +feat(platform-objects): `sys_job.timezone` and `sys_report_schedule.timezone` are validated against the IANA domain (#15872) + + + +The platform's two oldest IANA time-zone columns predate `valueDomain` and disagreed with each other in three dimensions at once — length (100 vs 64), default (none vs `'UTC'`) and validation (neither). This closes the third: both now declare `valueDomain: 'iana_time_zone'`, the same declaration and the same `Intl.DateTimeFormat` membership probe that `sys_business_unit.timezone` and `sys_organization.timezone` carry (#14238). Four columns, one spelling of "is this a real zone". + +**What it was worth, measured before the fix rather than assumed.** The two columns are not equally exposed, and only one of them was dangerous. + +- `sys_report_schedule.timezone` is read back and handed to a scheduler. `ReportService.nextRunAt` calls `new Cron(cron, { timezone }).nextRun(from)`, and croner does not reject a non-member zone at construction when there is no callback — it throws from `nextRun()`. That throw was caught and turned into a fall back to `interval_minutes`. So a typo'd zone silently discarded the cron expression: an admin's "every weekday 09:00 Asia/Shanghai" became "every 1440 minutes, forever", logged only as `invalid cron ''` — a warning naming the wrong input, because the expression was fine. Not a throw and not a fall back to UTC: the wrong instant, permanently. Refusing the write is what closes it. (`scheduleReport`'s eager create-time guard did not catch it either: it constructs a callback-less `Cron` and is blind to exactly this half of its own input. That is a separate defect in `plugin-reports`, carded, not fixed here.) +- `sys_job.timezone` is written and never read. `DbJobAdapter` mirrors the in-memory schedule onto the row; its three `sys_job` read sites take `id` / `run_count` / `failure_count` only. The zone the scheduler honours never travels through this column, and `DbJobAdapter.schedule` awaits the cron adapter before it upserts the row, so a non-member cannot even reach the column that way — croner constructed WITH a callback throws, and `AppPlugin` reports it as `Background job FAILED TO SCHEDULE — it will never run`. The door this declaration closes there is the other one: a direct write from Studio, REST or a script, which had no validation at all. + +**What is deliberately NOT converged**, and is pinned so that staying unconverged is a decision rather than a drift someone repairs by reflex: + +- **the defaults still differ.** A default here is a consumer semantic, not a shape question. `sys_report_schedule` documents and implements a UTC default; `sys_job` has no reader at all, and minting one would change what an unset row means. +- **the bounds still differ (100 vs 64).** `maxLength` is not only a write bound — it reaches DDL, and narrowing a physical `varchar(100)` is `driver-sql`'s `narrow_varchar` op at severity `error`, category destructive ("narrowing may truncate"). What the column physically holds in a deployment is not readable from the repo, so the convergence is a separate decision and #15872 stays open on it. Note what the domain declaration already costs the wider bound: no member is longer than 32 characters on the current Node baseline, so 100 now admits nothing 64 would not. diff --git a/packages/platform-objects/src/audit/platform-iana-timezone-columns.test.ts b/packages/platform-objects/src/audit/platform-iana-timezone-columns.test.ts new file mode 100644 index 0000000000..c96f9948d2 --- /dev/null +++ b/packages/platform-objects/src/audit/platform-iana-timezone-columns.test.ts @@ -0,0 +1,121 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #15872 — the platform's two OLDER IANA columns, `sys_job.timezone` and + * `sys_report_schedule.timezone`, predate `valueDomain` and disagreed with each + * other in three dimensions at once (length 100 vs 64, default none vs `'UTC'`, + * validation none vs none). This file pins what that card actually changed and, + * just as deliberately, what it did NOT. + * + * CLOSED here — validation. Both columns now declare + * `valueDomain: 'iana_time_zone'`, the same declaration the ruled pair + * `sys_business_unit.timezone` / `sys_organization.timezone` carries (#14238, + * pinned in `identity/org-hierarchy-timezone.test.ts`). Four columns, one + * membership predicate. + * + * LEFT ALONE, and pinned so that staying alone is a decision rather than a + * drift someone repairs by reflex: + * + * - the DEFAULTS still differ, because a default here is a CONSUMER semantic, + * not a shape question. `sys_report_schedule` documents "default UTC" and its + * reader falls back to `'UTC'`; `sys_job` says nothing, and giving it one + * would change what an unset row means. The ruled pair, for its own reasons, + * has none on either column — so "all four agree" is NOT the invariant, and + * a test asserting it would be asserting a bug. + * - the BOUNDS still differ (100 vs 64). `maxLength` is not only a write bound: + * it reaches DDL, and narrowing a physical `varchar(100)` is `driver-sql`'s + * `narrow_varchar` op at severity `error`, category destructive. What the + * column physically holds in a deployment is not readable from the repo, so + * the convergence is a separate decision and #15872 stays open on it. + * + * The reader measurement that decided the card's severity is recorded beside + * each declaration, not here: the `sys_job` column is written and never read, + * while the `sys_report_schedule` column is read back into croner by + * `ReportService.nextRunAt`, whose catch turned a non-member zone into a silent + * fall back to `interval_minutes` — the wrong instant, forever. + */ + +import { describe, it, expect } from 'vitest'; +import { isValueDomainMember } from '@objectstack/spec/shared'; +import { SysJob } from './sys-job.object'; +import { SysReportSchedule } from './sys-report-schedule.object'; + +type ColumnShape = { + type?: unknown; + required?: unknown; + maxLength?: unknown; + valueDomain?: unknown; + defaultValue?: unknown; +}; + +const jobColumn = () => (SysJob.fields as Record).timezone; +const scheduleColumn = () => (SysReportSchedule.fields as Record).timezone; + +describe('#15872 — the platform\'s two older IANA time-zone columns', () => { + it('reads the real declarations, not an empty probe', () => { + // Vacuity control: a renamed column or a changed export would otherwise let + // every assertion below pass over `undefined`. + expect(SysJob.name).toBe('sys_job'); + expect(SysReportSchedule.name).toBe('sys_report_schedule'); + expect(jobColumn()).toBeTypeOf('object'); + expect(scheduleColumn()).toBeTypeOf('object'); + }); + + it.each([ + ['sys_job', jobColumn], + ['sys_report_schedule', scheduleColumn], + ])('%s.timezone is an optional text column validated against the IANA domain', (_object, column) => { + const c = column(); + // `VALUE_DOMAIN_FIELD_TYPES` is `{text}`, so the declaration below is also + // the reason the type must stay `text`. + expect(c.type).toBe('text'); + expect(c.required).toBe(false); + expect(c.valueDomain).toBe('iana_time_zone'); + }); + + it('the DEFAULTS deliberately still differ — a default here is a consumer semantic', () => { + // ⛔ Not a tidy-up target. `sys_report_schedule`'s reader documents and + // implements a UTC default; `sys_job` has no reader at all, and minting one + // would give "unset" a new meaning on rows that predate it. + expect(scheduleColumn().defaultValue).toBe('UTC'); + expect('defaultValue' in jobColumn()).toBe(false); + }); + + it('the BOUNDS deliberately still differ — converging them is a DDL question, not a shape one', () => { + // If someone converges these, they owe the reading #15872 could not take: + // what the physical column holds. Red here is the prompt to go and take it. + expect(jobColumn().maxLength).toBe(100); + expect(scheduleColumn().maxLength).toBe(64); + }); + + it('the declared domain refuses every non-member this card was filed over', () => { + // Asked of the predicate the write path calls (`isValueDomainMember`) under + // the domain the columns actually declare — never a re-implementation. + const domain = jobColumn().valueDomain as 'iana_time_zone'; + expect(domain).toBe(scheduleColumn().valueDomain); + // The card's own three examples. `Mars/Olympus` is shape-valid and + // nonexistent, `UTC+8` and `China Standard Time` are the two spellings a + // human reaches for that the tzdb does not carry. + expect(isValueDomainMember(domain, 'Mars/Olympus')).toBe(false); + expect(isValueDomainMember(domain, 'UTC+8')).toBe(false); + expect(isValueDomainMember(domain, 'China Standard Time')).toBe(false); + // …and still admits what both columns must keep taking, `UTC` included — + // which `Intl.supportedValuesOf('timeZone')` omits, so a column judged + // against the enumeration would refuse `sys_report_schedule`'s own default. + expect(isValueDomainMember(domain, 'UTC')).toBe(true); + expect(isValueDomainMember(domain, 'Asia/Shanghai')).toBe(true); + expect(isValueDomainMember(domain, scheduleColumn().defaultValue as string)).toBe(true); + }); + + it('both bounds admit every zone the runtime enumerates, so neither refuses a legal value', () => { + // The smaller bound is the one that could bite; assert against both so a + // future ICU that enumerates a longer name reds here rather than silently + // refusing a legal zone at the write seam. + // `Intl.supportedValuesOf` is ES2022; the package's `lib` predates it, so + // the call is typed here rather than the whole program's lib widened. + const intl = Intl as unknown as { supportedValuesOf(key: 'timeZone'): string[] }; + const longest = Math.max(...intl.supportedValuesOf('timeZone').map((z) => z.length)); + expect(longest).toBeLessThanOrEqual(scheduleColumn().maxLength as number); + expect(longest).toBeLessThanOrEqual(jobColumn().maxLength as number); + }); +}); diff --git a/packages/platform-objects/src/audit/sys-job.object.ts b/packages/platform-objects/src/audit/sys-job.object.ts index 99af71f0c5..f2db10eac5 100644 --- a/packages/platform-objects/src/audit/sys-job.object.ts +++ b/packages/platform-objects/src/audit/sys-job.object.ts @@ -61,10 +61,44 @@ export const SysJob = ObjectSchema.create({ group: 'Schedule', }), + // [#15872] Validated on write by `valueDomain: 'iana_time_zone'` — the same + // declaration `sys_business_unit.timezone` / `sys_organization.timezone` + // carry (#14238), and the same shared `Intl.DateTimeFormat` probe, never the + // `Intl.supportedValuesOf('timeZone')` enumeration (which omits `UTC`). + // Written values only: the `min`/`max`/`maxLength` transition-gate class, so + // a stored non-member is never re-read and no migration is owed. + // + // WHAT READS THIS COLUMN, measured on #15872 before the declaration was + // added, because it decides what the declaration is worth: NOTHING does. + // `DbJobAdapter` writes it (`upsertJobRow`, `schedule.timezone ?? null`) and + // its three `sys_job` read sites take `id` / `run_count` / `failure_count` + // only — the tree's one `row.timezone` read belongs to `sys_report_schedule`. + // The value the scheduler actually honours travels in memory + // (`toBoundaryJobSchedule` -> `CronJobAdapter.schedule` -> croner), and + // `DbJobAdapter.schedule` awaits that call BEFORE `upsertJobRow`, so a + // non-member cannot reach this column through the scheduler at all: croner + // constructed WITH a callback throws on a non-member zone, `AppPlugin` + // catches it per job as `Background job FAILED TO SCHEDULE — it will never + // run` (error + `jobScheduleFailuresTotal`), and the row is never written. + // The door this declaration actually closes is the OTHER one: a direct write + // to the object (Studio, REST, a script), which had no validation whatever. + // + // ⚠️ `maxLength` deliberately still says 100 while `sys_report_schedule` + // says 64. Converging it is the card's third dimension and is NOT landed + // here: `maxLength` is not only a write bound, it reaches DDL — narrowing a + // physical `varchar(100)` produces `driver-sql`'s `narrow_varchar` op at + // severity `error`, category destructive ("narrowing may truncate", + // `os migrate apply --allow-destructive`). What this column physically holds + // in a deployment cannot be read from the repo, and 「IANA names are short」 + // is an argument about the domain, not a reading of the data. Left to a + // separate decision (#15872 stays open on that row). Note what the line + // above already costs it: no `valueDomain` member is longer than 32 + // characters on this Node baseline, so 100 now admits nothing 64 would not. timezone: Field.text({ label: 'Timezone', required: false, maxLength: 100, + valueDomain: 'iana_time_zone', group: 'Schedule', }), diff --git a/packages/platform-objects/src/audit/sys-report-schedule.object.ts b/packages/platform-objects/src/audit/sys-report-schedule.object.ts index 3fd6a71f22..8ce0c68073 100644 --- a/packages/platform-objects/src/audit/sys-report-schedule.object.ts +++ b/packages/platform-objects/src/audit/sys-report-schedule.object.ts @@ -78,11 +78,42 @@ export const SysReportSchedule = ObjectSchema.create({ group: 'Schedule', }), + // [#15872] Validated on write by `valueDomain: 'iana_time_zone'` — the same + // declaration `sys_business_unit.timezone` / `sys_organization.timezone` + // carry (#14238), and the same shared `Intl.DateTimeFormat` probe, never the + // `Intl.supportedValuesOf('timeZone')` enumeration (which omits `UTC`, this + // column's own default). Written values only (the `min`/`max`/`maxLength` + // transition-gate class), so a stored non-member survives and no migration + // is owed. + // + // WHY THIS COLUMN IS THE SHARP ONE, measured on #15872: unlike + // `sys_job.timezone`, this value IS read back and handed to a scheduler. + // `ReportService.rowFromSchedule` lifts it off the row and `nextRunAt` calls + // `new Cron(cron, { timezone }).nextRun(from)`. croner (10.0.1) does not + // reject a non-member zone when it is constructed WITHOUT a callback — it + // throws from `nextRun()` — and `nextRunAt` CATCHES that throw and falls + // back to `from + interval_minutes`. So before this line, a typo'd zone on a + // cron schedule silently discarded the cron: an admin's "every weekday 09:00 + // Asia/Shanghai" became "every 1440 minutes, forever", logged only as + // `invalid cron ''` — a warning that names the wrong input, since the + // expression was fine. Neither a throw nor a fall back to UTC: the wrong + // instant, permanently, which is the outcome this card was told to escalate + // on. `scheduleReport`'s eager create-time guard does not catch it either; + // it constructs a callback-less `Cron` and so is blind to exactly this half + // of its own input. Refusing the write is what closes it. + // + // `maxLength: 64` and `defaultValue: 'UTC'` are BOTH unchanged. The bound is + // already the value #14238 justified (twice the domain's real ceiling: the + // enumeration's longest name is 30 characters on this Node baseline, the + // longest tzdb link 32). The default is a consumer semantic — this reader + // documents "default UTC" and falls back to `'UTC'` in four places — and is + // deliberately NOT converged with `sys_job`, which has none. timezone: Field.text({ label: 'Timezone', required: false, maxLength: 64, defaultValue: 'UTC', + valueDomain: 'iana_time_zone', group: 'Schedule', }), diff --git a/packages/platform-objects/src/identity/org-hierarchy-timezone.test.ts b/packages/platform-objects/src/identity/org-hierarchy-timezone.test.ts index 6beb1caa4f..5ca2cf4de5 100644 --- a/packages/platform-objects/src/identity/org-hierarchy-timezone.test.ts +++ b/packages/platform-objects/src/identity/org-hierarchy-timezone.test.ts @@ -20,8 +20,11 @@ * no default on either. The card's thesis is that every author invents this * column differently, and the platform's own two precedents * (`sys_job.timezone`: 100, no default; `sys_report_schedule.timezone`: 64, - * default `UTC`; neither validated) already disagree in three dimensions. - * The ruled pair must not become a third and a fourth spelling; + * default `UTC`) disagreed in three dimensions. #15872 closed the third by + * giving both the same `valueDomain` this pair carries — pinned next to + * them in `audit/platform-iana-timezone-columns.test.ts` — and left length + * and default alone. The ruled pair must not become a third and a fourth + * spelling; * 3. the declared domain admits `UTC`, the fallback the contract names for a * wholly unset chain, and the declared bound admits every zone the runtime * enumerates. Why the first is not automatic — `Intl.supportedValuesOf` diff --git a/packages/platform-objects/src/identity/sys-business-unit.object.ts b/packages/platform-objects/src/identity/sys-business-unit.object.ts index d94825d81a..19d606670f 100644 --- a/packages/platform-objects/src/identity/sys-business-unit.object.ts +++ b/packages/platform-objects/src/identity/sys-business-unit.object.ts @@ -167,8 +167,9 @@ export const SysBusinessUnit = ObjectSchema.create({ // pairs a bound with the `UTC` default; the enumeration's longest name on // the repo's Node baseline is 30 characters and the tzdb caps each path // component at 14, so 64 is twice the domain's real ceiling and the smaller - // of the two precedents (`sys_job.timezone` says 100, neither declares a - // domain — the residue card). No `defaultValue`, deliberately: an explicit + // of the two precedents (`sys_job.timezone` still says 100 — #15872 gave + // both precedents this same `valueDomain` and deliberately left that one + // bound unconverged). No `defaultValue`, deliberately: an explicit // default here would mean "stop inheriting", which is the opposite of what // an unset unit means. timezone: Field.text({ diff --git a/packages/platform-objects/src/identity/sys-organization.object.ts b/packages/platform-objects/src/identity/sys-organization.object.ts index f5878cab70..2f216ddbe1 100644 --- a/packages/platform-objects/src/identity/sys-organization.object.ts +++ b/packages/platform-objects/src/identity/sys-organization.object.ts @@ -252,9 +252,10 @@ export const SysOrganization = ObjectSchema.create({ // `sys_business_unit.timezone` by design (`text`, optional, `maxLength: 64`, // `valueDomain: 'iana_time_zone'`, no default): the card's thesis is that // every author invents this column differently, and the platform's own two - // precedents (`sys_job`, `sys_report_schedule`) already disagree on length, - // default and validation — the ruled pair is one spelling, pinned in - // `org-hierarchy-timezone.test.ts`. + // precedents (`sys_job`, `sys_report_schedule`) disagreed on length, default + // AND validation — #15872 closed the validation dimension by giving both + // this same `valueDomain`, and left the other two as each reader expects. + // The ruled pair is one spelling, pinned in `org-hierarchy-timezone.test.ts`. timezone: Field.text({ label: 'Timezone', required: false,