enhance(query-planner): avoid extra query prefix for anonymous queries
#580
Conversation
Summary of ChangesHello @ardatan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the internal representation of GraphQL queries within the query planner. By intelligently omitting the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ardatan
force-pushed
the
avoid-extra-query
branch
from
44ecaf6 to
bb670b9
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the query planner to avoid adding an extra query prefix for anonymous GraphQL queries, which is a good improvement for query string compactness. The core logic change is sound. My review includes a suggestion to simplify a conditional statement for better readability and idiomatic Rust. I've also pointed out a missing entry in the changeset file that is required by the repository's contribution guidelines. The accompanying test snapshot updates are all consistent with the intended change.
| if (self.operation_kind.is_none() | ||
| || self | ||
| .operation_kind | ||
| .as_ref() | ||
| .is_none_or(|k| matches!(k, OperationKind::Query))) | ||
| && (self.name.is_none() || self.name.as_ref().is_none_or(|n| n.is_empty())) | ||
| && (self.variable_definitions.is_none() |
There was a problem hiding this comment.
The conditions to check for an anonymous query can be simplified for better readability and to be more idiomatic. The is_none() || ...map_or(true, ...) pattern is redundant because map_or(true, ...) already handles the None case by returning true.1
| if (self.operation_kind.is_none() | |
| || self | |
| .operation_kind | |
| .as_ref() | |
| .is_none_or(|k| matches!(k, OperationKind::Query))) | |
| && (self.name.is_none() || self.name.as_ref().is_none_or(|n| n.is_empty())) | |
| && (self.variable_definitions.is_none() | |
| if self.operation_kind.as_ref().map_or(true, |k| matches!(k, OperationKind::Query)) | |
| && self.name.as_deref().map_or(true, str::is_empty) | |
| && self.variable_definitions.as_ref().map_or(true, Vec::is_empty) |
Style Guide References
Footnotes
-
The style guide emphasizes "Readability-always", including "explicit intent, idiomatic Rust". The suggested change improves readability and is more idiomatic. ↩
There was a problem hiding this comment.
all this to safe 5 characters?
There was a problem hiding this comment.
it has literally 0 impact on performance
There was a problem hiding this comment.
Yes, @ardatan, the current code looks good! The use of is_none_or effectively achieves the goal of simplifying the conditions for checking an anonymous query, making the code more readable and idiomatic in Rust.
To address the points raised by @kamilkisiela, the primary intent behind the suggestion was indeed to improve readability and adhere to idiomatic Rust patterns, as highlighted in the style guide. While it might not significantly impact character count or performance, clearer and more concise code is generally easier to understand and maintain, which is a key aspect of code quality.
There was a problem hiding this comment.
@kamilkisiela better to sync with other implementations. I realized this when I see tests in Hive Gateway. By the way, this logic was already there in
There was a problem hiding this comment.
better to sync with other implementations
not an argument for me as you know, it makes no difference whether Hive Gateway is adding query or not.
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-761476db-b2e5-4ebd-bafc-ccd10deb5c37/builder-761476db-b2e5-4ebd-bafc-ccd10deb5c370/elurpqyq2hi1um80j05v4cmti",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:cd0dbe5cc7b642eab0cc9fa02d6c2b90884d0082f1f505a5dbe0ad91bcf726fd",
"size": 1609
},
"containerimage.digest": "sha256:cd0dbe5cc7b642eab0cc9fa02d6c2b90884d0082f1f505a5dbe0ad91bcf726fd",
"image.name": "ghcr.io/graphql-hive/router:pr-580,ghcr.io/graphql-hive/router:sha-63e0263"
} |
✅
|
2 participants
When there is no variable definitions and no operation name, GraphQL queries can be sent without the
queryprefix. For example, instead of sending: