-
-
Notifications
You must be signed in to change notification settings - Fork 127
Add support for single record queries by primary key #590
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
base: master
Are you sure you want to change the base?
Conversation
|
Hey @spitfire55 , thanks for this PR. We haven't forgotten this, just totally slammed right now. Will come back to it eventually. @olirice before we review this, do you an opinion on the approach here. Do we want to add a method to access object by pk as implemented in this PR? |
|
yes, i think there's been enough demand for it to justify the addition + it doesn't introduce any performance issues Like Raminder mentioned, we're pretty slammed right now so it may take some time to work through this and review so sorry in advance for that! From a very quick glance the things that jumped out are
appreciate your work on this, looking forward to getting it merged |
21cb2a1 to
59bc719
Compare
59bc719 to
42e65cb
Compare
42e65cb to
27e5503
Compare
5c033e1 to
e434169
Compare
|
@spitfire55 I'll find some time today or tomorrow to review it. Sorry for the delay and thanks again for your patience. |
|
LGTM apart from missing docs. @olirice would you like to take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imor conceptually I'm on board
but there are several untested pathes. The biggest ones are:
- nodeByPk with a nested function ref
- nodeByPk with a connection ref
- introspection including __typename ref
- selecting an unknown field ref
If
- we can resolve the coverage issues ^ + not have this PR decrease coverage
- you've reviewed the code, are happy with it
- its been tested in GraphiQL manually to make there are no errors and looks as expected with a few sample tables
- docs
then we'll be good to go
|
@spitfire55 would you mind making changes suggested by @olirice. |
|
Yep, I'll tackle these this week. |
- Fix TODO nodeId check, move out of transpile into builder - Create SupportedPrimaryKeyType enum for PK type matching
e434169 to
d68c423
Compare
…ction and nodeByPk with connection
|
@imor after months of neglect, I've updated this PR to get code coverage back to where it should be. Hopefully can be merged. |
|
Thanks for you hard work on this @spitfire55. I'll review this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @spitfire55,
I've reviewed the code and it is 99% there. I've also tested in GraphiQL with a complex schema and data and everything works.
I've left some minor comments to be tackled. There's a comprehensive test case coverage but imo we can add following test cases to cover a few more edge cases:
- Tables without a primary key shouldn't have a
<table>ByPkexposed in the query. - Test that aliases work correctly with
<table>ByPkfields. - Test that only supported column types have a
<table>ByPkfields exposed and other types don't
Apart from above if docs/api.md and docs/changelog.md files could be updated to mention the change with examples, this PR should be ready to merge.
…error for missing Pk columns
|
Addressed everything above. If the UUID --> Uuid rename should be split out to a separate PR, I can do that. |
|
Thanks again Dale, I'm going to merge this. I'll add docs in a separate PR. |
|
@olirice need your approval to merge since you requested changes. |
|
Draft PR for docs here: #624 |
What kind of change does this PR introduce?
Users can now use [table_name]ByPk(key) queries to get a single record using the primary key
What is the current behavior?
No first-party support for single record queries by primary key.
See #554 .
What is the new behavior?
Adds a [table_name]ByPk(pk: pk_type) method to all tables that define a primary key