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,