Make relationship arrays nonnull and required

[glentakahashi]

See #251

It will be harder (maybe impossible?) to make the Child -> Parent relationship Non-null, because there is not a single way to determine whether or not it's non-null.

In this example class

Child(db.Model):
    __tablename__ = "child"
    id = db.Column(Integer, primary_key=True)

    parent_id = db.Column(Integer, ForeignKey("parent.id"), nullable=False)
    parent = db.relationship("Parent", back_populates="children")

We can know for sure that parent is non-null because the parent_id column is nullable=False, however if it used primaryjoin=xxx or foreign_keys=[key1,key2], We would have to introspect those and check all the possible columns for nullability.

However, for the O2M or the M2M cases, SQLAlchemy returns a collection of objects: https://docs.sqlalchemy.org/en/13/orm/collections.html#customizing-collection-access

(It looks like you can also actually return a dictionary mapping instead of a colleciton, however graphene-sqlalchemy currently doesn't support this use case anyway since it hardcodes List)

[coveralls]

Coverage remained the same at 97.343% when pulling 5bed67f on hex-inc:gt/nonnull-relationships into 89c3726 on graphql-python:master.

[adrianschneider94]

How is the status of this PR?
I think this is a important feature, the current graphql types are more ore less wrong.

[this-vidor]

if it used primaryjoin=xxx or foreign_keys=[key1,key2], We would have to introspect those and check all the possible columns for nullability.

Agreed! Isn't that sort of work the value-premise of this library?

[glentakahashi]

We have stopped using this library (and python completely actually) so if someone wants to continue off my work feel free to do so in a new PR!

[this-vidor]

Dear fellow travelers:

I believe this would accomplish the check described in this PR description:

from sqlalchemy import inspect

def is_relationship_nullable(model, relationship_name: str) -> bool:
    """
    Return True if this is a singular relationship and any of the foreign-keys
    on the local side are nullable.
    """
    mapper = inspect(model)
    relationship = mapper.relationships[relationship_name]

    if relationship.uselist:
        return False

    return any(
        pair[0].nullable
        for pair in relationship.local_remote_pairs
    )

While I will probably not open a PR into this project, I will probably be using a code snippet
like this one as a workaround.

---

[treasuryspring]

I ended doing a monkey patch (not sure if a PR would work at the project seems inactive):

from graphene import Field
from graphene_sqlalchemy.batching import get_batch_resolver
from graphene_sqlalchemy.resolvers import get_custom_resolver, get_attr_resolver
import graphene_sqlalchemy.converter


def is_relationship_nullable(relationship) -> bool:
    """
    Return True if this is a singular relationship and any of the foreign-keys
    on the local side are nullable.
    """
    if relationship.uselist:
        return False

    return any(
        pair[0].nullable
        for pair in relationship.local_remote_pairs
    )


def _convert_o2o_or_m2o_relationship(relationship_prop, obj_type, batching, orm_field_name, **field_kwargs):
    """
    Convert one-to-one or many-to-one relationshsip. Return an object field.

    :param sqlalchemy.RelationshipProperty relationship_prop:
    :param SQLAlchemyObjectType obj_type:
    :param bool batching:
    :param str orm_field_name:
    :param dict field_kwargs:
    :rtype: Field
    """
    child_type = obj_type._meta.registry.get_type_for_model(relationship_prop.mapper.entity)

    resolver = get_custom_resolver(obj_type, orm_field_name)
    if resolver is None:
        resolver = get_batch_resolver(relationship_prop) if batching else \
            get_attr_resolver(obj_type, relationship_prop.key)

    return Field(child_type, resolver=resolver, required=not is_relationship_nullable(relationship_prop), **field_kwargs)


graphene_sqlalchemy.converter._convert_o2o_or_m2o_relationship = _convert_o2o_or_m2o_relationship

[erikwrede]

@treasuryspring If you're interested, I would review and merge your PR including this feature and the corresponding tests ASAP!