graphql-python / graphene-sqlalchemy

Graphene SQLAlchemy integration
http://docs.graphene-python.org/projects/sqlalchemy/en/latest/
MIT License
974 stars 225 forks source link

refactor!: use the same conversion system for hybrids and columns #371

Closed erikwrede closed 1 year ago

erikwrede commented 1 year ago

This PR refactors the type conversion system to use the same converters for all column types (column + hybrid). Previously, we had convert_sqlalchemy_column using singledispatch and convert_sqlalchemy_hybrid_property_type using a custom singledispatch-like solution to match column and hybrid property type(-hints) to their graphene equivalents. This implied having to maintain two separate type conversion systems with very similar functionality. This PR proposes merging the systems based on our custom singledispatch-style approach. This has several implications:

Breaking Change: convert_sqlalchemy_type now uses a matcher function

Our custom matcher needs to be able to check more than just the type of a column. Some type hints like unions or optionals need further processing. To convert old singledispatch-style converters to the new converters, the column type has to be wrapped in our custom value_equals helper function:

# Before 
@convert_sqlalchemy_type.register(JSON)

# After
@convert_sqlalchemy_type.register(value_equals(JSON))

Breaking Change: convert_sqlalchemy_type support for subtypes is dropped, each column type must be explicitly registered

Since we now use matcher functions, it doesn't make sense to check if any inherited types match the singledispatch converters, as they might interfere with other matchers. Because of that, all types must now be explicitly registered:

@convert_sqlalchemy_type.register(value_equals(MYColumnType))
def convert_my_type(column_type, **kwargs):
   return MyGrapheneType

See converter.py for more detailed examples.

Breaking Change: The first argument to converter functions is always a type

Previously, both type and instances of types were passed to the converter. (String vs String(30)). If you need information from the instance, use column.type instead. Remember to check if column is present and not a hybrid_property

Example:

@convert_sqlalchemy_type.register(column_type_eq(sqa_types.ARRAY))
@convert_sqlalchemy_type.register(column_type_eq(postgresql.ARRAY))
def convert_array_to_list(
    type_arg: Any,
    column: Optional[Union[MapperProperty, hybrid_property]] = None,
    registry: Registry = None,
    **kwargs,
):
    if column is None or isinstance(column, hybrid_property):
        raise Exception("SQL-Array conversion requires a column")
    item_type = column.type.item_type
    if not isinstance(item_type, type):
        item_type = type(item_type)
    inner_type = convert_sqlalchemy_type(
        item_type, column=column, registry=registry, **kwargs
    )
    return graphene.List(
        init_array_list_recursive(inner_type, (column.type.dimensions or 1) - 1)
    )

Breaking Change: convert_sqlalchemy_type's column and registry arguments must now be keyword arguments

To make additional support for type conversions possible, we converted all arguments to the converters to keyword arguments. It is not guaranteed for these to be present, so you should always check for existance of column or registry if you plan on using them. Since other arguments might be provided as well, make sure that **kwargs is present on your signature and that it is passed to any calls of nested type conversions (e.g. List[JSON] calls convert_list, then convert_list starts another conversion for the inner type)

Breaking Change: The hybrid property default column type is no longer a string. If no matching column type was found, an exception will be raised.

We introduced warnings for the string fallback in 3.0.0b2 as a soft transition to the new system. Since we merged both conversion systems, it is impractical to have a fallback for all types. If you want to keep the old string types for your hybrids, please use:

class MyType(SQLAlchemyObjectType):
    class Meta:
        model = MyModelWithHybrid
    myHybrid = ORMField(type_=graphene.String)
codecov[bot] commented 1 year ago

Codecov Report

Base: 96.24% // Head: 96.34% // Increases project coverage by +0.09% :tada:

Coverage data is based on head (3923322) compared to base (a03e74d). Patch coverage: 96.71% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #371 +/- ## ========================================== + Coverage 96.24% 96.34% +0.09% ========================================== Files 9 9 Lines 879 903 +24 ========================================== + Hits 846 870 +24 Misses 33 33 ``` | [Impacted Files](https://codecov.io/gh/graphql-python/graphene-sqlalchemy/pull/371?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python) | Coverage Δ | | |---|---|---| | [graphene\_sqlalchemy/converter.py](https://codecov.io/gh/graphql-python/graphene-sqlalchemy/pull/371/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python#diff-Z3JhcGhlbmVfc3FsYWxjaGVteS9jb252ZXJ0ZXIucHk=) | `95.73% <96.32%> (-0.53%)` | :arrow_down: | | [graphene\_sqlalchemy/registry.py](https://codecov.io/gh/graphql-python/graphene-sqlalchemy/pull/371/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python#diff-Z3JhcGhlbmVfc3FsYWxjaGVteS9yZWdpc3RyeS5weQ==) | `100.00% <100.00%> (ø)` | | | [graphene\_sqlalchemy/utils.py](https://codecov.io/gh/graphql-python/graphene-sqlalchemy/pull/371/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python#diff-Z3JhcGhlbmVfc3FsYWxjaGVteS91dGlscy5weQ==) | `95.86% <100.00%> (+2.00%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=graphql-python)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

erikwrede commented 1 year ago

@sabard All fixed up, feel free to merge if nothing else comes up :)

guillaumep commented 9 months ago

I've had regression in my code after upgrading graphene-sqlalchemy and it just by reading the changes one by one that I saw these breaking changes. I'd like to suggest such breaking changes to be indicated in the release notes otherwise they are hard to find.

erikwrede commented 9 months ago

@guillaumep sorry about that, it seems like the release notes for this release were not published properly. We will fix that up soon. As a tip: in the change list, we will use semantic commits in the future, and in the notes of this release, this PR was already marked with a !, which suggests a breaking change. Of course I acknowledge that this should not be the only information, and as you've seen from the other release notes, we usually publish more detailed information. For the 3.0 full release, there will be a full upgrade guide.

Apart from that, how is this change working for you? We haven't received too much feedback yet, so I'd love to hear your thoughts.

guillaumep commented 9 months ago

sorry about that, it seems like the release notes for this release were not published properly. We will fix that up soon.

Don't worry about it, and thanks for listening to my feedback.

As a tip: in the change list, we will use semantic commits in the future, and in the notes of this release, this PR was already marked with a !, which suggests a breaking change.

I was not completely aware of this convention in open source projects. I'll pay more attention to it starting now.

For the 3.0 full release, there will be a full upgrade guide.

Great!

Apart from that, how is this change working for you? We haven't received too much feedback yet, so I'd love to hear your thoughts.

I upgraded from 3.0.0b3 to 3.0.0b4. Once I figured out how to change my code everything worked pretty well. We register a custom multi lingual dictionary SQLAlchemy type, which serializes data to a string in the database. We are providing the data as an object to the API clients. Here's the code diff for the upgrade:

from graphene_sqlalchemy.converter import convert_sqlalchemy_type
+from graphene_sqlalchemy.utils import column_type_eq

-@convert_sqlalchemy_type.register(ModelMlText)
+@convert_sqlalchemy_type.register(column_type_eq(ModelMlText))
def convert_mltext_to_json(type, column, registry=None):
    return MlText

The key here was to figure-out the use of column_type_eq. I read graphene-sqlalchemy's source code to see existing examples to figure this one out.

Thanks for the ongoing work on the project!