-
Notifications
You must be signed in to change notification settings - Fork 490
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
No way to get requested fields inside resolver #17
Comments
This is definitely a feature we should think about. Is there anything like that in the graphql-js implementation? I am a bit worried that this can cause people to write bad resolvers. One nice attribute of GraphQL are all those consistency guarantees it provides. |
I agree that this could cause people to write bad code resolvers, but it could also help write efficient resolvers. For example, one could use that information to prevent an unnecessary "join" in his/her db query if the user didn't ask for any of those fields. Also, it could give user the ability to log which fields are being requested most frequently and then perform optimizations on those fields (like pulling from redis-cache rather than db). The last variable passed to graphql resolvers in graphql-js has lots of info about the schema, fields, ect... see: https://github.com/graphql/graphql-js/blob/master/src/type/definition.js#L489 |
@nicksrandall Do you know any example code that uses this feature with graphql-js? |
@DenisNeustroev Facebook has a recommendation on how to solve the n+1 problem with a technique called DataLoader. I have taken a stab at implementing this technique in go here: https://github.com/nicksrandall/dataloader. @neelance I am not aware of any examples of people using the schema info in a resolver but I haven't done much research on the subject. |
Being able to see what fields were requested would make it super easy to select a specific list of columns when performing an SQL query in a resolver. That means, we don't have to get all fields ( |
I know |
I have a similar issue, need a way to either defer the resolution so I can produce a query, or some way to access the nested fields of the query to properly form the query instead of simply filtering results a very expensive query (or possibly unknown, in which case it's impossible). Anyone come up with a workaround? EDIT: I ended up hacking it in. I think ideally there could be an optional preparation pass to the set of resolvers, or some similar set of structs, letting you construct the queries necessary first. Some variant of that haha. |
This POC could solve graph-gophers#17
This POC could solve graph-gophers#17
This POC could solve graph-gophers#17
This POC could solve graph-gophers#17
I'm still not a fan of exposing all this data to the upper resolvers. They should not care about that. However, the initial example by @DenisNeustroev definitely shows a need. What about instead of integrating that |
@neelance what do you mean by "upper resolvers"? I think it is reasonable for a resolver to be aware of it's children and thus aware of the fields requested for itself and it's children. Also, I like the idea of baking in the ability to execute a batch of resolvers in one go. I've been doing a form of that with my dataloader implementation and it has been very helpful for the performance of my service. |
It's nearly impossible to optimize a query without that information. For example my current use-case is to fetch a series of rows from Postgres, then I have to construct a query for Elasticsearch based on those, however I have to know what metrics the user is asking for, otherwise it's simply impossible to create the query.
I'm new to GQL so maybe this is a weird approach, you could maybe do it by passing arguments but it would be super ugly to the point where I wouldn't want to use GQL for it anyway. If this is an anti-pattern definitely let me know haha, but it seems logical to me. The main thing I like about GQL is this expressiveness vs passing |
@tj How much performance difference does it make in your case to fetch some fields vs. all fields? If all fields come from the same table (no JOIN), then the difference might be so small that it would be premature optimization. |
In my case it would be hugely detrimental, I'd have to fetch every possible permutation of what they can ask for and then filter. They're pretty expensive aggregate queries on large amounts of data as well so it's not like a quick select from Postgres, more like going from 200ms queries to 5s+. For the SQL use-case I totally agree that selecting specific columns is usually not a huge deal, I almost always |
All right. For that situation you'll probably want to use a custom resolver. That feature will be added soon, I'm just currently busy with adding validations. |
Cool cool no worries! The context thing will keep me covered for now, just passing the fields to a template to generate the massive Elasticsearch query haha. |
Ahh hitting cases where custom resolver would be the only way to go now. Doing something like this to provide a
which can't be properly mapped unless you have access to the name or alias anyway. |
What about something like #169 ? |
Having an issue with this as well, I'm eating the performance hit fine in my case though. The bigger issue is not being able to get query arguments in resolvers nested all the way down. I'm looking at the solution here for selectedFields (DealTap#5) and hacking that in to include arguments as well |
Now that we have two alternative solutions for this issue I decided to give my two cents about it.
However, I argue that this is a good idea giving that Go doesn't have an idiomatic way to pass optional parameters unless dynamic method dispatch is in place. Think of what the user's code will look like? In case 1 the optional parameter is added only where needed with already populated and ready-to-use value. Nothing else get changed. In case 2 instead there will be one repetitive block with something like: selectedFields, err := selected.GetFieldsFromContext(ctx)
if err != nil {
// for some reason the context value is missing. We know that one can't
// send a query with zero selected fields, then we must assume all
// fields are selected... I guess.
selectedFields = somehowGetAllFieldsForThisQueryOrHardCodeThem()
}
// Here selectedFields is just what would've been anyways with an optional
// parameter. The unfortunate part is that there's really no reliable and safe way to make TL;DR: I think the solution proposed in #169 is the right way to resolve this issue and should go towards getting it ready to be merged and eventually merging it. |
I've found a way around this without having to change the lib itself by creating another lib just for this purpose: gbaptista/requested-fields I don't like the idea of having to parse the query a second time or need to manually set the hierarchy of resolvers, but it was a simple and quick solution that works. |
A use case that is completely missed here is the difference between doing Additionally, this is common enough to use this information in both the Javascript implementation as well as the Ruby implementation (I've done it myself). With other implementations, they simply provide the AST to the resolver from the point in which the graph is currently being walked. It basically came for free in the other implementations. The ruby implementation even goes as far as allowing plugins to be able to make changes to the AST by using a copy-and-modify technique to keep immutability of the AST. This allows them to implement custom directives as a form of AST manipulation. |
I need to decide what to do within resolver based on fields requested by client, how to get the fields list requested by user? why not created one struct with all parameter related to request, including parents fields and inject as parameter on resolver? any update about this? I'd like to use this lib but seems is not maturity enough for production use which reach real situations, is it? |
I started using this lib a few months ago, but now we're running into this limitation. It's unfortunate. I think I'm going to have to rip it out, which is going to be a bunch of work. |
We need this too. We are looking to do field usage reporting and metrics in addition to tracing. We'd prefer not to spread the monitoring logic throughout the application. There doesn't seem to be a feasible way to do this currently. |
I've had #373 open for a few months as a possible solution to this issue. Would someone mind taking a look? |
#373 seems a very non-intrusive implementation and not really risky. The problem to be solved is pretty urgent though. Every time I read that one of the great things about graphql is that you only get what you ask for, I die a little, as the problem is still there at api-implementation which doesn't help at all. |
Additional vote here for #373. Using the context argument makes it backwards compatible and opt-in, so I think this is the best approach of the ones I've seen submitted. |
Hi all. Where do we stand on this today? I love the way this library works, but I have use cases that necessitate an "eject" button to resolve selected fields from another GraphQL instance (different shard). (My case closely matches this comment.) From what I've found, these are all the proposals and their statuses:
Edit: I thought I should add: This missing feature is looking like a deal-breaker for my team. Without this flexibility, we hit issues creating a global GraphQL service with multiple, regional database shards. A dataloader helps, but even 2 serial round-trip requests to another region is quite expensive time-wise. |
It seems 169 is the furthest along in discussion. I think the idea shows the most promise as well. Perhaps we start with the selected fields today behind a method on the struct, so it may be lazily constructed. Something like this? func (o *MyObject) MyField(ctx context.Context, args struct { ID graphql.ID }, info graphql.ResolveParams)
type ResolveParams struct {}
func (p *ResolveParams) SelectedNode() graphql.Type
// alternatively:
type SelectedField struct {
Name string
Fields []SelectedField
}
func (p *ResolveParams) SelectedFields() []SelectedField |
Unfortunate. I was hoping I could project the selected fields into my mongodb queries. It sounds like discussion on this has stalled since October last year? |
Hi @brian-pickens, |
This commit makes minimal and additive changes to the frontend and Go backend to make the new /scip highlighting endpoint available through GraphQL. There are currently three possible scenarios how a file might get highlighted: - HTML blob view, default: Highlighted HTML is generated by syntect - HTML blob view, tree sitter: SCIP data is generated by tree sitter, HTML is generated on the client side - CodeMirror blob view, default: SCIP is generated by either syntect or tree sitter So far SCIP data was only generated for specific languages, determined by the server. With CodeMirror, SCIP also needs to be generated when the client requests it. My preferred solution would have been to let the server determine this based on the requested fields in the GraphQL request, but the library we are using [does not support that yet](graph-gophers/graphql-go#17). Making the highlighting requests in the field resolvers (i.e. `HTML()` and `LSIF()`) is also not possible without additional changes because the `Aborted()` field depends on the result of the request. This led me change the `formatOnly` field from a boolean to an enum, with which the client can now request: - `HTML_PLAINTEXT` - `HTML_HIGHLIGHT` - `JSON_SCIP` It's not ideal because the server can still return SCIP data depending on whether tree sitter is configured for the language (see second bullet point at the beginning) but I think this is still better than introducing a new field for highlighting. So, if CodeMirror is *disabled*, everything works as before. When CodeMirror is *enabled* it will explicitly request `JSON_SCIP` data and only include the `lsif` field in the GraphQL request. The server will route that request to the new `/scip` endpoint.
This commit makes minimal and additive changes to the frontend and Go backend to make the new /scip highlighting endpoint available through GraphQL. There are currently three possible scenarios how a file might get highlighted: - HTML blob view, default: Highlighted HTML is generated by syntect - HTML blob view, tree sitter: SCIP data is generated by tree sitter, HTML is generated on the client side - CodeMirror blob view, default: SCIP is generated by either syntect or tree sitter So far SCIP data was only generated for specific languages, determined by the server. With CodeMirror, SCIP also needs to be generated when the client requests it. My preferred solution would have been to let the server determine this based on the requested fields in the GraphQL request, but the library we are using [does not support that yet](graph-gophers/graphql-go#17). Making the highlighting requests in the field resolvers (i.e. `HTML()` and `LSIF()`) is also not possible without additional changes because the `Aborted()` field depends on the result of the request. This led me change the `formatOnly` field from a boolean to an enum, with which the client can now request: - `HTML_PLAINTEXT` - `HTML_HIGHLIGHT` - `JSON_SCIP` It's not ideal because the server can still return SCIP data depending on whether tree sitter is configured for the language (see second bullet point at the beginning) but I think this is still better than introducing a new field for highlighting. So, if CodeMirror is *disabled*, everything works as before. When CodeMirror is *enabled* it will explicitly request `JSON_SCIP` data and only include the `lsif` field in the GraphQL request. The server will route that request to the new `/scip` endpoint.
Is there any progress on this? My use case is that I need to implement directives.
where Some fields of this type has directives attached for access control. For example
Then in the go code, I want to set But because We need something similar to |
@pavelnikolov Do you have a draft PR? |
@CreatCodeBuild you might be interested in PR #543 |
Hi @pavelnikolov! You mentioned above that you have a working branch with an in-progress implementation of this feature. Any chance there's been progress on an API? I'd be happy to contribute to this feature in any way that would be useful. Honestly, even just making the branch public would be great. Even if it means I have to integrate a breaking change in the future, I'd rather have something that's close-ish to what gets released. In any case, it'd likely be a smaller breaking change than the fork I'll likely make otherwise. |
@pavelnikolov I've been perusing the current state of the various issues, etc, and I was wondering if you'd be willing to share the branch you have? I already run a fork (which I keep up-to-date with the main project), so I am interested in experimenting with your changes. I know this is a broadly requested feature with endless pestering, but I haven't seen any commentary in 2024 about this so I thought I'd shoot my shot ❤️ |
Problem:
Inefficient microservice/database queries.
If I need to return 100 orders with their clients, I have to make 101 requests to microservices (1 for batch of orders, 100 for their clients (one per each order)).
Scheme (simplified):
Query:
Resolvers:
Instead, I'd like to know (in the order resolver) that the field with clients has been requested.
So I could make one batch request at the order resolver level.
Something like this:
So I get 2 request.
The text was updated successfully, but these errors were encountered: