graphql-rust / graphql-client

Typed, correct GraphQL requests and responses in Rust
Apache License 2.0
1.12k stars 152 forks source link

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

Open kontrol-apa opened 8 months ago

kontrol-apa commented 8 months ago

This pr aims to address this issue : https://github.com/graphql-rust/graphql-client/issues/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 commented 8 months ago

Would be nice to support mutation and subscription too?

tomhoule commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

@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.