-
Notifications
You must be signed in to change notification settings - Fork 52.3k
chore(core): Set foreign key constraint on ChatHub agentId #23275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
E2E Tests: n8n tests passed after 7m 35.6s Run Details
Groups
This message was posted automatically by
currents.dev | Integration Settings
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 5 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/@n8n/db/src/migrations/common/1765886667897-AddAgentIdForeignKeys.ts">
<violation number="1" location="packages/@n8n/db/src/migrations/common/1765886667897-AddAgentIdForeignKeys.ts:13">
P1: Hardcoded double-quote column escaping (`"agentId"`) will fail on MySQL/MariaDB. Use `escape.columnName('agentId')` to ensure cross-database compatibility.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
packages/@n8n/db/src/migrations/common/1765886667897-AddAgentIdForeignKeys.ts
Outdated
Show resolved
Hide resolved
guillaumejacquart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one question
| 'agentId', | ||
| [table.agents, 'id'], | ||
| 'FK_chat_hub_sessions_agentId', | ||
| 'SET NULL', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not cascade delete ? Is there a possibility that the agent is deleted and we should still keep the messages ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to keep sessions and messages, as these are historical data to browse even when the user doesn't need the agent anymore for new conversations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems better & more intuitive than deletion
guillaumejacquart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
|
Got released with |
Summary
This PR follows up #23032 to introduce foreign key constraint on
agentIdcolumn in the ChatHub session and message table.Related Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)