Hybrid properties don't work in the presence of future annotations

[polgfred]

This has been driving me a bit nuts since the columns/hybrids refactor, and I haven't been able to figure out if it's something new, expected, or something I'm doing wrong. Here's the simplest scenario I could come up with to demonstrate it.

@declarative_mixin
@as_declarative()
class Base:
    pass

class Person(Base):
    __tablename__ = "person"

    id = Column(postgresql.UUID(as_uuid=True), primary_key=True)

    @hybrid_property
    def hello(self) -> str:
        return "world"

class GPerson(SQLAlchemyObjectType):
    class Meta:
        model = Person

class GQuery(ObjectType):
    person = Field(GPerson)

schema = Schema(query=GQuery)

I would expect this to work, and it does. It even works if I make my hello property return a list[list[str]] or something weird like that. But! If you import __future__ annotations, this scenario no longer works:

from __future__ import annotations

<the code from above>

Traceback (most recent call last):
  File "/home/vscode/.local/share/virtualenvs/bios-backend-rDkHOn2N/lib/python3.10/site-packages/graphql/type/definition.py", line 808, in fields
    fields = resolve_thunk(self._fields)
  File "/home/vscode/.local/share/virtualenvs/bios-backend-rDkHOn2N/lib/python3.10/site-packages/graphql/type/definition.py", line 300, in resolve_thunk
    return thunk() if callable(thunk) else thunk
  File "/home/vscode/.local/share/virtualenvs/bios-backend-rDkHOn2N/lib/python3.10/site-packages/graphene/types/schema.py", line 305, in create_fields_for_type
    field_type = create_graphql_type(field.type)
  File "/home/vscode/.local/share/virtualenvs/bios-backend-rDkHOn2N/lib/python3.10/site-packages/graphene/types/field.py", line 116, in type
    return get_type(self._type)
  File "/home/vscode/.local/share/virtualenvs/bios-backend-rDkHOn2N/lib/python3.10/site-packages/graphene/types/utils.py", line 42, in get_type
    return _type()
  File "/home/vscode/.local/share/virtualenvs/bios-backend-rDkHOn2N/lib/python3.10/site-packages/graphene_sqlalchemy/converter.py", line 615, in forward_reference_solver
    raise TypeError(
TypeError: No model found in Registry for forward reference for type ForwardRef('str'). Only forward references to other SQLAlchemy Models mapped to SQLAlchemyObjectTypes are allowed.

This is because with __future__ annotations, -> str creates a ForwardRef, and forward refs (unlike normal types) have to be SQLAlchemy models (https://github.com/graphql-python/graphene-sqlalchemy/blob/master/graphene_sqlalchemy/converter.py#L610). So you have to explicitly supply hybrid field definitions, even for methods that return simple primitive types. This is a huge inconvenience, as we have many of these kinds of computed properties in our codebase.

Any ideas if this is something fixable? Or are we just stuck with having to manually add field definitions everywhere?

[polgfred]

Doing more research on this today.

As described above, from __future__ import annotations makes EVERYTHING a ForwardRef — even primitives, builtins, and things that are actually imported in the module. Essentially the parser replaces -> str with -> "str" everywhere in the module. This means that the current way of treating ForwardRefs as a special case that can only resolve to stuff in the SQLA type registry will never work with future annotations turned on. And turning those on is a very common use-case: particularly in database applications where circular references between relationships abound.

So I did a quick pass at handling ForwardRefs by first attempting to evaluate the type (the way inspect.get_annotations() would do), and then falling back to the original behavior of consulting the registry:

@convert_sqlalchemy_type.register(safe_isinstance(ForwardRef))
def convert_sqlalchemy_hybrid_property_forwardref(
    type_arg: Any, column: hybrid_property | None = None, **kwargs
):
    from .registry import get_global_registry

    # Try to get the module namespace where the hybrid property is defined.
    namespace = sys.modules[column.__module__].__dict__ if column else globals()

    def forward_reference_solver():
        try:
            # Evaluate the type definition in the module namespace.
            # (This is what inspect.get_annotations(eval_str=True) does)
            return convert_sqlalchemy_type(
                eval(type_arg.__forward_arg__, namespace), **kwargs
            )
        except:
            model = registry_sqlalchemy_model_from_str(type_arg.__forward_arg__)
            if not model:
                raise TypeError(
                    "No model found in Registry for forward reference for type %s. "
                    "Only forward references to other SQLAlchemy Models mapped to "
                    "SQLAlchemyObjectTypes are allowed." % type_arg
                )
            return get_global_registry().get_type_for_model(model)

    return forward_reference_solver

I think this could be made even more efficient by attempting the eval right away, and returning it if all the types in the annotation are already available, but this demonstrates how it would work. If you add from __future__ import annotations to tests/models.py, you can see that the test_sqlalchemy_hybrid_property_type_inference unit test fails because it's expecting those model types to be types, rather than the forward_reference_solver function. So we may need to have tests that check both scenarios, with and without __future__ annotations turned on.

With this change, all the hybrid properties in our codebase just work, with no extra fiddling or overriding.

I can roll this into a PR, let me know how you want to proceed!

[polgfred]

Actually, we don't even need to consult the model type registry in a separate step, if we just dump the contents of the registry into the namespace as well. Then we get the added benefit of being able to define hybrid properties that return composites like list[Model], Optional[Model], Model1 | Model2, etc., which is something that the current matching mechanism doesn't allow.

[polgfred]

Ok, reading this now, and it pretty much describes all the pain I'm having trying to get this to work. :) I think I may revert all our future annotations stuff until this much improved design of annotations is ready.

[erikwrede]

Thanks for the report! Yes, this is definitely an issue. Solving it at the current state is not trivial, because we need to ensure forward refs are actually imported to check their type (hence the usage of graphene.Dynamic which ensures the reference is first resolved at schema generation when all models should be registered). When we expect all type annotations to be Forward Refs, we need to take care of that ourselves, which is more complicated (it's doable, other libraries handle that as well). It's just a task that no one has gotten to yet. As to the PEP, I've seen it before, but it I havent seen any info on expected implementation. So we should probably not rely on that and favor a clean implementation in the current working state.

[polgfred]

I'm definitely not suggesting trying to implement the PEP-649 approach yet... but it does seem like the community is going to be moving in that direction, so I'm hesitant to dump much more work into this approach because it feels like chasing my tail. I did come up with a fairly clean way (I think?) of making things work with ForwardRefs that probably covers 95+% of use cases, but I don't know if it's reasonable or possible to make it work in all cases, due to the need to ensure that all types in the return-type annotation are imported/defined/in-scope. But I'm happy to roll what I was doing into a draft PR for you to take a look at. :)

One of the annoyances I was running into is that there are unit tests that define classes locally in the test function. Those classes are difficult to find at runtime — inspect.getclosurevars() can find it, but only if it's referenced in the hybrid function body (i.e. it's not enough for it to be the return type). That feels pretty brittle to support, and probably isn't something that will happen often in the real world.

[erikwrede]

so I'm hesitant to dump much more work into this approach because it feels like chasing my tail.

I Agree

Curious to see the draft, let's continue the discussion on the PR! 🙂