Skip to content
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

Complete deprecated fields and arguments support #1472

Merged

Conversation

vhutov
Copy link
Contributor

@vhutov vhutov commented Nov 12, 2022

Implementing enhancement from #1446
In this PR I ensure that Argument, Field and InputField support deprecation functionality.

TODOs from the issue:

  • Is this already supported in graphql-core?
  • Add deprecation reason field to Meta Options Classes and [Input]Field logic
    Skipped this one, as meta is not populated to the Mounted object.
  • Check that required NonNull input fields are not deprecatable (see spec)

Notes:

  1. Field and InputField already had this field, whereas Argument didn't.
  2. When constructing schema, some objects were already passing deprecation reason. I added the missing parts: InputField and Argument. Enum and Field were compliant.
  3. When mounting InputObject or Enum implicitly, meta fields are not populated to the Mounted object.

@codecov
Copy link

codecov bot commented Nov 12, 2022

Codecov Report

Base: 95.97% // Head: 95.98% // Increases project coverage by +0.00% 🎉

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1472   +/-   ##
=======================================
  Coverage   95.97%   95.98%           
=======================================
  Files          51       51           
  Lines        1739     1742    +3     
=======================================
+ Hits         1669     1672    +3     
  Misses         70       70           
Impacted Files Coverage Δ
graphene/types/schema.py 95.41% <ø> (ø)
graphene/types/argument.py 97.82% <100.00%> (+0.09%) ⬆️
graphene/types/inputfield.py 100.00% <100.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
Copy link
Member

That was fast! Thank you for the PR :) I'll check it out ASAP!

@vhutov
Copy link
Contributor Author

vhutov commented Nov 13, 2022

@erikwrede I felt some courage to play with MountedType and SubclassWithMeta support. Now it's generic and works with Enum's, InputObject's and Scalar's. I added a class method which describes which Meta fields to inherit. I think it's clear to a reader what to expect. LMKWYT

@vhutov
Copy link
Contributor Author

vhutov commented Nov 16, 2022

@erikwrede Hey! Have you had a chance to take a look?

@erikwrede
Copy link
Member

erikwrede commented Nov 16, 2022

@vhutov Sorry for taking to long, I was actually just looking at your new changes right now.
Need to play around with it to give you a more thorough opinion though, as this is had become a larger change than just adding the deprecation now. Trying to get back to you on the weekend 🙂

If you just need the deprecation for now, maybe it would probably best to split up the PRs and have a more fundamental discussion about refactoring the meta classes in the other PR

@vhutov
Copy link
Contributor Author

vhutov commented Nov 16, 2022

@erikwrede no worries, I'm not in rush. I just thought it'd be better to add Meta support as well. I can certainly split them in two, if you think it's better. Thanks for looking at this!

@erikwrede
Copy link
Member

Having thought about it again, think it's better to split up the two PRs and think about the mountable_meta again. The current proposal would also require users to adapt the new mountable_meta classmethod, potentially breaking things by requiring them to implement mountable_meta for all kwargs they want to be inheritable.

If we are talking about a redesign of that part, I'd personally prefer to explicitly get the inheritable fields from a type, without having to implement a second mountable_meta method.
That configuration, it would be less redundant and field name changes wouldn't pose an issue if someone forgot to modify the return of mountable_meta.

A rough concept could look something like this:

Assume we have a base class (roughly based on the actual InputTypeOptions class in Graphene)

class InputObjectTypeOptions:
   description: str
   deprecation_reason: str
   abcd: str

we would have an explicit definition of inheritable fields from that class. It is also extendable by subclassing it.

We could now adapt this for mounted types like Argument.
If a mounted Type specifies a class MountedTypeOptions, we can use that to extract all fields that are contained in the MountedTypeOptions from the UnmountedTypeOptions iff they exist on both types:

InputObjectType.meta_type = InputObjectTypeOptions
class ArgumentOptions:
   description: str
   deprecation_reason: str
   required: bool



Mount InputObjectType
# Extract deprecation reason from InputObjectTypeOptions (InputObjectType._meta)
# Don't extract required: field not defined on InputObjectTypeOptions
# Don't extract abcd: field not defined on ArgumentOptions

If no MountedTypeOptions class exists for the mounted type, we fall back to the legacy behavior.

This is just an initial idea, and by no means a finalized design. Also open to suggestions on why we should use mountable_meta instead. LMKWYT!

@vhutov vhutov force-pushed the feature/allow-deprecation-fields-1446 branch 2 times, most recently from f6fc761 to f28e3e5 Compare November 27, 2022 19:30
@vhutov
Copy link
Contributor Author

vhutov commented Nov 27, 2022

@erikwrede cleaned up the PR to contain only the deprecation reason ✅
Let me think about the proposed idea meanwhile.

@vhutov
Copy link
Contributor Author

vhutov commented Nov 27, 2022

UPD: Changed PR description

@vhutov vhutov force-pushed the feature/allow-deprecation-fields-1446 branch from f28e3e5 to e9c1676 Compare December 6, 2022 14:24
@vhutov
Copy link
Contributor Author

vhutov commented Dec 6, 2022

@erikwrede Could you take a look?

Copy link
Member

@erikwrede erikwrede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review for this was still on my list - thank you for updating the PR!

I noticed that you made changes to field.py to only allow deprecation on nullable fields. According to the Spec, this constraint only applies to input fields.
Look at the following example from spec:

type ExampleType {
  invalidField(
    newArg: String
    oldArg: String! @deprecated(reason: "Use `newArg`.")
  ): String
}

With input fields, the restriction makes sense, as we don't have any alternative to passing a value to the required oldField, although oldField is deprecated. For input fields, the alternative is to not query the field anymore.

The rest looks good, thank you!

Comment on lines 92 to 94
assert (
deprecation_reason is None
), f"Field {name} is required, cannot deprecate it."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This restriction only applies to input object fields & arguments : https://spec.graphql.org/draft/#sec--deprecated

Comment on lines 149 to 160
description="Create a user",
deprecation_reason="Is deprecated",
required=True,
required=False,
)

field = MyMutation._meta.fields["create_user"]
assert field.name == "createUser"
assert field.description == "Create a user"
assert field.deprecation_reason == "Is deprecated"
assert field.type == NonNull(CreateUser)
assert field.type == CreateUser

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See field.py

@vhutov vhutov force-pushed the feature/allow-deprecation-fields-1446 branch from e9c1676 to de78461 Compare December 9, 2022 23:14
@vhutov vhutov requested a review from erikwrede December 9, 2022 23:15
Copy link
Member

@erikwrede erikwrede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
As a follow-up, we should rework the get_introspection_query() for schema to allow for optional args to be introspected. Currently schema.introspect() calls this using the default args that set input_value_deprecation: bool = False,, excluding any deprecated fields or args.

@erikwrede erikwrede merged commit 19ea63b into graphql-python:master Dec 10, 2022
@erikwrede
Copy link
Member

Will release tonight in 3.2.1, thank you! 🙂

@vhutov
Copy link
Contributor Author

vhutov commented Dec 10, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants