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

Fixing bug with only __typename in the selection set #1525

Merged
merged 16 commits into from
Jul 6, 2023

Conversation

severussundar
Copy link
Contributor

Why make this change?

     {
        book_by_pk(id: 1){
             __typename
         }
     }
   

pk query bug

What is this change?

  • During the construction of SqlQueryStructure, if it is observed that there are no columns selected, then one of the primary keys is added so that the eventual database query constructed contains a valid column name in the SELECT statement.

How was this tested?

  • Integration Tests
  • Manual Tests

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Waiting for few answers, perhaps the optimization of not hitting the db can be a followup PR

@aaronpowell
Copy link
Contributor

Waiting for few answers, perhaps the optimization of not hitting the db can be a followup PR

I would have expected that to be the better approach, since you don't actually need any data back from the DB, but I can see there being a problem when you've done a query like:

query {
  books {
    items {
      __typename
    }
  }
}

That should return a items array that has n __typename properties in it, where n is the count of records in the query (up to 100).

@severussundar severussundar requested a review from Aniruddh25 June 7, 2023 04:00
Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

LGTM after 1 optimization to generate expected output

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for addressing all comments.

Copy link
Contributor

@ayush3797 ayush3797 left a comment

Choose a reason for hiding this comment

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

LGTM!

@severussundar severussundar merged commit 7568c5b into main Jul 6, 2023
@severussundar severussundar deleted the dev/shyamsundarj/fix_typename_bug branch July 6, 2023 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working graphql
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selection set without entity fields returns an error response for GraphQL query and mutation
4 participants