add missing page_info / cursors for Association Proxy logic (#241)

* add missing page_info / cursors for relay

* add RELEASE.md

* add missing cursor

* run formatter and update release

* added tests to proxy_association, still missing a test case

* Add a better messages in the exception, add new test with keysetconnection

* add _resolve_connection_edges to a staticmethod and run pre-commit

* update Release md

* Run pre-commit in exc.py

* run pre-commit in conftest.py

* run pre-commit in test_association_proxy.py

* run pre-commit in test_mapper

* Fix tests on windows error

* add a new test to null relationship

* refactor test_association_proxy for better understanding

* update release.md to add tylernisonoff credits

---------

Co-authored-by: Tyler Nisonoff <[email protected]>

Author: Ckk3

RELEASE.md

@@ -0,0 +1,5 @@
+Release type: patch
+
+Ensure association proxy resolvers return valid relay connections, including `page_info` and edge `cursor` details, even for empty results.
+
+Thanks to https://github.com/tylernisonoff for the original PR.

src/strawberry_sqlalchemy_mapper/exc.py

@@ -2,37 +2,38 @@ class UnsupportedColumnType(Exception):
     def __init__(self, key, type):
         super().__init__(
             f"Unsupported column type: `{type}` on column: `{key}`. "
-            + "Possible fix: exclude this column"
+            "Possible fix: exclude this column"
         )
 
 
 class UnsupportedAssociationProxyTarget(Exception):
     def __init__(self, key):
         super().__init__(
             f"Association proxy `{key}` is expected to be of form "
-            + "association_proxy(relationship_name, other relationship name)"
+            "association_proxy(relationship_name, other relationship name). "
+            "Ensure it matches the expected form or add this association proxy to __exclude__."
         )
 
 
 class HybridPropertyNotAnnotated(Exception):
     def __init__(self, key):
         super().__init__(
             f"Descriptor `{key}` is a hybrid property, but does not have an "
-            + "annotated return type"
+            "annotated return type"
         )
 
 
 class UnsupportedDescriptorType(Exception):
     def __init__(self, key):
         super().__init__(
             f"Descriptor `{key}` is expected to be a column, relationship, "
-            + "or association proxy."
+            "or association proxy."
         )
 
 
 class InterfaceModelNotPolymorphic(Exception):
     def __init__(self, model):
         super().__init__(
             f"Model `{model}` is not polymorphic or is not the base model of its "
-            + "inheritance chain, and thus cannot be used as an interface."
+            "inheritance chain, and thus cannot be used as an interface."
         )

src/strawberry_sqlalchemy_mapper/mapper.py

@@ -69,14 +69,12 @@
     Mapper,
     RelationshipProperty,
 )
-from sqlalchemy.orm.state import InstanceState
 from sqlalchemy.sql.type_api import TypeEngine
 from strawberry import relay
 from strawberry.annotation import StrawberryAnnotation
 from strawberry.scalars import JSON as StrawberryJSON
 from strawberry.types import Info
 from strawberry.types.base import WithStrawberryObjectDefinition, get_object_definition
-from strawberry.types.field import StrawberryField
 from strawberry.types.lazy_type import LazyType
 from strawberry.types.private import is_private
 
@@ -97,13 +95,14 @@
 from strawberry_sqlalchemy_mapper.scalars import BigInt
 
 if TYPE_CHECKING:
+    from sqlalchemy.orm.state import InstanceState
     from sqlalchemy.sql.expression import ColumnElement
+    from strawberry.types.field import StrawberryField
 
 BaseModelType = TypeVar("BaseModelType")
 
 SkipTypeSentinelT = NewType("SkipTypeSentinelT", object)
-SkipTypeSentinel = cast(SkipTypeSentinelT, sentinel.create("SkipTypeSentinel"))
-
+SkipTypeSentinel = cast("SkipTypeSentinelT", sentinel.create("SkipTypeSentinel"))
 
 #: Set on generated types to the original type handed to the decorator
 _ORIGINAL_TYPE_KEY = "_original_type"
@@ -151,13 +150,11 @@ class StrawberrySQLAlchemyType(Generic[BaseModelType]):
 
     @overload
     @classmethod
-    def from_type(cls, type_: type, *, strict: Literal[True]) -> Self:
-        ...
+    def from_type(cls, type_: type, *, strict: Literal[True]) -> Self: ...
 
     @overload
     @classmethod
-    def from_type(cls, type_: type, *, strict: bool = False) -> Optional[Self]:
-        ...
+    def from_type(cls, type_: type, *, strict: bool = False) -> Optional[Self]: ...
 
     @classmethod
     def from_type(
@@ -468,28 +465,34 @@ def make_connection_wrapper_resolver(
         edge_type = self._edge_type_for(type_name)
 
         async def wrapper(self, info: Info):
-            # TODO: Add pagination support to dataloader resolvers
-            edges = [
-                edge_type.resolve_edge(
-                    related_object,
-                    cursor=i,
-                )
-                for i, related_object in enumerate(await resolver(self, info))
-            ]
-            return connection_type(
-                edges=edges,
-                page_info=relay.PageInfo(
-                    has_next_page=False,
-                    has_previous_page=False,
-                    start_cursor=edges[0].cursor if edges else None,
-                    end_cursor=edges[-1].cursor if edges else None,
-                ),
+            return StrawberrySQLAlchemyMapper._resolve_connection_edges(
+                await resolver(self, info), edge_type, connection_type
             )
 
         setattr(wrapper, _IS_GENERATED_RESOLVER_KEY, True)
 
         return wrapper
 
+    @staticmethod
+    def _resolve_connection_edges(related_objects, edge_type, connection_type):
+        # TODO: Add pagination support to dataloader resolvers
+        edges = [
+            edge_type.resolve_edge(
+                related_object,
+                cursor=i,
+            )
+            for i, related_object in enumerate(related_objects)
+        ]
+        return connection_type(
+            edges=edges,
+            page_info=relay.PageInfo(
+                has_next_page=False,
+                has_previous_page=False,
+                start_cursor=edges[0].cursor if edges else None,
+                end_cursor=edges[-1].cursor if edges else None,
+            ),
+        )
+
     def relationship_resolver_for(
         self, relationship: RelationshipProperty
     ) -> Callable[..., Awaitable[Any]]:
@@ -499,7 +502,7 @@ def relationship_resolver_for(
         """
 
         async def resolve(self, info: Info):
-            instance_state = cast(InstanceState, inspect(self))
+            instance_state = cast("InstanceState", inspect(self))
             if relationship.key not in instance_state.unloaded:
                 related_objects = getattr(self, relationship.key)
             else:
@@ -575,7 +578,15 @@ async def resolve(self, info: Info):
             in_between_objects = await in_between_resolver(self, info)
             if in_between_objects is None:
                 if is_multiple:
-                    return connection_type(edges=[])
+                    return connection_type(
+                        edges=[],
+                        page_info=relay.PageInfo(
+                            has_next_page=False,
+                            has_previous_page=False,
+                            start_cursor=None,
+                            end_cursor=None,
+                        ),
+                    )
                 else:
                     return None
             if descriptor.value_attr in in_between_mapper.relationships:
@@ -595,7 +606,9 @@ async def resolve(self, info: Info):
                     outputs = await end_relationship_resolver(in_between_objects, info)
                 if not isinstance(outputs, collections.abc.Iterable):
                     return outputs
-                return connection_type(edges=[edge_type(node=obj) for obj in outputs])
+                return StrawberrySQLAlchemyMapper._resolve_connection_edges(
+                    outputs, edge_type, connection_type
+                )
             else:
                 assert descriptor.value_attr in in_between_mapper.columns
                 if isinstance(in_between_objects, collections.abc.Iterable):
@@ -668,7 +681,7 @@ def convert(type_: Any) -> Any:
             type_.__annotations__ = {
                 k: v for k, v in old_annotations.items() if is_private(v)
             }
-            mapper: Mapper = cast(Mapper, inspect(model))
+            mapper: Mapper = cast("Mapper", inspect(model))
             generated_field_keys = []
 
             excluded_keys = getattr(type_, "__exclude__", [])
@@ -707,7 +720,7 @@ def convert(type_: Any) -> Any:
                     generated_field_keys,
                 )
                 sqlalchemy_field = cast(
-                    StrawberryField,
+                    "StrawberryField",
                     field(
                         resolver=self.connection_resolver_for(
                             relationship,
@@ -751,7 +764,7 @@ def convert(type_: Any) -> Any:
                         type_, key, strawberry_type, generated_field_keys
                     )
                     sqlalchemy_field = cast(
-                        StrawberryField,
+                        "StrawberryField",
                         field(
                             resolver=self.association_proxy_resolver_for(
                                 mapper,
@@ -789,7 +802,8 @@ def convert(type_: Any) -> Any:
             # ignore inherited `is_type_of`
             if "is_type_of" not in type_.__dict__:
                 type_.is_type_of = (
-                    lambda obj, info: type(obj) == model or type(obj) == type_
+                    lambda obj, info: type(obj) == model  # noqa: E721
+                    or type(obj) == type_  # noqa: E721
                 )
 
             # Default querying methods for relay
@@ -822,7 +836,7 @@ def convert(type_: Any) -> Any:
                         setattr(
                             type_,
                             attr,
-                            types.MethodType(cast(classmethod, meth).__func__, type_),
+                            types.MethodType(cast("classmethod", meth).__func__, type_),
                         )
 
             # need to make fields that are already in the type

tests/conftest.py

@@ -20,14 +20,16 @@
 from sqlalchemy.ext import asyncio
 from sqlalchemy.ext.asyncio import create_async_engine
 from sqlalchemy.ext.asyncio.engine import AsyncEngine
+from strawberry_sqlalchemy_mapper import StrawberrySQLAlchemyMapper
 from testing.postgresql import Postgresql, PostgresqlFactory
 
 SQLA_VERSION = version.parse(sqlalchemy.__version__)
 SQLA2 = SQLA_VERSION >= version.parse("2.0")
 
 
 logging.basicConfig()
-logging.getLogger("sqlalchemy.engine").setLevel(logging.INFO)
+log = logging.getLogger("sqlalchemy.engine")
+log.setLevel(logging.INFO)
 
 
 def _pick_unused_port():
@@ -55,7 +57,7 @@ def postgresql(postgresql_factory) -> Postgresql:
     # Our windows test pipeline doesn't play nice with postgres because
     # Github Actions doesn't support containers on windows.
     # It would probably be nicer if we chcked if postgres is installed
-    logging.info("Skipping postgresql tests on Windows OS")
+    log.info("Skipping postgresql tests on Windows OS")
     SUPPORTED_DBS = []
 else:
     SUPPORTED_DBS = ["postgresql"]  # TODO: Add sqlite and mysql.
@@ -111,3 +113,8 @@ def async_sessionmaker(async_engine):
 @pytest.fixture
 def base():
     return orm.declarative_base()
+
+
+@pytest.fixture
+def mapper():
+    return StrawberrySQLAlchemyMapper()