fix(api): derive language cap from named constants instead of a magic number (#1765)#1771
fix(api): derive language cap from named constants instead of a magic number (#1765)#1771johan-bell wants to merge 2 commits into
Conversation
) The non-CMS language cap was the magic number 4 restated in three places (DEFAULT_MAX_LANGUAGES, configuration.ts env fallback, query.controller fallback) and kept "in step" with the client's 3-preferred cap only by a comment — zero headroom on the boundary, so any future language dimension or a legitimate query referencing one more id would trip a spurious 400. DEFAULT_MAX_LANGUAGES is now derived from named constants that move together — MAX_PREFERRED_LANGUAGES (3, mirrors app globalConfig) + AUTO_APPENDED_DEFAULT_LANGUAGES (1) + LANGUAGE_CAP_HEADROOM (1) = 5 — and the two duplicate fallbacks reference it instead of a literal. Adds a test asserting the cap keeps headroom above the client maximum.
|
I need to understand why we need a language cap headroom. When future languages are added, we still want the user to only be able to select 3 of them (auto-appending English if it is not part of the selection). This does not place a limit on how many language docs are synced - we need to sync all language docs for the app to be aware of the languages that exists. |
Per review: the client-side preferred-language cap (3 + auto-appended default = 4) is a fixed UX invariant enforced by normalizePreferredLanguages, not something that scales with how many Language docs exist in the system. Growing the language catalog doesn't grow a single query's language count, so there's no real case where the cap needs slack above 4 — and if the client cap itself is ever bumped, MAX_PREFERRED_LANGUAGES already has to move in lockstep, at which point the derived cap adjusts on its own.
|
@ivanslabbert You're right — I traced the actual code path and couldn't find a case where headroom is load-bearing. The cap only counts values under a language field in the query selector; it's unrelated to how many Language docs sync (that's unbounded regardless). The real ceiling is appDisplayLanguageIdsAsRef in app/src/globalConfig.ts: preferred (hard-capped at 3 via normalizePreferredLanguages's .slice(0, MAX_PREFERRED_LANGUAGES)) + auto-appended default = at most 4, always — and that's a fixed UX invariant, not something that grows as more languages get added to the system. If we ever did bump the client's preferred-language cap, MAX_PREFERRED_LANGUAGES here would need to move in lockstep anyway (already documented), and the derived total would adjust on its own — no pre-emptive headroom needed for that either. Pushed a follow-up (a9cef35) that drops LANGUAGE_CAP_HEADROOM and derives DEFAULT_MAX_LANGUAGES as exactly MAX_PREFERRED_LANGUAGES + AUTO_APPENDED_DEFAULT_LANGUAGES = 4. Keeps the original fix's actual value (single source of truth instead of a magic number duplicated in three places), just without the unjustified slack. |
| export const MAX_PREFERRED_LANGUAGES = 3; | ||
|
|
||
| /** The client auto-appends the default (display) language on top of the preferred set. */ | ||
| export const AUTO_APPENDED_DEFAULT_LANGUAGES = 1; | ||
|
|
||
| /** | ||
| * Fallback language cap when the caller doesn't supply one. Derived so the pieces move together | ||
| * instead of being restated as a magic number: preferred (3) + auto-appended default (1) = 4. A | ||
| * legitimate non-CMS content query never references more than this — the client-side selection | ||
| * cap is a fixed UX invariant (`normalizePreferredLanguages` hard-slices to | ||
| * `MAX_PREFERRED_LANGUAGES`), independent of how many languages exist in the system. Growing the | ||
| * language catalog doesn't grow a single query's language count; only bumping the client cap | ||
| * itself would, and that already requires updating `MAX_PREFERRED_LANGUAGES` here in lockstep. | ||
| */ | ||
| export const DEFAULT_MAX_LANGUAGES = MAX_PREFERRED_LANGUAGES + AUTO_APPENDED_DEFAULT_LANGUAGES; |
There was a problem hiding this comment.
This feels unnessasary. Since default max languages was already set before in the original. I feel like this does not really solve a real issue since this same variable is just being reused through this PR
There was a problem hiding this comment.
Ivan raised the same question above — headroom was in the first commit, but I traced the code path and couldn't find anywhere it'd actually be load-bearing (the cap only counts values under language in the query selector; that's unrelated to how many Language docs sync, which is unbounded regardless). Dropped it in the follow-up commit, so DEFAULT_MAX_LANGUAGES is back to exactly 4.
What's left is the other half of #1765: the 4 was a magic number duplicated in three files (validateQuery.ts, configuration.ts, query.controller.ts), kept "in sync" only by a comment. It's now derived once from MAX_PREFERRED_LANGUAGES + AUTO_APPENDED_DEFAULT_LANGUAGES, and the other two spots reference that instead of restating the literal — no behavior change today, but one constant to update instead of three call sites to remember if the client cap ever moves. Title/description were still describing the first commit (headroom); updating those now.
johan-bell
changed the title
3 participants
What & why
The non-CMS
/querylanguage cap was the magic number 4, restated in three places and kept "in step" with the client's 3-preferred-language cap only by a comment:DEFAULT_MAX_LANGUAGES = 4invalidateQuery.tsparseInt(process.env.QUERY_MAX_LANGUAGES, 10) || 4inconfiguration.ts?? 4inquery.controller.tsConstants that must move together (GitHub #1765) had nothing enforcing it — the
3 (preferred) + 1 (default) = 4relationship lived in prose, and the literal4was duplicated across three files. If the client's preferred-language cap ever changed, all three call sites had to be remembered and updated by hand.An earlier version of this PR also added a
LANGUAGE_CAP_HEADROOMslot (cap = 5) per #1765's "zero headroom" framing. Per review discussion (Ivan, then confirmed by tracing the code), the cap only counts values underlanguagein the query selector — that's unrelated to how many Language docs sync (unbounded regardless), and the client-side preferred-language cap is a fixed UX invariant (normalizePreferredLanguageshard-slices toMAX_PREFERRED_LANGUAGES), not something that grows as the language catalog grows. There was no case where headroom was actually load-bearing, so it was dropped; the cap stays at exactly 4.Change
DEFAULT_MAX_LANGUAGESis now derived from named constants that move together:MAX_PREFERRED_LANGUAGES (3)+AUTO_APPENDED_DEFAULT_LANGUAGES (1)= 4 (unchanged value).MAX_PREFERRED_LANGUAGESdocuments that it mirrorsapp/src/globalConfig.ts's constant.configuration.tsandquery.controller.tsnow referenceDEFAULT_MAX_LANGUAGESinstead of a literal — single source of truth.No behavioural change for legitimate clients (the cap is still 4); this removes the "must move together" fragility from #1765 without adding unjustified slack.
Verification
npx jest src/validation/query/validateQuery.spec.ts→ 44 passed.npx tsc --noEmitclean.Closes #1765