[feature/chucker-gql] GQL support against latest develop by alkber · Pull Request #800 · ChuckerTeam/chucker

alkber · 2022-04-18T08:17:43Z

There was already an old implementation done here . This PR seems to be old one (2019) and way out of sync with develop branch. I have refactored it to meet the latest changes.

This almost getting ready and not the final PR yet, need to get valuable input on the code quality and optimization if required.

Sample Activity has also been provided for the demo

Known Issues
Couldn't successfully run the sample until i commented this part of the code

//        StrictMode.setVmPolicy(
//            StrictMode.VmPolicy.Builder()
//                .detectLeakedClosableObjects()
//                .penaltyLog()
//                .penaltyDeath()
//                .build()
//        )
//
//        StrictMode.setThreadPolicy(
//            StrictMode.ThreadPolicy.Builder()
//                .detectDiskReads()
//                .detectDiskWrites()
//                .penaltyLog()
//                .penaltyDeath()
//                .build()
//        )

MiSikora

For a PR that introduces integration with a new specification, there are no tests. Please cover necessary scenarios such as GET and POST requests with ChuckerInterceptor as both application and network interceptor.

It would be also helpful if you could follow our PR template. Screenshots especially would be appreciated.

MiSikora · 2022-04-18T10:11:59Z

                Status.Failed -> " ! ! !  $method $path"
                Status.Requested -> " . . .  $method $path"
-                else -> "$responseCode $method $path"
+                else -> "$responseCode $method $path ${isGraphqlRequest.takeIf { true } ?: ""}"


This is always true.

MiSikora · 2022-04-18T10:19:51Z

+                gqlVisibiltyGroup.visibility = if (transaction.isGqlRequest) {
+                    transaction.gqlOperationName?.let { opName ->
+                        setGqlInfo(
+                            operationName = opName,
+                            ProtocolResources.Gql()
+                        )
+                        View.VISIBLE
+                    } ?: View.GONE
+                } else View.GONE


For a piece of code that deals with visibility, It is very hard to understand what is going on here. Please refactor it.

MiSikora · 2022-04-18T10:29:44Z

+    val countriesQuery = "query countries {\n" +
+        "  countries {\n" +
+        "    name\n" +
+        "  }\n" +
+        "}\n"


Please use raw strings. Preferably trimMargin() variant.

MiSikora · 2022-04-18T10:31:48Z

+
+    internal interface GraphQLSWApi {
+        @POST("/")
+        @Headers("Content-Type: application/json")


Is it necessary? GsonConverterFactory should add this header on its own.

MiSikora · 2022-04-18T10:32:01Z

+    internal interface GraphQLSWApi {
+        @POST("/")
+        @Headers("Content-Type: application/json")
+        fun getCountries(@Body body: GraphQLResponseBody?): Call<Any?>


The body doesn't have to be nullable.

MiSikora · 2022-04-18T10:44:26Z

+        } else {
+            decodedContent?.let { content ->
+                transaction.gqlOperationName = inferGqlRequest(content)
+            }


I don't think it is correct to have it in the else clause here. It indicates that having the GraphQL operation name is somehow related to reaching the request threshold.

MiSikora · 2022-04-18T10:56:34Z

+    )
+
+    internal interface GraphQLSWApi {
+        @POST("/")


I would like to have an example of a GET request if possible.

MiSikora · 2022-04-18T11:05:41Z

+    client: OkHttpClient,
+) : HttpTask {
+
+    private val api = Retrofit.Builder()


I think that the vast majority of people will use GraphQL with something like Apollo. I would prefer to see a real-life scenario integration within our sample instead of Retrofit.

MiSikora · 2022-04-18T11:13:53Z

            }
        }.firstOrNull()
+
+    private fun inferGqlRequest(body: String): String? {


Inferring GraphQL from a body won't work for GET requests as there is only URL query parameter for those.

It's a bit hard to have a full-proof solution as anyone could have query parameters or bodies that conform to GraphQL format. I have an idea of how to deal with it but let's just first implement handling of GET requests as well.

MiSikora · 2022-04-18T11:20:30Z

+    val isGraphqlRequest: Boolean
+        get() = !gqlOperationName.isNullOrEmpty()


This doesn't seem right. The operationName is an optional parameter in GraphQL. I think we can rely solely on query. Plus there is an additional issue for GET requests, which I mentioned here.

MiSikora · 2022-04-18T11:34:09Z

Known Issues Couldn't successfully run the sample until i commented this part of the code

//        StrictMode.setVmPolicy(
//            StrictMode.VmPolicy.Builder()
//                .detectLeakedClosableObjects()
//                .penaltyLog()
//                .penaltyDeath()
//                .build()
//        )
//
//        StrictMode.setThreadPolicy(
//            StrictMode.ThreadPolicy.Builder()
//                .detectDiskReads()
//                .detectDiskWrites()
//                .penaltyLog()
//                .penaltyDeath()
//                .build()
//        )

I experienced it in the past as well but I thought that #750 fixed it.

I'm retracting my review because I don't know how GH behaves after I remove myself from maintainers, and I don't want to lock this PR

cortinico · 2022-05-02T20:24:46Z

Sorry for the late reply here.
I've noticed we do have another similar PR here: #805
I'd like to understand which of the two we prefer to move forward.

alkber · 2022-05-05T06:28:31Z

Please close this PR in respect of a better implementation at https://github.com/ChuckerTeam/chucker/pull/805

cortinico · 2022-05-06T15:39:21Z

Please close this PR in respect of a better implementation at https://github.com/ChuckerTeam/chucker/pull/805

👍 We'll keep the disucssion going on #805

alkber added 10 commits April 18, 2022 12:56

gql icon introduced

6b738cc

updated chucker_list_item_transaction.xml for gql adaptation

620c79c

gqlOperationName introduced to HttpTransaction

0e0989c

gqlOperationName introduced to HttpTransactionDao

99db273

gqlOperationName introduced to HttpTransactionTuple

ea91d47

introduced GqlRequestBody for gql parsing

4796661

RequestProcessor now infer if the request is a gql one

10d1c6e

GQL introduced to ProtocolResources

74536e5

TransactionAdapter now render GQL request if applicable

1b83100

GQL sample introduced for the demo purpose

4fc54ef

MiSikora previously requested changes Apr 18, 2022

View reviewed changes

cortinico mentioned this pull request May 2, 2022

Improve transaction list for GraphQL requests #805

Merged

cortinico closed this May 6, 2022

		val isGraphqlRequest: Boolean
		get() = !gqlOperationName.isNullOrEmpty()

Uh oh!

Conversation

alkber commented Apr 18, 2022

Uh oh!

MiSikora left a comment

Choose a reason for hiding this comment

Uh oh!

MiSikora Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

MiSikora Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

MiSikora Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

MiSikora Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiSikora Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

MiSikora Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

MiSikora Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

MiSikora Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiSikora Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

MiSikora Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiSikora commented Apr 18, 2022

Uh oh!

cortinico commented May 2, 2022

Uh oh!

alkber commented May 5, 2022

Uh oh!

cortinico commented May 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MiSikora Apr 18, 2022 •

edited

Loading

MiSikora Apr 18, 2022 •

edited

Loading

MiSikora Apr 18, 2022 •

edited

Loading