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

feat: add validator and deprecation notices for non-paginated queries #2651

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

why-arong
Copy link
Contributor

@why-arong why-arong commented Aug 5, 2024

Fixes #2560

Summary:

In this update, I have introduced a new validator (GQLDeprecatedQueryValidator) to handle the deprecation of non-paginated GraphQL queries. This validator logs warning message whenever a non-paginated query is used. Additionally, I have marked specific queries with the deprecation_reason attribute to indicate that these fields are deprecated.

Details:

  1. Added Validator for Non-Paginated Queries:
  • I created a validator named GQLDeprecatedQueryValidator that checks if a field in a GraphQL query is deprecated due to being non-paginated.

  • The deprecated queries are listed in the deprecated_queries tuple. These include queries that:

    • End with -s (indicating a plural form).
    • Do not have limit and offset arguments.
    • Are not implemented using the Relay specification.
  • If a query matches all these conditions, the validator logs a warning message using the log.warning method.

  1. Deprecation Reason Added to Queries:
  • For each deprecated query, I have added a deprecation_reason attribute. This helps to clearly indicate that the query is deprecated and provides the reason why.

Example:

users = graphene.List(  # legacy non-paginated list
    User,
    domain_name=graphene.String(),
    group_id=graphene.UUID(),
    is_active=graphene.Boolean(),
    status=graphene.String(),
    deprecation_reason="This query is deprecated due to lack of pagination support and will be removed in future versions.",
)

Review Request:

  1. Validation of Deprecated Queries:
  1. Deprecation Reason:
  • The deprecation_reason for each deprecated field is currently set to indicate that the query is deprecated due to lack of pagination support. If there is a more appropriate message or additional context that should be provided, please suggest it.

Checklist:

  • Milestone metadata specifying the target backport version
  • Mention to the original issue

@why-arong why-arong added comp:manager Related to Manager component type:enhance Enhance component, behavior, internals without user-facing features urgency:3 Must be finished within a certain time frame. labels Aug 5, 2024
@why-arong why-arong added this to the 24.03 milestone Aug 5, 2024
@why-arong why-arong changed the title feat: add deprecated query validator & notice feat: add validator and deprecation notices for non-paginated queries Aug 5, 2024
@why-arong why-arong added the skip:changelog Make the action workflow to skip towncrier check label Aug 5, 2024
@why-arong why-arong marked this pull request as ready for review August 5, 2024 09:01
Comment on lines +12 to +41
deprecated_queries = (
"agents",
"domains",
"groups_by_name",
"groups",
"images",
"customized_images",
"users",
"keypairs",
"keypair_resource_policies",
"user_resource_policies",
"resource_presets",
"scaling_groups",
"scaling_groups_for_domain",
"scaling_groups_for_user_group",
"scaling_groups_for_keypair",
"vfolders",
"container_registries",
)


def is_deprecated_query(query_name: str) -> bool:
return query_name.lower() in deprecated_queries


class GQLDeprecatedQueryValidator(ValidationRule):
def enter_field(self, node: FieldNode, *_args):
field_name = node.name.value
if is_deprecated_query(field_name):
log.warning("Non-paginated query '{}' is being used.", field_name)
Copy link
Member

Choose a reason for hiding this comment

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

Referencing deprecated query outside of the query definition can cause confusion to future readers in a high chance. Please check about possibilities of flagging each query as deprecated within the query implementation.

@@ -289,6 +289,7 @@ class Queries(graphene.ObjectType):
Agent,
scaling_group=graphene.String(),
status=graphene.String(),
deprecation_reason="deprecated_reason",
Copy link
Member

Choose a reason for hiding this comment

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

When talking about the deprecation it is important to also mention about alternative solutions. Please update every deprecation reason comments to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:manager Related to Manager component skip:changelog Make the action workflow to skip towncrier check type:enhance Enhance component, behavior, internals without user-facing features urgency:3 Must be finished within a certain time frame.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate all non-paginated list queries in GraphQL
2 participants