Support setting @hybrid_property's return type from the functions type annotations.

[flipbit03]

This PR adds support for generating the GraphQL types from a @hybrid_property based on the actual type hints set in the function's return type annotation.

Based on this feedback comment, I devised a simple way to dispatch the actual final type generation machinery to a function using a mechanism similar to standard library's @singledispatch, but based on function matchers instead of fixed argument types. This way, we support most basic types in the standard library out of the box, and also give the end user the ability to extend the matcher system by registering new matchers in their own systems.

Types signatures supported:

str, int, float, bool, datetime.date, datetime.time, datetime.datetime, decimal.Decimal, List[T] where T is all of the above, and also any other previously generated SQLAlchemyObjectType and also self-referential T where T: SQLAlchemy model (using ForwardRef string-like references).

特別に、I'd like to thank @conao3 for the initial work this PR is based on. ありがとう！

closes #333

[codecov-commenter]

Codecov Report

Merging #340 (a17782b) into master (5da2048) will increase coverage by 0.28%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #340      +/-   ##
==========================================
+ Coverage   96.95%   97.23%   +0.28%     
==========================================
  Files           9        9              
  Lines         623      686      +63     
==========================================
+ Hits          604      667      +63     
  Misses         19       19              

Impacted Files	Coverage Δ
graphene_sqlalchemy/converter.py	97.75% <100.00%> (+0.76%)	:arrow_up:
graphene_sqlalchemy/utils.py	96.70% <100.00%> (+0.81%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5da2048...a17782b. Read the comment docs.

[erikwrede]

Thank you for the PR! I'm not done with the review yet, but it looks great so far 😄—only a few style and readability comments I've made. Maybe it would've been better to do the Article and Reporter re-formatting in a separate PR since it is not issue-related. On the other hand, my formatter doesn't complain, and it looks a bit tidier than before! You've added the ArticleType to multiple test cases where it's not used. Is there a reason for that?

Other than that, it looks great so far. Still testing it a bit, but this is a helpful change. Good job, @flipbit03 @conao3.

[flipbit03]

Thank you for the initial review!

Answering your question: To thoroughly test the new type inferring capabilities of the @hybrid_property, I have extended the set of properties we had in the Reporter SQLAlchemy model to also have a hybrid property which returns another SQLAlchemy model, in this case, an Article.

class Reporter(Base):
    ...

    @hybrid_property
    def hybrid_prop_first_article(self) -> Article:
        return self.articles[0]

So now, since we specified that type in there, to build the GraphQL ReporterType from Reporter, we need to have a previously-built GraphQL object in scope as well that stems from Article for ReporterType to be correctly generated. Those tests needed to be updated because the registry is cleared between each test function, so ArticleType had to be introduced in those test function scopes for ReporterType to be built successfully.

The default case:

@singledispatchbymatchfunction
def convert_hybrid_property_return_type_inner(arg: Any):
    existing_graphql_type = get_global_registry().get_type_for_model(arg)
    if existing_graphql_type:
        return existing_graphql_type
    raise Exception(f"I don't know how to generate a GraphQL type out of a \"{arg}\" type")

If you set an unsupported type signature (in the example above, a GraphQL object which doesn't exist yet), an exception is thrown. So if all you need is to return a String, we don't need to specify any type signature at all.

Idea: If you think throwing an exception is not the ideal case there, we can revert the default behavior to only return a String, so that if we get a crazy type signature or something that's needed on the SQLAlchemy side of things, the user will still be able to translate its type to GraphQL in 'best effort' capacity.

[flipbit03]

I also just thought about generating support for python Enum's as well, since that's in the standard library and graphene already has a function to convert a Python enum into a Graphene one.

[erikwrede]

Idea: If you think throwing an exception is not the ideal case there, we can revert the default behavior to only return a String so that if we get a crazy type signature or something that's needed on the SQLAlchemy side of things, the user will still be able to translate its type to GraphQL in 'best effort' capacity.

I think this is the best way to do it. Falling back to String would ensure backward compatibility for Users currently working with another solution. Maybe, a high-visibility warning could transparently communicate the missing Converter and the Fallback to String.

Answering your question: To thoroughly test the new type inferring capabilities of the @hybrid_property, I have extended the set of properties we had in the Reporter SQLAlchemy model to also have a hybrid property which returns another SQLAlchemy model, in this case, an Article.

Thanks for the clarification! As I mentioned above, I haven't reviewed every file just yet, so I must have missed that. I think that's a critical test case. However, I still believe that it should be moved to separate models for the hybrid property tests, as mentioned in my Review:

I think a separate SQA-model should be used for testing the hybrid property types. Otherwise, 4 unit tests, of which three are not directly related to hybrid properties, have to be adjusted every time a new converter is added. (test_sqlalchemy_override_fields,test_exclude_fields,test_custom_objecttype_registered) [...]

If you have a different opinion on that, feel free to convince me otherwise.

[erikwrede]

I also just thought about generating support for python Enum's as well, since that's in the standard library and graphene already has a function to convert a Python enum into a Graphene one.

Sounds good! In this case, it should not be much work to include the Graphene Enums. Do you see a use case for that?

[flipbit03]

Thank you for the suggestions. Agree on all of them.

I am still not able to see the Review Comments you mentioned in the reply above. I might be doing something wrong :sweat: but did you send off your review yet? I don't see any request for changes etc.

To be done:

• [x] Fallback to String when no type can be found, but issue a warning.
• [x] Revert all tests and write new ones specifically for the new model which tackle the hybrid_property test stuff

[erikwrede]

Sorry, I forgot to press finish review 👀

[flipbit03]

@erikwrede, I have implemented the changes suggested from conversations and the code review. Looking forward to getting this merged! Thanks in advance.

[erikwrede]

Did you remove the enum support you just added?

[flipbit03]

@erikwrede I tried to add the Enum support but it wasn't possible for me to compare the types properly. The references would never match since we don't have the same Enum registered somewhere (even if you recreate it from a python enum again, they don't match with == :cry:

Idea: Would it be acceptable to compare the Enum in tests by just comparing the invariants? If you agree, then I might try to do it that way.

I am trying to introduce self referential SQLAlchemy model properties and I think I might have a solution.

[erikwrede]

Of course, you can try if you want. But we can also move that to a separate PR/Issue, so we don't block the changes here. I'm not too familiar with the Enum converters in graphene, that's why I would have to look into that myself first. Was the issue a pure test issue, or was there trouble with the implementation as well?

I am trying to introduce self referential SQLAlchemy model properties and I think I might have a solution.

Are you referring to recursive types via hybrid properties?

[flipbit03]

The issue is more in Graphene itself regarding its Enum generation :cry:

graphene.Enum.from_enum(PYTHONENUM) always generates a new object, even if you pass it the same Python Enum. So the == comparison fails on the tests. The only surefire way would be to actually compare the enum's invariants to see if you have the same invariants and values.

I just got self referential SQLAlchemy types working via @hybrid_property type hints \o/

class ShoppingCart(Base):
    ...

    @hybrid_property
    def hybrid_prop_self_referential(self) -> 'ShoppingCart':
        return ShoppingCart(id=1)

    @hybrid_property
    def hybrid_prop_self_referential_list(self) -> List['ShoppingCart']:
        return [ShoppingCart(id=1)]

I used the fact that Graphene allows you to pass a Callable instead of a type and it'll call that callable later in the future. This is how it's implemented.

@convert_sqlalchemy_hybrid_property_type.register(safe_isinstance(typing.ForwardRef))
def convert_sqlalchemy_hybrid_property_forwardref(arg):
    """
    Generate a lambda that will resolve the type at runtime
    This takes care of self-references
    """

    def forward_reference_solver():
        model = registry_sqlalchemy_model_from_str(arg.__forward_arg__)
        # Always fall back to string if no ForwardRef type found.
        return get_global_registry().get_type_for_model(model) or String

    return forward_reference_solver

I also needed a way to convert bare typing strings into ForwardRefs (things like a : 'FutureType' = ...), so the whole system ties very neatly into itself. The "dispatch" system thing is really paying itself since we can just plug things into each other and it works correctly.

@convert_sqlalchemy_hybrid_property_type.register(safe_isinstance(str))
def convert_sqlalchemy_hybrid_property_forwardref(arg):
    """
    Convert Bare String to a ForwardRef
    """

    return convert_sqlalchemy_hybrid_property_type(typing.ForwardRef(arg))

[erikwrede]

Looks good! Speaking about forward references, wouldn't these also be necessary when handling cyclic hybrid properties? E.g. A has a hybrid property referencing B which has a hybrid property referencing A.

Speaking of enums, there might have been issues on the graphene repo about that. I'll have a look later. If not, it might make sense to open an issue for enhancement there.

[flipbit03]

I'm testing right now If we are covered in the A -> B -> A case. But if we aren't, I think we could just coalesce to String in those cases.

EDIT: Seems to be working fine. Writing tests.

[erikwrede]

Great. I think this would be a good point to consider basic feature integration done.

enums are a different discussion. Maybe we should move that to a different issue/PR to keep readability? This discussion has gotten quite long and we agreed on new features in the middle.

[flipbit03]

Absolutely agree that it should be another feature/discussion. I am submitting my PR in its current state for your review/approval. Thank you for your time.

PS: Do you hang around in some Discord server or something?

[flipbit03]

@erikwrede I cannot see the button to 'resubmit for review' in Github, since it's still waiting for your (last) review (sorry), so I'm tagging you here. It is ready to be reviewed now and I feel satisfied with the level of support we achieved.

Thank you!

[erikwrede]

Thanks! I'll review it later. Regarding Discord: I'm not active on any Dev-Related Servers, but feel free to reach me on the graphene slack :)

https://join.slack.com/t/graphenetools/shared_invite/enQtOTE2MDQ1NTg4MDM1LTA4Nzk0MGU0NGEwNzUxZGNjNDQ4ZjAwNDJjMjY0OGE1ZDgxZTg4YjM2ZTc4MjE2ZTAzZjE2ZThhZTQzZTkyMmM