Open Cito opened 6 years ago
I just googled and get here, this feature is exactly what I need, wondering why it is still lacking. Bravo dude, appreciate
I'd say this is a wanted feature and a needed feature. It may be a little difficult to implement efficiently though. And I think that is why hybrid_properties
exist. Perhaps a use case where hybrid_properties would not work should be given to demonstrate?
I messed around a bit with this. You can currently do this by making your property a hybrid_property
and making a resolve_PROPERTY
method on the SQLAlchemyObjectType
class.
For example:
class MyModel(...):
id = Column(Integer)
@hybrid_property
def above_ten(self):
return self.id > 10
class MyModelType(SQLAlchemyObjectType):
class Meta:
model = Meta
def resolve_above_ten(self, _):
return self.above_ten
If you use a regular property. You get the "Cannot find query field X" error.
@Kazanz - right, since version 2.0 hybrid properties are supported. But they are not pure SQLAlchemy (it's an extension) and actually used for a different purpose. The decorator @hybrid_property
actually says "I want this to be available on the class level", while we would be (ab)using it to say "I want this to be available in the GraphQL type" and don't care whether the code even works on the class level. Feels a bit like misusing the feature and extension for a different purpose, using the hybrid feature as a side effect only. Also I think it should be possible to use an existing SQLAlchemy model (maybe imported from somewhere) as it is, without modifying it for use by GraphQL. I.e. I prefer the approach that you declare which properties you want to be available only on the GraphQL schema side, not on the SQLAlchemy model side. Just like you can currently exclude fields, you should also be able to include properties in the Meta information on the GraphQL side.
The other problem I mentioned is that hybrid properties currently are only translated to String
fields, because there is no reliable way to guess the type of the property. We could introspect type annotations at the properties to get that information, or when we add some kind of include_property
Meta information as mentioned above, the type could be specified as part of that - which again would allow using an existing SQLAlchemy model without changes.
Posted the above for people who want to use properties in the interim.
Feels a bit like misusing the feature and extension for a different purpose, using the hybrid feature as a side effect only.
Agreed. You should be able to declare the which properties you want available on the graphql type without changing models else where, otherwise its coupled too tightly.
The other problem I mentioned is that hybrid properties currently are only translated to String fields, because there is no reliable way to guess the type of the property. We could introspect type annotations at the properties to get that information, or when we add some kind of include_property Meta information as mentioned above, the type could be specified as part of that - which again would allow using an existing SQLAlchemy model without changes.
Another option is to make a resolve_property
method that can read type annotations so the model doesn't have to be updated. (this is what you said in the original post?)
def resolve_above_ten(...) -> bool:
return self.above_ten
While I personally prefer the type annotation approach, not every uses type annotations and it would be a big leap to force type annotation on end users. So, I lean toward the the approach of an include_property
on the Meta field. It doesn't force type annotations, its closer to familiar patterns from this project and other projects, Django, DRF, etc, and doesn't require editing any existing models.
Another option is to make a resolve_property method that can read type annotations so the model doesn't have to be updated. (this is what you said in the original post?)
That would work, but require adding a resolver for the sole purpose of specifying the type. My idea was actually to introspect the return type of the model class property. But again, that would require changes in the model and we seem to agree that we don't want this - or at least not as the only possible way.
Yes, I agree. at the minimum there should be a way to do it without updating your models. I have worked on a project with hundreds of models containing many properties and it would be a bear to convert them all over. Also I think that by default no properties are allowed unless explicitly set as some properties could have side effects.
What about an option where you explicitly define the type on the SqlalchemyObjectType
with a subclassed graphene scalar?
Something like:
class MyModelType(SQLAlchemyObjectType):
above_ten = Property(graphene.Boolean)
class Meta:
model = Meta
Similar to adding properties to serializers in DRF.
Either that, or:
class MyModelType(SQLAlchemyObjectType):
class Meta:
model = MyModel
include_properties = {'above_ten': bool}
And if include_properties
is given as a sequence, then the types are derived from type hints, and if no properties are set, only the hybrid properties are used. Something like that. But I guess we need to experiment a little bit along these lines to find the best way. I'll try to do that if I find some time.
if include_properties is given as a sequence, then the types are derived from type hints, and if no properties are set, only the hybrid properties are used 👍
I think include_properties
is a better interface. I still would like to be able to use custom logic on serialization though, which could be done by subclassing graphene's Scalar
, Letting include_properties can take scalar or a type to include_properties
would work.
class MsgScalar(Scalar)
@staticmethod
def desc(value):
if value > 100:
return "really big"
elif value > 10:
return "big"
else:
return "not big"
serialize = desc
parse_value = desc
class MyModelType(SQLAlchemyObjectType):
class Meta:
model = MyModel
include_properties = {'above_ten': bool, 'above_ten_msg': MsgScalar}
Wouldn't have to worry about custom types as they could be passed to include_properties, but would be useful if you have properties that return multiple types (like a property that returns a string or null).
Is there any update on this capability? What can be done in the meantime?
Just bumping this again to inquire.
Hi, we're also very interested in this capability :)
Still Interested!
Messing around a bit more with what @Kazanz proposed I came up with something that could be used like this:
class Foo(graphene_sqlalchemy.SQLAlchemyObjectType):
class Meta:
model = UnrelatedField.enable(MyModel)
foo = UnrelatedField(type=graphene.String)
def resolve_foo(self, info):
return "Hello World!"
Using a UnrelatedField
class that subclasses ORMField
and routes all properties unrelated to the underlying sqlalchemy model to a "dummy" hybrid_property
that is hidden from the user.
import sqlalchemy.ext.hybrid
import graphene_sqlalchemy.types
class UnrelatedField(graphene_sqlalchemy.types.ORMField):
"""
ORMField hiding an (ugly) dummy field on the Model.
Usage:
class Foo(graphene_sqlalchemy.SQLAlchemyObjectType):
class Meta:
model = UnrelatedField.enable(MyModel)
foo = UnrelatedField(type=graphene.String)
def resolve_foo(self, info):
return "Hello World!"
"""
__modelattrname = "_gsa_unrelated_catchall"
def __init__(self, **kwargs):
"""Initialize ORMField with kwargs and a catchall model_attr."""
if "model_attr" in kwargs:
raise TypeError("UnrelatedField can not depend on model_attr.")
return super().__init__(model_attr=self.__modelattrname, **kwargs)
@classmethod
def enable(cls, Model):
"""Return a subclass of Model for use with UnrelatedField"""
return type("%sGSAProxy" % Model.__name__, (Model,), {
cls.__modelattrname: sqlalchemy.ext.hybrid.hybrid_property(lambda *_: None)
})
This creates a subclass of the Model which has the hybrid_property
because I thought that would be "cleaner" in regard to not touching the sqlalchemy models that might be used elsewhere in the app. But I'm not sure if the fact the registered model at the graphene-sqlalchemy side being different from the original one will cause any problems elsewhere in the code. I haven't used this solution in a bigger project yet. Any thoughts on that?
Reading the graphene-sqlalchemy's code I don't think always using the same (single) hybrid_property
instead of creating on for each attribute we want would cause any problems, but again I am not sure and haven't done any extensive testing at this time.
And lastly, of course this is still a ugly hack.
Happy to discuss any other ideas of how to do this relatively cleanly with minimal complications when actually writing code using it (and without having to wait for something to be merged in graphene sqlalchemy.)
Not sure if I understand the question (requirements) correctly, but what I needed was just to add a custom field to an SQLAlchemy type returned by one of my root queries, and (after Googling and reading some documentation without success) finally came up with rather simple solution (meta-ish code):
class MyRootQueryType(SQLAlchemyObjectType):
my_custom_field = graphene.Field(
lambda: graphene.Int # a custom subtype (inheriting SQLAlchemyObjectType) works here as well
)
class Meta:
model = MyType
interfaces = (relay.Node,)
def resolve_my_custom_field(my_type_obj, info):
return my_type_obj.some_number ** 2
..if this helps anybody?
@Raekkeri this is exactly what I'm looking for. But this function does not receive input query arguments. For the solution, I put the arguments or immediately custom fields in the object and return it here
class GetMessages(SQLAlchemyObjectType):
class Meta:
model = Message
is_sent = graphene.Field(graphene.Boolean)
def resolve_is_sent(self, info):
return self.is_sent_custom_field # put in another resolver
Bumping this again. Currently, there's not much of a way to serve any kind of annotated/labeled data either. So you're stuck with fields restricted to a subset of model fields. Having to introduce a hybrid property on a model just for this seems excessive and unnecessary.
EDIT : Found a way to perform this through query expressions . This is a simple way to approach this and is compatible with relay too, as long as the proper mappings are made. It's possible to exclude the query_expression itself and get the mapping in the graphene Field itself, so it isn't required to expose the query_expression to the web either.
See this :
Field(
graphene.Float,
source="search_expr",
required=False,
name="score",
)
Picking this up again since we recently pushed forward the use of type annotations for the generation of sqlalchemy-schema fields (https://github.com/graphql-python/graphene-sqlalchemy/pull/340).
It is still in evaluation, since supporting NonNull
would require sound contracting of return types as Optional[T]
in the type hint if they're nullable and make them NonNull
by default which is a deviation from the default behavior in graphene.
If we go forward with that, it might make sense to add support for properties as well. Re-using the type conversion implemented for @hybrid_property
would make this a small change.
In SQLAlchemy, you often want to provide additional custom properties on the model classes, like this:
Unfortunately, such properties are currently not made accessible in GraphQL by graphene_sqlalchemy. While there is a way to exclude existing column properties using the Meta attributes
exclude_fields
andonly_fields
, there is no way to include custom properties such asfull_name
from the above model.One possible solution is to use hybrid properties which are now supported by graphene_sqlalchemy since 2.0. However, "ordinary" properties should also be supported. Often you dont really want or need hybrid properties - it is not always necessary or feasible to provide the property on the class level. Also, the conversion of hybrid properties currently has the limitation that it always generates
String
fields.So I believe we need some mechanism for proxying arbitrary properties from SQLAlchemy to Graphene objects. I'm not yet sure whether all custom public properties (i.e. those not starting with an underscore) except those who are explicitly excluded should be proxied by default (like it is done with hybrid properties), or whether there should be a Meta attribute with a list of properties that must be explicitly set.
We probably also need to add a way to specify which type the (normal or hybrid) properties shall be converted to. This could also be defined in a Meta attribute, or maybe we could check for Python type hints if nothing is defined, like this:
I am willing to contribute with code, but first I want to get some feedback regarding this idea from others.