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

Update transaction list to differentiate GraphQL requests #848

Closed

Conversation

ArjanSM
Copy link
Contributor

@ArjanSM ArjanSM commented Jul 13, 2022

πŸ“· Screenshots

πŸ“„ Context

Issue #116 talks about the shortcoming of the initial approach of supporting GraphQL on Chucker.

This PR is an attempt support GraphQL on Chucker using the guidelines mentioned in Issue #116.

πŸ“ Changes

  1. Enable clients to set a graphQLEndpoint through the ChuckerInterceptor.Builder
  2. Pass the graphQLEndpoint as an argument to the RequestProcessor.process(..) method
  3. Add isGraphQLRequest property in the HttpTransaction entity & HttpTransactionTuple
  4. Expose isGraphQLRequest via HttpTransactionTuple
  5. Tests
  6. Update Sample app to illustrate the effects of the change.

πŸ“Ž Related PR

PR#805 is also doing the same thing but does not handle GraphQL requests via HTTP GET methods. But this PR does not block any other PR.

🚫 Breaking

  1. ChuckerInterceptor now contains an additional parameter to represent a GraphQL path or url
  2. RequestProcessor's process(..) method would accept the Graphql path passed by Interceptor.

πŸ› οΈ How to test

Test cases for RequestProcessor included in the PR.
chucker_update_transaction_list

⏱️ Next steps

@ArjanSM ArjanSM changed the title Update graphql transaction list Update transaction list to differentiate GraphQL requests Jul 13, 2022
@ArjanSM
Copy link
Contributor Author

ArjanSM commented Aug 25, 2022

@cortinico, @vbuberen My apologies I'm sure you all are very busy and I don't want to pester. I can understand this PR may not be a priority considering you all are in the middle of revamping the HttpTransactionModel.
But could we have a discussion about what the new transaction model would look like and what we could do to support GQL on chucker?

@cortinico
Copy link
Member

I'm leaning towards closing this as we're about to merge #805
Can I ask you to rebase your work on top of it?

@ArjanSM
Copy link
Contributor Author

ArjanSM commented Sep 14, 2022

@cortinico Sure!
I'll close this PR and have a new PR ready instead.

@ArjanSM ArjanSM closed this Sep 14, 2022
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

2 participants