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

Allow Custom Extensions in buildPostBody Method #5627

Closed
morux2 opened this issue Feb 19, 2024 · 6 comments
Closed

Allow Custom Extensions in buildPostBody Method #5627

morux2 opened this issue Feb 19, 2024 · 6 comments

Comments

@morux2
Copy link

morux2 commented Feb 19, 2024

Use case

We are trying to verify queries by passing signatures via extensions instead of using AutoPersistedQuery. As mentioned in #2922, we tried using the ByteStringHttpBody method. However, there is no way to serialize variables since FileUploadAwareJsonWriter is internal. We would like to call the original implementation as much as possible to keep up with future updates of apollo-kotlin.

スクリーンショット 2024-02-19 16 02 49

apollo-kotlin: 3.8.2

Describe the solution you'd like

add extensions argument to buildPostBody method.

スクリーンショット 2024-02-19 16 09 51
@martinbonnin
Copy link
Contributor

martinbonnin commented Feb 20, 2024

Makes a lot of sense 👍 Thanks for the feature request.

A version of buildPostBody that allows writing extensions is available here (will backport in v3 in a bit).

If you are not doing file uploads, you can use the following for the time being:

class WithExtensionsHttpRequestComposer(private val serverUrl: String) : HttpRequestComposer {
  override fun <D : Operation.Data> compose(apolloRequest: ApolloRequest<D>): HttpRequest {

    val request = HttpRequest.Builder(HttpMethod.Post, serverUrl)
        .body(
            ByteStringHttpBody(
                "application/json",
                buildJsonByteString {
                  writeObject {
                    name("query")
                    value(apolloRequest.operation.document())
                    name("operationName")
                    value(apolloRequest.operation.name())
                    name("variables")
                    writeObject {
                      apolloRequest.operation.serializeVariables(this, apolloRequest.executionContext[CustomScalarAdapters]!!, false)
                    }
                    name("extensions")
                    writeObject {
                      name("key")
                      value("value")
                    }
                  }
                }
            )
        )
        .build()

    return request
  }
}

This is all using public API so it's not going anywhere any time soon. If you're using file uploads on the other hand, I'm afraid you'll have to copy/paste a lot of FileUploadAwareJsonWriter until the fix gets released.

@morux2
Copy link
Author

morux2 commented Feb 21, 2024

Thanks for responding to our feature request and providing a temporary solution!
#5631 looks good to me.


We are not doing the file uploads, therefore temporary solutions looks good.
It seems that the serializeVariables function does not have a third argument.

val operation = apolloRequest.operation
val customScalarAdapters = apolloRequest.executionContext[CustomScalarAdapters] ?: CustomScalarAdapters.Empty
name("variables")
writeObject {
    operation.serializeVariables(this, customScalarAdapters)
}

@morux2
Copy link
Author

morux2 commented Feb 21, 2024

This is another feature request.

Currently, we are copying DefaultHttpRequestComposer's requestHeaders implementations.
Would it be possible to add a buildRequestHeaders() method to DefaultHttpRequestComposer for simplify custom composer implementaion?

class CustomHttpRequestComposer(
    private val serverUrl: String,
) : HttpRequestComposer {

  override fun <D : Operation.Data> compose(apolloRequest: ApolloRequest<D>): HttpRequest {
    val operation = apolloRequest.operation
    val requestHeaders = mutableListOf<HttpHeader>().apply {
        add(HttpHeader(DefaultHttpRequestComposer.HEADER_APOLLO_OPERATION_ID, operation.id()))
        add(HttpHeader(DefaultHttpRequestComposer.HEADER_APOLLO_OPERATION_NAME, operation.name()))
      if (apolloRequest.operation is Subscription<*>) {
        add(HttpHeader(HEADER_ACCEPT_NAME, HEADER_ACCEPT_VALUE_MULTIPART))
      } else {
        add(HttpHeader(HEADER_ACCEPT_NAME, HEADER_ACCEPT_VALUE_DEFER))
      }
      if (apolloRequest.httpHeaders != null) {
        addAll(apolloRequest.httpHeaders)
      }
    }
      return HttpRequest.Builder(
            method = HttpMethod.Post,
            url = serverUrl,
        ).addHeaders(requestHeaders)
            .body(
                DefaultHttpRequestComposer.buildPostBody(
                    operation,
                    customScalarAdapters,
                    query,
                ) {
                    name("extensions")
                    writeObject {
                        name("key")
                        value("value")
                    }
                }
            )
            .build()

    companion object {
        private const val HEADER_ACCEPT_NAME = "Accept"
        private const val HEADER_ACCEPT_VALUE_DEFER = "application/json"
        private const val HEADER_ACCEPT_VALUE_MULTIPART = "multipart/mixed; boundary=\"graphql\"; subscriptionSpec=1.0, application/json"
    }
}

@martinbonnin
Copy link
Contributor

Would it be possible to add a buildRequestHeaders() method to DefaultHttpRequestComposer for simplify custom composer implementaion?

Woops, sorry that fell through!

We could do something like so:

/*
 * Adds the default headers to this HttpRequest.Builder:
 * - Accept
 * - X-APOLLO-OPERATION-ID
 * - X-APOLLO-OPERATION-NAME
 */
fun HttpRequest.Builder.addDefaultHttpHeaders(): HttpRequest.Builder {}

That you could use like so:

      return HttpRequest.Builder(
            method = HttpMethod.Post,
            url = serverUrl,
        ).body(
                DefaultHttpRequestComposer.buildPostBody(
                    operation,
                    customScalarAdapters,
                    query,
                ) {
                    name("extensions")
                    writeObject {
                        name("key")
                        value("value")
                    }
                }
            )
            .addDefaultHttpHeaders()
            .build()

Heads up we are also changing the HTTP headers sent in v4. See #5533

I'm curious if you need those header specifically? Would anything break if we remove X-APOLLO-OPERATION-ID for an example?

@morux2
Copy link
Author

morux2 commented Feb 26, 2024

@martinbonnin

Sorry for the delayed response. Thank you for informing us about the latest changes to Apollo!
We've confirmed that there is no issue with the changing of HTTP headers.
And so, we decided to stop sending Headers to follow the update to Apollo.

It's okay to close this issue. Thank you for your warm support and response to the request 😄

@martinbonnin
Copy link
Contributor

Thanks for the follow up!

#5631 is now available in the SNAPSHOTs and will be in next 3.8 and 4.0-beta versions. Thanks again for surfacing this!

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

No branches or pull requests

2 participants