From 63de5141e0a94f6399e3e5780bfd1b5e29e3e46d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 09:40:20 +0000 Subject: [PATCH] =?UTF-8?q?fix(runtime,i18n):=20field=20labels=20read=20th?= =?UTF-8?q?e=20bundle=20shape=20producers=20write=20=E2=80=94=20one=20shar?= =?UTF-8?q?ed=20derivation=20(#3833)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /i18n/labels/:object/:locale` served through the dispatcher returned `{ labels: {} }` for every provider. Its derivation scanned for flat `o..fields.` keys — the dialect #3778 retired. No producer has ever written it, and a real bundle's top-level keys are the `TranslationData` groups (`objects`, `apps`, `messages`, ...), so the prefix matched nothing. 4cca74c fixed the identical derivation in service-i18n and did not reach the dispatcher's copy. Not a rare fallback: `getFieldLabels` is optional on `II18nService` and nothing implements it — not `memory-i18n`, not `file-i18n-adapter` — so the dedicated- method branch both surfaces check first is dead in production and this derivation is the only path there is. Any stack served by the dispatcher got an empty map, indistinguishable from "this object has no translated labels". Nothing errored, nothing warned. Worse than the class it was found beside: #3676 ignored a declared filter and returned the full bundle, a correct superset. This returned nothing and said it was fine. The derivation now lives once, as `resolveObjectFieldLabels` in `packages/spec/src/system/i18n-resolver.ts`, beside the other resolvers that read `TranslationData`. Both surfaces call it — keeping a copy each is precisely how one got fixed and the other did not. Fields with no non-empty `label` stay omitted rather than emitted blank: partial translation is the normal state, and callers merge this map over their source labels, where a '' would erase them. The tests were fiction on both sides. The dispatcher's fallback test fed flat `o.contact.fields.first_name` keys and asserted labels came back, so it passed on data that cannot occur while production returned `{}` — the same failure mode as the client test retired in #3676. It now feeds the nested shape, and was confirmed to FAIL against the pre-fix code rather than merely passing after it. The same suite's mock declared a `getFieldLabels` no shipped provider has and returned flat-dialect data; both now reflect what a real provider does. The shared helper carries unit tests, including one pinning the retired flat dialect to `{}`. Filed separately, not addressed here: `GetFieldLabelsResponseSchema` declares `labels` as `Record` while both surfaces emit `Record` — a third declared != enforced gap on this endpoint, and a wire-shape change too breaking to fold into a correctness fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0196gwfwMK5vW8RToPyzGMJo --- ...n-field-labels-shared-nested-derivation.md | 62 +++++++++++++++++++ packages/runtime/src/domains/i18n.ts | 22 ++++--- packages/runtime/src/http-dispatcher.test.ts | 61 ++++++++++++++++-- .../service-i18n/src/i18n-service-plugin.ts | 12 ++-- packages/spec/api-surface.json | 1 + .../spec/src/system/i18n-resolver.test.ts | 51 +++++++++++++++ packages/spec/src/system/i18n-resolver.ts | 38 ++++++++++++ 7 files changed, 224 insertions(+), 23 deletions(-) create mode 100644 .changeset/i18n-field-labels-shared-nested-derivation.md diff --git a/.changeset/i18n-field-labels-shared-nested-derivation.md b/.changeset/i18n-field-labels-shared-nested-derivation.md new file mode 100644 index 0000000000..24135cab49 --- /dev/null +++ b/.changeset/i18n-field-labels-shared-nested-derivation.md @@ -0,0 +1,62 @@ +--- +"@objectstack/spec": minor +"@objectstack/runtime": patch +"@objectstack/service-i18n": patch +--- + +fix(runtime,i18n): the dispatcher's field-labels route reads the bundle shape +producers actually write — one shared derivation (#3833) + +`GET /i18n/labels/:object/:locale` served through the dispatcher returned +`{ labels: {} }` for every provider. Its derivation scanned for flat +`o..fields.` keys: + +```ts +const prefix = `o.${objectName}.fields.`; +for (const [key, value] of Object.entries(translations)) { … } +``` + +That dialect was retired by #3778 — no producer has ever written it, and a real +bundle's top-level keys are the `TranslationData` groups (`objects`, `apps`, +`messages`, …), so the prefix could not match anything. 4cca74c fixed the +identical derivation in `service-i18n` and did not reach the dispatcher's copy. + +This is not a rare fallback. `getFieldLabels` is optional on `II18nService` and +**nothing implements it** — not `memory-i18n`, not `file-i18n-adapter` — so the +dedicated-method branch both surfaces check first is dead in production and this +derivation is the only path there is. Any stack served by the dispatcher (the +AppPlugin in-memory provider auto-registered for stacks declaring translation +bundles) got an empty map, indistinguishable from "this object has no translated +labels": nothing errored, nothing warned. + +Worse than the class it was found next to. #3676, which prompted the check, +ignored a declared filter and returned the full bundle — a correct superset. This +returned nothing and said it was fine. + +The derivation now lives once, as `resolveObjectFieldLabels` in +`packages/spec/src/system/i18n-resolver.ts`, alongside the other resolvers that +read `TranslationData`. Both surfaces call it. Keeping a copy each is precisely +how one got fixed and the other did not; the next bundle-shape change now has one +place to land. Fields carrying no non-empty `label` stay omitted rather than +emitted blank — partial translation is the normal state, and callers merge this +map over their source labels, where a `''` would erase them. + +### The tests were fiction on both sides + +The dispatcher's fallback test fed flat `o.contact.fields.first_name` keys and +asserted labels came back, so it passed on data that cannot occur while +production returned `{}` — the same failure mode as the client test retired in +#3676, which asserted a query string was built that no server read. It now feeds +the nested shape, and was confirmed to fail against the pre-fix code (`expected +{} to deeply equal { first_name: 'First Name', … }`) rather than merely passing +after it. The shared helper carries its own unit tests, including one pinning +that the retired flat dialect resolves to `{}`. + +The same suite's mock also declared a `getFieldLabels` no shipped provider has, +and returned flat-dialect data from `getTranslations`; both now reflect what a +real provider does, with the divergence noted where it remains deliberate. + +Not addressed here, filed separately: `GetFieldLabelsResponseSchema` declares +`labels` as `Record`, but both surfaces emit +`Record` — a third declared ≠ enforced gap in the same endpoint, +and a wire-shape change too breaking to fold into a correctness fix. diff --git a/packages/runtime/src/domains/i18n.ts b/packages/runtime/src/domains/i18n.ts index 9de488fa00..8a2dcf06f6 100644 --- a/packages/runtime/src/domains/i18n.ts +++ b/packages/runtime/src/domains/i18n.ts @@ -17,7 +17,8 @@ */ import { resolveLocale } from '@objectstack/core'; -import { CoreServiceName } from '@objectstack/spec/system'; +import { CoreServiceName, resolveObjectFieldLabels } from '@objectstack/spec/system'; +import type { TranslationData } from '@objectstack/spec/system'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -89,15 +90,16 @@ export async function handleI18nRequest( const labels = i18nService.getFieldLabels(objectName, locale); return { handled: true, response: deps.success({ object: objectName, locale, labels }) }; } - // Fallback: derive field labels from full translation bundle - const translations = i18nService.getTranslations(locale); - const prefix = `o.${objectName}.fields.`; - const labels: Record = {}; - for (const [key, value] of Object.entries(translations)) { - if (key.startsWith(prefix)) { - labels[key.substring(prefix.length)] = value as string; - } - } + // Fallback: derive field labels from the locale's translation bundle. + // This is not really a fallback — `getFieldLabels` is optional on + // `II18nService` and nothing implements it, so this is the path every + // provider takes. Shared with service-i18n's identical derivation so + // the next bundle-shape change cannot fix one copy and miss the other, + // which is how this one went on scanning the retired flat + // `o..fields.` dialect after #3778 and returned `{}` + // for every provider (#3833). + const translations = i18nService.getTranslations(locale) as TranslationData | undefined; + const labels = resolveObjectFieldLabels(translations, objectName); return { handled: true, response: deps.success({ object: objectName, locale, labels }) }; } diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index 5b951ceefb..19076d5ceb 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/packages/runtime/src/http-dispatcher.test.ts @@ -1801,7 +1801,16 @@ describe('HttpDispatcher', () => { beforeEach(() => { mockI18nService = { getLocales: vi.fn().mockReturnValue(['en', 'zh-CN', 'ja']), - getTranslations: vi.fn().mockReturnValue({ 'o.account.label': '客户', 'o.account.fields.name': '名称' }), + // The nested shape every producer writes and #3778 converged + // on. This used to be flat `o.account.label` keys — a dialect + // no bundle has ever carried. + getTranslations: vi.fn().mockReturnValue({ + objects: { account: { label: '客户', fields: { name: { label: '名称' } } } }, + }), + // Declared optional on `II18nService` and implemented by NO + // shipped provider — the tests below that assert it is called + // cover the dispatcher's handling of a provider that supplies + // it, not a path any current stack takes. See #3833. getFieldLabels: vi.fn().mockReturnValue({ name: '名称', industry: '行业' }), }; @@ -1824,7 +1833,9 @@ describe('HttpDispatcher', () => { expect(result.handled).toBe(true); expect(result.response?.status).toBe(200); expect(result.response?.body?.data?.locale).toBe('zh-CN'); - expect(result.response?.body?.data?.translations).toEqual({ 'o.account.label': '客户', 'o.account.fields.name': '名称' }); + expect(result.response?.body?.data?.translations).toEqual({ + objects: { account: { label: '客户', fields: { name: { label: '名称' } } } }, + }); expect(mockI18nService.getTranslations).toHaveBeenCalledWith('zh-CN'); }); @@ -1897,12 +1908,38 @@ describe('HttpDispatcher', () => { expect((body.error as { code: unknown }).code).toBe(400); }); - it('should fallback to deriving labels from translations when getFieldLabels is missing', async () => { + /** + * This is the path EVERY provider takes, not an edge case: + * `getFieldLabels` is optional on `II18nService` and nothing implements + * it — not `memory-i18n`, not `file-i18n-adapter` — so the dedicated- + * method branch above is dead in production and this derivation always + * runs. + * + * Its predecessor fed flat `o.contact.fields.first_name` keys and + * asserted labels came back. That dialect was retired by #3778 (no + * producer ever wrote it), so the test passed on data that cannot + * occur while the real path — scanning for an `o.` prefix in a bundle + * whose top-level keys are `objects`/`apps`/`messages` — returned `{}` + * for every caller. Feeding the shape real bundles actually have is + * the whole point of the test (#3833). + */ + it('derives labels from the NESTED bundle shape every producer writes (#3833)', async () => { delete mockI18nService.getFieldLabels; mockI18nService.getTranslations.mockReturnValue({ - 'o.contact.fields.first_name': 'First Name', - 'o.contact.fields.email': 'Email', - 'o.contact.label': 'Contact', + objects: { + contact: { + label: 'Contact', + fields: { + first_name: { label: 'First Name' }, + email: { label: 'Email', help: 'Primary address' }, + // No label — partial translation is the normal + // state, and a blank entry would overwrite the + // caller's source label with an empty string. + phone: { help: 'Mobile preferred' }, + }, + }, + }, + messages: { save: 'Save' }, }); const result = await dispatcher.handleI18n('/labels/contact/en', 'GET', {}, { request: {} }); @@ -1914,6 +1951,18 @@ describe('HttpDispatcher', () => { }); }); + it('returns {} for an object the locale does not translate, without throwing', async () => { + delete mockI18nService.getFieldLabels; + mockI18nService.getTranslations.mockReturnValue({ + objects: { contact: { fields: { email: { label: 'Email' } } } }, + }); + + const result = await dispatcher.handleI18n('/labels/account/en', 'GET', {}, { request: {} }); + expect(result.handled).toBe(true); + expect(result.response?.status).toBe(200); + expect(result.response?.body?.data?.labels).toEqual({}); + }); + it('should return 501 when i18n service is not available', async () => { (kernel as any).getService = vi.fn().mockResolvedValue(null); (kernel as any).services = new Map(); diff --git a/packages/services/service-i18n/src/i18n-service-plugin.ts b/packages/services/service-i18n/src/i18n-service-plugin.ts index f1f8df6088..5f05da44a4 100644 --- a/packages/services/service-i18n/src/i18n-service-plugin.ts +++ b/packages/services/service-i18n/src/i18n-service-plugin.ts @@ -5,6 +5,7 @@ import { wireAuthoredTranslationSync } from '@objectstack/core'; import type { IHttpServer, IHttpRequest, IHttpResponse } from '@objectstack/spec/contracts'; import type { II18nService } from '@objectstack/spec/contracts'; import type { TranslationData } from '@objectstack/spec/system'; +import { resolveObjectFieldLabels } from '@objectstack/spec/system'; import { FileI18nAdapter } from './file-i18n-adapter.js'; import type { FileI18nAdapterOptions } from './file-i18n-adapter.js'; @@ -234,14 +235,11 @@ export class I18nServicePlugin implements Plugin { // That data is NESTED (`objects..fields..label`) — the // flat dotted `o..fields.` keys this used to scan were a // third translation dialect that no producer ever wrote, so the - // fallback always returned `{}` (#3778). + // fallback always returned `{}` (#3778). The derivation now lives in + // `resolveObjectFieldLabels` so the dispatcher's copy of it cannot + // drift out of shape again the way it did after that fix (#3833). const data = i18n.getTranslations(locale) as TranslationData | undefined; - const fields = data?.objects?.[objectName]?.fields ?? {}; - const labels: Record = {}; - for (const [fieldName, field] of Object.entries(fields)) { - const label = field?.label; - if (typeof label === 'string' && label.length > 0) labels[fieldName] = label; - } + const labels = resolveObjectFieldLabels(data, objectName); res.json({ success: true, data: { object: objectName, locale, labels } }); } } catch (error: any) { diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 7fb1602d8a..f8a8a4b136 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -1300,6 +1300,7 @@ "resolveMetadataFormLabels (function)", "resolveMetadataTypeDescription (function)", "resolveMetadataTypeLabel (function)", + "resolveObjectFieldLabels (function)", "resolveSettingsActionConfirm (function)", "resolveSettingsActionLabel (function)", "resolveSettingsActionSuccess (function)", diff --git a/packages/spec/src/system/i18n-resolver.test.ts b/packages/spec/src/system/i18n-resolver.test.ts index 5e1b620210..15d6ee981f 100644 --- a/packages/spec/src/system/i18n-resolver.test.ts +++ b/packages/spec/src/system/i18n-resolver.test.ts @@ -11,6 +11,7 @@ import { resolveActionResultDialog, translateAction, translateMetadataDocument, + resolveObjectFieldLabels, } from './i18n-resolver'; describe('ObjectTranslationDataSchema (_views/_actions extensions)', () => { @@ -1038,3 +1039,53 @@ describe('translateObject inline actions (objectstack#3370)', () => { expect(Object.hasOwn(out, 'actions')).toBe(false); }); }); + +// ========================================== +// resolveObjectFieldLabels — the `/i18n/labels/:object/:locale` body +// ========================================== + +describe('resolveObjectFieldLabels (objectstack#3833)', () => { + const data = TranslationDataSchema.parse({ + objects: { + contact: { + label: 'Contact', + fields: { + first_name: { label: 'First Name' }, + email: { label: 'Email', help: 'Primary address' }, + phone: { help: 'Mobile preferred' }, + }, + }, + }, + messages: { save: 'Save' }, + }); + + it('enumerates the labels a locale actually translates', () => { + expect(resolveObjectFieldLabels(data, 'contact')).toEqual({ + first_name: 'First Name', + email: 'Email', + }); + }); + + it('omits fields carrying no label rather than emitting a blank one', () => { + // Partial translation is the normal state (see ObjectTranslationDataSchema), + // and callers merge this over their source labels — a '' would erase them. + expect(resolveObjectFieldLabels(data, 'contact')).not.toHaveProperty('phone'); + }); + + it('returns {} for an untranslated object, a bundle with no objects, and no bundle', () => { + expect(resolveObjectFieldLabels(data, 'account')).toEqual({}); + expect(resolveObjectFieldLabels({ messages: { save: 'Save' } }, 'contact')).toEqual({}); + expect(resolveObjectFieldLabels(undefined, 'contact')).toEqual({}); + }); + + it('never matches the retired flat `o..fields.` dialect', () => { + // The shape the dispatcher scanned for until #3833. It is not a bundle + // any producer writes, and reading it as one is what returned {} in + // production while a test built on the same fiction stayed green. + const flat = { + 'o.contact.fields.first_name': 'First Name', + 'o.contact.label': 'Contact', + } as unknown as Parameters[0]; + expect(resolveObjectFieldLabels(flat, 'contact')).toEqual({}); + }); +}); diff --git a/packages/spec/src/system/i18n-resolver.ts b/packages/spec/src/system/i18n-resolver.ts index 6bad3f161c..28af462cb8 100644 --- a/packages/spec/src/system/i18n-resolver.ts +++ b/packages/spec/src/system/i18n-resolver.ts @@ -724,6 +724,44 @@ function lookupObjectFieldAttr( return undefined; } +/** + * Enumerate an object's translated field labels out of ONE locale's + * `TranslationData` — the `GET /i18n/labels/:object/:locale` body. + * + * Distinct from `lookupObjectFieldAttr` above, which answers "what is this + * one field called" against a whole bundle and a locale chain. This answers + * "which fields does this locale translate at all", which is what the + * endpoint returns and what no per-field lookup can produce. + * + * Shared deliberately. Both serving surfaces derive this map — the dispatcher + * domain body and service-i18n's autonomous route — and they are the only + * path there is: `getFieldLabels` is optional on `II18nService` and NOTHING + * implements it (neither `memory-i18n` nor `file-i18n-adapter`), so the + * "dedicated method" branch both surfaces check first is dead and this + * derivation always runs. Keeping one copy each is how the dispatcher was + * left scanning the retired flat `o..fields.` dialect after + * #3778 converged the tree on nested `objects..fields..label` + * and fixed only service-i18n's copy — a scan that cannot match a real bundle, + * so that route returned `{}` for every provider (#3833). + * + * Fields with no non-empty `label` are omitted rather than emitted blank: + * partial translation is the normal state (see `ObjectTranslationDataSchema`), + * and a caller merges what comes back over its source labels. + */ +export function resolveObjectFieldLabels( + data: TranslationData | undefined, + objectName: string, +): Record { + const fields = data?.objects?.[objectName]?.fields; + const labels: Record = {}; + if (!fields) return labels; + for (const [fieldName, field] of Object.entries(fields)) { + const label = field?.label; + if (typeof label === 'string' && label.length > 0) labels[fieldName] = label; + } + return labels; +} + function lookupObjectFieldOption( bundle: TranslationBundle | undefined, objectName: string,