feat(core): Auto-initialise customFields for entities that support them - #4965
Conversation
Relates to OSS-408
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…o-initialise-configcustomfields-for-plugin-entities
Relates to OSS-408
CI fix:
|
michaelbromley
left a comment
There was a problem hiding this comment.
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
- 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 deprecatedServerConfig.customFieldConfigtype.getAllEntities()runs just beforerunPluginConfigurations()inpreBootstrapConfig, so the list could be passed in. - There are now three separate definitions of "translation entity / supports custom fields" in core: this function (target of a
translationsrelation), theConfigServicegetter (*Translationname suffix pluslanguageCodecolumn), andregisterCustomEntityFields. 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
|
Thanks for the thorough review, Michael — and for reproducing it against a live bootstrap. Blocker fixed (006adf7)
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 Added three tests ( On the two non-blocking suggestionsBoth 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:
Happy to fold either in here if you'd rather not split them — your call. |
michaelbromley
left a comment
There was a problem hiding this comment.
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()fromgetAllEntities(userConfig)rather than the globalgetMetadataArgsStorage(), to avoid phantom keys from imported-but-uninstalled plugins / the second-server case.getAllEntitiesis 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 invalidate-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.
|
Thanks Michael — all four addressed. Blocker fixed (ffce859)You're right, the same const translationEntityName = getRelationTargetName(translationsMetadata.type);
if (translationEntityName != null) {
const customFieldsTranslationsMetadata = getCustomFieldsMetadata(translationEntityName);
...
}
NitSwapped Postgres e2eGreen — on the current head ( Follow-ups filedBoth are on the backlog so they don't rot here:
|
Summary
Auto-initialises an empty
customFieldsarray for every entity that supports custom fields, so a plugin'sconfigurationcallback can extend any such entity (core or plugin-defined) without the defensive guard:Resolves OSS-408.
How
getEntityNamesWithCustomFields()(register-custom-entity-fields.ts) detects custom-field-capable entities from the TypeORM metadata (they declare acustomFieldsembedded column) — aHasCustomFieldsimplementscheck isn't available at runtime.runPluginConfigurations()(bootstrap.ts) pre-seedsconfig.customFields[EntityName] = []for each of those before the pluginconfigurationcallbacks 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 anyfootgun) is already resolved onmain:CustomFieldsnow carries a& { [entity: string]: CustomFieldConfig[] }index signature, soconfig.customFields.CompanyRoleis 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 })canpushto another entity's custom fields without a guard. Source typechecks clean; related entity specs pass.Relates to OSS-408
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.