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

hybrid_property annotations with built-in types defaulting to ForwardRef and failing schema compilation #396

Open
yesthesoup opened this issue Jul 5, 2023 · 2 comments

Comments

@yesthesoup
Copy link

yesthesoup commented Jul 5, 2023

dependencies:

python 3.10.7

sqlalchemy==1.4.48
graphene==3.2.2
graphene-sqlalchemy==v3.0.0b4
graphql-server[flask]==v3.0.0b6

I am upgrading from graphene==2.1.9 and graphene-sqlalchemy==2.3.0 where these hybrid properties worked.

simplified example:

# myproject/routes/graphql_view.py

from myproject.schema import schema

GraphQLView.as_view('graphql', schema=schema, graphiql=False) # simplified, used in flask create_app in __init__.py

# myproject/models/core.py

class MyModel(db.Model, DictMixin, Timestamp):
    id = db.Column(db.Integer, primary_key=True, autoincrement=True)
    ...

    @hybrid_property
    def prop_string -> str:
       # logic based on model's other fields
       result: str = ...
       return result

    @hybrid_property
    def prop_boolean -> bool:
       # logic based on model's other fields
       result: bool = ...
       return result

# myproject/schema.py

from myproject.models.core import MyModel

class MyModelQuery(SQLAlchemyObjectType):
    class Meta:
        model = MyModel
        interfaces = (relay.Node,)

class Query(graphene.ObjectType):
    node = relay.Node.Field()
    ... # other query fields
    model = graphene.Field(MyModelQuery, model_id=graphene.Int())

    def resolve_model(self, info, model_id):
        query = MyModelQuery.get_query(info)
        return query.get(model_id)

schema = graphene.Schema(query=Query, mutation=Mutation)

It seems like support was added for these annotations in #340 in v3.0.0b2, but then is no longer working as expected since several refactors.

I get this error for the string return type hybrid

../../../myproject/schema.py:581: in <module>
    schema = graphene.Schema(query=Query, mutation=Mutation)
../../../venv/lib/python3.10/site-packages/graphene/types/schema.py:440: in __init__
    self.graphql_schema = GraphQLSchema(
../../../venv/lib/python3.10/site-packages/graphql/type/schema.py:224: in __init__
    collect_referenced_types(query)
../../../venv/lib/python3.10/site-packages/graphql/type/schema.py:433: in collect_referenced_types
    collect_referenced_types(field.type)
../../../venv/lib/python3.10/site-packages/graphql/type/schema.py:433: in collect_referenced_types
    collect_referenced_types(field.type)
../../../venv/lib/python3.10/site-packages/graphql/type/schema.py:433: in collect_referenced_types
    collect_referenced_types(field.type)
../../../venv/lib/python3.10/site-packages/graphql/type/schema.py:432: in collect_referenced_types
    for field in named_type.fields.values():
/[...]/.pyenv/versions/3.10.7/lib/python3.10/functools.py:981: in __get__
    val = self.func(instance)
../../../venv/lib/python3.10/site-packages/graphql/type/definition.py:811: in fields
    raise cls(f"{self.name} fields cannot be resolved. {error}") from error
E   TypeError: MyModelQuery fields cannot be resolved. No model found in Registry for forward reference for type ForwardRef('str'). Only forward references to other SQLAlchemy Models mapped to SQLAlchemyObjectTypes are allowed.

and this for the Boolean.

TypeError: MyModelQuery fields cannot be resolved. No model found in Registry for forward reference for type ForwardRef('bool'). Only forward references to other SQLAlchemy Models mapped to SQLAlchemyObjectTypes are allowed.

MyModel in reality has many many fields so it was also hard to figure out it was the hybrid_property fields that were causing problems, vs. the actual relationship fields etc

Which seems to be because the function convert_hybrid_property_return_type
https://github.com/graphql-python/graphene-sqlalchemy/blob/v3.0.0b4/graphene_sqlalchemy/converter.py#L640

returns a string version of the type, and string defaults to being processed by convert_sqlalchemy_hybrid_property_bare_str even though those strings are valid types, not ForwardRefs.
https://github.com/graphql-python/graphene-sqlalchemy/blob/v3.0.0b4/graphene_sqlalchemy/converter.py#L632

I am getting around this by explicitly declaring the graphene types but my understanding was that graphene-sqlalchemy should be able to pick them up automatically due to the recent support that was integrated, as referenced above.

workaround:

class MyModelQuery(SQLAlchemyObjectType):
    class Meta:
        model = MyModel
        interfaces = (relay.Node,)

    prop_string = graphene.String()
    prop_boolean = graphene.Boolean()

If this is intended behaviour, could the error be more helpful? Spent several hours digging as I was not attempting anything with a ForwardRef type.

@erikwrede
Copy link
Member

Hello @yesthesoup,
Sorry for the delayed reply.

You're spot on with your understanding of the problem. Our approach to handling forward references was indeed shaped during a time of less clarity in Python's typing landscape. The shift towards from __future__ import annotations does change the dynamics significantly.

Your workaround of explicitly excluding affected fields or setting types for specific fields is a solid interim solution. Our recent update (#371 was intended to improve handling by inferring types from annotations instead of converting hybrids to strings. However, with from __future__ import annotations turning all hints into ForwardRefs, this does create a hurdle in scenarios like yours.

Manually setting the type for hybrids or use manual ForwardRefs (with type hints in "") while avoiding from __future__ import annotations are good ways to navigate this for now.

I agree that this is not the ideal state, and we're aiming to address this post the 3.0 release. Thanks for pushing this by sharing your experience.

@yesthesoup
Copy link
Author

Thank you for the reply - good to know this is on the radar.

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

No branches or pull requests

2 participants