-
Notifications
You must be signed in to change notification settings - Fork 28
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
GetById() requiring context #47
Comments
From some quick research I don't see a way to access the context from the GetById() method nor from the NodeGraphType object. I could draft up a PR to pass the context to the GetById method. However I'm not sure a PR is possible without it being a "breaking change" as it will be modifying the parameters the GetById method. We could make the context "optional" to prevent breaking changes, but that's somewhat weird as the context will never truly be null. Thoughts? |
It would be a breaking change either way, because you won't be able to override a method with an optional parameter unless the overriding member also has the optional parameter. A new member could be added that calls the old deprecated member, but I think it is fine to have breaking changes. The project hasn't been touched in almost 3 years. If anyone is still using this project, they are still using GraphQL.NET 2.4.0, and there are many breaking changes between it and GraphQL.NET 4.x. I have not looked in depth, but I think it makes sense for GetById to have the context. It would be necessary to have the context if the implementation needs to access |
Thanks for taking a look at this. I did end up upgrading to GraphQL.Net 3.x a while back and therefore had to move away from using this library, so this issue isn't a problem for me any more. Previously I was able to work around the issue by using an approach similar to the data loader context accessor implementation in GraphQL.Net 2.x. - something like the DataLoaderDocumentListener In my new code I also changed to return an I'm happy to either close or leave open this issue depending on what you guys want to do. Cheers |
Thanks for the update @PaulKlein ! Just FYI, although all of the data loader classes have been moved into the other package for 4.0, the Out of curiosity might you describe what parts of this library were useful to you, and what might you still be using if the project had been maintained properly? Thanks! |
Thanks @Shane32 , good to know about that IDataLoaderResult. We're in the process of migrating to 4.x at the moment actually but it won't be a problem for us to just reference that separate data loader package as we do make heavy use of the batch loaders. I think one very useful part of this library is the helpers around dealing with relay connections. Many of our connections are fairly static lists, so using the offset-based cursors was working well using the helpers. We were not using mutations when we were originally using this library, but are now starting to look at them, so that is another thing that would be useful. I didn't actually realise relay mutations imposed extra constraints on mutations so another benefit of the project is to consolidate all the requirements to be 'relay compatible' into one nice place. One thing we're trying to figure out at the moment is using cursors based on record ids instead of offset-based cursors. Some of our connections are e.g. things like logs, where they want to get a consistent paged stream of events without missing/duplicate records across pages. This is where offset-based paging struggles when new records are being added all the time. To make this more complicated we allow the user to specify custom sorting and filtering on the connection, so we can't just use We're using Entity Framework under the hood so I think we might be stuck here until the EF team implement something like dotnet/efcore#14104. I don't know if there are any generic helpers/classes that could help with id-based cursors that would be a good fit for this project - it's possibly all too tied in with the underlying code to be generalised easily. Finally one other thing that we really struggled with is to combine batch loaders with connections. Again this is probably too specific and tied in with the other packages, but imagine trying to do something like e.g. query {
users(first: 100) {
items {
name
friends(first: 50) {
items {
name
}
}
} We wanted to avoid loading the friends for each user separately. It ended up being quite a lot of code to batch-load the friend queries in the connection resolvers to fetch this data efficiently. I'm not sure if there are any generic helpers that could live in this project around these pain points or if it's too intertwined with the underlying resolver implementations but just FYI what kinds of things we're using and what problems we're running into Thanks again for your help and all the hard work you do on these projects! |
No problem and thanks for the feedback! Just thinking out loud about your nested-connection issue....: Perhaps if you configured Note that EF Core doesn't have ROW_NUMBER support included, but it can be added. Check out https://www.thinktecture.com/en/entity-framework-core/row_number-support-in-2-1/ I use linq2db where ROW_NUMBER is part of the supported feature set. Here's a sample of its use: linq2db/linq2db.EntityFrameworkCore#130 (comment) Linq2db can be used on top of Entity Framework if you desire. I think it also has helper functions included for ORing expressions together for use in a Where query. |
If you support multiple sort orders on the So I'd group the data loader key by the aspects that require different queries (such as first vs last, or sort order if applicable) and enumerate over the different sets, executing a different query for each. As you say, it's a bunch of code, but perhaps not so terribly bad. |
Hi @Shane32 , thanks a lot for that extra info and pointers - interesting stuff there for sure. I'll dig through it and see what we can come up with. I haven't even looked at Thanks again |
Is it possible to access the graphql context in GetById()?
In my case I have some user context in ResolveFieldContext<>.UserContext which I need in order to load by ID.
If it's not currently available, could it be possible to change IRelayNode to include this context somehow?
The text was updated successfully, but these errors were encountered: