Add support for GraphQL by rnitame · Pull Request #70 · ChuckerTeam/chucker

rnitame · 2019-07-25T04:34:08Z

cortinico

Would you be able to provide a sample public endpoint to include inside the Sample app?

cortinico · 2019-08-02T16:15:15Z

                val content = io.readFromBuffer(buffer, charset, maxContentLength)
                transaction.requestBody = content
+                transaction.operationName =
+                    Gson().fromJson(content, HttpRequestBody::class.java).operationName


This will be really inefficient :(
You're going to recreate a Gson instance for every request.
Please use a JsonConvertor.getInstance().

cortinico · 2019-08-02T16:16:54Z

    }
+
+    data class HttpRequestBody(
+        val operationName: String?


GSON annotation is missing here.

cortinico · 2019-08-02T16:17:08Z

        private val UTF8 = Charset.forName("UTF-8")
    }
+
+    data class HttpRequestBody(


Naming can be improved here:

Suggested change

data class HttpRequestBody(

data class GraphQLRequestBody(

Since the location of operation name was different depending on the library, it was absorbed using JsonParser

apollo-android :

{ "operationName": "xxx" }

retrofit-graphql :

{ "queryContainer" : { "operationName": "xxx" } }

cortinico · 2019-08-02T16:18:07Z

                Status.Failed -> " ! ! !  $method $path"
                Status.Requested -> " . . .  $method $path"
-                else -> responseCode.toString() + " " + method + " " + path
+                else -> responseCode.toString() + " " + method + " " + path + if (isGraphql) operationName else ""


Can you add a space between path and operation name?

rnitame · 2019-08-05T01:11:16Z

@cortinico Thanks for your comments!

Would you be able to provide a sample public endpoint to include inside the Sample app?

I added implementation using SWApi. http://graphql.org/swapi-graphql/ :)
Please review again.

cortinico · 2019-08-06T13:49:14Z

+        val key = "operationName"
+        if (content.has(key)) return content.get(key).asString
+
+        return if (content.has("queryContainer")) {


Hey @rnitame
Apparently here there is a problem. GraphQL request should have the operationName at the top level and should not be wrapped inside a queryContainer object:

This is an invalid GraphQL request.

The problem is caused by the initialisation of the retrofit interface:

Retrofit retrofit = new Retrofit.Builder() .baseUrl("https://graphql.org") .addConverterFactory(GsonConverterFactory.create()) .client(client) .build();

Although we could fix it by using the Converter factory provided by retrofit-graphql, I think it's better if we change approach here.

Can I ask you to:

a) Don't use retrofit-graphql. Is yet another dependency that we probably don't need.
b) Just create a simple data class and do a GraphQL request without external library (as described here: https://graphql.org/learn/serving-over-http/#post-request)
c) Remove all the .graphql file?

cortinico · 2019-08-13T08:57:01Z

Let me know when this is ready for review again :)

…support

olivierperez · 2019-10-02T21:10:54Z

+                    object : TypeToken<GraphQLRequestBody>() {
+                    }.type


We can use this trick

// Somewhere smart in the lib inline fun <reified T> genericType() = object: TypeToken<T>() {}.type

// Usage genericType<GraphQLRequestBody>()

olivierperez · 2019-10-02T21:13:40Z

+            operationName.setText(transaction.getOperationName());
+            operationName.setVisibility(transaction.isGraphQL() ? View.VISIBLE : View.GONE);
+            graphql.setVisibility(transaction.isGraphQL() ? View.VISIBLE : View.GONE);


All those lines are about GraphQL, can do something to group them ?

if (transaction.isGraphQL()) { operationName.setText(transaction.getOperationName()); operationName.setVisibility(View.VISIBLE); graphql.setVisibility(View.VISIBLE); }

And set them to View.GONE by default.

olivierperez · 2019-10-02T21:16:06Z

+repositories {
+    maven { url 'https://jitpack.io' }
+}
+


Can you add the repository in /build.gradle instead? Under allprojects.repositories.

Actually, why do we need this repository? Can we just remove it?

Correct, this can be removed entirely

olivierperez · 2019-10-02T21:18:55Z

            cache(30).enqueue(cb)
        }
+
+        with(SampleApiService.getGraphQLInstance(getClient(this))) {


The method getClient doesn't only get the HTTP client, it also create it. It should be named buildHttpClient or something, it would explicitly show that something is built by this method. And maybe we shouldn't abuse of calls to this method.

cortinico · 2019-10-24T00:00:00Z

+repositories {
+    maven { url 'https://jitpack.io' }
+}
+


Correct, this can be removed entirely

cortinico · 2019-10-24T00:00:07Z

 -dontwarn okio.**
 -dontwarn retrofit2.**
+
+# retrofit-graphql


Please remove this

cortinico · 2019-10-24T00:00:15Z


    <uses-permission android:name="android.permission.INTERNET" />

+    <uses-sdk tools:overrideLibrary="io.github.wax911.library" />


Please remove this

cortinico · 2019-10-24T00:25:40Z

+    android:viewportWidth="24"
+    android:viewportHeight="24">
+  <path
+      android:fillColor="#e535ab"


Please export this color

cortinico · 2019-10-24T00:27:45Z

        Call<Void> cache(@Path("seconds") int seconds);
    }
+
+    interface GraphQLSWApi {


Can you move this to a separate class, like a SimpleGraphQLService or something similar

vbuberen · 2020-01-16T08:40:13Z

@rnitame
Could you give some timeline on when you plan to finish this PR?

I would love to proceed with 3.1.0 release in some nearest time, so if it would take too much time we might need to postpone it to 3.2.0.

Agree @cortinico @olivierperez ?

cortinico · 2020-01-16T16:51:33Z

I would love to proceed with 3.1.0 release in some nearest time, so if it would take too much time we might need to postpone it to 3.2.0.

Agree @cortinico @olivierperez ?

Definitely, let's move this to 3.2.0 👍

vbuberen · 2020-01-27T10:46:03Z

@rnitame @cortinico
Hey guys, could someone provide some info on the status of this feature and what is required to make it to production?
Is any help or discussion required?

vbuberen · 2020-01-27T10:49:22Z

Also, let's consider points from #116 here.
Probably, it is better to outline some plan with help of @psh to improve implementation as much as we can?

cortinico · 2020-01-27T19:47:04Z

Probably, it is better to outline some plan with help of @psh to improve implementation as much as we can?

Agree here. I generally like the approach in this PR by @rnitame. I believe we could reuse the UI part at least.

On the other hand I believe GraphQL initialisation should be provided by the developer when creating the ChuckerInterceptor, rather than having the library trying to infer if a request is actually a valid GraphQL request. So on that side we should probably change our approach.

doodla · 2020-01-25T01:06:35Z

        transaction.requestContentLength = requestBody?.contentLength() ?: 0L

-        val encodingIsSupported = io.bodyHasSupportedEncoding(request.headers().get("Content-Encoding"))
+        val encodingIsSupported =


This looks like it didn't change content. Can be reverted to reduce the diff.

doodla · 2020-01-25T01:06:57Z


        if (requestBody != null && encodingIsSupported) {
-            val source = io.getNativeSource(Buffer(), io.bodyIsGzipped(request.headers().get("Content-Encoding")))
+            val source = io.getNativeSource(


quentin41500 · 2020-03-04T22:23:58Z

This is such a nice feature! Anyway I can help getting that merge? I'm not the branch owner so I don't think I can push to it, but I could make a new branch with the desired changes if that's ok.

Let me know!

cortinico · 2020-03-04T22:38:37Z

This is such a nice feature! Anyway I can help getting that merge? I'm not the branch owner so I don't think I can push to it, but I could make a new branch with the desired changes if that's ok.

Let me know!

You can definitely send a new PR branching from this one :)

vbuberen · 2020-03-05T09:14:31Z

@quentin41500 That is actually cool. I believe that you could cooperate with @rnitame to do a new version of PR.

Also, I would like to join you in these efforts, since from what I see there were not much attention to this PR and it is quite old already, while the feature is really useful. So ping me here or in Chucker Slack channel and let's make it happen in 3.2.0 😄

cortinico · 2022-09-10T15:12:51Z

Closing this one as we have two other PRs achieving the same 👍
Let's look into merging those.

Thanks for your time @rnitame

Add view and parameter of HttpTransactionTuple for GraphQL

99fbef7

rnitame changed the title ~~Add support for GraphQL~~ wip: Add support for GraphQL Jul 25, 2019

rnitame added 3 commits July 25, 2019 23:03

Apply operationName to Chucker view

81996a0

Add GraphQL icon

84fd230

Add operationName to notificationText and fix ktlint

1762dbb

rnitame changed the title ~~wip: Add support for GraphQL~~ Add support for GraphQL Jul 27, 2019

cortinico requested changes Aug 2, 2019

View reviewed changes

rnitame added 4 commits August 3, 2019 12:57

Rename HttpRequestBody to GraphQLRequestBody

50c431a

Add space before operation name

eda5e2f

Use 'JsonConvertor.getInstance()' instead of 'Gson()'

88a9376

sample: Add retrofit-graphql

e4f0fa5

rnitame force-pushed the feature/graphql-support branch from 6f5f74e to e4f0fa5 Compare August 3, 2019 04:33

rnitame added 3 commits August 3, 2019 14:09

sample: Add necessary graphql files

56426a3

sample: Add logic for SWApi

4dde908

Fix logic of getting operation name

151c570

rnitame force-pushed the feature/graphql-support branch from a9e78bb to 151c570 Compare August 3, 2019 06:57

cortinico requested changes Aug 6, 2019

View reviewed changes

rnitame added 2 commits August 9, 2019 20:07

Fix conflict

4074e0f

sample: Delete .graphql files and library

9f62ced

cortinico added this to the 3.1.0 milestone Aug 11, 2019

rnitame added 3 commits August 29, 2019 21:22

Change request method for GraphQL

cba50f2

Modify 'findOperationName' function

347fa0c

Fix conflicts

f85fac9

rnitame force-pushed the feature/graphql-support branch from dc949d9 to f85fac9 Compare August 29, 2019 12:50

rnitame force-pushed the feature/graphql-support branch from ab298b1 to 5bbecf9 Compare September 11, 2019 11:39

Merge remote-tracking branch 'upstream/develop' into feature/graphql-…

b4814b8

…support

rnitame force-pushed the feature/graphql-support branch from 5bbecf9 to b4814b8 Compare September 11, 2019 11:52

psh mentioned this pull request Sep 15, 2019

Additional GraphQL Support #116

Closed

olivierperez requested changes Oct 2, 2019

View reviewed changes

cortinico requested changes Oct 24, 2019

View reviewed changes

cortinico modified the milestones: 3.1.0, 3.2.0 Jan 16, 2020

doodla reviewed Jan 27, 2020

View reviewed changes

vbuberen removed this from the 3.2.0 milestone Mar 30, 2020

alkber mentioned this pull request Apr 18, 2022

[feature/chucker-gql] GQL support against latest develop #800

Closed

Canato mentioned this pull request May 3, 2022

Improve transaction list for GraphQL requests #805

Merged

This was referenced Jul 7, 2022

Update Graphql transaction list ArjanSM/chucker#1

Closed

Update transaction list to differentiate GraphQL requests #848

Closed

cortinico closed this Sep 10, 2022


		<uses-permission android:name="android.permission.INTERNET" />

		<uses-sdk tools:overrideLibrary="io.github.wax911.library" />

Uh oh!

Conversation

rnitame commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cortinico left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnitame commented Aug 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cortinico commented Aug 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbuberen commented Jan 16, 2020

Uh oh!

cortinico commented Jan 16, 2020

Uh oh!

vbuberen commented Jan 27, 2020

Uh oh!

vbuberen commented Jan 27, 2020

Uh oh!

cortinico commented Jan 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quentin41500 commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cortinico commented Mar 4, 2020

Uh oh!

vbuberen commented Mar 5, 2020

Uh oh!

cortinico commented Sep 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rnitame commented Jul 25, 2019 •

edited

Loading

quentin41500 commented Mar 4, 2020 •

edited

Loading