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

feature: Basic GraphQL support #328

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

pavelperc
Copy link
Contributor

@pavelperc pavelperc commented Nov 5, 2024

fixes #326

Hello. I added a basic GraphQL support for network plugin. It recognises POST api calls with "query" part in it and shows query name and type instead of api path in api calls list. Also, it recognises errors in api response and paints api calls in red, even if the status code is 200.

You may not like the way it is done. I thought about making a separate graphql interceptor with apollo graphql library (https://www.apollographql.com/docs/kotlin/advanced/interceptors-http), but graphql calls use http under the hood, so grapqhl calls would be duplicated in http interceptor. Also different clients use different versions of apollo library (2x, 3x, 4x), and it is a bad way to add a dependency only with one version of library. I thought it would be easier to parse raw request, rather then add an interceptor, but we can discuss and I can redo the implementation.

UPD. Image 2 (error handling) is not supported, see discussion

pluto1 pluto4 pluto5
pluto3 pluto6 pluto7

@pavelperc pavelperc changed the title feature: GraphQL support (issue #326) feature: Basic GraphQL support Nov 5, 2024
@srtvprateek
Copy link
Member

hey @pavelperc
thanks for the contribution

will check the changes & get back,
meanwhile can you please add the integration doc in the Network module README file.

also, can you check if the GraphQL support can be added as capability to Ktor interceptor as well.

@pavelperc
Copy link
Contributor Author

please add the integration doc in the Network module README file.

Hello. In the current implementation no extra integration is needed, because graphql is detected by request body.

also, can you check if the GraphQL support can be added as capability to Ktor interceptor as well.

All business logic is located in network-core module, so it should work with ktor too.

@@ -11,6 +11,7 @@ class NetworkData {
)

data class Response(
val request: Request,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an anti-pattern,
Request & Response are mutually exclusive objects.

can understand what you are trying to do here, but lets figure out a better way, than corrupting the Response class.

Copy link
Contributor Author

@pavelperc pavelperc Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, in both ktor and okhttp lib you can access request from response instances, so I thought this is okay.
I will just delete all the logic about parsing response.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes thats right,
we have ApiCallData for that purpose only.

internal val isGzipped: Boolean
get() = headers["Content-Encoding"].equals("gzip", ignoreCase = true)
}

data class Response(
val request: Request,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar concern

) {
private val contentTypeInternal: ContentType = ContentType.parse(contentType)
private val mediaType: String = contentTypeInternal.contentType
internal val mediaSubtype: String = contentTypeInternal.contentSubtype
internal val isBinary: Boolean = BINARY_MEDIA_TYPES.contains(mediaType)
val sizeInBytes: Long = body.length.toLong()
internal val mediaTypeFull: String = "$mediaType/$mediaSubtype"
val isLikelyJson get() = !isBinary && body.startsWith('{')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cant this be decided based on mediaType or mediaSubtype?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature[network-plugin]: GraphQL support
2 participants