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

erikwrede · 2022-12-25T23:19:09Z

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)

fix: insert missing create_type in union conversion Breaking Change: convert_sqlalchemy_type now uses a matcher function Breaking Change: convert_sqlalchemy type's column and registry arguments must now be keyword arguments Breaking Change: convert_sqlalchemy_type support for subtypes is dropped, each column type must be explicitly registered 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. Signed-off-by: Erik Wrede <[email protected]>

erikwrede · 2022-12-25T23:21:23Z

graphene_sqlalchemy/utils.py

+ def register(self, matcher_function: Callable[[Any], bool], func=None):
+ if func is None:
+ return lambda f: self.register(matcher_function, f)
+ self.registry[matcher_function] = func
+ return func


Had to adapt this to functools' SingleDispatch to allow directive chaining

graphene_sqlalchemy/utils.py

graphene_sqlalchemy/tests/models.py

Signed-off-by: Erik Wrede <[email protected]>

codecov · 2022-12-25T23:36:09Z

Codecov Report

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

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

@@            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	Coverage Δ
graphene_sqlalchemy/converter.py	`95.73% <96.32%> (-0.53%)`	⬇️
graphene_sqlalchemy/registry.py	`100.00% <100.00%> (ø)`
graphene_sqlalchemy/utils.py	`95.86% <100.00%> (+2.00%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

flipbit03

LGTM!

Signed-off-by: Erik Wrede <[email protected]>

chore: change the exception types Signed-off-by: Erik Wrede <[email protected]>

Signed-off-by: Erik Wrede <[email protected]>

sabard

nice—much cleaner! looking forward to seeing the rest of the changes.

sabard · 2023-01-03T12:21:17Z

graphene_sqlalchemy/tests/test_converter.py

+ pass
+
+ @hybrid_property
+ def hybrid_prop(self) -> "MyTypeNotInRegistry":


worth testing that this fails for a class type not in the registry as well?

Good point, added that!

sabard · 2023-01-03T12:42:10Z

graphene_sqlalchemy/converter.py

+ # ToDo Move to a separate converter later, matching all sqlalchemy mapped
+ # types and returning a dynamic


until this is done, there will still be the issue with initialization order, right?

Yes, I moved it to a function now but will change it to a dynamic later so we can recognize and wrap it in another dynamic in the filter PR - similar to how we do it with relationship filters.

Ok I realized we can't use Dynamic here, because graphene can't unpack nested dynamics (e.g. graphene.List(graphene.Dynamic))

So we'll need to check if a field type is a function (inspect.isfunction(_type) or isinstance(_type, partial)) additionally to the instanceof dynamiccheck in the filtering PR. This way we still maintain lazy support here.

sabard · 2023-01-03T12:47:27Z

graphene_sqlalchemy/converter.py

+ registry: Registry = None,
+ **kwargs,
+):
+ # fixme drop the primary key processing from here in another pr


what's the preferred way of handling primary_keys?

#102 is relevant here

Exactly what was mentioned in #102 and also the discussion we recently had about graphql-python/graphene#1428
Out of scope for this refactor though, just added the fixme so it's marked in the code

sabard · 2023-01-03T13:02:11Z

graphene_sqlalchemy/converter.py

+@convert_sqlalchemy_type.register(column_type_eq(float))
+@convert_sqlalchemy_type.register(column_type_eq(sqa_types.Float))
+@convert_sqlalchemy_type.register(column_type_eq(sqa_types.Numeric))
+@convert_sqlalchemy_type.register(column_type_eq(sqa_types.BigInteger))


maybe a bit off topic here, but wondering about the choice of BigInteger->Float. assume this would break down after 64 bits. any plans for graphql-core to provide a BigInt type like this?

Good point, let's put this into an issue. Will have a look at it later.

Started #373

Signed-off-by: Erik Wrede <[email protected]>

erikwrede · 2023-01-03T20:01:06Z

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

sabard

lgtm!

see graphql-python/graphene-sqlalchemy#371

guillaumep · 2023-09-26T17:24:07Z

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 · 2023-09-27T20:28:27Z

@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 · 2023-09-28T03:12:47Z

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!

erikwrede commented Dec 25, 2022

View reviewed changes

erikwrede changed the title ~~refactor!: use the same conversion system for hybrids and columns fix: insert missing create_type in union conversion~~ refactor!: use the same conversion system for hybrids and columns Dec 25, 2022

erikwrede added 2 commits December 26, 2022 00:30

fix: catch import error in older sqlalchemy versions

778fbd7

Signed-off-by: Erik Wrede <[email protected]>

fix: union test for 3.10

ea6fbdc

Signed-off-by: Erik Wrede <[email protected]>

flipbit03 previously approved these changes Dec 26, 2022

View reviewed changes

erikwrede added 3 commits January 2, 2023 13:24

fix: use type and value for all columns

233ece6

Signed-off-by: Erik Wrede <[email protected]>

refactor: rename value_equals to column_type_eq

9bf07dc

Signed-off-by: Erik Wrede <[email protected]>

tests: add tests for string fallback removal of hybrid property

524fc8c

chore: change the exception types Signed-off-by: Erik Wrede <[email protected]>

erikwrede dismissed flipbit03’s stale review via 524fc8c January 3, 2023 11:23

erikwrede added 4 commits January 3, 2023 12:37

chore: refactor converter for object types and scalars

c3ea36f

Signed-off-by: Erik Wrede <[email protected]>

chore: remove string fallback from forward references

9e6728a

Signed-off-by: Erik Wrede <[email protected]>

chore: adjust comment

abbeece

Signed-off-by: Erik Wrede <[email protected]>

fix: fix regression on id types from last commit

b68ef91

Signed-off-by: Erik Wrede <[email protected]>

sabard reviewed Jan 3, 2023

View reviewed changes

erikwrede added 2 commits January 3, 2023 20:23

refactor: made registry calls in converters lazy

7e1ab40

Signed-off-by: Erik Wrede <[email protected]>

fix: DeclarativeMeta import path adjusted for sqa<1.4

9756cee

Signed-off-by: Erik Wrede <[email protected]>

erikwrede mentioned this pull request Jan 3, 2023

Investigate BigInt to float conversion for large numbers. #373

Open

sabard approved these changes Jan 3, 2023

View reviewed changes

Merge branch 'master' into refactor-type-conversion

3923322

erikwrede merged commit 2041835 into graphql-python:master Jan 4, 2023

jendrikjoe mentioned this pull request Jan 13, 2023

Fix decimal type conversion #376

Closed

ghost pushed a commit to AsimovBio/graphene-sqlalchemy-filter that referenced this pull request Jan 18, 2023

fix call to convert_sqlalchemy_type to take a type

5123b0a

see graphql-python/graphene-sqlalchemy#371

erikwrede mentioned this pull request Feb 15, 2023

Adding support for Postgres TSTZRANGE #12

Closed

erikwrede mentioned this pull request Dec 4, 2023

hybrid_property annotations with built-in types defaulting to ForwardRef and failing schema compilation #396

Open

This was referenced Dec 19, 2023

RFC: Dependency conflict: fastapi-utils prevents upgrade to sqlachemy 2.0 galaxyproject/galaxy#17185

Closed

Fix usage of graphene-sqlalchemy, bump to 3.0.0rc1 galaxyproject/galaxy#17216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

erikwrede commented Dec 25, 2022 •

edited

Loading

erikwrede Dec 25, 2022 •

edited

Loading

codecov bot commented Dec 25, 2022 •

edited

Loading

flipbit03 left a comment

sabard left a comment

sabard Jan 3, 2023

erikwrede Jan 3, 2023

sabard Jan 3, 2023

erikwrede Jan 3, 2023

erikwrede Jan 3, 2023

sabard Jan 3, 2023

sabard Jan 3, 2023

erikwrede Jan 3, 2023

sabard Jan 3, 2023

erikwrede Jan 3, 2023

erikwrede Jan 3, 2023

erikwrede commented Jan 3, 2023

sabard left a comment

guillaumep commented Sep 26, 2023

erikwrede commented Sep 27, 2023 •

edited

Loading

guillaumep commented Sep 28, 2023

		# ToDo Move to a separate converter later, matching all sqlalchemy mapped
		# types and returning a dynamic

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

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

Conversation

erikwrede commented Dec 25, 2022 • edited Loading

Breaking Change: convert_sqlalchemy_type now uses a matcher function

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

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

Example:

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

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.

erikwrede Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 25, 2022 • edited Loading

Codecov Report

flipbit03 left a comment

Choose a reason for hiding this comment

sabard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikwrede commented Jan 3, 2023

sabard left a comment

Choose a reason for hiding this comment

guillaumep commented Sep 26, 2023

erikwrede commented Sep 27, 2023 • edited Loading

guillaumep commented Sep 28, 2023

erikwrede commented Dec 25, 2022 •

edited

Loading

erikwrede Dec 25, 2022 •

edited

Loading

codecov bot commented Dec 25, 2022 •

edited

Loading

erikwrede commented Sep 27, 2023 •

edited

Loading