Canonicalise chat entity references on eNames - #1131
Closed
Sahil2004 wants to merge 13 commits into
Closed
Conversation
…ique
Chat participants/admins and a message's sender used to be named by the id of
the referent's User profile MetaEnvelope. That only worked because two
implementations agreed on it, and it has no answer for the bootstrap case: a
user whose eVault holds no profile envelope yet has no envelope id to emit, so
a producer had to either block on provisioning one or quietly emit something
else. An eName exists from the moment the eVault does.
Adds an __ename() mapping directive so the shape of an entity reference lives
in one place instead of being hand-parsed per platform, and a TTL profile cache
to keep display-name hydration from becoming an N+1 now that a reference no
longer dereferences to a full User record.
Pictique's webhook previously ran ref.split("(")[1].split(")")[0] on every
participant, which throws a TypeError on a bare eName and took ingest down for
the whole envelope. Participants that cannot be resolved are now skipped and
logged: members may legitimately live on a platform this instance knows nothing
about, and one such member must not cost the room its other members.
mapChatData had no guard at all: it ran p.split("(")[1].split(")")[0] over
every participant, so one bare eName threw a TypeError and lost the whole room.
That is the likely root cause of Blabsy chats not syncing.
A Blabsy user document is already keyed by the user's eName, so a participant
reference needs no lookup once it is an eName; it is the local document id
already. The mapping now emits eNames directly instead of round-tripping doc
ids through the mapping store into envelope ids.
The chat reference on a message stays a local relation in table(id) form, since
it resolves through the mapping store rather than by eName, but its parse is
guarded too. A message with no resolvable chat is skipped rather than written
to a path built from the string "null".
Chat and message envelope mapping moves out of the controller so it can be
tested without a Firestore connection.
The Chat/Group schema (…440003) and Message schema (…440004) are shared by far
more than the three platforms the change started with. ereputation, esigner,
file-manager, ecurrency, dreamsync, evoting, cerberus, and group-charter-manager
all carried the same hand-rolled ref.split("(")[1].split(")")[0], and so all had
the same crash on a bare eName.
Participants, admins, members, a group's owner, and a message's sender are now
resolved through one shared helper that skips and logs what it cannot resolve.
Only entity references changed: a chat, group, file, or signature reference is a
local relation resolved through the mapping store, and keeps the table(id) form.
owner and admins needed producer-side work. Unlike participants they are stored
as bare local user ids with no relation for the mapping to follow, so they are
rewritten to eNames just before a group reaches the mapper, and resolved back to
local ids on the way in.
Cerberus keeps its per-lookup timeout, which now expresses itself as one skipped
participant rather than as a hung webhook.
…ed mappings There was no written spec saying what a chat entity reference contains. The envelope-id convention was two implementations agreeing, which is how the two sides drifted apart without anything failing loudly. The Chat and Message schemas now say eName, with a pattern, and the Chat schema declares the admins, owner, and ename fields that platforms were already emitting. The mapping guide taught the envelope-id form by example; it now documents __ename() and the difference between a reference to a record and a reference to a person. The access-control docs marked both reference shapes as equally current, which is no longer true: eNames are canonical, and profile ids are read only because records written before that convention are still at rest. The new test loads the mapping files the platforms actually ship rather than fixtures, so a producer and a consumer cannot drift apart again without a test failing. Reverting any single mapping to the envelope-id form fails it.
Under the envelope-id scheme a participant reference dereferenced straight to a full User record, so display names and avatars arrived free as part of the mapping. An eName carries identity but no profile data, so each one is now a separate read. Blabsy's chat surfaces fetched those profiles one at a time in a sequential loop, re-running on every render, across the chat list, the chat window, the member list, the settings pane, and add-members. That was already wasteful and becomes the change's real cost if left alone. Profiles are now loaded through one cache: reads run in parallel, a concurrent burst for the same eName collapses into a single read, and the current user is served from what the caller already holds. Misses are cached too, since a participant on a platform this instance knows nothing about is a stable condition rather than something to retry on every render. A failed read is not cached, so an offline blip does not persist for the whole TTL. Adds the @types/jest the client was already missing.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Two producer-side bugs, both silent, both found by probing the emit path directly rather than trusting the consumer tests. First: enrichGroupOwnership flattened admins to strings. Six platforms store admins as a User[] relation and their mappings read admins[].ename, so flattening left the mapping asking for .ename on a string and emitting an empty list. Every group would have replicated with no admins. A relation is now left untouched, and only a list of bare local ids is rewritten. Second: the enrichment ran in each watcher's enrichEntity, which several producer paths never call — junction-table changes, debounced group webhooks, and backfill scripts all reach handleChange directly. Those paths emitted a bare local id as the owner. The enrichment now lives in handleChange, behind an optional resolveEnameByUserId hook, because that is the one point every producer path passes through; enriching anywhere else means each new call site is another chance to emit a local id unnoticed. Also stops idToEName falling back to the input when a lookup finds nothing. That turned an unresolved local id into "@<uuid>": syntactically a valid eName and semantically nobody, which is the same class of silently-wrong reference this change exists to remove. An unresolved owner is now null and skipped. The shipped-mappings suite only exercised participants, which is why it missed the admins bug. It now builds every entity field in each platform's own local shape and runs the full producer path; reintroducing the flattening fails it on seven platforms. A new handleChange test covers the junction and backfill paths end to end.
The Chat/Group schema declared eleven properties with additionalProperties set to false, while the shipped mappings emit nine more: adminIds, memberIds, description, charter, isPrivate, visibility, signatureIds, originalMatchParticipants, and cerberus's eName casing. The Message schema was missing isSystemMessage and required senderId, which a system message by definition does not have. A published schema that disagrees with the mappings is how the original drift happened: the identifier convention lived only in two implementations agreeing, with nothing written down to contradict. Leaving the schema wrong in new ways while fixing it in one way would preserve exactly that failure mode. adminIds and memberIds are entity references and are declared as eNames. signatureIds and chatId are references to records, resolved through the id mapping, and are deliberately left without an eName pattern. The new test derives the expected field set from the mapping files themselves, so a mapping that emits something undeclared fails, as does an entity reference declared as a uuid.
The existing tests exercised the mapper and the resolvers directly. That cannot establish the thing this change is about: the bug was a producer and a consumer disagreeing about a wire format, and a disagreement like that only shows up when a whole envelope crosses the boundary and either lands in the database or does not. Pictique: an HTTP POST to /api/webhook, through the real controller, adapter and mapping files, into a real Postgres via the real TypeORM entities. Blabsy: an envelope through the real controller into a real Firestore emulator, with the documents read back out. Both suites cover the task's stated criteria against those real paths — an eName-only room ingests, a malformed entry does not lose the room, a message is attributed to the eName that sent it, display names still resolve — and both were confirmed non-vacuous by restoring the pre-fix parsing, which fails five of six cases in each. Running them surfaced something the unit tests could not: with the pre-fix code Pictique's webhook did not fail, it hung for the full 60s timeout. The catch-all logged the TypeError and never sent a response, so a crash in this handler was indistinguishable from a slow peer. It now answers 500. That is why an eVault could drop chats "with no error" — there was no error to see, only silence. Pictique needs unplugin-swc because esbuild, vitest's default transform, does not emit the decorator metadata TypeORM entities require. Blabsy starts the Firestore emulator from globalSetup, which needs a JRE and says so plainly when one is missing.
This is the platform where owner and admins are bare local user ids in plain columns rather than a relation the mapping can follow, so it exercises the part of the change with the most room to be wrong: an eName has to resolve back to a local id on the way in, and that local id has to leave as an eName on the way out. The outbound case calls handleChange directly, the way a junction-table change or a backfill script does, bypassing the watcher entirely. Removing the enrichment makes it emit "@<local-uuid>" — a syntactically valid eName naming nobody — and the test fails on exactly that.
It had no call sites. Only Blabsy actually needed per-eName profile hydration, because its chat surfaces read participants one document at a time; that cache lives in the client and is wired into all five of those components. Every other platform stores participants as a TypeORM relation, so display names arrive through a SQL join and there is nothing to cache. A generic cache in the shared adapter was an abstraction written for a caller that never existed, and its tests only proved it was self-consistent.
Running either API locally against a real Firestore or Postgres also starts the producer side: Blabsy's watchers and Pictique's TypeORM subscriber replicate every change to the eVaults named by PUBLIC_REGISTRY_URL, which is a shared environment. A developer starting the app to look at it does not necessarily expect it to publish there. That matters more than usual right now. This branch changes the format of chat entity references, and the rollout has a required order: consumers must accept eNames before any producer emits them. A local Blabsy pointed at the shared registry would emit the new format from an unreviewed branch, ahead of the deployment that teaches the other platforms to read it. BLABSY_DISABLE_EVAULT_SYNC and PICTIQUE_DISABLE_EVAULT_SYNC turn the producer off while leaving inbound webhooks working, so the apps stay usable for local work. Both are documented in .env.example and default to off, so nothing about deployed behaviour changes.
This reverts commit f9b9675.
Contributor
Author
|
Not needed. This was not the intended functionality. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Chat entity references are now eNames, everywhere, in both directions.
The Chat schema's
participantIds/participants,adminsandowner, and the Message schema'ssenderId, all carry an entity reference. That reference used to be the id of the referent's User profile MetaEnvelope. It is now the@-prefixed W3ID, e.g.@48468c9a-dc1b-5663-92fb-5e46e3d2a7f0.Why
An envelope id only worked because two implementations agreed on it, and it has no answer for the bootstrap case. A user whose eVault holds no profile envelope yet has no envelope id to emit, so a producer had to either block on provisioning one or quietly emit something else. The Nextcloud plugin took the second option: it wrote eNames whenever the envelope was missing while reading only envelope ids, so those chats replicated correctly and were then dropped on ingest with no error. An eName exists from the moment the eVault does.
Scope
The task named three platforms. The Chat/Group schema (
…440003) and Message schema (…440004) are shared by ten platforms in this repo, and every one of them carried the same hand-rolledref.split("(")[1].split(")")[0], which throws aTypeErroron a bare eName:pictique, blabsy, ereputation, esigner, file-manager, ecurrency, dreamsync, evoting, cerberus, group-charter-manager.
Fixing three would have left seven crashing on the first eName Nextcloud sent.
How
A
__ename()mapping directive inweb3-adapter. Entity references are declared in the mapping rather than parsed by hand in each webhook, so their shape lives in one place and can only change in one place. OntoGlobalvalues are emitted@-prefixed; onfromGlobalthey come back as eNames with unusable entries dropped. Whether a field is a list is decided by the mapping, not by whatever arrived, so a participant list that shows up asnullis an empty list rather than a scalar that downstream code then iterates.Unresolvable participants are skipped and logged, never fatal. This matters independently of the eName change and was its own bug. Members may legitimately live on a platform this instance knows nothing about, and one of them must not cost the room its other members.
Only entity references changed. A chat, group, file, poll or signature reference is a reference to a record, resolved through the mapping store, and keeps the
table(id)form. Those parses were guarded but not converted.ownerandadminsneeded producer-side work. Unlike participants they are stored as bare local user ids with no relation for the mapping to follow, so they are rewritten to eNames just before a group reaches the mapper and resolved back to local ids on the way in.Display names and avatars. Under the old scheme a reference dereferenced straight to a full User record, so profile data came free. Only Blabsy actually needed a cache: its chat surfaces read participants one document at a time, in a sequential loop, on every render, across five components. That is now a single cache with parallel reads, request collapsing and cached misses. Every other platform stores participants as a TypeORM relation, so display names arrive through a SQL join and never regressed — no cache needed, and none added.
The convention is now written down. The ontology schemas said
format: uuid; they now say eName, with a pattern, and the Chat schema declares theadmins,ownerandenamefields platforms were already emitting. The mapping guide taught the envelope-id form by example. The access-control docs presented both reference shapes as equally current; eNames are canonical, and profile ids are read only because records written before this are still at rest in eVaults — the ACL resolver still accepts both on read deliberately, and that is called out as legacy rather than removed.Verification
179 tests. 17 of them are acceptance tests that drive the real path, not the mapper in isolation:
pictique/.../webhook-chat.acceptance.test.tsPOST /api/webhook→ realWebhookController→ real adapter + shipped mappings → real Postgres (testcontainers) → rows read back through the real TypeORM entitiesblabsy/.../webhook-chat.acceptance.test.tsWebhookController→ real Firestore emulator → documents read backgroup-charter-manager/.../group-replication.acceptance.test.tshandleChangecall that bypasses the watcherEach of the task's stated criteria is checked on those real paths — an eName-only room ingests, a malformed entry does not lose the room, a message is attributed to the eName that sent it, display names still resolve, an unresolvable member is skipped rather than fatal.
Non-vacuity, proven per suite by restoring the pre-fix code: Pictique 5 of 6 fail, Blabsy 5 of 6 fail, and gcm's outbound test fails with
@<local-uuid>. Earlier per-fix reverts: reverting one mapping fails 3 cases, reintroducing theadminsflattening fails 7, declaringparticipantIdsas a uuid fails the ontology contract.What running the real path found that unit tests could not
With the pre-fix code, Pictique's webhook did not fail — it hung for the full 60s timeout. The controller's catch-all logged the
TypeErrorand never sent a response, so a crash in this handler was indistinguishable from a slow peer. That is very likely why chats appeared to be "dropped with no error": there was no error to see, only silence. It now answers 500.Four bugs this found in my own work
The first version of this PR was wrong and its tests passed anyway. Stating that plainly:
adminsemitted[]on six platforms — the ownership enrichment flattened aUser[]relation to strings while those mappings readadmins[].ename. Missed because the suite only exercisedparticipants.enrichEntity, which junction-table changes, debounced group webhooks and backfill scripts never call. Moved intohandleChange, the one chokepoint.idToENamefell back to its input, turning a local uuid into@<uuid>: syntactically valid, semantically nobody.Coverage gap, stated honestly
Seven platforms (ereputation, esigner, file-manager, ecurrency, dreamsync, evoting, cerberus) have no acceptance suite — they had no test infrastructure at all, and standing up seven more containerised suites was not proportionate. They run the identical shared code path now covered end to end on the three representative platforms, and are verified by typecheck plus the
shipped-mappingscontract test over their real mapping files. That is weaker evidence than the three above, and worth knowing before deploy.Live cross-platform replication remains unverified here: it needs deployed eVaults and the Nextcloud PR, which is the deploy step below.
Running these locally
Starting either API also starts the producer side: Blabsy's Firestore watchers and Pictique's TypeORM subscriber replicate every local change to the eVaults named by
PUBLIC_REGISTRY_URL.Point
PUBLIC_REGISTRY_URLat a non-shared registry before running these against a real database, or the local instance will publish to whatever that URL names.Blabsy's browser client also needs its own
NEXT_PUBLIC_*Firebase config inplatforms/blabsy/client/.env.local, separate from the admin credentials the API uses. Without it the client returns 500 with "Firebase config is not set or incomplete".Deploy ordering — important
Consumers must accept eNames before any producer emits them.
Every consumer and producer in this repo changes together here, so this PR is internally consistent. Relative to Nextcloud:
Deploying #26 first would send eNames to consumers that still reject them.
One caveat worth stating plainly: this changes the wire format with no dual-read window. Chat envelopes already at rest carrying envelope-id participants will not resolve their participants after this deploys, and affected rooms will need re-syncing from their producing platform. Group ACL resolution is unaffected, since that path still reads both shapes.
Related: ensombl/nextcloud-w3ds-login#16 (the identifier inconsistency), #15 (Blabsy chats not syncing —
mapChatDatahad no guard at all, so one bare eName threw and lost the room, which is a plausible root cause).