Skip to content

Conversation

@watbe
Copy link
Contributor

@watbe watbe commented Nov 17, 2025

Hi, thanks for your work on this library.

I've been trying to generate a nice Ktor client for the TVDB spec and your generator is one of the easier ones to understand. The TVDB swagger file is here: https://thetvdb.github.io/v4-api/swagger.yml

However, I ran into two issues:

  1. The bearer auth method didn't work because the subclasses of Http() were not @Serializable
  2. Kotlinpoet doesn't sanitize Kdoc strings, leading to description fields that contained */ to generate invalid Kotlin files.
  3. The existing main branch didn't compile due to some weird import issues.

I've addressed both issues in this PR:

  1. I've added @Serializable to the necessary subclasses for Http()
  2. I've introduced an extension function fun String?.sanitizeForKdoc(): String? which simply escapes any instances of */ in a openapi description.
  3. I've removed some unused imports that caused test compilation failures
  4. I've added a test by simply making sure the swagger.yml provided by TVDB compiles.

I'll admit the solution for the KDoc sanitization feels a bit "icky" but I wasn't sure how else to do this. Open to suggestions.

Additionally, I've made a few more changes (see comments in this PR):

  1. Removed duplicate sourceset (commonTest already includes commonMain sources)
  2. Added KMP task dependencies to avoid implicit task dependency warnings
  3. Use kotlin.Exception as the exception superclass instead of java.lang.Exception

Thanks again for this library!

@watbe
Copy link
Contributor Author

watbe commented Nov 17, 2025

I noticed a warning in IntelliJ - the plugin added the same sourceset to both commonMain and commonTest - this isn't necessary because everything in commonMain is already available in commonTest. The latest commit 2381681 removes the commonTest sourceset

@watbe
Copy link
Contributor Author

watbe commented Nov 18, 2025

Added a few more changes:

  1. Added task dependencies for Kotlin Multiplatform (I was getting an implicit task dependency warning in Gradle)
  2. Force usage of kotlin.Exception as java.lang.Exception doesn't compile under commonMain.

@dshatz
Copy link
Owner

dshatz commented Nov 23, 2025

Hi!

Thank you very much for trying it out and submitting improvements! Glad you liked.

I noticed that for TVDB the call parameters are not generated. I am not sure if it's your changes (all other tests pass) or there is an issue somewhere deeper.

For example TVDB postLogin:

public suspend fun postLogin(): HttpResult<PostLoginResponse, PostLoginResponse401> {
    try {
      val response = httpClient.post("login") {
        (authSchemes["bearerAuth"] as AuthScheme.WithBearer)?.bearer?.let { bearerAuth(it) }}
      val result = when (response.status.value) {
        200 -> response.body<PostLoginResponse>()
        else -> throw UnknownSuccessCodeError(response.body(), response.status.value, response)
      }
      return HttpResult.Ok(result, response)
    }
    catch (e: ClientRequestException) {
      val responseData: PostLoginResponse401 = when(e.response.status.value) {
        401 -> e.response.body<PostLoginResponse401>()
        else -> throw e
      }
      return HttpResult.Failure(responseData, e.response, e)
    }
  }

The yaml does have the body described like this:

  /login:
  post:
    summary: create an auth token. The token has one month validation length.
    requestBody:
      content:
        application/json:
          schema:
            type: object
            required:
              - apikey
            properties:
              apikey:
                type: string
              pin:
                type: string
      required: true

Can you check please?

@dshatz dshatz force-pushed the fix/httpBearerAuth branch from 2465f5f to 2e878f8 Compare December 15, 2025 18:04
@dshatz dshatz force-pushed the fix/httpBearerAuth branch from 2e878f8 to 6fd7642 Compare December 15, 2025 18:11
@dshatz dshatz added this to the 1.0.7 milestone Dec 15, 2025
@dshatz
Copy link
Owner

dshatz commented Dec 15, 2025

@watbe The issue I mentioned existed before this PR, sorry. I have fixed it and pushed the changes together with the ones from you.

It will be available in 1.0.7.

Thank you again for help!

@dshatz dshatz force-pushed the fix/httpBearerAuth branch from adeed39 to a1cfd9c Compare December 15, 2025 18:47
@dshatz dshatz force-pushed the fix/httpBearerAuth branch from a1cfd9c to 0597660 Compare December 15, 2025 18:49
@dshatz dshatz merged commit 6ab641c into dshatz:main Dec 15, 2025
1 of 2 checks passed
@watbe
Copy link
Contributor Author

watbe commented Dec 16, 2025

thanks for the fixes @dshatz! I meant to follow up but it dropped off my radar.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants