Skip to content

Conversation

@spitfire55
Copy link
Contributor

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

@imor
Copy link
Contributor

imor commented May 8, 2025

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?

@olirice
Copy link
Contributor

olirice commented May 8, 2025

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

  • lets filter the entities that automatically get these entrypoints to those that have primary keys of a type that's in a whitelist -> int, bigint, uuid, string are the ones that make sense to me
  • use exhaustive pattern matching where possible so its easier to find what needs updating when we extend enums in the future

appreciate your work on this, looking forward to getting it merged

@spitfire55 spitfire55 force-pushed the single_record_query_by_pk branch from 21cb2a1 to 59bc719 Compare May 13, 2025 20:26
@spitfire55 spitfire55 force-pushed the single_record_query_by_pk branch from 59bc719 to 42e65cb Compare June 16, 2025 16:57
@spitfire55 spitfire55 force-pushed the single_record_query_by_pk branch from 42e65cb to 27e5503 Compare July 22, 2025 01:01
@spitfire55
Copy link
Contributor Author

@imor @olirice any chance this can get reviewed and merged this week?

@spitfire55 spitfire55 force-pushed the single_record_query_by_pk branch 3 times, most recently from 5c033e1 to e434169 Compare July 22, 2025 01:05
@imor
Copy link
Contributor

imor commented Jul 22, 2025

@spitfire55 I'll find some time today or tomorrow to review it. Sorry for the delay and thanks again for your patience.

@imor
Copy link
Contributor

imor commented Jul 22, 2025

LGTM apart from missing docs. @olirice would you like to take a look?

@olirice olirice self-requested a review July 22, 2025 15:42
Copy link
Contributor

@olirice olirice left a 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

@imor
Copy link
Contributor

imor commented Jul 23, 2025

@spitfire55 would you mind making changes suggested by @olirice.

@spitfire55
Copy link
Contributor Author

Yep, I'll tackle these this week.

spitfire55 and others added 3 commits January 20, 2026 11:58
- Fix TODO nodeId check, move out of transpile into builder
- Create SupportedPrimaryKeyType enum for PK type matching
@spitfire55 spitfire55 force-pushed the single_record_query_by_pk branch from e434169 to d68c423 Compare January 20, 2026 16:59
@spitfire55
Copy link
Contributor Author

@imor after months of neglect, I've updated this PR to get code coverage back to where it should be. Hopefully can be merged.

@imor
Copy link
Contributor

imor commented Jan 21, 2026

Thanks for you hard work on this @spitfire55. I'll review this.

Copy link
Contributor

@imor imor left a 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>ByPk exposed in the query.
  • Test that aliases work correctly with <table>ByPk fields.
  • Test that only supported column types have a <table>ByPk fields 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.

@spitfire55
Copy link
Contributor Author

Addressed everything above. If the UUID --> Uuid rename should be split out to a separate PR, I can do that.

@imor
Copy link
Contributor

imor commented Jan 27, 2026

Thanks again Dale, I'm going to merge this. I'll add docs in a separate PR.

@imor imor requested a review from olirice January 27, 2026 04:04
@imor
Copy link
Contributor

imor commented Jan 27, 2026

@olirice need your approval to merge since you requested changes.

@imor
Copy link
Contributor

imor commented Jan 27, 2026

Draft PR for docs here: #624

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.

3 participants