Improve auto creation of Graphene Enums. #98

Cito · 2017-12-01T11:43:49Z

This PR is an improvement of PR #78.

The automatically created Graphene Enums are now registered in order to make them reusable. Different SQLAlchemy Enums with the same items are mapped to the same Graphene Enum Types if their names and items match.

When Graphene Enums are automatically created, the type names and the names of the options are mangled to match the GraphQL conventions (CamelCase type names and UPPERCASE options).

coveralls · 2017-12-01T11:44:55Z

Coverage decreased (-0.6%) to 90.415% when pulling d8c39a5 on Cito:convert_enums into 4827ce2 on graphql-python:master.

coveralls · 2017-12-01T11:54:46Z

Coverage decreased (-0.6%) to 90.415% when pulling baa454d on Cito:convert_enums into 4827ce2 on graphql-python:master.

coveralls · 2017-12-01T11:54:46Z

Coverage decreased (-0.6%) to 90.415% when pulling baa454d on Cito:convert_enums into 4827ce2 on graphql-python:master.

coveralls · 2017-12-01T12:23:06Z

Coverage increased (+0.7%) to 91.64% when pulling 76b32ff on Cito:convert_enums into 4827ce2 on graphql-python:master.

coveralls · 2017-12-01T12:23:06Z

Coverage increased (+0.3%) to 93.224% when pulling 361b44c on Cito:convert_enums into ef1fce2 on graphql-python:master.

coveralls · 2017-12-01T12:23:06Z

Coverage increased (+0.7%) to 91.64% when pulling 76b32ff on Cito:convert_enums into 4827ce2 on graphql-python:master.

coveralls · 2017-12-01T12:23:06Z

Coverage increased (+0.7%) to 91.64% when pulling 76b32ff on Cito:convert_enums into 4827ce2 on graphql-python:master.

Fedalto · 2017-12-08T20:00:08Z

I think the enum should not be modified here (CamelCase type names and UPPERCASE options). Because it would be inconsistent with what graphene.Enum does.
Imagine that I have a python MyEnum, and use it in a column with sqlalchemy.Enum(MyEnum) and then I want a mutation that received this value.
I cannot do
value = graphene.Argument(graphene.Enum.from_enum(MyEnum)) because this is generating a different enum than the one based on the sqlalchemy column.

Cito · 2017-12-10T10:51:21Z

@Fedalto: Thanks for the feedback. We really need discussion, since this is complicated.

First, just to clarify: The CamelCase conversion is done to the enum type name, the UPPERCASE conversion happens to the enum key names. Graphene converts method names, but not type names, because Python has already the right name convention for these. However, databases usually have lower case type names for their enum types. Like in this example:

CREATE TYPE user_mood AS ENUM ('sad', 'ok', 'happy');

In SQLAlchemy you would write this type as

sqlalchemy.Enum('sad', 'ok', 'happy', name='user_mood')

It makes sense to convert this to a Graphene GraphQL Enum Type with he name UserMood and keys SAD, OK and HAPPY because that's what you expect in GraphQL, and on the other hand you don't want to have camelcase type and uppercase value names in the database. Graphene is not in the business of converting database types, so I don't think we can speak of an inconsistency here.

The complexity comes from the fact that we are dealing with 4 different kinds of enums: The actual database enum type (which might be a native type or just simulated with a check constraint), the SQLAlchemy Enum type that reflects it, the Python Enum that might (or might not) be underlying that SQLAlchemy Enum, and finally the graphene Enum GraphQL type.

Note that the SQLAlchemy Enum type must not necessarily be based off a Python Enum. You can define a SQLAlchemy Enum simply like in the example above, without using a Python Enum, or you can use a Python Enum like this:

PyUserMoodType = enum.Enum('UserMood', 'sad ok happy')
SaUserMoodType = sqlalchemy.Enum(PyUserMoodType)

In this case, the type name would be converted to usermood on the database.

My point is, when using graphene_sqlalchemy, we must use the SQLAlchemy Enum type as the source for converting to GraphQL, not the Python Enum type, because we might not even have one and because graphene_sqlalchemy operates one level higher in the abstraction.

The typical example is, like in your case, you have a property user_mood in an SQLAlchemy base class User like this:

UserMood = sqlalchemy.Enum('sad', 'ok', 'happy', name='user_mood')

class User(Base):
    __tablename__ = 'users'

    id = Column(Integer, primary_key=True)
    user_name = Column(String)
    user_mood = Column(UserMood)

class UserType(SQLAlchemyObjectType):
    class Meta:
        model = User

The question is how we can use the automatically created graphene enum type for the user mood in Arguments (like a filter in a query or in a mutation). We can't just redefine it with graphene.Enum because, as you noticed we then would have two different enum types. My solution was using the registry - that already exists in graphene_sqlalchemy - to not only store the types for sqlalchemy object types, but also for sqlalchemy enums. So we could do this:

from graphene_sqlalchemy.registry import registry

users = List(UserType, kind=Argument(registry.get_type_for_enum(UserMood)))

This would use the exact same GraphQL type for the argument as it is used on the column.

Alternatively, if you want to avoid using the registry, you can also do this:

users = List(UserType, kind=Argument(UserType._meta.fields['user_mood'].type))

I know this is a bit ugly (particularly because it is accessing a "private" attribute), but still readable. The expression simply gets the (GraphQL) type of the 'user_mood' field of the UserType GraphQL type.

Sorry for this lengthy explanation, but as I said this matter is complicated because of all the layers of abstraction and "impedence mismatch" on all the layers.

Fedalto · 2017-12-11T17:02:31Z

@Cito, thanks for the explanation.

Graphene converts method names, but not type names, because Python has already the right name convention for these.

I think I have a problem with this assumption. I'd not assume that a python enum will always have uppercase keys. So, I guess my problem is that this enum would result in lowercase keys:

graphene.Enum.from_enum(PyEnum('UserMood', 'sad ok happy')

but not when coming from a sqlalchemy enum.

What about the key and name changes be a responsibility of graphene.Enum?
That way, the GraphQL enum will always comply with the convention no matter the enum source of truth.

Cito · 2017-12-11T18:16:16Z

@Fedalto Thanks for the feedback.

It would be nice if we could shift the responsibility for name mangling to Graphene. But we can't do that since Graphene is a lower level interface and therefore does not and should not care. It takes the Enum type names and options exactly like you define them. This allows you to build anything with it. Note that while Graphene does in fact convert field names automatically, you can still override it by explicitly setting names on the fields.

However, Graphene-SQLAlchemy is a higher level interface that seeks to "translate" from SQL to GraphQL in a conventient and automatic fashion. Therefore, in my view, it should - at least by default - also translate the naming conventions, which are lowercase enum options and type names in SQL, but uppercase enum options and camel case type names in GraphQL. That way you get what you normally expect in each of the two worlds instead of forcing an alien naming convention to the other world.

Note that Graphene-SQLAlchemy also converts field names from snake_case to camelCase using the default conversion mechanism in Graphene, but contrary to when you use Graphene manually, you can't change that behavior - since the fields are created automatically, you have no chance to override the name parameter in that automatic process.

Of course, it would be nice if we could make the automatic conversion configurable on the level of the registry, class or field. On the class level, maybe we could use the Meta attribute, like this?

class UserType(SQLAlchemyObjectType):
    class Meta:
        model = User
        translate_field_names = True
        translate_enum_options = False
        translate_type_names = True

kavink · 2018-02-13T23:17:10Z

Any thoughts on if this is going to be merged or how should one handle enum's ?

kavink · 2018-02-19T20:38:54Z

@Cito @Fedalto @syrusakbary Any thoughts or recommendations on how one should proceed with implementing Enums as Graphene enums ?

Lelby · 2018-02-26T10:00:44Z

I tried to use these auto-created enums with #87, and it fails. Is it supposed to be working though?

I am really looking forward to be able to use graphene-sqlalchemy fully, but with current 'Enums problems' and 'manually-generated mutations' it is a bummer :(

UPD By 'it fails' I mean the following error: ValueError: Expected XXX_SOME_FIELD_XXX to be Argument, but received Field. Try using Argument(XXX_MY_ENUM_NAME_XXX).

arlobryer · 2018-05-22T12:17:41Z

Hi, I'd love to know if this can be merged - @Cito, @Fedalto, @syrusakbary, any further thoughts from your perspective?

Cito · 2018-06-03T18:21:25Z

No further thoughts from my side. Would also love to see graphene-sqlalchemy moving forward.

wichert

I'm looking at this PR a bit since I am also trying to use Enums with graphene_sqlalchemy. I just submitted #154 which probably conflicts with PR since it makes changes to the same test.

One things I am missing is this PR are some tests that prove it works with enum.Enum / PyEnum based enums as well; they tend to behave subtly differently so it would be good to have some coverage for those as well.

I'm a bit conflicted about the naming discussion: I buy the argument that following GraphQL standards is a good thing, and that graphene_sqlalchemy is the right place. I can see that it may be important to opt out of the renaming, and a meta attribute would be a good way to do that.

wichert · 2018-08-03T12:24:11Z

graphene_sqlalchemy/converter.py

                 description=get_column_doc(column),
                 required=not(is_column_nullable(column)))


 @convert_sqlalchemy_type.register(ChoiceType)
-def convert_column_to_enum(type, column, registry=None):
+def convert_choice_to_enum(type, column, registry=None):


Why this rename? You are still converting a column here.

The functions must have specific names, because columns with ChoiceType and Enum both convert to Enum. The function name would then be convert_column_to_enum for both. Actually the other functions should also have more specific names, i.e. contain not only the target type, but also the source type.

wichert · 2018-08-03T12:25:56Z

graphene_sqlalchemy/registry.py

+                name = to_type_name(name)
+            else:
+                name = 'Enum{}'.format(len(self._registry_enums) + 1)
+            items = [(key.upper(), key) for key in sql_type.enums]


You are doing normalisation here in two places: lines 44 and 52. Wouldn't it be better to do that outside of the if here, to make sure you don't accidentally start using different rules when you make a later change?

Good point. I changed that and moved the conversion of enum value names into a separate function.

…qlalchemy

jnak · 2019-04-10T01:22:01Z

Hello everyone,

I'm just catching up with this PR. Since everyone seems to agree that our current Enum situation is not ideal, it's time to make some progress on it.

From what I understand, the main contention point is around the automatic Enum values mangling. While I'm not a big fan of the automatic name mangling, I think it's fine in this case because it is consistent with the rest of graphene-sqlalchemy. Medium-term we should definitely figure out a way to opt-out of this. But I don't think we should block this PR for this because we can already manually override that behavior if we really need to by opting out of the automatic field generation:

class UserType(SQLAlchemyObjectType):
    class Meta:
        model = User
       exclude_fields = ('user_mood',)
    
   user_mood = graphene.Enum(UserMood)
   
  def resolve_user_mood(root, info, **args):
     ...

Let's plan to figure out how to cleanly opt-out of this Enum name mangling as part of a larger PR on how to selectively opt-out of all the name mangling done by graphene-sqlalchemy. There is already one PR addressing this: #178

@Cito Would you be interested in fixing the merge conflicts in the next few days? If not let me know and I'll take care of it.

Cheers,
J

Cito · 2019-04-10T19:47:18Z

@jnak - yes, this should move forward. I'look into this again in the next days..

This also avoids problems when test tables are changed, in which case we would need to drop and recreate them.

The created Graphene Enums are now registered and reused, because their names must be unique in a GraphQL schema. Also the naming conventions for Enum type names (CamelCase) and options (UPPER_CASE) are applied when creating them.

Cito · 2019-04-11T23:40:26Z

@jnak - just pushed anew with some changes and based on the lastest master.

However, I noticed that meanwhile a function has been added to create special enums for sorting. That function caches the enums in a separate cache, not in the registry like my PR does.

I want to change that so that the sorting enums use the same caching mechanism in the registry. Also, I want to change the default function for creating the sort enum value names so that they also use uppercase. Will push another commit tomorrow with these changes.

jnak · 2019-04-12T15:48:36Z

@Cito Great. Let me know when it's ready for me to review

Slightly changed the API: Creating and getting sort enums are different functions now. When no sort enum is created, a default one will be created automatically. Sort enums now also use upper case symbols by default.

Cito · 2019-04-12T17:07:34Z

@jnak - I'm done with this now.

I have changed the API slightly by making the creation of sort enums a separate function. They are created automatically when requested, so you normally don't need manual creation. The old functions are still available with deprecation warnings.

jnak

Looks great overall. Had a few questions. Thanks!

graphene_sqlalchemy/converter.py

jnak · 2019-04-12T21:47:25Z

graphene_sqlalchemy/registry.py

+            members = OrderedDict(
+                (to_enum_value_name(key), key) for key in sql_type.enums
+            )
+        return self.register_enum(name, members)


Why does get_type_for_enum need to register the enum? It seems inconsistent with get_type_for_model and get_sort_params_for_model?

The naming of the methods and division between registry and utils modules could certainly be improved. Need to sleep over it, or we refactor this in a new PR.

I'd prefer to address this in this PR because this may lead to unexpected behaviors. For example, as is we are getting different enums for the same un-named sql enum:

reg = Registry() sa_enum_1 = SQLAlchemyEnum('red', 'blue') assert reg.get_type_for_enum(sa_enum_1) is reg.get_type_for_enum(sa_enum_1) # fails

Ideally a getter should not have any side effects. Why can't we just return the enum from the registry?

self. _registry_enums[enum]

See below, I'll try to come up with a better solution.

jnak · 2019-04-12T21:50:01Z

graphene_sqlalchemy/registry.py

+        else:
+            self._registry[cls._meta.model] = cls
+
+    def register_enum(self, name, members):


Why does register_enum takes a name instead of SQLAlchemyEnumType? It seems inconsistent with register and register_sort_params

Because not all enums are derived from SQLAlchemyEnumTypes. Some are created for sorting purposes, they don't exist as SQLAlchemyEnumTypes. All of the enums should use the same registry because Enum names in the schema must be unique - you want to get an error when a sort enum accidentally has the same name as an existing enum for a column, and you want to have a way for requesting registered enums by name.

I see. How about we instead have this function take an enum or a column that maps to SQLAlchemyEnumType to keep all that logic encapsulated here? In both cases, we would be able to extract a name.

Regarding name collisions, the names need to be unique across all types (vs just within Enums). graphene already raises an error to prevent this. Why do we need to handle this in graphene-sqlalchemy as well?

Right. I will think about the whole mechanism again and try to create a better-thought-out solution. But maybe I'll do this as a new PR.

jnak · 2019-04-12T22:14:25Z

graphene_sqlalchemy/registry.py

 class Registry(object):
-    def __init__(self):
+    def __init__(self, check_duplicate_registration=False):


What are the legitimate cases where one would want to disable duplicate registration?

The get_type_for_model() method (used by convert_sqlalchemy_relationship()) assumes that there is only one graphene type per model. You may want to make sure that this assumption is met and you haven't accidentally associated two different types with one model.

This case makes sense to me. I was asking about the legitimate cases where you one does not want this function to raise when there are duplicates.

Ah, ok - good question. Not checking was the behavior until now, and test_types would break because CustomCharacter uses the same model as Character. It only matters if you have relationships. Suggestion: Maybe we should always allow duplicate registration, but memorize all of them instead of only the last. And only when get_type_for_model is called, we throw an error which lists the duplicates. I.e. you only get an error, when this is really a problem.

jnak · 2019-04-12T22:18:48Z

graphene_sqlalchemy/registry.py

+            name = (
+                to_type_name(name)
+                if name
+                else "Enum{}".format(len(self._registry_enums) + 1)


Hmm in theory all columns should have a name. I wonder if this can only happen in our tests because SQLAlchemy is not fully initialized.

This is the name of the SQLAlchemy Enum type, not the name of the column. That name may not exist.

I see. What are the benefits of having this function take a SQLAlchemyEnumType instead of a column that maps to a SQLAlchemyEnumType? I can't think of a case where we would have a SQLAlchemyEnumType without its associated column.

You would need to get the SQLAlchemy enum type anyway with column.type. The convert_sqlalchemy_column function already does this. We need the name of the enum type, not the name of the column, because the same enum could be used in different columns.

graphene_sqlalchemy/tests/test_query.py

graphene_sqlalchemy/tests/test_registry.py

jnak · 2019-04-12T22:54:45Z

graphene_sqlalchemy/tests/test_utils.py

-    enum = sort_enum_for_model(Pet)
+def test_to_type_name():
+    assert to_type_name("make_camel_case") == "MakeCamelCase"
+    assert to_type_name("AlreadyCamelCase") == "AlreadyCamelCase"


Maybe add a case where it's a mix of snake_case and CamelCase

graphene_sqlalchemy/tests/test_utils.py

…qlalchemy

Cito · 2019-04-13T11:08:42Z

@jnak Will be busy until next weekend. I suggest we create a different issue for refactoring the API and making it more consistent. We should decide whether this functionality should be implemented in the registry module or utility module, or maybe in a separate module, and whether/which registry functions we want to replicate in the utility module (a bit more convenient when using the global registry). Maybe we should create a special enum module with support functions for handling enums.

jnak

No problem - I'm always offline on weekends. I'm open to have a larger discussion about the API in a separate issue. But I'm hesitant to merge this as is because I'm worried that people or PRs start relying on APIs that will potentially change very soon. I completely understand that after all this time you want this to move forward but I think we should take the time we need to get this right. I'm committed to be very responsive until we get this over to the finish line.

jnak · 2019-04-15T00:30:30Z

graphene_sqlalchemy/registry.py

 class Registry(object):
-    def __init__(self):
+    def __init__(self, check_duplicate_registration=False):


This case makes sense to me. I was asking about the legitimate cases where you one does not want this function to raise when there are duplicates.

graphene_sqlalchemy/tests/test_utils.py

jnak · 2019-04-15T17:30:47Z

graphene_sqlalchemy/registry.py

+            name = (
+                to_type_name(name)
+                if name
+                else "Enum{}".format(len(self._registry_enums) + 1)


I see. What are the benefits of having this function take a SQLAlchemyEnumType instead of a column that maps to a SQLAlchemyEnumType? I can't think of a case where we would have a SQLAlchemyEnumType without its associated column.

jnak · 2019-04-15T17:43:34Z

graphene_sqlalchemy/registry.py

+            members = OrderedDict(
+                (to_enum_value_name(key), key) for key in sql_type.enums
+            )
+        return self.register_enum(name, members)


I'd prefer to address this in this PR because this may lead to unexpected behaviors. For example, as is we are getting different enums for the same un-named sql enum:

reg = Registry() sa_enum_1 = SQLAlchemyEnum('red', 'blue') assert reg.get_type_for_enum(sa_enum_1) is reg.get_type_for_enum(sa_enum_1) # fails

Ideally a getter should not have any side effects. Why can't we just return the enum from the registry?

self. _registry_enums[enum]

jnak · 2019-04-15T19:17:43Z

graphene_sqlalchemy/registry.py

+        else:
+            self._registry[cls._meta.model] = cls
+
+    def register_enum(self, name, members):


I see. How about we instead have this function take an enum or a column that maps to SQLAlchemyEnumType to keep all that logic encapsulated here? In both cases, we would be able to extract a name.

Regarding name collisions, the names need to be unique across all types (vs just within Enums). graphene already raises an error to prevent this. Why do we need to handle this in graphene-sqlalchemy as well?

Cito · 2019-04-18T14:15:20Z

@jnak Let's move the discussion to #208, because this review really gets convoluted. I will then create a new PR based on the outcome of the discussion.

Cito · 2019-04-27T12:29:04Z

This has been superseded by #210 now after discussion in #208.

Cito mentioned this pull request Dec 1, 2017

Proper support for the SQLAlchemy Enum type #78

Merged

arlobryer mentioned this pull request May 22, 2018

When is the next release ? #113

Closed

k4r1 approved these changes May 22, 2018

View reviewed changes

wichert reviewed Aug 3, 2018

View reviewed changes

wichert mentioned this pull request Aug 7, 2018

flask_sqlalchemy example : different types with the same name in the schema: EmployeeConnection, EmployeeConnection. #153

Open

Cito added 2 commits March 31, 2019 12:10

Merge branch 'master' of https://github.com/graphql-python/graphene-s…

67a621f

…qlalchemy

Merge branch 'master' of https://github.com/graphql-python/graphene-s…

9dd34ca

…qlalchemy

Cito added 4 commits April 11, 2019 20:19

Fix converter function name

2f9145c

Use in-memory database to speed up tests

3143397

This also avoids problems when test tables are changed, in which case we would need to drop and recreate them.

Ignore virtualenv created in .venv

8819829

Improve auto creation of Graphene Enums.

c2c4a77

The created Graphene Enums are now registered and reused, because their names must be unique in a GraphQL schema. Also the naming conventions for Enum type names (CamelCase) and options (UPPER_CASE) are applied when creating them.

Cito force-pushed the convert_enums branch from 76b32ff to c2c4a77 Compare April 11, 2019 23:22

jnak mentioned this pull request Apr 12, 2019

Fix enum column conversion by "instantiating" Enum #201

Closed

Register sort enums in the registry

c278a6f

Slightly changed the API: Creating and getting sort enums are different functions now. When no sort enum is created, a default one will be created automatically. Sort enums now also use upper case symbols by default.

Don't cover deprecated functions in tests

ec6d4a2

jnak reviewed Apr 12, 2019

View reviewed changes

Cito added 3 commits April 13, 2019 01:59

Minor changes discussed in review

67d789d

Merge branch 'master' of https://github.com/graphql-python/graphene-s…

082169e

…qlalchemy

Merge master into convert_enums

361b44c

jnak reviewed Apr 15, 2019

View reviewed changes

Cito mentioned this pull request Apr 18, 2019

Discussion: Auto-creation of Graphene Enums #208

Closed

Cito closed this Apr 27, 2019

Cito deleted the convert_enums branch May 5, 2019 17:55

This was referenced Jan 7, 2021

sqlalchemy-utils enum to graphene type double conversion #297

Closed

AssertionError: Found different types with the same name in the schema #211

Open

erikwrede mentioned this pull request May 15, 2022

How can I reference an enum type created by SqlAlchemy mapping ? #96

Closed

Improve auto creation of Graphene Enums. #98

Improve auto creation of Graphene Enums. #98

Conversation

Cito commented Dec 1, 2017

coveralls commented Dec 1, 2017 • edited Loading

coveralls commented Dec 1, 2017

coveralls commented Dec 1, 2017 • edited Loading

coveralls commented Dec 1, 2017

coveralls commented Dec 1, 2017 • edited Loading

coveralls commented Dec 1, 2017

coveralls commented Dec 1, 2017

Fedalto commented Dec 8, 2017

Cito commented Dec 10, 2017 • edited Loading

Fedalto commented Dec 11, 2017

Cito commented Dec 11, 2017

kavink commented Feb 13, 2018

kavink commented Feb 19, 2018 • edited Loading

Lelby commented Feb 26, 2018 • edited Loading

arlobryer commented May 22, 2018

Cito commented Jun 3, 2018

wichert 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

jnak commented Apr 10, 2019 • edited Loading

Cito commented Apr 10, 2019

Cito commented Apr 11, 2019

jnak commented Apr 12, 2019

Cito commented Apr 12, 2019

jnak 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

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

Cito commented Apr 13, 2019 • edited Loading

jnak 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

Cito commented Apr 18, 2019

Cito commented Apr 27, 2019

coveralls commented Dec 1, 2017 •

edited

Loading

coveralls commented Dec 1, 2017 •

edited

Loading

coveralls commented Dec 1, 2017 •

edited

Loading

Cito commented Dec 10, 2017 •

edited

Loading

kavink commented Feb 19, 2018 •

edited

Loading

Lelby commented Feb 26, 2018 •

edited

Loading

jnak commented Apr 10, 2019 •

edited

Loading

Cito commented Apr 13, 2019 •

edited

Loading