polgfred
commented
1 year ago 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_solverI 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!
erikwrede
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.
I would expect this to work, and it does. It even works if I make my
helloproperty return alist[list[str]]or something weird like that. But! If you import__future__annotations, this scenario no longer works:This is because with
__future__annotations,-> strcreates aForwardRef, 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?