Skip to content

Annotate variable struct fields too #390

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

Merged
merged 2 commits into from
Oct 16, 2021

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Oct 5, 2021

This PR attempts to properly rename variable struct fields.

Before this PR, variable field names do not carry #[serde] annotation that establishes mapping between the safe name and the original schema name. With this PR, the derive macro will attach those annotation labels to renamed fields.

This is important, because some GraphQL query input containing where field, which is fairly common by the way, and the derive macro maps it to where_. Without the annotation, this field gets serialized to where_, which the GraphQL server will certainly not recognize.

@tomhoule
Copy link
Member

tomhoule commented Oct 5, 2021

Thanks! I don't remember the details of the code, so I can't say in detail, but it looks like a good change. Would it be easy to write a test for? That would make sure we don't introduce a regression in the future.

@dingxiangfei2009
Copy link
Contributor Author

@tomhoule Thank you for the suggestion. Proc-macros are rather difficult to test on its own, so I have decided to put a test in graphql_client, along with input_object_variables tests. I hope that this would make sense.

@dingxiangfei2009
Copy link
Contributor Author

Hi @tomhoule. Do you think the newly added test sufficient for preventing future regression?

value
.as_object()
.unwrap()
.get("extern")
Copy link
Contributor Author

@dingxiangfei2009 dingxiangfei2009 Oct 16, 2021

Choose a reason for hiding this comment

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

An assertion is made here so that the original field extern of the query variable is sent to the GraphQL server, instead of extern_ which mostly likely gets ignored.

Copy link
Member

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

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

sorry I didn't get to review this sooner, it looks great, thank you for the PR!

@tomhoule tomhoule merged commit a6a8972 into graphql-rust:main Oct 16, 2021
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