|
| 1 | +# Proposal: Migrate the Apps-Engine converters to Zod codecs |
| 2 | + |
| 3 | +## Status |
| 4 | + |
| 5 | +In progress — Phase 0 landed (PR #41205). Phases 1–5 pending. |
| 6 | + |
| 7 | +## Problem |
| 8 | + |
| 9 | +`apps/meteor/app/apps/server/converters/` translates data between two models in both |
| 10 | +directions: |
| 11 | + |
| 12 | +- **Rocket.Chat → Apps-Engine** ("to app"): Mongo documents (`IUser`, `IRoom`, `IMessage`, …) |
| 13 | + become Apps-Engine objects (`IAppsUser`, `IAppsRoom`, `IAppsMessage`, …). Methods such as |
| 14 | + `convertToApp` / `convertRoom` / `convertMessage`. |
| 15 | +- **Apps-Engine → Rocket.Chat** ("from app"): the reverse. Methods such as `convertToRocketChat` / |
| 16 | + `convertApp*`. |
| 17 | + |
| 18 | +Today the two directions are implemented by two unrelated mechanisms: |
| 19 | + |
| 20 | +- The **to-app** direction is driven by `transformMappedData`, a declarative field remapper |
| 21 | + (`{ to: 'from' | fn | { from, map, list } }`) that also collects everything it did not map into an |
| 22 | + `_unmappedProperties_` bucket. |
| 23 | +- The **from-app** direction is hand-written object construction with long chains of |
| 24 | + `...(cond && { … })` spreads, re-merging `_unmappedProperties_` at the end. |
| 25 | + |
| 26 | +This split has real costs: |
| 27 | + |
| 28 | +1. **Two sources of truth per entity.** The field mapping is expressed once in a `map` and again, |
| 29 | + inverted, in a hand-written builder. They drift. |
| 30 | +2. **No runtime validation at the boundary.** The from-app path trusts whatever an app sends and |
| 31 | + spreads it onto a Mongo document. |
| 32 | +3. **Weak typing.** Several converters were untyped `.js`; the mapping is stringly-typed and the |
| 33 | + relationship between the two directions is invisible to the compiler. |
| 34 | + |
| 35 | +### Inventory |
| 36 | + |
| 37 | +| Converter | Was | To-app | From-app | Async / DB | Notable hazards | |
| 38 | +|--------------------|-----------|:------:|:--------:|:----------:|-----------------| |
| 39 | +| `settings` | js → ts | ✅ | — | sync | `SettingType` enum | |
| 40 | +| `roles` | ts | ✅ | — | sync | trivial | |
| 41 | +| `videoConferences` | ts | ✅ | ✅ | sync | pass-through clone | |
| 42 | +| `visitors` | js → ts | ✅ | ✅ | sync | `_unmappedProperties_` bucket | |
| 43 | +| `departments` | js → ts | ✅ | ✅ | sync | `_unmappedProperties_` bucket | |
| 44 | +| `users` | js → ts | ✅ | ✅ | sync | `UserType` / `UserStatusConnection` enums, contextual `console.warn` | |
| 45 | +| `uploads` | js → ts | ✅ | ✅ | **async** | cross-converter (rooms/users/visitors) | |
| 46 | +| `rooms` | js → ts | ✅ | ✅ | **async** | cross-converter fan-out, `isPartial`, `secureFieldsMapper`, `RoomType` enum | |
| 47 | +| `messages` | js → ts | ✅ | ✅ | **async** | `WeakMap` + `cachedFunction` memoization, `isPartial`, attachment sub-map, visitor-sender fallback | |
| 48 | +| `threads` | ts | ✅ | — | **async** | duplicates the messages attachment map | |
| 49 | +| `contacts` | ts | ✅ | ✅ | sync | deep nested `list` reverse map | |
| 50 | + |
| 51 | +Infrastructure: `transformMappedData.ts`, `cachedFunction.ts`, `convertMessageFiles.ts`. |
| 52 | + |
| 53 | +## Proposed Solution |
| 54 | + |
| 55 | +Model each entity as a **Zod codec** — a single bidirectional artifact that replaces the split |
| 56 | +"map + hand-written inverse" pair and adds runtime validation. |
| 57 | + |
| 58 | +Zod 4.3 (`~4.3.6`) is already a dependency, and the repo already ships a codec — |
| 59 | +`TimestampSchema` in `packages/core-typings/src/utils.ts`: |
| 60 | + |
| 61 | +```ts |
| 62 | +export const TimestampSchema = z.codec(z.iso.datetime(), z.date(), { |
| 63 | + encode: (date) => date.toISOString(), |
| 64 | + decode: (str) => new Date(str), |
| 65 | +}); |
| 66 | +``` |
| 67 | + |
| 68 | +`z.codec(RcSchema, AppSchema, { decode, encode })` maps directly onto our two directions: |
| 69 | + |
| 70 | +- `decode` : Rocket.Chat → Apps-Engine (today's `convertToApp` / `convertRoom` / …) |
| 71 | +- `encode` : Apps-Engine → Rocket.Chat (today's `convertToRocketChat` / `convertApp*`) |
| 72 | + |
| 73 | +`z.decode(codec, x)` / `z.encode(codec, x)` run them synchronously; `z.decodeAsync` / `z.encodeAsync` |
| 74 | +run them when the transforms are async. |
| 75 | + |
| 76 | +The converter **classes stay** as the public façade (they implement the `IAppXConverter` interfaces |
| 77 | +and are reached via `orch.getConverters().get('x')`, used in ~25 files). Codecs are an internal |
| 78 | +implementation detail those methods delegate to. |
| 79 | + |
| 80 | +## Key design decisions |
| 81 | + |
| 82 | +1. **Async transforms → `z.decodeAsync` / `z.encodeAsync`.** rooms/messages/uploads/threads perform |
| 83 | + DB lookups inside the mapping. Their codec transforms are async and must be driven with the async |
| 84 | + entry points; the sync `z.decode` throws `$ZodAsyncError` on an async codec. Sync converters |
| 85 | + (settings, roles, users, visitors, departments, contacts, videoConferences) use plain |
| 86 | + `z.decode` / `z.encode`. *Validated against the installed Zod build during Phase 0.* |
| 87 | + |
| 88 | +2. **Orchestrator dependency → codec factories.** Cross-converter converters cannot be static |
| 89 | + singletons — they need `orch`. Expose `createRoomCodec(orch)`, `createUploadsCodec(orch)`, etc., |
| 90 | + returning a closure-bound codec. DB-free converters export a static codec constant. |
| 91 | + |
| 92 | +3. **Preserve the `_unmappedProperties_` contract — do not drop it.** It is load-bearing: the reverse |
| 93 | + converters merge it back, the EE redactor (`ee/server/apps/lib/redactor.ts`) references the path, |
| 94 | + and `RoomBridge` reads it. A plain `z.object` strips unknown keys; `z.looseObject` keeps them |
| 95 | + inline but *without* the bucket. We will build a small reusable helper (working name |
| 96 | + `mappedCodec`) that reproduces `transformMappedData`'s bucket semantics exactly, so output is |
| 97 | + byte-identical. This keeps the migration behaviour-preserving rather than a behaviour change. |
| 98 | + The helper is intentionally **not** built up-front — it will be co-designed with the first |
| 99 | + bidirectional converter (Phase 2) to avoid guessing the abstraction. |
| 100 | + |
| 101 | +4. **`isPartial` (rooms/messages from-app) stays in the class method, not the codec.** Partial mode |
| 102 | + skips required-field generation, skips the unmapped merge, and strips `undefined`. Model it as the |
| 103 | + class calling either `z.encode` (full) or a partial path; do not encode the flag into the schema. |
| 104 | + |
| 105 | +5. **Enum conversions → shared codec constants.** `UserType`, `UserStatusConnection`, `RoomType`, |
| 106 | + `SettingType` become small codecs (like `TimestampSchema`) in `converters/codecs/enums.ts`, |
| 107 | + reproducing the current `switch` logic including the pass-through/upper-case fallbacks. |
| 108 | + Contextual `console.warn` calls that need data unavailable to a pure enum mapping (e.g. the |
| 109 | + affected user's id/username in the status-connection warning) stay in the converter layer. |
| 110 | + |
| 111 | +6. **Memoization (messages/threads) stays.** The `WeakMap` + `cachedFunction` dedup of user/room |
| 112 | + lookups within a single conversion is preserved by constructing the message codec per-conversion |
| 113 | + through the factory, passing in the memoized lookups. |
| 114 | + |
| 115 | +7. **Loose validation on the from-app path.** Apps send arbitrary data today; strict schemas would |
| 116 | + reject payloads that currently pass. Use `.loose()` / `.optional()` generously on the app-side |
| 117 | + schemas so the migration introduces no rejections. Tightening is a deliberate, separate follow-up. |
| 118 | + |
| 119 | +8. **Preserve the public surface.** The `IAppXConverter` interfaces and the |
| 120 | + `orch.getConverters().get('x')` usage must not change. |
| 121 | + |
| 122 | +### Direction convention (illustrative) |
| 123 | + |
| 124 | +```ts |
| 125 | +const UserCodec = z.codec(UserRocketChatSchema, AppsUserSchema, { |
| 126 | + decode: (user) => ({ id: user._id, /* … */ }), // convertToApp |
| 127 | + encode: (appUser) => removeEmpty({ _id: appUser.id, /* … */ }), // convertToRocketChat |
| 128 | +}); |
| 129 | + |
| 130 | +// RC -> App: z.decode(UserCodec, user) |
| 131 | +// App -> RC: z.encode(UserCodec, appUser) |
| 132 | +``` |
| 133 | + |
| 134 | +## Migration phases |
| 135 | + |
| 136 | +Each phase is independently shippable, keeps the class façade, and is gated by the golden tests. |
| 137 | + |
| 138 | +### Phase 0 — Scaffolding & de-risk (done, PR #41205) |
| 139 | + |
| 140 | +- Converted the 7 remaining `.js` converters to `.ts`, behaviour-preserving (including the legacy |
| 141 | + `utfOffset` read and throw-on-null paths). `rooms`/`messages` keep loose typing on their transform |
| 142 | + maps for now; that tightens when each is codec-ified. |
| 143 | +- Added `converters/codecs/` with the first shared primitives: bidirectional enum codecs |
| 144 | + (`UserType`, `UserStatusConnection`, `RoomType`, `SettingType`). |
| 145 | +- Added the **behavioural safety net**: enum-codec unit tests (asserting parity with the legacy |
| 146 | + helpers) and **golden-snapshot tests** locking the current RC ↔ Apps-Engine field mapping, |
| 147 | + including `_unmappedProperties_` bucketing, for settings, users, visitors, departments, roles, |
| 148 | + videoConferences, contacts and uploads. |
| 149 | +- Validated the async-codec approach (`z.decodeAsync` / `z.encodeAsync`, `$ZodAsyncError`) that |
| 150 | + Phases 3–4 depend on. |
| 151 | + |
| 152 | +### Phase 1 — Trivial, one-way, sync |
| 153 | + |
| 154 | +`settings`, `roles`, `videoConferences`. Proves the codec + enum-codec pattern end-to-end at the |
| 155 | +lowest risk. |
| 156 | + |
| 157 | +### Phase 2 — Self-contained, bidirectional, sync |
| 158 | + |
| 159 | +`visitors`, `departments`, `users`. Introduces and hardens the `mappedCodec` unmapped-bucket helper |
| 160 | +and exercises the enum codecs with the `console.warn` side effects preserved in both directions. |
| 161 | + |
| 162 | +### Phase 3 — Async, cross-converter |
| 163 | + |
| 164 | +`uploads`, then `rooms`. Introduces the codec-factory pattern, `z.decodeAsync` / `z.encodeAsync`, |
| 165 | +`isPartial` handled in the class layer, and `secureFieldsMapper` integration. |
| 166 | + |
| 167 | +### Phase 4 — Async + memoized + shared sub-schemas |
| 168 | + |
| 169 | +`messages` + `threads` together — extract one shared **attachment codec** to remove the duplicated |
| 170 | +`_convertAttachmentsToApp` map — plus `contacts` (deep nested `list` maps → nested codecs). Highest |
| 171 | +risk; done last. |
| 172 | + |
| 173 | +### Phase 5 — Cleanup |
| 174 | + |
| 175 | +Once every converter is on codecs, delete `transformMappedData.ts` (and relocate/retire its |
| 176 | +importer-located spec) and consolidate `cachedFunction` if memoization has moved into the factories. |
| 177 | + |
| 178 | +## Testing strategy |
| 179 | + |
| 180 | +- **Golden snapshots** (added in Phase 0) are the equivalence oracle: each phase must keep them green |
| 181 | + while swapping internals. Output is compared after `JSON.parse(JSON.stringify(...))` so Dates |
| 182 | + normalise to ISO strings and `undefined` fields drop — matching how payloads cross the app bridge. |
| 183 | +- **Enum-codec unit tests** assert `decode`/`encode` match the legacy `_convert*` helpers for every |
| 184 | + input, including fallbacks. |
| 185 | +- The existing `rooms.tests.ts` and `messages.tests.js` remain the oracle for the two hardest |
| 186 | + converters. |
| 187 | +- New codecs get their own focused tests as they are introduced. |
| 188 | + |
| 189 | +## Risks |
| 190 | + |
| 191 | +- **`_unmappedProperties_` fidelity** — the single biggest behavioural trap; the `mappedCodec` helper |
| 192 | + plus golden tests are the mitigation. |
| 193 | +- **From-app validation rejecting live app payloads** — mitigated by loose schemas; do not tighten |
| 194 | + during the migration. |
| 195 | +- **Async codec ergonomics** — de-risked in Phase 0, but the factory wiring for rooms/messages should |
| 196 | + still be spiked before committing to Phase 3. |
| 197 | +- **Test rewrite** — the existing tests use `proxyquire.noCallThru()` against module paths; codec-ing |
| 198 | + each converter means updating those loaders. |
| 199 | + |
| 200 | +## Non-goals |
| 201 | + |
| 202 | +- Changing the `IAppXConverter` interfaces or any app-facing behaviour. |
| 203 | +- Tightening the from-app validation surface (tracked as a separate follow-up). |
| 204 | +- Migrating converters that are not in the directory above. |
0 commit comments