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

introduced a simple query extraction mechanism for single-file multi … #458

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kontrol-apa
Copy link

This pr aims to address this issue : #433

If there are multiple queries in the same file, they are all concatenated into one string which requires manual adjustments after generation. This function extracts the relevant query string. It assumes that all queries start with "query operation_name" and are followed by another "query".
If the extraction fails due to some reason, the original query string is returned, so the original behavior is not modified. It also handles the case of just one query in a single file.

@Sytten
Copy link

Sytten commented Oct 17, 2023

Would be nice to support mutation and subscription too?

@tomhoule
Copy link
Member

We would at least need tests to make sure nothing breaks. I also think the string matching isn't robust enough. We do parse the queries with graphql-parser, so we could use the AST for that.

I see one reason not to do this: validation errors from the server. They will return file positions in the query the server received. It's confusing for users if they get an error with a position in a document they did not write but that was constructed from their queries.

@kontrol-apa
Copy link
Author

@tomhoule
Thanks for the quick reply. I am happy to spend sometime on this and get this pr merged eventually. I plan to:

  1. Use parse_query (as used here) to format the the extracted query string, to make sure that its extracted correctly. Question: This doesnt actually help with extracting individual queries, so i will keep the extraction mechanism the same but using parse_query will make sure that they are valid and are properly formatted, is this correct ?
  2. Write some tests for the cli with different query files. I think the function i am modifying is used else where as well but i assume those parts have their tests accordingly which would break if there is an issue.
  3. I didnt have to use mutation and subscription myself, but i would take a look at it once i am done with this. @Sytten

About validation errors, i understand the concern and this might be a problem if you have a lot of large queries in a single file. But to my experience if thats the case, you already need to manually modify the query_string variable in the generated code which would cause the same issue (the file position not matching the actual query file). Another idea would be splitting the large query file into multiple files with a single query in them, but this beats the purpose of containing related queries into the same query file (which what got me going in the first place)

@tomhoule
Copy link
Member

Use parse_query (as used here) to format the the extracted query string, to make sure that its extracted correctly. Question: This doesnt actually help with extracting individual queries, so i will keep the extraction mechanism the same but using parse_query will make sure that they are valid and are properly formatted, is this correct ?

What I had in mind would have been using the Position on the parsed operation to locate the query within the file. String matching is not very reliable.

@kontrol-apa
Copy link
Author

@tomhoule
Following your suggestion, i have modified the pr to use positions to extract query strings. It indeed became much more robust and now a mixed query file with subscriptions, mutations and queries is also parsed correctly. I have tested the changes locally by using the the following query file.

query StarWarsQuery($episodeForHero: Episode!) {
  hero(episode: $episodeForHero) {
    name
    __typename
  }
}
mutation CreateReviewForANewHope {
  createReview(episode: NEWHOPE, review: {
    stars: 5,
    commentary: "A classic that started it all!",
    favorite_color: {
      red: 255,
      green: 255,
      blue: 0
    }
  }) {
    stars
    commentary
  }
}
subscription OnReviewAdded {
  reviewAdded(episode: NEWHOPE) {
    stars
    commentary
    episode
  }
}
query StarWarsQuery2($episodeForHero: Episode!) {
  hero(episode: $episodeForHero) {
    name
    __typename
  }
}

i have tried to not touch any of the query.rs functionality, so the rest of the codebase should remain unaffected. I can add more tests if its needed.

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.

None yet

4 participants