-
Notifications
You must be signed in to change notification settings - Fork 38.8k
chore(core): Move scopes and roles into database in preparation for custom roles #17226
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
base: master
Are you sure you want to change the base?
Conversation
❌ 1631 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
4d9a6d0
to
6602f56
Compare
49e3965
to
14920c9
Compare
BundleMonFiles added (2)
Total files change +255.73KB Groups added (2)
Final result: ✅ View report in BundleMon website ➡️ |
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.
cubic analysis
10 issues found across 26 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai
to give feedback, ask questions, or re-run the review.
systemRole: true, | ||
}, | ||
); | ||
} catch (error) { |
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.
Catching and ignoring every error hides real migration failures; consider only suppressing duplicate-key violations.
Prompt for AI agents
Address the following comment on packages/@n8n/db/src/migrations/common/1750252139168-LinkRoleToUserTable.ts at line 30:
<comment>Catching and ignoring every error hides real migration failures; consider only suppressing duplicate-key violations.</comment>
<file context>
@@ -0,0 +1,78 @@
+import type { MigrationContext, ReversibleMigration } from '../migration-types';
+
+/*
+ * This migration
+ */
+
+export class LinkRoleToUserTable1750252139168 implements ReversibleMigration {
+ async up({
+ schemaBuilder: { addForeignKey, addColumns, column },
</file context>
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.
Agree with the bot here 🤣
packages/@n8n/db/src/migrations/common/1750252139168-LinkRoleToUserTable.ts
Show resolved
Hide resolved
packages/@n8n/db/src/migrations/common/1750252139168-LinkRoleToUserTable.ts
Outdated
Show resolved
Hide resolved
packages/@n8n/db/src/migrations/common/1750252139168-LinkRoleToUserTable.ts
Outdated
Show resolved
Hide resolved
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.
some minor comments
@@ -105,7 +105,7 @@ export interface PublicUser { | |||
passwordResetToken?: string; | |||
createdAt: Date; | |||
isPending: boolean; | |||
role?: GlobalRole; | |||
role?: string; |
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.
Could we do this on followup PR ?
systemRole: true, | ||
}, | ||
); | ||
} catch (error) { |
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.
Agree with the bot here 🤣
const systemRoleColumn = escape.columnName('systemRole'); | ||
|
||
// Make sure that the global roles that we need exist | ||
try { |
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.
DO you think it would make sense to have a loop here (of ['global:owner', ''global:admin', 'global:member']) ?
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.
Some additional minor comments
packages/@n8n/db/src/migrations/common/1750252139167-AddRolesTables.ts
Outdated
Show resolved
Hide resolved
packages/@n8n/db/src/migrations/common/1750252139168-LinkRoleToUserTable.ts
Show resolved
Hide resolved
packages/@n8n/db/src/migrations/common/1750252139168-LinkRoleToUserTable.ts
Show resolved
Hide resolved
for (const role of ['global:owner', 'global:admin', 'global:member']) { | ||
if (dbType === 'sqlite') { | ||
await runQuery( | ||
`INSERT OR REPLACE INTO ${tableName} (${slugColumn}, ${roleTypeColumn}, ${systemRoleColumn}) VALUES (:slug, :roleType, :systemRole)`, |
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.
Sqlite supports upsert now: https://sqlite.org/lang_upsert.html
So it could have the same syntax as postgres I think.
Plus currently, your sqlite/mysql query replace in case of conflict, while the postgres one ignore on conflict
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.
You are right, I was able to change the insert queries to all ignore on conflict, but still all 3 db queries are different, the sqlite one needs an INSERT OR REPLACE, which postgres doesn't recognize.
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 was under the impression that Sqlite did not need INSERT OR REPLACE (see examples here): https://sqlite.org/lang_upsert.html#examples
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.
Ah you are right, I miss read 🙈
}; | ||
return [slug, info.displayName, info.description ?? null] as const; | ||
}) | ||
.filter(([slug, displayName, description]) => { |
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.
(completely optional)
I'm questioning the relevance of splitting filter with map, because we need to check existing scope exists in both steps.
What about:
const scopesToSave = ALL_SCOPES.map((slug) => {
const info = scopeInformation[slug] ?? { displayName: slug, description: null };
const existing = existingScopes.find(scope => scope.slug === slug);
// Create new scope if doesn't exist
if (!existing) {
const newScope = new Scope();
newScope.slug = slug;
newScope.displayName = info.displayName;
newScope.description = info.description ?? null;
return newScope;
}
// Update existing scope if needed
const needsUpdate = existing.displayName !== info.displayName ||
existing.description !== (info.description ?? null);
if (needsUpdate) {
existing.displayName = info.displayName;
existing.description = info.description ?? null;
return existing;
}
return null;
}).filter(Boolean);
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
packages/@n8n/db/src/migrations/common/1750252139168-LinkRoleToUserTable.ts
Show resolved
Hide resolved
f6c05ad
to
680c013
Compare
E2E Tests: n8n tests passed after 5m 29.4s Run Details
This message was posted automatically by
currents.dev | Integration Settings
|
default: false, | ||
name: 'systemRole', | ||
}) | ||
systemRole: boolean; // Indicates if the role is managed by the system and cannot be edited |
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.
Can we move the comments here to JSDoc so they are discoverable via intellisense?
async up({ schemaBuilder: { createTable, column } }: MigrationContext) { | ||
await createTable('scope').withColumns( | ||
column('slug').varchar(128).primary.notNull, | ||
column('displayName').text.default(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.
This seems to be not null in the entity definition. Probably should be here as well? Same applies to the other migrations
* slug | Text | Unique identifier of the scope for example: 'project:create' | ||
* displayName | Text | Name used to display in the UI | ||
* description | Text | Text describing the scope in more detail of users |
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.
Could we add these descriptions as comments to the columns with .comment(...)
. Same applies to the other migrations
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.
Any reason why we have 3 separate migrations instead of 1?
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 wanted to keep the migrations small and simple and specific to do one thing.
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.
Personally I'd prefer just one, since every migration is extra overhead in instance startup. One migration can also be composed into smaller functions. But I can understand your reasoning as well.
column('slug').varchar(128).primary.notNull, | ||
column('displayName').text.default(null), | ||
column('description').text.default(null), | ||
column('roleType').text.default(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.
Could we have a constraint that enforces the valid values for this column?
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.
We opted to not validate the values in the database in there, because we might want to add more values here later on, at which point we would need to do another migration to extend the the type.
return resourceScopes; | ||
} | ||
|
||
export const ALL_SCOPES = buildResourceScopes(); |
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.
What do you think about having a test case that does expect(ALL_SCOPES).toMatchSnapshot()
so a) we have a list of all scopes and b) one needs explicit action if they are changed?
description: null, | ||
}; | ||
|
||
const existingScope = availableScopes.find((scope) => scope.slug === slug); |
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.
nit: This is a linear search. The array is probably not big, but it all adds up. Using e.g. Map
for availableScopes
would be more approriate
for (const roleNamespace of Object.keys(ALL_ROLES) as Array<keyof typeof ALL_ROLES>) { | ||
const rolesToUpdate = ALL_ROLES[roleNamespace] | ||
.map((role) => { | ||
const existingRole = existingRoles.find((r) => r.slug === role.role); |
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.
Same here
await this.scopeRepository.save(scopesToUpdate); | ||
this.logger.info('Scopes updated successfully.'); | ||
} else { | ||
this.logger.info('No scopes to update.'); |
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.
Would debug be more approriate here? Not sure if this is needs to be logged on every startup
await this.roleRepository.save(rolesToUpdate); | ||
this.logger.info(`${roleNamespace} roles updated successfully.`); | ||
} else { | ||
this.logger.info(`No ${roleNamespace} roles to update.`); |
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.
Same here
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.
Very clean and easy to follow code. Nice job 👏
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 only looked at the migrations and they look solid. 👍🏾
3bdc588
680c013
to
3bdc588
Compare
99f21b5
to
55e8cc9
Compare
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.
Thank you for addressing the feedback 💘 LGTM now. Just one question regarding multi-main mode
} | ||
} | ||
|
||
async init() { |
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.
Does this still behave correctly when we have multiple mains starting up simultaneously and all racing to run this?
Thanks, that is a good question: |
Summary
We move scopes and roles into the database and use the database as the source of truth for these. This is to
prepare the system for custom roles, which can be stored in the database.
Related Linear tickets, Github issues, and Community forum posts
closes https://linear.app/n8n/issue/PAY-3054/move-role-definitions-into-the-database
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)