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

Add a way to serve custom properties on model classes #103

Open
Cito opened this issue Dec 17, 2017 · 18 comments
Open

Add a way to serve custom properties on model classes #103

Cito opened this issue Dec 17, 2017 · 18 comments
Milestone

Comments

@Cito
Copy link
Member

Cito commented Dec 17, 2017

In SQLAlchemy, you often want to provide additional custom properties on the model classes, like this:

class User(Base):
    __tablename__ = 'users'
    first_name = Column(String)
    last_name = Column(String)

    @property
    def full_name(self):
        return self.first_name + ' ' + self.last_name

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 and only_fields, there is no way to include custom properties such as full_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:

    def full_name(self) -> str:
        return self.first_name + ' '  + self.last_name

    def current_age(self) -> int:
        return current_year() - self.year_of_birth

I am willing to contribute with code, but first I want to get some feedback regarding this idea from others.

@microidea
Copy link

I just googled and get here, this feature is exactly what I need, wondering why it is still lacking.
Bravo dude, appreciate

@exit99
Copy link

exit99 commented Jan 22, 2018

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?

@exit99
Copy link

exit99 commented Jan 22, 2018

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.

@Cito
Copy link
Member Author

Cito commented Jan 23, 2018

@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.

@exit99
Copy link

exit99 commented Jan 24, 2018

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.

@Cito
Copy link
Member Author

Cito commented Jan 24, 2018

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.

@exit99
Copy link

exit99 commented Jan 24, 2018

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.

@Cito
Copy link
Member Author

Cito commented Jan 24, 2018

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.

@exit99
Copy link

exit99 commented Jan 25, 2018

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).

@flewellyn
Copy link

Is there any update on this capability? What can be done in the meantime?

@flewellyn
Copy link

Just bumping this again to inquire.

@satshabad-cr
Copy link

Hi, we're also very interested in this capability :)

@JasperCheung
Copy link

Still Interested!

@wapiflapi
Copy link

wapiflapi commented Sep 25, 2019

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)
        })

⚠️ the downsides & warnings

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.)

@Raekkeri
Copy link

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?

@romdevios
Copy link

@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

@AHarmlessPyro
Copy link

AHarmlessPyro commented Oct 11, 2021

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",
    )

@erikwrede
Copy link
Member

Picking this up again since we recently pushed forward the use of type annotations for the generation of sqlalchemy-schema fields (#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.

@erikwrede erikwrede added this to the 3.0 milestone May 15, 2022
@erikwrede erikwrede mentioned this issue May 15, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests