Skip to content

Commit

Permalink
feat!: Stricter non-null fields for relationships (#367)
Browse files Browse the repository at this point in the history
to-many relationships are now non-null by default. (List[MyType] -> List[MyType!]!)
The behavior can be adjusted back to legacy using `converter.set_non_null_many_relationships(False)`
or using an `ORMField` manually setting the type for more granular Adjustments
  • Loading branch information
polgfred committed Jan 13, 2023
1 parent 2041835 commit d3a4320
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 3 deletions.
42 changes: 41 additions & 1 deletion graphene_sqlalchemy/converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,39 @@

is_selectin_available = getattr(strategies, "SelectInLoader", None)

"""
Flag for whether to generate stricter non-null fields for many-relationships.
For many-relationships, both the list element and the list field itself will be
non-null by default. This better matches ORM semantics, where there is always a
list for a many relationship (even if it is empty), and it never contains None.
This option can be set to False to revert to pre-3.0 behavior.
For example, given a User model with many Comments:
class User(Base):
comments = relationship("Comment")
The Schema will be:
type User {
comments: [Comment!]!
}
When set to False, the pre-3.0 behavior gives:
type User {
comments: [Comment]
}
"""
use_non_null_many_relationships = True


def set_non_null_many_relationships(non_null_flag):
global use_non_null_many_relationships
use_non_null_many_relationships = non_null_flag


def get_column_doc(column):
return getattr(column, "doc", None)
Expand Down Expand Up @@ -160,7 +193,14 @@ def _convert_o2m_or_m2m_relationship(
)

if not child_type._meta.connection:
return graphene.Field(graphene.List(child_type), **field_kwargs)
# check if we need to use non-null fields
list_type = (
graphene.NonNull(graphene.List(graphene.NonNull(child_type)))
if use_non_null_many_relationships
else graphene.List(child_type)
)

return graphene.Field(list_type, **field_kwargs)

# TODO Allow override of connection_field_factory and resolver via ORMField
if connection_field_factory is None:
Expand Down
35 changes: 35 additions & 0 deletions graphene_sqlalchemy/tests/test_converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
convert_sqlalchemy_hybrid_method,
convert_sqlalchemy_relationship,
convert_sqlalchemy_type,
set_non_null_many_relationships,
)
from ..fields import UnsortedSQLAlchemyConnectionField, default_connection_field_factory
from ..registry import Registry, get_global_registry
Expand Down Expand Up @@ -71,6 +72,16 @@ class Model(declarative_base()):
)


@pytest.fixture
def use_legacy_many_relationships():
set_non_null_many_relationships(False)
try:
yield
finally:
set_non_null_many_relationships(True)



def test_hybrid_prop_int():
@hybrid_property
def prop_method() -> int:
Expand Down Expand Up @@ -501,6 +512,30 @@ class Meta:
True,
"orm_field_name",
)
# field should be [A!]!
assert isinstance(dynamic_field, graphene.Dynamic)
graphene_type = dynamic_field.get_type()
assert isinstance(graphene_type, graphene.Field)
assert isinstance(graphene_type.type, graphene.NonNull)
assert isinstance(graphene_type.type.of_type, graphene.List)
assert isinstance(graphene_type.type.of_type.of_type, graphene.NonNull)
assert graphene_type.type.of_type.of_type.of_type == A


@pytest.mark.usefixtures("use_legacy_many_relationships")
def test_should_manytomany_convert_connectionorlist_list_legacy():
class A(SQLAlchemyObjectType):
class Meta:
model = Pet

dynamic_field = convert_sqlalchemy_relationship(
Reporter.pets.property,
A,
default_connection_field_factory,
True,
"orm_field_name",
)
# field should be [A]
assert isinstance(dynamic_field, graphene.Dynamic)
graphene_type = dynamic_field.get_type()
assert isinstance(graphene_type, graphene.Field)
Expand Down
6 changes: 4 additions & 2 deletions graphene_sqlalchemy/tests/test_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,10 @@ class Meta:

pets_field = ReporterType._meta.fields["pets"]
assert isinstance(pets_field, Dynamic)
assert isinstance(pets_field.type().type, List)
assert pets_field.type().type.of_type == PetType
assert isinstance(pets_field.type().type, NonNull)
assert isinstance(pets_field.type().type.of_type, List)
assert isinstance(pets_field.type().type.of_type.of_type, NonNull)
assert pets_field.type().type.of_type.of_type.of_type == PetType
assert pets_field.type().description == "Overridden"


Expand Down

0 comments on commit d3a4320

Please sign in to comment.