feat(core): Extend custom relational field config#4448
Conversation
Adds properties like cascade, onDelete and onUpdate
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe RelationCustomFieldConfig type was updated to reuse TypeORM's RelationOptions by adding the properties cascade, onDelete, onUpdate, and eager (removing the locally declared eager). The custom-entity field registration now reads cascade, onDelete, onUpdate, and eager from relation custom fields and forwards nullable, cascade, onDelete, onUpdate, and eager to the ManyToMany and ManyToOne decorators. Documentation was edited to document and provide examples for the cascade, onDelete, and onUpdate options for relation custom fields. Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
3921bf4 to
d0e1244
Compare
…gn/vendure into feat/custom-field-relations
grolmus
left a comment
There was a problem hiding this comment.
Thanks for the PR! Exposing TypeORM relation options on custom fields is a useful feature. A few things to address before this can go in:
Must address
1. nullable silently injected into relation decorators
The diff passes nullable (from the outer destructure on line 42) into both ManyToMany and ManyToOne options, but this wasn't part of the type change or PR description.
- ManyToMany:
nullableis semantically meaningless here — ManyToMany uses a junction table, there's no nullable FK column on the entity itself. TypeORM accepts it silently but it does nothing. Should be removed from the ManyToMany call. - ManyToOne:
nullabledoes make sense, but this is an undocumented behavioral change. Previously,nullable: falseon a relation custom field was silently ignored. Now it gets enforced. This should either be called out explicitly or handled in a separate commit.
2. Auto-generated reference docs edited directly
docs/docs/reference/typescript-api/custom-fields/typed-custom-single-field-config.mdx is auto-generated and will be overwritten on the next npm run docs:build. This file shouldn't be edited manually — the line number change will happen automatically when the source types change.
3. Documentation for new properties
The new sections for cascade, onDelete, and onUpdate in the custom fields guide look good. Consider adding a note about the implications of cascade delete — e.g., that using onDelete: 'CASCADE' on a relation to a core entity could cascade-delete core data.
Should address
4. No guardrails on cascade delete of core entities
With this change, a plugin author could write:
{
name: 'myThing',
type: 'relation',
entity: Product,
onDelete: 'CASCADE',
}This would cascade-delete Product rows through a custom field relation. At minimum, a runtime Logger.warn() when cascade delete targets a built-in Vendure entity would help prevent accidents.
5. No tests
Adding options that affect database schema and cascade behavior should ideally have at least a basic test — e.g., verifying that onDelete/cascade are passed through to the TypeORM metadata correctly.
Looks good
The Pick<RelationOptions, 'cascade' | 'onDelete' | 'onUpdate' | 'eager'> approach is clean and keeps the types in sync with TypeORM. The eager type didn't change, so this is backwards-compatible.
|
Thanks for the comprehensive feedback!
I will implement the rest of the feedback asap. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/guides/developer-guide/custom-fields/index.mdx`:
- Line 1361: The sentence uses singular agreement incorrectly; update the text
so it reads that the various configuration options for each of the built-in form
inputs (e.g. `suffix`) are documented in the `DefaultFormConfigHash` object,
i.e., change "form input ... is documented" to "form inputs ... are documented"
and keep the reference to `DefaultFormConfigHash`.
- Around line 1189-1192: The docs currently describe onDelete: 'CASCADE'
backwards; update the text around the onDelete: 'CASCADE' explanation so it
states that when the referenced entity is deleted the database will cascade and
delete the rows that reference it (i.e. the owning/child rows), not that
deleting the owner deletes the referenced entity; adjust the ProductVariant
example to show that if the referenced ProductVariant is deleted any entities
that reference it will be removed, and add a short caution to use onDelete:
'CASCADE' only when you intend child rows to be removed on referenced-entity
deletion.
In `@packages/core/src/entity/register-custom-entity-fields.ts`:
- Around line 51-60: The warning logic incorrectly triggers for all relation
kinds and misstates the delete direction; update the check so it only runs for
ManyToOne relations (e.g. guard on relation type / kind === 'ManyToOne' or
equivalent) in the register-custom-entity-fields flow, and change the
Logger.warn text (while still referencing relatedEntityName and coreEntitiesMap)
to say that deleting the referenced core entity will cascade and delete the
custom entity rows that reference it (not the reverse). Ensure the condition
still checks onDelete === 'CASCADE' and coreEntitiesMap membership before
logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1241bc36-91fa-4ac1-b131-185abdb966cd
📒 Files selected for processing (2)
docs/docs/guides/developer-guide/custom-fields/index.mdxpackages/core/src/entity/register-custom-entity-fields.ts
There was a problem hiding this comment.
The tests are vibe coded. They seem to test the right behaviour and look good to me. But please double check and I would need some help if anything is missing in the test file.
Author: @coderabbitai Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…gn/vendure into feat/custom-field-relations
9355c1d to
42ef82b
Compare
Description
Adds properties like cascade, onDelete and onUpdate to custom fields of type
relationBreaking changes
Does this PR include any breaking changes we should be aware of?
Screenshots
You can add screenshots here if applicable.
Checklist
📌 Always:
👍 Most of the time: