Closed NeedleInAJayStack closed 1 year ago
Awesome work @NeedleInAJayStack!
So if I'm understanding correctly, the way to have custom connections or edges is to extend Connection
and/or Edge
and specify that the Node is a certain type? Very elegant!
Will you update the Usage Guide with an example?
I use the custom connection function to retrieve paginated objects from the database. How can I add custom field totalCount
to Connection
structure in this implementation? I get the total count from the database and I need to pass it to the Connection
structure during initialization. At the moment I am using my own Connection
, PageInfo
and Edge
structures. The last change with adding public access to PageInfo
and Edge
doesn't work for me either because I can't initialize them, I still need to add public inits to them.
I use the custom connection function to retrieve paginated objects from the database. How can I add custom field
totalCount
toConnection
structure in this implementation? I get the total count from the database and I need to pass it to theConnection
structure during initialization. At the moment I am using my ownConnection
,PageInfo
andEdge
structures. The last change with adding public access toPageInfo
andEdge
doesn't work for me either because I can't initialize them, I still need to add public inits to them.
Regarding that, I don't think making the PageInfo
and Edge
struct have a public constructor wouldn't immediately solve the issue because you still can't really add custom properties to it or really extends it, unless I am missing something here.
Not 100% what is the best approach here. I am not sure what's the role of the built in Connection
type is. Whether it is suppose to be a convenient built in solution that doesn't support all scenarios or the all in one solution where it should be used in any scenarios big or small.
I use the custom connection function to retrieve paginated objects from the database. How can I add custom field
totalCount
toConnection
structure in this implementation? I get the total count from the database and I need to pass it to theConnection
structure during initialization. At the moment I am using my ownConnection
,PageInfo
andEdge
structures. The last change with adding public access toPageInfo
andEdge
doesn't work for me either because I can't initialize them, I still need to add public inits to them.Regarding that, I don't think making the
PageInfo
andEdge
struct have a public constructor wouldn't immediately solve the issue because you still can't really add custom properties to it or really extends it, unless I am missing something here.Not 100% what is the best approach here. I am not sure what's the role of the built in
Connection
type is. Whether it is suppose to be a convenient built in solution that doesn't support all scenarios or the all in one solution where it should be used in any scenarios big or small.
I have a few initial ideas on this:
Add a key value dictionary property to Connection
similar to Vapor's Storage. This would allow you to effectively pass data to the Connection
resolvers without having to create a custom Connection
constructor. A new public constructor could be created on Connection
that takes an optional dictionary argument.
Add something analogous to Graphiti's Context
to pass to the connection type. I'm not entirely sure how this would work.
Add an optional totalCount
field to Connection
since that is probably one of the most common use cases and then add a constructor that takes an optional async throws -> Int
closure that is used to return the count.
Do nothing
Thoughts?
@ZirgVoice
I use the custom connection function to retrieve paginated objects from the database. How can I add custom field
totalCount
toConnection
structure in this implementation? I get the total count from the database and I need to pass it to theConnection
structure during initialization. At the moment I am using my ownConnection
,PageInfo
andEdge
structures. The last change with adding public access toPageInfo
andEdge
doesn't work for me either because I can't initialize them, I still need to add public inits to them.
Not sure I understand the issue here. You can get the total count of edges by doing the same thing as in the added tests:
extension Connection {
func total(context _: NoContext, arguments _: NoArguments) throws -> Int {
return edges.count
}
}
Or if you want to read them from a database (which I assume is stored in the context), something like this is totally possible:
extension Connection where Node == MyTableType {
func total(context: Context, arguments _: NoArguments) throws -> Int {
return context.db.run("SELECT COUNT(*) FROM my_table")
}
}
Neither of these require a custom initializer.
If you are using your own custom Connection
, PageInfo
and Edge
structures, then you're in complete control and you must do everything yourself.
@cshadek @d-exclaimation
I am not sure what's the role of the built in Connection type is. Whether it is suppose to be a convenient built in solution that doesn't support all scenarios or the all in one solution where it should be used in any scenarios big or small.
I see the role of our Connection
implementation as a convenience, simplifying the common use cases, but not necessarily attacking the complex ones. In fact, that is really the intent of this package as a whole. At the end of the day, users can always drop down into https://github.com/GraphQLSwift/GraphQL for complete customization.
@NeedleInAJayStack This seems like a solution. But how do I pass arguments with filters that are in the resolver? I need to calculate the total number taking into account the filters that are used for pagination.
@NeedleInAJayStack This seems like a solution. But how do I pass arguments with filters that are in the resolver? I need to calculate the total number taking into account the filters that are used for pagination.
I agree.
For example, if you had a connection for friends of a user (not necessarily the current user), how would you accomplish getting the total count? You might not want to query all the friends (it could be in the thousands), so edges.count
might not work.
And I'm not sure how you'd pass the originating user's id
to get the friends of the user to the connection. This is the reasoning behind having a way to pass more data to the connection. This is a simplified example, but you could have much more complex cases where you pass many arguments, and it would be nice to not have to reimplement all the connection cursor and slicing logic.
Ok cool, still catching up. To make sure I understand - the issue is that within the Connection
extensions that define our custom field resolvers, we've lost the information as to how we got there (for example the pagination args like first
, before
, etc as well as the upstream object like user:1
). Makes sense.
Adding those in is definitely a tougher problem and maybe we hold off on these changes until we figure it out. I think cshadek gave some interesting approaches. If one of you would like to take a crack at it, I'd be happy to review!
Ok cool, still catching up. To make sure I understand - the issue is that within the
Connection
extensions that define our custom field resolvers, we've lost the information as to how we got there (for example the pagination args likefirst
,before
, etc as well as the upstream object likeuser:1
). Makes sense.Adding those in is definitely a tougher problem and maybe we hold off on these changes until we figure it out. I think cshadek gave some interesting approaches. If one of you would like to take a crack at it, I'd be happy to review!
I can take a crack at it. Do you guys have a preference on which of the 3 options (not including 4) is the best way to go? My instinct says to go with (1) or (3) unless there's another better alternative.
- Add a key value dictionary property to
Connection
similar to Vapor's Storage. This would allow you to effectively pass data to theConnection
resolvers without having to create a customConnection
constructor. A new public constructor could be created onConnection
that takes an optional dictionary argument.
If we did use (1) what would be the best way to implement the dictionary? [String: Task<Any, Error>]
? That would allow you to pass an async task into the connection for specific string keys.
So for (1) you could have something like:
let connectionParams = [String: Task<Any, Error>]()
connectionParams["totalCount"] = Task {
try await UserModel.query(...).filter(...).count()
}
let users = ....().connection(from: arguments, params: connectionParams)
extension Connection where Node == User {
func totalCount(context: Context, arguments: NoArguments) async throws -> Int {
return self.params["totalCount"] as? Int
}
}
(I'm not sure about the naming, connectionParams
vs connectionContext
, etc)
I also don't like using the strings instead of something more strongly typed. Is there a better way such as keyPaths
, enums
, etc?
- Add an optional
totalCount
field toConnection
since that is probably one of the most common use cases and then add a constructor that takes an optionalasync throws -> Int
closure that is used to return the count.
Option 3 could also be a good option for now. It takes care of a big use case by adding an optional totalCount: Task<Int, Error>?
property to Connection.
let totalCount = Task {
try await UserModel.query(...).filter(...).count()
}
let users = ....().connection(from: arguments, totalCount: totalCount)
A 5th option would be to somehow let Connection
be aware of the upstream object and arguments. This may actually be the best option, but it would require changing the Connection
type to have another associated type Parent
. So then the Connection resolvers would be aware of Parent and the arguments and could resolve accordingly. Then you could change the constructor for Connection
to include a parent: Parent
argument and you could use then still use the extensions similar to before:
extension Connection where Node == User, Parent == SomeUpstreamObject {
func totalCount(context: Context, arguments: NoArguments) async throws -> Int {
// Use self.parent and maybe self.arguments to fine tune this count query
}
}
A potential problem with this approach is that one parent object could have multiple fields that resolve to the same connection type, so you'd need a way to differentiate between the different fields.
@NeedleInAJayStack @d-exclaimation @ZirgVoice
What are your thoughts? Also how does a future desire to have connections of Unions and Interfaces affect this decision?
The 5th option looks better and I think something like this would work for this
I don't have a preference here. I think I would normally just use a custom connection and edge types if I need more that what it asked
@d-exclaimation What about option 3 then? Or what if we made more of the inner connection logic accessible (slicing, etc). That way it would be easier to create custom connections. Maybe the standard should be to create custom connection and edge types, but at the same time, couldn't we make that process more efficient?
@ZirgVoice It seems to me like option 5 could be very complex and requires a lot of changes.
what if we made more of the inner connection logic accessible (slicing, etc). That way it would be easier to create custom connections?
I like this idea, it make sense to allow user to have control over that. I haven't look into it so I don't know how difficult that may be.
What about option 3 then?
I don't mind having that but I assumed the original design based on this, not sure if I want to go beyond that specifications (technically there's no restriction on additional fields)
I think the issue stems from the fact that we can't really get to the Edge
and Connection
initializers and we can't add properties with storage in extensions. We still want to use the internal slicing and count logic though.
What if we could do something like the following:
let users = [User]()
// Custom Connection Case
users.connection(from: arguments, connection: FriendConnection(..... custom args), edge:
{ node -> FriendEdge in
return FriendEdge(..... custom args)
} )
// Default Connection Case
users.connection(from: arguments)
Then we can use the edge
closure to create the edges when handling pagination. Ideally, Connectable would have protocol extensions/ default implementation for all the pagination logic.
FriendConnection
is an example, but it could be any custom Connection type that implements a new protocol Connectable.
FriendEdge
is an example, but it could be any custom Edge type that implements Edgeable.
I think this implementation would solve the issues that @ZirgVoice, @d-exclaimation and @NeedleInAJayStack mentioned.
A user could then create the custom types as follows:
struct FriendEdge: Edgeable where Node == User {
var upstreamUserId: UUID?
var node: Node
var cursor: String
init(upstreamUserId: UUID?) {
self.upstreamUserId = upstreamUserId
}
// Custom fields on the friend edge that use upstreamUserId, etc
func friendsSince(context: Context, arguments: NoArguments) async throws -> Date? {
// Use the custom args provided in `init` to return this
}
}
struct FriendConnection: Connectable where Edge == FriendEdge {
// Something similar to FriendEdge with custom fields and a custom init
func totalCount(context: Context, arguments: NoArguments) async throws -> Int {
// Use the custom args provided in `init` to return this
}
}
I'm not sure how this would work with ConnectionType
and the schema creation logic, but this seems like it would be intuitive from the developer's perspective. It likely requires a bunch of breaking changes though.
This is just an initial idea and I haven't actually tested any code.
Thoughts?
@NeedleInAJayStack any thoughts?
@cshadek, Sorry about the slow response, it's been a busy weekend 😀.
WRT the exposing the slicing logic, one concern I have is that the slicing logic is based on having an array in memory prior to pagination, whereas that may not be true in some cases (loading from a database, for example). So more general closure-based tools may be better.
The solution you outline above could work well too, but it is certainly a more complex API for the basic case.
Honestly, the more I think about it, Connection stuff may live best in a separate "relay" extension package. In that case I think we would have more freedom to be creative and iterative with the implementation. If there's big difficulties in doing this as a separate package, we could prioritize extensibility in Graphiti.
Honestly, the more I think about it, Connection stuff may live best in a separate "relay" extension package. In that case I think we would have more freedom to be creative and iterative with the implementation. If there's big difficulties in doing this as a separate package, we could prioritize extensibility in Graphiti.
@NeedleInAJayStack sorry it's been a busy few weeks for me haha. I agree with this approach. It probably makes sense to keep Graphiti as close to the GraphQL spec as possible.
WRT the exposing the slicing logic, one concern I have is that the slicing logic is based on having an array in memory prior to pagination, whereas that may not be true in some cases (loading from a database, for example). So more general closure-based tools may be better.
I agree with this approach as well.
Should we just merge this as is and revisit in the future?
@cshadek, no worries. Since this MR doesn't give us quite what we want, I think I'm going to close it. Thanks everyone for the great discussion here!
These fields can be added to either the Connection or the Edge types