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

Support built-in ID type serialization in GraphQLServerRequest #1952

Merged

Conversation

T45K
Copy link
Contributor

@T45K T45K commented Apr 13, 2024

📝 Description

Problem

after graphql-kotlin 7.1.0, the following GraphQLRequest our team uses started to fail.

GraphQLRequest(
    "query(${'$'}id: ID!) { ... }",
    variables = mapOf("id" to ID("1"))
)

this is because kotlinx.serialization is recently introduced instead of Jackson, but ID serialization is missing, so it is converted to JsonNull.

since ID is built-in scalar type, i think ID serialization should be supported.

Solution

add serialization logic to AnyNullableKSerializer

🔗 Related Issues

N/A

@samuelAndalon
Copy link
Contributor

Thank you for the pr, could you adjust the jacoco thresholds ?

Rule violated for bundle graphql-kotlin-server: branches covered ratio is 0.71, but expected minimum is 0.72

@T45K
Copy link
Contributor Author

T45K commented Apr 14, 2024

@samuelAndalon
thank you for review!

sure, i added a test that covers my change to GraphQLServerRequestTest

please check again 🙇

@T45K
Copy link
Contributor Author

T45K commented Apr 15, 2024

hmm, build-example still failed but i cannot find out the cause

Error: Plugin com.expediagroup:graphql-kotlin-maven-plugin:7.0.0-SNAPSHOT or one of its dependencies could not be resolved: The following artifacts could not be resolved: com.expediagroup:graphql-kotlin-maven-plugin:jar:7.0.0-SNAPSHOT (absent): Could not find artifact com.expediagroup:graphql-kotlin-maven-plugin:jar:7.0.0-SNAPSHOT -> [Help 1]

could you tell me what is wrong?

@dariuszkuc
Copy link
Collaborator

👋 re: example build failure -> that was my miss in the version bump. This should fix it -> #1953

@T45K
Copy link
Contributor Author

T45K commented Apr 15, 2024

@dariuszkuc
i see.
after PR you created is merged into main branch, i'll merge main branch again, and then CI failure will be fixed

@T45K
Copy link
Contributor Author

T45K commented Apr 16, 2024

updated
could you check again 🙏

@samuelAndalon samuelAndalon merged commit 51a48c3 into ExpediaGroup:master Apr 16, 2024
10 checks passed
samuelAndalon pushed a commit that referenced this pull request Apr 16, 2024
…1952)

### 📝 Description

#### Problem
after graphql-kotlin 7.1.0, the following `GraphQLRequest` our team uses
started to fail.

```kotlin
GraphQLRequest(
    "query(${'$'}id: ID!) { ... }",
    variables = mapOf("id" to ID("1"))
)
```

this is because `kotlinx.serialization` is recently introduced instead
of Jackson, but `ID` serialization is missing, so it is converted to
`JsonNull`.

since `ID` is built-in scalar type, i think `ID` serialization should be
supported.

#### Solution
add serialization logic to `AnyNullableKSerializer` 

### 🔗 Related Issues
N/A
samuelAndalon pushed a commit that referenced this pull request Apr 16, 2024
…1952)

### 📝 Description

#### Problem
after graphql-kotlin 7.1.0, the following `GraphQLRequest` our team uses
started to fail.

```kotlin
GraphQLRequest(
    "query(${'$'}id: ID!) { ... }",
    variables = mapOf("id" to ID("1"))
)
```

this is because `kotlinx.serialization` is recently introduced instead
of Jackson, but `ID` serialization is missing, so it is converted to
`JsonNull`.

since `ID` is built-in scalar type, i think `ID` serialization should be
supported.

#### Solution
add serialization logic to `AnyNullableKSerializer` 

### 🔗 Related Issues
N/A
samuelAndalon added a commit that referenced this pull request Apr 16, 2024
…1959)

Backport of #1952 to
v6

Co-authored-by: Tasuku Nakagawa <38446259+T45K@users.noreply.github.com>
samuelAndalon added a commit that referenced this pull request Apr 16, 2024
…1958)

Backport of #1952 to
v7

Co-authored-by: Tasuku Nakagawa <38446259+T45K@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants