feat: Synchronize deletions when pulling from source control #12170

despairblue · 2024-12-11T17:19:04Z

Summary

This PR changes how pulling changes from the git repo works. If workflows, credentials, tags or variables exist locally, but not in the remote, they will be deleted locally.

That is to keep production environment clean by keeping them more tightly in sync with the staging environments.

This PR contains the BE and FE changes, make sure to review them respectively.

Related Linear tickets, Github issues, and Community forum posts

PAY-2304 BE
PAY-2428 FE

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
- https://linear.app/n8n/issue/DOC-1255/document-latest-changes-and-improvements-to-environments
Tests included.
~~PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)~~

…licts on the FE

codecov · 2025-01-09T12:09:00Z

Codecov Report

Attention: Patch coverage is 79.78339% with 56 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...nts.ee/source-control/source-control.service.ee.ts	27.27%	32 Missing ⚠️
...source-control/source-control-import.service.ee.ts	16.66%	12 Missing and 3 partials ⚠️
packages/editor-ui/src/utils/sourceControlUtils.ts	96.19%	2 Missing and 2 partials ⚠️
...ackages/cli/src/credentials/credentials.service.ts	66.66%	2 Missing ⚠️
....ee/source-control/source-control.controller.ee.ts	0.00%	1 Missing ⚠️
.../handlers/source-control/source-control.handler.ts	0.00%	1 Missing ⚠️
...tor-ui/src/components/MainSidebarSourceControl.vue	66.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

…-moving-between-projects

packages/editor-ui/src/components/MainSidebarSourceControl.test.ts

packages/editor-ui/src/components/MainSidebarSourceControl.vue

packages/editor-ui/src/utils/sourceControlUtils.ts

netroy · 2025-01-13T16:27:04Z

packages/cli/src/commands/ldap/reset.ts

@@ -110,7 +111,7 @@ export class Reset extends BaseCommand {
 		}

 		for (const credential of ownedCredentials) {
-			await Container.get(CredentialsService).delete(credential);
+			await Container.get(CredentialsRepository).remove(credential);


this change means that we now do not run the credentials.delete hook.
I don't know if that is what we want or not. Just want to confirm that this change is intentional.

That wasn't intentional.

I changed it back to calling CredentialsService.delete and passing the owner. That way it's in line with deleting the workflow above it:

n8n/packages/cli/src/commands/ldap/reset.ts

Lines 108 to 114 in 09115e0

for (const { workflowId } of ownedSharedWorkflows) {

await Container.get(WorkflowService).delete(owner, workflowId);

}

for (const credential of ownedCredentials) {

await Container.get(CredentialsService).delete(owner, credential.id);

}

netroy · 2025-01-13T16:28:47Z

packages/cli/src/credentials/credentials.service.ts

+	async delete(user: User, credentialId: string) {
+		await this.externalHooks.run('credentials.delete', [credentialId]);
+
+		const credential = await this.sharedCredentialsRepository.findCredentialForUser(


The check for if a user has access to a credential belongs in the controller (or in a middleware) IMO.

Oh, I've moved that to the services as much as possible with RBAC, because

it's more reusable: every controller that wants to get, update or delete a resource does not need access to the repositories to first get that resource and then pass it into a service function, although that would make service functions easier to test

it's safer: e.g. you can call credService.remove(user, ids) from any place and be certain that it will check if the user is allowed to do that action or not, otherwise we put the onus of not creating security flows on the caller, not the callee

transaction handling is easier, the controller does not need to create a transaction, use repositories to get the resources that the user can use in that context, then pass this to one or multiple service functions passing the transaction context as well

at least workflows currently work like that

n8n/packages/cli/src/workflows/workflow.service.ts

Lines 275 to 282 in 1f5d8c2

/**

* Deletes a workflow and returns it.

*

* If the workflow is active this will deactivate the workflow.

* If the user does not have the permissions to delete the workflow this does

* nothing and returns void.

*/

async delete(user: User, workflowId: string): Promise<WorkflowEntity | undefined> {

That said, I can change it, if we already have a consensus about this.

cypress · 2025-01-14T08:33:14Z

n8n Run #8845

Run Properties: Passed #8845 • 9b9c452aa6: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*

Project	`n8n`
Branch Review	`pay-2304-spike-sync-deleting-workflows-and-moving-between-projects`
Run status	`Passed #8845`
Run duration	`04m 49s`
Commit	`9b9c452aa6: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*`
Committer	`r00gm`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`1`
Pending	`0`
Skipped	`0`
Passing	`489`
View all changes introduced in this branch ↗︎

github-actions · 2025-01-14T08:33:14Z

✅ All Cypress E2E specs passed

…-moving-between-projects

netroy

I'm not familiar enough with a lot of this code, but the parts I do understand, look fine to me. but left a comment about the deletion queries.

netroy · 2025-01-20T13:47:43Z

packages/cli/src/environments.ee/source-control/source-control-import.service.ee.ts

+	async deleteWorkflowsNotInWorkfolder(user: User, candidates: SourceControlledFile[]) {
+		for (const candidate of candidates) {
+			await this.workflowService.delete(user, candidate.id);
+		}
+	}
+
+	async deleteCredentialsNotInWorkfolder(user: User, candidates: SourceControlledFile[]) {
+		for (const candidate of candidates) {
+			await this.credentialsService.delete(user, candidate.id);
+		}
+	}
+
+	async deleteVariablesNotInWorkfolder(candidates: SourceControlledFile[]) {
+		for (const candidate of candidates) {
+			await this.variablesService.delete(candidate.id);
+		}
+	}
+
+	async deleteTagsNotInWorkfolder(candidates: SourceControlledFile[]) {
+		for (const candidate of candidates) {
+			await this.tagService.delete(candidate.id);
+		}
+	}
+


these currently fire multiple read and then delete queries. maybe we should add a deleteMany method in all these services, and use that to call a single delete query for each resource type.

I can do that. Honestly I avoided this because I don't think this is a hot path and being correct was more important to me than being efficient and those functions will always work and there is no chance that they get out of sync with the deleteMany and vice versa.

What I could do is create a deleteMany and implement the delete using that, similar to this: 307ede1 (#12329)

If you approve I'll do that, let's just agree on what we want before we start to work on it.

github-actions · 2025-01-20T15:36:32Z

✅ All Cypress E2E specs passed

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Dec 11, 2024

despairblue force-pushed the pay-2304-spike-sync-deleting-workflows-and-moving-between-projects branch from 53630b1 to 002d0b6 Compare December 30, 2024 10:12

despairblue added 6 commits December 30, 2024 11:24

check a user's permission before deleting it

c93c018

add jsdoc

e0f7d40

dont' filter out locally or remotely created credentials when pulling

1bb030a

extract filter functions

4198543

delete workflows, credentials, tags and variables when pulling

f46d07e

show credentials, workflows, tags and variables to be deleted as conf…

b398a56

…licts on the FE

despairblue force-pushed the pay-2304-spike-sync-deleting-workflows-and-moving-between-projects branch from 002d0b6 to b398a56 Compare January 9, 2025 11:47

despairblue changed the title ~~feature: synchronize deletions when pulling from source control~~ feature: Synchronize deletions when pulling from source control Jan 9, 2025

r00gm and others added 10 commits January 10, 2025 15:59

WIP: pull modal UI

1a5fc09

chore: code clean up

2dd1459

Merge branch 'master' into pay-2304-spike-sync-deleting-workflows-and…

a04f7c0

…-moving-between-projects

fix: type

ade8362

fix: service

a8e1e94

use static DI instead of dynamic DI

181918e

chore: DRY user notifications

d8ad3ab

fix test

c33b873

test: increase coverage

aaa2d2d

test: pull modal

d0d9211

r00gm reviewed Jan 13, 2025

View reviewed changes

packages/editor-ui/src/components/MainSidebarSourceControl.test.ts Show resolved Hide resolved

r00gm reviewed Jan 13, 2025

View reviewed changes

packages/editor-ui/src/components/MainSidebarSourceControl.vue Show resolved Hide resolved

r00gm reviewed Jan 13, 2025

View reviewed changes

packages/editor-ui/src/components/MainSidebarSourceControl.vue Show resolved Hide resolved

chore: clean up comments and unused variables

1f5d8c2

r00gm reviewed Jan 13, 2025

View reviewed changes

packages/editor-ui/src/utils/sourceControlUtils.ts Outdated Show resolved Hide resolved

r00gm changed the title ~~feature: Synchronize deletions when pulling from source control~~ feat: Synchronize deletions when pulling from source control Jan 13, 2025

despairblue marked this pull request as ready for review January 13, 2025 13:29

netroy reviewed Jan 13, 2025

View reviewed changes

cstuncsik previously approved these changes Jan 14, 2025

View reviewed changes

netroy self-requested a review January 14, 2025 08:26

call CredentialsService.delete again from the ldap command

09115e0

despairblue dismissed cstuncsik’s stale review via 09115e0 January 14, 2025 09:52

r00gm added 2 commits January 20, 2025 12:21

Merge branch 'master' into pay-2304-spike-sync-deleting-workflows-and…

1273e31

…-moving-between-projects

don't close notification on click, redirect to filtered variables

9b9c452

netroy reviewed Jan 20, 2025

View reviewed changes

netroy approved these changes Jan 20, 2025

View reviewed changes

despairblue merged commit 967ee4b into master Jan 20, 2025
63 checks passed

despairblue deleted the pay-2304-spike-sync-deleting-workflows-and-moving-between-projects branch January 20, 2025 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Synchronize deletions when pulling from source control #12170

feat: Synchronize deletions when pulling from source control #12170

despairblue commented Dec 11, 2024 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

netroy Jan 13, 2025

despairblue Jan 14, 2025

netroy Jan 13, 2025

despairblue Jan 14, 2025

cypress bot commented Jan 14, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025

netroy left a comment

netroy Jan 20, 2025

despairblue Jan 20, 2025

github-actions bot commented Jan 20, 2025

	for (const { workflowId } of ownedSharedWorkflows) {
	await Container.get(WorkflowService).delete(owner, workflowId);
	}

	for (const credential of ownedCredentials) {
	await Container.get(CredentialsService).delete(owner, credential.id);
	}

	/**
	* Deletes a workflow and returns it.
	*
	* If the workflow is active this will deactivate the workflow.
	* If the user does not have the permissions to delete the workflow this does
	* nothing and returns void.
	*/
	async delete(user: User, workflowId: string): Promise<WorkflowEntity \| undefined> {

feat: Synchronize deletions when pulling from source control #12170

feat: Synchronize deletions when pulling from source control #12170

Conversation

despairblue commented Dec 11, 2024 • edited Loading

Summary

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

netroy Jan 13, 2025

Choose a reason for hiding this comment

despairblue Jan 14, 2025

Choose a reason for hiding this comment

netroy Jan 13, 2025

Choose a reason for hiding this comment

despairblue Jan 14, 2025

Choose a reason for hiding this comment

cypress bot commented Jan 14, 2025 • edited Loading

n8n Run #8845

github-actions bot commented Jan 14, 2025

netroy left a comment

Choose a reason for hiding this comment

netroy Jan 20, 2025

Choose a reason for hiding this comment

despairblue Jan 20, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 20, 2025

despairblue commented Dec 11, 2024 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

cypress bot commented Jan 14, 2025 •

edited

Loading