Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(core): Fix workflow tagging failure due to unique constraint check #8505

Merged
merged 3 commits into from
Feb 2, 2024
Merged

fix(core): Fix workflow tagging failure due to unique constraint check #8505

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ae7ab2
fix(core): Fix workflow tagging failure due to unique constraint check
ivov Jan 31, 2024
cd66d35
Make test order-agnostic
ivov Jan 31, 2024
689610a
Use less verbose syntax
ivov Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/cli/src/databases/repositories/workflowTagMapping.repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,14 @@ export class WorkflowTagMappingRepository extends Repository<WorkflowTagMapping>
constructor(dataSource: DataSource) {
super(WorkflowTagMapping, dataSource.manager);
}

async overwriteTaggings(workflowId: string, tagIds: string[]) {
return await this.manager.transaction(async (tx) => {
await tx.delete(WorkflowTagMapping, { workflowId });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we change this to await this.delete({ workflowId });, would that run the query then outside the transaction somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should check the source but from quick looking at the docs, all the examples use the transactional entity manager syntax.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we know for certain that on sqlite this will definitely execute inside a transaction. we can always test this manually on postgres to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this syntax:

async overwriteTaggings(workflowId: string, tagIds: string[]) {
	return await this.manager.transaction(async () => {
		await this.delete({ workflowId });

		const taggings = tagIds.map((tagId) => this.create({ workflowId, tagId }));

		return await this.insert(taggings);
	});
}

Runs both queries inside a transaction in Postgres:

query: START TRANSACTION
query is slow: START TRANSACTION
execution time: 2
query: DELETE FROM "public"."workflows_tags" WHERE "workflowId" = $1 -- PARAMETERS: ["Jvb4CPOPXZ96KKEe"]
query is slow: DELETE FROM "public"."workflows_tags" WHERE "workflowId" = $1 -- PARAMETERS: ["Jvb4CPOPXZ96KKEe"]
execution time: 2
query: SELECT 1
query is slow: SELECT 1
execution time: 2
query: INSERT INTO "public"."workflows_tags"("workflowId", "tagId") VALUES ($1, $2) -- PARAMETERS: ["Jvb4CPOPXZ96KKEe","ovPnVOJny6XLhk0A"]
query is slow: INSERT INTO "public"."workflows_tags"("workflowId", "tagId") VALUES ($1, $2) -- PARAMETERS: ["Jvb4CPOPXZ96KKEe","ovPnVOJny6XLhk0A"]
execution time: 2
query: COMMIT

Why do we prefer this syntax over tx.delete and tx.insert? I find this more explicit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not that I prefer one way over the other. I'm just curious if we can make code like this generally less verbose by not having to pass the entity type in the repository for that entity. we don't have to pass WorkflowTagMapping in any of the non transaction calls, having to pass it in transaction calls feels like a bad API.


const taggings = tagIds.map((tagId) => this.create({ workflowId, tagId }));

return await tx.insert(WorkflowTagMapping, taggings);
});
}
}
5 changes: 1 addition & 4 deletions packages/cli/src/workflows/workflow.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,7 @@ export class WorkflowService {
);

if (tagIds && !config.getEnv('workflowTagsDisabled')) {
await this.workflowTagMappingRepository.delete({ workflowId });
await this.workflowTagMappingRepository.insert(
tagIds.map((tagId) => ({ tagId, workflowId })),
);
await this.workflowTagMappingRepository.overwriteTaggings(workflowId, tagIds);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we update the tags in the same transaction as we update the workflow itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, same for workflow history - I was looking to avoid a bigger change than for this issue. Let me know otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: https://n8nio.slack.com/archives/C069HS026UF/p1706783989128759?thread_ts=1706782548.306949&cid=C069HS026UF

}

if (workflow.versionId !== shared.workflow.versionId) {
Expand Down
90 changes: 90 additions & 0 deletions packages/cli/test/integration/workflow-tag-mapping.repository.integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import Container from 'typedi';

import * as testDb from './shared/testDb';
import { WorkflowTagMappingRepository } from '@/databases/repositories/workflowTagMapping.repository';
import { createWorkflow } from './shared/db/workflows';
import { TagRepository } from '@/databases/repositories/tag.repository';

describe('WorkflowTagMappingRepository', () => {
let taggingRepository: WorkflowTagMappingRepository;
let tagRepository: TagRepository;

beforeAll(async () => {
await testDb.init();

taggingRepository = Container.get(WorkflowTagMappingRepository);
tagRepository = Container.get(TagRepository);
});

afterEach(async () => {
await testDb.truncate(['WorkflowTagMapping', 'Workflow', 'Tag']);
});

afterAll(async () => {
await testDb.terminate();
});

describe('overwriteTaggings', () => {
test('should overwrite taggings in a workflow', async () => {
const workflow = await createWorkflow();

const oldTags = await tagRepository.save(
['tag1', 'tag2'].map((name) => tagRepository.create({ name })),
);

const oldTaggings = oldTags.map((tag) =>
taggingRepository.create({
tagId: tag.id,
workflowId: workflow.id,
}),
);

await taggingRepository.save(oldTaggings);

const newTags = await tagRepository.save(
['tag3', 'tag4'].map((name) => tagRepository.create({ name })),
);

await taggingRepository.overwriteTaggings(
workflow.id,
newTags.map((t) => t.id),
);

const taggings = await taggingRepository.findBy({ workflowId: workflow.id });

expect(taggings).toHaveLength(2);

const [firstNewTag, secondNewTag] = newTags;

expect(taggings).toEqual(
expect.arrayContaining([
expect.objectContaining({ tagId: firstNewTag.id, workflowId: workflow.id }),
expect.objectContaining({ tagId: secondNewTag.id, workflowId: workflow.id }),
]),
);
});

test('should delete taggings if no tags are provided', async () => {
const workflow = await createWorkflow();

const oldTags = await tagRepository.save(
['tag1', 'tag2'].map((name) => tagRepository.create({ name })),
);

const oldTaggings = oldTags.map((tag) =>
taggingRepository.create({
tagId: tag.id,
workflowId: workflow.id,
}),
);

await taggingRepository.save(oldTaggings);

await taggingRepository.overwriteTaggings(workflow.id, []);

const taggings = await taggingRepository.findBy({ workflowId: workflow.id });

expect(taggings).toHaveLength(0);
});
});
});
Loading