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

Fix interface fields selection #607

Merged
merged 5 commits into from
Mar 23, 2020
Merged

Fix interface fields selection #607

merged 5 commits into from
Mar 23, 2020

Conversation

illambo
Copy link
Contributor

@illambo illambo commented Mar 20, 2020

Summary

Fix interface fields selection.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Existing tests have been adapted and/or new tests have been added

Links

Resolve #605
Resolve #491

@illambo illambo changed the title Fix interface fields selection, resolve issue #605 Fix interface fields selection Mar 20, 2020
@mfn
Copy link
Collaborator

mfn commented Mar 21, 2020

@crissi what do you think about this? Looks "sane" to me, but 🤷‍♀️

@crissi
Copy link
Contributor

crissi commented Mar 21, 2020

Does the sql alias still work?

The test does not seem to cover the bug, I only see changes to the sql

@illambo
Copy link
Contributor Author

illambo commented Mar 21, 2020

I try to elaborate a targeted test if needs.

@crissi
Copy link
Contributor

crissi commented Mar 21, 2020

Of course it would be better if the missing foreign key was added instead of *, but that might be difficult

@illambo
Copy link
Contributor Author

illambo commented Mar 21, 2020

I agree @crissi but I don't have time now to go further, is an acceptable solution for you now?
Thanks

@mfn
Copy link
Collaborator

mfn commented Mar 22, 2020

Would this also fix #491 ?

Would be a brief summary of how we fixed it? Would the following be an accurate description?

Fix interface field selection by forgoing individual column selects and simply allows fetch the complete model.

?

thanks

@illambo
Copy link
Contributor Author

illambo commented Mar 22, 2020

Yes, I think also fix that, but the unit test actually don’t cover this case (maybe later I can try to add that if needs).
The description seems good for me.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Alright!

Thanks @crissi

@illambo please also add a changelog entry ; I tried to do it myself but pushing to your branch wasn't allowed, something like this please:

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3f51e77..2c829b0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,7 @@ CHANGELOG
 - Add support for macros on the GraphQL service/facade [\#592 / stevelacey](https://github.com/rebing/graphql-laravel/pull/592)
 ### Fixed
 - Fix the infinite loop as well as sending the correct matching input data to the rule-callback [\#579 / crissi](https://github.com/rebing/graphql-laravel/pull/579)
+- Fix selecting not the correct columns for interface fields [\#607 / illambo](https://github.com/rebing/graphql-laravel/pull/607)
 ### Changed
 - Refactor route files with the goal of making adding subscription support easier [\#575 / crissi](https://github.com/rebing/graphql-laravel/pull/575)
 ### Removed

thanks!

@illambo
Copy link
Contributor Author

illambo commented Mar 22, 2020

@mfn ok, done

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Much appreciated, thanks 🙏

@mfn mfn merged commit 1cf8d92 into rebing:master Mar 23, 2020
@mfn
Copy link
Collaborator

mfn commented Apr 3, 2020

https://github.com/rebing/graphql-laravel/releases/tag/5.0.0 has been released which includes this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants