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/rustberry integration v2 #3504

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

Conversation

erikwrede
Copy link
Member

@erikwrede erikwrede commented May 18, 2024

WIP

@botberry
Copy link
Member

botberry commented May 18, 2024

Hi, thanks for contributing to Strawberry 🍓!

We noticed that this PR is missing a RELEASE.md file. We use that to automatically do releases here on GitHub and, most importantly, to PyPI!

So as soon as this PR is merged, a release will be made 🚀.

Here's an example of RELEASE.md:

Release type: patch

Description of the changes, ideally with some examples, if adding a new feature.

Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :)

Here's the tweet text:

🆕 Release (next) is out! Thanks to Erik Wrede for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @erikwrede - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 7 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -82,6 +82,7 @@ async def execute(
execution_context: ExecutionContext,
execution_context_class: Optional[Type[GraphQLExecutionContext]] = None,
process_errors: Callable[[List[GraphQLError], Optional[ExecutionContext]], None],
executor: Executor,
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider making the executor parameter optional.

If the executor parameter is not provided, you could default to a standard executor like GraphQlCoreExecutor. This would make the function more flexible and backward-compatible.

Suggested change
executor: Executor,
executor: Executor = GraphQlCoreExecutor(),

@@ -29,6 +30,45 @@
from .graphql import OperationType


class Executor(abc.ABC):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider adding a docstring to the Executor class.

Adding a docstring to the Executor class would help other developers understand its purpose and usage.

Suggested change
class Executor(abc.ABC):
class Executor(abc.ABC):
"""
Abstract base class for executing operations on a given schema.
Attributes:
schema (Schema): The GraphQL schema to execute operations on.
"""

) -> None: ...


class GraphQlCoreExecutor(Executor):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider adding a docstring to the GraphQlCoreExecutor class.

Adding a docstring to the GraphQlCoreExecutor class would help other developers understand its purpose and usage.

Suggested change
class GraphQlCoreExecutor(Executor):
class GraphQlCoreExecutor(Executor):
"""
Executes GraphQL queries using the provided schema.
This class extends the Executor to handle the execution of GraphQL
queries, mutations, and subscriptions.
"""

@@ -82,6 +83,7 @@ def __init__(
Dict[object, Union[Type, ScalarWrapper, ScalarDefinition]]
] = None,
schema_directives: Iterable[object] = (),
executor_class: Optional[Type[Executor]] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider adding a docstring to the executor_class parameter.

Adding a docstring to the executor_class parameter would help other developers understand its purpose and usage.

Suggested change
executor_class: Optional[Type[Executor]] = None,
"""
executor_class: Optional[Type[Executor]] = None,
The class to use for executing queries. If not provided, a default executor will be used.
"""
executor_class: Optional[Type[Executor]] = None,

@@ -0,0 +1,35 @@
from typing import TYPE_CHECKING
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider adding a module-level docstring.

Adding a module-level docstring to strawberry/schema/executors.py would help other developers understand the purpose and usage of the module.

Suggested change
from typing import TYPE_CHECKING
"""
This module provides the necessary components for executing GraphQL queries
within the Strawberry framework. It includes error handling and type checking
utilities to ensure robust and type-safe query execution.
"""
from typing import TYPE_CHECKING
from graphql import GraphQLError

RUSTBERRY_DOCUMENT_FIELD = "__rustberry_document"


class RustberryExecutor(Executor):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider adding a docstring to the RustberryExecutor class.

Adding a docstring to the RustberryExecutor class would help other developers understand its purpose and usage.

Suggested change
class RustberryExecutor(Executor):
class RustberryExecutor(Executor):
"""
Executor for handling Rustberry-specific operations within the schema.
Args:
schema (Schema): The GraphQL schema to execute against.
"""

validation_successful = self.compiler.validate(document)
if not validation_successful:
execution_context.errors = execution_context.errors or []
execution_context.errors.append(GraphQLError("Validation failed"))
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider providing more context in the validation error message.

Providing more context in the validation error message, such as which part of the document failed validation, would help in debugging and understanding the issue.

Suggested change
execution_context.errors.append(GraphQLError("Validation failed"))
validation_errors = self.compiler.get_validation_errors(document)
error_message = f"Validation failed: {validation_errors}"
execution_context.errors.append(GraphQLError(error_message))

extensions=extensions_runner.get_extensions_results_sync(),
)
except GraphQLError as exc:
execution_context.errors = [exc]
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (code-quality): Extract duplicate code into function (extract-duplicate-method)

@botberry
Copy link
Member

botberry commented May 18, 2024

Apollo Federation Subgraph Compatibility Results

Federation 1 Support Federation 2 Support
_service🟢
@key (single)🟢
@key (multi)🟢
@key (composite)🟢
repeatable @key🟢
@requires🟢
@provides🟢
federated tracing🔲
@link🟢
@shareable🟢
@tag🟢
@override🟢
@inaccessible🟢
@composeDirective🟢
@interfaceObject🟢

Learn more:

Copy link

codecov bot commented May 18, 2024

Codecov Report

Attention: Patch coverage is 93.33333% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 94.24%. Comparing base (fc322a8) to head (3ca28b3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3504      +/-   ##
==========================================
- Coverage   96.51%   94.24%   -2.28%     
==========================================
  Files         511      507       -4     
  Lines       32965    31865    -1100     
  Branches     5482     3666    -1816     
==========================================
- Hits        31815    30030    -1785     
- Misses        917     1550     +633     
- Partials      233      285      +52     

Copy link

codspeed-hq bot commented May 18, 2024

CodSpeed Performance Report

Merging #3504 will improve performances by ×1,100

Comparing feat/rustberry-integration-v2 (3ca28b3) with main (fc322a8)

Summary

⚡ 4 improvements
✅ 8 untouched benchmarks

Benchmarks breakdown

Benchmark main feat/rustberry-integration-v2 Change
test_execute_complex_schema[50] 23,158.3 ms 21.3 ms ×1,100
test_execute_with_100_items 58.6 ms 39.7 ms +47.74%
test_execute_with_10_items 27.3 ms 6.1 ms ×4.5
test_execute_with_many_fields_and_directives 232.3 ms 210.1 ms +10.55%

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.

2 participants