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

Apply type condition in inline fragments to fix panic when using inline fragments with interfaces #816

Conversation

LukasKalbertodt
Copy link
Contributor

Fixes #815

Note that I was completely unfamiliar with the code and am still unsure if this is the proper way to solve the problem. It works for me. Let me know if I have to change more stuff, if I should fix the bug at a different place in code or whether I should change anything else.

Thanks!

@tyranron tyranron requested a review from LegNeato December 9, 2020 12:50
@LukasKalbertodt LukasKalbertodt marked this pull request as draft December 9, 2020 12:53
@LukasKalbertodt
Copy link
Contributor Author

(Converting to draft to fix test failures and add a new tests making sure this doesn't regress)

Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@LukasKalbertodt thanks for the effort! ❤️

You've changed the async_await.rs file, so fixed the async resolution. But there is still a sync resolution which behaves the old way. It would be nice to fix stuff there too.

@tyranron tyranron added the enhancement Improvement of existing features or bugfix label Dec 9, 2020
This fixes a panic when using inline fragments with interfaces (graphql-rust#815)
`type_name` was already forwarded to `QueryT`, so I guess
`concrete_type_name` should be forwarded as well. `RootNode` is an
"object" anyway and those should implement the method.
@LukasKalbertodt LukasKalbertodt force-pushed the fix-interface-inline-fragment-panic branch from f8700f0 to 9d8f2a3 Compare December 9, 2020 15:33
@LukasKalbertodt LukasKalbertodt force-pushed the fix-interface-inline-fragment-panic branch from 9d8f2a3 to 4c9071c Compare December 9, 2020 15:36
@LukasKalbertodt LukasKalbertodt marked this pull request as ready for review December 9, 2020 16:19
@LukasKalbertodt
Copy link
Contributor Author

Sorry for opening the PR without running the test. Now it seems to be fine. I fixed it for the sync code, too. And I added a test. Let me know if I can improve anything!

Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@LukasKalbertodt Yay! Thanks! Seems legit!

@tyranron tyranron merged commit 2c15ea7 into graphql-rust:master Dec 9, 2020
@tyranron tyranron removed the request for review from LegNeato December 9, 2020 18:27
@LukasKalbertodt
Copy link
Contributor Author

Thanks for merging and for this amazing response time!

@LukasKalbertodt LukasKalbertodt deleted the fix-interface-inline-fragment-panic branch December 10, 2020 07:39
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when using interfaces: Field homePlanet not found on type Some("Droid")
2 participants