fix(core): Optimize SharedWorkflow queries #6297

flipswitchingmonkey · 2023-05-22T15:52:12Z

No description provided.

github-actions · 2023-05-22T15:52:42Z

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

If fixing bug, added test to cover scenario.
If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

Added unit tests to cover new or updated functionality.

Make sure to check off this list before asking for review.

netroy · 2023-05-22T16:11:17Z

packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts

@@ -24,8 +24,6 @@ export async function getSharedWorkflowIds(user: User): Promise<string[]> {
 		select: ['workflowId'],
 	});
 	return sharedWorkflows.map(({ workflowId }) => workflowId);
-
-	return sharedWorkflows.map(({ workflowId }) => workflowId);


This is a good argument for enabling no-unreachable linting rule.

netroy · 2023-05-22T16:12:22Z

packages/cli/src/ActiveWorkflowRunner.ts

@@ -348,30 +345,31 @@ export class ActiveWorkflowRunner {
 	/**
 	 * Returns the ids of the currently active workflows
 	 */
-	async getActiveWorkflows(user?: User): Promise<IWorkflowDb[]> {
+	async getActiveWorkflows(user?: User): Promise<string[]> {


How about

Suggested change

async getActiveWorkflows(user?: User): Promise<string[]> {

async getActiveWorkflowIds(user?: User): Promise<IWorkflowDb['id'][]> {

that'd be an {id:string} object instead of string[], which for some reason the frontend expects

codecov · 2023-05-22T16:12:42Z

Codecov Report

Patch coverage: 40.00% and no project coverage change.

Comparison is base (2d90125) 27.65% compared to head (283b198) 27.65%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6297   +/-   ##
=======================================
  Coverage   27.65%   27.65%           
=======================================
  Files        2948     2948           
  Lines      180885   180885           
  Branches    19647    19645    -2     
=======================================
  Hits        50019    50019           
  Misses     130119   130119           
  Partials      747      747

Impacted Files	Coverage Δ
...blicApi/v1/handlers/workflows/workflows.service.ts	`92.30% <ø> (+1.74%)`	⬆️
packages/cli/src/Server.ts	`0.00% <0.00%> (ø)`
...ckages/cli/src/UserManagement/PermissionChecker.ts	`73.77% <ø> (ø)`
packages/cli/src/ActiveWorkflowRunner.ts	`45.17% <44.44%> (-0.32%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

netroy · 2023-05-22T16:15:08Z

packages/cli/src/ActiveWorkflowRunner.ts

 		if (!user || user.globalRole.name === 'owner') {
 			activeWorkflows = await Db.collections.Workflow.find({
 				select: ['id'],
 				where: { active: true },
 			});
+			return activeWorkflows.map((workflow) => workflow.id.toString());


why is the .toString needed? this is already handled in the ORM code, and at this point the id should already be a string.

for some reason js turns the string into a number again, which lets the tests fail because we get arrays of [1, '1'] now. so i'm forcing the string

this sounds like a bug that should be fixed, instead of being worked around.

could be a mix of TypeORM, since on the db level the ids are still INT (which will change very soon), and js type inferance.

krynble

LGTM - n8n is able to start, acitvate and deactivate workflows fine even with the changes. Listing executions also does not seem to have been negatively affected.

krynble

LGTM - n8n is able to start, acitvate and deactivate workflows fine even with the changes. Listing executions also does not seem to have been negatively affected.

github-actions · 2023-05-23T07:34:22Z

✅ All Cypress E2E specs passed

github-actions · 2023-05-23T07:35:41Z

✅ All Cypress E2E specs passed

* allow execution on node 19,20,22 * fix(ERPNext Node): Fix issue with credential test and add frappe cloud url (n8n-io#6283) * fix(editor): UI copy fix for Date & Time node (no-changelog) (n8n-io#6291) * whether -> when * lint fix --------- Co-authored-by: Jonathan Bennetts <jonathan.bennetts@gmail.com> * feat(RabbitMQ Node): Add mode for acknowledging and deleting from queue later in workflow (n8n-io#6225) * Add later in workflow mode * Add new operation * Acknowledge message in next node * Add response and emit for responsePromiseHook * Remove double success message, close channel correctly * Answser messages correctly * Remove option from delete operation * move operation name to camelCase * Fix versioning * To remove: add action item in v1 * Add notice for delete from queue * Correctly only execute only the delete operation * Refactor delete from queue operator and add return last items --------- Co-authored-by: Marcus <marcus@n8n.io> * feat: Add dangerouslyUseHTMLString where needed (no-changelog) (n8n-io#6292) feat: add dangerouslyUseHTMLString where needed (no-changelog) * fix(Wekan Node): Handle response correctly (n8n-io#6296) Fix bug when response wasn't array * Added procfile * Added procfile * redployed * redployed * redployed * redployed * redployed * redployed * dont start * redployed * redployed * update version * redployed * redployed * redployed * redployed * Revert "update version" This reverts commit c249585. * add heroku stack version * delete * add app.json setting heroku build stack * update * update vite * update mem * update PORT * redployed * fix(core): Optimize SharedWorkflow queries (n8n-io#6297) * optimize SharedWorkflow queries * fix int to string ids * fix(Strapi Node): Strapi credentials notice (n8n-io#6289) * fix(SSH Node): Private key field as password and credential test (n8n-io#6298) * fix: Prevent removing manual executions when setting says to save (n8n-io#6300) * fix: Initialize license in queue mode correctly (n8n-io#6301) * feat(Ldap Node): Add LDAP node (n8n-io#4783) * feat(LoneScale Node): Add LoneScale node and Trigger node (n8n-io#5146) * feat: Add SSO SAML metadataUrl support and various improvements (n8n-io#6139) * feat: add various sso improvements * fix: remove test button assertion * fix: fix type imports * test: attempt fixing unit tests * fix: changed to using useToast for error toasts * Minor copy tweaks and swapped buttons position. * fix locale ref * align error with UI wording * simplify saving ux * fix pretty * fix: update saml sso setting saving * fix: undo try/catch changes when saving saml config * metadata url tab selected at first * chore: fix linting issue * test: fix activation checkbox test --------- Co-authored-by: Giulio Andreini <g.andreini@gmail.com> Co-authored-by: Michael Auerswald <michael.auerswald@gmail.com> Co-authored-by: Romain Minaud <romain.minaud@gmail.com> * fix(editor): Fix canvas loading when page gets restored from bfcache (n8n-io#6304) * fix(editor): Fix canvas loading when page gets restored from bfcache * Lint fix * fix(editor): Link to log streaming doc from log streaming (no-changelog) (n8n-io#6303) link to log streaming doc from log streaming --------- Co-authored-by: Eyal Fishler <eyalfishler@gmail.com> Co-authored-by: Jon <jonathan.bennetts@gmail.com> Co-authored-by: Deborah <deborah@starfallprojects.co.uk> Co-authored-by: agobrech <45268029+agobrech@users.noreply.github.com> Co-authored-by: Marcus <marcus@n8n.io> Co-authored-by: Alex Grozav <alex@grozav.com> Co-authored-by: Omri Attoun <omri@joyous.team> Co-authored-by: Michael Auerswald <michael.auerswald@gmail.com> Co-authored-by: Michael Kret <88898367+michael-radency@users.noreply.github.com> Co-authored-by: Omar Ajoue <krynble@gmail.com> Co-authored-by: Yann ALEMAN <yann.aleman27@gmail.com> Co-authored-by: Giulio Andreini <g.andreini@gmail.com> Co-authored-by: Romain Minaud <romain.minaud@gmail.com> Co-authored-by: OlegIvaniv <me@olegivaniv.com>

github-actions · 2023-05-24T16:55:40Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

janober · 2023-05-25T11:40:57Z

Got released with n8n@0.230.1

optimize SharedWorkflow queries

fe43d89

flipswitchingmonkey marked this pull request as draft May 22, 2023 15:52

fix int to string ids

283b198

flipswitchingmonkey marked this pull request as ready for review May 22, 2023 15:57

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels May 22, 2023

netroy reviewed May 22, 2023

View reviewed changes

flipswitchingmonkey requested a review from netroy May 22, 2023 17:43

krynble approved these changes May 23, 2023

View reviewed changes

flipswitchingmonkey merged commit ed7f3b8 into master May 23, 2023

flipswitchingmonkey deleted the pay-454-improve-sharedworkflow-queries branch May 23, 2023 07:40

krynble approved these changes May 24, 2023

View reviewed changes

janober added the Released label May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Optimize SharedWorkflow queries #6297

fix(core): Optimize SharedWorkflow queries #6297

flipswitchingmonkey commented May 22, 2023

github-actions bot commented May 22, 2023

netroy May 22, 2023

netroy May 22, 2023

flipswitchingmonkey May 22, 2023

codecov bot commented May 22, 2023

netroy May 22, 2023

flipswitchingmonkey May 22, 2023

netroy May 23, 2023

flipswitchingmonkey May 23, 2023

krynble left a comment

krynble left a comment

github-actions bot commented May 23, 2023

github-actions bot commented May 23, 2023

github-actions bot commented May 24, 2023

janober commented May 25, 2023

	async getActiveWorkflows(user?: User): Promise<string[]> {
	async getActiveWorkflowIds(user?: User): Promise<IWorkflowDb['id'][]> {

fix(core): Optimize SharedWorkflow queries #6297

fix(core): Optimize SharedWorkflow queries #6297

Conversation

flipswitchingmonkey commented May 22, 2023

github-actions bot commented May 22, 2023

netroy May 22, 2023

Choose a reason for hiding this comment

netroy May 22, 2023

Choose a reason for hiding this comment

flipswitchingmonkey May 22, 2023

Choose a reason for hiding this comment

codecov bot commented May 22, 2023

Codecov Report

netroy May 22, 2023

Choose a reason for hiding this comment

flipswitchingmonkey May 22, 2023

Choose a reason for hiding this comment

netroy May 23, 2023

Choose a reason for hiding this comment

flipswitchingmonkey May 23, 2023

Choose a reason for hiding this comment

krynble left a comment

Choose a reason for hiding this comment

krynble left a comment

Choose a reason for hiding this comment

github-actions bot commented May 23, 2023

github-actions bot commented May 23, 2023

github-actions bot commented May 24, 2023

janober commented May 25, 2023