feat(core): Add tables for reporting dashboard (no-changelog) #13336
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
af781a9 to
7645505
Compare
| async up({ dbType, schemaBuilder: { createTable, column } }: MigrationContext) { | ||
| await createTable(names.t.analyticsMetadata) | ||
| .withColumns( | ||
| column(names.c.analyticsMetadata.metaId).int.primary.autoGenerate, |
There was a problem hiding this comment.
I think typeorm uses SERIAL for postgres by default for auto generated columns, which is not recommended. We should instead use identity. With typeorm that can be done by specifying generationStrategy identity. I think we need to update our DSL to support this. We can't just change the current autoGenerate, as it would change how previous migrations behave
There was a problem hiding this comment.
I can deprecate autoGenerate and create autoGenerate2 or I can change the default, but make it configurable and update all old migrations to override it to the old default column('foo', {defaultGenerationStrategy: 'increment'}),.
I'd prefer the first one, but it depends on people reading the deprecation notice. What do you think?
There was a problem hiding this comment.
We could also create versions in the migrations:
export class AnyOldMigration implements ReversibleMigration {
version: 1
// ...
}Based on that version another context is injected. By default it's the newest version and when we do a backward incompatible change we go back to all old migrations that don't have a version set and set it to the previous one, thus locking it in.
There was a problem hiding this comment.
That's an interesting approach. I kinda like it, but it does unfortunately add complexity and a new degree of freedom. After that two different installations can behave differently. I'd say let's go with your option 1: Create autoGenerateV2 and deprecate the old 👍
packages/cli/src/databases/migrations/common/1739549398681-CreateAnalyticsTables.ts
Outdated
Show resolved
Hide resolved
|
|
||
| await createTable(names.t.analyticsRaw) | ||
| .withColumns( | ||
| column('id').int.primary.autoGenerate, |
There was a problem hiding this comment.
Same comment here regardgin the auto generate
packages/cli/src/databases/migrations/common/1739549398681-CreateAnalyticsTables.ts
Outdated
Show resolved
Hide resolved
|
|
||
| await createTable(names.t.analyticsByPeriod) | ||
| .withColumns( | ||
| column('id').int.primary.autoGenerate, |
Summary
Add schemas according to the RFC
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/PAY-2581/add-reporting-analytics-to-database
Review / Merge checklist
Docs updated or follow-up ticket created.Tests included.PR Labeled withrelease/backport(if the PR is an urgent fix that needs to be backported)