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

Expose inner value of GraphQLResponse. #1293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RyogaK
Copy link

@RyogaK RyogaK commented Nov 12, 2024

In my use case, I need to parse the GraphQLResponse from Juniper’s response and record it as structured logs. While GraphQLRequest has a public (pub) internal structure, but the internals of GraphQLResponse are not accessible, so currently, I have to serialize it once and then re-parse it as a string. This intermediary processing is cumbersome, and I would like to directly access the internal data of GraphQLResponse.

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Thanks for the change! Perhaps it is better to have an into_result function to mirror the from_result function?

I would also love a simple test that uses the function so we can make sure to not regress accidentally.

@tyranron tyranron added enhancement Improvement of existing features or bugfix k::api Related to API (application interface) labels Nov 14, 2024
@tyranron tyranron added this to the 0.17.0 milestone Nov 14, 2024
@RyogaK
Copy link
Author

RyogaK commented Dec 9, 2024

@LegNeato Thank you for the idea. However, in GraphQLRequest (not GraphQLResponse), the internal values are exposed as pub, and there is no test there at all. In Rust, pub values remain immutable unless marked as mut, ensuring that no logic can be violated. So, what exactly is the purpose of this test? Additionally, the name into_result suggests that it moves self, but GraphQLResponse needs to consistently be returned as a response after its contents are inspected by middleware. Therefore, I believe that cloning it solely for the purpose of referencing its contents is not appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix k::api Related to API (application interface)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants