feat: Introduce `HooksService` #8962

RicardoE105 · 2024-03-24T16:39:40Z

Summary

On cloud we allow user to invite members during sign-up. This functionality was handled in part in the cloud backend hooks. In there, we were using the UserService.inviteMembers method. Sadly in this PR, we renamed the method to inviteUsers and broke the functionality. To avoid this from happening again, we are introducing the HooksService. All methods in the service must be used by the BE hooks, hence must not be removed without making sure we do not use then anymore. With this, we can avoid breaking functionally in the BE by renaming, move functionality to another service, etc.

Edit:

We also added all other DB access methods to the hooks so we can slowly migrate them to rely only on the HooksService.

Related tickets and issues

https://linear.app/n8n/issue/ADO-2264/add-hooks-service-to-n8n

Review / Merge checklist

PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
Tests included - We already tests for the downstream method.

packages/cli/src/services/hooks.service.ts

mutdmour · 2024-04-02T06:56:47Z

packages/cli/test/unit/services/hooks.service.test.ts

+		jest.clearAllMocks();
+	});
+
+	it('hooksService.inviteUsers should call userService.inviteUsers', async () => {


thank you for adding unit tests here ✅

cypress · 2024-04-02T07:06:34Z

1 flaky test on run #5406 ↗︎

0	365	0	0	1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 RicardoE105 🗃️ e2e/*
Project: n8n	Commit: `1d8084d684`
Status: Passed	Duration: 04:39 💡
Started: Jun 10, 2024 10:40 PM	Ended: Jun 10, 2024 10:44 PM

e2e/20-workflow-executions.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Current Workflow Executions > should auto load more items if there is space and auto scroll		`Test Replay` `Screenshots` `Video`

Review all test suite changes for PR #8962 ↗︎

github-actions · 2024-04-02T07:06:37Z

✅ All Cypress E2E specs passed

ivov · 2024-04-02T07:23:49Z

@RicardoE105 @mutdmour This wrapper does not protect against renaming - there's no guarantee that whoever is renaming a method will remember to check this file or even be aware it exists. Could we add an assertion to a test instead expect('inviteUsers' in userService).toBe(true)? Or is there a reason against this? I just tested and renaming inviteUsers does not affect an assertion like this.

RicardoE105 · 2024-04-02T12:42:34Z

@ivov If someone attempts to rename userService.inviteUsers that is fine, the test will fail and the user will update the name there. What we do not want is they renaming hooksService.inviteUsers, and for that we have all the warnings in the comments and if they do, the tests will fail. I guess we can have an specific assertion to make sure inviteUsers exist in the hooksService? That would be a "extra" layer of security in case however is making the changes ignores the comments in the hooks service file? What am I missing?

ivov · 2024-04-03T07:48:06Z

Let's sync later today! I'd love it if we can protect method names without adding a wrapper.

RicardoE105 · 2024-04-04T09:43:10Z

@netroy, any thoughts here?

packages/cli/src/services/hooks.service.ts

RicardoE105 · 2024-06-06T20:27:26Z

Fixes: https://linear.app/n8n/issue/ADO-2264/add-hooks-service-to-n8n

tomi

Nice work. This makes things a lot better 👏

packages/cli/src/services/hooks.service.ts

github-actions · 2024-06-07T06:35:05Z

✅ All Cypress E2E specs passed

tomi · 2024-06-07T10:13:59Z

packages/cli/src/services/hooks.service.ts

+		private settingsRepository: SettingsRepository,
+		private workflowRepository: WorkflowRepository,
+		private credentialsRepository: CredentialsRepository,
+	) {}


Btw should we move the dbCollections to this service as well? At the moment they are available in this.dbCollections in the hooks. I would still like us to have quite flexible way to query data without needing to add a separate method for every single query

any changes to the external hooks interface should be considered a breaking change though, since we do have self-hosting customers who use external hooks.
we can definitely add the repositories here, but we should leave dbCollections as they are for now.

@tomi, the main idea is to force us to use only methods we have in the hooks service. If the repositories are also available here, we might have a case where if we change the model for that repo, we will break the hooks and not know. However, if we only use the methods we have in the hooks, we will notice here that we broke them and adjusted the method to use the new format/change. Thoughts?

It's always going to be balance between flexibility and runtime safety. I think it makes sense that certain things (like operations that change things in the DB or touch external systems like send email) have their own method to do that. But we can't think of every single use case beforehand, so thinking that we would do everything only based on the HooksService seems unrealistic. IMHO a good balance could be to have the methods here that we know we need but also expose certain repositories so we also have the flexibility. We can then put everything we know we need here, and rely on the repositories to implement the functionality for older versions that don't yet have the new methods in the HooksService. So I'd expose the repositories here as well. Then it's a mutual agreement that we don't use those unless absolutely necessary and rely on tests (in the hooks side) to make sure they work for different n8n versions

as long as we don't remove dbCollections from ExternalHooks, I agree with @tomi.
maybe we can mark it as @deprecated and remove it in v2.0.

But we can't think of every single use case beforehand, so thinking that we would do everything only based on the HooksService seems unrealistic. IMHO, a good balance could be to have the methods here that we know we need but also expose certain repositories so we also have the flexibility

I agree that specifying everything to be in the hooks is unrealistic, if we needed something, we would add it to the hooks service in n8n, wait for it to be release it, and then use it on the BE hooks via the hooks. So, we add functionality as we discover we need it.

We can then put everything we know we need here and rely on the repositories to implement the functionality for older versions that don't yet have the new methods in the HooksService.

For this we still have the dbCollections we inject when loading the hooks. When people upgrade to a n8n version which include the hooks service then we use that instead. Once at instances have the hooks service we can stop using the dbCollections altogether and as I mentioned before start adding functionality to the hooks as we need then.

Conclusion:

In any case, we need some way to keep supporting access to these repositories because we have some self-hosting users relaying on this. So will:

Add the dbCollections here.

Mark the dbCollections in we pass as parameter in the external hooks as @deprecated

Co-authored-by: Tomi Turtiainen <[email protected]>

tomi

LGTM 🚀

github-actions · 2024-06-10T13:38:35Z

✅ All Cypress E2E specs passed

packages/cli/test/unit/services/hooks.service.test.ts

netroy

Pushed some changes. Please let me know if you prefer to make any of these into a separate PR 🙏🏽

github-actions · 2024-06-10T14:34:27Z

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

packages/cli/src/services/hooks.service.ts

github-actions · 2024-06-10T15:25:42Z

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

github-actions · 2024-06-10T17:29:02Z

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

github-actions · 2024-06-10T21:01:17Z

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

…not-getting-invited

github-actions · 2024-06-10T22:44:47Z

✅ All Cypress E2E specs passed

* master: ci: Fix custom docker builds (no-changelog) (#9702) test: Fix e2e for projects missing instance owner (no-changelog) (#9703) ci: Refactor e2e tests to be less flaky (no-changelog) (#9695) feat(editor): Add move resources option to workflows and credentials on (#9654) fix: Introduce `HooksService` (#8962) fix(editor): Improve large data warning in input/output panel (#9671) ci(editor): Enforce type-safety in @n8n/chat builds as well (no-changelog) (#9685) fix(editor): Un-skip workflow save test (no-changelog) (#9698) refactor(core): Remove more dead code from event bus (no-changelog) (#9697) ci: Remove unused WaitTracker mocking (no-changelog) (#9694) feat: Update NPS Value Survey (#9638) refactor(core): Remove event bus channel (no-changelog) (#9663) refactor(core): Remove event bus helpers (no-changelog) (#9690) refactor(core): Merge event bus controllers and remove dead code (no-changelog) (#9688) ci: Fix e2e tests (no-changelog) (#9689) refactor(core): Use `@Licensed()` in event bus controller (no-changelog) (#9687) fix(editor): Node background for executing nodes in dark mode (#9682) fix(editor): Prevent saving already saved workflows (#9670) fix(editor): Fix node connection showing incorrect item count during … (#9684) refactor: Set up Cypress as pnpm workspace (no-changelog) (#6049)

* master: fix(editor): Add back credential type icon (no-changelog) (#9704) feat(editor): Add canvas edge toolbar hover show/hide support (no-changelog) (#9699) ci: Fix custom docker builds (no-changelog) (#9702) test: Fix e2e for projects missing instance owner (no-changelog) (#9703) ci: Refactor e2e tests to be less flaky (no-changelog) (#9695) feat(editor): Add move resources option to workflows and credentials on (#9654) fix: Introduce `HooksService` (#8962) fix(editor): Improve large data warning in input/output panel (#9671) ci(editor): Enforce type-safety in @n8n/chat builds as well (no-changelog) (#9685) fix(editor): Un-skip workflow save test (no-changelog) (#9698) refactor(core): Remove more dead code from event bus (no-changelog) (#9697) ci: Remove unused WaitTracker mocking (no-changelog) (#9694) feat: Update NPS Value Survey (#9638) refactor(core): Remove event bus channel (no-changelog) (#9663) refactor(core): Remove event bus helpers (no-changelog) (#9690)

netroy · 2024-06-12T11:24:34Z

Just noticed that the PR title here shouldn't have been fix.

RicardoE105 · 2024-06-12T12:48:12Z

@netroy Thanks for fixing! Not sure why I started it with that name 😅

janober · 2024-06-12T14:06:45Z

Got released with [email protected]

fix: Introduce HooksService

7c4ff27

RicardoE105 requested a review from mutdmour March 24, 2024 16:49

fix description

656002d

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Mar 24, 2024

mutdmour reviewed Mar 25, 2024

View reviewed changes

packages/cli/src/services/hooks.service.ts Show resolved Hide resolved

RicardoE105 added 2 commits March 27, 2024 18:39

add test to make sure downstream service is called.

b64f340

improve function documentation

d0d8d44

RicardoE105 requested a review from mutdmour March 27, 2024 22:40

mutdmour approved these changes Apr 2, 2024

View reviewed changes

mutdmour previously approved these changes Apr 2, 2024

View reviewed changes

RicardoE105 assigned netroy Apr 15, 2024

netroy requested changes Apr 17, 2024

View reviewed changes

packages/cli/src/services/hooks.service.ts Outdated Show resolved Hide resolved

RicardoE105 added 3 commits June 5, 2024 19:16

Sync master

dddbe20

Add more methods used in the hooks to the service

d793d48

Add DB access in BE hooks to hooks service

d7b201c

RicardoE105 dismissed mutdmour’s stale review via d7b201c June 6, 2024 20:23

RicardoE105 requested a review from tomi June 6, 2024 20:24

tomi previously approved these changes Jun 7, 2024

View reviewed changes

packages/cli/src/services/hooks.service.ts Outdated Show resolved Hide resolved

tomi reviewed Jun 7, 2024

View reviewed changes

Make all properties read only

87e2e62

Co-authored-by: Tomi Turtiainen <[email protected]>

RicardoE105 dismissed tomi’s stale review via 87e2e62 June 9, 2024 15:08

RicardoE105 requested a review from tomi June 10, 2024 13:25

tomi previously approved these changes Jun 10, 2024

View reviewed changes

RicardoE105 requested a review from netroy June 10, 2024 13:56

netroy reviewed Jun 10, 2024

View reviewed changes

packages/cli/test/unit/services/hooks.service.test.ts Outdated Show resolved Hide resolved

fix typing errors

1e3db1e

netroy dismissed tomi’s stale review via 1e3db1e June 10, 2024 14:26

netroy previously approved these changes Jun 10, 2024

View reviewed changes

RicardoE105 commented Jun 10, 2024

View reviewed changes

packages/cli/src/services/hooks.service.ts Outdated Show resolved Hide resolved

revert the accidental change

51c11f7

netroy dismissed their stale review via 51c11f7 June 10, 2024 14:56

Use the Auth repo when looking for user

a4bbdba

netroy approved these changes Jun 10, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into ado-1912-bug-users-…

1d8084d

…not-getting-invited

netroy approved these changes Jun 10, 2024

View reviewed changes

RicardoE105 merged commit dda7901 into master Jun 11, 2024
33 checks passed

RicardoE105 deleted the ado-1912-bug-users-not-getting-invited branch June 11, 2024 11:28

netroy changed the title ~~fix: Introduce HooksService~~ feat: Introduce HooksService Jun 12, 2024

github-actions bot mentioned this pull request Jun 12, 2024

🚀 Release 1.46.0 #9716

Merged

janober added the Released label Jun 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce `HooksService` #8962

feat: Introduce `HooksService` #8962

RicardoE105 commented Mar 24, 2024 •

edited

Loading

mutdmour Apr 2, 2024

cypress bot commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024

ivov commented Apr 2, 2024

RicardoE105 commented Apr 2, 2024 •

edited

Loading

ivov commented Apr 3, 2024

RicardoE105 commented Apr 4, 2024

RicardoE105 commented Jun 6, 2024

tomi left a comment

github-actions bot commented Jun 7, 2024

tomi Jun 7, 2024 •

edited

Loading

netroy Jun 7, 2024

RicardoE105 Jun 10, 2024

tomi Jun 10, 2024

netroy Jun 10, 2024

RicardoE105 Jun 10, 2024

tomi left a comment

github-actions bot commented Jun 10, 2024

netroy left a comment

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

netroy commented Jun 12, 2024

RicardoE105 commented Jun 12, 2024

janober commented Jun 12, 2024

feat: Introduce HooksService #8962

feat: Introduce HooksService #8962

Conversation

RicardoE105 commented Mar 24, 2024 • edited Loading

Summary

Related tickets and issues

Review / Merge checklist

mutdmour Apr 2, 2024

Choose a reason for hiding this comment

cypress bot commented Apr 2, 2024 • edited Loading

1 flaky test on run #5406 ↗︎

Review all test suite changes for PR #8962 ↗︎

github-actions bot commented Apr 2, 2024

ivov commented Apr 2, 2024

RicardoE105 commented Apr 2, 2024 • edited Loading

ivov commented Apr 3, 2024

RicardoE105 commented Apr 4, 2024

RicardoE105 commented Jun 6, 2024

tomi left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 7, 2024

tomi Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

netroy Jun 7, 2024

Choose a reason for hiding this comment

RicardoE105 Jun 10, 2024

Choose a reason for hiding this comment

tomi Jun 10, 2024

Choose a reason for hiding this comment

netroy Jun 10, 2024

Choose a reason for hiding this comment

RicardoE105 Jun 10, 2024

Choose a reason for hiding this comment

tomi left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2024

netroy left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024

netroy commented Jun 12, 2024

RicardoE105 commented Jun 12, 2024

janober commented Jun 12, 2024

feat: Introduce `HooksService` #8962

feat: Introduce `HooksService` #8962

RicardoE105 commented Mar 24, 2024 •

edited

Loading

cypress bot commented Apr 2, 2024 •

edited

Loading

RicardoE105 commented Apr 2, 2024 •

edited

Loading

tomi Jun 7, 2024 •

edited

Loading