feat: added object_type_name_prefix #1466

superlevure · 2022-10-14T13:22:14Z

This MR adds the ability to pass object_type_name_prefix: string to Schema, the prefix will be prepended to:

Queries name
Mutations name
Subscription name
Types name

Fields are left unmodified.

This is useful in micro-services architecture to add the service's name in the Schema.

With

schema = Schema(
    Query,
    Mutation,
    Subscription,
    auto_camelcase=True,
    object_type_name_prefix="prefix",
)

The schema:

type Query {
    inner: MyOtherType
}

type MyOtherType {
    field: String
    myType: MyType
}

type MyType {
    field: String
}

type Mutation {
    createUser(name: String): CreateUser
}

type CreateUser {
    name: String
}

type Subscription {
    countToTen: Int
}

Becomes

schema {
    query: PrefixQuery
    mutation: PrefixMutation
    subscription: PrefixSubscription
}

type PrefixQuery {
    prefixInner: PrefixMyOtherType
}

type PrefixMyOtherType {
    field: String
    myType: PrefixMyType
}

type PrefixMyType {
    field: String
}

type PrefixMutation {
    prefixCreateUser(name: String): PrefixCreateUser
}

type PrefixCreateUser {
    name: String
}

type PrefixSubscription {
    prefixCountToTen: Int
}

codecov · 2022-10-14T13:37:43Z

Codecov Report

Base: 95.87% // Head: 95.94% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (0ea4636) compared to base (6969023).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1466      +/-   ##
==========================================
+ Coverage   95.87%   95.94%   +0.07%     
==========================================
  Files          50       50              
  Lines        1722     1753      +31     
==========================================
+ Hits         1651     1682      +31     
  Misses         71       71

Impacted Files	Coverage Δ
graphene/types/base.py	`96.96% <100.00%> (+0.09%)`	⬆️
graphene/types/schema.py	`95.73% <100.00%> (+0.41%)`	⬆️
graphene/types/utils.py	`89.18% <100.00%> (+1.68%)`	⬆️
graphene/types/enum.py	`100.00% <0.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.

erikwrede · 2022-10-22T21:42:12Z

Thanks for the PR, this looks helpful!
Will test it out locally tomorrow.

erikwrede · 2022-10-23T17:27:50Z

Quick comment which is not a full review: This also prefixes the root type names. I'm wondering if this is really desirable, as Query, Mutation and Subscription types are basically the standard/usual names. The prefixes will cause Docs to deviate from the usual pattern. Maybe a way to edit this behavior would be useful.

My intuition is that excluding root types from prefixing actually helps with schema stitching and general readability of a schema. Happy to have my mind changed on this one though, I'm open to discuss it if you have any compelling reasons! 🙂

erikwrede

After testing, I can conclude that this is indeed useful to have. I have small comments (including my comment that is not part of the review):

Is there a reason we are only prefixing ObjectTypes, but excluding things like Enums, I terfaces and Unions? Especially in a Microservice scenario like the one you mentioned these would most likely also be specific to the Microservice. The parameter name sort of specifies that the prefix is only applicable to ObjectTypes, but I could imagine this restriction to be confusing for other devs.
Can we add a test case for the Field("Typename-As-import-String") notation? Just want to make sure future changes to that syntax aren't broken by prefixes. This might be the case if we ever decide to allow Field("Typename") using the schema as a registry instead of import strings.

Looking forward to get this integrated soon! Maybe @tcleonard has some additional points.

superlevure · 2022-10-24T10:09:16Z

Hi @erikwrede

Thanks for the review! I'm glad you think that it could be useful to Graphene.

Quick comment which is not a full review: This also prefixes the root type names. I'm wondering if this is really desirable, as Query, Mutation and Subscription types are basically the standard/usual names. The prefixes will cause Docs to deviate from the usual pattern. Maybe a way to edit this behavior would be useful.

My intuition is that excluding root types from prefixing actually helps with schema stitching and general readability of a schema. Happy to have my mind changed on this one though, I'm open to discuss it if you have any compelling reasons! 🙂

Absolutely, I was not sure about this one but I agree. I'll push a fix

Is there a reason we are only prefixing ObjectTypes, but excluding things like Enums, I terfaces and Unions? Especially in a Microservice scenario like the one you mentioned these would most likely also be specific to the Microservice. The parameter name sort of specifies that the prefix is only applicable to ObjectTypes, but I could imagine this restriction to be confusing for other devs.

I missed that indeed, we should apply the prefix to those too.

Can we add a test case for the Field("Typename-As-import-String") notation? Just want to make sure future changes to that syntax aren't broken by prefixes. This might be the case if we ever decide to allow Field("Typename") using the schema as a registry instead of import strings.

Sure thing!

superlevure · 2022-10-24T16:41:37Z

I've updated the PR to address your comments @erikwrede:

Root type names are left untouched
Prefix is now added to the name of:
- ObjectType
- InputType
- Interface
- Union
- Enum
- Query / Mutation / Subscription
Added a test for the Field("Typename-As-import-String") notation
Updated documentation

Let me know what you think :)

docs/requirements.txt

graphene/types/schema.py

superlevure · 2022-10-24T16:31:00Z

graphene/types/schema.py

@@ -391,6 +411,23 @@ def resolve_type(self, resolve_type_func, type_name, root, info, _type):
        return_type = self[type_name]
        return default_type_resolver(root, info, return_type)

+    def add_prefix_to_type_name(self, name):
+        if self.type_name_prefix:
+            return self.type_name_prefix[0].upper() + self.type_name_prefix[1:] + name


This is to go from camelCase to PascalCase as per convention for type names.

Ideally we'd need a auto_pascalcase argument for Schema to control this like we do with auto_camelcase.

It feels a bit overkill though, any thoughts on this?

Suggested change

return self.type_name_prefix[0].upper() + self.type_name_prefix[1:] + name

return self.type_name_prefix.capitalize() + name

I did that at first actually, but:

mmmm that's a fair point! (that also means that if we have auto camel case on and we have my_fieldIsGreat = Field(...) it would become myFieldisgreat because it does use .capitalize()... should we change that @erikwrede in your opinion?)

@tcleonard great catch! I'm not sure about changing that since my_fieldIsGreat is invalid snake case and the contract for auto_camelcase is to transform valid snake_case to PascalCase. So the specified behavior is currently 'undefined'. Would definitely be a breaking change, so we should ask for more opinions on Slack.

For prefixes, we should expect the use of proper PascalCase.

graphene/types/schema.py

tcleonard · 2022-10-25T12:03:39Z

graphene/types/schema.py

@@ -391,6 +411,23 @@ def resolve_type(self, resolve_type_func, type_name, root, info, _type):
        return_type = self[type_name]
        return default_type_resolver(root, info, return_type)

+    def add_prefix_to_type_name(self, name):
+        if self.type_name_prefix:
+            return self.type_name_prefix[0].upper() + self.type_name_prefix[1:] + name


Suggested change

return self.type_name_prefix[0].upper() + self.type_name_prefix[1:] + name

return self.type_name_prefix.capitalize() + name

graphene/types/schema.py

tcleonard · 2022-10-25T12:16:57Z

graphene/types/schema.py

+            if field.name:
+                field_name = field.name
+            else:
+                if graphene_type._meta.name in self.root_type_names:
+                    field_name = self.add_prefix_to_field_name(name)
+                else:
+                    field_name = self.get_name(name)


I'm not completely sure but I think that even if the name comes from field.name we want to add the prefix... would be good to add a unit test for this.

Suggested change

if field.name:

field_name = field.name

else:

if graphene_type._meta.name in self.root_type_names:

field_name = self.add_prefix_to_field_name(name)

else:

field_name = self.get_name(name)

field_name = field.name or self.get_name(name)

if graphene_type._meta.name in self.root_type_names:

field_name = self.add_prefix_to_field_name(name)

Looking again at the code I see that we bypass auto_camelcase when the field name is explicitly defined, shouldn't we expect the same for type_name_prefix ?

I'm not sure neither to be honest

yeah I saw that we bypass the auto camel case but I am not sure it makes sense to remove the prefixing... 🤷‍♂️

Should this really be part of type_name_prefix, or should we rather add a meta option field_name_prefix to ObjectType? Covers more use cases than this.

The drawback is having to configure the same prefix in different places. But we are more exact as the type_name_prefix only applies to type names, not to fields.

If we go for field_name_prefix, I agree with @tcleonard to add the prefix to all field names.
If we go for type_name_prefixes I am not sure either.

tcleonard · 2022-10-25T12:30:12Z

graphene/types/schema.py

+    def add_prefix_to_field_name(self, name):
+        if self.type_name_prefix:
+            if self.auto_camelcase:
+                return self.get_name(
+                    self.type_name_prefix[0].lower()
+                    + self.type_name_prefix[1:]
+                    + "_"
+                    + name
+                )
+            return self.get_name(self.type_name_prefix + name)
+        return self.get_name(name)


I don't think there is a need to deal with the auto_camelcase case here as it is already dealt with in the get_name.

Suggested change

def add_prefix_to_field_name(self, name):

if self.type_name_prefix:

if self.auto_camelcase:

return self.get_name(

self.type_name_prefix[0].lower()

+ self.type_name_prefix[1:]

+ "_"

+ name

)

return self.get_name(self.type_name_prefix + name)

return self.get_name(name)

def add_prefix_to_field_name(self, name):

if self.type_name_prefix:

return self.get_name(

self.type_name_prefix[0].lower()

+ self.type_name_prefix[1:]

+ "_"

+ name

)

return self.get_name(name)

also note that in your current implementation where you don't lower the first character in the case we are not in auto camel case means the output is not exactly what you give in the doc:

type Query { myPrefixInner: MyPrefixMyType }

indeed that would be MyPrefixInner...

indeed that would be MyPrefixInner...

By default auto_camelcase=True so the query name would be myPrefixInner indeed isn't it?

yes but my point was if auto_camelcase is:

True -> myPrefixInner

False -> MyPrefixInner
which doesn't seem very intuitive... no?

I'm confused, what would be more intuitive in your view?

Because

MyPrefixInner (PascalCase) -> auto_camelcase=True -> myPrefixInner (camelCase)

MyPrefixInner (PascalCase) -> auto_camelcase=False -> MyPrefixInner (PascalCase)

Feels the most natural to me 🤔

Gentle ping on that one @tcleonard

superlevure · 2022-10-26T08:05:26Z

I've pushed some changes to add the ability to override type_name_prefix for specific types @tcleonard @erikwrede

I realized that this is needed when defining external GraphQL Types in a federated Schema to avoid ending up with ServiceAServiceBTypeC

Let me know what you think :)

erikwrede · 2022-12-23T07:00:01Z

@superlevure will test this in combination with some graphene federation v2.0 support reviews (graphql-python/graphene-federation#4 ) over the weekend, because that's probably the main use case for this feature. Going to make sure we haven't created any incompatibilities by accident.

superlevure · 2022-12-23T16:38:05Z

@superlevure will test this in combination with some graphene federation v2.0 support reviews (graphql-python/graphene-federation#4 ) over the weekend, because that's probably the main use case for this feature. Going to make sure we haven't created any incompatibilities by accident.

Thank you! 🙏

Let me know if I can do anything to help

erikwrede · 2022-12-23T16:42:21Z

@superlevure if you have experience using federation, then trying out the PR I mentioned would definitely be very useful 😊

erikwrede

As promised, I played around with this feature and graphene-federation. The basics (@key) are working if you manually disable the prefix for those types.
More advanced directives such as inaccessible cause problems since their resolving relies on getting the graphql-core type from the schema by schema.graphql_schema.get_type(entity._meta.name). Since _meta.name doesn't include the prefix, get_type returns None, breaking the schema generation.
Other extensions of graphene probably use this functionality as well.

Should we try to attach the prefix to schema.get_type in case the first search without the attached prefix fails, or should we let the extensions like -federation handle this?

erikwrede · 2022-12-25T13:53:04Z

graphene/types/utils.py

+def get_type_name(graphene_type, type_name_prefix):
+    type_name_prefix = (
+        graphene_type._meta.type_name_prefix
+        if graphene_type._meta.type_name_prefix is not None
+        else type_name_prefix
+    )
+    if type_name_prefix:
+        return (
+            type_name_prefix[0].upper()
+            + type_name_prefix[1:]
+            + graphene_type._meta.name
+        )
+    return graphene_type._meta.name


It's great that we can override the prefixes for individual types using type_name_prefix. When federating a schema, using @keyon a type we could automatically disable the prefix like that (requiring an extra PR in graphene-federation). For now, this would have to be implemented manually which is also fine.

Personally I dislike disabling the prefix using type_name_prefix = "". It doesn't feel very pythonic to use an empty string to mark something as disabled.
Why do we even need a custom per-object-type prefix? It's kind of verbose to have a Meta option name that can be extended using a prefix just for this single ObjectType. Are there any reasons to not include that custom prefix in the name in the first place?
If there are specific use cases where a separate prefix is needed, it would be helpful to understand these in order to better evaluate the value of this feature. Please let me know if you have any use cases ☺️

One potential alternative could be to use a boolean flag, such as enable_type_name_prefix, with a default value of True, since by default all ObjectTypes use the prefix. This would allow the prefix to be easily enabled or disabled as needed. We could even set the default for this to False in the init_subclass_with_meta for type names Query, Mutation, Subscription so these could still be prefixed if the user wishes to do so regardless of the points made in an older review.

erikwrede · 2022-12-25T14:34:41Z

graphene/types/schema.py

+            if field.name:
+                field_name = field.name
+            else:
+                if graphene_type._meta.name in self.root_type_names:
+                    field_name = self.add_prefix_to_field_name(name)
+                else:
+                    field_name = self.get_name(name)


Should this really be part of type_name_prefix, or should we rather add a meta option field_name_prefix to ObjectType? Covers more use cases than this.

The drawback is having to configure the same prefix in different places. But we are more exact as the type_name_prefix only applies to type names, not to fields.

If we go for field_name_prefix, I agree with @tcleonard to add the prefix to all field names.
If we go for type_name_prefixes I am not sure either.

erikwrede · 2022-12-25T14:45:46Z

graphene/types/schema.py

@@ -391,6 +411,23 @@ def resolve_type(self, resolve_type_func, type_name, root, info, _type):
        return_type = self[type_name]
        return default_type_resolver(root, info, return_type)

+    def add_prefix_to_type_name(self, name):
+        if self.type_name_prefix:
+            return self.type_name_prefix[0].upper() + self.type_name_prefix[1:] + name


@tcleonard great catch! I'm not sure about changing that since my_fieldIsGreat is invalid snake case and the contract for auto_camelcase is to transform valid snake_case to PascalCase. So the specified behavior is currently 'undefined'. Would definitely be a breaking change, so we should ask for more opinions on Slack.

For prefixes, we should expect the use of proper PascalCase.

erikwrede · 2022-12-25T15:03:01Z

graphene/types/schema.py

+    def get_type_name(self, graphene_type):
+        return get_type_name(graphene_type, self.type_name_prefix)
+
+    def get_field_name(self, graphene_type, name):
+        if graphene_type._meta.name in self.root_type_names:
+            # We only add the prefix to the root types and types defined prefixes take precedence
+            # over schema defined prefix.
+            type_name_prefix = (


Both methods should either be defined here or in utils.

Hey @erikwrede sorry I haven't gotten back to you yet, I'm busy at work at the moment but I haven't forgotten about this PR

@superlevure great to hear from u! no worries, feel free to tag me once you get to it 🙂

erikwrede · 2023-07-19T07:10:38Z

Hey @superlevure any updates? Would love to get this merged soon 😊

superlevure · 2023-08-10T16:08:18Z

Hey @superlevure any updates? Would love to get this merged soon 😊

Hey @erikwrede, sorry I haven't had the chance to get back at it yet; it is still on my radar though

feat: added object_type_name_prefix

2dee4f2

fix: style

0633b62

superlevure marked this pull request as ready for review October 21, 2022 09:33

tcleonard requested review from erikwrede and Cito October 21, 2022 09:34

erikwrede reviewed Oct 23, 2022

View reviewed changes

fix: reviews comments

8cb9791

superlevure force-pushed the feat/add-optional-prefix branch from ecd17f5 to 8cb9791 Compare October 24, 2022 16:46

superlevure commented Oct 24, 2022

View reviewed changes

tcleonard reviewed Oct 25, 2022

View reviewed changes

superlevure added 2 commits October 25, 2022 14:28

fix: added Scalar

8423706

fix: review

97dd7e5

superlevure and others added 4 commits October 26, 2022 08:11

fix: style

b0e179f

fix: docs

673a282

Merge branch 'master' into feat/add-optional-prefix

10c53d3

fix: static method

0ea4636

erikwrede reviewed Dec 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added object_type_name_prefix #1466

feat: added object_type_name_prefix #1466

superlevure commented Oct 14, 2022 •

edited

codecov bot commented Oct 14, 2022 •

edited

erikwrede commented Oct 22, 2022

erikwrede commented Oct 23, 2022 •

edited

erikwrede left a comment •

edited

superlevure commented Oct 24, 2022

superlevure commented Oct 24, 2022

superlevure Oct 24, 2022

tcleonard Oct 25, 2022

superlevure Oct 25, 2022

tcleonard Oct 26, 2022

erikwrede Dec 25, 2022

tcleonard Oct 25, 2022

tcleonard Oct 25, 2022

superlevure Oct 25, 2022

tcleonard Oct 26, 2022

erikwrede Dec 25, 2022

tcleonard Oct 25, 2022

superlevure Oct 26, 2022

tcleonard Oct 26, 2022

superlevure Oct 26, 2022 •

edited

superlevure Nov 7, 2022

superlevure commented Oct 26, 2022

erikwrede commented Dec 23, 2022 •

edited

superlevure commented Dec 23, 2022

erikwrede commented Dec 23, 2022 •

edited

erikwrede left a comment

erikwrede Dec 25, 2022

erikwrede Dec 25, 2022

erikwrede Dec 25, 2022

erikwrede Dec 25, 2022

superlevure Mar 5, 2023

erikwrede Mar 6, 2023

erikwrede commented Jul 19, 2023

superlevure commented Aug 10, 2023

	return self.type_name_prefix[0].upper() + self.type_name_prefix[1:] + name
	return self.type_name_prefix.capitalize() + name

feat: added object_type_name_prefix #1466

Are you sure you want to change the base?

feat: added object_type_name_prefix #1466

Conversation

superlevure commented Oct 14, 2022 • edited

codecov bot commented Oct 14, 2022 • edited

Codecov Report

erikwrede commented Oct 22, 2022

erikwrede commented Oct 23, 2022 • edited

erikwrede left a comment • edited

Choose a reason for hiding this comment

superlevure commented Oct 24, 2022

superlevure commented Oct 24, 2022

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

superlevure Oct 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superlevure commented Oct 26, 2022

erikwrede commented Dec 23, 2022 • edited

superlevure commented Dec 23, 2022

erikwrede commented Dec 23, 2022 • edited

erikwrede 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

erikwrede commented Jul 19, 2023

superlevure commented Aug 10, 2023

superlevure commented Oct 14, 2022 •

edited

codecov bot commented Oct 14, 2022 •

edited

erikwrede commented Oct 23, 2022 •

edited

erikwrede left a comment •

edited

superlevure Oct 26, 2022 •

edited

erikwrede commented Dec 23, 2022 •

edited

erikwrede commented Dec 23, 2022 •

edited