strawberry-graphql / strawberry-sqlalchemy

A SQLAlchemy Integration for strawberry-graphql
MIT License
91 stars 26 forks source link

What is the rational for wrapping relationships into a Connection? #7

Closed CDTiernan closed 2 years ago

CDTiernan commented 2 years ago

I'll preface this with saying I am not an expert at GraphQL, so might be missing an obvious point.

Through my testing, it seems like wrapping of relationships adds 2 levels of unnecessary hierarchy in the schema. The code references "to support future pagination" but I am not privy to how this wrapping will make pagination possible and why it is not without it.

As an example, I will steal the query from the README:

With wrapping

query {
    departments {
        id
        name
        employees {
            edge {        # First added level of hierarchy
                node {    # Second
                      # ...
                    }
                }
            }
        }
    }
}

Without wrapping

query {
    departments {
        id
        name
        employees {
            # ...
        }
    }
}

My Idea

I spent a few minutes with the code and was able to remove wrapping by adding an attribute to the StrawberrySQLAlchemyMapper class:

    def __init__(
        self,
        paginate_relationships: Optional[bool] = True,
        model_to_type_name: Optional[Callable[[Type[BaseModelType]], str]] = None,
        model_to_interface_name: Optional[Callable[[Type[BaseModelType]], str]] = None,
        extra_sqlalchemy_type_to_strawberry_type_map: Optional[
            Mapping[Type[TypeEngine], Type[Any]]
        ] = None,
    ) -> None:
        self.paginate_relationships = paginate_relationships
        # ...

Then adding a few conditionals:

    def _convert_relationship_to_strawberry_type(
        self, relationship: RelationshipProperty
    ) -> Union[Type[Any], ForwardRef]:
        # ...
        if relationship.uselist:
            if self.paginate_relationships:
                return self._connection_type_for(type_name)
            else:
                return List[ForwardRef(type_name)]
        # ...
    def connection_resolver_for(
        self, relationship: RelationshipProperty
    ) -> Callable[..., Awaitable[Any]]:
        # ...
        if self.paginate_relationships and relationship.uselist:
            return self.make_connection_wrapper_resolver(
                relationship_resolver,
                self.model_to_type_or_interface_name(relationship.entity.entity),
            )
        else:
            return relationship_resolver

This maintains the current behavior as the default.

I do not expect this solution to be complete or even valid, but more of a proof of concept. Mostly, I am interested in learning the rationale behind the wrapping of related models.

Thanks!

raguiar2 commented 2 years ago

This is a relationship management function to support compatibility with relay and pagination in the graphql docs. See the following links:

https://relay.dev/docs/guided-tour/list-data/rendering-connections/

https://graphql.org/learn/pagination/#pagination-and-edges