-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
Add workspacePreQueryHook module #3879
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Weiko
force-pushed
the
c--add-workspace-pre-query-hook-module
branch
3 times, most recently
from
February 13, 2024 13:54
cc04a3e
to
0b6e700
Compare
magrinj
approved these changes
Feb 13, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ! Some minor comments
packages/twenty-server/src/graphql-config/graphql-config.service.ts
Outdated
Show resolved
Hide resolved
import { WorkspaceMemberService } from 'src/workspace/messaging/repositories/workspace-member/workspace-member.service'; | ||
import { WorkspaceDataSourceModule } from 'src/workspace/workspace-datasource/workspace-datasource.module'; | ||
|
||
// TODO: Move outside of the messaging module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
...workspace/workspace-query-runner/workspace-pre-query-hook/workspace-pre-query-hook.config.ts
Show resolved
Hide resolved
packages/twenty-server/src/workspace/workspace-query-runner/workspace-query-runner.service.ts
Show resolved
Hide resolved
Weiko
force-pushed
the
c--add-workspace-pre-query-hook-module
branch
from
February 13, 2024 16:10
27cd80c
to
66740fa
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Due to the way we dynamically generate resolvers and their logic, we can't simply introduce logic into dedicated services called by standard object resolvers, each generated resolver will directly call pg_graphql (the DB) and return its result.
We want to introduce "query-hooks" logic that will allow us to attach some logic before and after the call to the DB.
This PR adds pre-query-hooks that are called before, more specifically we want to introduce validation logic to the findMany endpoint of the message standard object.
Implementation
A WorkspacePreQueryHookModule has been added to WorkspaceQueryRunner module, we use it to call WorkspacePreQueryHookService which will use the workspacePreQueryHooks config (that we want to replace by decorators in the future) to map each object and each method (findOne, findMany, etc) to a set of "hooks".
Added MessageFindManyPreQueryHook, it verifies when findManyMessages is called if we can fetch the different messages of the thread (using threadId filter). If the associated channel does not have share_everything visibility or if the requesting workspace member does not have the message attached to one of its channels, it will throw a 403
Due to the fact that we want to apply validations, we need to have access to the userId of the request context inside those hooks. Because of that, I'm updating the code to pass down the userId from the graphql config. In practice, we actually might want to send the workspaceMember instead but it's not that easy so instead I'm querying it in the pre-hook when needed.
Moving "repositories" kind service to a dedicated repositories folder inside messaging. This will enforce us to create those services with a "repository" pattern in mind and avoid any business logic and will allow us to easily replace them later if we decide to use typeorm repositories (or other ORM).
Note
How to add a query hook
For now, only pre-query-hook are implemented.
Test