Skip to content

Conversation

@cameocoder
Copy link

Ensure that null query parameters are not included when building the query parameter map in generated API clients.
Null query parameters were being generated, but then filtered out in buildUrl. This ensures that the query parameters will be non-null, however buildUrl signature has not changed as it is public and there my be usage outside the sdk.

New generated code will look like this when parameters are nullable:

		val queryParameters = buildMap<String, Any?>(1) {
			if (imageIndex != null) {
				put("imageIndex", imageIndex)
			}
		}

Excluding null query parameters like this matches the behaviour of the default openapi-generator kotlin output.

Instantiate KotlinLogging logger once per Client class

Added tests to cover scenarios where path parameters should be ignored or are empty.

Fixed HooksModule.kt typo in the README.md

…query parameter map in generated API clients.

Instantiate KotlinLogging logger once per class

Added tests to cover scenarios where path parameters should be ignored or are empty.

Fixed `HooksModule.kt` typo in the `README.md`
@nielsvanvelzen
Copy link
Member

What issue does this fix?

@cameocoder
Copy link
Author

What issue does this fix?

The tldr; is that it is a minor performance optimization part one of other changes I had in mind. It also aligns the jellyfin code with what would be produced from the existing openapi generator if this sdk was not overriding the generating process.

Screenshot 2025-06-19 at 1 58 10 PM

These changes came from an observation that buildUrl was a significant portion of CardPresenter.onBindViewHolder when I was profiling. I was expecting it to be getDrawable for the placeholder (which is the tall red spike), so I was surprised that buildUrl was consuming the most time since it should just be building a string.

These changes just focus on the queryParameter setup. A map of queryParameters was being built but it included mapping of parameters to null. Then in buildUrl they are filtered out:

queryParameters: Map<String, Any?> = mapOf(),

			queryParameters
				.filterNot { it.value == null }

This change ensures you are not passing in any null queryParameters in the first place. Ideally the map of query parameters would not be nullable, but I didn't think I could change that at this point, not knowing how callers outside the sdk may be using buildUrl (since it is public).

Also, I have some familiarity with the output from the kotlin openApi generator from previous projects. For example the openApi generator using kotlin and the multiplatform library produces code that looks like this:

    public open suspend fun updateItemUserData(itemId: kotlin.String, updateUserItemDataDto: UpdateUserItemDataDto, userId: kotlin.String? = null): HttpResponse<UserItemDataDto> {

        val localVariableAuthNames = listOf<String>("CustomAuthentication")

        val localVariableBody = updateUserItemDataDto

        val localVariableQuery = mutableMapOf<String, List<String>>()
        userId?.apply { localVariableQuery["userId"] = listOf("$userId") }
        val localVariableHeaders = mutableMapOf<String, String>()

        val localVariableConfig = RequestConfig<kotlin.Any?>(
            RequestMethod.POST,
            "/UserItems/{itemId}/UserData".replace("{" + "itemId" + "}", "$itemId"),
            query = localVariableQuery,
            headers = localVariableHeaders,
            requiresAuthentication = true,
        )

        return jsonRequest(
            localVariableConfig,
            localVariableBody,
            localVariableAuthNames
        ).wrap()
        

Again, it only adds the userId queryParameter if the parameter is non-null.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Oct 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflict Conflicts prevent merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants