Skip to content

feat(core): Auto-initialise customFields for entities that support them - #4965

Merged
michaelbromley merged 5 commits into
vendurehq:minorfrom
grolmus:mgrolmus/oss-408-auto-initialise-configcustomfields-for-plugin-entities
Jul 30, 2026
Merged

feat(core): Auto-initialise customFields for entities that support them#4965
michaelbromley merged 5 commits into
vendurehq:minorfrom
grolmus:mgrolmus/oss-408-auto-initialise-configcustomfields-for-plugin-entities

Conversation

@grolmus

@grolmus grolmus commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

Auto-initialises an empty customFields array for every entity that supports custom fields, so a plugin's configuration callback can extend any such entity (core or plugin-defined) without the defensive guard:

// Before — guard required, or startup crashes with "Cannot read properties of undefined"
configuration: config => {
    if (!config.customFields.CompanyRole) (config.customFields as any).CompanyRole = [];
    config.customFields.CompanyRole.push({ name: 'spendingLimit', type: 'int' });
}

// After — just works
configuration: config => {
    config.customFields.CompanyRole.push({ name: 'spendingLimit', type: 'int' });
}

Resolves OSS-408.

How

  • New getEntityNamesWithCustomFields() (register-custom-entity-fields.ts) detects custom-field-capable entities from the TypeORM metadata (they declare a customFields embedded column) — a HasCustomFields implements check isn't available at runtime.
  • runPluginConfigurations() (bootstrap.ts) pre-seeds config.customFields[EntityName] = [] for each of those before the plugin configuration callbacks run, only where an entry doesn't already exist (never overwrites configured fields).

Empty arrays are ignored by registerCustomEntityFields (it already skips zero-length configs), so this is inert for entities nobody extends — no schema/column changes.

The TypeScript half of the original issue (the as any footgun) is already resolved on main: CustomFields now carries a & { [entity: string]: CustomFieldConfig[] } index signature, so config.customFields.CompanyRole is typed without a cast. This PR adds the matching runtime guarantee.

Tests

bootstrap.spec.ts — custom-field-capable entities get [] initialised; existing entries are preserved; and a @VendurePlugin({ configuration }) can push to another entity's custom fields without a guard. Source typechecks clean; related entity specs pass.

Relates to OSS-408


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

@vercel

vercel Bot commented Jul 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
vendure-storybook Ready Ready Preview, Comment Jul 22, 2026 11:05am

Request Review

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1e6c6e56-d148-4e76-8b12-6afff4a92363

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…o-initialise-configcustomfields-for-plugin-entities
@grolmus

grolmus commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

CI fix: Field "CreateCollectionTranslationInput.customFields" can only be defined once

After bringing this branch up to date with the latest minor, CI turned red across build, codegen, unit tests and all e2e jobs with:

Error: Field "CreateCollectionTranslationInput.customFields" can only be defined once.

Root cause

This branch auto-initialises config.customFields[EntityName] = [] for every entity that declares a customFields embedded property, detected from the TypeORM metadata via getEntityNamesWithCustomFields().

In minor, the translation entities (CollectionTranslation, ProductTranslation, …, and the new ApiKeyTranslation) now also declare a customFields embedded — that's how localized custom-field values are stored. As a result getEntityNamesWithCustomFields() started returning translation entity names too, and the bootstrap loop began seeding config.customFields.CollectionTranslation = [], and so on.

Translation entities are never valid config.customFields keys — localized custom fields are declared on the base entity (e.g. Collection, with localized: true) and the framework fans them out to the translation table itself. Seeding a translation key makes the GraphQL schema builder (graphql-custom-fields.ts) emit the customFields field on the translation input types twice:

  • from the spurious CollectionTranslation key — the empty-array branch adds customFields: JSON to CreateCollectionTranslationInput unconditionally whenever that input type exists;
  • from the base Collection entity's localized fields — which legitimately add customFields to Create/UpdateCollectionTranslationInput.

Two definitions of the same field → the SDL error, which then cascades into every downstream job.

Worth noting: empty arrays are correctly ignored on the DB side (registerCustomEntityFields has a .length guard), which is why the original comment assumed the empty seed was inert — but the GraphQL builder has no equivalent guard.

Fix

getEntityNamesWithCustomFields() now excludes translation entities, detecting them as the target of a translations relation in the TypeORM metadata — the same signal registerCustomEntityFields already uses to locate the translation type, and a hard framework invariant (Translatable.translations). This removes exactly the spurious translation keys and nothing else; plugin-defined entities (the intended feature) are unaffected, and it doesn't rely on a *Translation name convention.

Verification

  • New regression test in bootstrap.spec.ts asserting translation entities are not auto-initialised (fails before the fix, passes after).
  • bootstrap.spec.ts (4) and graphql-custom-fields.spec.ts (15) green; core typecheck clean.
  • Full schema build / e2e confirmed by CI on this push.

@grolmus
grolmus requested a review from michaelbromley July 15, 2026 12:10
@michaelbromley michaelbromley added the T3: Systemic Involves a systemic decision. Decide before implementing. label Jul 16, 2026

@michaelbromley michaelbromley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with local reproduction (unit-level against the changed functions, plus full server bootstraps via @vendure/testing, with and without this diff applied).

First, scope: the empty-array seeding itself is less of a behaviour change than it looks. The ConfigService.customFields getter has been lazily seeding [] for every registered entity with a customFields embedded since 58943e3, and the schema build reads custom fields through that getter. The schema-level consequences I went hunting for (duplicate customFields on plugin-defined inputs, extend type on non-object type names) reproduce identically with and without this PR, so they are pre-existing and out of scope here. What this PR genuinely changes is the timing (early enough for configuration callbacks, which is the point) and the entity source.

Blocker

getEntityNamesWithCustomFields() does this for every relation named translations in the global metadata storage:

.map(relation => (relation.type as () => Function)().name)

TypeORM also permits string targets, commonly used to break circular imports:

@Entity()
class Article extends VendureEntity {
    @OneToMany('ArticleTranslation', 'base')
    translations: Array<Translation<Article>>;
}

relation.type is then the string 'ArticleTranslation', and calling it throws TypeError: relation.type is not a function. Because this runs unconditionally at the top of runPluginConfigurations(), one such relation anywhere in the process kills every bootstrap, even if the entity has nothing to do with custom fields. Reproduced against this branch: getEntityNamesWithCustomFields() throws and runPluginConfigurations() rejects.

Core already guards this exact case elsewhere — see getEntityTranslation() in validate-custom-fields-config.ts (typeof type === 'function'). Same guard needed here, plus handling for a closure that returns a string name (also legal), which currently yields undefined and silently fails to exclude the translation entity — landing you in the duplicate customFields schema error this PR's translation exclusion exists to prevent.

Non-blocking suggestions

  1. Seed from this instance's entities rather than getMetadataArgsStorage(). The global storage contains every entity imported into the process, including ones not registered with this server (a second test server in the same process, or an imported-but-uninstalled plugin). Those get phantom keys, which surface as fields on the deprecated ServerConfig.customFieldConfig type. getAllEntities() runs just before runPluginConfigurations() in preBootstrapConfig, so the list could be passed in.
  2. There are now three separate definitions of "translation entity / supports custom fields" in core: this function (target of a translations relation), the ConfigService getter (*Translation name suffix plus languageCode column), and registerCustomEntityFields. Since the getter skips keys that already exist, this PR's earlier seeding pre-empts it, and entities the two heuristics classify differently change behaviour. Worth collapsing into one shared helper so they cannot diverge.

`getEntityNamesWithCustomFields()` called every `translations` relation target
as a function to read its `.name`. TypeORM also allows a bare string target and
a closure returning a string (both used to break circular imports, e.g.
`@OneToMany('ArticleTranslation', ...)`). The former threw
`relation.type is not a function` — and since this runs unconditionally at the
top of `runPluginConfigurations()`, one such relation anywhere in the process
aborted every bootstrap; the latter yielded `undefined` and silently failed to
exclude the translation entity, re-introducing the duplicate-`customFields`
schema error this exclusion exists to prevent.

Added `getRelationTargetName()` which resolves all three shapes to the entity
name, guarding `typeof type === 'function'` the same way `getEntityTranslation`
in validate-custom-fields-config.ts does. Tests cover string, closure-returning-
string, and constructor-closure targets.

Relates to vendurehq#4965
@grolmus

grolmus commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough review, Michael — and for reproducing it against a live bootstrap.

Blocker fixed (006adf7)

getEntityNamesWithCustomFields() now resolves the relation target through a small helper that handles all three shapes TypeORM allows, rather than calling the target unconditionally:

function getRelationTargetName(type: RelationMetadataArgs['type']): string | undefined {
    const resolved: unknown = typeof type === 'function' ? (type as () => unknown)() : type;
    if (typeof resolved === 'string') {
        return resolved;
    }
    if (typeof resolved === 'function') {
        return resolved.name;
    }
    return (resolved as { name?: string } | undefined)?.name;
}

Same typeof type === 'function' guard as getEntityTranslation in validate-custom-fields-config.ts, plus the string-returning-closure case you flagged — which previously yielded undefined and silently failed to exclude the translation entity.

Added three tests (bootstrap.spec.ts) covering a bare string target, a closure returning a string, and the usual constructor closure. Confirmed they go red without the fix (TypeError: relation.type is not a function on the string target; empty array instead of undefined on the closure-string) and green with it. bootstrap + validate-custom-fields-config suites: 16 passed.

On the two non-blocking suggestions

Both are good calls and I'd rather do them properly than rush them into this PR, so I've left them as follow-ups unless you'd prefer them here:

  1. Seed from getAllEntities() rather than global metadata. Agreed — the phantom-key exposure via ServerConfig.customFieldConfig is real. It's a small change mechanically (getAllEntities(userConfig) is already computed just above the call site), but it changes which entities we seed from, so it deserves its own test for the second-server / imported-but-uninstalled-plugin case rather than riding along on a guard fix.

  2. Collapse the three "translation entity / supports custom fields" definitions. Also agreed that having this function, the ConfigService getter (*Translation suffix + languageCode column), and registerCustomEntityFields classify independently is a divergence risk. That's a cross-file refactor with its own review surface, so cleaner as a dedicated PR.

Happy to fold either in here if you'd rather not split them — your call.

@michaelbromley michaelbromley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick turnaround, Martin. The blocker fix looks solid — resolving all three legal TypeORM translations target shapes (constructor closure, bare string, closure-returning-string) is exactly right, and the tests around it are good: exact assertions, synthetic metadata cleaned up in afterEach, and a real @VendurePlugin end-to-end case rather than testing the helper in isolation.

One thing I'd like closed out before merge though:

The same bug class is still live a few lines away in the same file. In registerCustomEntityFields() (register-custom-entity-fields.ts), this pre-existing line is unguarded:

const translationType: Function = (translationsMetadata.type as Function)();

That's the identical relation.type is not a function pattern you just fixed in getEntityNamesWithCustomFields(). Any entity with a bare-string translations target and real custom fields will crash right here. I know you flagged the broader "three divergent implementations" consolidation as a follow-up PR, and I agree that full cleanup deserves its own review surface — but this specific instance isn't cross-file, it's the same file, and it can just reuse the new getRelationTargetName() helper sitting right above it. That's a one-line change, not the refactor. I'd rather fix it here than ship a known crash and rely on the follow-up landing.

Non-blocking, but let's not lose these — could you open OSS tickets for them so they don't rot as PR comments?

  • Seed getEntityNamesWithCustomFields() from getAllEntities(userConfig) rather than the global getMetadataArgsStorage(), to avoid phantom keys from imported-but-uninstalled plugins / the second-server case. getAllEntities is already computed just above the call site, so it's cheap — I get that you deferred it because it wants its own test, which is fair.
  • The full three-way consolidation of the "is this a translation entity?" heuristic (the new helper, the typeof === 'function'-only guard in validate-custom-fields-config.ts, and the inline one above).

Also worth confirming the postgres e2e job goes green before merge — it was showing failure with an exit-code-130 (SIGINT) annotation, which smells like a cancellation/timeout rather than a real assertion failure, and nothing in the diff is postgres-specific, but let's not assume flake.

Tiny nit, take or leave: in the spec, import { coreEntitiesMap } ...; void coreEntitiesMap; is just forcing the entity-registration side effect while dodging the unused-import lint — a bare import './entity/entities'; does the same without the fake reference.

@grolmus

grolmus commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks Michael — all four addressed.

Blocker fixed (ffce859)

You're right, the same type is not a function was still live one branch down in registerCustomEntityFields(). It now resolves through the getRelationTargetName() helper instead of (translationsMetadata.type as Function)():

const translationEntityName = getRelationTargetName(translationsMetadata.type);
if (translationEntityName != null) {
    const customFieldsTranslationsMetadata = getCustomFieldsMetadata(translationEntityName);
    ...
}

getCustomFieldsMetadata already accepts Function | string, so the resolved name drops straight in — behaviour is identical for the constructor-closure case and no longer crashes on a bare-string target. Added a test that registers a translatable entity with a bare-string translations target and real custom fields and asserts registerCustomEntityFields doesn't throw (red without the fix).

Nit

Swapped import { coreEntitiesMap } … void coreEntitiesMap; for the bare import './entity/entities';.

Postgres e2e

Green — on the current head (006adf76) the whole matrix passed, e2e (postgres) (22.x) included; the exit-130 you saw was a transient cancellation on an earlier push, not an assertion failure. This push re-runs it (fork workflow, needs a run approval).

Follow-ups filed

Both are on the backlog so they don't rot here:

  • OSS-653 — seed getEntityNamesWithCustomFields() from getAllEntities(userConfig) instead of the global metadata storage (the phantom-key / second-server case).
  • OSS-654 — consolidate the three "is this a translation entity?" heuristics into one shared helper.

@grolmus
grolmus requested a review from michaelbromley July 30, 2026 08:06
@michaelbromley
michaelbromley merged commit 41d8082 into vendurehq:minor Jul 30, 2026
29 of 30 checks passed
@vendure-ci-automation-bot vendure-ci-automation-bot Bot locked and limited conversation to collaborators Jul 30, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

T3: Systemic Involves a systemic decision. Decide before implementing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants