-
Notifications
You must be signed in to change notification settings - Fork 110
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
chore(RFC): open core module system v1 #1063
base: main
Are you sure you want to change the base?
Conversation
A few goals I'd like to see covered:
Additional comments: I'll look more into the RFC and give more specific feedback later/tomorrow. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Closed accidentally from bot |
|
||
// Rewrite errors in the response | ||
if len(res.Response.Errors) > 0 { | ||
for _, err := range res.Response.Errors { |
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.
1.1 We are interested in changing the message, as I understand we will now have this access.
1.2 It is not clear how we will work with untyped extensions, through fastjson? It's important for us not to change custom extensions that we didn't expect.
2. We would also like to understand which subgraph the error belongs to, so that we can change it with this logic in mind. Let's say the status code returned a subgraph
3. We want to do ctx.Set, so that we can get the modified errors in other methods (for example, for logging).
Request *core.GraphQLRequest | ||
// The parsed, normaliazed and planned operation with all the information like name, variables, type, document representation, | ||
// client name version, uploaded files, plan, normalization, persisted operation etc. | ||
Operation *core.Operation |
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.
Somewhere in here I wanted to be able to get
Id string
Name string
Url *url.URL
UrlString string
}
So that this information can be used when changing the error.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
As of today, customers can extend the router with custom modules. These modules can be used to change the behavior of the router, add custom logic, or integrate with other systems. The current module system has several limitations that we want to address with this RFC: | ||
|
||
- The current module system is not native to GraphQL. It is based on HTTP middleware and does not provide a GraphQL-specific API. | ||
- The current module system is inconsistent and hard to use. It does not provide a clear API for developers to intercept and modify GraphQL requests and responses. |
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.
Hi,
Great stuff here. One thing that I didn't see is the possibility to hook into the EDFS lifecycle.
Would it be possible with the Custom Module System to intercept subscription requests to subgraphs and inject a custom transport, similar to the Kafka or NATS that you already provide.
I would be particularly interested in adding Redis PubSub support to EDFS.
Thx,
Goran
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.
Hi @ghorak-happening, that's a great idea. The goal of this RFC is to propose an architecture that is flexible enough to add those extensions later. The first version will focus on the fundamentals. I'm going to add your requirements to the list so as not to forget them.
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.
That would be great @StarpTech, thank you!
|
||
## Custom Authentication and Authorization | ||
|
||
Custom modules can be used to implement custom authentication and authorization logic in the router. The module can intercept incoming requests and validate the user's credentials, scopes, and permissions before forwarding the request to the subgraph. The router has built-in support for JWK. The parsed token information is available in the request `req.Request.Auth` field. |
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.
I would suggest supporting the scenario where you only want to customize authentication but still use the built in authorization via directives (such as requiresScopes). For example, if I sign my JWT using an algorithm that Cosmo doesn't support, I would like to build a custom auth plugin that validates the JWT and injects the claims/scopes into the context, so that Cosmo can authorize access using the directives in the schema.
Motivation and Context
Related work:
TODO