-
-
Notifications
You must be signed in to change notification settings - Fork 542
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
Custom field API discussion #2168
Comments
API 1 - Extending
|
Pros | Cons |
---|---|
more understand able API, no need for rocket science to know how strawberry calls it | decorating with a class is awkward |
allows to modify everything you can in one place | type cannot be determined statically |
allows inheriting other custom fields and modifying upon them | |
opens a door for new contributors |
API 2 - Magic decorators :
Pros | Cons |
---|---|
looks nicer user wise | can't override type (easily ) |
can't override name | |
can't override arguments type | |
ordering the decorators can get cumbersome | |
How would you deal with basic fields? |
API 3 - Extension classes in conjunction with API 1:
class BaseExtension:
@classmethod
def override_name(cls, field) -> str:
return field.graphql_name
@classmethod
def override_type(cls, field) -> StrawberryAnnotation:
return field.type_annotation
@classmethod
def resolve(cls, previous, root, info, args, kwargs) -> Any:
return previous
class NoUnderScore(BaseExtension):
@classmethod
def override_name(cls, field):
return field.python_name.strip("_")
@strawberry.type
class A:
_a: int = strawberry.field(extensions=[NoUnderScore])
Extension would be called in StrawberryField.__post__init__()
by order and their benefit is that they don't change the field's signature.
If a want to use API 1 he would be discouraged by the docs but it would be possible.
Pros | Cons |
---|---|
type is not changed | magically called by strawberry |
somewhat more user-friendly than API 1 ot 2 | may be causing issues if used with a customized field but idk |
allows validating the use gave the right type to override |
I vote for 3
For 2: can't override type (easily )my idea was to use the decorated function to update the return type: @strawberry.type
class Paginated(T):
items: T
has_next: bool
def paginated(function: Callable[[...], T]):
@strawberry.wraps(function)
def paginated_function(root: Any, info: Any, first: int, **kwargs) -> Paginated[T]:
# strawberry.wraps will change the original function
# to always accept root and info
result = function(root, info=info, **kwargs)
return Paginated(items=result[:first], has_next=True)
return paginated_function can't override namewe could find a way to do this, but... do you want it? you can also still pass a custom name to ordering the decoratorsYes, the requirement here is that you need to use the decorators after using How would you deal with basic fields?Good question, I'm not sure yet |
I personally don't but there was someone on discord... https://discord.com/channels/689806334337482765/1012409889437515867/1012411651330424872 |
what if the user gave ... -> Paginated[T].resolve_type() this would a. lose the type hints. b. cause the user to mess up with native typing stuff instead the nice API of |
another thing to note here is that we should probably provide a configuration for a default field class and that is a +1 for API 1 | 3 |
the type resolution would be done in
why? |
I can't think of a use cases but:
if a user wants to use a custom field all over the schema.
and basically this can replace |
I think we should go for the Field Extension classes that @nrbnlulu suggested. I would modify the API slightly to something like this: class UpperCaseExtension(FieldExtension):
def apply(self, field):
assert field.type is str # check that field type is a string
def resolve(self, next, source, info, arguments):
result = next(source, info, arguments)
return result.upper()
@strawberry.type
class A:
a: str = strawberry.field(extensions=[UpperCaseExtension]) A more complicated example: class PaginatedExtension(FieldExtension):
def apply(self, field):
assert field.type is list
if "first" not in field.arguments:
field.arguments["first"] = int
def resolve(self, next, source, info, arguments):
first = arguments.pop("first")
result = next(source, info, arguments)
return result[:first]
@strawberry.type
class Query:
@strawberry.field(extensions=[PaginatedExtension])
def books() -> List[str]:
return [
"Pride and Prejudice",
"Sense and Sensibility",
"Persuasion",
"Mansfield Park",
] I think this API allows you to easily modify field arguments, return types and executions while being easy to apply multiple extensions. It also pairs nicely with our existing Schema Extensions (where could actually deprecate the usage of Any objections @patrick91 @nrbnlulu ? |
I think that looks good, if I want to add description to argument will it look like this? class PaginatedExtension(FieldExtension):
def apply(self, field):
assert field.type is list
if "first" not in field.arguments:
field.arguments["first"] = Annotated[int, strawberry.argument(description="ABC") ?
I think this might not work in extensions like the ones for tracing |
Yes I guess so. Though it might be worth making this work: field.arguments["first"] = strawberry.argument(int, description="ABC") Or we could add a helper function: field.arguments["first"] = self.create_argument(int, description="ABC")
Good point. We can shelve this idea for now then. It's not essential. |
Yes, I'd love to have either! I think I'm in favour of the second option for the time being. The reason is that if we allow the first one, we should also make this use case work: def a_resolver(a: strawberry.argument(int, description="ABC")) -> str:
return str(a) but unfortunately this is not allowed by any type checkers (and might never be) :( So it might be easier to have a dedicated function (which might be just an alias to using Annoated) for the time being 😊 |
Where should we start working on this? My PR is getting older and older :) |
@nrbnlulu I think the first step is to add support for field extensions that can implement the Basic requirements:
I think it's worth starting a new PR from scratch and please keep it focussed on just these changes so that's it's easier to review. Does that sound like a good approach? |
@jkimbo I like the idea though I think the code base needs re-factorization and it would be harder to do after adding more features. so it is either to go with my big PR with the changes needed, or create a new PR just for refactoring and then start working on this one. |
@nrbnlulu any refactoring should definitely be in it's own PR because it becomes complicated to review both new features and refactoring at the same time. I'm happy to spend some time working on a first pass of field extensions (as I described above) if you want to carry on with the refactoring? I would also highly recommend splitting out your refactoring work into multiple changes if possible so that it's easier to review. |
@jkimbo O.K, currently I have no time but if I will hopefully I'll start working on splitting things up there. |
#2567 implementing this is ready for review now. The Permissions refactor PR using extensions shows an example on how to use it. Please check it out and let me know if that works for you |
Released in https://github.com/strawberry-graphql/strawberry/releases/tag/0.162.0 🥳 |
We've been discussing the API for custom fields quite a bit on discord, I'll summarize some of the thoughts here.
Goal
We want to allow users of Strawberry to leverage code to change how fields work and what they return.
Some use cases:
None
if users are not logged in)Worries
My personal worry is this might make it harder to understand how the final schema will look like, but I might be worrying too much 😊
How it should work
In #920 we have a proposal that uses a field class to update information about the field. I was also thinking we could try to leverage python's dynamic features to do the same thing. Let's see both approaches:
API 1
API 2
I'm not 100% sure if this API is doable, but it might be worth trying
I haven't added any pro and cons for both APIs mostly because I want to hear other people's opinion first 😊
The text was updated successfully, but these errors were encountered: