refactor(core): Auto-register controllers at startup (no-changelog) #9781
Conversation
also stop using type-unsafe `Reflect.getMetadata`
n8n-assistant
bot
added
core
| route = {} as RouteMetadata; | ||
| metadata.routes.set(handlerName, route); |
There was a problem hiding this comment.
Doesn't this break the type safety, if we can't rely anymore that the objects in the route metadata conform to that type? Should we instead validate in the activateController that the metadata has been defined correctly and throw if not?
There was a problem hiding this comment.
Doesn't this break the type safety
it kinda does. but If someone creates a new controller class that does not use the decorators correctly, the application will already fail to start, so I'm not that worried about it. But I can also add some additional validation in activateController, to add more explicit errors.
There was a problem hiding this comment.
adding the validation seems pretty straight forward, but adding tests for it require resetting the registry, which is making the code a bit more complicated. I'd like to try doing this in another PR.
|
✅ No visual regressions found. |
|
✅ All Cypress E2E specs passed |
2 flaky tests on run #5572 ↗︎
Details:
|
|||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Undo/Redo > should undo/redo adding connected nodes |
Test Replay
Screenshots
Video
|
|
12-canvas.cy.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| Canvas Node Manipulation and Navigation > should preserve connections after rename & node-view switch |
Test Replay
Screenshots
Video
|
|
Review all test suite changes for PR #9781 ↗︎
|
|
||
| // ---------------------------------------- | ||
| // SAML | ||
| // ---------------------------------------- | ||
| await this.registerAdditionalControllers(); | ||
|
|
||
| // initialize SamlService if it is licensed, even if not enabled, to | ||
| // set up the initial environment | ||
| try { | ||
| await Container.get(SamlService).init(); | ||
| } catch (error) { | ||
| this.logger.warn(`SAML initialization failed: ${error.message}`); | ||
| } | ||
|
|
||
| // ---------------------------------------- | ||
| // Source Control | ||
| // ---------------------------------------- | ||
| try { | ||
| await Container.get(SourceControlService).init(); | ||
| } catch (error) { | ||
| this.logger.warn(`Source Control initialization failed: ${error.message}`); | ||
| } | ||
| // register all known controllers | ||
| Container.get(ControllerRegistry).activate(app); |
There was a problem hiding this comment.
The order in which these are registered has been inversed here. Is that intentional or does it matter?
There was a problem hiding this comment.
here the controllers used to be registered before the init on the related service had been called, which likely made some of those endpoint 5xx for a short while after startup. This change is meant to ensure that all relevant services have been initialized before we setup any of the routers.
|
✅ All Cypress E2E specs passed |
|
✅ All Cypress E2E specs passed |
|
Got released with |
1 similar comment
|
Got released with |
Summary
Extracted out of #9734
This PR:
RestControllerdecorator. All we now need to do is to ensure that the relevant controller classes are imported somewhere in the dependency tree.Reflect.getMetadataReview / Merge checklist