refactor(core): Generalize binary data manager interface (no-changelog) #7164

ivov · 2023-09-13T15:56:58Z

Depends on: #7092 | Story: PAY-768

This PR:

Generalizes the IBinaryDataManager interface.
Adjusts Filesystem.ts to satisfy the interface.
Sets up an S3 client stub to be filled in in the next PR.
Turns BinaryDataManager into an injectable service.
Adjusts the config schema and adds new validators.

Note that the PR looks large but all the main changes are in packages/core/src/binaryData.

Out of scope:

BinaryDataManager (now BinaryDataService) and Filesystem.ts (now fs.client.ts) were slightly refactored for maintainability, but fully overhauling them is not the focus of this PR, which is meant to clear the way for the S3 implementation. Future improvements for these two should include setting up a backwards-compatible dir structure that makes it easier to locate binary data files to delete, removing duplication, simplifying cloning methods, using integers for binary data size instead of prettyBytes(), writing tests for existing binary data logic, etc.

github-actions · 2023-09-20T17:40:34Z

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.

Files matching packages/nodes-base/nodes/**:

Added workflow tests for nodes if possible.

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

netroy · 2023-09-21T08:21:27Z

packages/cli/src/config/schema.ts

@@ -900,9 +893,9 @@ export const schema = {
 		},
 	},

-	binaryDataManager: {
+	binaryDataService: {
 		availableModes: {


Renaming this is a breaking change, and should be documented in BREAKING-CHANGES.md

Reverted: 605d02b

Maybe in future we can bundle all changes like this together, to minimize annoyance for users.

if we are reverting, can we revert all of the renaming, and not just the config? either that, or we don't revert and list this as a breaking change.

I think there is benefit in a temporary middle ground. Preexisting code uses "manager" for both binary data manager and its modes, so it's rather confusing and inconsistent. With this division into service and managers, the distinction is clearer, and the inconsistency is reduced to only this handful of entrypoint config spots where the renaming would cause a breaking change. So on balance the bulk of code becomes clearer.

krynble

LGTM

krynble · 2023-09-20T14:22:21Z

packages/core/src/BinaryData/BinaryData.service.ts

+		if (this.availableModes.includes('filesystem') && config.mode === 'filesystem') {
+			this.managers.filesystem = new FileSystemManager(config.localStoragePath);
+
+			await this.managers.filesystem.init();


Loading this conditionally means that if we switch back to default after using filesystem for a while means we cannot read previous executions anymore. Given we want to allow people to switch freely between modes, I think we always have to init all managers based on the available modes rather than the default one, wdyt?

Thank you, please see 20d5ea7.

krynble · 2023-09-22T08:51:06Z

packages/core/src/BinaryData/FileSystem.manager.ts

+import type { BinaryData } from './types';
+
+const EXECUTION_ID_EXTRACTOR =
+	/^(\w+)(?:[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12})$/;


For learning only, this regular expression can be simplified to use lazy operator instead of lookahead, which is a bit more expensive. Feel free to ignore.

Suggested change

/^(\w+)(?:[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12})$/;

/^(.*?)([0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12})$/;

Love it, I didn't know this. Since there are no tests and we will be refactoring this in future, for now I'll keep it as it was beforehand.

krynble · 2023-09-22T08:55:45Z

packages/core/src/BinaryData/utils.ts

+ * - `filesystem` (on disk)
+ * - `object` (S3)
+ */
+export const BINARY_DATA_MODES = ['default', 'filesystem', 'object'] as const;


The name object makes me think of the native js Object and I have a tendency to think this is somewhat related to a hash map.

What about just calling it S3 since this is what it does reflect?

Please see 0cdfb79

cypress · 2023-09-22T09:19:48Z

1 flaky test on run #2252 ↗︎

0	235	3	0	1

Details:

🌳 pay-768-generalize-binary-data-manager-interface 🖥️ browsers:node18.12.0-chr...
Project: n8n	Commit: `d27e4c4467`
Status: Passed	Duration: 08:33 💡
Started: Sep 22, 2023 2:42 PM	Ended: Sep 22, 2023 2:51 PM

cypress/e2e/24-ndv-paired-item.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
NDV > resolves expression with default item when input node is not parent, while still pairing items		`Output` `Screenshots` `Video`

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

github-actions · 2023-09-22T09:19:54Z

✅ All Cypress E2E specs passed

krynble · 2023-09-22T09:54:59Z

Small note: for some reason I'm getting this on my terminal:

Press "o" to open in Browser.
InvalidBinaryDataManagerError: No binary data manager found
InvalidBinaryDataManagerError: No binary data manager found
InvalidBinaryDataManagerError: No binary data manager found

When viewing executions. Any ideas @ivov ?

Edit: feel free to ignore, I can no longer reproduce this issue. I guess I was running an old build.

`LogCatch` was merged into master at a different file: `/packages/core/binaryData/index.ts`, which is now at `BinaryData.service.ts`

ivov · 2023-09-22T10:36:59Z

Small note: for some reason I'm getting this on my terminal:
Press "o" to open in Browser.
InvalidBinaryDataManagerError: No binary data manager found
InvalidBinaryDataManagerError: No binary data manager found
InvalidBinaryDataManagerError: No binary data manager found
When viewing executions. Any ideas @ivov ?

Unable to reproduce, messaging you for more details.

github-actions · 2023-09-22T11:50:37Z

✅ All Cypress E2E specs passed

krynble

LGTM

github-actions · 2023-09-22T14:51:36Z

✅ All Cypress E2E specs passed

…ager interface (no-changelog) (#7195) Depends on: #7164 | Story: [PAY-838](https://linear.app/n8n/issue/PAY-838/introduce-object-store-service-for-binary-data) This PR removes `storeMetadata` and `getSize` from the binary data manager interface, as these are specific to filesystem mode. Also this disambiguates identifiers: ``` binaryDataId filesystem:289b4aac51e-dac6-4167-b793-6d5c415e2b47 {mode}:{fileId} fileId - FS 289b4aac51e-dac6-4167-b793-6d5c415e2b47 {executionId}{uuid} fileId - S3 /workflows/{workflowId}/executions/{executionId}/binary_data/b4aac51e-dac6-4167-b793-6d5c415e2b47 ``` Note: The object store changes originally in this PR were extracted out into the final PR. --------- Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ <aditya@netroy.in>

janober · 2023-09-28T16:18:54Z

Got released with n8n@1.9.0

netroy and others added 28 commits September 4, 2023 12:37

refactor(core): Simplify executions and binary data pruning

29f1d11

Merge branch 'master' into pay-771-implement-soft-deletion

f7206fd

Merge branch 'master' into pay-771-implement-soft-deletion

9c8efbc

Remove test code from service

05e3fef

Use time constants

9dabbe8

Improve naming

3f7de8e

Use native typeorm soft deletion

5583814

Improve naming

f50108a

Make batch size class field

85ac53a

Cleanup

1d87e9e

Remove unused method

dd16e45

Cleanup

619bff6

Filter out soft-deleted executions

8cace11

Merge branch 'master' into pay-771-implement-soft-deletion

1fea046

Add tests

c4ffe6d

Improve test

738eb1b

Soft-delete in single pass

259fe75

Restore value

f3f5b27

Restore from debug value

65d17cb

Add clarifying comments

96ae7b4

Update tests

21040b5

Add typedi to core

aa4d633

Update package.json

e54becb

Turn BinaryDataManager into service

80e7258

Add object to binary data schema options

3a8960f

Use service throughout core and cli

64b1a96

Update tests

f974b55

Reduce diff

36d0243

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

ivov added 2 commits September 20, 2023 19:35

Merge master, fix conflicts

4f025b2

Fix misresolved conflict

80a4fc5

ivov added 2 commits September 20, 2023 19:44

Remove excess check on init

20d5ea7

Remove ObjectStore.manager.ts stub

0f84eb6

netroy reviewed Sep 21, 2023

View reviewed changes

ivov added 2 commits September 21, 2023 10:29

Revert rename to BinaryDataManager

605d02b

Missing renaming in test

c430f22

krynble previously approved these changes Sep 22, 2023

View reviewed changes

Rename constant

0cdfb79

ivov dismissed krynble’s stale review via 0cdfb79 September 22, 2023 09:31

ivov added 3 commits September 22, 2023 12:01

Merge master

08ce3f7

Restore LogCatch

61c2f5a

`LogCatch` was merged into master at a different file: `/packages/core/binaryData/index.ts`, which is now at `BinaryData.service.ts`

Add file not found error to getSize

74adb24

Add mode to InvalidBinaryDataManagerError

0bc2457

krynble previously approved these changes Sep 22, 2023

View reviewed changes

Delete many by execution IDs only if manager available

d27e4c4

ivov dismissed krynble’s stale review via d27e4c4 September 22, 2023 13:21

krynble approved these changes Sep 22, 2023

View reviewed changes

ivov merged commit 6d6e248 into master Sep 22, 2023
52 of 53 checks passed

ivov deleted the pay-768-generalize-binary-data-manager-interface branch September 22, 2023 15:22

janober added the Released label Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Generalize binary data manager interface (no-changelog) #7164

refactor(core): Generalize binary data manager interface (no-changelog) #7164

ivov commented Sep 13, 2023 •

edited

Loading

github-actions bot commented Sep 20, 2023

netroy Sep 21, 2023

ivov Sep 21, 2023 •

edited

Loading

netroy Sep 21, 2023

ivov Sep 21, 2023

krynble left a comment

krynble Sep 20, 2023

ivov Sep 22, 2023 •

edited

Loading

krynble Sep 22, 2023

ivov Sep 22, 2023

krynble Sep 22, 2023

ivov Sep 22, 2023

cypress bot commented Sep 22, 2023 •

edited

Loading

github-actions bot commented Sep 22, 2023

krynble commented Sep 22, 2023 •

edited

Loading

ivov commented Sep 22, 2023

github-actions bot commented Sep 22, 2023

krynble left a comment

github-actions bot commented Sep 22, 2023

janober commented Sep 28, 2023

	/^(\w+)(?:[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12})$/;
	/^(.*?)([0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12})$/;

refactor(core): Generalize binary data manager interface (no-changelog) #7164

refactor(core): Generalize binary data manager interface (no-changelog) #7164

Conversation

ivov commented Sep 13, 2023 • edited Loading

github-actions bot commented Sep 20, 2023

Choose a reason for hiding this comment

ivov Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krynble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Sep 22, 2023 • edited Loading

1 flaky test on run #2252 ↗︎

github-actions bot commented Sep 22, 2023

krynble commented Sep 22, 2023 • edited Loading

ivov commented Sep 22, 2023

github-actions bot commented Sep 22, 2023

krynble left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 22, 2023

janober commented Sep 28, 2023

ivov commented Sep 13, 2023 •

edited

Loading

ivov Sep 21, 2023 •

edited

Loading

ivov Sep 22, 2023 •

edited

Loading

cypress bot commented Sep 22, 2023 •

edited

Loading

krynble commented Sep 22, 2023 •

edited

Loading