-
Notifications
You must be signed in to change notification settings - Fork 819
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A new way to write ObjectType with python3's annotation #729
Comments
Very interesting! |
I really like this @ocavue ! I had no idea you could do this python annotations. I don't know much about annotations actually but I have a couple of questions:
I think keeping the ObjectType and AnnotatedObjectType separate is essential, not just for keeping python 2.7 compatibility, but also supporting both ways of writing ObjectTypes. |
@ekampf @jkimbo Thanks for your like! @jkimbo Here are my answers for your questions. I'm not very familiar with Graphene nor Annotation. So any corrections are welcome.
When
if
The only thing that I can find is that python type can do static check by tools like mypy or pyre. For example: ➜ cat -n test_type.py
1 from typing import List
2
3
4 def process_user(user_ids: List[int]):
5 pass
6
7
8 process_user([1, 2, 3])
9 process_user([4, 5, "NOT_INT"])
➜ mypy .
test_type.py:9: error: List item 2 has incompatible type "str"; expected "int" But I don't think it's a big deal because of two reasons:
Agree! A downward compatibility API is very important. We all know the story about python 2.7 😂. |
Thanks for the answers.
I meant could you do something like this: class Heroine(AnnotationObjectType):
def best_friend(root, info) -> Person:
if only_first_name:
return [self._first_name]
return [self._first_name, self._last_name] where Also I thought of another question: How would you define fields that don't need a resolver? Currently if you have:
and it gets passed an object with an attribute |
I do write a simple class AnnotationObjectType(ObjectType):
def __init_subclass__(cls, *args, **kwargs):
fields = []
for name, func in cls.__dict__.items():
if name != "Meta" and not name.startswith("__"): # __init__ etc ...
fields.append((name, func))
for name, func in fields:
setattr(cls, "resolve_{}".format(name), func)
setattr(
cls,
name,
Field(func.__annotations__.pop("return"), **func.__annotations__),
)
super().__init_subclass__(*args, **kwargs) And it works fine for "mix" of query {
hero {
name
wife { # AnnotationObjectType
name ( only_first_name: true )
}
best_friend { # Interface
name ( only_first_name: true )
}
}
heroine {
name
husband { # normal ObjectType
name ( only_first_name: true )
}
best_friend { # Interface
name ( only_first_name: true )
}
}
} {"hero": {"best_friend": {"name": ["Hope"]},
"name": ["Scott", "Lang"],
"wife": {"name": ["Hope"]}},
"heroine": {"best_friend": {"name": ["Scott"]},
"husband": {"name": ["Scott"]},
"name": ["Hope", "Van", "Dyne"]}} You can find the entire example in here.
class User(AnnotationObjectType):
def name(self, info) -> graphene.String(required=True):
pass class User(ObjectType):
name = graphene.String(required=True)
def resolve_name(self, info):
pass For my implementing of Update: A easier way for writting is as below:
class User(AnnotationObjectType):
name = graphene.String(required=True) |
This is great! I'd love to see first-class support for annotation-based schemas. One thought: |
@ocavue thanks for opening the discussion. The new resolution syntax on Graphene 2 Personally, I do really like the way Inspired in all this I think the next version of Graphene (Graphene 3.0) could also allow developers to type their schemas like the following: @ObjectType
class Person:
id: str
name: str
last_name: str
def full_name(self, info) -> str:
return f'{self.name} {self.last_name}'
persons_by_id = {
'1': Person(id='1', name='Alice'),
'2': Person(id='2', name='Bob')
}
@ObjectType
class Query:
def get_person(self, info, id: str) -> Person:
'''Get a person by their id'''
return persons_by_id.get(id) There are some reasons on why I think this is a very powerful syntax:
However, there are some cons of this approach:
@ObjectType(skip: ['password'])
class Person:
# ...
password: str
@Connection(node=People)
class ConnectionPeople:
pass
@ObjectType
class Query:
def all_people(self, info, first: int = 10) -> ConnectionPeople:
return ConnectionPeople.get_from_iterable(...)
What are your thoughts? |
@syrusakbary Thank's for your comment. It's a beautify syntax. I really like it.
I'm ok with those two rules since graphene_sqlalchemy has aleady something similar: class User(Base):
id = Column(Integer, primary_key=True)
username = Column(String(30), nullable=False, unique=True)
password = Column(String(128), nullable=False)
class User(SQLAlchemyObjectType):
class Meta:
model = User
only_fields = ("id", "username") We should also consider that someone may do want to add attributes starting _ into GraphQL.
if we use native python types, mypy may raise an error when using DataLoader because what def get_student_names(self, info) -> typing.List(str):
return student_names_dataloader.load(self.__class_id) python<=3.5 don't support variable annotation syntax. So maybe we can support both syntaxes below: @ObjectType
class Person:
id: str
@ObjectType
class Person:
id = graphene.String() # current syntax |
(Coming from #886) I like the idea in general, but I have a problem in trying to glue together the type systems of GraphQL and Python - or rather, with writing a single statement to describe both at the same time: @ObjectType
class Collection:
def items(self, min: int = 0, max: int = 10) -> List[Item]:
return get_items(....) Python type system doesn't let me describe some important aspects of GraphQL type system:
More importantly, there's a certain level of hackyness here: it requires that Python and GQL type systems play very nice together. They probably mostly do, but type systems are always more complicated than they appear, and any little incompatibility will break the abstraction. |
@avivey Yeah, those are also my concerns. Using python annotations to define the schema is cute, but will conflict with the intended usage for static type checking. Using That being said, I think a middle ground would be to require explicit flagging of resolvers, and still rely on annotations as much as they remain valid type hints:
We still need to have a separate way to define any extra information for arguments, fields, types, etc.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I discovered this limitation in another project, and I got round it by defining the type within a context manager which then added the extra information at runtime, but didn't interfere at type checking time: I could imagine using a similar trick to write something like: @ObjectType
class Person:
with Anno("The ID of the person"):
id: str
with Anno("The first name of the person"):
name: str
with Anno("The last name of the person", deprecated=True):
last_name: str
def full_name(self, info) -> str:
return f'{self.name} {self.last_name}'
id_annotation = Person.__annotations__["id"]
print(id_annotation.description) # prints: The ID of the person
print(id_annotation.type) # prints: <class 'str'> I've written a gist to check this works: |
Pydantic is a library for defining data models, used in API frameworks(fastapi for example). It relies heavily on type annotations. Extra information that cannot be put in the type can be added through actual value assignments or through a Config nested class. |
@DrPyser this looks very interesting. I assume you would add the description and the deprecated items to a Schema() object? Do Schema() objects and dataclasses work together? |
@thomascobb That's a good question: https://pydantic-docs.helpmanual.io/#id1 . Would have to try or look into the code. Either using something like Pydantic's |
Reopening the issue. This seems still useful for the future of Graphene |
Looks like someone already thought of combining Pydantic and Graphene... |
Does the |
I am totally hoping the approach going forward is very much in-line with how pydantic works. Some of the reasons for this include:
Additionally the graphene-pydantic project could likely get absorbed into the future work. |
@Eraldo the Also the graphene-pydantic project looks very cool! https://github.com/strawberry-graphql/strawberry also uses dataclasses to define a graphql server. |
wow... I didn't know about https://github.com/strawberry-graphql/strawberry ... ( shameless plug) I was thinking about using python-type-extractor to make code-gen that spits out graphene-specialized code... (it's a lib for creating "type-nodes" from python type-annotations... this provides a nice compat-layer for python 3.6 ~ 3.8) to see what it does, you can just see how test-fixtures are converted in the tests (too lazy to write docs...) and |
Just dropping by, but I think PEP-593 |
As pydantic and strawberry has been mentioned, I would like to mention apischema too (I'm the author of this library). It generates GraphQL schema directly from your models using type annotations; models can be dataclasses, but not only (for example SqlAlchemy tables. However, it's not a Graphene schema but a graphql-core |
@wyfo thanks for the interesting link. Is there any way to use apischema to validate function arguments like https://pydantic-docs.helpmanual.io/usage/validation_decorator/ ? Also, I see that you support Unions for GraphQL output types. GraphQL seem to be heading towards a tagged union input type (graphql/graphql-spec#733). Are there any tools in apischema that would help create tagged inputs and corresponding outputs from a subclass Union such as https://wyfo.github.io/apischema/examples/subclasses_union/ ? |
No, for several reasons, the first being that apischema encourage using typing annotations and thus static type checking instead of runtime checks. But this question being quite off-topic here, so I invite you to start a discussion in apischema repository if you are interested, i'm always open to argument.
There aren't yet, because this feature is under discussion concerning the specifications, and because I need to see how it will be handled by graphql-core (depending on how graphql-js will implement it). But as soon it will be released, apischema will support it. |
@wyfo thanks, I've moved the discussion to wyfo/apischema#56 |
I have an implementation that allows me to write functional definitions for query resolvers and mutations using Python type annotations. I do not need to wrap these functions in a class. It isn't necessary to define an With this function, you can write queries like this:
...and mutations in basically the same way except the class is built with Queries and mutations defined this way can consume and return ObjectTypes without difficulty. One advantage of doing it this way is that it makes it obvious that query resolvers can be called There is one drawback: A source file can either contain queries or it can contain mutations. It cannot contain both. Also, there is no support for doing this with I've been running mutations and queries written this way in production for some time. If it interests you, I could ask my employer if I can contribute the |
This issue is more than four years old and this is already well-supported by other libraries. If you're looking for a library supporting this syntax, please check out I'll be closing this issue for now. If you feel like this is the wrong choice, please feel free to start a discussion on our Discord server 😊 |
This issue is a feature discussion.
For now, the way to write a ObjectType is as below:
I define name twice (
Hero.name
andHero.resolve_name
) because I have to define types of arguments and return value. This will cause some reading and writing problems.Since python3, annotation feature bring a native way to describe types. By using annotation, we can rewrite this class with a clearer way:
AnnotationObjectType
shouldn't be difficult to write if we somehow transform it intoObjectType
. But I think a looooot of cases should be tested before being released.Of cause even if we write an
AnnotationObjectType
class,ObjectType
class will not be dropped since python2.7 doesn't support annotation.I would like to hear you guys' comments and suggestions before doing this.
The text was updated successfully, but these errors were encountered: