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: Allow specifying custom scalars when generating queries using Kotlin query projections #1860

Open
dmoidl opened this issue Mar 27, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@dmoidl
Copy link

dmoidl commented Mar 27, 2024

Describe the Feature Request

When using the new Kotlin Query projections, it is currently impossible to use custom scalar configuration when constructing the query.

Using custom scalars is available when using the GraphQLQueryRequest API, so this looks like a missing feature in the new API.

I understand that the new Kotlin codegen is still experimental, but having checked the code, I found there is currently no way to possibly achieve this, hence this feature request.

Describe Preferred Solution

It would be ideal if we could provide custom scalar implementations to the new query projections in a similar fashion it is currently done for GraphQLQueryRequest.

Describe Alternatives

I can imagine being able to register custom scalar implementations in a different way, possibly on the client level or such. But the only other alternative seems to be the current scenario where this is not possible at all.

@dmoidl dmoidl added the enhancement New feature or request label Mar 27, 2024
@srinivasankavitha
Copy link
Contributor

cc @mbossenbroek

@mbossenbroek
Copy link

That should be supported, unless you're thinking of something different than what we have...

Check out this integration test example:
https://github.com/Netflix/dgs-codegen/tree/master/graphql-dgs-codegen-core/src/integTest/kotlin/com/netflix/graphql/dgs/codegen/cases/dataClassWithMappedTypes

There are a couple of custom scalars in there, mapped with the same configuration as the normal codegen stuff. This is where it gets wired up for these tests:

https://github.com/Netflix/dgs-codegen/blob/master/graphql-dgs-codegen-core/src/integTest/kotlin/com/netflix/graphql/dgs/codegen/Kotlin2CodeGenTest.kt#L52-L58

In gradle, the same config would look something like this:

typeMapping = [
  "Long" : "kotlin.Long",
  "DateTime" : "java.time.OffsetDateTime",
  "PageInfo" : "graphql.relay.PageInfo",
  "EntityConnection" : "graphql.relay.SimpleListConnection<com.netflix.graphql.dgs.codegen.cases.dataClassWithMappedTypes.expected.types.EntityEdge>"
]

This was the type mapping setup that the codegen stuff was using when I made this kotlin version. It looks like that there's a new way of passing this mapping in code that we don't support in the new DSL. Let me know if the configuration mapping won't work for you & I can take another look.

@dmoidl
Copy link
Author

dmoidl commented Mar 28, 2024

Hi @mbossenbroek,

Thanks for a prompt response. It seems I didn't make myself clear enough, sorry.

The problem I'm experiencing is not related to the code generation itself, that actually works fine. But it is related to the type mapping. Let me explain.

Context

We have the codegen configured with type mapping like so:

    typeMapping =
        mutableMapOf(
            "ISO8601Date" to "java.time.LocalDate",
            "UUID" to "java.util.UUID",
            "ID" to "com.productboard.common.graphql.id.GraphqlId"
        )

where the class we map the ID to looks like this:

data class GraphqlId(val type: String, val id: String)

This correctly generates Kotlin classes where the GraphqlId is used for roperties representing GQL ID fields.

We then use something like this

@DgsScalar(name = "ID")
class GraphQlIdScalar : Coercing<GraphqlId, String> {
    // implementation here
}

to plug the custom type into the framework. So far so good.

The problem

However, when I try actually generating a query using a projection with input arguments containing this custom type, it doesn't validate.

Each GraphqlId value gets incorrectly serialized into the query as

{id: 'someId', type: 'someType'}

This is apparently because the projection doesn't know it should serialize this type using the corresponding Coercing and so it uses the default property-based serialization.

While this seems to be supported in the "old" API via the option to pass explicit scalars into a GraphQLQueryRequest, there's no such possibility when using DgsClient.build*.

Having checked the code, it looks like when generating the query string, the program reaches this point where it tries to serialize the value. However, the inputSerializer is instantiated like this, i.e., with no arguments.

This is different from the aforementioned GraphQLQueryRequest usage where it takes the provided scalars configuration (if any) into account, see here.

Note that in my case, this manifests in tests where I'm using the projection of a query with input arguments to generate a query string which then gets executed.


Hope this make it clearer. I see how my original post could have easily been misunderstood, sorry about that 🙂

@dmoidl dmoidl changed the title feature: Allow specifying custom scalars in Kotlin query projections feature: Allow specifying custom scalars when generating queries using Kotlin query projections Mar 28, 2024
@mbossenbroek
Copy link

Gotcha - that makes more sense. I'll take a deeper look at this, but it should be doable. That said, I might not get to it for a few weeks, and as always - feel free to drop a PR with the change :)

@dmoidl
Copy link
Author

dmoidl commented Mar 29, 2024

I'll do my best to have a look 🙃 I just think it won't be exactly trivial to implement for someone with zero knowledge of the codegen internals such as myself.

I suspect this will require changes to the code generation itself to make the option available on the DgsClient which is generated, unlike the GraphQLQueryRequest. Correct? 🤔

@mbossenbroek
Copy link

Yeah, that's totally fair. If you're really interested and/or need it sooner, I'd be happy to jump on a VC & walk you through it, but also totally understand if not & just want to wait a couple weeks

@dmoidl
Copy link
Author

dmoidl commented Apr 1, 2024

This is by no means a blocker for us, so I'm more than happy to wait 🙃 Thanks for being so helpful though 👍

@mbossenbroek
Copy link

Hey @dmoidl , I was just about to do this today & somebody else dropped this PR, which I believe will satisfy your need, right? Lmk if you think this will work or not... Netflix/dgs-codegen#675

@mbossenbroek
Copy link

This is released now

@dmoidl
Copy link
Author

dmoidl commented May 1, 2024

Thank @mbossenbroek for letting me know 🙂 I think this will do just fine. However, I believe it's not yet released, is it? 🤔 The latest available version seems to be 6.1.10 and I can't see the new InputValueSerializerProvider in there.

No worries though, I'll wait until it makes it to a released version.

@mbossenbroek
Copy link

Ahh apologies - I changed the original PR a bit in this PR: https://github.com/Netflix/dgs-codegen/pull/677/files#diff-39435f9dc1a43ff8f5570174fdbd1d26970cc2fe5b3b94c22f43e06f403bd8b0

The old way used a thread local to pass in a custom serializer; the new way generates the projections such that they take an optional parameter that's an InputValueSerializerInterface. I was trying to dig up the test I wrote for this as an example, but now I can't find it 🤔 . Maybe it got lost in a merge somewhere?

In any case, lmk if that doesn't make sense & I'll reinstate an example/test for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants