Skip to content
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

feature: ConnectionType supports custom fields #112

Conversation

NeedleInAJayStack
Copy link
Member

These fields can be added to either the Connection or the Edge types

These fields can be added to either the Connection or the Edge types
@cshadek
Copy link
Contributor

cshadek commented Mar 14, 2023

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?

@ZirgVoice
Copy link

ZirgVoice commented Mar 14, 2023

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.

Copy link
Member

@d-exclaimation d-exclaimation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@d-exclaimation
Copy link
Member

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.

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.

Copy link
Member

@paulofaria paulofaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@cshadek
Copy link
Contributor

cshadek commented Mar 14, 2023

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.

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 have a few initial ideas on this:

  1. 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.

  2. Add something analogous to Graphiti's Context to pass to the connection type. I'm not entirely sure how this would work.

  3. 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.

  4. Do nothing

Thoughts?

@NeedleInAJayStack
Copy link
Member Author

@ZirgVoice

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.

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.

@ZirgVoice
Copy link

@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.

@cshadek
Copy link
Contributor

cshadek commented Mar 14, 2023

@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.

@NeedleInAJayStack
Copy link
Member Author

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!

@cshadek
Copy link
Contributor

cshadek commented Mar 14, 2023

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!

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.


Option 1:

  1. 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.

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?


Option 3:

  1. 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.

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)

Option 5 (New idea):

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?

@ZirgVoice
Copy link

The 5th option looks better and I think something like this would work for this

@d-exclaimation
Copy link
Member

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

@cshadek
Copy link
Contributor

cshadek commented Mar 14, 2023

@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.

@d-exclaimation
Copy link
Member

d-exclaimation commented Mar 14, 2023

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)

@cshadek
Copy link
Contributor

cshadek commented Mar 16, 2023

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?

@cshadek
Copy link
Contributor

cshadek commented Mar 18, 2023

@NeedleInAJayStack any thoughts?

@NeedleInAJayStack
Copy link
Member Author

@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.

@cshadek
Copy link
Contributor

cshadek commented Mar 30, 2023

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?

@NeedleInAJayStack
Copy link
Member Author

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants